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

[Markus Armbruster <armbru@redhat.com>]

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;