11:03 a.m.
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
11:03 a.m.
New subject: [PATCH 1/5] conf: virNetDevVPortProfileParse refactor
From: Kirill Shchetiniuk <kshcheti(a)redhat.com>
Refactored the virNetDevVPortProfileParse function to use the appropriate
virXMLProp* functions to parse input configuration XML.
Signed-off-by: Kirill Shchetiniuk <kshcheti(a)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
12:22 p.m.
New subject: [PATCH 1/5] conf: virNetDevVPortProfileParse refactor
On a Monday in 2025, Kirill Shchetiniuk via Devel wrote:
From: Kirill Shchetiniuk <kshcheti(a)redhat.com>
Refactored the virNetDevVPortProfileParse function to use the appropriate
virXMLProp* functions to parse input configuration XML.
Signed-off-by: Kirill Shchetiniuk <kshcheti(a)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
6:59 p.m.
New subject: [PATCH 1/5] conf: virNetDevVPortProfileParse refactor
14. 7. 2025 v 19:22, Ján Tomko <jtomko(a)redhat.com>:
On a Monday in 2025, Kirill Shchetiniuk via Devel wrote:
> From: Kirill Shchetiniuk <kshcheti(a)redhat.com>
>
> Refactored the virNetDevVPortProfileParse function to use the appropriate
> virXMLProp* functions to parse input configuration XML.
>
> Signed-off-by: Kirill Shchetiniuk <kshcheti(a)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
11:03 a.m.
New subject: [PATCH 2/5] conf: virDomainHostdevSubsysMediatedDevDefParseXML refactor
From: Kirill Shchetiniuk <kshcheti(a)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(a)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
11:42 a.m.
New subject: [PATCH 2/5] conf: virDomainHostdevSubsysMediatedDevDefParseXML
refactor
On a Monday in 2025, Kirill Shchetiniuk via Devel wrote:
From: Kirill Shchetiniuk <kshcheti(a)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(a)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(a)redhat.com>
and pushed
Jano
11:03 a.m.
New subject: [PATCH 3/5] util: virSecretLookupParseSecret refactor
From: Kirill Shchetiniuk <kshcheti(a)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(a)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
11:03 a.m.
New subject: [PATCH 4/5] conf: virDomainChrDefParseTargetXML refactor
From: Kirill Shchetiniuk <kshcheti(a)redhat.com>
Refactored the default case port option parsing logic to use the
appropriate virXMLPropInt function.
Signed-off-by: Kirill Shchetiniuk <kshcheti(a)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
11:03 a.m.
New subject: [PATCH 5/5] qemu: qemuDomainObjPrivateXMLParseVcpu refactor
From: Kirill Shchetiniuk <kshcheti(a)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(a)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
12:09 p.m.
New subject: [PATCH 5/5] qemu: qemuDomainObjPrivateXMLParseVcpu refactor
On a Monday in 2025, Kirill Shchetiniuk via Devel wrote:
From: Kirill Shchetiniuk <kshcheti(a)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(a)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
6:55 p.m.
New subject: [PATCH 5/5] qemu: qemuDomainObjPrivateXMLParseVcpu refactor
14. 7. 2025 v 19:09, Ján Tomko <jtomko(a)redhat.com>:
On a Monday in 2025, Kirill Shchetiniuk via Devel wrote:
> From: Kirill Shchetiniuk <kshcheti(a)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(a)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
2
days inactive
9
days old
10 comments
2 participants
participants (2)
-
Ján Tomko -
Kirill Shchetiniuk