Re: [libvirt] [PATCH v2 09/11] Give virDomainDef parser & formatter their own flags

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.

> @@ -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?

ACK with the small issues fixed (I don't want to review this for the second time).