From 45a61cbf68a2459202b7eda4a01a56bfcb6be048 Mon Sep 17 00:00:00 2001 From: Peter Krempa Date: Wed, 21 Apr 2021 08:49:51 +0200 Subject: [PATCH] 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 Reviewed-by: Michal Privoznik --- src/conf/cpu_conf.c | 2 +- src/conf/domain_conf.c | 24 ++++++++++++------------ src/conf/network_conf.c | 2 +- src/util/virxml.h | 2 +- 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index 58ca1e2019..58d04df1b8 100644 --- 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 ac0386a2d1..9a0d1f9285 100644 --- 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 17835ac8d3..d6eafa3f57 100644 --- 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 f7ec4ef957..c83d16a14a 100644 --- 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;