Re: [PATCH 3/3] close_dump_bitmap: simplify logic
On Tue, 2016-08-16 at 07:59 +0200, Petr Tesarik wrote: > On Tue, 16 Aug 2016 00:37:09 + > Atsushi Kumagaiwrote: > > > 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
On Tue, 16 Aug 2016 00:37:09 + Atsushi Kumagaiwrote: > >> > 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
>> > 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
On Wed, 2016-08-10 at 15:08 +0200, Petr Tesarik wrote: > On Wed, 10 Aug 2016 14:56:58 +0200 > Martin Wilckwrote: > > > 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
On Wed, 10 Aug 2016 14:56:58 +0200 Martin Wilckwrote: > 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
[PATCH 3/3] close_dump_bitmap: simplify logic
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) return; if ((info->fd_bitmap = close(info->fd_bitmap)) < 0) -- 2.9.2 ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec