On 2020-12-07 at 17:38, DanielP. Berrangé wrote:
>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@xxxxxxxxxxxxxx>
>> >> ---
>> >> 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 ?
Hmm. I try it.
Shi Lei
>
>
>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 :|
>
