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

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.

> > Signed-off-by: Andrea Bolognani <abologna(a)redhat.com&gt; > > --- > > 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