On Fri, 2022-04-22 at 13:13 +0200, Markus Armbruster wrote:
> Nicolas Saenz Julienne <nsaen...@redhat.com> writes:
>
> > 'event-loop-base' provides basic property handling for all 'AioContext'
> > based event loops. So let's define a new 'MainLoopClass' that inherits
> > from it. This will permit tweaking the main loop's properties through
> > qapi as well as through the command line using the '-object' keyword[1].
> > Only one instance of 'MainLoopClass' might be created at any time.
> >
> > 'EventLoopBaseClass' learns a new callback, 'can_be_deleted()' so as to
> > mark 'MainLoop' as non-deletable.
> >
> > [1] For example:
> > -object main-loop,id=main-loop,aio-max-batch=<value>
> >
> > Signed-off-by: Nicolas Saenz Julienne <nsaen...@redhat.com>
> > Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com>
>
> [...]
>
> > diff --git a/qapi/qom.json b/qapi/qom.json
> > index eeb5395ff3..e5f31c4469 100644
> > --- a/qapi/qom.json
> > +++ b/qapi/qom.json
> > @@ -499,6 +499,17 @@
> > '*repeat': 'bool',
> > '*grab-toggle': 'GrabToggleKeys' } }
> >
> > +##
> > +# @EventLoopBaseProperties:
> > +#
> > +# Common properties for objects derived from EventLoopBase
>
> This makes sense as internal documentation: QAPI type
> EventLoopBaseProperties is associated with C type EventLoopBase. Doc
> comments are *external* documentation: they go into the QEMU QMP
> Reference Manual.
>
> What about "Common properties for event loops"?
Sounds better, yes. I'll change it.
> > +#
> > +# @aio-max-batch: maximum number of requests in a batch for the AIO engine,
> > +# 0 means that the engine will use its default.
>
> Missing:
>
> # Since: 7.1
>
> Permit me a short digression. We add these unthinkingly, because
> thinking is expensive. Even when the type isn't really part of the
> external interface. The deeper problem is that we're trying to generate
> documentation of the external interface from doc comments that are
> written as documentation of the internal QAPI data structures. Here,
> for example, we document EventLoopBaseProperties even though it is a
> purely internal thing: whether we factor out common members into a base
> type or not is not visible in QMP.
Thanks for the explanation.
> > +##
> > +{ 'struct': 'EventLoopBaseProperties',
> > + 'data': { '*aio-max-batch': 'int' } }
> > +
> > ##
> > # @IothreadProperties:
> > #
> > @@ -516,17 +527,26 @@
> > # algorithm detects it is spending too long polling without
> > # encountering events. 0 selects a default behaviour
> > (default: 0)
> > #
> > -# @aio-max-batch: maximum number of requests in a batch for the AIO engine,
> > -# 0 means that the engine will use its default
> > -# (default:0, since 6.1)
> > +# The @aio-max-batch option is available since 6.1.
>
> This separates the member's "since" information from its defintion.
> Can't be helped, because its defined in the base type, but the since
> only applies here. Okay.
IIUC my compromise of having the 'since version' annotated on each externally
visible type is good, right? No need to add the info in
EventLoopBaseProperties.
> > #
> > # Since: 2.0
> > ##
> > { 'struct': 'IothreadProperties',
> > + 'base': 'EventLoopBaseProperties',
> > 'data': { '*poll-max-ns': 'int',
> > '*poll-grow': 'int',
> > - '*poll-shrink': 'int',
> > - '*aio-max-batch': 'int' } }
> > + '*poll-shrink': 'int' } }
> > +
> > +##
> > +# @MainLoopProperties:
> > +#
> > +# Properties for the main-loop object.
> > +#
> > +# Since: 7.1
> > +##
> > +{ 'struct': 'MainLoopProperties',
> > + 'base': 'EventLoopBaseProperties',
> > + 'data': {} }
>
> The patch does two things:
>
> 1. Factor EventLoopBaseProperties out of IothreadProperties.
>
> 2. Create MainLoopProperties.
>
> I'd split it. This is not a demand.
Since I'm preparing a v5 of the series, I agree it makes sense to move 1. to
the fist patch.
Thanks!
--
Nicolás Sáenz
