util: xml: Fix confusing semantics of VIR_XML_PROP_OPTIONAL flag

The new enum helpers use a set of flags to modify their behaviour, but
the declared set of flags is semantically confusing:

 typedef enum {
     VIR_XML_PROP_OPTIONAL = 0, /* Attribute may be absent */
     VIR_XML_PROP_REQUIRED = 1 << 0, /* Attribute may not be absent */

Since VIR_XML_PROP_OPTIONAL is declared as 0 any other flag shadows it
and makes it impossible to detect. The functions are not able to detect
a semantic nonsense of VIR_XML_PROP_OPTIONAL | VIR_XML_PROP_REQUIRED and
it's a perfectly valid statement for the compilers.

In general having two flags to do the same boolean don't make sense and
the implementation doesn't fix any shortcomings either.

To prevent mistakes, rename VIR_XML_PROP_OPTIONAL to VIR_XML_PROP_NONE,
so that there's always an enum value used with the calls but it doesn't
imply that the flag makes the property optional when the actual value is
0.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
index 58ca1e20190f7f9fd905006f29727b8d2e8dd08e..58d04df1b8a7bb8f59b4029e436b3134597850d4 100644 (file)
--- a/src/conf/cpu_conf.c
+++ b/src/conf/cpu_conf.c
@@ -434,7 +434,7 @@ virCPUDefParseXML(xmlXPathContextPtr ctxt,
         }
 
         if (virXMLPropEnum(ctxt->node, "check", virCPUCheckTypeFromString,
-                           VIR_XML_PROP_OPTIONAL, &def->check) < 0)
+                           VIR_XML_PROP_NONE, &def->check) < 0)
             return -1;
     }
 

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index ac0386a2d1d6e15e7705fa1eeb4a0db2319e950d..9a0d1f9285fdd191be1e37f00095060b8c604146 100644 (file)
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -9337,28 +9337,28 @@ virDomainDiskDefParseXML(virDomainXMLOption *xmlopt,
     def->device = VIR_DOMAIN_DISK_DEVICE_DISK;
 
     if (virXMLPropEnum(node, "device", virDomainDiskDeviceTypeFromString,
-                       VIR_XML_PROP_OPTIONAL, &def->device) < 0)
+                       VIR_XML_PROP_NONE, &def->device) < 0)
         return NULL;
 
     if (virXMLPropEnum(node, "model", virDomainDiskModelTypeFromString,
-                       VIR_XML_PROP_OPTIONAL, &def->model) < 0)
+                       VIR_XML_PROP_NONE, &def->model) < 0)
         return NULL;
 
     if (virXMLPropEnum(node, "snapshot", virDomainSnapshotLocationTypeFromString,
-                       VIR_XML_PROP_OPTIONAL | VIR_XML_PROP_NONZERO, &def->snapshot) < 0)
+                       VIR_XML_PROP_NONZERO, &def->snapshot) < 0)
         return NULL;
 
-    if (virXMLPropTristateBool(node, "rawio", VIR_XML_PROP_OPTIONAL, &def->rawio) < 0)
+    if (virXMLPropTristateBool(node, "rawio", VIR_XML_PROP_NONE, &def->rawio) < 0)
         return NULL;
 
     if (virXMLPropEnum(node, "sgio", virDomainDeviceSGIOTypeFromString,
-                       VIR_XML_PROP_OPTIONAL | VIR_XML_PROP_NONZERO, &def->sgio) < 0)
+                       VIR_XML_PROP_NONZERO, &def->sgio) < 0)
         return NULL;
 
     if ((sourceNode = virXPathNode("./source", ctxt))) {
         if (virXMLPropEnum(sourceNode, "startupPolicy",
                            virDomainStartupPolicyTypeFromString,
-                           VIR_XML_PROP_OPTIONAL | VIR_XML_PROP_NONZERO,
+                           VIR_XML_PROP_NONZERO,
                            &def->startupPolicy) < 0)
             return NULL;
     }
@@ -9368,19 +9368,19 @@ virDomainDiskDefParseXML(virDomainXMLOption *xmlopt,
 
         if (virXMLPropEnum(targetNode, "bus",
                            virDomainDiskBusTypeFromString,
-                           VIR_XML_PROP_OPTIONAL | VIR_XML_PROP_NONZERO,
+                           VIR_XML_PROP_NONZERO,
                            &def->bus) < 0)
             return NULL;
 
         if (virXMLPropEnum(targetNode, "tray", virDomainDiskTrayTypeFromString,
-                           VIR_XML_PROP_OPTIONAL, &def->tray_status) < 0)
+                           VIR_XML_PROP_NONE, &def->tray_status) < 0)
             return NULL;
 
-        if (virXMLPropTristateSwitch(targetNode, "removable", VIR_XML_PROP_OPTIONAL,
+        if (virXMLPropTristateSwitch(targetNode, "removable", VIR_XML_PROP_NONE,
                                      &def->removable) < 0)
             return NULL;
 
-        if (virXMLPropUInt(targetNode, "rotation_rate", 10, VIR_XML_PROP_OPTIONAL,
+        if (virXMLPropUInt(targetNode, "rotation_rate", 10, VIR_XML_PROP_NONE,
                            &def->rotation_rate) < 0)
             return NULL;
     }
@@ -9391,11 +9391,11 @@ virDomainDiskDefParseXML(virDomainXMLOption *xmlopt,
     }
 
     if ((blockioNode = virXPathNode("./blockio", ctxt))) {
-        if (virXMLPropUInt(blockioNode, "logical_block_size", 10, VIR_XML_PROP_OPTIONAL,
+        if (virXMLPropUInt(blockioNode, "logical_block_size", 10, VIR_XML_PROP_NONE,
                            &def->blockio.logical_block_size) < 0)
             return NULL;
 
-        if (virXMLPropUInt(blockioNode, "physical_block_size", 10, VIR_XML_PROP_OPTIONAL,
+        if (virXMLPropUInt(blockioNode, "physical_block_size", 10, VIR_XML_PROP_NONE,
                            &def->blockio.physical_block_size) < 0)
             return NULL;
     }

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 17835ac8d363c3311931260e4a9160624f6a9296..d6eafa3f575c06d9358ffa3a2976317a06f161a3 100644 (file)
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -1332,7 +1332,7 @@ virNetworkForwardNatDefParseXML(const char *networkName,
         return -1;
     }
 
-    if (virXMLPropTristateBool(node, "ipv6", VIR_XML_PROP_OPTIONAL,
+    if (virXMLPropTristateBool(node, "ipv6", VIR_XML_PROP_NONE,
                                &def->natIPv6) < 0)
         return -1;
 

diff --git a/src/util/virxml.h b/src/util/virxml.h
index f7ec4ef957fe119e851afd190e2102794f602dcc..c83d16a14ae72a1061605a91fcae99ad8ea1d12e 100644 (file)
--- a/src/util/virxml.h
+++ b/src/util/virxml.h
@@ -35,7 +35,7 @@ xmlXPathContextPtr virXMLXPathContextNew(xmlDocPtr xml)
 
 
 typedef enum {
-    VIR_XML_PROP_OPTIONAL = 0, /* Attribute may be absent */
+    VIR_XML_PROP_NONE = 0,
     VIR_XML_PROP_REQUIRED = 1 << 0, /* Attribute may not be absent */
     VIR_XML_PROP_NONZERO = 1 << 1, /* Attribute may not be zero */
 } virXMLPropFlags;