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

login
register
mail settings
Submitter Jes Sorensen
Date Dec. 16, 2010, 11:04 a.m.
Message ID <1292497454-32573-2-git-send-email-Jes.Sorensen@redhat.com>
Download mbox | patch
Permalink /patch/75738/
State New
Headers show

Comments

Jes Sorensen - Dec. 16, 2010, 11:04 a.m.
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(-)
Kevin Wolf - Dec. 16, 2010, 11:35 a.m.
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
Jes Sorensen - Dec. 16, 2010, 12:05 p.m.
On 12/16/10 12:35, Kevin Wolf wrote:
> Am 16.12.2010 12:04, schrieb Jes.Sorensen@redhat.com:
>> +
>> +    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.

Urgh yes, my bad! I had it the other way, but decided to change it -
very silly.

>> +
>> +    // 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).

I see - this was copied wholesale from qemu-img.c but I'll just remove
the error check.

>> +            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.

Whoops - another one of those conflicting ones. It's all Stefan's fault :)

>> +
>> +    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.

I like errno, I made it behave like this for consistency with the rest
of QEMU. In most places we don't seem to like anything but -1/1 for
error values.

Let me know what you prefer, or alternatively we can change it in a
follow-on patch?

Cheers,
Jes

Patch

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 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;
+            }
+            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);
+
+    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;
+    }
+    return 0;
+}
diff --git a/block.h b/block.h
index 78ecfac..b812172 100644
--- a/block.h
+++ b/block.h
@@ -227,6 +227,10 @@  int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
 int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf,
                       int64_t pos, int size);
 
+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);
+
 #define BDRV_SECTORS_PER_DIRTY_CHUNK 2048
 
 void bdrv_set_dirty_tracking(BlockDriverState *bs, int enable);
diff --git a/qemu-img.c b/qemu-img.c
index 1d936ed..603bdb3 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -287,9 +287,6 @@  static int img_create(int argc, char **argv)
     const char *base_fmt = NULL;
     const char *filename;
     const char *base_filename = NULL;
-    BlockDriver *drv, *proto_drv;
-    QEMUOptionParameter *param = NULL, *create_options = NULL;
-    QEMUOptionParameter *backing_fmt = NULL;
     char *options = NULL;
 
     for(;;) {
@@ -350,110 +347,9 @@  static int img_create(int argc, char **argv)
         goto out;
     }
 
-    /* Find driver and parse its options */
-    drv = bdrv_find_format(fmt);
-    if (!drv) {
-        error("Unknown file format '%s'", fmt);
-        ret = -1;
-        goto out;
-    }
-
-    proto_drv = bdrv_find_protocol(filename);
-    if (!proto_drv) {
-        error("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("Invalid options for file format '%s'.", fmt);
-            ret = -1;
-            goto out;
-        }
-    }
-
-    /* Add old-style options to parameters */
-    ret = add_old_style_options(fmt, param, base_filename, base_fmt);
-    if (ret < 0) {
-        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("Unknown backing file format '%s'",
-                  backing_fmt->value.s);
-            ret = -1;
-            goto out;
-        }
-    }
-
-    // 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) {
-            BlockDriverState *bs;
-            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_open(backing_file->value.s, fmt, BDRV_O_FLAGS);
-            if (!bs) {
-                ret = -1;
-                goto out;
-            }
-            bdrv_get_geometry(bs, &size);
-            size *= 512;
-            bdrv_delete(bs);
-
-            snprintf(buf, sizeof(buf), "%" PRId64, size);
-            set_option_parameter(param, BLOCK_OPT_SIZE, buf);
-        } else {
-            error("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);
-
-    if (ret < 0) {
-        if (ret == -ENOTSUP) {
-            error("Formatting or formatting option not supported for file format '%s'", fmt);
-        } else if (ret == -EFBIG) {
-            error("The image size is too large for file format '%s'", fmt);
-        } else {
-            error("%s: error while creating %s: %s", filename, fmt, strerror(-ret));
-        }
-    }
+    ret = bdrv_img_create(filename, fmt, base_filename, base_fmt,
+                          options, img_size, BDRV_O_FLAGS);
 out:
-    free_option_parameters(create_options);
-    free_option_parameters(param);
     if (ret) {
         return 1;
     }