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

[Peter Xu <peterx@redhat.com>]

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