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 <k...@linux.vnet.ibm.com> > --- > 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list