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

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(a)massclouds.com&gt; >> --- >>  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.