Re: [PATCH 4/4] qapi: introduce CONFIG_READ event

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> On 18.10.23 09:47, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>> 
>>> On 17.10.23 18:00, Markus Armbruster wrote:
>>>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>>>>
>>>>> Send a new event when guest reads virtio-pci config after
>>>>> virtio_notify_config() call.
>>>>>
>>>>> That's useful to check that guest fetched modified config, for example
>>>>> after resizing disk backend.
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> [...]
>> 
>>>>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>>>>> index 2468f8bddf..37a8785b81 100644
>>>>> --- a/qapi/qdev.json
>>>>> +++ b/qapi/qdev.json
>>>>> @@ -329,3 +329,25 @@
>>>>>    # Since: 8.2
>>>>>    ##
>>>>>    { 'command': 'x-device-sync-config', 'data': {'id': 'str'} }
>>>>> +
>>>>> +##
>>>>> +# @X_CONFIG_READ:
>>>>> +#
>>>>> +# Emitted whenever guest reads virtio device config after config change.
>>>>> +#
>>>>> +# @device: device name
>>>>> +#
>>>>> +# @path: device path
>>>>> +#
>>>>> +# Since: 5.0.1-24
>>>>> +#
>>>>> +# Example:
>>>>> +#
>>>>> +# <- { "event": "X_CONFIG_READ",
>>>>> +#      "data": { "device": "virtio-net-pci-0",
>>>>> +#                "path": "/machine/peripheral/virtio-net-pci-0" },
>>>>> +#      "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>>>>> +#
>>>>> +##
>>>>> +{ 'event': 'X_CONFIG_READ',
>>>>> +  'data': { '*device': 'str', 'path': 'str' } }
>>>>
>>>> The commit message talks about event CONFIG_READ, but you actually name
>>>> it x-device-sync-config.
>>>
>>> will fix
>>>
>>>> I figure you use x- to signify "unstable".  Please use feature flag
>>>> 'unstable' for that.  See docs/devel/qapi-code-gen.rst section
>>>> "Features", in particular "Special features", and also the note on x- in
>>>> section "Naming rules and reserved names".
>>>
>>> OK, will do.
>>>
>>> Hmm, it say
>>>
>>>     Names beginning with ``x-`` used to signify "experimental".  This
>>>     convention has been replaced by special feature "unstable".
>>>
>>> "replaced".. So, I should use "unstable" flag without "x-" prefix? Can't 
>>> find an example. Seems "unstable" always used together with "x-".
>>
>> True.
>>
>> The "x-" prefix originated with qdev properties.  First use might be
>> commit f0c07c7c7b4.  The convention wasn't documented then, but QOM/qdev
>> properties have always been a documentation wasteland.  It then spread
>> to other places, and eventually to the QAPI schema.  Where we try pretty
>> hard to document things properly.  We documented the "x-" prefix in
>> commit e790e666518:
>>
>>      Any name (command, event, type, field, or enum value) beginning with
>>      "x-" is marked experimental, and may be withdrawn or changed
>>      incompatibly in a future release.
>>
>> Minor pain point: when things grow up from experimental to stable, we
>> have to rename.
>>
>> The convention didn't stop us from naming non-experimental things x-FOO,
>> e.g. QOM property "x-origin" in commit 6105683da35.  Made it to the QAPI
>> schema in commit 8825587b53c.  Point is: the prefix isn't a reliable
>> marker for "unstable".
>>
>> Since I needed a reliable marker for my "set policy for unstable
>> interfaces" feature (see CLI option -compat), I created special feature
>> flag "unstable", and dropped the "x-" convention for the QAPI schema.
>>
>> Renaming existing "x-" names felt like pointless churn, so I didn't.
>>
>> I'm not objecting to new names starting with "x-".  Nor am I objecting
>> to feature 'unstable' on names that don't start with "x-".
>>
>> I guess "x-" remains just fine for things we don't intend to make stable
>> at some point.  The "x-" can remind humans "this is unstable" better
>> than a feature flag can (for machines, it's the other way round).
>>
>> For things we do intend (hope?) to make stable, I wouldn't bother with
>> the "x-".
>>
>> Clearer now?
>
> Yes, thanks.
>
> x- seems safer for management tool that doesn't know about "unstable" 
> properties..

Easy, traditional, and unreliable :)

> But on the other hand, changing from x- to no-prefix is already done when the 
> feature is stable, and thouse who use it already use the latest version of 
> interface, so, removing the prefix is just extra work.

Exactly.

> So, I think, I'd go without prefix.

Makes sense.

>>> Also, nothing said about events. Is using "X_" wrong idea? Should it be 
>>> x-SOME_EVENT instead?
>>
>> Since this is the first unstable event, there is no precedent.  Let's
>> use no prefix, and move on.
>> 
>>>> The name CONFIG_READ feels overly generic for something that makes sense
>>>> only with virtio devices.
>>>
>>> Hmm, right.. I think, we can say same thing about DEVICE_UNPLUG_GUEST_ERROR.
>>
>> That one came to be as a generalization of existing MEM_UNPLUG_ERROR and
>> a concrete need to signal CPU unplug errors.  Demonstrates "unplug guest
>> errors" can happen for different kinds of devices.  So we went with a
>> generic event we can use for all of them.
>> This doesn't seem to be the case for this patch's event.  Thoughts?
>
> Right.. VIRTIO_CONFIG_READ maybe?

Works for me.

[...]