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@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
Cheers,
Victor
Attachment:
signature.asc
Description: PGP signature
