On Thu, Jan 08, 2015 at 15:48:20 +0000, Daniel Berrange wrote:
The virDomainDefParse* and virDomainDefFormat* methods both accept the VIR_DOMAIN_XML_* flags defined in the public API, along with a set of other VIR_DOMAIN_XML_INTERNAL_* flags defined in domain_conf.c.
This is seriously confusing & error prone for a number of reasons:
- VIR_DOMAIN_XML_SECURE, VIR_DOMAIN_XML_MIGRATABLE and VIR_DOMAIN_XML_UPDATE_CPU are only relevant for the formatting operation - Some of the VIR_DOMAIN_XML_INTERNAL_* flags only apply to parse or to format, but not both.
This patch cleanly separates out the flags. There are two distint VIR_DOMAIN_DEF_PARSE_* and VIR_DOMAIN_DEF_FORMAT_* flags that are used by the corresponding methods. The VIR_DOMAIN_XML_* flags received via public API calls must be converted to the VIR_DOMAIN_DEF_FORMAT_* flags where needed.
The various calls to virDomainDefParse which hardcoded the use of the VIR_DOMAIN_XML_INACTIVE flag change to use the VIR_DOMAIN_DEF_PARSE_INACTIVE flag. ... diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1e3bede..4361834 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -86,39 +86,12 @@ struct _virDomainXMLOption {
/* XML namespace callbacks */ virDomainXMLNamespace ns; - }; - - -/* Private flags used internally by virDomainSaveStatus and - * virDomainLoadStatus, in addition to the public virDomainXMLFlags. */ -typedef enum { - /* dump internal domain status information */ - VIR_DOMAIN_XML_INTERNAL_STATUS = 1 << 16, - /* dump/parse <actual> element */ - VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET = 1 << 17, - /* dump/parse original states of host PCI device */ - VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES = 1 << 18, - VIR_DOMAIN_XML_INTERNAL_ALLOW_ROM = 1 << 19, - VIR_DOMAIN_XML_INTERNAL_ALLOW_BOOT = 1 << 20, - VIR_DOMAIN_XML_INTERNAL_CLOCK_ADJUST = 1 << 21, - /* parse only source half of <disk> */ - VIR_DOMAIN_XML_INTERNAL_DISK_SOURCE = 1 << 22, -} virDomainXMLInternalFlags; - -#define DUMPXML_FLAGS \ - (VIR_DOMAIN_XML_SECURE | \ - VIR_DOMAIN_XML_INACTIVE | \ - VIR_DOMAIN_XML_UPDATE_CPU | \ - VIR_DOMAIN_XML_MIGRATABLE) - -verify(((VIR_DOMAIN_XML_INTERNAL_STATUS | - VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET | - VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES | - VIR_DOMAIN_XML_INTERNAL_ALLOW_ROM | - VIR_DOMAIN_XML_INTERNAL_ALLOW_BOOT | - VIR_DOMAIN_XML_INTERNAL_CLOCK_ADJUST | - VIR_DOMAIN_XML_INTERNAL_DISK_SOURCE) - & DUMPXML_FLAGS) == 0); +}; + +#define VIR_DOMAIN_DEF_FORMAT_COMMON_FLAGS \ + (VIR_DOMAIN_DEF_FORMAT_SECURE | \ + VIR_DOMAIN_DEF_FORMAT_INACTIVE | \ + VIR_DOMAIN_DEF_FORMAT_MIGRATABLE)
DUMPXML_FLAGS used to contain VIR_DOMAIN_XML_UPDATE_CPU so you either need to include it in VIR_DOMAIN_DEF_FORMAT_COMMON_FLAGS too or explicitly spell it everywhere VIR_DOMAIN_DEF_FORMAT_COMMON_FLAGS is used in place of DUMPXML_FLAGS.
VIR_ENUM_IMPL(virDomainTaint, VIR_DOMAIN_TAINT_LAST, "custom-argv", ... @@ -19303,8 +19274,7 @@ virDomainDefHasCapabilitiesFeatures(virDomainDefPtr def) return false; }
-/* This internal version can accept VIR_DOMAIN_XML_INTERNAL_*, - * whereas the public version cannot. Also, it appends to an existing +/* This internal version appends to an existing
s/ / / and I'd even reflow the whole paragraph
* buffer (possibly with auto-indent), rather than flattening to string. * Return -1 on failure. */ int @@ -19320,11 +19290,11 @@ virDomainDefFormatInternal(virDomainDefPtr def, bool blkio = false; bool cputune = false;
- virCheckFlags(DUMPXML_FLAGS | - VIR_DOMAIN_XML_INTERNAL_STATUS | - VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET | - VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES | - VIR_DOMAIN_XML_INTERNAL_CLOCK_ADJUST, + virCheckFlags(VIR_DOMAIN_DEF_FORMAT_COMMON_FLAGS | + VIR_DOMAIN_DEF_FORMAT_STATUS | + VIR_DOMAIN_DEF_FORMAT_ACTUAL_NET | + VIR_DOMAIN_DEF_FORMAT_PCI_ORIG_STATES | + VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST, -1);
This check does not allow VIR_DOMAIN_DEF_FORMAT_UPDATE_CPU because it's not explicitly spelled here and VIR_DOMAIN_DEF_FORMAT_COMMON_FLAGS doesn't contain it either.
if (!(type = virDomainVirtTypeToString(def->virtType))) {
...
@@ -19878,7 +19848,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, }
if (virCPUDefFormatBufFull(buf, def->cpu, - !!(flags & VIR_DOMAIN_XML_UPDATE_CPU)) < 0) + !!(flags & VIR_DOMAIN_DEF_FORMAT_UPDATE_CPU)) < 0) goto error;
However, the function is apparently designed to be called with VIR_DOMAIN_DEF_FORMAT_UPDATE_CPU.
virBufferAsprintf(buf, "<clock offset='%s'",
...
@@ -20131,12 +20101,29 @@ virDomainDefFormatInternal(virDomainDefPtr def, return -1; }
+int virDomainDefFormatConvertXMLFlags(unsigned int flags) +{ + int formatFlags = 0;
The function should return unsigned int and formatFlags should be unsigned too.
+ + if (flags & VIR_DOMAIN_XML_SECURE) + formatFlags |= VIR_DOMAIN_DEF_FORMAT_SECURE; + if (flags & VIR_DOMAIN_XML_INACTIVE) + formatFlags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE; + if (flags & VIR_DOMAIN_XML_UPDATE_CPU) + formatFlags |= VIR_DOMAIN_DEF_FORMAT_UPDATE_CPU; + if (flags & VIR_DOMAIN_XML_MIGRATABLE) + formatFlags |= VIR_DOMAIN_DEF_FORMAT_MIGRATABLE; + + return formatFlags; +} + + char * virDomainDefFormat(virDomainDefPtr def, unsigned int flags) { virBuffer buf = VIR_BUFFER_INITIALIZER;
- virCheckFlags(DUMPXML_FLAGS, NULL); + virCheckFlags(VIR_DOMAIN_DEF_FORMAT_COMMON_FLAGS, NULL);
This will refuse VIR_DOMAIN_DEF_FORMAT_UPDATE_CPU.
if (virDomainDefFormatInternal(def, flags, &buf) < 0) return NULL;
...
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ac1f4f8..00ebdf8 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h ... @@ -2467,6 +2498,8 @@ bool virDomainDefCheckABIStability(virDomainDefPtr src,
int virDomainDefAddImplicitControllers(virDomainDefPtr def);
+int virDomainDefFormatConvertXMLFlags(unsigned int flags);
unsigned int
+ char *virDomainDefFormat(virDomainDefPtr def, unsigned int flags); int virDomainDefFormatInternal(virDomainDefPtr def, ... diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3d4023c..c4a9508 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c ... @@ -2160,7 +2162,8 @@ qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm,
virUUIDFormat(vm->def->uuid, uuidstr); newxml = virDomainSnapshotDefFormat(uuidstr, snapshot->def, - QEMU_DOMAIN_FORMAT_LIVE_FLAGS, 1); + VIR_DOMAIN_DEF_FORMAT_SECURE | + VIR_DOMAIN_DEF_FORMAT_UPDATE_CPU, 1);
What about virDomainDefFormatConvertXMLFlags(QEMU_DOMAIN_FORMAT_LIVE_FLAGS) to make sure we still use the right flags in case QEMU_DOMAIN_FORMAT_LIVE_FLAGS is ever changed?
if (newxml == NULL) return -1;
... ACK with the small issues fixed (I don't want to review this for the second time). Jirka