Re: [PATCH v8 08/12] migration/multifd: Add new migration option for multifd DSA offloading.

[Peter Xu]

On Mon, Dec 23, 2024 at 11:11:46PM -0800, Yichen Wang wrote:
> > @@ -563,6 +572,15 @@ void hmp_migrate_set_parameter(Monitor *mon, const 
> > QDict *qdict)
> >          p->has_x_checkpoint_delay = true;
> >          visit_type_uint32(v, param, &p->x_checkpoint_delay, &err);
> >          break;
> > +    case MIGRATION_PARAMETER_ACCEL_PATH:
> > +        p->has_accel_path = true;
> > +        char **strv = g_strsplit(valuestr ? : "", " ", -1);
> > +        strList **tail = &p->accel_path;
> > +        for (int i = 0; strv[i]; i++) {
> > +            QAPI_LIST_APPEND(tail, strv[i]);
> > +        }
> > +        g_strfreev(strv);
> > +        break;
> 
> I am doing my final testing, and seeing a new issue for above. This
> code doesn't really work, because strv is freed and all contents after
> the string split are gone. So here is what I am thinking:
> 
> 1. This is supposed to be an easy visit_type_strList(v, param,
> &p->accel_path, &err), but it actually doesn't work. The code will
> throw:
> qemu-system-x86_64.dsa: ../../../qapi/string-input-visitor.c:343:
> parse_type_str: Assertion `siv->lm == LM_NONE' failed.
> when you are doing "migrate_set_parameter accel-path
> dsa:/dev/dsa/wq0.1" from HMP.

IIUC that's for JSON only.

> 
> 2. If I remove the g_strfreev(strv), things are working perfectly. But
> I am worried about the memory leak here. As technically if you keep
> doing migrate_set_parameter for say 1 million times, memory will be
> exhausted.

Right, better not leak mem.  Can you dup the str when constructing the
artifact?  I mean something like:

-   QAPI_LIST_APPEND(tail, strv[i]);
+   QAPI_LIST_APPEND(tail, g_strdup(strv[i]));

Thanks,

-- 
Peter Xu