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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

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.

> 
> > > Signed-off-by: Andrea Bolognani <abologna@xxxxxxxxxx>
> > > ---
> > >  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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux