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