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

[Vladimir Sementsov-Ogievskiy]

On 22.05.23 12:27, Markus Armbruster wrote:
> "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?

Is there a syntax to add comment to the QAPI structure, which doesn't go into 
compiled public documentation?

I agree that we don't need this in compiled documentation, but this place in 
the code really good for the rationale, to avoid starting the discussion from 
the beginning again.

> > >   #
> > >   # @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

--
Best regards,
Vladimir