[PATCH 1/2] discover/syslinux-parser: clean up boot option list entries

[Samuel Mendoza-Jonas]

On Wed, 2018-04-18 at 08:50 +1000, Brett Grandbois wrote:
> 
> On 17/04/18 15:11, Samuel Mendoza-Jonas wrote:
> > On Thu, 2018-04-12 at 16:12 +1000, Brett Grandbois wrote:
> > > in finalize loop or we can get duplicate boot entries as well as
> > > the memory leak
> > > 
> > > Signed-off-by: Brett Grandbois <brett.grandbois at opengear.com>
> > > ---
> > >   discover/syslinux-parser.c | 10 +++++++---
> > >   1 file changed, 7 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/discover/syslinux-parser.c b/discover/syslinux-parser.c
> > > index d948765..8f5bfeb 100644
> > > --- a/discover/syslinux-parser.c
> > > +++ b/discover/syslinux-parser.c
> > > @@ -310,20 +310,21 @@ static void syslinux_finalize(struct conf_context *conf)
> > >   		implicit_image = false;
> > >   
> > >   	list_for_each_entry(&state->processed_options, syslinux_opt, list) {
> > > +		list_remove(&syslinux_opt->list);
> > 
> > Should be list_for_each_entry_safe() now.
> 
> Yep that's fine.  This is an overall policy?  Should all the calls be 
> list_for_each_entry be updated while I'm there?

Only if we're modifying the list while walking it but that looks like the
only case in syslinux-parser.

> > 
> > >   		/* need a valid image */
> > >   		if (!syslinux_opt->image)
> > > -			continue;
> > > +			goto next;
> > >   
> > >   		image = syslinux_opt->image;
> > >   		label = syslinux_opt->label;
> > >   
> > >   		/* if implicit is disabled we must have a label */
> > >   		if (!label && !implicit_image)
> > > -			continue;
> > > +			goto next;
> > >   
> > >   		d_opt = discover_boot_option_create(dc, dc->device);
> > >   		if (!d_opt)
> > > -			continue;
> > > +			goto next;
> > >   		if (!d_opt->option)
> > >   			goto fail;
> > >   
> > > @@ -403,8 +404,11 @@ static void syslinux_finalize(struct conf_context *conf)
> > >   		conf_strip_str(opt->description);
> > >   
> > >   		discover_context_add_boot_option(dc, d_opt);
> > > +next:
> > > +		talloc_free(syslinux_opt);
> > >   		continue;
> > >   fail:
> > > +		talloc_free(syslinux_opt);
> > >   		talloc_free(d_opt);
> > >   	}
> > 
> > I wonder if it might be simpler to after the loop do
> > 
> > 	list_for_each_entry_safe(&state->processed_options, syslinux_opt, list)
> > 		talloc_free(syslinux_opt);
> > 	list_init(&state->processed_options);
> > 
> > Then we just need talloc_free(d_opt) at the end of the first loop and we're
> > sure the list is empty before returning to syslinux_parse(). What do you think?
> 
> It's still the same logic, the difference being whether you walk through 
> the list once or twice.  The only real complexity you're saving is the 
> goto next which I admit I'm not thrilled about but is consistent with 
> the goto fail case flow.  Hmm, it does make the intent a lot more 
> explicit rather than spreading it across the top and bottom though.  Yep 
> I don't see any issues changing it.

Sounds like the same thought process I was going through :)

> > 
> > >   }
> 
> _______________________________________________
> Petitboot mailing list
> Petitboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/petitboot