[RFCv2 37/46] conf: Replace virDomainGraphicsDefParseXMLSpice(hardcoded) with virDomainGraphicsSpiceDefParseXML(generated)

[Daniel P. Berrangé]

On Mon, Dec 07, 2020 at 03:47:39PM +0800, Shi Lei wrote:
> On 2020-12-05 at 02:17, DanielP. Berrangé wrote:
> >On Fri, Sep 04, 2020 at 11:35:29AM +0800, Shi Lei wrote:
> >> Signed-off-by: Shi Lei <shi_lei at massclouds.com>
> >> ---
> >>  src/conf/domain_conf.c | 272 ++---------------------------------------
> >>  src/conf/domain_conf.h |  37 +++---
> >>  2 files changed, 26 insertions(+), 283 deletions(-)
> >>
> >> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> >> index b3ec111..20d731b 100644
> >> --- a/src/conf/domain_conf.c
> >> +++ b/src/conf/domain_conf.c
> >> @@ -877,6 +877,7 @@ VIR_ENUM_IMPL(virDomainGraphicsVNCSharePolicy,
> >> 
> >>  VIR_ENUM_IMPL(virDomainGraphicsSpiceChannelName,
> >>                VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_LAST,
> >> +              "none",
> >>                "main",
> >>                "display",
> >>                "inputs",
> >> @@ -14431,13 +14432,14 @@ virDomainGraphicsRDPDefParseXMLHook(xmlNodePtr node
> >> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> >> index df84763..f27f429 100644
> >> --- a/src/conf/domain_conf.h
> >> +++ b/src/conf/domain_conf.h
> >> @@ -1584,6 +1584,7 @@ struct _virDomainGraphicsAuthDef {  /* genparse, genformat:separate */
> >>  };
> >> 
> >>  typedef enum {
> >> +    VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_NONE = 0,
> >
> >I'm not sure why this extra enum field needs to be added ?
> >
> >IMHO we don't really want to have such extra values except in
> >a few special cases where we need to track some "default"
> >explicitly. 
> 
> There're two forms of enum in the code.
> 
> (1) Start with a 'default' or 'none' item, e.g., virDomainDiskIo
> 
>     The virDomainDiskIo has VIR_DOMAIN_DISK_IO_DEFAULT which is ZERO-indexed.
> 
>     When parsing it, the code is like:
> 
>     if ((tmp = virXMLPropString(cur, "io")) &&
>         (def->iomode = virDomainDiskIoTypeFromString(tmp)) <= 0) {
>         virReportError(...);
>         return -1;
>     }
> 
>     It checks XXXTypeFromString() ** <= ** 0, because the form <... io='default'>
>     is illegal.
> 
> (2) Without a 'default' or 'none' value, e.g., virDomainDiskDevice
> 
>     When parsing it, the code is like:
> 
>      if ((tmp = virXMLPropString(node, "device")) &&
>          (def->device = virDomainDiskDeviceTypeFromString(tmp)) < 0) {
>          virReportError(...);
>         return -1;
>     }
> 
>     It just checks XXXTypeFromString() ** < ** 0, because the enum's ZERO-Value item
>     is valid.
> 
> 
> To handle these two situations, I add extra "default" for case #2 and unify the form of enums,
> so that it's easier to implement the generator.
> 
> Another solution is to add an extra directive to indicate whether a enum has a 'default' item,
> so that the generator can decide between '<=' and '<'. Perhaps it is more safe and reliable.

Looks like there are about 200 enums,  43 use _NONE and 56 use _DEFAULT,
and the remaining 100 use neither.

Maybe we can do without the need to add a directive, if we change all cases of
NONE to DEFAULT, and just make the generator look at whether _DEFAULT is present
or not ?


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