[libvirt PATCH v3 00/51] Refactor XML parsing boilerplate code

Tim Wiederhake posted 51 patches 3 years ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20210319155748.680146-1-twiederh@redhat.com
There is a newer version of this series
src/conf/backup_conf.c          |  32 +-
src/conf/device_conf.c          |   8 +-
src/conf/device_conf.h          |   4 +-
src/conf/domain_conf.c          | 794 +++++++-------------------------
src/conf/domain_conf.h          |  28 +-
src/conf/network_conf.c         |  15 +-
src/conf/numa_conf.c            |  14 +-
src/conf/storage_adapter_conf.c |  14 +-
src/conf/storage_adapter_conf.h |   2 +-
src/conf/storage_conf.c         |  17 +-
src/conf/storage_conf.h         |   2 +-
src/conf/storage_source_conf.h  |   4 +-
src/libvirt_private.syms        |   2 +
src/qemu/qemu_command.c         |   3 +-
src/qemu/qemu_hotplug.c         |   2 +-
src/util/virpci.h               |   2 +-
src/util/virxml.c               |  82 ++++
src/util/virxml.h               |   9 +
18 files changed, 301 insertions(+), 733 deletions(-)
[libvirt PATCH v3 00/51] Refactor XML parsing boilerplate code
Posted by Tim Wiederhake 3 years ago
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.

V1: https://listman.redhat.com/archives/libvir-list/2021-March/msg00853.html
V2: https://listman.redhat.com/archives/libvir-list/2021-March/msg00994.html

Changes since V2:
* Fixed the -Wtautological-unsigned-enum-zero-compare issues in the first
  part of the series. These issues were solved later in the series, but
  this change makes it easier to bisect in the future.

I was thinking about only re-sending the first couple of patches, but the
latter part would have endet up with quite a few conflicts, so I am sending
it in its entirety, again. Sorry for the spam!

Cheers,
Tim

Tim Wiederhake (51):
  conf: Use virTristateXXX in virStorageSource
  conf: Use virTristateXXX in virStorageSourceNVMeDef
  conf: Use virTristateXXX in virDomainDeviceInfo
  conf: Use virTristateXXX in virDomainDiskDef
  conf: Use virTristateXXX in virDomainActualNetDef
  conf: Use virTristateXXX in virDomainNetDef
  conf: Use virTristateXXX in virDomainChrSourceDef
  conf: Use virTristateXXX in virDomainGraphicsDef
  conf: Use virTristateXXX in virDomainMemballoonDef
  conf: Use virTristateXXX in virDomainLoaderDef
  conf: Use virTristateXXX in virDomainDef
  conf: Use virTristateXXX in virStorageAdapterFCHost
  conf: Use virTristateXXX in virStoragePoolSourceDevice
  conf: Use virTristateXXX in virPCIDeviceAddress
  virxml: Add virXMLPropTristateBool
  virxml: Add virXMLPropTristateSwitch
  domain_conf: Use virXMLPropTristateXXX in
    virDomainKeyWrapCipherDefParseXML
  domain_conf: Use virXMLPropTristateXXX in
    virDomainVirtioOptionsParseXML
  domain_conf: Use virXMLPropTristateXXX in virDomainDeviceInfoParseXML
  domain_conf: Use virXMLPropTristateXXX in
    virDomainDiskSourceNetworkParse
  domain_conf: Use virXMLPropTristateXXX in virDomainDiskSourceNVMeParse
  domain_conf: Use virXMLPropTristateXXX in
    virDomainDiskDefDriverParseXML
  domain_conf: Use virXMLPropTristateXXX in
    virDomainActualNetDefParseXML
  domain_conf: Use virXMLPropTristateXXX in
    virDomainChrSourceReconnectDefParseXML
  domain_conf: Use virXMLPropTristateXXX in virDomainNetDefParseXML
  domain_conf: Use virXMLPropTristateXXX in
    virDomainChrSourceDefParseTCP
  domain_conf: Use virXMLPropTristateXXX in
    virDomainChrSourceDefParseFile
  domain_conf: Use virXMLPropTristateXXX in
    virDomainChrSourceDefParseLog
  domain_conf: Use virXMLPropTristateXXX in
    virDomainGraphicsDefParseXMLVNC
  domain_conf: Use virXMLPropTristateXXX in
    virDomainGraphicsDefParseXMLSDL
  domain_conf: Use virXMLPropTristateXXX in
    virDomainGraphicsDefParseXMLSpice
  domain_conf: Use virXMLPropTristateXXX in virDomainAudioCommonParse
  domain_conf: Use virXMLPropTristateXXX in virDomainAudioJackParse
  domain_conf: Use virXMLPropTristateXXX in virDomainAudioOSSParse
  domain_conf: Use virXMLPropTristateXXX in virDomainAudioDefParseXML
  domain_conf: Use virXMLPropTristateXXX in
    virDomainMemballoonDefParseXML
  domain_conf: Use virXMLPropTristateXXX in virDomainShmemDefParseXML
  domain_conf: Use virXMLPropTristateXXX in
    virDomainPerfEventDefParseXML
  domain_conf: Use virXMLPropTristateXXX in virDomainMemoryDefParseXML
  domain_conf: Use virXMLPropTristateXXX in virDomainIOMMUDefParseXML
  domain_conf: Use virXMLPropTristateXXX in virDomainVsockDefParseXML
  domain_conf: Use virXMLPropTristateXXX in virDomainFeaturesDefParse
  domain_conf: Use virXMLPropTristateXXX in virDomainLoaderDefParseXML
  domain_conf: Use virXMLPropTristateXXX in virDomainVcpuParse
  backup_conf: Use virXMLPropTristateXXX in
    virDomainBackupDiskDefParseXML
  backup_conf: Use virXMLPropTristateXXX in virDomainBackupDefParse
  device_conf: Use virXMLPropTristateXXX in virPCIDeviceAddressParseXML
  network_conf: Use virXMLPropTristateXXX in
    virNetworkForwardNatDefParseXML
  numa_conf: Use virXMLPropTristateXXX in virDomainNumaDefParseXML
  storage_adapter_conf: Use virXMLPropTristateXXX in
    virStorageAdapterParseXMLFCHost
  storage_conf: Use virXMLPropTristateXXX in
    virStoragePoolDefParseSource

 src/conf/backup_conf.c          |  32 +-
 src/conf/device_conf.c          |   8 +-
 src/conf/device_conf.h          |   4 +-
 src/conf/domain_conf.c          | 794 +++++++-------------------------
 src/conf/domain_conf.h          |  28 +-
 src/conf/network_conf.c         |  15 +-
 src/conf/numa_conf.c            |  14 +-
 src/conf/storage_adapter_conf.c |  14 +-
 src/conf/storage_adapter_conf.h |   2 +-
 src/conf/storage_conf.c         |  17 +-
 src/conf/storage_conf.h         |   2 +-
 src/conf/storage_source_conf.h  |   4 +-
 src/libvirt_private.syms        |   2 +
 src/qemu/qemu_command.c         |   3 +-
 src/qemu/qemu_hotplug.c         |   2 +-
 src/util/virpci.h               |   2 +-
 src/util/virxml.c               |  82 ++++
 src/util/virxml.h               |   9 +
 18 files changed, 301 insertions(+), 733 deletions(-)

-- 
2.26.2


Re: [libvirt PATCH v3 00/51] Refactor XML parsing boilerplate code
Posted by Michal Privoznik 3 years ago
On 3/19/21 4:56 PM, 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.
> 
> V1: https://listman.redhat.com/archives/libvir-list/2021-March/msg00853.html
> V2: https://listman.redhat.com/archives/libvir-list/2021-March/msg00994.html
> 
> Changes since V2:
> * Fixed the -Wtautological-unsigned-enum-zero-compare issues in the first
>    part of the series. These issues were solved later in the series, but
>    this change makes it easier to bisect in the future.
> 
> I was thinking about only re-sending the first couple of patches, but the
> latter part would have endet up with quite a few conflicts, so I am sending
> it in its entirety, again. Sorry for the spam!
> 
> Cheers,
> Tim

Alternatively, rewrite to virXMLPropTristateXXX() could have gone in the 
first, followed by change of struct members to virTristateXXX which 
would have produced smalled diff. But I guess it doesn't matter.

> 
> Tim Wiederhake (51):
>    conf: Use virTristateXXX in virStorageSource
>    conf: Use virTristateXXX in virStorageSourceNVMeDef
>    conf: Use virTristateXXX in virDomainDeviceInfo
>    conf: Use virTristateXXX in virDomainDiskDef
>    conf: Use virTristateXXX in virDomainActualNetDef
>    conf: Use virTristateXXX in virDomainNetDef
>    conf: Use virTristateXXX in virDomainChrSourceDef
>    conf: Use virTristateXXX in virDomainGraphicsDef
>    conf: Use virTristateXXX in virDomainMemballoonDef
>    conf: Use virTristateXXX in virDomainLoaderDef
>    conf: Use virTristateXXX in virDomainDef
>    conf: Use virTristateXXX in virStorageAdapterFCHost
>    conf: Use virTristateXXX in virStoragePoolSourceDevice
>    conf: Use virTristateXXX in virPCIDeviceAddress
>    virxml: Add virXMLPropTristateBool
>    virxml: Add virXMLPropTristateSwitch
>    domain_conf: Use virXMLPropTristateXXX in
>      virDomainKeyWrapCipherDefParseXML
>    domain_conf: Use virXMLPropTristateXXX in
>      virDomainVirtioOptionsParseXML
>    domain_conf: Use virXMLPropTristateXXX in virDomainDeviceInfoParseXML
>    domain_conf: Use virXMLPropTristateXXX in
>      virDomainDiskSourceNetworkParse
>    domain_conf: Use virXMLPropTristateXXX in virDomainDiskSourceNVMeParse
>    domain_conf: Use virXMLPropTristateXXX in
>      virDomainDiskDefDriverParseXML
>    domain_conf: Use virXMLPropTristateXXX in
>      virDomainActualNetDefParseXML
>    domain_conf: Use virXMLPropTristateXXX in
>      virDomainChrSourceReconnectDefParseXML
>    domain_conf: Use virXMLPropTristateXXX in virDomainNetDefParseXML
>    domain_conf: Use virXMLPropTristateXXX in
>      virDomainChrSourceDefParseTCP
>    domain_conf: Use virXMLPropTristateXXX in
>      virDomainChrSourceDefParseFile
>    domain_conf: Use virXMLPropTristateXXX in
>      virDomainChrSourceDefParseLog
>    domain_conf: Use virXMLPropTristateXXX in
>      virDomainGraphicsDefParseXMLVNC
>    domain_conf: Use virXMLPropTristateXXX in
>      virDomainGraphicsDefParseXMLSDL
>    domain_conf: Use virXMLPropTristateXXX in
>      virDomainGraphicsDefParseXMLSpice
>    domain_conf: Use virXMLPropTristateXXX in virDomainAudioCommonParse
>    domain_conf: Use virXMLPropTristateXXX in virDomainAudioJackParse
>    domain_conf: Use virXMLPropTristateXXX in virDomainAudioOSSParse
>    domain_conf: Use virXMLPropTristateXXX in virDomainAudioDefParseXML
>    domain_conf: Use virXMLPropTristateXXX in
>      virDomainMemballoonDefParseXML
>    domain_conf: Use virXMLPropTristateXXX in virDomainShmemDefParseXML
>    domain_conf: Use virXMLPropTristateXXX in
>      virDomainPerfEventDefParseXML
>    domain_conf: Use virXMLPropTristateXXX in virDomainMemoryDefParseXML
>    domain_conf: Use virXMLPropTristateXXX in virDomainIOMMUDefParseXML
>    domain_conf: Use virXMLPropTristateXXX in virDomainVsockDefParseXML
>    domain_conf: Use virXMLPropTristateXXX in virDomainFeaturesDefParse
>    domain_conf: Use virXMLPropTristateXXX in virDomainLoaderDefParseXML
>    domain_conf: Use virXMLPropTristateXXX in virDomainVcpuParse
>    backup_conf: Use virXMLPropTristateXXX in
>      virDomainBackupDiskDefParseXML
>    backup_conf: Use virXMLPropTristateXXX in virDomainBackupDefParse
>    device_conf: Use virXMLPropTristateXXX in virPCIDeviceAddressParseXML
>    network_conf: Use virXMLPropTristateXXX in
>      virNetworkForwardNatDefParseXML
>    numa_conf: Use virXMLPropTristateXXX in virDomainNumaDefParseXML
>    storage_adapter_conf: Use virXMLPropTristateXXX in
>      virStorageAdapterParseXMLFCHost
>    storage_conf: Use virXMLPropTristateXXX in
>      virStoragePoolDefParseSource
> 
>   src/conf/backup_conf.c          |  32 +-
>   src/conf/device_conf.c          |   8 +-
>   src/conf/device_conf.h          |   4 +-
>   src/conf/domain_conf.c          | 794 +++++++-------------------------
>   src/conf/domain_conf.h          |  28 +-
>   src/conf/network_conf.c         |  15 +-
>   src/conf/numa_conf.c            |  14 +-
>   src/conf/storage_adapter_conf.c |  14 +-
>   src/conf/storage_adapter_conf.h |   2 +-
>   src/conf/storage_conf.c         |  17 +-
>   src/conf/storage_conf.h         |   2 +-
>   src/conf/storage_source_conf.h  |   4 +-
>   src/libvirt_private.syms        |   2 +
>   src/qemu/qemu_command.c         |   3 +-
>   src/qemu/qemu_hotplug.c         |   2 +-
>   src/util/virpci.h               |   2 +-
>   src/util/virxml.c               |  82 ++++
>   src/util/virxml.h               |   9 +
>   18 files changed, 301 insertions(+), 733 deletions(-)
> 

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Will push at the end of day, to give others chance to object.

Michal