Re: [PULL 40/45] migration: cpr-exec-command parameter

[Peter Xu <peterx@redhat.com>]

On Thu, Oct 23, 2025 at 04:41:35PM +0100, Peter Maydell wrote:
> On Fri, 3 Oct 2025 at 16:40, Peter Xu <peterx@redhat.com> wrote:
> >
> > From: Steve Sistare <steven.sistare@oracle.com>
> >
> > Create the cpr-exec-command migration parameter, defined as a list of
> > strings.  It will be used for cpr-exec migration mode in a subsequent
> > patch, and contains forward references to cpr-exec mode in the qapi
> > doc.
> >
> > No functional change, except that cpr-exec-command is shown by the
> > 'info migrate' command.
> 
> Hi; I was doing a 'git grep' for accidental misuses
> of g_autofree on a char** (which will free only the
> top level array, not the strings it points to), and
> the usage in this commit confused me for a bit:
> 
> > +    case MIGRATION_PARAMETER_CPR_EXEC_COMMAND: {
> > +        g_autofree char **strv = NULL;
> 
> g_shell_parse_argv() documents that we should free the
> vector with g_strfreev(), but g_autofree will only
> g_free() it. So this looks like a bug, but...
> 
> > +        g_autoptr(GError) gerr = NULL;
> > +        strList **tail = &p->cpr_exec_command;
> > +
> > +        if (!g_shell_parse_argv(valuestr, NULL, &strv, &gerr)) {
> > +            error_setg(&err, "%s", gerr->message);
> > +            break;
> > +        }
> > +        for (int i = 0; strv[i]; i++) {
> > +            QAPI_LIST_APPEND(tail, strv[i]);
> 
> ...we copy out the pointers to the individual
> strings here, and QAPI_LIST_APPEND() doesn't do a
> string copy, so it's correct that we don't g_strfreev()
> the char**.
> 
> Do you think it's worth a brief comment saying why
> g_autofree is correct here, or is it obvious enough
> without ?

It'll never hurt to have a comment indeed.  I'll send a patch, thanks for
raising this.

-- 
Peter Xu