[v2,1/4] util: xml: add virXMLPropertyCount

Message ID 91732e3293e52a92698aff3a474cd29bdb8249ef.1463339181.git.crobinso@redhat.com
State New
Headers show

Commit Message

Cole Robinson May 15, 2016, 7:11 p.m.
Returns an integer count of the number of XML properties an element
has. Will be used in future patches.
---
 src/libvirt_private.syms |  1 +
 src/util/virxml.c        | 17 +++++++++++++++++
 src/util/virxml.h        |  1 +
 3 files changed, 19 insertions(+)

-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Comments

Cole Robinson May 16, 2016, 2:51 p.m. | #1
On 05/16/2016 10:39 AM, Peter Krempa wrote:
> On Sun, May 15, 2016 at 15:11:26 -0400, Cole Robinson wrote:

>> Returns an integer count of the number of XML properties an element

>> has. Will be used in future patches.

>> ---

>>  src/libvirt_private.syms |  1 +

>>  src/util/virxml.c        | 17 +++++++++++++++++

>>  src/util/virxml.h        |  1 +

>>  3 files changed, 19 insertions(+)

> 

> [...]

> 

>> diff --git a/src/util/virxml.c b/src/util/virxml.c

>> index 489bad8..86c021c 100644

>> --- a/src/util/virxml.c

>> +++ b/src/util/virxml.c

>> @@ -890,6 +890,23 @@ virXMLChildElementCount(xmlNodePtr node)

>>      return ret;

>>  }

>>  

>> +/* Returns the number of node's properties, or -1 on error.  */

>> +long

>> +virXMLPropertyCount(xmlNodePtr node)

>> +{

>> +    long ret = 0;

>> +    xmlAttrPtr cur = NULL;

>> +

>> +    if (!node || node->type != XML_ELEMENT_NODE)

>> +        return -1;

>> +    cur = node->properties;

>> +    while (cur) {

>> +        ret++;

>> +        cur = cur->next;

>> +    }

>> +    return ret;

>> +}

> 

> Function like this looks rather fishy. Checking according to the count

> of attributes is very likely to break. I'd rather see us checking

> explicitly that certain attributes are present or not.

> 

> I think this will make the code more fragile in the long therm with

> little gain in the short term.

> 


The pattern could be abused, but the way it's used in the next patch is to
check type=pci and count=1, so I don't see how that is fragile in this
context. Maybe if PCI addresses grew sub elements one day we might do the
wrong thing... I could add a check for that too.

IMO what _is_ fragile is trying to check

if (!domain &&
    !bus &&
    !slot &&
    !function &&
    !multi)
    info->auto_allocate = true;

since a new property added in the future may forget to update that list. Plus
the only clear place to put that is in PCIDeviceAddressParseXML which is used
in several other places that explicitly do _not_ deal with auto_allocate, so
it could warrant an extra argument to the function, or more logic at the call
sites to reject auto_allocate.

I'm certainly not in love with the approach but I can't think of anything
cleaner at the moment...

Thanks,
Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patch hide | download patch | download mbox

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index fb24808..40b70a1 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2550,6 +2550,7 @@  virXMLExtractNamespaceXML;
 virXMLNodeToString;
 virXMLParseHelper;
 virXMLPickShellSafeComment;
+virXMLPropertyCount;
 virXMLPropString;
 virXMLSaveFile;
 virXMLValidateAgainstSchema;
diff --git a/src/util/virxml.c b/src/util/virxml.c
index 489bad8..86c021c 100644
--- a/src/util/virxml.c
+++ b/src/util/virxml.c
@@ -890,6 +890,23 @@  virXMLChildElementCount(xmlNodePtr node)
     return ret;
 }
 
+/* Returns the number of node's properties, or -1 on error.  */
+long
+virXMLPropertyCount(xmlNodePtr node)
+{
+    long ret = 0;
+    xmlAttrPtr cur = NULL;
+
+    if (!node || node->type != XML_ELEMENT_NODE)
+        return -1;
+    cur = node->properties;
+    while (cur) {
+        ret++;
+        cur = cur->next;
+    }
+    return ret;
+}
+
 
 /**
  * virXMLNodeToString: convert an XML node ptr to an XML string
diff --git a/src/util/virxml.h b/src/util/virxml.h
index b94de74..5cf082e 100644
--- a/src/util/virxml.h
+++ b/src/util/virxml.h
@@ -72,6 +72,7 @@  int              virXPathNodeSet(const char *xpath,
 char *          virXMLPropString(xmlNodePtr node,
                                  const char *name);
 long     virXMLChildElementCount(xmlNodePtr node);
+long     virXMLPropertyCount(xmlNodePtr node);
 
 /* Internal function; prefer the macros below.  */
 xmlDocPtr      virXMLParseHelper(int domcode,