Vladimir Sementsov-Ogievskiy <vsement...@yandex-team.ru> writes: > On 17.10.23 18:00, Markus Armbruster wrote: >> Vladimir Sementsov-Ogievskiy <vsement...@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 <vsement...@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? > 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? > So, what about DEVICE_GUEST_READ_CONFIG ? > >> >>> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c >>> index b485375049..d0f022e925 100644 >>> --- a/softmmu/qdev-monitor.c >>> +++ b/softmmu/qdev-monitor.c >>> @@ -1252,3 +1252,8 @@ void qdev_hotplug_device_on_event(DeviceState *dev) >>> dev->device_on_event_sent = true; >>> qapi_event_send_x_device_on(dev->id, dev->canonical_path); >>> } >>> + >>> +void qdev_config_read_event(DeviceState *dev) >>> +{ >>> + qapi_event_send_x_config_read(dev->id, dev->canonical_path); >>> +} >>