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

["Dr. David Alan Gilbert" <dave@treblig.org>]

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