On 10/30/2017 04:51 AM, Kothapally Madhu Pavan wrote:
> This patch will allow user to edit the inactive XML snapshot
> configuration when it is available in the snapshot.
>
> Signed-off-by: Kothapally Madhu Pavan <kmp@xxxxxxxxxxxxxxxxxx>
> ---
> include/libvirt/libvirt-domain.h | 1 +
> src/conf/domain_conf.c | 6 ++++--
> src/conf/domain_conf.h | 2 ++
> src/conf/snapshot_conf.c | 35 ++++++++++++++++++++++++++++++++++-
> src/qemu/qemu_driver.c | 3 ++-
> 5 files changed, 43 insertions(+), 4 deletions(-)
>
> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index 4048acf..e70c664 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -1546,6 +1546,7 @@ typedef enum {
> VIR_DOMAIN_XML_INACTIVE = (1 << 1), /* dump inactive domain information */
> VIR_DOMAIN_XML_UPDATE_CPU = (1 << 2), /* update guest CPU requirements according to host CPU */
> VIR_DOMAIN_XML_MIGRATABLE = (1 << 3), /* dump XML suitable for migration */
> + VIR_DOMAIN_XML_ACTIVE_ONLY = (1 << 4), /* dump active XML and avoid inactive XML in snapshot */
Not liking the name - makes me wonder why the negation of the existing
VIR_DOMAIN_XML_INACTIVE wasn't used in some way...
It doesn't scream use this only for SNAPSHOT manipulation.
Consider: VIR_DOMAIN_XML_SNAPSHOT_ACTIVE
> } virDomainXMLFlags;
>
> char * virDomainGetXMLDesc (virDomainPtr domain,
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 77c20c6..36cebe5 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -25687,8 +25687,8 @@ virDomainDefFormatInternal(virDomainDefPtr def,
> 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);
> + VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST |
> + VIR_DOMAIN_DEF_FORMAT_ACTIVE_ONLY, -1);
>
> if (!(type = virDomainVirtTypeToString(def->virtType))) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -26472,6 +26472,8 @@ unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags)
> formatFlags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE;
> if (flags & VIR_DOMAIN_XML_MIGRATABLE)
> formatFlags |= VIR_DOMAIN_DEF_FORMAT_MIGRATABLE;
> + if (flags & VIR_DOMAIN_XML_ACTIVE_ONLY)
> + formatFlags |= VIR_DOMAIN_DEF_FORMAT_ACTIVE_ONLY;
>
> return formatFlags;
> }
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 38de70b..0659220 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2853,6 +2853,8 @@ typedef enum {
> VIR_DOMAIN_DEF_FORMAT_ALLOW_ROM = 1 << 6,
> VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT = 1 << 7,
> VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST = 1 << 8,
> + /* format active XML and avoid inactive XML */
> + VIR_DOMAIN_DEF_FORMAT_ACTIVE_ONLY = 1 << 9,
> } virDomainDefFormatFlags;
>
> /* Use these flags to skip specific domain ABI consistency checks done
> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
> index bfe3d6c..3cb7cd4 100644
> --- a/src/conf/snapshot_conf.c
> +++ b/src/conf/snapshot_conf.c
> @@ -290,6 +290,29 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt,
> } else {
> VIR_WARN("parsing older snapshot that lacks domain");
> }
> +
> + /* Older snapshots were created without inactive domain configuration.
> + * In that case, leave the newDom NULL. */
> + if ((tmp = virXPathString("string(./inactiveDomain/domain/@type)", ctxt))) {
> + int domainflags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
> + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE;
> + if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL)
> + domainflags |= VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS;
> + xmlNodePtr domainNode = virXPathNode("./inactiveDomain/domain", ctxt);
> +
> + VIR_FREE(tmp);
> + if (domainNode) {
> + def->newDom = virDomainDefParseNode(ctxt->node->doc, domainNode,
> + caps, xmlopt, NULL, domainflags);
> + if (!def->newDom)
> + goto cleanup;
> + } else {
> + VIR_WARN("missing inactive domain in snapshot");
But if dom->newDef never existed, then why would snapshotDef->newDom exist?
> + }
> + } else {
> + VIR_WARN("parsing older snapshot that lacks inactive domain");
So? You need to be able to handle a NULL snapshotDef->newDom being NULL
anyway, so not sure the WARN is appropriate here.
> + }
> +
> } else {
> def->creationTime = tv.tv_sec;
> }
> @@ -705,7 +728,8 @@ virDomainSnapshotDefFormat(const char *domain_uuid,
> virBuffer buf = VIR_BUFFER_INITIALIZER;
> size_t i;
>
> - virCheckFlags(VIR_DOMAIN_DEF_FORMAT_SECURE, NULL);
> + virCheckFlags(VIR_DOMAIN_DEF_FORMAT_SECURE |
> + VIR_DOMAIN_DEF_FORMAT_ACTIVE_ONLY, NULL);
>
> flags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE;
>
> @@ -757,6 +781,15 @@ virDomainSnapshotDefFormat(const char *domain_uuid,
> virBufferAddLit(&buf, "</domain>\n");
> }
>
> + if (def->newDom && !(flags & VIR_DOMAIN_DEF_FORMAT_ACTIVE_ONLY)) {
> + virBufferAddLit(&buf, "<inactiveDomain>\n");
> + virBufferAdjustIndent(&buf, 2);
> + if (virDomainDefFormatInternal(def->newDom, caps, flags, &buf) < 0)
> + goto error;
> + virBufferAdjustIndent(&buf, -2);
> + virBufferAddLit(&buf, "</inactiveDomain>\n");
> + }
> +
So after all this, it's not clear why there needs to be a special flag
for snapshot and why having this flag allows editing (IOW: The commit
message perhaps is misleading).
It's even more problematic since domain->newDef isn't necessarily
defined and thus def->newDom wouldn't be either.
I think perhaps the Parse and Format code should be their own patch
anyway, so that part is useful; however, the new flag, I'm not sure yet.
If your goal is to mimic the domain inactive flag for snapshot, then
follow how domain command line processing goes. Inventing an
"active-only" (subsequent patch) is not something I'm sure is desired
(yet). Consistency between various commands is (to me at least) far more
desirable. Currently it seems usage if "--inactive" on the dumpxml
command line is preferred (domain, net-*, iface-*, pool-*, etc).
If "today" what someone is doing is editing/dumping the active XML, then
what you're allowing in the future is to allow someone to edit/dump the
inactive XML. For that purpose, there are existing flags.
John
> if (virSaveCookieFormatBuf(&buf, def->cookie,
> virDomainXMLOptionGetSaveCookie(xmlopt)) < 0)
> goto error;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index aecfcff..a0a4384 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -15469,7 +15469,8 @@ qemuDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot,
> virDomainSnapshotObjPtr snap = NULL;
> char uuidstr[VIR_UUID_STRING_BUFLEN];
>
> - virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL);
> + virCheckFlags(VIR_DOMAIN_XML_SECURE |
> + VIR_DOMAIN_XML_ACTIVE_ONLY, NULL);
>
> if (!(vm = qemuDomObjFromSnapshot(snapshot)))
> return NULL;
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
