[Qemu-devel] Re: [PATCH 1/3] qemu-img.c: Re-factor img_create()

[Kevin Wolf <kwolf@redhat.com>]

Am 16.12.2010 12:04, schrieb Jes.Sorensen@redhat.com:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> This patch re-factors img_create() moving the code doing the actual
> work into block.c where it can be shared with QEMU. This is needed to
> be able to create images from QEMU to be used for live snapshots.
> 
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  block.c    |  144 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  block.h    |    4 ++
>  qemu-img.c |  108 +--------------------------------------------
>  3 files changed, 150 insertions(+), 106 deletions(-)
> 
> diff --git a/block.c b/block.c
> index b4aaf41..765f9f3 100644
> --- a/block.c
> +++ b/block.c
> @@ -2758,3 +2758,147 @@ int64_t bdrv_get_dirty_count(BlockDriverState *bs)
>  {
>      return bs->dirty_count;
>  }
> +
> +int bdrv_img_create(const char *filename, const char *fmt,
> +                    const char *base_filename, const char *base_fmt,
> +                    char *options, uint64_t img_size, int flags)
> +{
> +    QEMUOptionParameter *param = NULL, *create_options = NULL;
> +    QEMUOptionParameter *backing_fmt;
> +    BlockDriverState *bs = NULL;
> +    BlockDriver *drv, *proto_drv;
> +    int ret = 0;
> +
> +    /* Find driver and parse its options */
> +    drv = bdrv_find_format(fmt);
> +    if (!drv) {
> +        error_report("Unknown file format '%s'", fmt);
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    proto_drv = bdrv_find_protocol(filename);
> +    if (!proto_drv) {
> +        error_report("Unknown protocol '%s'", filename);
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    create_options = append_option_parameters(create_options,
> +                                              drv->create_options);
> +    create_options = append_option_parameters(create_options,
> +                                              proto_drv->create_options);
> +
> +    /* Create parameter list with default values */
> +    param = parse_option_parameters("", create_options, param);
> +
> +    set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size);
> +
> +    /* Parse -o options */
> +    if (options) {
> +        param = parse_option_parameters(options, create_options, param);
> +        if (param == NULL) {
> +            error_report("Invalid options for file format '%s'.", fmt);
> +            ret = -1;
> +            goto out;
> +        }
> +    }
> +
> +    if (base_filename) {
> +        if (set_option_parameter(param, BLOCK_OPT_BACKING_FILE,
> +                                 base_filename)) {
> +            error_report("Backing file not supported for file format '%s'",
> +                         fmt);
> +            ret = -1;
> +            goto out;
> +        }
> +    }
> +
> +    backing_fmt = get_option_parameter(param, BLOCK_OPT_BACKING_FMT);
> +    if (backing_fmt && backing_fmt->value.s) {
> +        if (!bdrv_find_format(backing_fmt->value.s)) {
> +            error_report("Unknown backing file format '%s'",
> +                         backing_fmt->value.s);
> +            ret = -1;
> +            goto out;
> +        }
> +    }
> +
> +    if (base_fmt) {
> +        if (set_option_parameter(param, BLOCK_OPT_BACKING_FMT, base_fmt)) {
> +            error_report("Backing file format not supported for file "
> +                         "format '%s'", fmt);
> +            ret = -1;
> +            goto out;
> +        }
> +    }

The order is wrong here. If you use -F, the backing format won't be checked.

> +
> +    // The size for the image must always be specified, with one exception:
> +    // If we are using a backing file, we can obtain the size from there
> +    if (get_option_parameter(param, BLOCK_OPT_SIZE)->value.n == -1) {
> +        QEMUOptionParameter *backing_file =
> +            get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
> +
> +        if (backing_file && backing_file->value.s) {
> +            uint64_t size;
> +            const char *fmt = NULL;
> +            char buf[32];
> +
> +            if (backing_fmt && backing_fmt->value.s) {
> +                fmt = backing_fmt->value.s;
> +            }
> +
> +            bs = bdrv_new("");
> +            if (!bs) {
> +                error_report("Not enough memory to allocate BlockDriverState");
> +                ret = -1;
> +                goto out;
> +            }

bdrv_new never returns NULL (it's an indirect qemu_malloc call).

> +            ret = bdrv_open(bs, backing_file->value.s, flags, drv);
> +            if (ret < 0) {
> +                error_report("Could not open '%s'", filename);
> +                ret = -1;
> +                goto out;
> +            }
> +            bdrv_get_geometry(bs, &size);
> +            size *= 512;
> +
> +            snprintf(buf, sizeof(buf), "%" PRId64, size);
> +            set_option_parameter(param, BLOCK_OPT_SIZE, buf);
> +        } else {
> +            error_report("Image creation needs a size parameter");
> +            ret = -1;
> +            goto out;
> +        }
> +    }
> +
> +    printf("Formatting '%s', fmt=%s ", filename, fmt);
> +    print_option_parameters(param);
> +    puts("");
> +
> +    ret = bdrv_create(drv, filename, param);
> +    free_option_parameters(create_options);
> +    free_option_parameters(param);

These need to be after out: to avoid leaking in error cases.

You're basically reverting a87a6721d with this.

> +
> +    if (ret < 0) {
> +        if (ret == -ENOTSUP) {
> +            error_report("Formatting or formatting option not supported for "
> +                         "file format '%s'", fmt);
> +        } else if (ret == -EFBIG) {
> +            error_report("The image size is too large for file format '%s'",
> +                         fmt);
> +        } else {
> +            error_report("%s: error while creating %s: %s", filename, fmt,
> +                         strerror(-ret));
> +        }
> +    }
> +
> +out:
> +    if (bs) {
> +        bdrv_delete(bs);
> +    }
> +    if (ret) {
> +        return 1;
> +    }

Maybe we should better use the usual 0/-errno style. In qemu-img it was
the exit code of the program, but now it's a block layer function.

Kevin