1. Patchew
  2. Libvirt
  3. [PATCH 0/5] Some parsing functions refactor
  4. View patch

[PATCH 1/5] conf: virNetDevVPortProfileParse refactor

Kirill Shchetiniuk via Devel posted 5 patches 5 months, 1 week ago
There is a newer version of this series
[PATCH 1/5] conf: virNetDevVPortProfileParse refactor
Posted by Kirill Shchetiniuk via Devel 5 months, 1 week ago 
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
Re: [PATCH 1/5] conf: virNetDevVPortProfileParse refactor
Posted by Ján Tomko via Devel 5 months ago 
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
Re: [PATCH 1/5] conf: virNetDevVPortProfileParse refactor
Posted by Kirill Shchetiniuk via Devel 5 months ago 
> 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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.