diff mbox series

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

Message ID 1524010844-8748-2-git-send-email-brett.grandbois@opengear.com
State Superseded
Headers show
Series some syslinux parser updates | expand

Commit Message

Grandbois, Brett April 18, 2018, 12:20 a.m. UTC 
in finalize loop or we can get duplicate boot entries as well as the
memory leak
---
 discover/syslinux-parser.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Sam Mendoza-Jonas April 18, 2018, 5:01 a.m. UTC | #1 
On Wed, 2018-04-18 at 10:20 +1000, Brett Grandbois wrote:
> in finalize loop or we can get duplicate boot entries as well as the
> memory leak
> ---
>  discover/syslinux-parser.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

Looks good but your Signed-off-by fell off in v2.

> 
> diff --git a/discover/syslinux-parser.c b/discover/syslinux-parser.c
> index d948765..50e798a 100644
> --- a/discover/syslinux-parser.c
> +++ b/discover/syslinux-parser.c
> @@ -285,7 +285,7 @@ static void syslinux_process_pair(struct conf_context *conf, const char *name, c
>  static void syslinux_finalize(struct conf_context *conf)
>  {
>  	struct syslinux_options *state = conf->parser_info;
> -	struct syslinux_boot_option *syslinux_opt;
> +	struct syslinux_boot_option *syslinux_opt, *tmp;
>  	struct discover_context *dc = conf->dc;
>  	struct discover_boot_option *d_opt;
>  	bool implicit_image = true;
> @@ -309,7 +309,7 @@ static void syslinux_finalize(struct conf_context *conf)
>  	if (conf_get_global_option(conf, "implicit"), "0")
>  		implicit_image = false;
>  
> -	list_for_each_entry(&state->processed_options, syslinux_opt, list) {
> +	list_for_each_entry_safe(&state->processed_options, syslinux_opt, tmp, list) {
>  		/* need a valid image */
>  		if (!syslinux_opt->image)
>  			continue;
> @@ -404,9 +404,14 @@ static void syslinux_finalize(struct conf_context *conf)
>  
>  		discover_context_add_boot_option(dc, d_opt);
>  		continue;
> +
>  fail:
>  		talloc_free(d_opt);
>  	}
> +
> +	list_for_each_entry_safe(&state->processed_options, syslinux_opt, tmp, list)
> +		talloc_free(syslinux_opt);
> +	list_init(&state->processed_options);
>  }
>  
>  static int syslinux_parse(struct discover_context *dc)
Grandbois, Brett April 18, 2018, 5:42 a.m. UTC | #2 
On 18/04/18 15:01, Samuel Mendoza-Jonas wrote:
> On Wed, 2018-04-18 at 10:20 +1000, Brett Grandbois wrote:
>> in finalize loop or we can get duplicate boot entries as well as the
>> memory leak
>> ---
>>   discover/syslinux-parser.c | 9 +++++++--
>>   1 file changed, 7 insertions(+), 2 deletions(-)
> Looks good but your Signed-off-by fell off in v2.
Do you want me to re-post with the signed off?  If so I can put back the 
non-_safe list loops where they're not needed since I understand now 
what the difference is.
>> diff --git a/discover/syslinux-parser.c b/discover/syslinux-parser.c
>> index d948765..50e798a 100644
>> --- a/discover/syslinux-parser.c
>> +++ b/discover/syslinux-parser.c
>> @@ -285,7 +285,7 @@ static void syslinux_process_pair(struct conf_context *conf, const char *name, c
>>   static void syslinux_finalize(struct conf_context *conf)
>>   {
>>   	struct syslinux_options *state = conf->parser_info;
>> -	struct syslinux_boot_option *syslinux_opt;
>> +	struct syslinux_boot_option *syslinux_opt, *tmp;
>>   	struct discover_context *dc = conf->dc;
>>   	struct discover_boot_option *d_opt;
>>   	bool implicit_image = true;
>> @@ -309,7 +309,7 @@ static void syslinux_finalize(struct conf_context *conf)
>>   	if (conf_get_global_option(conf, "implicit"), "0")
>>   		implicit_image = false;
>>   
>> -	list_for_each_entry(&state->processed_options, syslinux_opt, list) {
>> +	list_for_each_entry_safe(&state->processed_options, syslinux_opt, tmp, list) {
>>   		/* need a valid image */
>>   		if (!syslinux_opt->image)
>>   			continue;
>> @@ -404,9 +404,14 @@ static void syslinux_finalize(struct conf_context *conf)
>>   
>>   		discover_context_add_boot_option(dc, d_opt);
>>   		continue;
>> +
>>   fail:
>>   		talloc_free(d_opt);
>>   	}
>> +
>> +	list_for_each_entry_safe(&state->processed_options, syslinux_opt, tmp, list)
>> +		talloc_free(syslinux_opt);
>> +	list_init(&state->processed_options);
>>   }
>>   
>>   static int syslinux_parse(struct discover_context *dc)
Sam Mendoza-Jonas April 18, 2018, 5:44 a.m. UTC | #3 
On Wed, 2018-04-18 at 15:42 +1000, Brett Grandbois wrote:
> 
> On 18/04/18 15:01, Samuel Mendoza-Jonas wrote:
> > On Wed, 2018-04-18 at 10:20 +1000, Brett Grandbois wrote:
> > > in finalize loop or we can get duplicate boot entries as well as the
> > > memory leak
> > > ---
> > >   discover/syslinux-parser.c | 9 +++++++--
> > >   1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > Looks good but your Signed-off-by fell off in v2.
> 
> Do you want me to re-post with the signed off?  If so I can put back the 
> non-_safe list loops where they're not needed since I understand now 
> what the difference is.

Yes please :)

> > > diff --git a/discover/syslinux-parser.c b/discover/syslinux-parser.c
> > > index d948765..50e798a 100644
> > > --- a/discover/syslinux-parser.c
> > > +++ b/discover/syslinux-parser.c
> > > @@ -285,7 +285,7 @@ static void syslinux_process_pair(struct conf_context *conf, const char *name, c
> > >   static void syslinux_finalize(struct conf_context *conf)
> > >   {
> > >   	struct syslinux_options *state = conf->parser_info;
> > > -	struct syslinux_boot_option *syslinux_opt;
> > > +	struct syslinux_boot_option *syslinux_opt, *tmp;
> > >   	struct discover_context *dc = conf->dc;
> > >   	struct discover_boot_option *d_opt;
> > >   	bool implicit_image = true;
> > > @@ -309,7 +309,7 @@ static void syslinux_finalize(struct conf_context *conf)
> > >   	if (conf_get_global_option(conf, "implicit"), "0")
> > >   		implicit_image = false;
> > >   
> > > -	list_for_each_entry(&state->processed_options, syslinux_opt, list) {
> > > +	list_for_each_entry_safe(&state->processed_options, syslinux_opt, tmp, list) {
> > >   		/* need a valid image */
> > >   		if (!syslinux_opt->image)
> > >   			continue;
> > > @@ -404,9 +404,14 @@ static void syslinux_finalize(struct conf_context *conf)
> > >   
> > >   		discover_context_add_boot_option(dc, d_opt);
> > >   		continue;
> > > +
> > >   fail:
> > >   		talloc_free(d_opt);
> > >   	}
> > > +
> > > +	list_for_each_entry_safe(&state->processed_options, syslinux_opt, tmp, list)
> > > +		talloc_free(syslinux_opt);
> > > +	list_init(&state->processed_options);
> > >   }
> > >   
> > >   static int syslinux_parse(struct discover_context *dc)
> 
>
diff mbox series

Patch

diff --git a/discover/syslinux-parser.c b/discover/syslinux-parser.c
index d948765..50e798a 100644
--- a/discover/syslinux-parser.c
+++ b/discover/syslinux-parser.c
@@ -285,7 +285,7 @@  static void syslinux_process_pair(struct conf_context *conf, const char *name, c
 static void syslinux_finalize(struct conf_context *conf)
 {
 	struct syslinux_options *state = conf->parser_info;
-	struct syslinux_boot_option *syslinux_opt;
+	struct syslinux_boot_option *syslinux_opt, *tmp;
 	struct discover_context *dc = conf->dc;
 	struct discover_boot_option *d_opt;
 	bool implicit_image = true;
@@ -309,7 +309,7 @@  static void syslinux_finalize(struct conf_context *conf)
 	if (conf_get_global_option(conf, "implicit"), "0")
 		implicit_image = false;
 
-	list_for_each_entry(&state->processed_options, syslinux_opt, list) {
+	list_for_each_entry_safe(&state->processed_options, syslinux_opt, tmp, list) {
 		/* need a valid image */
 		if (!syslinux_opt->image)
 			continue;
@@ -404,9 +404,14 @@  static void syslinux_finalize(struct conf_context *conf)
 
 		discover_context_add_boot_option(dc, d_opt);
 		continue;
+
 fail:
 		talloc_free(d_opt);
 	}
+
+	list_for_each_entry_safe(&state->processed_options, syslinux_opt, tmp, list)
+		talloc_free(syslinux_opt);
+	list_init(&state->processed_options);
 }
 
 static int syslinux_parse(struct discover_context *dc)