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 :|
