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

[Markus Armbruster <armbru@redhat.com>]

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