[libvirt] [PATCHv1 12/13] Rework qemu-img command generation for inactive external snapshots

[Peter Krempa]

On Fri, Apr 10, 2015 at 14:59:04 +0200, Ján Tomko wrote:
> Reuse the code from storage backend.
> 
> This also fixes the backing_fmd typo by removing it.
> ---
>  src/qemu/qemu_driver.c        | 46 ++++++++++---------------------------------
>  src/storage/storage_backend.c |  2 +-
>  2 files changed, 11 insertions(+), 37 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 4f14546..3ea42f2 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -13468,7 +13468,6 @@ qemuDomainSnapshotCreateInactiveExternal(virQEMUDriverPtr driver,
>      size_t i;
>      virDomainSnapshotDiskDefPtr snapdisk;
>      virDomainDiskDefPtr defdisk;
> -    virCommandPtr cmd = NULL;
>      const char *qemuImgPath;
>      virBitmapPtr created = NULL;
>      virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> @@ -13489,48 +13488,25 @@ qemuDomainSnapshotCreateInactiveExternal(virQEMUDriverPtr driver,
>          if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
>              continue;
>  
> -        if (!snapdisk->src->format)
> -            snapdisk->src->format = VIR_STORAGE_FILE_QCOW2;
> -
> -        /* creates cmd line args: qemu-img create -f qcow2 -o */
> -        if (!(cmd = virCommandNewArgList(qemuImgPath,
> -                                         "create",
> -                                         "-f",
> -                                         virStorageFileFormatTypeToString(snapdisk->src->format),
> -                                         "-o",
> -                                         NULL)))
> +        if (defdisk->src->format == VIR_STORAGE_FILE_NONE &&
> +            !cfg->allowDiskFormatProbing) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("unknown image format of '%s' and "
> +                             "format probing is disabled"),
> +                           defdisk->src->path);
>              goto cleanup;
> -
> -        if (defdisk->src->format > 0) {
> -            /* adds cmd line arg: backing_file=/path/to/backing/file,backing_fmd=format */
> -            virCommandAddArgFormat(cmd, "backing_file=%s,backing_fmt=%s",
> -                                   defdisk->src->path,
> -                                   virStorageFileFormatTypeToString(defdisk->src->format));
> -        } else {
> -            if (!cfg->allowDiskFormatProbing) {
> -                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                               _("unknown image format of '%s' and "
> -                                 "format probing is disabled"),
> -                               defdisk->src->path);
> -                goto cleanup;
> -            }
> -
> -            /* adds cmd line arg: backing_file=/path/to/backing/file */
> -            virCommandAddArgFormat(cmd, "backing_file=%s", defdisk->src->path);
>          }
>  
> -        /* adds cmd line args: /path/to/target/file */
> -        virCommandAddArg(cmd, snapdisk->src->path);
> +        if (!snapdisk->src->format)
> +            snapdisk->src->format = VIR_STORAGE_FILE_QCOW2;
>  
>          /* If the target does not exist, we're going to create it possibly */
>          if (!virFileExists(snapdisk->src->path))
>              ignore_value(virBitmapSetBit(created, i));
>  
> -        if (virCommandRun(cmd, NULL) < 0)
> +        if (virStorageFileCreateWithFormat(snapdisk->src, snapdisk->src->path,
> +                                           defdisk->src, qemuImgPath) < 0)

This part will need adjusting after my review to the previous patch.

>              goto cleanup;
> -
> -        virCommandFree(cmd);
> -        cmd = NULL;
>      }
>  
>      /* update disk definitions */
> @@ -13554,8 +13530,6 @@ qemuDomainSnapshotCreateInactiveExternal(virQEMUDriverPtr driver,
>      ret = 0;
>  
>   cleanup:
> -    virCommandFree(cmd);
> -
>      /* unlink images if creation has failed */
>      if (ret < 0 && created) {
>          ssize_t bit = -1;
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index 9ffbc6e..ec8b7b5 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -819,7 +819,7 @@ virStorageBackendCreateQemuImgOpts(char **opts,
>  {
>      virBuffer buf = VIR_BUFFER_INITIALIZER;
>  
> -    if (info.backingPath)
> +    if (info.backingPath && info.backingFormat)

In retrospect I think we should forbid snapshots without specifying the
format so that we don't create even more broken configurations.

>          virBufferAsprintf(&buf, "backing_fmt=%s,",
>                            virStorageFileFormatTypeToString(info.backingFormat));
>      if (info.encryption)

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150413/c6e4fcc4/attachment-0001.sig>