Re: [PATCH v3 3/4] migration/qapi: Replace @MigrateSetParameters with @MigrationParameters

[Peter Xu <peterx@redhat.com>]

On Wed, Oct 11, 2023 at 06:28:26AM +0200, Markus Armbruster wrote:
> >> Something like
> >> 
> >>     {"execute": "migrate-set-parameters",
> >>      "arguments": {"tls-authz": null}}
> >> 
> >> Hmm, crashes in migrate_params_apply(), which is a bug.  I'm getting
> >> more and more suspicious about user-facing migration code...
> >
> > Did you apply patch 1 of this series?
> 
> Since we're talking about "how to set it to NULL before", I was using
> master.

I see.  Then it seems we're good.  The fix just landed master branch
(86dec715a7).

> 
> > https://lore.kernel.org/qemu-devel/20230905162335.235619-2-peterx@redhat.com/
> >
> > QMP "migrate-set-parameters" does not go via migration_properties, so even
> > if we change handling of migration_properties, it shouldn't yet affect the
> > QMP interface of that.
> 
> I see.
> 
> I want to understand the impact of the change from 'str' to 'StrOrNull'
> on external interfaces.  The first step is to know where exactly the
> type is exposed externally.  *Know*, not gut-feel based on intended use.
> 
> I'll have another look at the schema change, and how the types are used.

Thanks.

> 
> >> If the migration object is accessible with qom-set, then that's another
> >> way to assign null values.
> >
> > I see what you meant.  IMHO we just don't need to worry on breaking that as
> > I am not aware of anyone using that to set migration parameters, and I
> > think the whole idea of migration_properties is for debugging.  The only
> > legal way an user should set migration parameters should be via QMP, afaik.
> 
> No matter the intended purpose of an interface, its meaning should be
> clear.  If we accept null, what does it mean?

IMHO here it means anything parsed from migration_properties will be a
qstring and impossible to be a qnull, at least if with that of my (probably
unmature, or even wrong..) fix:

https://lore.kernel.org/all/ZRsff7Lmy7TnggK9@x1n/

+static bool string_input_start_alternate(Visitor *v, const char *name,
+                                         GenericAlternate **obj, size_t size,
+                                         Error **errp)
+{
+    *obj = g_malloc0(size);
+    (*obj)->type = QTYPE_QSTRING;          <---------------- constantly set to string type
+    return true;
+}

I think it means when we parse the string, we'll always go with:

visit_type_StrOrNull():
    switch ((*obj)->type) {
    case QTYPE_QSTRING:
        ok = visit_type_str(v, name, &(*obj)->u.s, errp);
        break;

Finally, parse_type_str().

So it'll be impossible to specify null for -global migration.tls_*=XXX.

I suppose it's fine then?  Because it actually matches with previous when
it was still a string, and we use empty string to show tls disabled.

Thanks,

-- 
Peter Xu