[Qemu-devel] Re: [PATCH 2/3] Introduce do_snapshot_blkdev() and monitor command to handle it.

[Qemu-devel] Re: [PATCH 2/3] Introduce do_snapshot_blkdev() and monitor command to handle it.

[Kevin Wolf]

Am 13.12.2010 08:32, schrieb Jes.Sorensen@redhat.com:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> The monitor command is:
> snapshot_blkdev <device> [snapshot-file] [format]
> 
> Default format is qcow2. For now snapshots without a snapshot-file, eg
> internal snapshots, are not supported.
> 
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  blockdev.c      |   61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  blockdev.h      |    1 +
>  hmp-commands.hx |   19 +++++++++++++++++
>  3 files changed, 81 insertions(+), 0 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index f6ac439..1ea24d7 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -514,6 +514,67 @@ void do_commit(Monitor *mon, const QDict *qdict)
>      }
>  }
>  
> +int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +{
> +    const char *device = qdict_get_str(qdict, "device");
> +    const char *filename = qdict_get_try_str(qdict, "snapshot_file");
> +    const char *format = qdict_get_try_str(qdict, "format");
> +    const char format_qcow2[] = "qcow2";
> +    BlockDriverState *bs;
> +    BlockDriver *drv, *proto_drv;
> +    int ret = 0;
> +    int flags;
> +
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        qerror_report(QERR_DEVICE_NOT_FOUND, device);
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    if (!format) {
> +        format = format_qcow2;
> +    }
> +
> +    drv = bdrv_find_format(format);
> +    if (!drv) {
> +        qerror_report(QERR_INVALID_BLOCK_FORMAT, format);
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    proto_drv = bdrv_find_protocol(filename);
> +    if (!proto_drv) {
> +        qerror_report(QERR_INVALID_BLOCK_FORMAT, format);
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    ret = bdrv_img_create(filename, format, bs->filename,
> +                          bs->drv->format_name, NULL, -1, bs->open_flags);
> +    if (ret) {
> +        goto out;
> +    }
> +
> +    qemu_aio_flush();
> +    bdrv_flush(bs);
> +
> +    flags = bs->open_flags;
> +    bdrv_close(bs);
> +    ret = bdrv_open(bs, filename, flags, drv);
> +    /*
> +     * If reopening the image file we just created fails, we really
> +     * are in trouble :(
> +     */
> +    assert(ret == 0);
> +out:
> +    if (ret) {
> +        ret = 1;

I seem to remember that errors are always -1 for monitor commands.

Kevin

[Qemu-devel] Re: [PATCH 2/3] Introduce do_snapshot_blkdev() and monitor command to handle it.

[Jes Sorensen]

On 12/15/10 17:55, Kevin Wolf wrote:
>> +int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data)
>> +{
>> +    const char *device = qdict_get_str(qdict, "device");
>> +    const char *filename = qdict_get_try_str(qdict, "snapshot_file");
>> +    const char *format = qdict_get_try_str(qdict, "format");
>> +    const char format_qcow2[] = "qcow2";
>> +    BlockDriverState *bs;
>> +    BlockDriver *drv, *proto_drv;
>> +    int ret = 0;
>> +    int flags;
>> +
>> +    bs = bdrv_find(device);
>> +    if (!bs) {
>> +        qerror_report(QERR_DEVICE_NOT_FOUND, device);
>> +        ret = -1;
>> +        goto out;
>> +    }
>> +
>> +    if (!format) {
>> +        format = format_qcow2;
>> +    }
>> +
>> +    drv = bdrv_find_format(format);
>> +    if (!drv) {
>> +        qerror_report(QERR_INVALID_BLOCK_FORMAT, format);
>> +        ret = -1;
>> +        goto out;
>> +    }
>> +
>> +    proto_drv = bdrv_find_protocol(filename);
>> +    if (!proto_drv) {
>> +        qerror_report(QERR_INVALID_BLOCK_FORMAT, format);
>> +        ret = -1;
>> +        goto out;
>> +    }
>> +
>> +    ret = bdrv_img_create(filename, format, bs->filename,
>> +                          bs->drv->format_name, NULL, -1, bs->open_flags);
>> +    if (ret) {
>> +        goto out;
>> +    }
>> +
>> +    qemu_aio_flush();
>> +    bdrv_flush(bs);
>> +
>> +    flags = bs->open_flags;
>> +    bdrv_close(bs);
>> +    ret = bdrv_open(bs, filename, flags, drv);
>> +    /*
>> +     * If reopening the image file we just created fails, we really
>> +     * are in trouble :(
>> +     */
>> +    assert(ret == 0);
>> +out:
>> +    if (ret) {
>> +        ret = 1;
> 
> I seem to remember that errors are always -1 for monitor commands.

I mapped it after something else, but admitted I cannot remember where -
can someone clarify?

Cheers,
Jes

[Qemu-devel] Re: [PATCH 2/3] Introduce do_snapshot_blkdev() and monitor command to handle it.

[Luiz Capitulino]

On Wed, 15 Dec 2010 17:57:25 +0100
Jes Sorensen <Jes.Sorensen@redhat.com> wrote:

> On 12/15/10 17:55, Kevin Wolf wrote:
> >> +int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data)
> >> +{
> >> +    const char *device = qdict_get_str(qdict, "device");
> >> +    const char *filename = qdict_get_try_str(qdict, "snapshot_file");
> >> +    const char *format = qdict_get_try_str(qdict, "format");
> >> +    const char format_qcow2[] = "qcow2";
> >> +    BlockDriverState *bs;
> >> +    BlockDriver *drv, *proto_drv;
> >> +    int ret = 0;
> >> +    int flags;
> >> +
> >> +    bs = bdrv_find(device);
> >> +    if (!bs) {
> >> +        qerror_report(QERR_DEVICE_NOT_FOUND, device);
> >> +        ret = -1;
> >> +        goto out;
> >> +    }
> >> +
> >> +    if (!format) {
> >> +        format = format_qcow2;
> >> +    }
> >> +
> >> +    drv = bdrv_find_format(format);
> >> +    if (!drv) {
> >> +        qerror_report(QERR_INVALID_BLOCK_FORMAT, format);
> >> +        ret = -1;
> >> +        goto out;
> >> +    }
> >> +
> >> +    proto_drv = bdrv_find_protocol(filename);
> >> +    if (!proto_drv) {
> >> +        qerror_report(QERR_INVALID_BLOCK_FORMAT, format);
> >> +        ret = -1;
> >> +        goto out;
> >> +    }
> >> +
> >> +    ret = bdrv_img_create(filename, format, bs->filename,
> >> +                          bs->drv->format_name, NULL, -1, bs->open_flags);
> >> +    if (ret) {
> >> +        goto out;
> >> +    }
> >> +
> >> +    qemu_aio_flush();
> >> +    bdrv_flush(bs);
> >> +
> >> +    flags = bs->open_flags;
> >> +    bdrv_close(bs);
> >> +    ret = bdrv_open(bs, filename, flags, drv);
> >> +    /*
> >> +     * If reopening the image file we just created fails, we really
> >> +     * are in trouble :(
> >> +     */
> >> +    assert(ret == 0);
> >> +out:
> >> +    if (ret) {
> >> +        ret = 1;
> > 
> > I seem to remember that errors are always -1 for monitor commands.
> 
> I mapped it after something else, but admitted I cannot remember where -
> can someone clarify?

-1

> 
> Cheers,
> Jes
> 

[Qemu-devel] [PATCH v2 0/3] Re-factor img_create() and add live snapshots

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Hi,

This set of patches re-factors img_create() and moves the core part of
it into block.c so it can be accessed from qemu as well as
qemu-img. The second patch adds basic live snapshots support to the
code, however only snapshots to external QCOW2 images is supported for
now. QED support should be trivial once the QED patches go into
upstream.

The last patch fixes a small gotcha which is present in the old code
as well. Try to catch cases where a user tries to create an image with
itself as the backing file. QEMU does 'interesting' things when you do
this.....

Many thanks to Kevin for his help with block layer internals!

New in v2:
 - Fix error return value in monitor command
 - Clarify help message for command
 - Fix patch conflict against block tree. It's all Stefan's fault :)
   f8feb11f4d76f390dddc5cc5345abf99f7659a78

Cheers,
Jes

Jes Sorensen (3):
  qemu-img.c: Re-factor img_create()
  Introduce do_snapshot_blkdev() and monitor command to handle it.
  Prevent creating an image with the same filename as backing file

 block.c         |  151 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 block.h         |    4 ++
 blockdev.c      |   61 ++++++++++++++++++++++
 blockdev.h      |    1 +
 hmp-commands.hx |   19 +++++++
 qemu-img.c      |  108 +--------------------------------------
 6 files changed, 238 insertions(+), 106 deletions(-)

-- 
1.7.3.3

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

[Jes.Sorensen]

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 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;
     }
-- 
1.7.3.3

[Qemu-devel] [PATCH 2/3] Introduce do_snapshot_blkdev() and monitor command to handle it.

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

The monitor command is:
snapshot_blkdev <device> [snapshot-file] [format]

Default format is qcow2. For now snapshots without a snapshot-file, eg
internal snapshots, are not supported.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 blockdev.c      |   61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 blockdev.h      |    1 +
 hmp-commands.hx |   19 +++++++++++++++++
 3 files changed, 81 insertions(+), 0 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 3b3b82d..9d6f72c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -516,6 +516,67 @@ void do_commit(Monitor *mon, const QDict *qdict)
     }
 }
 
+int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+    const char *device = qdict_get_str(qdict, "device");
+    const char *filename = qdict_get_try_str(qdict, "snapshot_file");
+    const char *format = qdict_get_try_str(qdict, "format");
+    const char format_qcow2[] = "qcow2";
+    BlockDriverState *bs;
+    BlockDriver *drv, *proto_drv;
+    int ret = 0;
+    int flags;
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        qerror_report(QERR_DEVICE_NOT_FOUND, device);
+        ret = -1;
+        goto out;
+    }
+
+    if (!format) {
+        format = format_qcow2;
+    }
+
+    drv = bdrv_find_format(format);
+    if (!drv) {
+        qerror_report(QERR_INVALID_BLOCK_FORMAT, format);
+        ret = -1;
+        goto out;
+    }
+
+    proto_drv = bdrv_find_protocol(filename);
+    if (!proto_drv) {
+        qerror_report(QERR_INVALID_BLOCK_FORMAT, format);
+        ret = -1;
+        goto out;
+    }
+
+    ret = bdrv_img_create(filename, format, bs->filename,
+                          bs->drv->format_name, NULL, -1, bs->open_flags);
+    if (ret) {
+        goto out;
+    }
+
+    qemu_aio_flush();
+    bdrv_flush(bs);
+
+    flags = bs->open_flags;
+    bdrv_close(bs);
+    ret = bdrv_open(bs, filename, flags, drv);
+    /*
+     * If reopening the image file we just created fails, we really
+     * are in trouble :(
+     */
+    assert(ret == 0);
+out:
+    if (ret) {
+        ret = -1;
+    }
+
+    return ret;
+}
+
 static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
 {
     if (!force) {
diff --git a/blockdev.h b/blockdev.h
index 4cb8ca9..4536b5c 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -52,5 +52,6 @@ int do_block_set_passwd(Monitor *mon, const QDict *qdict, QObject **ret_data);
 int do_change_block(Monitor *mon, const char *device,
                     const char *filename, const char *fmt);
 int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
+int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data);
 
 #endif
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 23024ba..dd3db36 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -801,6 +801,25 @@ STEXI
 Set maximum tolerated downtime (in seconds) for migration.
 ETEXI
 
+    {
+        .name       = "snapshot_blkdev",
+        .args_type  = "device:s,snapshot_file:s?,format:s?",
+        .params     = "device [new-image-file] [format]",
+        .help       = "initiates a live snapshot\n\t\t\t"
+                      "of device. If a new image file is specified, the\n\t\t\t"
+                      "new image file will become the new root image.\n\t\t\t"
+                      "If format is specified, the snapshot file will\n\t\t\t"
+                      "be created in that format. Otherwise the\n\t\t\t"
+                      "snapshot will be internal! (currently unsupported)",
+        .mhandler.cmd_new = do_snapshot_blkdev,
+    },
+
+STEXI
+@item snapshot_blkdev
+@findex snapshot_blkdev
+Snapshot device, using snapshot file as target if provided
+ETEXI
+
 #if defined(TARGET_I386)
     {
         .name       = "drive_add",
-- 
1.7.3.3

[Qemu-devel] [PATCH 3/3] Prevent creating an image with the same filename as backing file

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 block.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 765f9f3..027dc6a 100644
--- a/block.c
+++ b/block.c
@@ -2769,6 +2769,13 @@ int bdrv_img_create(const char *filename, const char *fmt,
     BlockDriver *drv, *proto_drv;
     int ret = 0;
 
+    if (!strcmp(filename, base_filename)) {
+        error_report("Error: Trying to create a snapshot with the same "
+                     "filename as the backing file");
+        ret = -1;
+        goto out;
+    }
+
     /* Find driver and parse its options */
     drv = bdrv_find_format(fmt);
     if (!drv) {
-- 
1.7.3.3

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

[Kevin Wolf]

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

[Qemu-devel] Re: [PATCH 2/3] Introduce do_snapshot_blkdev() and monitor command to handle it.

[Kevin Wolf]

Am 16.12.2010 12:04, schrieb Jes.Sorensen@redhat.com:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> The monitor command is:
> snapshot_blkdev <device> [snapshot-file] [format]
> 
> Default format is qcow2. For now snapshots without a snapshot-file, eg
> internal snapshots, are not supported.
> 
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  blockdev.c      |   61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  blockdev.h      |    1 +
>  hmp-commands.hx |   19 +++++++++++++++++
>  3 files changed, 81 insertions(+), 0 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 3b3b82d..9d6f72c 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -516,6 +516,67 @@ void do_commit(Monitor *mon, const QDict *qdict)
>      }
>  }
>  
> +int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +{
> +    const char *device = qdict_get_str(qdict, "device");
> +    const char *filename = qdict_get_try_str(qdict, "snapshot_file");
> +    const char *format = qdict_get_try_str(qdict, "format");
> +    const char format_qcow2[] = "qcow2";
> +    BlockDriverState *bs;
> +    BlockDriver *drv, *proto_drv;
> +    int ret = 0;
> +    int flags;
> +
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        qerror_report(QERR_DEVICE_NOT_FOUND, device);
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    if (!format) {
> +        format = format_qcow2;

Why introducing format_qcow2 instead of directly using the string
literal here?

> +    }
> +
> +    drv = bdrv_find_format(format);
> +    if (!drv) {
> +        qerror_report(QERR_INVALID_BLOCK_FORMAT, format);
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    proto_drv = bdrv_find_protocol(filename);
> +    if (!proto_drv) {
> +        qerror_report(QERR_INVALID_BLOCK_FORMAT, format);
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    ret = bdrv_img_create(filename, format, bs->filename,
> +                          bs->drv->format_name, NULL, -1, bs->open_flags);
> +    if (ret) {
> +        goto out;
> +    }
> +
> +    qemu_aio_flush();
> +    bdrv_flush(bs);
> +
> +    flags = bs->open_flags;
> +    bdrv_close(bs);
> +    ret = bdrv_open(bs, filename, flags, drv);
> +    /*
> +     * If reopening the image file we just created fails, we really
> +     * are in trouble :(
> +     */
> +    assert(ret == 0);

bdrv_commit handles this case by setting bs->drv = NULL. After this the
device will fail all requests with -ENOMEDIUM, but at least you don't
lose your VM immediately.

Kevin

Re: [Qemu-devel] [PATCH 3/3] Prevent creating an image with the same filename as backing file

[Stefan Hajnoczi]

On Thu, Dec 16, 2010 at 11:04 AM,  <Jes.Sorensen@redhat.com> wrote:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  block.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/block.c b/block.c
> index 765f9f3..027dc6a 100644
> --- a/block.c
> +++ b/block.c
> @@ -2769,6 +2769,13 @@ int bdrv_img_create(const char *filename, const char *fmt,
>     BlockDriver *drv, *proto_drv;
>     int ret = 0;
>
> +    if (!strcmp(filename, base_filename)) {
> +        error_report("Error: Trying to create a snapshot with the same "
> +                     "filename as the backing file");

Can we avoid using the word "snapshot" here?  Just "image" would make sense too.

Users could hit this if they incorrectly create an image manually.
They might not be thinking in terms of "snapshot" and that word is
already overloaded in QEMU.

Stefan

[Qemu-devel] Re: [PATCH 3/3] Prevent creating an image with the same filename as backing file

[Kevin Wolf]

Am 16.12.2010 12:04, schrieb Jes.Sorensen@redhat.com:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  block.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 765f9f3..027dc6a 100644
> --- a/block.c
> +++ b/block.c
> @@ -2769,6 +2769,13 @@ int bdrv_img_create(const char *filename, const char *fmt,
>      BlockDriver *drv, *proto_drv;
>      int ret = 0;
>  
> +    if (!strcmp(filename, base_filename)) {
> +        error_report("Error: Trying to create a snapshot with the same "
> +                     "filename as the backing file");
> +        ret = -1;
> +        goto out;
> +    }
> +
>      /* Find driver and parse its options */
>      drv = bdrv_find_format(fmt);
>      if (!drv) {

This doesn't catch things like qemu-img create -f qcow2
-obacking_file=foo.qcow2 foo.qcow2 though it will work if you use the
legacy -b option. I think we should keep it consistent.

Kevin

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

[Jes Sorensen]

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

[Qemu-devel] Re: [PATCH 2/3] Introduce do_snapshot_blkdev() and monitor command to handle it.

[Jes Sorensen]

On 12/16/10 12:45, Kevin Wolf wrote:
> Am 16.12.2010 12:04, schrieb Jes.Sorensen@redhat.com:
>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>
>> The monitor command is:
>> snapshot_blkdev <device> [snapshot-file] [format]
>>
>> Default format is qcow2. For now snapshots without a snapshot-file, eg
>> internal snapshots, are not supported.
>>
>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>> ---
>>  blockdev.c      |   61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  blockdev.h      |    1 +
>>  hmp-commands.hx |   19 +++++++++++++++++
>>  3 files changed, 81 insertions(+), 0 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index 3b3b82d..9d6f72c 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -516,6 +516,67 @@ void do_commit(Monitor *mon, const QDict *qdict)
>>      }
>>  }
>>  
>> +int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data)
>> +{
>> +    const char *device = qdict_get_str(qdict, "device");
>> +    const char *filename = qdict_get_try_str(qdict, "snapshot_file");
>> +    const char *format = qdict_get_try_str(qdict, "format");
>> +    const char format_qcow2[] = "qcow2";
>> +    BlockDriverState *bs;
>> +    BlockDriver *drv, *proto_drv;
>> +    int ret = 0;
>> +    int flags;
>> +
>> +    bs = bdrv_find(device);
>> +    if (!bs) {
>> +        qerror_report(QERR_DEVICE_NOT_FOUND, device);
>> +        ret = -1;
>> +        goto out;
>> +    }
>> +
>> +    if (!format) {
>> +        format = format_qcow2;
> 
> Why introducing format_qcow2 instead of directly using the string
> literal here?

It should generate the same code - I kinda liked my style better, but
I'll change it.

>> +    flags = bs->open_flags;
>> +    bdrv_close(bs);
>> +    ret = bdrv_open(bs, filename, flags, drv);
>> +    /*
>> +     * If reopening the image file we just created fails, we really
>> +     * are in trouble :(
>> +     */
>> +    assert(ret == 0);
> 
> bdrv_commit handles this case by setting bs->drv = NULL. After this the
> device will fail all requests with -ENOMEDIUM, but at least you don't
> lose your VM immediately.

Well if we hit this situation something catastrophic happened. I don't
see how we can continue beyond this point really. The old image was
dropped in the swap and the new one is not accessible ... we're dead :(

Cheers,
Jes