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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list