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-pet...@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
