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

[Vladimir Sementsov-Ogievskiy]

On 22.05.23 15:13, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>
> > 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?
>
> Yes!  qapi-code-gen.rst: "A multi-line comment that starts and ends with
> a ``##`` line is a documentation comment."  All other comments are not,
> and won't be included in generated documentation.

Good, thanks!

> Example: qapi/qapi-schema.json has
>
>      { 'include': 'pragma.json' }
>
>      # Documentation generated with qapi-gen.py is in source order, with
>      # included sub-schemas inserted at the first include directive
>      # (subsequent include directives have no effect).  To get a sane and
>      # stable order, it's best to include each sub-schema just once, or
>      # include it first right here.
>
>      { 'include': 'error.json' }
>
> Not a documentation comment, thus not included in generated
> documentation.
>
> Additionally, TODO sections in documentation comments are omitted from
> generated documentation.  qapi-code-gen.rst again: "TODO" sections are
> not rendered at all (they are for developers, not users of QMP).
>
> > 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.
>
> Saving rationale so we can refer to it later is good.  We tend to use
> commit messages for that.  I'd say use comments when the rationale needs
> to be more visible.

Yes I can move this to the commit message. I just wasn't sure that it's an 
obvious place in our case, as that's not a commit that introduces new structure 
but just a no-logic-change refactoring.

--
Best regards,
Vladimir