[libvirt PATCH 2/2] include: Explicitly reserve values for overlapping flag types

Victor Toso victortoso at redhat.com
Thu May 5 09:04:00 UTC 2022 

Hi,

On Thu, May 05, 2022 at 08:57:04AM +0100, Daniel P. Berrangé wrote:
> On Thu, May 05, 2022 at 09:48:54AM +0200, Victor Toso wrote:
> > Hi,
> > 
> > On Wed, May 04, 2022 at 07:02:48PM +0100, Daniel P. Berrangé wrote:
> > > On Wed, May 04, 2022 at 07:30:08PM +0200, Andrea Bolognani wrote:
> > > > Due to hystorical reasons, it needs to be possible to pass values
> > 
> > hystorical -> historical ?
> > 
> > > > from the virTypedParameterFlags and virDomainModificationImpact
> > > > enumerations to a function at the same time, so it is very
> > > > important that the two never overlap.
> > > > 
> > > > Right now this is "enforced" by the presence of special comments;
> > > > unfortunately, said comments are not handled correctly by
> > > > apibuild.py and end up, quite confusingly, showing up as part of
> > > > the documentation for symbols preceding or following them.
> > > > 
> > > > Introduce actual entires in each enumeration for each of the
> > > > overlapping values, which is more explicit and results in
> > > > comments being parsed correctly.
> > > 
> > > I don't really like the idea of adding stuff to the public API
> > > to workaround brokenness in apibuild.py.
> > 
> > While apibuild.py needs to be fixed to error/warn in this
> > scenarios, I'd argue that the patch moves towards consistency
> > with comments blocks and improves the documentation of already
> > exposed API.
> > 
> > > It seems like we only need apibuild.py to not merge together
> > > distinct comment blocks.
> > 
> > What is not trivial is to (1) define which comment block belongs
> > to which element/type. We need to define what is acceptable and
> > what is not and (2) enforce that to stay consistent.
> 
> If we have multiple opened+clsoed comment blocks immediately after
> each other such as this scenario:
> 
>     /* 1 << 0 is reserved for virDomainModificationImpact */
>     /* 1 << 1 is reserved for virDomainModificationImpact */
> 
>     /* Older servers lacked the ability to handle string typed
>      * parameters.  Attempts to set a string parameter with an older
>      * server will fail at the client, but attempts to retrieve
>      * parameters must not return strings from a new server to an
>      * older client, so this flag exists to identify newer clients to
>      * newer servers.  This flag is automatically set when needed, so
>      * the user does not have to worry about it; however, manually
>      * setting the flag can be used to reject servers that cannot
>      * return typed strings, even if no strings would be returned.
>      *
>      * Since: v0.9.8
>      */
>     VIR_TYPED_PARAM_STRING_OKAY = 1 << 2,
> 
> 
> IMHO it is pretty straightforward for apibuild.py to have a policy
> that the comment block closest to the declaration is the API docs
> and the preceeding ones are irrelevant to hte API docs.

While working on the Since metadata, I recall finding instances
of:

    /* (C1) Comment before enum value */
    ENUM_VALUE,  /* (C2) Comment same line
                    that might span to multiple lines */
    /* (C3) Comment after enum value */


Not necessary at the same time, but the point is that they were
meant to document ENUM_VALUE. IIRC, (C3) was only for the enum
sentinel value (_ENUM_TYPE_LAST).

Sure, we can pick one and follow it but there is some fixing to
do in the existing documentation to keep it consistent.

> I very much doubt we hav a case where we have multiple open+closed
> comment blocks which all should be part of the API docs for a given
> declaration, and if we did, then we should merge them into a single
> open+closed comment block.

I think we should not merge them, but apibuild should warn/error
instead as it seems unlikely that two block of comments refer to
a single type.

> > > > Signed-off-by: Andrea Bolognani <abologna at redhat.com>
> > > > ---
> > > >  include/libvirt/libvirt-common.h.in | 19 +++++++++++++++++--
> > > >  include/libvirt/libvirt-domain.h    |  8 ++++----
> > > >  2 files changed, 21 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/include/libvirt/libvirt-common.h.in b/include/libvirt/libvirt-common.h.in
> > > > index ccdbb2a100..2f20456dfd 100644
> > > > --- a/include/libvirt/libvirt-common.h.in
> > > > +++ b/include/libvirt/libvirt-common.h.in
> > > > @@ -159,8 +159,23 @@ typedef enum {
> > > >   * Since: 0.9.8
> > > >   */
> > > >  typedef enum {
> > > > -    /* 1 << 0 is reserved for virDomainModificationImpact */
> > > > -    /* 1 << 1 is reserved for virDomainModificationImpact */
> > > > +    /* Reserved for virDomainModificationImpact. Do not use.
> > > > +     *
> > > > +     * Since: 8.4.0
> > > > +     */
> > > > +    VIR_TYPED_PARAM_RESERVED1 = 0,
> > > > +
> > > > +    /* Reserved for virDomainModificationImpact. Do not use.
> > > > +     *
> > > > +     * Since: 8.4.0
> > > > +     */
> > > > +    VIR_TYPED_PARAM_RESERVED2 = 1 << 0,
> > > > +
> > > > +    /* Reserved for virDomainModificationImpact. Do not use.
> > > > +     *
> > > > +     * Since: 8.4.0
> > > > +     */
> > > > +    VIR_TYPED_PARAM_RESERVED3 = 1 << 1,
> > > >  
> > > >      /* Older servers lacked the ability to handle string typed
> > > >       * parameters.  Attempts to set a string parameter with an older
> > > > diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> > > > index 2edef9c4e1..94cb4a6615 100644
> > > > --- a/include/libvirt/libvirt-domain.h
> > > > +++ b/include/libvirt/libvirt-domain.h
> > > > @@ -321,10 +321,10 @@ typedef virDomainControlInfo *virDomainControlInfoPtr;
> > > >   * Since: 0.9.2
> > > >   */
> > > >  typedef enum {
> > > > -    VIR_DOMAIN_AFFECT_CURRENT = 0,      /* Affect current domain state. (Since: 0.9.2)  */
> > > > -    VIR_DOMAIN_AFFECT_LIVE    = 1 << 0, /* Affect running domain state. (Since: 0.9.2)  */
> > > > -    VIR_DOMAIN_AFFECT_CONFIG  = 1 << 1, /* Affect persistent domain state. (Since: 0.9.2) */
> > > > -    /* 1 << 2 is reserved for virTypedParameterFlags */
> > > > +    VIR_DOMAIN_AFFECT_CURRENT   = 0,      /* Affect current domain state. (Since: 0.9.2)  */
> > > > +    VIR_DOMAIN_AFFECT_LIVE      = 1 << 0, /* Affect running domain state. (Since: 0.9.2)  */
> > > > +    VIR_DOMAIN_AFFECT_CONFIG    = 1 << 1, /* Affect persistent domain state. (Since: 0.9.2) */
> > > > +    VIR_DOMAIN_AFFECT_RESERVED1 = 1 << 2, /* Reserved for virTypedParameterFlags. Do not use. (Since: 8.4.0) */
> > > >  } virDomainModificationImpact;
> > > >  
> > > >  /**
> > > > -- 
> > > > 2.35.1
> > > > 
> > > 
> > > With regards,
> > > Daniel
> > 
> > Cheers,
> > Victor
> 
> With regards,
> Daniel

Cheers,
Victor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20220505/990148ab/attachment.sig>

More information about the libvir-list mailing list