[libvirt PATCH v2 00/10] Refactor more XML parsing boilerplate code
For background, see https://listman.redhat.com/archives/libvir-list/2021-April/msg00668.html Changes since V1: * Rebased * s/VIR_XML_PROP_OPTIONAL/VIR_XML_PROP_NONE/ * Declare variables 1 / line * Commented on strictening of parser in patch #10 Tim Wiederhake (10): virDomainBackupDiskDefParseXML: Use virXMLProp* virDomainBackupDefParse: Use virXMLProp* virZPCIDeviceAddressParseXML: Use virXMLProp* virPCIDeviceAddressParseXML: Use virXMLProp* virDomainDeviceCCWAddressParseXML: Use virXMLProp* virDomainDeviceDriveAddressParseXML: Use virXMLProp* virDomainDeviceVirtioSerialAddressParseXML: Use virXMLProp* virDomainDeviceCcidAddressParseXML: Use virXMLProp* virDomainDeviceUSBAddressParseXML: Use virXMLProp* virInterfaceLinkParseXML: Use virXMLProp* src/conf/backup_conf.c | 57 +++------- src/conf/device_conf.c | 237 ++++++++++++----------------------------- 2 files changed, 80 insertions(+), 214 deletions(-) -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/backup_conf.c | 40 ++++++++++------------------------------ 1 file changed, 10 insertions(+), 30 deletions(-) diff --git a/src/conf/backup_conf.c b/src/conf/backup_conf.c index 7f54a25ff6..e4464b91a0 100644 --- a/src/conf/backup_conf.c +++ b/src/conf/backup_conf.c @@ -106,10 +106,6 @@ virDomainBackupDiskDefParseXML(xmlNodePtr node, g_autofree char *type = NULL; g_autofree char *format = NULL; g_autofree char *idx = NULL; - g_autofree char *backup = NULL; - g_autofree char *state = NULL; - g_autofree char *backupmode = NULL; - int tmp; xmlNodePtr srcNode; unsigned int storageSourceParseFlags = 0; bool internal = flags & VIR_DOMAIN_BACKUP_PARSE_INTERNAL; @@ -127,15 +123,9 @@ virDomainBackupDiskDefParseXML(xmlNodePtr node, def->backup = VIR_TRISTATE_BOOL_YES; - if ((backup = virXMLPropString(node, "backup"))) { - if ((tmp = virTristateBoolTypeFromString(backup)) <= 0) { - virReportError(VIR_ERR_XML_ERROR, - _("invalid disk 'backup' state '%s'"), backup); - return -1; - } - - def->backup = tmp; - } + if (virXMLPropTristateBool(node, "backup", VIR_XML_PROP_NONE, + &def->backup) < 0) + return -1; /* don't parse anything else if backup is disabled */ if (def->backup == VIR_TRISTATE_BOOL_NO) @@ -146,28 +136,18 @@ virDomainBackupDiskDefParseXML(xmlNodePtr node, def->exportbitmap = virXMLPropString(node, "exportbitmap"); } - if ((backupmode = virXMLPropString(node, "backupmode"))) { - if ((tmp = virDomainBackupDiskBackupModeTypeFromString(backupmode)) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("invalid backupmode '%s' of disk '%s'"), - backupmode, def->name); - return -1; - } - - def->backupmode = tmp; - } + if (virXMLPropEnum(node, "backupmode", + virDomainBackupDiskBackupModeTypeFromString, + VIR_XML_PROP_NONE, &def->backupmode) < 0) + return -1; def->incremental = virXMLPropString(node, "incremental"); if (internal) { - if (!(state = virXMLPropString(node, "state")) || - (tmp = virDomainBackupDiskStateTypeFromString(state)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("disk '%s' backup state wrong or missing'"), def->name); + if (virXMLPropEnum(node, "state", + virDomainBackupDiskStateTypeFromString, + VIR_XML_PROP_REQUIRED, &def->state) < 0) return -1; - } - - def->state = tmp; } type = virXMLPropString(node, "type"); -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/backup_conf.c | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-) diff --git a/src/conf/backup_conf.c b/src/conf/backup_conf.c index e4464b91a0..7f176b783f 100644 --- a/src/conf/backup_conf.c +++ b/src/conf/backup_conf.c @@ -222,8 +222,6 @@ virDomainBackupDefParse(xmlXPathContextPtr ctxt, def->incremental = virXPathString("string(./incremental)", ctxt); if ((node = virXPathNode("./server", ctxt))) { - g_autofree char *tls = NULL; - if (def->type != VIR_DOMAIN_BACKUP_TYPE_PULL) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("use of <server> requires pull mode backup")); @@ -249,18 +247,9 @@ virDomainBackupDefParse(xmlXPathContextPtr ctxt, return NULL; } - if ((tls = virXMLPropString(node, "tls"))) { - int tmp; - - if ((tmp = virTristateBoolTypeFromString(tls)) <= 0) { - virReportError(VIR_ERR_XML_ERROR, - _("unknown value '%s' of 'tls' attribute"),\ - tls); - return NULL; - } - - def->tls = tmp; - } + if (virXMLPropTristateBool(node, "tls", VIR_XML_PROP_NONE, + &def->tls) < 0) + return NULL; } if ((n = virXPathNodeSet("./disks/*", ctxt, &nodes)) < 0) -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/device_conf.c | 34 ++++++++++++---------------------- 1 file changed, 12 insertions(+), 22 deletions(-) diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index 6a4b14cfda..9abbd5ebb7 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -52,32 +52,22 @@ static int virZPCIDeviceAddressParseXML(xmlNodePtr node, virPCIDeviceAddress *addr) { - virZPCIDeviceAddress def = { .uid = { 0 }, .fid = { 0 } }; - g_autofree char *uid = NULL; - g_autofree char *fid = NULL; + int retUid; + int retFid; - uid = virXMLPropString(node, "uid"); - fid = virXMLPropString(node, "fid"); + if ((retUid = virXMLPropUInt(node, "uid", 0, VIR_XML_PROP_NONE, + &addr->zpci.uid.value)) < 0) + return -1; - if (uid) { - if (virStrToLong_uip(uid, NULL, 0, &def.uid.value) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot parse <address> 'uid' attribute")); - return -1; - } - def.uid.isSet = true; - } + if (retUid > 0) + addr->zpci.uid.isSet = true; - if (fid) { - if (virStrToLong_uip(fid, NULL, 0, &def.fid.value) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot parse <address> 'fid' attribute")); - return -1; - } - def.fid.isSet = true; - } + if ((retFid = virXMLPropUInt(node, "fid", 0, VIR_XML_PROP_NONE, + &addr->zpci.fid.value)) < 0) + return -1; - addr->zpci = def; + if (retFid > 0) + addr->zpci.fid.isSet = true; return 0; } -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/device_conf.c | 47 +++++++++++------------------------------- 1 file changed, 12 insertions(+), 35 deletions(-) diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index 9abbd5ebb7..801552a9cf 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -200,52 +200,29 @@ virPCIDeviceAddressParseXML(xmlNodePtr node, { xmlNodePtr cur; xmlNodePtr zpci = NULL; - g_autofree char *domain = virXMLPropString(node, "domain"); - g_autofree char *bus = virXMLPropString(node, "bus"); - g_autofree char *slot = virXMLPropString(node, "slot"); - g_autofree char *function = virXMLPropString(node, "function"); - g_autofree char *multi = virXMLPropString(node, "multifunction"); memset(addr, 0, sizeof(*addr)); - if (domain && - virStrToLong_uip(domain, NULL, 0, &addr->domain) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot parse <address> 'domain' attribute")); + if (virXMLPropUInt(node, "domain", 0, VIR_XML_PROP_NONE, + &addr->domain) < 0) return -1; - } - if (bus && - virStrToLong_uip(bus, NULL, 0, &addr->bus) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot parse <address> 'bus' attribute")); + if (virXMLPropUInt(node, "bus", 0, VIR_XML_PROP_NONE, + &addr->bus) < 0) return -1; - } - if (slot && - virStrToLong_uip(slot, NULL, 0, &addr->slot) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot parse <address> 'slot' attribute")); + if (virXMLPropUInt(node, "slot", 0, VIR_XML_PROP_NONE, + &addr->slot) < 0) return -1; - } - if (function && - virStrToLong_uip(function, NULL, 0, &addr->function) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot parse <address> 'function' attribute")); + if (virXMLPropUInt(node, "function", 0, VIR_XML_PROP_NONE, + &addr->function) < 0) + return -1; + + if (virXMLPropTristateSwitch(node, "multifunction", VIR_XML_PROP_NONE, + &addr->multi) < 0) return -1; - } - if (multi) { - int value; - if ((value = virTristateSwitchTypeFromString(multi)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unknown value '%s' for <address> 'multifunction' attribute"), - multi); - return -1; - } - addr->multi = value; - } if (!virPCIDeviceAddressIsEmpty(addr) && !virPCIDeviceAddressIsValid(addr, true)) return -1; -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/device_conf.c | 53 ++++++++++++++++++------------------------ 1 file changed, 23 insertions(+), 30 deletions(-) diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index 801552a9cf..e527899d7d 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -267,43 +267,36 @@ int virDomainDeviceCCWAddressParseXML(xmlNodePtr node, virDomainDeviceCCWAddress *addr) { - g_autofree char *cssid = virXMLPropString(node, "cssid"); - g_autofree char *ssid = virXMLPropString(node, "ssid"); - g_autofree char *devno = virXMLPropString(node, "devno"); + int cssid; + int ssid; + int devno; memset(addr, 0, sizeof(*addr)); + if ((cssid = virXMLPropUInt(node, "cssid", 0, VIR_XML_PROP_NONE, + &addr->cssid)) < 0) + return -1; + + if ((ssid = virXMLPropUInt(node, "ssid", 0, VIR_XML_PROP_NONE, + &addr->ssid)) < 0) + return -1; + + if ((devno = virXMLPropUInt(node, "devno", 0, VIR_XML_PROP_NONE, + &addr->devno)) < 0) + return -1; + + if (!virDomainDeviceCCWAddressIsValid(addr)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid specification for virtio ccw address: cssid='0x%x' ssid='0x%x' devno='0x%04x'"), + addr->cssid, addr->ssid, addr->devno); + return -1; + } + if (cssid && ssid && devno) { - if (cssid && - virStrToLong_uip(cssid, NULL, 0, &addr->cssid) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot parse <address> 'cssid' attribute")); - return -1; - } - if (ssid && - virStrToLong_uip(ssid, NULL, 0, &addr->ssid) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot parse <address> 'ssid' attribute")); - return -1; - } - if (devno && - virStrToLong_uip(devno, NULL, 0, &addr->devno) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot parse <address> 'devno' attribute")); - return -1; - } - if (!virDomainDeviceCCWAddressIsValid(addr)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Invalid specification for virtio ccw" - " address: cssid='%s' ssid='%s' devno='%s'"), - cssid, ssid, devno); - return -1; - } addr->assigned = true; } else if (cssid || ssid || devno) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Invalid partial specification for virtio ccw" - " address")); + _("Invalid partial specification for virtio ccw address")); return -1; } -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/device_conf.c | 31 ++++++------------------------- 1 file changed, 6 insertions(+), 25 deletions(-) diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index e527899d7d..66b7af109f 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -319,40 +319,21 @@ int virDomainDeviceDriveAddressParseXML(xmlNodePtr node, virDomainDeviceDriveAddress *addr) { - g_autofree char *controller = virXMLPropString(node, "controller"); - g_autofree char *bus = virXMLPropString(node, "bus"); - g_autofree char *target = virXMLPropString(node, "target"); - g_autofree char *unit = virXMLPropString(node, "unit"); - memset(addr, 0, sizeof(*addr)); - if (controller && - virStrToLong_uip(controller, NULL, 10, &addr->controller) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot parse <address> 'controller' attribute")); + if (virXMLPropUInt(node, "controller", 10, VIR_XML_PROP_NONE, + &addr->controller) < 0) return -1; - } - if (bus && - virStrToLong_uip(bus, NULL, 10, &addr->bus) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot parse <address> 'bus' attribute")); + if (virXMLPropUInt(node, "bus", 10, VIR_XML_PROP_NONE, &addr->bus) < 0) return -1; - } - if (target && - virStrToLong_uip(target, NULL, 10, &addr->target) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot parse <address> 'target' attribute")); + if (virXMLPropUInt(node, "target", 10, VIR_XML_PROP_NONE, + &addr->target) < 0) return -1; - } - if (unit && - virStrToLong_uip(unit, NULL, 10, &addr->unit) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot parse <address> 'unit' attribute")); + if (virXMLPropUInt(node, "unit", 10, VIR_XML_PROP_NONE, &addr->unit) < 0) return -1; - } return 0; } -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/device_conf.c | 23 ++++------------------- 1 file changed, 4 insertions(+), 19 deletions(-) diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index 66b7af109f..5360fb301f 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -342,32 +342,17 @@ int virDomainDeviceVirtioSerialAddressParseXML(xmlNodePtr node, virDomainDeviceVirtioSerialAddress *addr) { - g_autofree char *controller = virXMLPropString(node, "controller"); - g_autofree char *bus = virXMLPropString(node, "bus"); - g_autofree char *port = virXMLPropString(node, "port"); - memset(addr, 0, sizeof(*addr)); - if (controller && - virStrToLong_uip(controller, NULL, 10, &addr->controller) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot parse <address> 'controller' attribute")); + if (virXMLPropUInt(node, "controller", 10, VIR_XML_PROP_NONE, + &addr->controller) < 0) return -1; - } - if (bus && - virStrToLong_uip(bus, NULL, 10, &addr->bus) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot parse <address> 'bus' attribute")); + if (virXMLPropUInt(node, "bus", 10, VIR_XML_PROP_NONE, &addr->bus) < 0) return -1; - } - if (port && - virStrToLong_uip(port, NULL, 10, &addr->port) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot parse <address> 'port' attribute")); + if (virXMLPropUInt(node, "port", 10, VIR_XML_PROP_NONE, &addr->port) < 0) return -1; - } return 0; } -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/device_conf.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index 5360fb301f..dbe30b05c1 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -361,24 +361,14 @@ int virDomainDeviceCcidAddressParseXML(xmlNodePtr node, virDomainDeviceCcidAddress *addr) { - g_autofree char *controller = virXMLPropString(node, "controller"); - g_autofree char *slot = virXMLPropString(node, "slot"); - memset(addr, 0, sizeof(*addr)); - if (controller && - virStrToLong_uip(controller, NULL, 10, &addr->controller) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot parse <address> 'controller' attribute")); + if (virXMLPropUInt(node, "controller", 10, VIR_XML_PROP_NONE, + &addr->controller) < 0) return -1; - } - if (slot && - virStrToLong_uip(slot, NULL, 10, &addr->slot) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot parse <address> 'slot' attribute")); + if (virXMLPropUInt(node, "slot", 10, VIR_XML_PROP_NONE, &addr->slot) < 0) return -1; - } return 0; } -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/device_conf.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index dbe30b05c1..9b0b81b2cb 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -401,19 +401,15 @@ virDomainDeviceUSBAddressParseXML(xmlNodePtr node, virDomainDeviceUSBAddress *addr) { g_autofree char *port = virXMLPropString(node, "port"); - g_autofree char *bus = virXMLPropString(node, "bus"); memset(addr, 0, sizeof(*addr)); if (port && virDomainDeviceUSBAddressParsePort(addr, port) < 0) return -1; - if (bus && - virStrToLong_uip(bus, NULL, 10, &addr->bus) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot parse <address> 'bus' attribute")); + if (virXMLPropUInt(node, "bus", 10, VIR_XML_PROP_NONE, &addr->bus) < 0) return -1; - } + return 0; } -- 2.26.3
This strictens the parser to disallow negative values (interpreted as `UINT_MAX + value + 1`) for attribute `speed`, which does not make sense for a value measured in Mbits per second. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/device_conf.c | 25 +++++-------------------- 1 file changed, 5 insertions(+), 20 deletions(-) diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index 9b0b81b2cb..034f072df4 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -465,28 +465,13 @@ int virInterfaceLinkParseXML(xmlNodePtr node, virNetDevIfLink *lnk) { - int state; - - g_autofree char *stateStr = virXMLPropString(node, "state"); - g_autofree char *speedStr = virXMLPropString(node, "speed"); - - if (stateStr) { - if ((state = virNetDevIfStateTypeFromString(stateStr)) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("unknown link state: %s"), - stateStr); - return -1; - } - lnk->state = state; - } + if (virXMLPropEnum(node, "state", virNetDevIfStateTypeFromString, + VIR_XML_PROP_NONE, &lnk->state) < 0) + return -1; - if (speedStr && - virStrToLong_ui(speedStr, NULL, 10, &lnk->speed) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Unable to parse link speed: %s"), - speedStr); + if (virXMLPropUInt(node, "speed", 10, VIR_XML_PROP_NONE, &lnk->speed) < 0) return -1; - } + return 0; } -- 2.26.3
On Wed, Apr 21, 2021 at 14:07:58 +0200, Tim Wiederhake wrote:
For background, see https://listman.redhat.com/archives/libvir-list/2021-April/msg00668.html
Changes since V1: * Rebased * s/VIR_XML_PROP_OPTIONAL/VIR_XML_PROP_NONE/ * Declare variables 1 / line * Commented on strictening of parser in patch #10
Tim Wiederhake (10): virDomainBackupDiskDefParseXML: Use virXMLProp* virDomainBackupDefParse: Use virXMLProp* virZPCIDeviceAddressParseXML: Use virXMLProp* virPCIDeviceAddressParseXML: Use virXMLProp* virDomainDeviceCCWAddressParseXML: Use virXMLProp* virDomainDeviceDriveAddressParseXML: Use virXMLProp* virDomainDeviceVirtioSerialAddressParseXML: Use virXMLProp* virDomainDeviceCcidAddressParseXML: Use virXMLProp* virDomainDeviceUSBAddressParseXML: Use virXMLProp* virInterfaceLinkParseXML: Use virXMLProp*
Pushed now.
participants (2)
-
Peter Krempa -
Tim Wiederhake