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

[Brett Grandbois]

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