| Message ID | 1523513574-16211-2-git-send-email-brett.grandbois@opengear.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | some syslinux parser updates | expand |
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? > }
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. > >> }
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
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); } }
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(-)