[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/4] qapi: introduce device-sync-config
From: |
Markus Armbruster |
Subject: |
Re: [PATCH 2/4] qapi: introduce device-sync-config |
Date: |
Tue, 17 Oct 2023 16:57:37 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) |
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;