[libvirt] [PATCH RFCv2 10/10] qemu: snapshot: Improve approach to deal with snapshot metadata

[Eric Blake]

On 06/19/2014 07:46 AM, Peter Krempa wrote:
> Until now we were changing information about the disk source via
> multiple steps of copying data. Now that we changed to a pointer to
> store the disk source we might use it to change the approach to track
> the data.
> 
> Additionally this will allow proper tracking of the backing chain.
> ---
> 
> Now the security data is copied, but I'm still not sure that's everything we need.
> 
>  src/qemu/qemu_driver.c | 113 ++++++++++++-------------------------------------
>  1 file changed, 27 insertions(+), 86 deletions(-)

This patch touches just snapshot creation (good - doing it in pieces is
the right way to go); but blockpull, blockcommit, and blockcopy also
probably all could benefit from similar cleanups.

> 
> -    if (VIR_STRDUP(newsource, snap->src->path) < 0)
> +    if (!(newDiskSrc = virStorageSourceCopy(snap->src, false)))
> +        goto cleanup;
> +
> +    /* transfer seclabels from the old disk */
> +    if (!newDiskSrc->seclabels &&
> +        virStorageSourceCopySeclabels(newDiskSrc, disk->src) < 0)
>          goto cleanup;

May need rebasing depending on if you rename things earlier in the
series.  But looks like the correct action for newDiskSrc.

> 
>      if (persistDisk &&
> -        VIR_STRDUP(persistSource, snap->src->path) < 0)
> +        !(persistDiskSrc = virStorageSourceCopy(newDiskSrc, false)))

Hmm, this copies the security labels into the persistDiskSrc that were
already copied into newDiskSrc.  But aren't security labels different
between live and persistent XML?  That is, when SELinux labels are
generated at runtime, the live XML contains the generated label while
the persistent one does not.  I'm thinking that you probably need to
hoise the clone into persistDiskSrc to occur before you copy seclables
into newDiskSrc, and that you want then want to copy seclabels from
persistDisk into persistDiskSrc rather than cloning those from newDiskSrc.


>      if (persistDisk) {
> -        VIR_FREE(persistDisk->src->path);
> -        virStorageNetHostDefFree(persistDisk->src->nhosts,
> -                                 persistDisk->src->hosts);
> -
> -        persistDisk->src->path = persistSource;
> -        persistDisk->src->format = format;
> -        persistDisk->src->type = snap->src->type;
> -        persistDisk->src->protocol = snap->src->protocol;
> -        persistDisk->src->nhosts = snap->src->nhosts;
> -        persistDisk->src->hosts = persistHosts;
> -
> -        persistSource = NULL;
> -        persistHosts = NULL;
> +        persistDiskSrc->backingStore = persistDisk->src;
> +        persistDisk->src = persistDiskSrc;
> +        persistDiskSrc = NULL;

I LOVE how much nicer this looks !

> @@ -13015,21 +12980,15 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,
>  static void
>  qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver,
>                                         virDomainObjPtr vm,
> -                                       virDomainDiskDefPtr origdisk,
>                                         virDomainDiskDefPtr disk,
>                                         virDomainDiskDefPtr persistDisk,
>                                         bool need_unlink)

Makes sense - now that we are dealing with a linked list, we are just
peeling off the head of the list and don't need an origdisk for reference.

> @@ -13037,35 +12996,18 @@ qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver,
>          virStorageFileUnlink(disk->src) < 0)
>          VIR_WARN("Unable to remove just-created %s", disk->src->path);
> 
> +    virStorageFileDeinit(disk->src);
> +

Why do you need to explicitly deinit? Can't you just let that happen
automatically when freeing the virStorageSourcePtr down below?

>      /* Update vm in place to match changes.  */
> -    VIR_FREE(disk->src->path);
> -    disk->src->path = source;
> -    source = NULL;
> -    disk->src->format = origdisk->src->format;
> -    disk->src->type = origdisk->src->type;
> -    disk->src->protocol = origdisk->src->protocol;
> -    virStorageNetHostDefFree(disk->src->nhosts, disk->src->hosts);
> -    disk->src->nhosts = origdisk->src->nhosts;
> -    disk->src->hosts = virStorageNetHostDefCopy(origdisk->src->nhosts,
> -                                                origdisk->src->hosts);
> +    tmp = disk->src->backingStore;
> +    virStorageSourceFree(disk->src);

Oops.  virStorageSourceFree() is recursive, which invalidates the memory
pointed to by tmp.  You are missing:
 disk->src->backingStore = NULL;
in between assigning to tmp and freeing disk->src.

> +    disk->src = tmp;

But this is indeed a slick reduction of the size of the backing chain :)

> +
>      if (persistDisk) {
> -        VIR_FREE(persistDisk->src->path);
> -        persistDisk->src->path = persistSource;
> -        persistSource = NULL;
> -        persistDisk->src->format = origdisk->src->format;
> -        persistDisk->src->type = origdisk->src->type;
> -        persistDisk->src->protocol = origdisk->src->protocol;
> -        virStorageNetHostDefFree(persistDisk->src->nhosts,
> -                                 persistDisk->src->hosts);
> -        persistDisk->src->nhosts = origdisk->src->nhosts;
> -        persistDisk->src->hosts = virStorageNetHostDefCopy(origdisk->src->nhosts,
> -                                                           origdisk->src->hosts);
> +        tmp = persistDisk->src->backingStore;

Another missing assignment to NULL.

> +        virStorageSourceFree(persistDisk->src);
> +        persistDisk->src = tmp;
>      }
> -
> - cleanup:
> -    virStorageFileDeinit(disk->src);
> -    VIR_FREE(source);
> -    VIR_FREE(persistSource);
>  }
> 
>  /* The domain is expected to be locked and active. */
> @@ -13168,7 +13110,6 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver,
>                  }
> 
>                  qemuDomainSnapshotUndoSingleDiskActive(driver, vm,
> -                                                       snap->def->dom->disks[i],
>                                                         vm->def->disks[i],
>                                                         persistDisk,
>                                                         need_unlink);
> 

Not quite ready for prime-time, but definitely closer this time around.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140619/69a9377a/attachment-0001.sig>