On Tue, 16 Aug 2016 00:37:09 +0000
Atsushi Kumagai <ats-kumagai at wm.jp.nec.com> 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 <mwilck at suse.de>
> >> > ---
> >> > ?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
