Re: [libvirt PATCH 00/38] Refactor XML parsing boilerplate code

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

On 3/18/21 9:00 AM, Tim Wiederhake wrote:
> This series replaces some recurring boilerplate code in src/conf/ regarding
> the extraction of a virTristate(Switch|Bool) XML attribute.
>
> The boilerplate code looks roughly like this,
>
>    g_autofree char *str = NULL;
>    if (str = virXMLPropString(node, ...)) {
>      int val;
>      if ((val = virTristateBoolTypeFromString(str)) <= 0) {
>        virReportError(...)
>        return -1;
>      }
>      def->... = val;
>    }
>
> with some variations regarding how `str` is free'd in case of later re-use,
> the exact error message for invalid values, whether or not
> `VIR_TRISTATE_(SWITCH|BOOL)_ABSENT` is explicitly checked for (`val < 0` vs.
> `val <= 0`), whether an intermediate variable is used or the value is assigned
> directly, and in some cases the conditions in the two if-blocks are merged.
>
> As a side effect, this makes the error messages for invalid values in these
> attributes much more consistent and catches some instances where e.g.
> `<foo bar="default"/>` would incorrectly be accepted.
>
> Patch #11 (virDomainChrSourceReconnectDefParseXML) is a good example of what
> this refactoring is about.
>
> Tim Wiederhake (38):
>    virStorageAdapterFCHost: Fix comment
>    virxml: Add virXMLPropYesNo
>    virxml: Add virXMLPropOnOff
>    domain_conf: Use virXMLProp(OnOff|YesNo) in
>      virDomainKeyWrapCipherDefParseXML
>    domain_conf: Use virXMLProp(OnOff|YesNo) in
>      virDomainVirtioOptionsParseXML
>    domain_conf: Use virXMLProp(OnOff|YesNo) in
>      virDomainDeviceInfoParseXML
>    domain_conf: Use virXMLProp(OnOff|YesNo) in
>      virDomainDiskSourceNetworkParse
>    domain_conf: Use virXMLProp(OnOff|YesNo) in
>      virDomainDiskSourceNVMeParse
>    domain_conf: Use virXMLProp(OnOff|YesNo) in
>      virDomainDiskDefDriverParseXML
>    domain_conf: Use virXMLProp(OnOff|YesNo) in
>      virDomainActualNetDefParseXML
>    domain_conf: Use virXMLProp(OnOff|YesNo) in
>      virDomainChrSourceReconnectDefParseXML
>    domain_conf: Use virXMLProp(OnOff|YesNo) in virDomainNetDefParseXML
>    domain_conf: Use virXMLProp(OnOff|YesNo) in
>      virDomainChrSourceDefParseTCP
>    domain_conf: Use virXMLProp(OnOff|YesNo) in
>      virDomainChrSourceDefParseFile
>    domain_conf: Use virXMLProp(OnOff|YesNo) in
>      virDomainChrSourceDefParseLog
>    domain_conf: Use virXMLProp(OnOff|YesNo) in
>      virDomainGraphicsDefParseXMLVNC
>    domain_conf: Use virXMLProp(OnOff|YesNo) in
>      virDomainGraphicsDefParseXMLSDL
>    domain_conf: Use virXMLProp(OnOff|YesNo) in
>      virDomainGraphicsDefParseXMLSpice
>    domain_conf: Use virXMLProp(OnOff|YesNo) in virDomainAudioCommonParse
>    domain_conf: Use virXMLProp(OnOff|YesNo) in virDomainAudioJackParse
>    domain_conf: Use virXMLProp(OnOff|YesNo) in virDomainAudioOSSParse
>    domain_conf: Use virXMLProp(OnOff|YesNo) in virDomainAudioDefParseXML
>    domain_conf: Use virXMLProp(OnOff|YesNo) in
>      virDomainMemballoonDefParseXML
>    domain_conf: Use virXMLProp(OnOff|YesNo) in virDomainShmemDefParseXML
>    domain_conf: Use virXMLProp(OnOff|YesNo) in
>      virDomainPerfEventDefParseXML
>    domain_conf: Use virXMLProp(OnOff|YesNo) in virDomainMemoryDefParseXML
>    domain_conf: Use virXMLProp(OnOff|YesNo) in virDomainIOMMUDefParseXML
>    domain_conf: Use virXMLProp(OnOff|YesNo) in virDomainVsockDefParseXML
>    domain_conf: Use virXMLProp(OnOff|YesNo) in virDomainFeaturesDefParse
>    domain_conf: Use virXMLProp(OnOff|YesNo) in virDomainLoaderDefParseXML
>    domain_conf: Use virXMLProp(OnOff|YesNo) in virDomainVcpuParse
>    backup_conf: Use virXMLProp(OnOff|YesNo) in
>      virDomainBackupDiskDefParseXML
>    backup_conf: Use virXMLProp(OnOff|YesNo) in virDomainBackupDefParse
>    device_conf: Use virXMLProp(OnOff|YesNo) in
>      virPCIDeviceAddressParseXML
>    network_conf: Use virXMLProp(OnOff|YesNo) in
>      virNetworkForwardNatDefParseXML
>    numa_conf: Use virXMLProp(OnOff|YesNo) in virDomainNumaDefParseXML
>    storage_adapter_conf: Use virXMLProp(OnOff|YesNo) in
>      virStorageAdapterParseXMLFCHost
>    storage_conf: Use virXMLProp(OnOff|YesNo) in
>      virStoragePoolDefParseSource
>
>   src/conf/backup_conf.c          |  32 +-
>   src/conf/device_conf.c          |  10 +-
>   src/conf/domain_conf.c          | 791 +++++++++-----------------------
>   src/conf/network_conf.c         |  15 +-
>   src/conf/numa_conf.c            |  13 +-
>   src/conf/storage_adapter_conf.c |  17 +-
>   src/conf/storage_adapter_conf.h |   2 +-
>   src/conf/storage_conf.c         |  16 +-
>   src/util/virxml.c               |  74 +++
>   src/util/virxml.h               |   7 +
>   10 files changed, 314 insertions(+), 663 deletions(-)

Reviewed-by: Michal Privoznik <mprivozn@xxxxxxxxxx>

Nice cleanup!

And if you agree with my suggestions in 01-02/38 (suggestion from 02 
affects also 03), I can squash the changes in and push.

Michal