Re: [PATCH v7 1/4] qapi/qdev.json: unite DEVICE_* event data into single structure

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Fri, Apr 21, 2023 at 01:32:04PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> DEVICE_DELETED and DEVICE_UNPLUG_GUEST_ERROR has equal data, let's
>> refactor it to one structure. That also helps to add new events
>> consistently.
>> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>
> Can QAPI maintainers please review this patchset?
> It's been a month.

It's been a busy month; sorry for the delay.

>> ---
>>  qapi/qdev.json | 39 +++++++++++++++++++++++++++------------
>>  1 file changed, 27 insertions(+), 12 deletions(-)
>> 
>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>> index 2708fb4e99..135cd81586 100644
>> --- a/qapi/qdev.json
>> +++ b/qapi/qdev.json
>> @@ -114,16 +114,37 @@
>>  { 'command': 'device_del', 'data': {'id': 'str'} }
>>  
>>  ##
>> -# @DEVICE_DELETED:
>> +# @DeviceAndPath:
>>  #
>> -# Emitted whenever the device removal completion is acknowledged by the 
>> guest.
>> -# At this point, it's safe to reuse the specified device ID. Device removal 
>> can
>> -# be initiated by the guest or by HMP/QMP commands.
>> +# In events we designate devices by both their ID (if the device has one)
>> +# and QOM path.
>> +#
>> +# Why we need ID? User specify ID in device_add command and in command line
>> +# and expects same identifier in the event data.
>> +#
>> +# Why we need QOM path? Some devices don't have ID and we still want to emit
>> +# events for them.
>> +#
>> +# So, we have a bit of redundancy, as QOM path for device that has ID is
>> +# always /machine/peripheral/ID. But that's hard to change keeping both
>> +# simple interface for most users and universality for the generic case.

Hmm.  I appreciate rationale, but I'm not sure it fits here.  Would
readers be worse off if we dropped it?

>>  #
>>  # @device: the device's ID if it has one
>>  #
>>  # @path: the device's QOM path
>>  #
>> +# Since: 8.0
>> +##
>> +{ 'struct': 'DeviceAndPath',
>> +  'data': { '*device': 'str', 'path': 'str' } }
>> +
>
> Should be Since: 8.1 no?

Yes.

Please format like

   ##
   # @DeviceAndPath:
   #
   # In events we designate devices by both their ID (if the device has
   # one) and QOM path.
   #
   # Why we need ID?  User specify ID in device_add command and in
   # command line and expects same identifier in the event data.
   #
   # Why we need QOM path?  Some devices don't have ID and we still want
   # to emit events for them.
   #
   # So, we have a bit of redundancy, as QOM path for device that has ID
   # is always /machine/peripheral/ID. But that's hard to change keeping
   # both simple interface for most users and universality for the
   # generic case.
   #
   # @device: the device's ID if it has one
   #
   # @path: the device's QOM path
   #
   # Since: 8.0
   ##

to blend in with recent commit a937b6aa739 (qapi: Reformat doc comments
to conform to current conventions).

>> +##
>> +# @DEVICE_DELETED:
>> +#
>> +# Emitted whenever the device removal completion is acknowledged by the 
>> guest.
>> +# At this point, it's safe to reuse the specified device ID. Device removal 
>> can
>> +# be initiated by the guest or by HMP/QMP commands.
>> +#

Conflict resolution:

    # Emitted whenever the device removal completion is acknowledged by
    # the guest.  At this point, it's safe to reuse the specified device
    # ID. Device removal can be initiated by the guest or by HMP/QMP
    # commands.

>>  # Since: 1.5
>>  #
>>  # Example:
>> @@ -134,18 +155,13 @@
>>  #      "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>>  #
>>  ##
>> -{ 'event': 'DEVICE_DELETED',
>> -  'data': { '*device': 'str', 'path': 'str' } }
>> +{ 'event': 'DEVICE_DELETED', 'data': 'DeviceAndPath' }
>>  
>>  ##
>>  # @DEVICE_UNPLUG_GUEST_ERROR:
>>  #
>>  # Emitted when a device hot unplug fails due to a guest reported error.
>>  #
>> -# @device: the device's ID if it has one
>> -#
>> -# @path: the device's QOM path
>> -#
>>  # Since: 6.2
>>  #
>>  # Example:
>> @@ -156,5 +172,4 @@
>>  #      "timestamp": { "seconds": 1615570772, "microseconds": 202844 } }
>>  #
>>  ##
>> -{ 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
>> -  'data': { '*device': 'str', 'path': 'str' } }
>> +{ 'event': 'DEVICE_UNPLUG_GUEST_ERROR', 'data': 'DeviceAndPath' }
>> -- 
>> 2.34.1