migration/cpr: Document obscure usage of g_autofree when parse str

[PATCH] migration/cpr: Document obscure usage of g_autofree when parse str

Posted by Peter Xu 3 days, 16 hours ago

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

Re: [PATCH] migration/cpr: Document obscure usage of g_autofree when parse str

Posted by Dr. David Alan Gilbert 3 days, 15 hours ago

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

Re: [PATCH] migration/cpr: Document obscure usage of g_autofree when parse str

Posted by Peter Xu 3 days, 14 hours ago

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

Re: [PATCH] migration/cpr: Document obscure usage of g_autofree when parse str

Posted by Dr. David Alan Gilbert 3 days, 12 hours ago

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