Re: [PATCH v2 11/11] qemu: Implement bulk snapshot operations

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

 




On 2/23/19 4:24 PM, Eric Blake wrote:
> Implement the new flags for bulk snapshot dump and redefine. This
> borrows from ideas in the test driver, but is further complicated
> by the fact that qemu must write snapshot XML to disk, and thus must
> do additional validation after the initial parse to ensure the user
> didn't attempt to rename a snapshot with "../" or similar.
> 
> Of note: all prior callers of qemuDomainSnapshotDiscardAllMetadata()
> were at points where it did not matter if vm->current_snapshot and
> the metaroot in vm->snapshots were left pointing to stale memory,
> because we were about to delete the entire vm object; but now it is
> important to reset things properly so that the domain still shows
> as having no snapshots on failure.
> 
> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
> ---
>  src/qemu/qemu_domain.h |  2 +-
>  src/qemu/qemu_domain.c | 35 +++++++++++++++++------
>  src/qemu/qemu_driver.c | 64 ++++++++++++++++++++++++++++++++++++++++--
>  3 files changed, 89 insertions(+), 12 deletions(-)
> 

NB: I couldn't get this one to git am -3 apply - I didn't chase the
conflict though.

> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 7c6b50184c..37c9813ec5 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -683,7 +683,7 @@ int qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm,
>                                      virDomainSnapshotObjPtr snapshot,
>                                      virCapsPtr caps,
>                                      virDomainXMLOptionPtr xmlopt,
> -                                    char *snapshotDir);
> +                                    const char *snapshotDir);

Theoretically separable.

> 
>  int qemuDomainSnapshotForEachQcow2(virQEMUDriverPtr driver,
>                                     virDomainObjPtr vm,
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index cb1665c8f9..cf8b6e8eaf 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -7704,6 +7704,7 @@ qemuDomainDefCopy(virQEMUDriverPtr driver,
> 
>  static int
>  qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver,
> +                               virDomainObjPtr vm,
>                                 virDomainDefPtr def,
>                                 virCPUDefPtr origCPU,
>                                 unsigned int flags,
> @@ -7713,8 +7714,10 @@ qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver,
>      virDomainDefPtr copy = NULL;
>      virCapsPtr caps = NULL;
>      virQEMUCapsPtr qemuCaps = NULL;
> +    bool snapshots = flags & VIR_DOMAIN_XML_SNAPSHOTS;
> 
> -    virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS | VIR_DOMAIN_XML_UPDATE_CPU, -1);
> +    virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS | VIR_DOMAIN_XML_UPDATE_CPU |
> +                  VIR_DOMAIN_XML_SNAPSHOTS, -1);
> 
>      if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
>          goto cleanup;
> @@ -7881,7 +7884,14 @@ qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver,
>      }
> 
>   format:
> -    ret = virDomainDefFormatInternal(def, caps, NULL, NULL,
> +    if (snapshots && !vm) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("snapshots XML requested but not provided"));

Error msg is a bit odd - the error is that a snapshot listing was
desired, but consumer didn't pass the @vm object.

> +        goto cleanup;
> +    }
> +    ret = virDomainDefFormatInternal(def, caps,
> +                                     snapshots ? vm->snapshots : NULL,
> +                                     snapshots ? vm->current_snapshot : NULL,
>                                       virDomainDefFormatConvertXMLFlags(flags),
>                                       buf, driver->xmlopt);

Perhaps this one should [be | have been] turned into a
virDomainDefFormatFull (or whatever new name is chosen if one is
chosen).  I think it shows the hazards of exposing *Internal to many
consumers.

> 
> @@ -7899,19 +7909,21 @@ qemuDomainDefFormatBuf(virQEMUDriverPtr driver,
>                         unsigned int flags,
>                         virBufferPtr buf)
>  {
> -    return qemuDomainDefFormatBufInternal(driver, def, NULL, flags, buf);
> +    return qemuDomainDefFormatBufInternal(driver, NULL, def, NULL, flags, buf);
>  }
> 
> 
>  static char *
>  qemuDomainDefFormatXMLInternal(virQEMUDriverPtr driver,
> +                               virDomainObjPtr vm,
>                                 virDomainDefPtr def,
>                                 virCPUDefPtr origCPU,
>                                 unsigned int flags)
>  {
>      virBuffer buf = VIR_BUFFER_INITIALIZER;
> 
> -    if (qemuDomainDefFormatBufInternal(driver, def, origCPU, flags, &buf) < 0)
> +    if (qemuDomainDefFormatBufInternal(driver, vm, def, origCPU, flags,
> +                                       &buf) < 0)

Existing; however, considering earlier comment about snapshot list being
filled into the buffer until error, but the *ListFormat not clearing the
buffer on error, I think another patch should be added here to do the
virBufferFreeAndReset(&buf); before return NULL.

>          return NULL;
> 
>      return virBufferContentAndReset(&buf);
> @@ -7923,7 +7935,7 @@ qemuDomainDefFormatXML(virQEMUDriverPtr driver,
>                         virDomainDefPtr def,
>                         unsigned int flags)
>  {
> -    return qemuDomainDefFormatXMLInternal(driver, def, NULL, flags);
> +    return qemuDomainDefFormatXMLInternal(driver, NULL, def, NULL, flags);
>  }
> 
> 
> @@ -7942,7 +7954,7 @@ char *qemuDomainFormatXML(virQEMUDriverPtr driver,
>          origCPU = priv->origCPU;
>      }
> 
> -    return qemuDomainDefFormatXMLInternal(driver, def, origCPU, flags);
> +    return qemuDomainDefFormatXMLInternal(driver, vm, def, origCPU, flags);
>  }
> 
>  char *
> @@ -7959,7 +7971,7 @@ qemuDomainDefFormatLive(virQEMUDriverPtr driver,
>      if (compatible)
>          flags |= VIR_DOMAIN_XML_MIGRATABLE;
> 
> -    return qemuDomainDefFormatXMLInternal(driver, def, origCPU, flags);
> +    return qemuDomainDefFormatXMLInternal(driver, NULL, def, origCPU, flags);
>  }
> 
> 
> @@ -8386,7 +8398,7 @@ qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm,
>                                  virDomainSnapshotObjPtr snapshot,
>                                  virCapsPtr caps,
>                                  virDomainXMLOptionPtr xmlopt,
> -                                char *snapshotDir)
> +                                const char *snapshotDir)
>  {
>      char *newxml = NULL;
>      int ret = -1;
> @@ -8605,6 +8617,7 @@ qemuDomainSnapshotDiscardAllMetadata(virQEMUDriverPtr driver,
>                                       virDomainObjPtr vm)
>  {
>      virQEMUSnapRemove rem;
> +    virDomainSnapshotObjPtr snap;
> 
>      rem.driver = driver;
>      rem.vm = vm;
> @@ -8612,6 +8625,12 @@ qemuDomainSnapshotDiscardAllMetadata(virQEMUDriverPtr driver,
>      rem.err = 0;
>      virDomainSnapshotForEach(vm->snapshots, qemuDomainSnapshotDiscardAll,
>                               &rem);
> +    if (rem.current)
> +        vm->current_snapshot = NULL;
> +    /* Update the metaroot to match that all children were dropped */
> +    snap = virDomainSnapshotFindByName(vm->snapshots, NULL);
> +    snap->nchildren = 0;
> +    snap->first_child = NULL;
> 
>      return rem.err;
>  }
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index b3f4dc6f5c..4df9e18e4c 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -7337,8 +7337,8 @@ static char
>      virDomainObjPtr vm;
>      char *ret = NULL;
> 
> -    virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS | VIR_DOMAIN_XML_UPDATE_CPU,
> -                  NULL);
> +    virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS | VIR_DOMAIN_XML_UPDATE_CPU |
> +                  VIR_DOMAIN_XML_SNAPSHOTS, NULL);
> 
>      if (!(vm = qemuDomObjFromDomain(dom)))
>          goto cleanup;
> @@ -15733,6 +15733,31 @@ qemuDomainSnapshotValidate(virDomainSnapshotDefPtr def, int state,
>      return 0;
>  }
> 
> +/* Struct and hash-iterator callback used when bulk redefining snapshots */
> +struct qemuDomainSnapshotBulk {
> +    virDomainObjPtr vm;
> +    virQEMUDriverPtr driver;
> +    const char *snapshotDir;
> +    unsigned int flags;
> +};
> +
> +static int
> +qemuDomainSnapshotBulkRedefine(void *payload, const void *name ATTRIBUTE_UNUSED,

one arg each line

> +                               void *opaque)
> +{
> +    virDomainSnapshotObjPtr snap = payload;
> +    struct qemuDomainSnapshotBulk *data = opaque;
> +
> +    if (qemuDomainSnapshotValidate(snap->def, snap->def->state,
> +                                   data->flags) < 0)
> +        return -1;
> +    if (qemuDomainSnapshotWriteMetadata(data->vm, snap, data->driver->caps,
> +                                        data->driver->xmlopt,
> +                                        data->snapshotDir) < 0)
> +        return -1;
> +    return 0;
> +}
> +
>  static virDomainSnapshotPtr
>  qemuDomainSnapshotCreateXML(virDomainPtr domain,
>                              const char *xmlDesc,
> @@ -15762,7 +15787,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
>                    VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT |
>                    VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE |
>                    VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC |
> -                  VIR_DOMAIN_SNAPSHOT_CREATE_LIVE, NULL);
> +                  VIR_DOMAIN_SNAPSHOT_CREATE_LIVE |
> +                  VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST, NULL);
> 
>      VIR_REQUIRE_FLAG_RET(VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE,
>                           VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY,
> @@ -15794,6 +15820,38 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
>          goto cleanup;
>      }
> 
> +    if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST) {
> +        struct qemuDomainSnapshotBulk bulk = {
> +            .vm = vm,
> +            .driver = driver,
> +            .snapshotDir = cfg->snapshotDir,
> +            .flags = flags,
> +        };
> +
> +        if (virDomainSnapshotDefParseList(xmlDesc, vm->def->uuid,
> +                                          vm->snapshots, &vm->current_snapshot,
> +                                          caps, driver->xmlopt,
> +                                          parse_flags) < 0)
> +            goto cleanup;
> +        /* Validate and save the snapshots to disk. Since we don't get
> +         * here unless there were no snapshots beforehand, just delete
> +         * everything if anything failed, ignoring further errors. */
> +        if (virDomainSnapshotForEach(vm->snapshots,
> +                                     qemuDomainSnapshotBulkRedefine,
> +                                     &bulk) < 0) {
> +            virErrorPtr orig_err = virSaveLastError();
> +
> +            qemuDomainSnapshotDiscardAllMetadata(driver, vm);
> +            virSetError(orig_err);
> +            virFreeError(orig_err);
> +            goto cleanup;
> +        }
> +        /* Return is arbitrary, so use the first root */
> +        snap = virDomainSnapshotFindByName(vm->snapshots, NULL);

Similar to test driver - this isn't used during cleanup.

John

> +        snapshot = virGetDomainSnapshot(domain, snap->first_child->def->name);
> +        goto cleanup;
> +    }
> +
>      if (!vm->persistent && (flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT)) {
>          virReportError(VIR_ERR_OPERATION_INVALID, "%s",
>                         _("cannot halt after transient domain snapshot"));
> 

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