[PATCH 0/4] vhost-user-blk: live resize additional APIs

[PATCH 0/4] vhost-user-blk: live resize additional APIs

[Vladimir Sementsov-Ogievskiy]

In vhost-user protocol we have VHOST_USER_BACKEND_CONFIG_CHANGE_MSG,
which backend may send to notify Qemu, that we should re-read the
config, and notify the guest.

Still that's not always convenient: backend may not support this
message. Also, having QMP command to force config sync is more reliable
than waiting for notification from external program. It also may be
helpful for debug/restore: if we have changed disk size, but guest
doesn't see that, it's good to have a separate QMP command to trigger
resync of the config.

So, the series proposes two experimental APIs:

1. x-device-sync-config command, to trigger config synchronization

2. X_CONFIG_READ event, which notify management tool that guest read the
updated config. Of course, that can't guarantee that the guest correctly
handled the updated config, but it's still better than nothing: for sure
guest will not show new disk size until it read the updated config. So,
management tool may wait for this event to report success to the user.


The series is based on "[PATCH v8 0/4] pci hotplug tracking": it doesn't
depend on it, but just modify same files, so I just to avoid extra
conflicts.
Based-on: <20231005092926.56231-1-vsementsov@yandex-team.ru>

Vladimir Sementsov-Ogievskiy (4):
  vhost-user-blk: simplify and fix vhost_user_blk_handle_config_change
  qapi: introduce device-sync-config
  qapi: device-sync-config: check runstate
  qapi: introduce CONFIG_READ event

 hw/block/vhost-user-blk.c | 32 ++++++++++++++---------
 hw/virtio/virtio-pci.c    | 18 +++++++++++++
 include/hw/qdev-core.h    |  3 +++
 include/monitor/qdev.h    |  1 +
 include/sysemu/runstate.h |  1 +
 monitor/monitor.c         |  1 +
 qapi/qdev.json            | 36 ++++++++++++++++++++++++++
 softmmu/qdev-monitor.c    | 53 +++++++++++++++++++++++++++++++++++++++
 softmmu/runstate.c        |  5 ++++
 9 files changed, 138 insertions(+), 12 deletions(-)

-- 
2.34.1

[PATCH 1/4] vhost-user-blk: simplify and fix vhost_user_blk_handle_config_change

[Vladimir Sementsov-Ogievskiy]

Let's not care about what was changed and update the whole config,
reasons:

1. config->geometry should be updated together with capacity, so we fix
   a bug.

2. Vhost-user protocol doesn't say anything about config change
   limitation. Silent ignore of changes doesn't seem to be correct.

3. vhost-user-vsock reads the whole config

4. on realize we don't do any checks on retrieved config, so no reason
   to care here

Also, let's notify guest unconditionally:

1. So does vhost-user-vsock

2. We are going to reuse the functionality in new cases when we do want
   to notify the guest unconditionally. So, no reason to create extra
   branches in the logic.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 hw/block/vhost-user-blk.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index eecf3f7a81..1ee05b46ee 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -93,7 +93,6 @@ static void vhost_user_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
 static int vhost_user_blk_handle_config_change(struct vhost_dev *dev)
 {
     int ret;
-    struct virtio_blk_config blkcfg;
     VirtIODevice *vdev = dev->vdev;
     VHostUserBlk *s = VHOST_USER_BLK(dev->vdev);
     Error *local_err = NULL;
@@ -102,19 +101,15 @@ static int vhost_user_blk_handle_config_change(struct vhost_dev *dev)
         return 0;
     }
 
-    ret = vhost_dev_get_config(dev, (uint8_t *)&blkcfg,
+    ret = vhost_dev_get_config(dev, (uint8_t *)&s->blkcfg,
                                vdev->config_len, &local_err);
     if (ret < 0) {
         error_report_err(local_err);
         return ret;
     }
 
-    /* valid for resize only */
-    if (blkcfg.capacity != s->blkcfg.capacity) {
-        s->blkcfg.capacity = blkcfg.capacity;
-        memcpy(dev->vdev->config, &s->blkcfg, vdev->config_len);
-        virtio_notify_config(dev->vdev);
-    }
+    memcpy(dev->vdev->config, &s->blkcfg, vdev->config_len);
+    virtio_notify_config(dev->vdev);
 
     return 0;
 }
-- 
2.34.1

Re: [PATCH 1/4] vhost-user-blk: simplify and fix vhost_user_blk_handle_config_change

[Raphael Norwitz]

I don’t understand the “valid for resize only” comment. Looks like this is zero day behavior and I can’t tell why it was added. Does anyone know?

With that, reasoning and code looks good:

Acked-by: Raphael Norwitz <raphael.norwitz@nutanix.com>

> On Oct 6, 2023, at 4:20 PM, Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
> 
> Let's not care about what was changed and update the whole config,
> reasons:
> 
> 1. config->geometry should be updated together with capacity, so we fix
>   a bug.
> 
> 2. Vhost-user protocol doesn't say anything about config change
>   limitation. Silent ignore of changes doesn't seem to be correct.
> 
> 3. vhost-user-vsock reads the whole config
> 
> 4. on realize we don't do any checks on retrieved config, so no reason
>   to care here
> 
> Also, let's notify guest unconditionally:
> 
> 1. So does vhost-user-vsock
> 
> 2. We are going to reuse the functionality in new cases when we do want
>   to notify the guest unconditionally. So, no reason to create extra
>   branches in the logic.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> hw/block/vhost-user-blk.c | 11 +++--------
> 1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index eecf3f7a81..1ee05b46ee 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -93,7 +93,6 @@ static void vhost_user_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
> static int vhost_user_blk_handle_config_change(struct vhost_dev *dev)
> {
>     int ret;
> -    struct virtio_blk_config blkcfg;
>     VirtIODevice *vdev = dev->vdev;
>     VHostUserBlk *s = VHOST_USER_BLK(dev->vdev);
>     Error *local_err = NULL;
> @@ -102,19 +101,15 @@ static int vhost_user_blk_handle_config_change(struct vhost_dev *dev)
>         return 0;
>     }
> 
> -    ret = vhost_dev_get_config(dev, (uint8_t *)&blkcfg,
> +    ret = vhost_dev_get_config(dev, (uint8_t *)&s->blkcfg,
>                                vdev->config_len, &local_err);
>     if (ret < 0) {
>         error_report_err(local_err);
>         return ret;
>     }
> 
> -    /* valid for resize only */
> -    if (blkcfg.capacity != s->blkcfg.capacity) {
> -        s->blkcfg.capacity = blkcfg.capacity;
> -        memcpy(dev->vdev->config, &s->blkcfg, vdev->config_len);
> -        virtio_notify_config(dev->vdev);
> -    }
> +    memcpy(dev->vdev->config, &s->blkcfg, vdev->config_len);
> +    virtio_notify_config(dev->vdev);
> 
>     return 0;
> }
> -- 
> 2.34.1
> 

[PATCH 2/4] qapi: introduce device-sync-config

[Vladimir Sementsov-Ogievskiy]

Add command to sync config from vhost-user backend to the device. It
may be helpful when VHOST_USER_SLAVE_CONFIG_CHANGE_MSG failed or not
triggered interrupt to the guest or just not available (not supported
by vhost-user server).

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 hw/block/vhost-user-blk.c | 27 ++++++++++++++++++++-------
 hw/virtio/virtio-pci.c    |  9 +++++++++
 include/hw/qdev-core.h    |  3 +++
 qapi/qdev.json            | 14 ++++++++++++++
 softmmu/qdev-monitor.c    | 23 +++++++++++++++++++++++
 5 files changed, 69 insertions(+), 7 deletions(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 1ee05b46ee..05ce7b5684 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -90,27 +90,39 @@ static void vhost_user_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
     s->blkcfg.wce = blkcfg->wce;
 }
 
+static int vhost_user_blk_sync_config(DeviceState *dev, Error **errp)
+{
+    int ret;
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    VHostUserBlk *s = VHOST_USER_BLK(vdev);
+
+    ret = vhost_dev_get_config(&s->dev, (uint8_t *)&s->blkcfg,
+                               vdev->config_len, errp);
+    if (ret < 0) {
+        return ret;
+    }
+
+    memcpy(vdev->config, &s->blkcfg, vdev->config_len);
+    virtio_notify_config(vdev);
+
+    return 0;
+}
+
 static int vhost_user_blk_handle_config_change(struct vhost_dev *dev)
 {
     int ret;
-    VirtIODevice *vdev = dev->vdev;
-    VHostUserBlk *s = VHOST_USER_BLK(dev->vdev);
     Error *local_err = NULL;
 
     if (!dev->started) {
         return 0;
     }
 
-    ret = vhost_dev_get_config(dev, (uint8_t *)&s->blkcfg,
-                               vdev->config_len, &local_err);
+    ret = vhost_user_blk_sync_config(DEVICE(dev->vdev), &local_err);
     if (ret < 0) {
         error_report_err(local_err);
         return ret;
     }
 
-    memcpy(dev->vdev->config, &s->blkcfg, vdev->config_len);
-    virtio_notify_config(dev->vdev);
-
     return 0;
 }
 
@@ -580,6 +592,7 @@ static void vhost_user_blk_class_init(ObjectClass *klass, void *data)
 
     device_class_set_props(dc, vhost_user_blk_properties);
     dc->vmsd = &vmstate_vhost_user_blk;
+    dc->sync_config = vhost_user_blk_sync_config;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
     vdc->realize = vhost_user_blk_device_realize;
     vdc->unrealize = vhost_user_blk_device_unrealize;
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index edbc0daa18..dd4620462b 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -2315,6 +2315,14 @@ static void virtio_pci_dc_realize(DeviceState *qdev, Error **errp)
     vpciklass->parent_dc_realize(qdev, errp);
 }
 
+static int virtio_pci_sync_config(DeviceState *dev, Error **errp)
+{
+    VirtIOPCIProxy *proxy = VIRTIO_PCI(dev);
+    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
+
+    return qdev_sync_config(DEVICE(vdev), errp);
+}
+
 static void virtio_pci_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -2331,6 +2339,7 @@ static void virtio_pci_class_init(ObjectClass *klass, void *data)
     device_class_set_parent_realize(dc, virtio_pci_dc_realize,
                                     &vpciklass->parent_dc_realize);
     rc->phases.hold = virtio_pci_bus_reset_hold;
+    dc->sync_config = virtio_pci_sync_config;
 }
 
 static const TypeInfo virtio_pci_info = {
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index a27ef2eb24..6fa5bac86e 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -95,6 +95,7 @@ typedef void (*DeviceUnrealize)(DeviceState *dev);
 typedef void (*DeviceReset)(DeviceState *dev);
 typedef void (*BusRealize)(BusState *bus, Error **errp);
 typedef void (*BusUnrealize)(BusState *bus);
+typedef int (*DeviceSyncConfig)(DeviceState *dev, Error **errp);
 
 /**
  * struct DeviceClass - The base class for all devices.
@@ -162,6 +163,7 @@ struct DeviceClass {
     DeviceReset reset;
     DeviceRealize realize;
     DeviceUnrealize unrealize;
+    DeviceSyncConfig sync_config;
 
     /**
      * @vmsd: device state serialisation description for
@@ -555,6 +557,7 @@ bool qdev_hotplug_allowed(DeviceState *dev, Error **errp);
  */
 HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev);
 void qdev_unplug(DeviceState *dev, Error **errp);
+int qdev_sync_config(DeviceState *dev, Error **errp);
 void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
                                   DeviceState *dev, Error **errp);
 void qdev_machine_creation_done(void);
diff --git a/qapi/qdev.json b/qapi/qdev.json
index fa80694735..2468f8bddf 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -315,3 +315,17 @@
 # Since: 8.2
 ##
 { 'event': 'X_DEVICE_ON', 'data': 'DeviceAndPath' }
+
+##
+# @x-device-sync-config:
+#
+# Sync config from backend to the guest.
+#
+# @id: the device's ID or QOM path
+#
+# Returns: Nothing on success
+#          If @id is not a valid device, DeviceNotFound
+#
+# Since: 8.2
+##
+{ 'command': 'x-device-sync-config', 'data': {'id': 'str'} }
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 19c31446d8..b6da24389f 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -987,6 +987,29 @@ HotplugInfo *qmp_x_query_hotplug(const char *id, Error **errp)
     return hotplug_handler_get_state(hotplug_ctrl, dev, errp);
 }
 
+int qdev_sync_config(DeviceState *dev, Error **errp)
+{
+    DeviceClass *dc = DEVICE_GET_CLASS(dev);
+
+    if (!dc->sync_config) {
+        error_setg(errp, "device-sync-config is not supported for '%s'",
+                   object_get_typename(OBJECT(dev)));
+        return -ENOTSUP;
+    }
+
+    return dc->sync_config(dev, errp);
+}
+
+void qmp_x_device_sync_config(const char *id, Error **errp)
+{
+    DeviceState *dev = find_device_state(id, errp);
+    if (!dev) {
+        return;
+    }
+
+    qdev_sync_config(dev, errp);
+}
+
 void hmp_device_add(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
-- 
2.34.1

Re: [PATCH 2/4] qapi: introduce device-sync-config

[Markus Armbruster]

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

> Add command to sync config from vhost-user backend to the device. It
> may be helpful when VHOST_USER_SLAVE_CONFIG_CHANGE_MSG failed or not
> triggered interrupt to the guest or just not available (not supported
> by vhost-user server).
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

[...]

> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index fa80694735..2468f8bddf 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -315,3 +315,17 @@
>  # Since: 8.2
>  ##
>  { 'event': 'X_DEVICE_ON', 'data': 'DeviceAndPath' }
> +
> +##
> +# @x-device-sync-config:
> +#
> +# Sync config from backend to the guest.

"Sync" is not a word; "synchronize" is :)

> +#
> +# @id: the device's ID or QOM path
> +#
> +# Returns: Nothing on success
> +#          If @id is not a valid device, DeviceNotFound

Why not GenericError?  

> +#
> +# Since: 8.2
> +##
> +{ 'command': 'x-device-sync-config', 'data': {'id': 'str'} }

The commit message above and the error message below talk about command
device-sync-config, but you actually name it x-device-sync-config.

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

We tend to eschew abbreviations in QAPI schema names.
device-synchronize-config is quite a mouthful, though.  What do you
think?

> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index 19c31446d8..b6da24389f 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -987,6 +987,29 @@ HotplugInfo *qmp_x_query_hotplug(const char *id, Error **errp)
>      return hotplug_handler_get_state(hotplug_ctrl, dev, errp);
>  }
>  
> +int qdev_sync_config(DeviceState *dev, Error **errp)
> +{
> +    DeviceClass *dc = DEVICE_GET_CLASS(dev);
> +
> +    if (!dc->sync_config) {
> +        error_setg(errp, "device-sync-config is not supported for '%s'",
> +                   object_get_typename(OBJECT(dev)));
> +        return -ENOTSUP;
> +    }
> +
> +    return dc->sync_config(dev, errp);
> +}
> +
> +void qmp_x_device_sync_config(const char *id, Error **errp)
> +{
> +    DeviceState *dev = find_device_state(id, errp);

Not your patch's fault, but here goes anyway: when @id refers to a
non-device, find_device_state() fails with "is not a hotpluggable
device".  "hotpluggable" is misleading.

> +    if (!dev) {
> +        return;
> +    }
> +
> +    qdev_sync_config(dev, errp);
> +}
> +
>  void hmp_device_add(Monitor *mon, const QDict *qdict)
>  {
>      Error *err = NULL;

Re: [PATCH 2/4] qapi: introduce device-sync-config

[Vladimir Sementsov-Ogievskiy]

On 17.10.23 17:57, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> Add command to sync config from vhost-user backend to the device. It
>> may be helpful when VHOST_USER_SLAVE_CONFIG_CHANGE_MSG failed or not
>> triggered interrupt to the guest or just not available (not supported
>> by vhost-user server).
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> 
> [...]
> 
>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>> index fa80694735..2468f8bddf 100644
>> --- a/qapi/qdev.json
>> +++ b/qapi/qdev.json
>> @@ -315,3 +315,17 @@
>>   # Since: 8.2
>>   ##
>>   { 'event': 'X_DEVICE_ON', 'data': 'DeviceAndPath' }
>> +
>> +##
>> +# @x-device-sync-config:
>> +#
>> +# Sync config from backend to the guest.
> 
> "Sync" is not a word; "synchronize" is :)

Seems, I learn English from code :)

> 
>> +#
>> +# @id: the device's ID or QOM path
>> +#
>> +# Returns: Nothing on success
>> +#          If @id is not a valid device, DeviceNotFound
> 
> Why not GenericError?

I just designed the command looking at device_del. device_del reports DeviceNotFound in this case. GenericError is OK for me, if you think it's better even in this case. I remember now that everything except GenericError is not recommended.

> 
>> +#
>> +# Since: 8.2
>> +##
>> +{ 'command': 'x-device-sync-config', 'data': {'id': 'str'} }
> 
> The commit message above and the error message below talk about command
> device-sync-config, but you actually name it x-device-sync-config.
> 
> 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".
> 
> We tend to eschew abbreviations in QAPI schema names.
> device-synchronize-config is quite a mouthful, though.  What do you
> think?

OK for me.

Hmm, could I ask here, is "config" a word?)) device-synchronize-configuration would become a precedent, I'm afraid)

> 
>> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
>> index 19c31446d8..b6da24389f 100644
>> --- a/softmmu/qdev-monitor.c
>> +++ b/softmmu/qdev-monitor.c
>> @@ -987,6 +987,29 @@ HotplugInfo *qmp_x_query_hotplug(const char *id, Error **errp)
>>       return hotplug_handler_get_state(hotplug_ctrl, dev, errp);
>>   }
>>   
>> +int qdev_sync_config(DeviceState *dev, Error **errp)
>> +{
>> +    DeviceClass *dc = DEVICE_GET_CLASS(dev);
>> +
>> +    if (!dc->sync_config) {
>> +        error_setg(errp, "device-sync-config is not supported for '%s'",
>> +                   object_get_typename(OBJECT(dev)));
>> +        return -ENOTSUP;
>> +    }
>> +
>> +    return dc->sync_config(dev, errp);
>> +}
>> +
>> +void qmp_x_device_sync_config(const char *id, Error **errp)
>> +{
>> +    DeviceState *dev = find_device_state(id, errp);
> 
> Not your patch's fault, but here goes anyway: when @id refers to a
> non-device, find_device_state() fails with "is not a hotpluggable
> device".  "hotpluggable" is misleading.

Hmm. Thanks, OK, I'll rework it somehow in v2.

> 
>> +    if (!dev) {
>> +        return;
>> +    }
>> +
>> +    qdev_sync_config(dev, errp);
>> +}
>> +
>>   void hmp_device_add(Monitor *mon, const QDict *qdict)
>>   {
>>       Error *err = NULL;
> 

-- 
Best regards,
Vladimir

Re: [PATCH 2/4] qapi: introduce device-sync-config

[Markus Armbruster]

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

> On 17.10.23 17:57, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>> 
>>> Add command to sync config from vhost-user backend to the device. It
>>> may be helpful when VHOST_USER_SLAVE_CONFIG_CHANGE_MSG failed or not
>>> triggered interrupt to the guest or just not available (not supported
>>> by vhost-user server).
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> [...]
>> 
>>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>>> index fa80694735..2468f8bddf 100644
>>> --- a/qapi/qdev.json
>>> +++ b/qapi/qdev.json
>>> @@ -315,3 +315,17 @@
>>>   # Since: 8.2
>>>   ##
>>>   { 'event': 'X_DEVICE_ON', 'data': 'DeviceAndPath' }
>>> +
>>> +##
>>> +# @x-device-sync-config:
>>> +#
>>> +# Sync config from backend to the guest.
>>
>> "Sync" is not a word; "synchronize" is :)
>
> Seems, I learn English from code :)

It's working, so no worries ;)

>>> +#
>>> +# @id: the device's ID or QOM path
>>> +#
>>> +# Returns: Nothing on success
>>> +#          If @id is not a valid device, DeviceNotFound
>>
>> Why not GenericError?
>
> I just designed the command looking at device_del. device_del reports DeviceNotFound in this case. GenericError is OK for me, if you think it's better even in this case. I remember now that everything except GenericError is not recommended.

I figure you picked up DeviceNotFound by reusing find_device_state().

Same happened when commit 9680caee0fa added blk_by_qdev_id().  At least
some of its users don't document the error code.  I'm not sure the
unwanted use of DeviceNotFound is worth fixing after all this time.  But
I certainly don't want it documented.

For your patch, not reusing find_device_state() would let you avoid
DeviceNotFound at the price of a few more lines of code.

>>> +#
>>> +# Since: 8.2
>>> +##
>>> +{ 'command': 'x-device-sync-config', 'data': {'id': 'str'} }
>>
>> The commit message above and the error message below talk about command
>> device-sync-config, but you actually name it x-device-sync-config.
>>
>> 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".
>>
>> We tend to eschew abbreviations in QAPI schema names.
>> device-synchronize-config is quite a mouthful, though.  What do you
>> think?
>
> OK for me.
>
> Hmm, could I ask here, is "config" a word?)) device-synchronize-configuration would become a precedent, I'm afraid)

In the words of Captain Barbossa, it's "more what you'd call
'guidelines' than actual rules."

I didn't come up with the "avoid abbreviations" stylistic guideline.  I
inherited it.

I do like consistent style.  I don't like excessively long names.
Sometimes these likes conflict, and we need to pick.

Checking... alright, there precedence both for 'config' and for 'sync'
in the QAPI schema.  You pick what you like best.

>>> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
>>> index 19c31446d8..b6da24389f 100644
>>> --- a/softmmu/qdev-monitor.c
>>> +++ b/softmmu/qdev-monitor.c
>>> @@ -987,6 +987,29 @@ HotplugInfo *qmp_x_query_hotplug(const char *id, Error **errp)
>>>       return hotplug_handler_get_state(hotplug_ctrl, dev, errp);
>>>   }
>>>   +int qdev_sync_config(DeviceState *dev, Error **errp)
>>> +{
>>> +    DeviceClass *dc = DEVICE_GET_CLASS(dev);
>>> +
>>> +    if (!dc->sync_config) {
>>> +        error_setg(errp, "device-sync-config is not supported for '%s'",
>>> +                   object_get_typename(OBJECT(dev)));
>>> +        return -ENOTSUP;
>>> +    }
>>> +
>>> +    return dc->sync_config(dev, errp);
>>> +}
>>> +
>>> +void qmp_x_device_sync_config(const char *id, Error **errp)
>>> +{
>>> +    DeviceState *dev = find_device_state(id, errp);
>>
>> Not your patch's fault, but here goes anyway: when @id refers to a
>> non-device, find_device_state() fails with "is not a hotpluggable
>> device".  "hotpluggable" is misleading.
>
> Hmm. Thanks, OK, I'll rework it somehow in v2.

I think "hotpluggable" is misleading for all the existing uses of
find_device_state().  Suggest a preliminary patch deleting the word.

>>> +    if (!dev) {
>>> +        return;
>>> +    }
>>> +
>>> +    qdev_sync_config(dev, errp);
>>> +}
>>> +
>>>   void hmp_device_add(Monitor *mon, const QDict *qdict)
>>>   {
>>>       Error *err = NULL;
>> 

[PATCH 3/4] qapi: device-sync-config: check runstate

[Vladimir Sementsov-Ogievskiy]

Command result is racy if allow it during migration. Let's allow the
sync only in RUNNING state.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 include/sysemu/runstate.h |  1 +
 softmmu/qdev-monitor.c    | 27 ++++++++++++++++++++++++++-
 softmmu/runstate.c        |  5 +++++
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/include/sysemu/runstate.h b/include/sysemu/runstate.h
index 08afb97695..1fc14c8122 100644
--- a/include/sysemu/runstate.h
+++ b/include/sysemu/runstate.h
@@ -5,6 +5,7 @@
 #include "qemu/notify.h"
 
 bool runstate_check(RunState state);
+const char *current_run_state_str(void);
 void runstate_set(RunState new_state);
 RunState runstate_get(void);
 bool runstate_is_running(void);
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index b6da24389f..b485375049 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -23,6 +23,7 @@
 #include "monitor/monitor.h"
 #include "monitor/qdev.h"
 #include "sysemu/arch_init.h"
+#include "sysemu/runstate.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-qdev.h"
 #include "qapi/qapi-events-qdev.h"
@@ -1002,7 +1003,31 @@ int qdev_sync_config(DeviceState *dev, Error **errp)
 
 void qmp_x_device_sync_config(const char *id, Error **errp)
 {
-    DeviceState *dev = find_device_state(id, errp);
+    MigrationState *s = migrate_get_current();
+    DeviceState *dev;
+
+    /*
+     * During migration there is a race between syncing`config and migrating it,
+     * so let's just not allow it.
+     *
+     * Moreover, let's not rely on setting up interrupts in paused state, which
+     * may be a part of migration process.
+     */
+
+    if (migration_is_running(s->state)) {
+        error_setg(errp, "Config synchronization is not allowed "
+                   "during migration.");
+        return;
+    }
+
+    if (!runstate_is_running()) {
+        error_setg(errp, "Config synchronization allowed only in '%s' state, "
+                   "current state is '%s'", RunState_str(RUN_STATE_RUNNING),
+                   current_run_state_str());
+        return;
+    }
+
+    dev = find_device_state(id, errp);
     if (!dev) {
         return;
     }
diff --git a/softmmu/runstate.c b/softmmu/runstate.c
index 1652ed0439..3a8211474e 100644
--- a/softmmu/runstate.c
+++ b/softmmu/runstate.c
@@ -181,6 +181,11 @@ bool runstate_check(RunState state)
     return current_run_state == state;
 }
 
+const char *current_run_state_str(void)
+{
+    return RunState_str(current_run_state);
+}
+
 static void runstate_init(void)
 {
     const RunStateTransition *p;
-- 
2.34.1

[PATCH 4/4] qapi: introduce CONFIG_READ event

[Vladimir Sementsov-Ogievskiy]

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>
---
 hw/virtio/virtio-pci.c |  9 +++++++++
 include/monitor/qdev.h |  1 +
 monitor/monitor.c      |  1 +
 qapi/qdev.json         | 22 ++++++++++++++++++++++
 softmmu/qdev-monitor.c |  5 +++++
 5 files changed, 38 insertions(+)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index dd4620462b..f24f8ff03d 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -23,6 +23,7 @@
 #include "hw/boards.h"
 #include "hw/virtio/virtio.h"
 #include "migration/qemu-file-types.h"
+#include "monitor/qdev.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_bus.h"
 #include "hw/qdev-properties.h"
@@ -541,6 +542,10 @@ static uint64_t virtio_pci_config_read(void *opaque, hwaddr addr,
     }
     addr -= config;
 
+    if (vdev->generation > 0) {
+        qdev_config_read_event(DEVICE(proxy));
+    }
+
     switch (size) {
     case 1:
         val = virtio_config_readb(vdev, addr);
@@ -1728,6 +1733,10 @@ static uint64_t virtio_pci_device_read(void *opaque, hwaddr addr,
         return UINT64_MAX;
     }
 
+    if (vdev->generation > 0) {
+        qdev_config_read_event(DEVICE(proxy));
+    }
+
     switch (size) {
     case 1:
         val = virtio_config_modern_readb(vdev, addr);
diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index 949a3672cb..f0b0eab07e 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -39,6 +39,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
 const char *qdev_set_id(DeviceState *dev, char *id, Error **errp);
 
 void qdev_hotplug_device_on_event(DeviceState *dev);
+void qdev_config_read_event(DeviceState *dev);
 
 DeviceAndPath *qdev_new_device_and_path(DeviceState *dev);
 
diff --git a/monitor/monitor.c b/monitor/monitor.c
index 941f87815a..f8aa91b190 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -315,6 +315,7 @@ static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
     [QAPI_EVENT_QUORUM_FAILURE]    = { 1000 * SCALE_MS },
     [QAPI_EVENT_VSERPORT_CHANGE]   = { 1000 * SCALE_MS },
     [QAPI_EVENT_MEMORY_DEVICE_SIZE_CHANGE] = { 1000 * SCALE_MS },
+    [QAPI_EVENT_X_CONFIG_READ]   = { 300 * SCALE_MS },
 };
 
 /*
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' } }
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);
+}
-- 
2.34.1

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

[Markus Armbruster]

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>
> ---
>  hw/virtio/virtio-pci.c |  9 +++++++++
>  include/monitor/qdev.h |  1 +
>  monitor/monitor.c      |  1 +
>  qapi/qdev.json         | 22 ++++++++++++++++++++++
>  softmmu/qdev-monitor.c |  5 +++++
>  5 files changed, 38 insertions(+)
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index dd4620462b..f24f8ff03d 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -23,6 +23,7 @@
>  #include "hw/boards.h"
>  #include "hw/virtio/virtio.h"
>  #include "migration/qemu-file-types.h"
> +#include "monitor/qdev.h"
>  #include "hw/pci/pci.h"
>  #include "hw/pci/pci_bus.h"
>  #include "hw/qdev-properties.h"
> @@ -541,6 +542,10 @@ static uint64_t virtio_pci_config_read(void *opaque, hwaddr addr,
>      }
>      addr -= config;
>  
> +    if (vdev->generation > 0) {
> +        qdev_config_read_event(DEVICE(proxy));
> +    }
> +
>      switch (size) {
>      case 1:
>          val = virtio_config_readb(vdev, addr);
> @@ -1728,6 +1733,10 @@ static uint64_t virtio_pci_device_read(void *opaque, hwaddr addr,
>          return UINT64_MAX;
>      }
>  
> +    if (vdev->generation > 0) {
> +        qdev_config_read_event(DEVICE(proxy));
> +    }
> +
>      switch (size) {
>      case 1:
>          val = virtio_config_modern_readb(vdev, addr);
> diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
> index 949a3672cb..f0b0eab07e 100644
> --- a/include/monitor/qdev.h
> +++ b/include/monitor/qdev.h
> @@ -39,6 +39,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
>  const char *qdev_set_id(DeviceState *dev, char *id, Error **errp);
>  
>  void qdev_hotplug_device_on_event(DeviceState *dev);
> +void qdev_config_read_event(DeviceState *dev);
>  
>  DeviceAndPath *qdev_new_device_and_path(DeviceState *dev);
>  
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 941f87815a..f8aa91b190 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -315,6 +315,7 @@ static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
>      [QAPI_EVENT_QUORUM_FAILURE]    = { 1000 * SCALE_MS },
>      [QAPI_EVENT_VSERPORT_CHANGE]   = { 1000 * SCALE_MS },
>      [QAPI_EVENT_MEMORY_DEVICE_SIZE_CHANGE] = { 1000 * SCALE_MS },
> +    [QAPI_EVENT_X_CONFIG_READ]   = { 300 * SCALE_MS },
>  };
>  
>  /*
> 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.

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

The name CONFIG_READ feels overly generic for something that makes sense
only with virtio devices.

> 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);
> +}

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

[Vladimir Sementsov-Ogievskiy]

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>
>> ---
>>   hw/virtio/virtio-pci.c |  9 +++++++++
>>   include/monitor/qdev.h |  1 +
>>   monitor/monitor.c      |  1 +
>>   qapi/qdev.json         | 22 ++++++++++++++++++++++
>>   softmmu/qdev-monitor.c |  5 +++++
>>   5 files changed, 38 insertions(+)
>>
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index dd4620462b..f24f8ff03d 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -23,6 +23,7 @@
>>   #include "hw/boards.h"
>>   #include "hw/virtio/virtio.h"
>>   #include "migration/qemu-file-types.h"
>> +#include "monitor/qdev.h"
>>   #include "hw/pci/pci.h"
>>   #include "hw/pci/pci_bus.h"
>>   #include "hw/qdev-properties.h"
>> @@ -541,6 +542,10 @@ static uint64_t virtio_pci_config_read(void *opaque, hwaddr addr,
>>       }
>>       addr -= config;
>>   
>> +    if (vdev->generation > 0) {
>> +        qdev_config_read_event(DEVICE(proxy));
>> +    }
>> +
>>       switch (size) {
>>       case 1:
>>           val = virtio_config_readb(vdev, addr);
>> @@ -1728,6 +1733,10 @@ static uint64_t virtio_pci_device_read(void *opaque, hwaddr addr,
>>           return UINT64_MAX;
>>       }
>>   
>> +    if (vdev->generation > 0) {
>> +        qdev_config_read_event(DEVICE(proxy));
>> +    }
>> +
>>       switch (size) {
>>       case 1:
>>           val = virtio_config_modern_readb(vdev, addr);
>> diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
>> index 949a3672cb..f0b0eab07e 100644
>> --- a/include/monitor/qdev.h
>> +++ b/include/monitor/qdev.h
>> @@ -39,6 +39,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
>>   const char *qdev_set_id(DeviceState *dev, char *id, Error **errp);
>>   
>>   void qdev_hotplug_device_on_event(DeviceState *dev);
>> +void qdev_config_read_event(DeviceState *dev);
>>   
>>   DeviceAndPath *qdev_new_device_and_path(DeviceState *dev);
>>   
>> diff --git a/monitor/monitor.c b/monitor/monitor.c
>> index 941f87815a..f8aa91b190 100644
>> --- a/monitor/monitor.c
>> +++ b/monitor/monitor.c
>> @@ -315,6 +315,7 @@ static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
>>       [QAPI_EVENT_QUORUM_FAILURE]    = { 1000 * SCALE_MS },
>>       [QAPI_EVENT_VSERPORT_CHANGE]   = { 1000 * SCALE_MS },
>>       [QAPI_EVENT_MEMORY_DEVICE_SIZE_CHANGE] = { 1000 * SCALE_MS },
>> +    [QAPI_EVENT_X_CONFIG_READ]   = { 300 * SCALE_MS },
>>   };
>>   
>>   /*
>> 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-".

Also, nothing said about events. Is using "X_" wrong idea? Should it be x-SOME_EVENT instead?

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

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);
>> +}
> 

-- 
Best regards,
Vladimir

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

[Markus Armbruster]

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?

> 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);
>>> +}
>> 

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

[Vladimir Sementsov-Ogievskiy]

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

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.

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

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

> 
>> 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);
>>>> +}
>>>
> 

-- 
Best regards,
Vladimir

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.

[...]

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

[Michael S. Tsirkin]

On Wed, Oct 18, 2023 at 12:36:10PM +0200, Markus Armbruster wrote:
> > 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.
> 

I think "x-" is still better for command line use of properties - we
don't have an API to mark things unstable there, do we?

-- 
MST

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

[Daniel P. Berrangé]

On Wed, Oct 18, 2023 at 06:51:41AM -0400, Michael S. Tsirkin wrote:
> On Wed, Oct 18, 2023 at 12:36:10PM +0200, Markus Armbruster wrote:
> > > 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.
> > 
> 
> I think "x-" is still better for command line use of properties - we
> don't have an API to mark things unstable there, do we?

Personally I like to see "x-" prefix present *everywhere* there is
an unstable feature, and consider the need to rename when declaring
it stable to be good thing as it sets an easily identifiable line
in the sand and is self-evident to outside observers.

The self-documenting nature of the "x-" prefer is what makes it most
compelling to me. A patch submission, or command line invokation or
an example QMP command, or a bug report, that exhibit an 'x-' prefix
are an immediate red flag to anyone who sees them.

If someone sees a QMP comamnd / a typical giant QEMU command line,
they are never going to go look at the QAPI schema to check whether
any feature used had an 'unstable' marker. The 'unstable' marker
might as well not exist in most cases.

IOW, having the 'unstable' flag in the QAPI schema is great for machine
introspection, but it isn't a substitute for having an 'x-' prefix used
for the benefit of humans IMHO.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Oct 18, 2023 at 06:51:41AM -0400, Michael S. Tsirkin wrote:
>> On Wed, Oct 18, 2023 at 12:36:10PM +0200, Markus Armbruster wrote:
>> > > 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.
>> > 
>> 
>> I think "x-" is still better for command line use of properties - we
>> don't have an API to mark things unstable there, do we?
>
> Personally I like to see "x-" prefix present *everywhere* there is
> an unstable feature, and consider the need to rename when declaring
> it stable to be good thing as it sets an easily identifiable line
> in the sand and is self-evident to outside observers.
>
> The self-documenting nature of the "x-" prefer is what makes it most
> compelling to me. A patch submission, or command line invokation or
> an example QMP command, or a bug report, that exhibit an 'x-' prefix
> are an immediate red flag to anyone who sees them.

Except when it isn't, like in "x-origin".

> If someone sees a QMP comamnd / a typical giant QEMU command line,
> they are never going to go look at the QAPI schema to check whether
> any feature used had an 'unstable' marker. The 'unstable' marker
> might as well not exist in most cases.
>
> IOW, having the 'unstable' flag in the QAPI schema is great for machine
> introspection, but it isn't a substitute for having an 'x-' prefix used
> for the benefit of humans IMHO.

I'm not sure there's disagreement.  Quoting myself:

    The "x-" can remind humans "this is unstable" better than a feature
    flag can (for machines, it's the other way round).

CLI and HMP are for humans.  We continue to use "x-" there.

QMP is for machines.  The feature flag is the sole source of truth.
Additional use of "x-" is fine, but not required.

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

[Daniel P. Berrangé]

On Wed, Oct 18, 2023 at 02:02:08PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Wed, Oct 18, 2023 at 06:51:41AM -0400, Michael S. Tsirkin wrote:
> >> On Wed, Oct 18, 2023 at 12:36:10PM +0200, Markus Armbruster wrote:
> >> > > 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.
> >> > 
> >> 
> >> I think "x-" is still better for command line use of properties - we
> >> don't have an API to mark things unstable there, do we?
> >
> > Personally I like to see "x-" prefix present *everywhere* there is
> > an unstable feature, and consider the need to rename when declaring
> > it stable to be good thing as it sets an easily identifiable line
> > in the sand and is self-evident to outside observers.
> >
> > The self-documenting nature of the "x-" prefer is what makes it most
> > compelling to me. A patch submission, or command line invokation or
> > an example QMP command, or a bug report, that exhibit an 'x-' prefix
> > are an immediate red flag to anyone who sees them.
> 
> Except when it isn't, like in "x-origin".
> 
> > If someone sees a QMP comamnd / a typical giant QEMU command line,
> > they are never going to go look at the QAPI schema to check whether
> > any feature used had an 'unstable' marker. The 'unstable' marker
> > might as well not exist in most cases.
> >
> > IOW, having the 'unstable' flag in the QAPI schema is great for machine
> > introspection, but it isn't a substitute for having an 'x-' prefix used
> > for the benefit of humans IMHO.
> 
> I'm not sure there's disagreement.  Quoting myself:
> 
>     The "x-" can remind humans "this is unstable" better than a feature
>     flag can (for machines, it's the other way round).
> 
> CLI and HMP are for humans.  We continue to use "x-" there.
> 
> QMP is for machines.  The feature flag is the sole source of truth.
> Additional use of "x-" is fine, but not required.

I guess we have different defintions of "for humans" in this context.
I consider QMP  data still relevant for humans, because humans are
reviewing patches to libvirt that add usage of QMP features, or
triaging bug reports that include examples of usage, and in both
cases it is pretty relevant to make unstable features stand out to
the human via the x- prefix IMHO.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (berrange@redhat.com) wrote:
> On Wed, Oct 18, 2023 at 02:02:08PM +0200, Markus Armbruster wrote:
> > Daniel P. Berrangé <berrange@redhat.com> writes:
> > 
> > > On Wed, Oct 18, 2023 at 06:51:41AM -0400, Michael S. Tsirkin wrote:
> > >> On Wed, Oct 18, 2023 at 12:36:10PM +0200, Markus Armbruster wrote:
> > >> > > 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.
> > >> > 
> > >> 
> > >> I think "x-" is still better for command line use of properties - we
> > >> don't have an API to mark things unstable there, do we?
> > >
> > > Personally I like to see "x-" prefix present *everywhere* there is
> > > an unstable feature, and consider the need to rename when declaring
> > > it stable to be good thing as it sets an easily identifiable line
> > > in the sand and is self-evident to outside observers.
> > >
> > > The self-documenting nature of the "x-" prefer is what makes it most
> > > compelling to me. A patch submission, or command line invokation or
> > > an example QMP command, or a bug report, that exhibit an 'x-' prefix
> > > are an immediate red flag to anyone who sees them.
> > 
> > Except when it isn't, like in "x-origin".
> > 
> > > If someone sees a QMP comamnd / a typical giant QEMU command line,
> > > they are never going to go look at the QAPI schema to check whether
> > > any feature used had an 'unstable' marker. The 'unstable' marker
> > > might as well not exist in most cases.
> > >
> > > IOW, having the 'unstable' flag in the QAPI schema is great for machine
> > > introspection, but it isn't a substitute for having an 'x-' prefix used
> > > for the benefit of humans IMHO.
> > 
> > I'm not sure there's disagreement.  Quoting myself:
> > 
> >     The "x-" can remind humans "this is unstable" better than a feature
> >     flag can (for machines, it's the other way round).
> > 
> > CLI and HMP are for humans.  We continue to use "x-" there.
> > 
> > QMP is for machines.  The feature flag is the sole source of truth.
> > Additional use of "x-" is fine, but not required.
> 
> I guess we have different defintions of "for humans" in this context.
> I consider QMP  data still relevant for humans, because humans are
> reviewing patches to libvirt that add usage of QMP features, or
> triaging bug reports that include examples of usage, and in both
> cases it is pretty relevant to make unstable features stand out to
> the human via the x- prefix IMHO.

Using x- for events makes sense to me; the semantics of events can be
quite subtle; often you don't find out how broken they are until you
wire them through libvirt and up the stack; so it's not impossible
you might need to change it - but then without the x- the semantics
(rather than existence) of the event is carved in stone.

Dave

> With regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

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

[Markus Armbruster]

"Dr. David Alan Gilbert" <dave@treblig.org> writes:

> Using x- for events makes sense to me; the semantics of events can be
> quite subtle; often you don't find out how broken they are until you
> wire them through libvirt and up the stack; so it's not impossible
> you might need to change it - but then without the x- the semantics
> (rather than existence) of the event is carved in stone.

It is carved in stone unless feature 'unstable' is declared.  The name
gets no vote.

We may elect to name unstable QAPI things 'x-FOO' to help humans.

[...]

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

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Oct 18, 2023 at 02:02:08PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > On Wed, Oct 18, 2023 at 06:51:41AM -0400, Michael S. Tsirkin wrote:
>> >> On Wed, Oct 18, 2023 at 12:36:10PM +0200, Markus Armbruster wrote:
>> >> > > 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.
>> >> > 
>> >> 
>> >> I think "x-" is still better for command line use of properties - we
>> >> don't have an API to mark things unstable there, do we?
>> >
>> > Personally I like to see "x-" prefix present *everywhere* there is
>> > an unstable feature, and consider the need to rename when declaring
>> > it stable to be good thing as it sets an easily identifiable line
>> > in the sand and is self-evident to outside observers.
>> >
>> > The self-documenting nature of the "x-" prefer is what makes it most
>> > compelling to me. A patch submission, or command line invokation or
>> > an example QMP command, or a bug report, that exhibit an 'x-' prefix
>> > are an immediate red flag to anyone who sees them.
>> 
>> Except when it isn't, like in "x-origin".
>> 
>> > If someone sees a QMP comamnd / a typical giant QEMU command line,
>> > they are never going to go look at the QAPI schema to check whether
>> > any feature used had an 'unstable' marker. The 'unstable' marker
>> > might as well not exist in most cases.
>> >
>> > IOW, having the 'unstable' flag in the QAPI schema is great for machine
>> > introspection, but it isn't a substitute for having an 'x-' prefix used
>> > for the benefit of humans IMHO.
>> 
>> I'm not sure there's disagreement.  Quoting myself:
>> 
>>     The "x-" can remind humans "this is unstable" better than a feature
>>     flag can (for machines, it's the other way round).
>> 
>> CLI and HMP are for humans.  We continue to use "x-" there.
>> 
>> QMP is for machines.  The feature flag is the sole source of truth.
>> Additional use of "x-" is fine, but not required.
>
> I guess we have different defintions of "for humans" in this context.
> I consider QMP  data still relevant for humans, because humans are
> reviewing patches to libvirt that add usage of QMP features, or

There's a much more reliable way:

1. Require tests

2. Run them with -compat unstable-input=reject,unstable-output=hide

   Or unstable-input=crash if you don't trust or don't want to rely on
   your test harness.

> triaging bug reports that include examples of usage,

Here you can run the reproducer with -compat.

>                                                      and in both
> cases it is pretty relevant to make unstable features stand out to
> the human via the x- prefix IMHO.

Your point remains at least somewhat valid all the same.

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

[Vladimir Sementsov-Ogievskiy]

On 18.10.23 15:02, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
>> On Wed, Oct 18, 2023 at 06:51:41AM -0400, Michael S. Tsirkin wrote:
>>> On Wed, Oct 18, 2023 at 12:36:10PM +0200, Markus Armbruster wrote:
>>>>> 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.
>>>>
>>>
>>> I think "x-" is still better for command line use of properties - we
>>> don't have an API to mark things unstable there, do we?
>>
>> Personally I like to see "x-" prefix present *everywhere* there is
>> an unstable feature, and consider the need to rename when declaring
>> it stable to be good thing as it sets an easily identifiable line
>> in the sand and is self-evident to outside observers.
>>
>> The self-documenting nature of the "x-" prefer is what makes it most
>> compelling to me. A patch submission, or command line invokation or
>> an example QMP command, or a bug report, that exhibit an 'x-' prefix
>> are an immediate red flag to anyone who sees them.
> 
> Except when it isn't, like in "x-origin".
> 

Interesting how many such stable x-FOO things we have? Probably we could deprecate and than rename them?


-- 
Best regards,
Vladimir

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

[Markus Armbruster]

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

> On 18.10.23 15:02, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>>> On Wed, Oct 18, 2023 at 06:51:41AM -0400, Michael S. Tsirkin wrote:
>>>> On Wed, Oct 18, 2023 at 12:36:10PM +0200, Markus Armbruster wrote:
>>>>>> 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.
>>>>>
>>>>
>>>> I think "x-" is still better for command line use of properties - we
>>>> don't have an API to mark things unstable there, do we?
>>>
>>> Personally I like to see "x-" prefix present *everywhere* there is
>>> an unstable feature, and consider the need to rename when declaring
>>> it stable to be good thing as it sets an easily identifiable line
>>> in the sand and is self-evident to outside observers.
>>>
>>> The self-documenting nature of the "x-" prefer is what makes it most
>>> compelling to me. A patch submission, or command line invokation or
>>> an example QMP command, or a bug report, that exhibit an 'x-' prefix
>>> are an immediate red flag to anyone who sees them.
>> Except when it isn't, like in "x-origin".
>> 
>
> Interesting how many such stable x-FOO things we have?

I don't know.

I think the more interesting question is how many *unstable* things we
have that aren't named x-FOO.  I'm thinking of stuff that was never
intended to be exposed externally.  QOM/qdev properties, mostly.  For
extra spiciness, throw in qom-get and especially qom-set.

> Probably we could deprecate and than rename them?

I guess we can if we care :)