Re: [PATCH v4 2/3] util/main-loop: Introduce the main loop into QOM

[Nicolas Saenz Julienne <nsaenzju@redhat.com>]

On Fri, 2022-04-22 at 13:13 +0200, Markus Armbruster wrote:
> Nicolas Saenz Julienne <nsaenzju@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 <nsaenzju@redhat.com>
> > Reviewed-by: Stefan Hajnoczi <stefanha@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