Peter Xu <pet...@redhat.com> writes:
> 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?
Since we're talking about "how to set it to NULL before", I was using
master.
> 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.
>> 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?
>> 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.
I forgot it the switch to alternate crashes on normal usage, not just
when you attempt to pass null.
> 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-pet...@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.
Okay.
