Re: [libvirt] [PATCH 2/2] snapshot: wire up qemu transaction command

[Peter Krempa <pkrempa redhat com>]

On 03/17/2012 10:27 PM, Eric Blake wrote:
> The hardest part about adding transactions is not using the new
> monitor command, but undoing the partial changes we made prior
> to a failed transaction.
>
> * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateDiskActive): Use
> transaction when available.
> (qemuDomainSnapshotUndoSingleDiskActive): New function.
> (qemuDomainSnapshotCreateSingleDiskActive): Pass through actions.
> (qemuDomainSnapshotCreateXML): Adjust caller.
> ---
>   src/qemu/qemu_driver.c |  112 +++++++++++++++++++++++++++++++++++++++++++++--
>   1 files changed, 107 insertions(+), 5 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index c3bbc3f..2bc8d5d 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -9822,7 +9822,8 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver,
>                                            virDomainObjPtr vm,
>                                            virDomainSnapshotDiskDefPtr snap,
>                                            virDomainDiskDefPtr disk,
> -                                         virDomainDiskDefPtr persistDisk)
> +                                         virDomainDiskDefPtr persistDisk,
> +                                         virJSONValuePtr actions)
>   {
>       qemuDomainObjPrivatePtr priv = vm->privateData;
>       char *device = NULL;
> @@ -9882,7 +9883,7 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver,
>       origdriver = NULL;
>
>       /* create the actual snapshot */
> -    ret = qemuMonitorDiskSnapshot(priv->mon, NULL, device, source);
> +    ret = qemuMonitorDiskSnapshot(priv->mon, actions, device, source);
>       virDomainAuditDisk(vm, disk->src, source, "snapshot", ret>= 0);
>       if (ret<  0)
>           goto cleanup;
> @@ -9923,6 +9924,69 @@ cleanup:
>       return ret;
>   }
>
> +/* The domain is expected to hold monitor lock.  This is the
> + * counterpart to qemuDomainSnapshotCreateSingleDiskActive, called
> + * only on a failed transaction. */
> +static void
> +qemuDomainSnapshotUndoSingleDiskActive(struct qemud_driver *driver,
> +                                       virDomainObjPtr vm,
> +                                       virDomainDiskDefPtr origdisk,
> +                                       virDomainDiskDefPtr disk,
> +                                       virDomainDiskDefPtr persistDisk,
> +                                       bool need_unlink)
> +{
> +    char *source = NULL;
> +    char *driverType = NULL;
> +    char *persistSource = NULL;
> +    char *persistDriverType = NULL;
> +    struct stat st;
> +
> +    if (!(source = strdup(origdisk->src)) ||
> +        (origdisk->driverType&&
> +         !(driverType = strdup(origdisk->driverType))) ||
> +        (persistDisk&&
> +         (!(persistSource = strdup(source)) ||
> +          (driverType&&  !(persistDriverType = strdup(driverType)))))) {
> +        virReportOOMError();
> +        goto cleanup;
> +    }
> +
> +    if (virSecurityManagerRestoreImageLabel(driver->securityManager,
> +                                            vm->def, disk)<  0)
> +        VIR_WARN("Unable to restore security label on %s", disk->src);
> +    if (virDomainLockDiskDetach(driver->lockManager, vm, disk)<  0)
> +        VIR_WARN("Unable to release lock on %s", source);
> +    if (need_unlink&&  stat(disk->src,&st) == 0
> +&&  st.st_size == 0&&  S_ISREG(st.st_mode)&&  unlink(disk->src)<  0)
> +        VIR_WARN("Unable to release lock on %s", disk->src);

Is this second warning correct? It looks to me like if you are unlinking 
the disk source file, but the error message states that lock release failed.

> +
> +    /* Update vm in place to match changes.  */
> +    VIR_FREE(disk->src);
> +    disk->src = source;
> +    source = NULL;
> +    VIR_FREE(disk->driverType);
> +    if (driverType) {
> +        disk->driverType = driverType;
> +        driverType = NULL;
> +    }
> +    if (persistDisk) {
> +        VIR_FREE(persistDisk->src);
> +        persistDisk->src = persistSource;
> +        persistSource = NULL;
> +        VIR_FREE(persistDisk->driverType);
> +        if (persistDriverType) {
> +            persistDisk->driverType = persistDriverType;
> +            persistDriverType = NULL;
> +        }
> +    }
> +
> +cleanup:
> +    VIR_FREE(source);
> +    VIR_FREE(driverType);
> +    VIR_FREE(persistSource);
> +    VIR_FREE(persistDriverType);
> +}
> +

I don't have many experience working with the locking and security code, 
but this funcion looks as it's doing what it should. I'd feel more 
confident if somebody other would look at this part.

ACK to the rest, and a not-so-confident ACK of the marked part if my 
question gets cleared.

Peter