Re: [libvirt] [PATCH 1/2] Introduce virDomainYesNo enum type

On 07/14/2014 08:56 AM, Daniel P. Berrange wrote: > On Mon, Jul 14, 2014 at 04:47:16PM +0200, Ján Tomko wrote: >> @@ -10901,9 +10884,9 @@ virDomainDefParseBootXML(xmlXPathContextPtr ctxt, >> "for useserial")); >> goto cleanup; >> } >> - 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; >> } >> VIR_FREE(tmp); >> } >> >> @@ -16943,10 +16926,10 @@ virDomainGraphicsDefFormat(virBufferPtr buf, >> virDomainGraphicsSpiceMouseModeTypeToString(def->data.spice.mousemode)); >> if (def->data.spice.copypaste) >> virBufferAsprintf(buf, "<clipboard copypaste='%s'/>\n", >> - virDomainGraphicsSpiceClipboardCopypasteTypeToString(def->data.spice.copypaste)); >> + virDomainYesNoTypeToString(def->data.spice.copypaste)); >> 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.