On Thu, Oct 23, 2025 at 04:41:35PM +0100, Peter Maydell wrote: > On Fri, 3 Oct 2025 at 16:40, Peter Xu <[email protected]> wrote: > > > > From: Steve Sistare <[email protected]> > > > > 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
