[PATCH v2 0/4] 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. Changelog fromm v1: 1. 'ret' temp value is now 'rc' 2. code style fixes 3. 'id' param is now parsed as not mandatory Kirill Shchetiniuk (4): conf: virNetDevVPortProfileParse refactor util: virSecretLookupParseSecret refactor conf: virDomainChrDefParseTargetXML refactor qemu: qemuDomainObjPrivateXMLParseVcpu refactor src/conf/domain_conf.c | 16 +--- src/conf/netdev_vport_profile_conf.c | 112 +++++++++++---------------- src/qemu/qemu_domain.c | 16 +--- src/util/virsecret.c | 19 ++--- 4 files changed, 55 insertions(+), 108 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 | 112 +++++++++++---------------- 1 file changed, 44 insertions(+), 68 deletions(-) diff --git a/src/conf/netdev_vport_profile_conf.c b/src/conf/netdev_vport_profile_conf.c index 032a3147d7..92819c2f34 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"); - } - - if (virtPortManagerID) { + int rc; unsigned int val; + g_autofree char *virtPortProfileID = virXMLPropString(parameters, "profileid"); - if (virStrToLong_ui(virtPortManagerID, NULL, 0, &val)) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("cannot parse value of managerid parameter")); + if ((rc = virXMLPropUInt(parameters, "managerid", 10, VIR_XML_PROP_NONE, &val)) < 0) return NULL; + + if (rc > 0) { + if (val > 0xff) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("value of managerid out of range")); + return NULL; + } + + virtPort->managerID = (uint8_t)val; + virtPort->managerID_specified = true; } - if (val > 0xff) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("value of managerid out of range")); + + if ((rc = virXMLPropUInt(parameters, "typeid", 10, VIR_XML_PROP_NONE, &val)) < 0) return NULL; - } - virtPort->managerID = (uint8_t)val; - virtPort->managerID_specified = true; - } - if (virtPortTypeID) { - unsigned int val; + if (rc > 0) { + if (val > 0xffffff) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("value for typeid out of range")); + return NULL; + } - if (virStrToLong_ui(virtPortTypeID, NULL, 0, &val)) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("cannot parse value of typeid parameter")); - return NULL; + virtPort->typeID = (uint32_t)val; + virtPort->typeID_specified = true; } - if (val > 0xffffff) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("value for typeid out of range")); + + if ((rc = virXMLPropUInt(parameters, "typeidversion", 10, VIR_XML_PROP_NONE, &val)) < 0) return NULL; - } - virtPort->typeID = (uint32_t)val; - virtPort->typeID_specified = true; - } - if (virtPortTypeIDVersion) { - unsigned int val; + if (rc > 0) { + if (val > 0xff) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("value of typeidversion out of range")); + return NULL; + } - if (virStrToLong_ui(virtPortTypeIDVersion, NULL, 0, &val)) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("cannot parse value of typeidversion parameter")); - return NULL; + virtPort->typeIDVersion = (uint8_t)val; + virtPort->typeIDVersion_specified = true; } - if (val > 0xff) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("value of typeidversion out of range")); + + if ((rc = virXMLPropUUID(parameters, "instanceid", VIR_XML_PROP_NONE, virtPort->instanceID)) < 0) return NULL; - } - 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")); + if (rc > 0) + virtPort->instanceID_specified = true; + + if ((rc = virXMLPropUUID(parameters, "interfaceid", VIR_XML_PROP_NONE, virtPort->interfaceID)) < 0) return NULL; - } - virtPort->instanceID_specified = true; - } - if (virtPortProfileID && - virStrcpyStatic(virtPort->profileID, virtPortProfileID) < 0) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("profileid parameter too long")); - return NULL; - } + if (rc > 0) + 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 7/22/25 17:12, 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 | 112 +++++++++++---------------- 1 file changed, 44 insertions(+), 68 deletions(-)
diff --git a/src/conf/netdev_vport_profile_conf.c b/src/conf/netdev_vport_profile_conf.c index 032a3147d7..92819c2f34 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"); - } - - if (virtPortManagerID) { + int rc; unsigned int val; + g_autofree char *virtPortProfileID = virXMLPropString(parameters, "profileid");
- if (virStrToLong_ui(virtPortManagerID, NULL, 0, &val)) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("cannot parse value of managerid parameter")); + if ((rc = virXMLPropUInt(parameters, "managerid", 10, VIR_XML_PROP_NONE, &val)) < 0)
We don't really like long lines. Our coding style says there's a limit at 80 characters.
return NULL; + + if (rc > 0) { + if (val > 0xff) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("value of managerid out of range")); + return NULL; + } + + virtPort->managerID = (uint8_t)val;
This seems rather redundant. The type of managerID is already uint8_t. Both compilers that we care about (gcc and clang) handle this typecasting automatically.
+ virtPort->managerID_specified = true; }
Michal
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..d85a563949 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 rc; - 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 ((rc = virXMLPropUUID(secretnode, "uuid", VIR_XML_PROP_NONE, def->u.uuid)) < 0) return -1; - } - if (uuid && usage) { + if (!usage == (rc == 0)) { 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 (rc > 0) { 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
On 7/22/25 17:12, Kirill Shchetiniuk via Devel wrote:
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..d85a563949 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 rc;
- 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 ((rc = virXMLPropUUID(secretnode, "uuid", VIR_XML_PROP_NONE, def->u.uuid)) < 0) return -1; - }
- if (uuid && usage) { + if (!usage == (rc == 0)) {
This works, but it's horrible to read. Since we already ...
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 (rc > 0) {
have different branches when "uuid" attribute was present or not, might as well put corresponding checks here.
def->type = VIR_SECRET_LOOKUP_TYPE_UUID; } else { def->u.usage = g_steal_pointer(&usage); def->type = VIR_SECRET_LOOKUP_TYPE_USAGE; } + return 0; }
Michal
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 | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ba0d4a7b12..49d041706e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10473,7 +10473,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; @@ -10544,20 +10543,9 @@ 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); + /* Set default to negative value to indicate we should set it later */ + 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 54eda9e12f..d580c31653 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2812,28 +2812,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_NONE, &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 7/22/25 17:12, Kirill Shchetiniuk via Devel wrote:
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.
Changelog fromm v1:
1. 'ret' temp value is now 'rc' 2. code style fixes 3. 'id' param is now parsed as not mandatory
Kirill Shchetiniuk (4): conf: virNetDevVPortProfileParse refactor util: virSecretLookupParseSecret refactor conf: virDomainChrDefParseTargetXML refactor qemu: qemuDomainObjPrivateXMLParseVcpu refactor
src/conf/domain_conf.c | 16 +--- src/conf/netdev_vport_profile_conf.c | 112 +++++++++++---------------- src/qemu/qemu_domain.c | 16 +--- src/util/virsecret.c | 19 ++--- 4 files changed, 55 insertions(+), 108 deletions(-)
-- 2.49.0
I'm fixing all the small nits I've raised and merging. Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Kirill Shchetiniuk -
Michal Prívozník