[libvirt] [PATCH 1.5/2] snapshot: use qemu-img on disks in use at time of snapshot

[Laine Stump]

On 10/04/2011 07:39 PM, Eric Blake wrote:
> Once we know which set of disks belong to a snapshot, reverting or
> deleting that snapshot should visit just those disks, rather than
> also visiting disks that were hot-plugged in the meantime or
> skipping disks that were hot-unplugged in the meantime.
>
> * src/qemu/qemu_domain.c (qemuDomainSnapshotForEachQcow2): Use
> snapshot domain details when available.  Avoid NULL deref.
> ---
>
> Detected while actually testing patch 2/2 in the series.  This
> fixes the issue, but is worth a separate review before I push
> the series.
>
>   src/qemu/qemu_domain.c |   24 ++++++++++++++++--------
>   1 files changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 65f721a..85bebd6 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1402,6 +1402,14 @@ qemuDomainSnapshotForEachQcow2(struct qemud_driver *driver,
>       const char *qemuimgarg[] = { NULL, "snapshot", NULL, NULL, NULL, NULL };
>       int i;
>       bool skipped = false;
> +    virDomainDefPtr def;
> +
> +    /* Prefer action on the disks in use at the time the snapshot was
> +     * created; but fall back to current definition if dealing with a
> +     * snapshot created prior to libvirt 0.9.5.  */
> +    def = snap->def->dom;
> +    if (!def)
> +        def = vm->def;
>
>       qemuimgarg[0] = qemuFindQemuImgBinary(driver);
>       if (qemuimgarg[0] == NULL) {
> @@ -1412,36 +1420,36 @@ qemuDomainSnapshotForEachQcow2(struct qemud_driver *driver,
>       qemuimgarg[2] = op;
>       qemuimgarg[3] = snap->def->name;
>
> -    for (i = 0; i<  vm->def->ndisks; i++) {
> +    for (i = 0; i<  def->ndisks; i++) {
>           /* FIXME: we also need to handle LVM here */
>           /* FIXME: if we fail halfway through this loop, we are in an
>            * inconsistent state.  I'm not quite sure what to do about that
>            */
> -        if (vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
> -            if (!vm->def->disks[i]->driverType ||
> -                STRNEQ(vm->def->disks[i]->driverType, "qcow2")) {
> +        if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
> +            if (!def->disks[i]->driverType ||
> +                STRNEQ(def->disks[i]->driverType, "qcow2")) {
>                   if (try_all) {
>                       /* Continue on even in the face of error, since other
>                        * disks in this VM may have the same snapshot name.
>                        */
>                       VIR_WARN("skipping snapshot action on %s",
> -                             vm->def->disks[i]->dst);
> +                             def->disks[i]->dst);
>                       skipped = true;
>                       continue;
>                   }
>                   qemuReportError(VIR_ERR_OPERATION_INVALID,
>                                   _("Disk device '%s' does not support"
>                                     " snapshotting"),
> -                                vm->def->disks[i]->dst);
> +                                def->disks[i]->dst);
>                   return -1;
>               }
>
> -            qemuimgarg[4] = vm->def->disks[i]->src;
> +            qemuimgarg[4] = def->disks[i]->src;
>
>               if (virRun(qemuimgarg, NULL)<  0) {
>                   if (try_all) {
>                       VIR_WARN("skipping snapshot action on %s",
> -                             vm->def->disks[i]->info.alias);
> +                             def->disks[i]->dst);
>                       skipped = true;
>                       continue;
>                   }

ACK.