On 07/14/2014 06:58 PM, Eric Blake wrote:
On 07/14/2014 10:40 AM, Daniel P. Berrange wrote:
} - def->os.bios.useserial = VIR_DOMAIN_BIOS_USESERIAL_YES; + def->os.bios.useserial = VIR_DOMAIN_YES_NO_ENABLED; } else { - def->os.bios.useserial = VIR_DOMAIN_BIOS_USESERIAL_NO; + def->os.bios.useserial = VIR_DOMAIN_YES_NO_DISABLED; }
if (def->data.spice.filetransfer) virBufferAsprintf(buf, "<filetransfer enable='%s'/>\n", - virDomainGraphicsSpiceAgentFileTransferTypeToString(def->data.spice.filetransfer)); + virDomainYesNoTypeToString(def->data.spice.filetransfer)); }
I'm not really a fan of this cleanup, as IMHO the result is less clear & harder to follow than the original code.
How so? The original code was very repetitive, with multiple enums (all with long names) copying the same few enum elements. We're not painting ourselves into a corner - if any of the replaced enums ever grows a third value (such as "on", "hybrid", "off"), then we just break that one enum back into a named list rather than using the generic on/off enum. I'm actually in favor of this cleanup.
Specifically a enum constant name like YES_NO_DISABLED is just awful IMHO compared to the original desriptive name.
Agreed, my constant names are awful. But it's the original type names I don't like: I'd expect virDomainGraphicsSpiceAgentFileTransfer to be an enum of different modes of transfer, not just default/no/yes.
Is it just a matter of coming up with a better name? Maybe:
VIR_TRISTATE_ABSENT = 0, VIR_TRISTATE_NO, VIR_TRISTATE_YES,
Without the DOMAIN prefix, this could be used for network_conf.c too. How about: VIR_TRISTATE_SWITCH_ABSENT = 0, VIR_TRISTATE_SWITCH_OFF VIR_TRISTATE_SWITCH_ON for the other enum? (And maybe VIR_TRISTATE_BOOL for the first one?)
def->os.bios.useserial = VIR_TRISTATE_NO;
if (def->data.spice.filetransfer) { virBufferAsprintf(buf, "<filetransfer enable='%s'/>\n", virTristateToString(def->data.spice.filetransfer));
Jan