[libvirt] [PATCHv1 09/13] Split out qemu-img command generation

[Peter Krempa]

On Fri, Apr 10, 2015 at 14:59:01 +0200, Ján Tomko wrote:
> Do not require the volume definition.
> This will allow code reuse when creating snapshots.
> ---
>  src/storage/storage_backend.c | 203 +++++++++++++++++++++++-------------------
>  1 file changed, 109 insertions(+), 94 deletions(-)
> 
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index f5b95ec..bfbc193 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -870,38 +870,16 @@ virStorageBackendCreateQemuImgOpts(char **opts,
>      return -1;
>  }
>  
> -/* Create a qemu-img virCommand from the supplied binary path,
> - * volume definitions and imgformat
> - */
> -virCommandPtr
> -virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn,
> -                                         virStoragePoolObjPtr pool,
> -                                         virStorageVolDefPtr vol,
> -                                         virStorageVolDefPtr inputvol,
> -                                         unsigned int flags,
> -                                         const char *create_tool,
> -                                         int imgformat)
> +static virCommandPtr
> +virStorageBackendCreateQemuImgCmd(const char *create_tool,
> +                                  int imgformat,
> +                                  struct _virStorageBackendQemuImgInfo info)
>  {
>      virCommandPtr cmd = NULL;
> -    const char *type;
> +    const char *type = NULL;
>      const char *backingType = NULL;
>      const char *inputType = NULL;
>      char *opts = NULL;
> -    struct _virStorageBackendQemuImgInfo info = {
> -        .format = vol->target.format,
> -        .path = vol->target.path,
> -        .encryption = vol->target.encryption != NULL,
> -        .preallocate = !!(flags & VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA),
> -        .compat = vol->target.compat,
> -        .features = vol->target.features,
> -        .nocow = vol->target.nocow,
> -    };
> -
> -    virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL);
> -
> -    /* Treat output block devices as 'raw' format */
> -    if (vol->type == VIR_STORAGE_VOL_BLOCK)
> -        info.format = VIR_STORAGE_FILE_RAW;
>  
>      if (!(type = virStorageFileFormatTypeToString(info.format))) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -926,6 +904,107 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn,
>          return NULL;
>      }
>  
> +    if (info.inputPath &&
> +        !(inputType = virStorageFileFormatTypeToString(info.inputFormat))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("unknown storage vol type %d"),
> +                       info.inputFormat);
> +        return NULL;
> +    }
> +
> +    if (info.backingPath) {
> +        if (info.preallocate) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("metadata preallocation conflicts with backing"
> +                             " store"));
> +            return NULL;
> +        }
> +
> +        if (!(backingType = virStorageFileFormatTypeToString(info.backingFormat))) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("unknown storage vol backing store type %d"),
> +                           info.backingFormat);
> +            return NULL;
> +        }
> +    }
> +
> +    /* ignore the backing volume when we're converting a volume */
> +    if (info.inputPath) {
> +        info.backingPath = NULL;
> +        backingType = NULL;
> +    }

Shouldn't this go before you bother to check if the backing info is
correct?

> +
> +    cmd = virCommandNew(create_tool);
> +
> +    if (info.inputPath)
> +        virCommandAddArgList(cmd, "convert", "-f", inputType, "-O", type, NULL);
> +    else
> +        virCommandAddArgList(cmd, "create", "-f", type, NULL);
> +
> +    if (info.backingPath)
> +        virCommandAddArgList(cmd, "-b", info.backingPath, NULL);
> +
> +    if (imgformat >= QEMU_IMG_BACKING_FORMAT_OPTIONS) {
> +        if (info.format == VIR_STORAGE_FILE_QCOW2 && !info.compat &&
> +            imgformat == QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT)
> +            info.compat = "0.10";
> +
> +        if (virStorageBackendCreateQemuImgOpts(&opts, info) < 0) {
> +            virCommandFree(cmd);
> +            return NULL;
> +        }
> +        if (opts)
> +            virCommandAddArgList(cmd, "-o", opts, NULL);
> +        VIR_FREE(opts);
> +    } else {
> +        if (info.backingPath) {
> +            if (imgformat == QEMU_IMG_BACKING_FORMAT_FLAG)
> +                virCommandAddArgList(cmd, "-F", backingType, NULL);
> +            else
> +                VIR_DEBUG("Unable to set backing store format for %s with %s",
> +                          info.path, create_tool);
> +        }
> +        if (info.encryption)
> +            virCommandAddArg(cmd, "-e");
> +    }
> +
> +    if (info.inputPath)
> +        virCommandAddArg(cmd, info.inputPath);
> +    virCommandAddArg(cmd, info.path);
> +    if (!info.inputPath && info.size_arg)
> +        virCommandAddArgFormat(cmd, "%lluK", info.size_arg);
> +    return cmd;
> +}

Looks good otherwise.

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/a45dee64/attachment-0001.sig>