* [PATCH] migration/cpr: Document obscure usage of g_autofree when parse str @ 2025-10-23 16:16 Peter Xu 2025-10-23 17:42 ` Dr. David Alan Gilbert 0 siblings, 1 reply; 4+ messages in thread From: Peter Xu @ 2025-10-23 16:16 UTC (permalink / raw) To: qemu-devel Cc: Fabiano Rosas, Juraj Marcin, peterx, Dr. David Alan Gilbert, Peter Maydell HMP parsing of cpr_exec_command contains an obscure usage of g_autofree. Provide a document for it to be clear that it's intentional, rather than memory leaked. Cc: Dr. David Alan Gilbert <dave@treblig.org> Reported-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/migration-hmp-cmds.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c index 847d18faaa..79426bf5d7 100644 --- a/migration/migration-hmp-cmds.c +++ b/migration/migration-hmp-cmds.c @@ -734,6 +734,12 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) visit_type_bool(v, param, &p->direct_io, &err); break; case MIGRATION_PARAMETER_CPR_EXEC_COMMAND: { + /* + * NOTE: g_autofree will only auto g_free() the strv array when + * needed, it will not free the strings within the array. It's + * intentional: when strv is set, the ownership of the strings will + * always be passed to p->cpr_exec_command via QAPI_LIST_APPEND(). + */ g_autofree char **strv = NULL; g_autoptr(GError) gerr = NULL; strList **tail = &p->cpr_exec_command; -- 2.50.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] migration/cpr: Document obscure usage of g_autofree when parse str 2025-10-23 16:16 [PATCH] migration/cpr: Document obscure usage of g_autofree when parse str Peter Xu @ 2025-10-23 17:42 ` Dr. David Alan Gilbert 2025-10-23 18:34 ` Peter Xu 0 siblings, 1 reply; 4+ messages in thread From: Dr. David Alan Gilbert @ 2025-10-23 17:42 UTC (permalink / raw) To: Peter Xu; +Cc: qemu-devel, Fabiano Rosas, Juraj Marcin, Peter Maydell * Peter Xu (peterx@redhat.com) wrote: > HMP parsing of cpr_exec_command contains an obscure usage of g_autofree. > Provide a document for it to be clear that it's intentional, rather than > memory leaked. > > Cc: Dr. David Alan Gilbert <dave@treblig.org> > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > migration/migration-hmp-cmds.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c > index 847d18faaa..79426bf5d7 100644 > --- a/migration/migration-hmp-cmds.c > +++ b/migration/migration-hmp-cmds.c > @@ -734,6 +734,12 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) > visit_type_bool(v, param, &p->direct_io, &err); > break; > case MIGRATION_PARAMETER_CPR_EXEC_COMMAND: { > + /* > + * NOTE: g_autofree will only auto g_free() the strv array when > + * needed, it will not free the strings within the array. It's > + * intentional: when strv is set, the ownership of the strings will > + * always be passed to p->cpr_exec_command via QAPI_LIST_APPEND(). > + */ Eww that's a bit weird isn't it. It's not clear to me if g_shell_parse_argv() might return an error part way through its parsing, and if it does whether there may be valid entries in strv which really do need freeing. https://docs.gtk.org/glib/func.shell_parse_argv.html doesn't seem to say. Dave > g_autofree char **strv = NULL; > g_autoptr(GError) gerr = NULL; > strList **tail = &p->cpr_exec_command; > -- > 2.50.1 > -- -----Open up your eyes, open up your mind, open up your code ------- / Dr. David Alan Gilbert | Running GNU/Linux | Happy \ \ dave @ treblig.org | | In Hex / \ _________________________|_____ http://www.treblig.org |_______/ ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] migration/cpr: Document obscure usage of g_autofree when parse str 2025-10-23 17:42 ` Dr. David Alan Gilbert @ 2025-10-23 18:34 ` Peter Xu 2025-10-23 20:07 ` Dr. David Alan Gilbert 0 siblings, 1 reply; 4+ messages in thread From: Peter Xu @ 2025-10-23 18:34 UTC (permalink / raw) To: Dr. David Alan Gilbert Cc: qemu-devel, Fabiano Rosas, Juraj Marcin, Peter Maydell On Thu, Oct 23, 2025 at 05:42:27PM +0000, Dr. David Alan Gilbert wrote: > * Peter Xu (peterx@redhat.com) wrote: > > HMP parsing of cpr_exec_command contains an obscure usage of g_autofree. > > Provide a document for it to be clear that it's intentional, rather than > > memory leaked. > > > > Cc: Dr. David Alan Gilbert <dave@treblig.org> > > Reported-by: Peter Maydell <peter.maydell@linaro.org> > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > migration/migration-hmp-cmds.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c > > index 847d18faaa..79426bf5d7 100644 > > --- a/migration/migration-hmp-cmds.c > > +++ b/migration/migration-hmp-cmds.c > > @@ -734,6 +734,12 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) > > visit_type_bool(v, param, &p->direct_io, &err); > > break; > > case MIGRATION_PARAMETER_CPR_EXEC_COMMAND: { > > + /* > > + * NOTE: g_autofree will only auto g_free() the strv array when > > + * needed, it will not free the strings within the array. It's > > + * intentional: when strv is set, the ownership of the strings will > > + * always be passed to p->cpr_exec_command via QAPI_LIST_APPEND(). > > + */ > > Eww that's a bit weird isn't it. > It's not clear to me if g_shell_parse_argv() might return an error part > way through its parsing, and if it does whether there may be valid entries in > strv which really do need freeing. > > https://docs.gtk.org/glib/func.shell_parse_argv.html doesn't seem to say. I checked glib code, it should be fine, the code looks like this since 2000: gboolean g_shell_parse_argv (const gchar *command_line, gint *argcp, gchar ***argvp, GError **error) { ... failed: g_assert (error == NULL || *error != NULL); g_strfreev (argv); <------------- g_slist_free_full (tokens, g_free); return FALSE; } The hope is with the comment, the current way is still the most efficient, avoiding strdup()s. Still, let me know if any of us still prefer changing the code instead of adding the comment.. The perf isn't a major issue, afaiu. But it's still good to consider that always, I believe that was when Steve developed this. Thanks! -- Peter Xu ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] migration/cpr: Document obscure usage of g_autofree when parse str 2025-10-23 18:34 ` Peter Xu @ 2025-10-23 20:07 ` Dr. David Alan Gilbert 0 siblings, 0 replies; 4+ messages in thread From: Dr. David Alan Gilbert @ 2025-10-23 20:07 UTC (permalink / raw) To: Peter Xu; +Cc: qemu-devel, Fabiano Rosas, Juraj Marcin, Peter Maydell * Peter Xu (peterx@redhat.com) wrote: > On Thu, Oct 23, 2025 at 05:42:27PM +0000, Dr. David Alan Gilbert wrote: > > * Peter Xu (peterx@redhat.com) wrote: > > > HMP parsing of cpr_exec_command contains an obscure usage of g_autofree. > > > Provide a document for it to be clear that it's intentional, rather than > > > memory leaked. > > > > > > Cc: Dr. David Alan Gilbert <dave@treblig.org> > > > Reported-by: Peter Maydell <peter.maydell@linaro.org> > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > > --- > > > migration/migration-hmp-cmds.c | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c > > > index 847d18faaa..79426bf5d7 100644 > > > --- a/migration/migration-hmp-cmds.c > > > +++ b/migration/migration-hmp-cmds.c > > > @@ -734,6 +734,12 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) > > > visit_type_bool(v, param, &p->direct_io, &err); > > > break; > > > case MIGRATION_PARAMETER_CPR_EXEC_COMMAND: { > > > + /* > > > + * NOTE: g_autofree will only auto g_free() the strv array when > > > + * needed, it will not free the strings within the array. It's > > > + * intentional: when strv is set, the ownership of the strings will > > > + * always be passed to p->cpr_exec_command via QAPI_LIST_APPEND(). > > > + */ > > > > Eww that's a bit weird isn't it. > > It's not clear to me if g_shell_parse_argv() might return an error part > > way through its parsing, and if it does whether there may be valid entries in > > strv which really do need freeing. > > > > https://docs.gtk.org/glib/func.shell_parse_argv.html doesn't seem to say. > > I checked glib code, it should be fine, the code looks like this since 2000: > > gboolean > g_shell_parse_argv (const gchar *command_line, > gint *argcp, > gchar ***argvp, > GError **error) > { > ... > failed: > > g_assert (error == NULL || *error != NULL); > g_strfreev (argv); <------------- > g_slist_free_full (tokens, g_free); > > return FALSE; > } OK, thanks for checking. > The hope is with the comment, the current way is still the most efficient, > avoiding strdup()s. Yeh, it's at least better with the comment! Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org> > Still, let me know if any of us still prefer changing the code instead of > adding the comment.. The perf isn't a major issue, afaiu. But it's still > good to consider that always, I believe that was when Steve developed this. > > Thanks! Thanks for adding it. Dave > -- > Peter Xu > -- -----Open up your eyes, open up your mind, open up your code ------- / Dr. David Alan Gilbert | Running GNU/Linux | Happy \ \ dave @ treblig.org | | In Hex / \ _________________________|_____ http://www.treblig.org |_______/ ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-10-23 20:08 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-10-23 16:16 [PATCH] migration/cpr: Document obscure usage of g_autofree when parse str Peter Xu 2025-10-23 17:42 ` Dr. David Alan Gilbert 2025-10-23 18:34 ` Peter Xu 2025-10-23 20:07 ` Dr. David Alan Gilbert
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).