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

Message ID 1523513574-16211-2-git-send-email-brett.grandbois@opengear.com
State Superseded
Headers show
Series
  • some syslinux parser updates
Related show

Commit Message

Grandbois, Brett April 12, 2018, 6:12 a.m.
in finalize loop or we can get duplicate boot entries as well as
the memory leak

Signed-off-by: Brett Grandbois <brett.grandbois@opengear.com>
---
 discover/syslinux-parser.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Samuel Mendoza-Jonas April 17, 2018, 5:11 a.m. | #1
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@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.

>  		/* 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?

>  }
Grandbois, Brett April 17, 2018, 10:50 p.m. | #2
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@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?
>
>>   		/* 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.
>
>>   }
Samuel Mendoza-Jonas April 18, 2018, 1:08 a.m. | #3
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@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@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/petitboot

Patch

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);
 		/* 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);
 	}
 }