Re: [PATCH 3/7] conf: Allow editing inactive snapshot configuration

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux