Re: [PATCH 3/3] close_dump_bitmap: simplify logic

[Martin Wilck]

On Tue, 2016-08-16 at 07:59 +0200, Petr Tesarik wrote:
> On Tue, 16 Aug 2016 00:37:09 +
> Atsushi Kumagai  wrote:
> 
> > So, could you work for that cleanup before your three patches as an
> > additional cleanup patch ?
> 
> OK, I take it. ;-)
> 
> Martin, do you mind rebasing your patch when I'm done with the
> cleanup?

No problem.

Regards
Martin



___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 3/3] close_dump_bitmap: simplify logic

[Petr Tesarik]

On Tue, 16 Aug 2016 00:37:09 +
Atsushi Kumagai  wrote:

> >> > The boolean expression replicates the logic of open_dump_bitmap().
> >> > It's simpler and less error-prone to simply check if fd_bitmap is
> >> > valid.
> >> >
> >> > Signed-off-by: Martin Wilck 
> >> > ---
> >> >  makedumpfile.c | 3 +--
> >> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >> >
> >> > diff --git a/makedumpfile.c b/makedumpfile.c
> >> > index 43278f1..771ab7c 100644
> >> > --- a/makedumpfile.c
> >> > +++ b/makedumpfile.c
> >> > @@ -8579,8 +8579,7 @@ close_dump_file(void)
> >> >  void
> >> >  close_dump_bitmap(void)
> >> >  {
> >> > -if (!info->working_dir && !info->flag_reassemble && !info-
> >> > >flag_refiltering
> >> > -&& !info->flag_sadump && !info->flag_mem_usage)
> >> > +if (!info->fd_bitmap)
> >>
> >> Strictly speaking, zero is a valid FD. I can see that it is highly
> >> unlikely to be the bitmap FD, but it would be a nice cleanup to
> >> initialize fd_bitmap to a negative number and check info->fd_bitmap <
> >> 0.
> >> I'm just not sure where to put the initializition...
> >
> >
> >> > OTOH I know I'm asking you to fix something that you didn't break.
> >
> >I had the same thought, and the same excuse not to address it in this
> >patch set. If you grep makedumpfile.c for "fd_bitmap", you'll see many
> >checks like "if (info->fd_bitmap)". I just followed that pattern for
> >now.
> 
> I see, it would be better to make the checks strict on this occasion.
> So, could you work for that cleanup before your three patches as an
> additional cleanup patch ?

OK, I take it. ;-)

Martin, do you mind rebasing your patch when I'm done with the cleanup?

Petr T

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

RE: [PATCH 3/3] close_dump_bitmap: simplify logic

[Atsushi Kumagai]

>> > The boolean expression replicates the logic of open_dump_bitmap().
>> > It's simpler and less error-prone to simply check if fd_bitmap is
>> > valid.
>> >
>> > Signed-off-by: Martin Wilck 
>> > ---
>> >  makedumpfile.c | 3 +--
>> >  1 file changed, 1 insertion(+), 2 deletions(-)
>> >
>> > diff --git a/makedumpfile.c b/makedumpfile.c
>> > index 43278f1..771ab7c 100644
>> > --- a/makedumpfile.c
>> > +++ b/makedumpfile.c
>> > @@ -8579,8 +8579,7 @@ close_dump_file(void)
>> >  void
>> >  close_dump_bitmap(void)
>> >  {
>> > -  if (!info->working_dir && !info->flag_reassemble && !info-
>> > >flag_refiltering
>> > -  && !info->flag_sadump && !info->flag_mem_usage)
>> > +  if (!info->fd_bitmap)
>>
>> Strictly speaking, zero is a valid FD. I can see that it is highly
>> unlikely to be the bitmap FD, but it would be a nice cleanup to
>> initialize fd_bitmap to a negative number and check info->fd_bitmap <
>> 0.
>> I'm just not sure where to put the initializition...
>
>
>> > OTOH I know I'm asking you to fix something that you didn't break.
>
>I had the same thought, and the same excuse not to address it in this
>patch set. If you grep makedumpfile.c for "fd_bitmap", you'll see many
>checks like "if (info->fd_bitmap)". I just followed that pattern for
>now.

I see, it would be better to make the checks strict on this occasion.
So, could you work for that cleanup before your three patches as an
additional cleanup patch ?


Thanks,
Atsushi Kumagai
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 3/3] close_dump_bitmap: simplify logic

[Martin Wilck]

On Wed, 2016-08-10 at 15:08 +0200, Petr Tesarik wrote:
> On Wed, 10 Aug 2016 14:56:58 +0200
> Martin Wilck  wrote:
> 
> > The boolean expression replicates the logic of open_dump_bitmap().
> > It's simpler and less error-prone to simply check if fd_bitmap is
> > valid.
> > 
> > Signed-off-by: Martin Wilck 
> > ---
> >  makedumpfile.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/makedumpfile.c b/makedumpfile.c
> > index 43278f1..771ab7c 100644
> > --- a/makedumpfile.c
> > +++ b/makedumpfile.c
> > @@ -8579,8 +8579,7 @@ close_dump_file(void)
> >  void
> >  close_dump_bitmap(void)
> >  {
> > -   if (!info->working_dir && !info->flag_reassemble && !info-
> > >flag_refiltering
> > -   && !info->flag_sadump && !info->flag_mem_usage)
> > +   if (!info->fd_bitmap)
> 
> Strictly speaking, zero is a valid FD. I can see that it is highly
> unlikely to be the bitmap FD, but it would be a nice cleanup to
> initialize fd_bitmap to a negative number and check info->fd_bitmap <
> 0.
> I'm just not sure where to put the initializition...


> > OTOH I know I'm asking you to fix something that you didn't break.

I had the same thought, and the same excuse not to address it in this
patch set. If you grep makedumpfile.c for "fd_bitmap", you'll see many
checks like "if (info->fd_bitmap)". I just followed that pattern for
now.

Martin

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 3/3] close_dump_bitmap: simplify logic

[Petr Tesarik]

On Wed, 10 Aug 2016 14:56:58 +0200
Martin Wilck  wrote:

> The boolean expression replicates the logic of open_dump_bitmap().
> It's simpler and less error-prone to simply check if fd_bitmap is
> valid.
> 
> Signed-off-by: Martin Wilck 
> ---
>  makedumpfile.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/makedumpfile.c b/makedumpfile.c
> index 43278f1..771ab7c 100644
> --- a/makedumpfile.c
> +++ b/makedumpfile.c
> @@ -8579,8 +8579,7 @@ close_dump_file(void)
>  void
>  close_dump_bitmap(void)
>  {
> - if (!info->working_dir && !info->flag_reassemble && 
> !info->flag_refiltering
> - && !info->flag_sadump && !info->flag_mem_usage)
> + if (!info->fd_bitmap)

Strictly speaking, zero is a valid FD. I can see that it is highly
unlikely to be the bitmap FD, but it would be a nice cleanup to
initialize fd_bitmap to a negative number and check info->fd_bitmap < 0.
I'm just not sure where to put the initializition...

OTOH I know I'm asking you to fix something that you didn't break.

Petr T

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec