[PATCH 0/5] Some parsing functions refactor
This patch series is supposed to refactor the existing logic of some parsing functions, changing the previous approach of getting the string fisrt and then parsing the string itself. Now the parsing logic is implemented by using the appropriate virXMLProp* functions. In some places the error reporter was changed along with the reported error message and this change was also reflected in exsiting test cases. Kirill Shchetiniuk (5): conf: virNetDevVPortProfileParse refactor conf: virDomainHostdevSubsysMediatedDevDefParseXML refactor util: virSecretLookupParseSecret refactor conf: virDomainChrDefParseTargetXML refactor qemu: qemuDomainObjPrivateXMLParseVcpu refactor src/conf/domain_conf.c | 28 +--- src/conf/netdev_vport_profile_conf.c | 120 +++++++----------- src/qemu/qemu_domain.c | 16 +-- src/util/virsecret.c | 19 +-- ...mdev-src-address-invalid.x86_64-latest.err | 2 +- 5 files changed, 60 insertions(+), 125 deletions(-) -- 2.49.0
From: Kirill Shchetiniuk <kshcheti@redhat.com> Refactored the virNetDevVPortProfileParse function to use the appropriate virXMLProp* functions to parse input configuration XML. Signed-off-by: Kirill Shchetiniuk <kshcheti@redhat.com> --- src/conf/netdev_vport_profile_conf.c | 120 +++++++++++---------------- 1 file changed, 48 insertions(+), 72 deletions(-) diff --git a/src/conf/netdev_vport_profile_conf.c b/src/conf/netdev_vport_profile_conf.c index 032a3147d7..9be19de808 100644 --- a/src/conf/netdev_vport_profile_conf.c +++ b/src/conf/netdev_vport_profile_conf.c @@ -29,12 +29,6 @@ virNetDevVPortProfile * virNetDevVPortProfileParse(xmlNodePtr node, unsigned int flags) { g_autofree char *virtPortType = NULL; - g_autofree char *virtPortManagerID = NULL; - g_autofree char *virtPortTypeID = NULL; - g_autofree char *virtPortTypeIDVersion = NULL; - g_autofree char *virtPortInstanceID = NULL; - g_autofree char *virtPortProfileID = NULL; - g_autofree char *virtPortInterfaceID = NULL; g_autofree virNetDevVPortProfile *virtPort = NULL; xmlNodePtr parameters; @@ -55,88 +49,70 @@ virNetDevVPortProfileParse(xmlNodePtr node, unsigned int flags) } if ((parameters = virXMLNodeGetSubelement(node, "parameters"))) { - virtPortManagerID = virXMLPropString(parameters, "managerid"); - virtPortTypeID = virXMLPropString(parameters, "typeid"); - virtPortTypeIDVersion = virXMLPropString(parameters, "typeidversion"); - virtPortInstanceID = virXMLPropString(parameters, "instanceid"); - virtPortProfileID = virXMLPropString(parameters, "profileid"); - virtPortInterfaceID = virXMLPropString(parameters, "interfaceid"); - } + int ret; + unsigned int tempUInt; + g_autofree char *virtPortProfileID = virXMLPropString(parameters, "profileid"); - if (virtPortManagerID) { - unsigned int val; + if ((ret = virXMLPropUInt(parameters, "managerid", 10, VIR_XML_PROP_NONE, &tempUInt))) { + if (ret < 0) + return NULL; - if (virStrToLong_ui(virtPortManagerID, NULL, 0, &val)) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("cannot parse value of managerid parameter")); - return NULL; - } - if (val > 0xff) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("value of managerid out of range")); - return NULL; + if (tempUInt > 0xff) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("value of managerid out of range")); + return NULL; + } + + virtPort->managerID = (uint8_t)tempUInt; + virtPort->managerID_specified = true; } - virtPort->managerID = (uint8_t)val; - virtPort->managerID_specified = true; - } - if (virtPortTypeID) { - unsigned int val; + if ((ret = virXMLPropUInt(parameters, "typeid", 10, VIR_XML_PROP_NONE, &tempUInt))) { + if (ret < 0) + return NULL; - if (virStrToLong_ui(virtPortTypeID, NULL, 0, &val)) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("cannot parse value of typeid parameter")); - return NULL; - } - if (val > 0xffffff) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("value for typeid out of range")); - return NULL; + if (tempUInt > 0xffffff) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("value for typeid out of range")); + return NULL; + } + virtPort->typeID = (uint32_t)tempUInt; + virtPort->typeID_specified = true; } - virtPort->typeID = (uint32_t)val; - virtPort->typeID_specified = true; - } - if (virtPortTypeIDVersion) { - unsigned int val; + if ((ret = virXMLPropUInt(parameters, "typeidversion", 10, VIR_XML_PROP_NONE, &tempUInt))) { + if (ret < 0) + return NULL; - if (virStrToLong_ui(virtPortTypeIDVersion, NULL, 0, &val)) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("cannot parse value of typeidversion parameter")); - return NULL; - } - if (val > 0xff) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("value of typeidversion out of range")); - return NULL; + if (tempUInt > 0xff) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("value of typeidversion out of range")); + return NULL; + } + virtPort->typeIDVersion = (uint8_t)tempUInt; + virtPort->typeIDVersion_specified = true; } - virtPort->typeIDVersion = (uint8_t)val; - virtPort->typeIDVersion_specified = true; - } - if (virtPortInstanceID) { - if (virUUIDParse(virtPortInstanceID, virtPort->instanceID) < 0) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("cannot parse instanceid parameter as a uuid")); - return NULL; + if ((ret = virXMLPropUUID(parameters, "instanceid", VIR_XML_PROP_NONE, virtPort->instanceID))) { + if (ret < 0) + return NULL; + + virtPort->instanceID_specified = true; } - virtPort->instanceID_specified = true; - } - if (virtPortProfileID && - virStrcpyStatic(virtPort->profileID, virtPortProfileID) < 0) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("profileid parameter too long")); - return NULL; - } + if ((ret = virXMLPropUUID(parameters, "interfaceid", VIR_XML_PROP_NONE, virtPort->interfaceID))) { + if (ret < 0) + return NULL; + + virtPort->interfaceID_specified = true; + } - if (virtPortInterfaceID) { - if (virUUIDParse(virtPortInterfaceID, virtPort->interfaceID) < 0) { + if (virtPortProfileID && + virStrcpyStatic(virtPort->profileID, virtPortProfileID) < 0) { virReportError(VIR_ERR_XML_ERROR, "%s", - _("cannot parse interfaceid parameter as a uuid")); + _("profileid parameter too long")); return NULL; } - virtPort->interfaceID_specified = true; } /* generate default instanceID/interfaceID if appropriate */ -- 2.49.0
On a Monday in 2025, Kirill Shchetiniuk via Devel wrote:
From: Kirill Shchetiniuk <kshcheti@redhat.com>
Refactored the virNetDevVPortProfileParse function to use the appropriate virXMLProp* functions to parse input configuration XML.
Signed-off-by: Kirill Shchetiniuk <kshcheti@redhat.com> --- src/conf/netdev_vport_profile_conf.c | 120 +++++++++++---------------- 1 file changed, 48 insertions(+), 72 deletions(-)
diff --git a/src/conf/netdev_vport_profile_conf.c b/src/conf/netdev_vport_profile_conf.c index 032a3147d7..9be19de808 100644 --- a/src/conf/netdev_vport_profile_conf.c +++ b/src/conf/netdev_vport_profile_conf.c @@ -29,12 +29,6 @@ virNetDevVPortProfile * virNetDevVPortProfileParse(xmlNodePtr node, unsigned int flags) { g_autofree char *virtPortType = NULL; - g_autofree char *virtPortManagerID = NULL; - g_autofree char *virtPortTypeID = NULL; - g_autofree char *virtPortTypeIDVersion = NULL; - g_autofree char *virtPortInstanceID = NULL; - g_autofree char *virtPortProfileID = NULL; - g_autofree char *virtPortInterfaceID = NULL; g_autofree virNetDevVPortProfile *virtPort = NULL; xmlNodePtr parameters;
@@ -55,88 +49,70 @@ virNetDevVPortProfileParse(xmlNodePtr node, unsigned int flags) }
if ((parameters = virXMLNodeGetSubelement(node, "parameters"))) { - virtPortManagerID = virXMLPropString(parameters, "managerid"); - virtPortTypeID = virXMLPropString(parameters, "typeid"); - virtPortTypeIDVersion = virXMLPropString(parameters, "typeidversion"); - virtPortInstanceID = virXMLPropString(parameters, "instanceid"); - virtPortProfileID = virXMLPropString(parameters, "profileid"); - virtPortInterfaceID = virXMLPropString(parameters, "interfaceid"); - } + int ret;
Generally, we use 'ret' as the value that will be returned by the current function, and 'rc' for storing temporary values of the functions we call.
+ unsigned int tempUInt;
There's no need to encode the type in the variable name, the old 'val' was just fine.
+ g_autofree char *virtPortProfileID = virXMLPropString(parameters, "profileid");
- if (virtPortManagerID) { - unsigned int val; + if ((ret = virXMLPropUInt(parameters, "managerid", 10, VIR_XML_PROP_NONE, &tempUInt))) {
Omitting the != 0 should be avoided according to our coding style. https://libvirt.org/coding-style.html#conditional-expressions Moreover, writing this as: if ((rc = virXMLPropUint(...)) < 0) return NULL; if (rc > 0) { .... } is easier to read, IMO
+ if (ret < 0) + return NULL;
Jano
14. 7. 2025 v 19:22, Ján Tomko <jtomko@redhat.com>:
On a Monday in 2025, Kirill Shchetiniuk via Devel wrote:
From: Kirill Shchetiniuk <kshcheti@redhat.com>
Refactored the virNetDevVPortProfileParse function to use the appropriate virXMLProp* functions to parse input configuration XML.
Signed-off-by: Kirill Shchetiniuk <kshcheti@redhat.com> --- src/conf/netdev_vport_profile_conf.c | 120 +++++++++++---------------- 1 file changed, 48 insertions(+), 72 deletions(-)
diff --git a/src/conf/netdev_vport_profile_conf.c b/src/conf/netdev_vport_profile_conf.c index 032a3147d7..9be19de808 100644 --- a/src/conf/netdev_vport_profile_conf.c +++ b/src/conf/netdev_vport_profile_conf.c @@ -29,12 +29,6 @@ virNetDevVPortProfile * virNetDevVPortProfileParse(xmlNodePtr node, unsigned int flags) { g_autofree char *virtPortType = NULL; - g_autofree char *virtPortManagerID = NULL; - g_autofree char *virtPortTypeID = NULL; - g_autofree char *virtPortTypeIDVersion = NULL; - g_autofree char *virtPortInstanceID = NULL; - g_autofree char *virtPortProfileID = NULL; - g_autofree char *virtPortInterfaceID = NULL; g_autofree virNetDevVPortProfile *virtPort = NULL; xmlNodePtr parameters;
@@ -55,88 +49,70 @@ virNetDevVPortProfileParse(xmlNodePtr node, unsigned int flags) }
if ((parameters = virXMLNodeGetSubelement(node, "parameters"))) { - virtPortManagerID = virXMLPropString(parameters, "managerid"); - virtPortTypeID = virXMLPropString(parameters, "typeid"); - virtPortTypeIDVersion = virXMLPropString(parameters, "typeidversion"); - virtPortInstanceID = virXMLPropString(parameters, "instanceid"); - virtPortProfileID = virXMLPropString(parameters, "profileid"); - virtPortInterfaceID = virXMLPropString(parameters, "interfaceid"); - } + int ret;
Generally, we use 'ret' as the value that will be returned by the current function, and 'rc' for storing temporary values of the functions we call.
+ unsigned int tempUInt;
There's no need to encode the type in the variable name, the old 'val' was just fine.
+ g_autofree char *virtPortProfileID = virXMLPropString(parameters, "profileid");
- if (virtPortManagerID) { - unsigned int val; + if ((ret = virXMLPropUInt(parameters, "managerid", 10, VIR_XML_PROP_NONE, &tempUInt))) {
Omitting the != 0 should be avoided according to our coding style. https://libvirt.org/coding-style.html#conditional-expressions
Moreover, writing this as:
if ((rc = virXMLPropUint(...)) < 0) return NULL; if (rc > 0) { .... }
is easier to read, IMO
+ if (ret < 0) + return NULL;
Jano <signature.asc>
Sure, will be fixed within next series, thanks! Kirill
From: Kirill Shchetiniuk <kshcheti@redhat.com> Refactored the virDomainHostdevSubsysMediatedDevDefParseXML function to use virXMLPropUUID fuction instead of getting a string and parsing it later. Due to parsing function change the missing uuid error reporter and message were changed and changed error message was also reflected in tests' outputs. Signed-off-by: Kirill Shchetiniuk <kshcheti@redhat.com> --- src/conf/domain_conf.c | 13 +------------ ...stdev-mdev-src-address-invalid.x86_64-latest.err | 2 +- 2 files changed, 2 insertions(+), 13 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1e24e41a48..bfc62b6270 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6348,7 +6348,6 @@ virDomainHostdevSubsysMediatedDevDefParseXML(virDomainHostdevDef *def, unsigned char uuid[VIR_UUID_BUFLEN] = {0}; xmlNodePtr node = NULL; virDomainHostdevSubsysMediatedDev *mdevsrc = &def->source.subsys.u.mdev; - g_autofree char *uuidxml = NULL; if (!(node = virXPathNode("./source/address", ctxt))) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -6356,18 +6355,8 @@ virDomainHostdevSubsysMediatedDevDefParseXML(virDomainHostdevDef *def, return -1; } - if (!(uuidxml = virXMLPropString(node, "uuid"))) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Missing 'uuid' attribute for element <address>")); - return -1; - } - - if (virUUIDParse(uuidxml, uuid) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", - _("Cannot parse uuid attribute of element <address>")); + if (virXMLPropUUID(node, "uuid", VIR_XML_PROP_REQUIRED, uuid) < 0) return -1; - } virUUIDFormat(uuid, mdevsrc->uuidstr); return 0; diff --git a/tests/qemuxmlconfdata/hostdev-mdev-src-address-invalid.x86_64-latest.err b/tests/qemuxmlconfdata/hostdev-mdev-src-address-invalid.x86_64-latest.err index 20a91e7fa6..29dfbfb1ce 100644 --- a/tests/qemuxmlconfdata/hostdev-mdev-src-address-invalid.x86_64-latest.err +++ b/tests/qemuxmlconfdata/hostdev-mdev-src-address-invalid.x86_64-latest.err @@ -1 +1 @@ -unsupported configuration: Missing 'uuid' attribute for element <address> +XML error: Missing required attribute 'uuid' in element 'address' -- 2.49.0
On a Monday in 2025, Kirill Shchetiniuk via Devel wrote:
From: Kirill Shchetiniuk <kshcheti@redhat.com>
Refactored the virDomainHostdevSubsysMediatedDevDefParseXML function to use virXMLPropUUID fuction instead of getting a string and parsing it later.
Due to parsing function change the missing uuid error reporter and message were changed and changed error message was also reflected in tests' outputs.
Signed-off-by: Kirill Shchetiniuk <kshcheti@redhat.com> --- src/conf/domain_conf.c | 13 +------------ ...stdev-mdev-src-address-invalid.x86_64-latest.err | 2 +- 2 files changed, 2 insertions(+), 13 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> and pushed Jano
From: Kirill Shchetiniuk <kshcheti@redhat.com> Refactored the virSecretLookupParseSecret fucntion to use the virXMLPropUUID fucntion, avoid getting the string and parsing it later. Previously two separate error states merged into one by using boolean NXOR operation. Signed-off-by: Kirill Shchetiniuk <kshcheti@redhat.com> --- src/util/virsecret.c | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/src/util/virsecret.c b/src/util/virsecret.c index 8a220a37ec..c5eb1055be 100644 --- a/src/util/virsecret.c +++ b/src/util/virsecret.c @@ -64,34 +64,27 @@ int virSecretLookupParseSecret(xmlNodePtr secretnode, virSecretLookupTypeDef *def) { - g_autofree char *uuid = NULL; g_autofree char *usage = NULL; + int ret; - uuid = virXMLPropString(secretnode, "uuid"); usage = virXMLPropString(secretnode, "usage"); - if (uuid == NULL && usage == NULL) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing secret uuid or usage attribute")); + + if ((ret = virXMLPropUUID(secretnode, "uuid", VIR_XML_PROP_NONE, def->u.uuid)) < 0) return -1; - } - if (uuid && usage) { + if (!(usage) == !(ret)) { virReportError(VIR_ERR_XML_ERROR, "%s", _("either secret uuid or usage expected")); return -1; } - if (uuid) { - if (virUUIDParse(uuid, def->u.uuid) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("invalid secret uuid '%1$s'"), uuid); - return -1; - } + if (ret) { def->type = VIR_SECRET_LOOKUP_TYPE_UUID; } else { def->u.usage = g_steal_pointer(&usage); def->type = VIR_SECRET_LOOKUP_TYPE_USAGE; } + return 0; } -- 2.49.0
From: Kirill Shchetiniuk <kshcheti@redhat.com> Refactored the default case port option parsing logic to use the appropriate virXMLPropInt function. Signed-off-by: Kirill Shchetiniuk <kshcheti@redhat.com> --- src/conf/domain_conf.c | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bfc62b6270..42e3db5547 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10469,7 +10469,6 @@ virDomainChrDefParseTargetXML(virDomainChrDef *def, g_autofree char *targetType = virXMLPropString(cur, "type"); g_autofree char *targetModel = NULL; g_autofree char *addrStr = NULL; - g_autofree char *portStr = NULL; VIR_XPATH_NODE_AUTORESTORE(ctxt) ctxt->node = cur; @@ -10540,20 +10539,8 @@ virDomainChrDefParseTargetXML(virDomainChrDef *def, break; default: - portStr = virXMLPropString(cur, "port"); - if (portStr == NULL) { - /* Set to negative value to indicate we should set it later */ - def->target.port = -1; - break; - } - - if (virStrToLong_ui(portStr, NULL, 10, &port) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid port number: %1$s"), - portStr); + if (virXMLPropInt(cur, "port", 10, VIR_XML_PROP_NONNEGATIVE, &def->target.port, -1) < 0) return -1; - } - def->target.port = port; break; } -- 2.49.0
From: Kirill Shchetiniuk <kshcheti@redhat.com> Refactored the qemuDomainObjPrivateXMLParseVcpu function to use the appropriate virXMLPropUInt function to parse unsigned integers, avoiding unccessery string parsing operations. Signed-off-by: Kirill Shchetiniuk <kshcheti@redhat.com> --- src/qemu/qemu_domain.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2d62bcc62b..2b505fbddb 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2811,28 +2811,18 @@ qemuDomainObjPrivateXMLParseVcpu(xmlNodePtr node, virDomainDef *def) { virDomainVcpuDef *vcpu; - g_autofree char *idstr = NULL; - g_autofree char *pidstr = NULL; unsigned int tmp; - idstr = virXMLPropString(node, "id"); - - if (idstr && - (virStrToLong_uip(idstr, NULL, 10, &idx) < 0)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse vcpu index '%1$s'"), idstr); + if (virXMLPropUInt(node, "id", 10, VIR_XML_PROP_REQUIRED, &idx) < 0) return -1; - } + if (!(vcpu = virDomainDefGetVcpu(def, idx))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid vcpu index '%1$u'"), idx); return -1; } - if (!(pidstr = virXMLPropString(node, "pid"))) - return -1; - - if (virStrToLong_uip(pidstr, NULL, 10, &tmp) < 0) + if (virXMLPropUInt(node, "pid", 10, VIR_XML_PROP_REQUIRED, &tmp) < 0) return -1; QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->tid = tmp; -- 2.49.0
On a Monday in 2025, Kirill Shchetiniuk via Devel wrote:
From: Kirill Shchetiniuk <kshcheti@redhat.com>
Refactored the qemuDomainObjPrivateXMLParseVcpu function to use the appropriate virXMLPropUInt function to parse unsigned integers, avoiding unccessery string parsing operations.
Signed-off-by: Kirill Shchetiniuk <kshcheti@redhat.com> --- src/qemu/qemu_domain.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2d62bcc62b..2b505fbddb 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2811,28 +2811,18 @@ qemuDomainObjPrivateXMLParseVcpu(xmlNodePtr node, virDomainDef *def) { virDomainVcpuDef *vcpu; - g_autofree char *idstr = NULL; - g_autofree char *pidstr = NULL; unsigned int tmp;
- idstr = virXMLPropString(node, "id"); - - if (idstr && - (virStrToLong_uip(idstr, NULL, 10, &idx) < 0)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse vcpu index '%1$s'"), idstr); + if (virXMLPropUInt(node, "id", 10, VIR_XML_PROP_REQUIRED, &idx) < 0) return -1;
Before, the id attribute was not required. After your change it's mandatory. A refactor should (ideally) not include any changes in behavior. If it's safe to do such change, it should at least be mentioned in the commit message. I did not look into history whether it's safe to make it mandatory, but in case the 'id' XML attribute is mandatory, it's pointless to pass 'idx' as the function argument. Jano
- } + if (!(vcpu = virDomainDefGetVcpu(def, idx))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid vcpu index '%1$u'"), idx); return -1; }
- if (!(pidstr = virXMLPropString(node, "pid"))) - return -1; - - if (virStrToLong_uip(pidstr, NULL, 10, &tmp) < 0) + if (virXMLPropUInt(node, "pid", 10, VIR_XML_PROP_REQUIRED, &tmp) < 0) return -1;
QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->tid = tmp; -- 2.49.0
14. 7. 2025 v 19:09, Ján Tomko <jtomko@redhat.com>:
On a Monday in 2025, Kirill Shchetiniuk via Devel wrote:
From: Kirill Shchetiniuk <kshcheti@redhat.com>
Refactored the qemuDomainObjPrivateXMLParseVcpu function to use the appropriate virXMLPropUInt function to parse unsigned integers, avoiding unccessery string parsing operations.
Signed-off-by: Kirill Shchetiniuk <kshcheti@redhat.com> --- src/qemu/qemu_domain.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2d62bcc62b..2b505fbddb 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2811,28 +2811,18 @@ qemuDomainObjPrivateXMLParseVcpu(xmlNodePtr node, virDomainDef *def) { virDomainVcpuDef *vcpu; - g_autofree char *idstr = NULL; - g_autofree char *pidstr = NULL; unsigned int tmp;
- idstr = virXMLPropString(node, "id"); - - if (idstr && - (virStrToLong_uip(idstr, NULL, 10, &idx) < 0)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse vcpu index '%1$s'"), idstr); + if (virXMLPropUInt(node, "id", 10, VIR_XML_PROP_REQUIRED, &idx) < 0) return -1;
Before, the id attribute was not required. After your change it's mandatory.
A refactor should (ideally) not include any changes in behavior. If it's safe to do such change, it should at least be mentioned in the commit message.
I did not look into history whether it's safe to make it mandatory, but in case the 'id' XML attribute is mandatory, it's pointless to pass 'idx' as the function argument.
Jano
- } + if (!(vcpu = virDomainDefGetVcpu(def, idx))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid vcpu index '%1$u'"), idx); return -1; }
- if (!(pidstr = virXMLPropString(node, "pid"))) - return -1; - - if (virStrToLong_uip(pidstr, NULL, 10, &tmp) < 0) + if (virXMLPropUInt(node, "pid", 10, VIR_XML_PROP_REQUIRED, &tmp) < 0) return -1;
QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->tid = tmp; -- 2.49.0
<signature.asc>
Hi Jano, oh yeah, I see now, I have incorrectly interpreted idstr NULL check as mandatory requirement for id, will fix it within next series, thanks! Kirill
participants (2)
-
Ján Tomko -
Kirill Shchetiniuk