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

[Peter Xu <peterx@redhat.com>]

Hi, Markus,

On Tue, Oct 10, 2023 at 09:18:23PM +0200, Markus Armbruster wrote:

[...]

> >> The point I was trying to make is this.  Before the patch, we reject
> >> attempts to set the property value to null.  Afterwards, we accept them,
> >> i.e. the patch loses "reject null property value".  If this loss is
> >> undesirable, we better replace it with suitable hand-written code.
> >
> > I don't even know how to set it to NULL before.. as it can only be accessed
> > via cmdline "-global" as mentioned above, which must be a string anyway.
> > So I assume this is not an issue.
> 
> 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?

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.

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

> In my "QAPI string visitors crashes" memo, I demonstrated that the crash
> on funny property type predates your series.  You merely add another
> instance.  Moreover, crashing -global is less serious than a crashing
> monitor command, because only the latter can take down a running guest.
> Can't see why your series needs to wait for a fix of the crash bug.
> Makes sense?

What's your suggestion to move on with this series without a fix for that
crash bug?

I started this series with making tls_* all strings (rather than StrOrNull)
and that actually worked out, mostly.  We switched to StrOrNull just
because we think it's cleaner and 100% not breaking anyone (even though I
still don't think the other way will).  I don't see how I can proceed this
series without fixing this visitor issue but keep using StrOrNull.

Please don't worry on blocking my work: it won't anymore.  The thing I need is:

https://lore.kernel.org/qemu-devel/20230905193802.250440-1-peterx@redhat.com/

While this whole series is just paving way for it.  If I can't get
immediate results out of this series, I'll just go with the triplications,
leaving all the rest for later.

Thanks,

-- 
Peter Xu