[Qemu-devel] [PATCH 0/3] block/qcow2: Image file option amendment

[Qemu-devel] [PATCH 0/3] block/qcow2: Image file option amendment

[Max Reitz]

This series adds support to qemu-img, block and qcow2 for amending image
options on existing image files.

Max Reitz (3):
  block: Image file option amendment
  qcow2: Implement bdrv_amend_options
  qemu-iotest: qcow2 image option amendment

 block.c                    |   8 ++
 block/qcow2-cluster.c      |  66 +++++++++++
 block/qcow2.c              | 182 ++++++++++++++++++++++++++++
 block/qcow2.h              |   2 +
 include/block/block.h      |   2 +
 include/block/block_int.h  |   3 +
 qemu-img-cmds.hx           |   6 +
 qemu-img.c                 |  90 ++++++++++++++
 qemu-img.texi              |   5 +
 tests/qemu-iotests/061     | 121 +++++++++++++++++++
 tests/qemu-iotests/061.out | 289 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 12 files changed, 775 insertions(+)
 create mode 100755 tests/qemu-iotests/061
 create mode 100644 tests/qemu-iotests/061.out

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/3] block: Image file option amendment

[Max Reitz]

This patch adds the "amend" option to qemu-img which allows changing
image options on existing image files. It also adds the generic bdrv
implementation which is basically just a wrapper for the image format
specific function.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c                   |  8 +++++
 include/block/block.h     |  2 ++
 include/block/block_int.h |  3 ++
 qemu-img-cmds.hx          |  6 ++++
 qemu-img.c                | 90 +++++++++++++++++++++++++++++++++++++++++++++++
 qemu-img.texi             |  5 +++
 6 files changed, 114 insertions(+)

diff --git a/block.c b/block.c
index a387c1a..9c40a15 100644
--- a/block.c
+++ b/block.c
@@ -4674,3 +4674,11 @@ void bdrv_add_before_write_notifier(BlockDriverState *bs,
 {
     notifier_with_return_list_add(&bs->before_write_notifiers, notifier);
 }
+
+int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter *options)
+{
+    if (bs->drv->bdrv_amend_options == NULL) {
+        return -ENOTSUP;
+    }
+    return bs->drv->bdrv_amend_options(bs, options);
+}
diff --git a/include/block/block.h b/include/block/block.h
index 742fce5..40fad1b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -223,6 +223,8 @@ typedef enum {
 
 int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix);
 
+int bdrv_amend_options(BlockDriverState *bs_new, QEMUOptionParameter *options);
+
 /* async block I/O */
 typedef void BlockDriverDirtyHandler(BlockDriverState *bs, int64_t sector,
                                      int sector_num);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8012e25..3c93766 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -205,6 +205,9 @@ struct BlockDriver {
     int (*bdrv_check)(BlockDriverState* bs, BdrvCheckResult *result,
         BdrvCheckMode fix);
 
+    int (*bdrv_amend_options)(BlockDriverState *bs,
+        QEMUOptionParameter *options);
+
     void (*bdrv_debug_event)(BlockDriverState *bs, BlkDebugEvent event);
 
     /* TODO Better pass a option string/QDict/QemuOpts to add any rule? */
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 4ca7e95..5a066b5 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -61,5 +61,11 @@ DEF("resize", img_resize,
     "resize [-q] filename [+ | -]size")
 STEXI
 @item resize [-q] @var{filename} [+ | -]@var{size}
+ETEXI
+
+DEF("amend", img_amend,
+    "amend [-q] [-f fmt] -o options filename")
+STEXI
+@item amend [-q] [-f @var{fmt}] -o @var{options} @var{filename}
 @end table
 ETEXI
diff --git a/qemu-img.c b/qemu-img.c
index b9a848d..bb8eabb 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2308,6 +2308,96 @@ out:
     return 0;
 }
 
+static int img_amend(int argc, char **argv)
+{
+    int c, ret = 0;
+    char *options = NULL;
+    QEMUOptionParameter *create_options = NULL, *options_param = NULL;
+    const char *fmt = NULL, *filename;
+    bool quiet = false;
+    BlockDriverState *bs = NULL;
+    BlockDriver *bdrv;
+
+    for (;;) {
+        c = getopt(argc, argv, "h?qf:o:");
+        if (c == -1) {
+            break;
+        }
+
+        switch (c) {
+            case 'h':
+            case '?':
+                help();
+                break;
+            case 'o':
+                options = optarg;
+                break;
+            case 'f':
+                fmt = optarg;
+                break;
+            case 'q':
+                quiet = true;
+                break;
+        }
+    }
+
+    if (optind != argc - 1) {
+        help();
+    }
+
+    if (!options) {
+        help();
+    }
+
+    filename = argv[argc - 1];
+
+    bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR, true, quiet);
+    if (!bs) {
+        error_report("Could not open image.");
+        ret = -1;
+        goto out;
+    }
+
+    bdrv = bs->drv;
+    if (!bdrv) {
+        error_report("No driver found for this image file.");
+        ret = -1;
+        goto out;
+    }
+
+    fmt = bdrv->format_name;
+
+    if (is_help_option(options)) {
+        ret = print_block_option_help(filename, fmt);
+        goto out;
+    }
+
+    create_options = append_option_parameters(create_options,
+            bdrv->create_options);
+    options_param = parse_option_parameters(options, create_options,
+            options_param);
+    if (options_param == NULL) {
+        error_report("Invalid options for file format '%s'.", fmt);
+        ret = -1;
+        goto out;
+    }
+
+    ret = bdrv_amend_options(bs, options_param);
+    if (ret < 0) {
+        error_report("Error while amending options: %s", strerror(-ret));
+        goto out;
+    }
+
+out:
+    if (bs) {
+        bdrv_delete(bs);
+    }
+    if (ret) {
+        return 1;
+    }
+    return 0;
+}
+
 static const img_cmd_t img_cmds[] = {
 #define DEF(option, callback, arg_string)        \
     { option, callback },
diff --git a/qemu-img.texi b/qemu-img.texi
index 69f1bda..4e39933 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -282,6 +282,11 @@ sizes accordingly.  Failure to do so will result in data loss!
 After using this command to grow a disk image, you must use file system and
 partitioning tools inside the VM to actually begin using the new space on the
 device.
+
+@item amend [-f @var{fmt}] -o @var{options} @var{filename}
+
+Amends the image format specific @var{options} for the image file
+@var{filename}. Only the format @code{qcow2} supports this.
 @end table
 @c man end
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/3] block: Image file option amendment

[Kevin Wolf]

Am 28.08.2013 um 10:08 hat Max Reitz geschrieben:
> This patch adds the "amend" option to qemu-img which allows changing
> image options on existing image files. It also adds the generic bdrv
> implementation which is basically just a wrapper for the image format
> specific function.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c                   |  8 +++++
>  include/block/block.h     |  2 ++
>  include/block/block_int.h |  3 ++
>  qemu-img-cmds.hx          |  6 ++++
>  qemu-img.c                | 90 +++++++++++++++++++++++++++++++++++++++++++++++
>  qemu-img.texi             |  5 +++
>  6 files changed, 114 insertions(+)

> +static int img_amend(int argc, char **argv)
> +{
> +    int c, ret = 0;
> +    char *options = NULL;
> +    QEMUOptionParameter *create_options = NULL, *options_param = NULL;
> +    const char *fmt = NULL, *filename;
> +    bool quiet = false;
> +    BlockDriverState *bs = NULL;
> +    BlockDriver *bdrv;
> +
> +    for (;;) {
> +        c = getopt(argc, argv, "h?qf:o:");
> +        if (c == -1) {
> +            break;
> +        }
> +
> +        switch (c) {
> +            case 'h':
> +            case '?':
> +                help();
> +                break;
> +            case 'o':
> +                options = optarg;
> +                break;
> +            case 'f':
> +                fmt = optarg;
> +                break;
> +            case 'q':
> +                quiet = true;
> +                break;
> +        }
> +    }
> +
> +    if (optind != argc - 1) {
> +        help();
> +    }
> +
> +    if (!options) {
> +        help();
> +    }
> +
> +    filename = argv[argc - 1];
> +
> +    bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR, true, quiet);
> +    if (!bs) {
> +        error_report("Could not open image.");
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    bdrv = bs->drv;
> +    if (!bdrv) {

This can't happen. A successful bdrv_open() always results in a
BlockDriverState that has a driver assigned.

> +        error_report("No driver found for this image file.");
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    fmt = bdrv->format_name;
> +
> +    if (is_help_option(options)) {
> +        ret = print_block_option_help(filename, fmt);
> +        goto out;
> +    }
> +
> +    create_options = append_option_parameters(create_options,
> +            bdrv->create_options);
> +    options_param = parse_option_parameters(options, create_options,
> +            options_param);
> +    if (options_param == NULL) {
> +        error_report("Invalid options for file format '%s'.", fmt);
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    ret = bdrv_amend_options(bs, options_param);
> +    if (ret < 0) {
> +        error_report("Error while amending options: %s", strerror(-ret));
> +        goto out;
> +    }
> +
> +out:
> +    if (bs) {
> +        bdrv_delete(bs);
> +    }
> +    if (ret) {
> +        return 1;
> +    }
> +    return 0;
> +}

Some free_option_parameters() calls are missing here.

Kevin

Re: [Qemu-devel] [PATCH 1/3] block: Image file option amendment

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 940 bytes --]

On 08/28/2013 02:08 AM, Max Reitz wrote:
> This patch adds the "amend" option to qemu-img which allows changing
> image options on existing image files. It also adds the generic bdrv
> implementation which is basically just a wrapper for the image format
> specific function.
> 

> +
> +@item amend [-f @var{fmt}] -o @var{options} @var{filename}
> +
> +Amends the image format specific @var{options} for the image file
> +@var{filename}. Only the format @code{qcow2} supports this.

The flattening of implicit zero clusters into explicit may take a
significant amount of time; please consider adding support for -p
(progress meter) as part of this addition.  (I'm already bothered by the
fact that the 'commit' operation lacks a progress meter; and even
'check' might have cases where it could be long-running)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 1/3] block: Image file option amendment

[Kevin Wolf]

Am 28.08.2013 um 14:22 hat Eric Blake geschrieben:
> On 08/28/2013 02:08 AM, Max Reitz wrote:
> > This patch adds the "amend" option to qemu-img which allows changing
> > image options on existing image files. It also adds the generic bdrv
> > implementation which is basically just a wrapper for the image format
> > specific function.
> > 
> 
> > +
> > +@item amend [-f @var{fmt}] -o @var{options} @var{filename}
> > +
> > +Amends the image format specific @var{options} for the image file
> > +@var{filename}. Only the format @code{qcow2} supports this.
> 
> The flattening of implicit zero clusters into explicit may take a
> significant amount of time; please consider adding support for -p
> (progress meter) as part of this addition.  (I'm already bothered by the
> fact that the 'commit' operation lacks a progress meter; and even
> 'check' might have cases where it could be long-running)

Good point. I think we can keep that separate, though. Let's get the
basic functionality in first, and then add a series on top that adds a
progress meter to those qemu-img subcommands for which it makes sense.

Kevin

[Qemu-devel] [PATCH 2/3] qcow2: Implement bdrv_amend_options

[Max Reitz]

Implement bdrv_amend_options for compat, size, backing_file, backing_fmt
and lazy_refcounts.

Downgrading images from compat=1.1 to compat=0.10 is achieved through
handling all incompatible flags accordingly, clearing all compatible and
autoclear flags and expanding all zero clusters.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-cluster.c |  66 ++++++++++++++++++
 block/qcow2.c         | 182 ++++++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.h         |   2 +
 3 files changed, 250 insertions(+)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index cca76d4..ac50db2 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1476,3 +1476,69 @@ fail:
 
     return ret;
 }
+
+/*
+ * Expands all zero clusters on the image; important for downgrading to a qcow2
+ * version which doesn't yet support metadata zero clusters.
+ */
+int qcow2_expand_zero_clusters(BlockDriverState *bs)
+{
+    BDRVQcowState *s = bs->opaque;
+    int ret;
+    int i;
+
+    for (i = 0; i < s->l1_size; i++) {
+        uint64_t *l2_table;
+        int l2_index;
+        int j;
+        bool l2_dirty = false;
+
+        ret = get_cluster_table(bs, (uint64_t)i << (s->l2_bits +
+                s->cluster_bits), &l2_table, &l2_index);
+        if (ret < 0) {
+            return ret;
+        }
+
+        for (j = 0; j < s->l2_size; j++) {
+            uint64_t l2_entry = be64_to_cpu(l2_table[j]);
+            if (!(l2_entry & QCOW_OFLAG_COMPRESSED) &&
+                (l2_entry & QCOW_OFLAG_ZERO)) {
+                /* uncompressed zero cluster */
+                int64_t offset = qcow2_alloc_clusters(bs, s->cluster_size);
+                if (offset < 0) {
+                    ret = offset;
+                    goto fail;
+                }
+
+                ret = bdrv_write_zeroes(bs->file, offset >> BDRV_SECTOR_BITS,
+                                        s->cluster_size >> BDRV_SECTOR_BITS);
+                if (ret < 0) {
+                    qcow2_free_clusters(bs, offset, s->cluster_size,
+                            QCOW2_DISCARD_ALWAYS);
+                    goto fail;
+                }
+
+                l2_table[j] = cpu_to_be64(offset | QCOW_OFLAG_COPIED);
+                l2_dirty = true;
+            }
+        }
+
+        ret = 0;
+
+fail:
+        if (l2_dirty) {
+            qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_table);
+        }
+
+        if (ret < 0) {
+            qcow2_cache_put(bs, s->l2_table_cache, (void **)&l2_table);
+        } else {
+            ret = qcow2_cache_put(bs, s->l2_table_cache, (void **)&l2_table);
+        }
+        if (ret < 0) {
+            return ret;
+        }
+    }
+
+    return 0;
+}
diff --git a/block/qcow2.c b/block/qcow2.c
index 78097e5..47cd5ad 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1735,6 +1735,187 @@ static int qcow2_load_vmstate(BlockDriverState *bs, uint8_t *buf,
     return ret;
 }
 
+/*
+ * Downgrades an image's version. To achieve this, any incompatible features
+ * have to be removed.
+ */
+static int qcow2_downgrade(BlockDriverState *bs, int target_version)
+{
+    BDRVQcowState *s = bs->opaque;
+    int current_version = s->qcow_version;
+    int ret;
+
+    if (target_version == current_version) {
+        return 0;
+    } else if (target_version > current_version) {
+        return -EINVAL;
+    } else if (target_version != 2) {
+        return -EINVAL;
+    }
+
+    /* clear incompatible features */
+    if (s->incompatible_features & QCOW2_INCOMPAT_DIRTY) {
+        BdrvCheckResult result;
+        ret = qcow2_check(bs, &result, BDRV_FIX_ERRORS);
+        if (ret < 0) {
+            return ret;
+        }
+        qcow2_mark_clean(bs);
+    }
+
+    if (s->incompatible_features) {
+        return -ENOTSUP;
+    }
+
+    /* since we can ignore compatible features, we can set them to 0 as well */
+    s->compatible_features = 0;
+    /* if lazy refcounts have been used, they have already been fixed through
+     * clearing the dirty flag */
+
+    /* clearing autoclear features is trivial */
+    s->autoclear_features = 0;
+
+    /* the refcount order might be different in newer images - however, qemu
+     * doesn't support anything different than 4 anyway, so nothing to fix
+     * there */
+
+    ret = qcow2_expand_zero_clusters(bs);
+    if (ret < 0) {
+        return ret;
+    }
+
+    s->qcow_version = target_version;
+    ret = qcow2_update_header(bs);
+    if (ret < 0) {
+        s->qcow_version = current_version;
+        return ret;
+    }
+    return 0;
+}
+
+static int qcow2_amend_options(BlockDriverState *bs,
+                               QEMUOptionParameter *options)
+{
+    BDRVQcowState *s = bs->opaque;
+    int old_version = s->qcow_version, new_version = old_version;
+    uint64_t new_size = 0;
+    const char *backing_file = NULL, *backing_format = NULL;
+    bool lazy_refcounts = s->use_lazy_refcounts;
+    int ret;
+    int i;
+
+    for (i = 0; options[i].name; i++)
+    {
+        if (!strcmp(options[i].name, "compat")) {
+            if (!options[i].value.s) {
+                /* preserve default */
+            } else if (!strcmp(options[i].value.s, "0.10")) {
+                new_version = 2;
+            } else if (!strcmp(options[i].value.s, "1.1")) {
+                new_version = 3;
+            } else {
+                fprintf(stderr, "Unknown compatibility level %s.\n",
+                        options[i].value.s);
+                return -EINVAL;
+            }
+        } else if (!strcmp(options[i].name, "preallocation")) {
+            if (options[i].value.s) {
+                fprintf(stderr, "Cannot change preallocation mode.\n");
+                return -ENOTSUP;
+            }
+        } else if (!strcmp(options[i].name, "size")) {
+            new_size = options[i].value.n;
+        } else if (!strcmp(options[i].name, "backing_file")) {
+            backing_file = options[i].value.s;
+        } else if (!strcmp(options[i].name, "backing_fmt")) {
+            backing_format = options[i].value.s;
+        } else if (!strcmp(options[i].name, "encryption")) {
+            if (options[i].value.n != !!s->crypt_method) {
+                fprintf(stderr, "Changing the encryption flag is not "
+                        "supported.\n");
+                return -ENOTSUP;
+            }
+        } else if (!strcmp(options[i].name, "cluster_size")) {
+            if (options[i].value.n && (options[i].value.n != s->cluster_size)) {
+                fprintf(stderr, "Changing the cluster size is not "
+                        "supported.\n");
+                return -ENOTSUP;
+            }
+        } else if (!strcmp(options[i].name, "lazy_refcounts")) {
+            /* TODO: detect whether this flag was indeed explicitly given */
+            lazy_refcounts = options[i].value.n;
+        } else {
+            fprintf(stderr, "Unknown option '%s'.\n", options[i].name);
+        }
+    }
+
+    if (new_version != old_version) {
+        if (new_version > old_version) {
+            /* Upgrade */
+            s->qcow_version = new_version;
+            ret = qcow2_update_header(bs);
+            if (ret < 0) {
+                s->qcow_version = old_version;
+                return ret;
+            }
+        } else {
+            qcow2_downgrade(bs, new_version);
+        }
+    }
+
+    if (new_size) {
+        ret = qcow2_truncate(bs, new_size);
+        if (ret < 0) {
+            return ret;
+        }
+    }
+
+    if (backing_file || backing_format) {
+        ret = qcow2_change_backing_file(bs, backing_file ?: bs->backing_file,
+                                        backing_format ?: bs->backing_format);
+        if (ret < 0) {
+            return ret;
+        }
+    }
+
+    if (s->use_lazy_refcounts != lazy_refcounts) {
+        if (lazy_refcounts) {
+            if (s->qcow_version < 3) {
+                fprintf(stderr, "Lazy refcounts only supported with compatibility "
+                        "level 1.1 and above (use compat=1.1 or greater)\n");
+                return -EINVAL;
+            }
+            s->compatible_features |= QCOW2_COMPAT_LAZY_REFCOUNTS;
+            ret = qcow2_update_header(bs);
+            if (ret < 0) {
+                s->compatible_features &= ~QCOW2_COMPAT_LAZY_REFCOUNTS;
+                return ret;
+            }
+            s->use_lazy_refcounts = true;
+        } else {
+            /* make image clean first */
+            if (s->incompatible_features & QCOW2_INCOMPAT_DIRTY) {
+                BdrvCheckResult result;
+                ret = qcow2_check(bs, &result, BDRV_FIX_ERRORS);
+                if (ret < 0) {
+                    return ret;
+                }
+            }
+            qcow2_mark_clean(bs);
+            /* now disallow lazy refcounts */
+            s->compatible_features &= ~QCOW2_COMPAT_LAZY_REFCOUNTS;
+            ret = qcow2_update_header(bs);
+            if (ret < 0) {
+                s->compatible_features |= QCOW2_COMPAT_LAZY_REFCOUNTS;
+                return ret;
+            }
+            s->use_lazy_refcounts = false;
+        }
+    }
+
+    return 0;
+}
+
 static QEMUOptionParameter qcow2_create_options[] = {
     {
         .name = BLOCK_OPT_SIZE,
@@ -1818,6 +1999,7 @@ static BlockDriver bdrv_qcow2 = {
 
     .create_options = qcow2_create_options,
     .bdrv_check = qcow2_check,
+    .bdrv_amend_options = qcow2_amend_options,
 };
 
 static void bdrv_qcow2_init(void)
diff --git a/block/qcow2.h b/block/qcow2.h
index dba9771..84109de 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -408,6 +408,8 @@ int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset,
     int nb_sectors);
 int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_sectors);
 
+int qcow2_expand_zero_clusters(BlockDriverState *bs);
+
 /* qcow2-snapshot.c functions */
 int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info);
 int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/3] qcow2: Implement bdrv_amend_options

[Kevin Wolf]

Am 28.08.2013 um 10:08 hat Max Reitz geschrieben:
> Implement bdrv_amend_options for compat, size, backing_file, backing_fmt
> and lazy_refcounts.
> 
> Downgrading images from compat=1.1 to compat=0.10 is achieved through
> handling all incompatible flags accordingly, clearing all compatible and
> autoclear flags and expanding all zero clusters.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-cluster.c |  66 ++++++++++++++++++
>  block/qcow2.c         | 182 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  block/qcow2.h         |   2 +
>  3 files changed, 250 insertions(+)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index cca76d4..ac50db2 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1476,3 +1476,69 @@ fail:
>  
>      return ret;
>  }
> +
> +/*
> + * Expands all zero clusters on the image; important for downgrading to a qcow2
> + * version which doesn't yet support metadata zero clusters.
> + */
> +int qcow2_expand_zero_clusters(BlockDriverState *bs)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +    int ret;
> +    int i;
> +
> +    for (i = 0; i < s->l1_size; i++) {

This fails to expand zero clusters in non-active L2 tables. (Please add
a test case for this scenario.)

> +        uint64_t *l2_table;
> +        int l2_index;
> +        int j;
> +        bool l2_dirty = false;
> +
> +        ret = get_cluster_table(bs, (uint64_t)i << (s->l2_bits +
> +                s->cluster_bits), &l2_table, &l2_index);
> +        if (ret < 0) {
> +            return ret;
> +        }
> +
> +        for (j = 0; j < s->l2_size; j++) {
> +            uint64_t l2_entry = be64_to_cpu(l2_table[j]);
> +            if (!(l2_entry & QCOW_OFLAG_COMPRESSED) &&
> +                (l2_entry & QCOW_OFLAG_ZERO)) {

qcow2_get_cluster_type()?

> +                /* uncompressed zero cluster */
> +                int64_t offset = qcow2_alloc_clusters(bs, s->cluster_size);
> +                if (offset < 0) {
> +                    ret = offset;
> +                    goto fail;
> +                }

Does it handle zero clusters with an offset (i.e. preallocation)
correctly? I believe we must either reuse that cluster or free it.

> +                ret = bdrv_write_zeroes(bs->file, offset >> BDRV_SECTOR_BITS,
> +                                        s->cluster_size >> BDRV_SECTOR_BITS);
> +                if (ret < 0) {
> +                    qcow2_free_clusters(bs, offset, s->cluster_size,
> +                            QCOW2_DISCARD_ALWAYS);
> +                    goto fail;
> +                }
> +
> +                l2_table[j] = cpu_to_be64(offset | QCOW_OFLAG_COPIED);
> +                l2_dirty = true;
> +            }
> +        }
> +
> +        ret = 0;
> +
> +fail:
> +        if (l2_dirty) {
> +            qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_table);

qcow2_cache_depends_on_flush(s->l2_table_cache), too. The L2 table must
only be written when the zeroes are stable on disk.

> +        }
> +
> +        if (ret < 0) {
> +            qcow2_cache_put(bs, s->l2_table_cache, (void **)&l2_table);
> +        } else {
> +            ret = qcow2_cache_put(bs, s->l2_table_cache, (void **)&l2_table);
> +        }
> +        if (ret < 0) {
> +            return ret;
> +        }
> +    }
> +
> +    return 0;
> +}
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 78097e5..47cd5ad 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1735,6 +1735,187 @@ static int qcow2_load_vmstate(BlockDriverState *bs, uint8_t *buf,
>      return ret;
>  }
>  
> +/*
> + * Downgrades an image's version. To achieve this, any incompatible features
> + * have to be removed.
> + */
> +static int qcow2_downgrade(BlockDriverState *bs, int target_version)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +    int current_version = s->qcow_version;
> +    int ret;
> +
> +    if (target_version == current_version) {
> +        return 0;
> +    } else if (target_version > current_version) {
> +        return -EINVAL;
> +    } else if (target_version != 2) {
> +        return -EINVAL;
> +    }
> +
> +    /* clear incompatible features */
> +    if (s->incompatible_features & QCOW2_INCOMPAT_DIRTY) {
> +        BdrvCheckResult result;
> +        ret = qcow2_check(bs, &result, BDRV_FIX_ERRORS);
> +        if (ret < 0) {
> +            return ret;
> +        }

This is unnecessary: The image could be opened, so we know that it was
clean when we started. We also know that we haven't crashed yet, so if we
flush all in-memory data, we'll have a consistent on-disk state again.

qcow2_mark_clean() already calls bdrv_flush(bs), so it does everything
that is needed in this respect.

> +        qcow2_mark_clean(bs);

However, it can return errors, for which we should check.

> +    }
> +
> +    if (s->incompatible_features) {
> +        return -ENOTSUP;
> +    }
> +
> +    /* since we can ignore compatible features, we can set them to 0 as well */
> +    s->compatible_features = 0;
> +    /* if lazy refcounts have been used, they have already been fixed through
> +     * clearing the dirty flag */
> +
> +    /* clearing autoclear features is trivial */
> +    s->autoclear_features = 0;
> +
> +    /* the refcount order might be different in newer images - however, qemu
> +     * doesn't support anything different than 4 anyway, so nothing to fix
> +     * there */
> +
> +    ret = qcow2_expand_zero_clusters(bs);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    s->qcow_version = target_version;
> +    ret = qcow2_update_header(bs);
> +    if (ret < 0) {
> +        s->qcow_version = current_version;
> +        return ret;
> +    }
> +    return 0;
> +}
> +
> +static int qcow2_amend_options(BlockDriverState *bs,
> +                               QEMUOptionParameter *options)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +    int old_version = s->qcow_version, new_version = old_version;
> +    uint64_t new_size = 0;
> +    const char *backing_file = NULL, *backing_format = NULL;
> +    bool lazy_refcounts = s->use_lazy_refcounts;
> +    int ret;
> +    int i;
> +
> +    for (i = 0; options[i].name; i++)
> +    {
> +        if (!strcmp(options[i].name, "compat")) {
> +            if (!options[i].value.s) {
> +                /* preserve default */
> +            } else if (!strcmp(options[i].value.s, "0.10")) {
> +                new_version = 2;
> +            } else if (!strcmp(options[i].value.s, "1.1")) {
> +                new_version = 3;
> +            } else {
> +                fprintf(stderr, "Unknown compatibility level %s.\n",
> +                        options[i].value.s);
> +                return -EINVAL;
> +            }
> +        } else if (!strcmp(options[i].name, "preallocation")) {
> +            if (options[i].value.s) {
> +                fprintf(stderr, "Cannot change preallocation mode.\n");
> +                return -ENOTSUP;
> +            }
> +        } else if (!strcmp(options[i].name, "size")) {
> +            new_size = options[i].value.n;
> +        } else if (!strcmp(options[i].name, "backing_file")) {
> +            backing_file = options[i].value.s;
> +        } else if (!strcmp(options[i].name, "backing_fmt")) {
> +            backing_format = options[i].value.s;
> +        } else if (!strcmp(options[i].name, "encryption")) {
> +            if (options[i].value.n != !!s->crypt_method) {
> +                fprintf(stderr, "Changing the encryption flag is not "
> +                        "supported.\n");
> +                return -ENOTSUP;
> +            }
> +        } else if (!strcmp(options[i].name, "cluster_size")) {
> +            if (options[i].value.n && (options[i].value.n != s->cluster_size)) {
> +                fprintf(stderr, "Changing the cluster size is not "
> +                        "supported.\n");
> +                return -ENOTSUP;
> +            }
> +        } else if (!strcmp(options[i].name, "lazy_refcounts")) {
> +            /* TODO: detect whether this flag was indeed explicitly given */
> +            lazy_refcounts = options[i].value.n;

I can see two ways to achieve this:

1. Add a new field 'bool assigned' to QEMUOptionParameter, which would
   be cleared before parsing an option string and set for each option in
   set_option_parameter()

2. Get the QemuOpts conversion series in and add a function that tells
   whether a given option was specified or not.

The same TODO should actually apply to encryption and cluster_size as
well, shouldn't it?

> +        } else {
> +            fprintf(stderr, "Unknown option '%s'.\n", options[i].name);

That's actually a programming error, perhaps a case for assert(false);

> +        }
> +    }
> +
> +    if (new_version != old_version) {
> +        if (new_version > old_version) {
> +            /* Upgrade */
> +            s->qcow_version = new_version;
> +            ret = qcow2_update_header(bs);
> +            if (ret < 0) {
> +                s->qcow_version = old_version;
> +                return ret;
> +            }
> +        } else {
> +            qcow2_downgrade(bs, new_version);

Error handling?

> +        }
> +    }
> +
> +    if (new_size) {
> +        ret = qcow2_truncate(bs, new_size);
> +        if (ret < 0) {
> +            return ret;
> +        }
> +    }
> +
> +    if (backing_file || backing_format) {
> +        ret = qcow2_change_backing_file(bs, backing_file ?: bs->backing_file,
> +                                        backing_format ?: bs->backing_format);
> +        if (ret < 0) {
> +            return ret;
> +        }
> +    }
> +
> +    if (s->use_lazy_refcounts != lazy_refcounts) {
> +        if (lazy_refcounts) {
> +            if (s->qcow_version < 3) {
> +                fprintf(stderr, "Lazy refcounts only supported with compatibility "
> +                        "level 1.1 and above (use compat=1.1 or greater)\n");
> +                return -EINVAL;
> +            }
> +            s->compatible_features |= QCOW2_COMPAT_LAZY_REFCOUNTS;
> +            ret = qcow2_update_header(bs);
> +            if (ret < 0) {
> +                s->compatible_features &= ~QCOW2_COMPAT_LAZY_REFCOUNTS;
> +                return ret;
> +            }
> +            s->use_lazy_refcounts = true;
> +        } else {
> +            /* make image clean first */
> +            if (s->incompatible_features & QCOW2_INCOMPAT_DIRTY) {
> +                BdrvCheckResult result;
> +                ret = qcow2_check(bs, &result, BDRV_FIX_ERRORS);
> +                if (ret < 0) {
> +                    return ret;
> +                }
> +            }

Unnecessary, like above.

> +            qcow2_mark_clean(bs);

And error handling again.

> +            /* now disallow lazy refcounts */
> +            s->compatible_features &= ~QCOW2_COMPAT_LAZY_REFCOUNTS;
> +            ret = qcow2_update_header(bs);
> +            if (ret < 0) {
> +                s->compatible_features |= QCOW2_COMPAT_LAZY_REFCOUNTS;
> +                return ret;
> +            }
> +            s->use_lazy_refcounts = false;
> +        }
> +    }
> +
> +    return 0;
> +}

Kevin

Re: [Qemu-devel] [PATCH 2/3] qcow2: Implement bdrv_amend_options

[Max Reitz]

Hi,

Am 28.08.2013 13:06, schrieb Kevin Wolf:
> Am 28.08.2013 um 10:08 hat Max Reitz geschrieben:
>> Implement bdrv_amend_options for compat, size, backing_file, backing_fmt
>> and lazy_refcounts.
>>
>> Downgrading images from compat=1.1 to compat=0.10 is achieved through
>> handling all incompatible flags accordingly, clearing all compatible and
>> autoclear flags and expanding all zero clusters.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> +int qcow2_expand_zero_clusters(BlockDriverState *bs)
>> +{
>> +    BDRVQcowState *s = bs->opaque;
>> +    int ret;
>> +    int i;
>> +
>> +    for (i = 0; i < s->l1_size; i++) {
> This fails to expand zero clusters in non-active L2 tables. (Please add
> a test case for this scenario.)
Oh, yes, right.

>> +        uint64_t *l2_table;
>> +        int l2_index;
>> +        int j;
>> +        bool l2_dirty = false;
>> +
>> +        ret = get_cluster_table(bs, (uint64_t)i << (s->l2_bits +
>> +                s->cluster_bits), &l2_table, &l2_index);
>> +        if (ret < 0) {
>> +            return ret;
>> +        }
>> +
>> +        for (j = 0; j < s->l2_size; j++) {
>> +            uint64_t l2_entry = be64_to_cpu(l2_table[j]);
>> +            if (!(l2_entry & QCOW_OFLAG_COMPRESSED) &&
>> +                (l2_entry & QCOW_OFLAG_ZERO)) {
> qcow2_get_cluster_type()?
Sounds like an option to go for.

>> +                /* uncompressed zero cluster */
>> +                int64_t offset = qcow2_alloc_clusters(bs, s->cluster_size);
>> +                if (offset < 0) {
>> +                    ret = offset;
>> +                    goto fail;
>> +                }
> Does it handle zero clusters with an offset (i.e. preallocation)
> correctly? I believe we must either reuse that cluster or free it.
Okay.

>> +                ret = bdrv_write_zeroes(bs->file, offset >> BDRV_SECTOR_BITS,
>> +                                        s->cluster_size >> BDRV_SECTOR_BITS);
>> +                if (ret < 0) {
>> +                    qcow2_free_clusters(bs, offset, s->cluster_size,
>> +                            QCOW2_DISCARD_ALWAYS);
>> +                    goto fail;
>> +                }
>> +
>> +                l2_table[j] = cpu_to_be64(offset | QCOW_OFLAG_COPIED);
>> +                l2_dirty = true;
>> +            }
>> +        }
>> +
>> +        ret = 0;
>> +
>> +fail:
>> +        if (l2_dirty) {
>> +            qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_table);
> qcow2_cache_depends_on_flush(s->l2_table_cache), too. The L2 table must
> only be written when the zeroes are stable on disk.
Okay.

>> +    /* clear incompatible features */
>> +    if (s->incompatible_features & QCOW2_INCOMPAT_DIRTY) {
>> +        BdrvCheckResult result;
>> +        ret = qcow2_check(bs, &result, BDRV_FIX_ERRORS);
>> +        if (ret < 0) {
>> +            return ret;
>> +        }
> This is unnecessary: The image could be opened, so we know that it was
> clean when we started. We also know that we haven't crashed yet, so if we
> flush all in-memory data, we'll have a consistent on-disk state again.
>
> qcow2_mark_clean() already calls bdrv_flush(bs), so it does everything
> that is needed in this respect.
So that flag should always be already cleared at this point anyway?

>> +        } else if (!strcmp(options[i].name, "encryption")) {
>> +            if (options[i].value.n != !!s->crypt_method) {
>> +                fprintf(stderr, "Changing the encryption flag is not "
>> +                        "supported.\n");
>> +                return -ENOTSUP;
>> +            }
>> +        } else if (!strcmp(options[i].name, "cluster_size")) {
>> +            if (options[i].value.n && (options[i].value.n != s->cluster_size)) {
>> +                fprintf(stderr, "Changing the cluster size is not "
>> +                        "supported.\n");
>> +                return -ENOTSUP;
>> +            }
>> +        } else if (!strcmp(options[i].name, "lazy_refcounts")) {
>> +            /* TODO: detect whether this flag was indeed explicitly given */
>> +            lazy_refcounts = options[i].value.n;
> I can see two ways to achieve this:
>
> 1. Add a new field 'bool assigned' to QEMUOptionParameter, which would
>     be cleared before parsing an option string and set for each option in
>     set_option_parameter()
>
> 2. Get the QemuOpts conversion series in and add a function that tells
>     whether a given option was specified or not.
>
> The same TODO should actually apply to encryption and cluster_size as
> well, shouldn't it?
Kind of; however, a cluster_size of 0 seems invalid to me, therefore it 
is pretty easy to detect that option not being given.

The TODO rather also applies to encryption; however, since the worst it 
does there is generate an error message, it isn't as bad as here (were 
the code might actually change the image if the flag is not given).

>> +        } else {
>> +            fprintf(stderr, "Unknown option '%s'.\n", options[i].name);
> That's actually a programming error, perhaps a case for assert(false);
True.

>> +        }
>> +    }
>> +
>> +    if (new_version != old_version) {
>> +        if (new_version > old_version) {
>> +            /* Upgrade */
>> +            s->qcow_version = new_version;
>> +            ret = qcow2_update_header(bs);
>> +            if (ret < 0) {
>> +                s->qcow_version = old_version;
>> +                return ret;
>> +            }
>> +        } else {
>> +            qcow2_downgrade(bs, new_version);
> Error handling?
Oh, right, forgot it. Sorry.


Max

Re: [Qemu-devel] [PATCH 2/3] qcow2: Implement bdrv_amend_options

[Kevin Wolf]

Am 28.08.2013 um 13:39 hat Max Reitz geschrieben:
> >>+    /* clear incompatible features */
> >>+    if (s->incompatible_features & QCOW2_INCOMPAT_DIRTY) {
> >>+        BdrvCheckResult result;
> >>+        ret = qcow2_check(bs, &result, BDRV_FIX_ERRORS);
> >>+        if (ret < 0) {
> >>+            return ret;
> >>+        }
> >This is unnecessary: The image could be opened, so we know that it was
> >clean when we started. We also know that we haven't crashed yet, so if we
> >flush all in-memory data, we'll have a consistent on-disk state again.
> >
> >qcow2_mark_clean() already calls bdrv_flush(bs), so it does everything
> >that is needed in this respect.
> So that flag should always be already cleared at this point anyway?

The qcow2_mark_clean() call is on the next line (which you removed from
the quote), so not at this point but one line later. But yeah, it's done
by other code.

> >>+        } else if (!strcmp(options[i].name, "encryption")) {
> >>+            if (options[i].value.n != !!s->crypt_method) {
> >>+                fprintf(stderr, "Changing the encryption flag is not "
> >>+                        "supported.\n");
> >>+                return -ENOTSUP;
> >>+            }
> >>+        } else if (!strcmp(options[i].name, "cluster_size")) {
> >>+            if (options[i].value.n && (options[i].value.n != s->cluster_size)) {
> >>+                fprintf(stderr, "Changing the cluster size is not "
> >>+                        "supported.\n");
> >>+                return -ENOTSUP;
> >>+            }
> >>+        } else if (!strcmp(options[i].name, "lazy_refcounts")) {
> >>+            /* TODO: detect whether this flag was indeed explicitly given */
> >>+            lazy_refcounts = options[i].value.n;
> >I can see two ways to achieve this:
> >
> >1. Add a new field 'bool assigned' to QEMUOptionParameter, which would
> >    be cleared before parsing an option string and set for each option in
> >    set_option_parameter()
> >
> >2. Get the QemuOpts conversion series in and add a function that tells
> >    whether a given option was specified or not.
> >
> >The same TODO should actually apply to encryption and cluster_size as
> >well, shouldn't it?
> Kind of; however, a cluster_size of 0 seems invalid to me, therefore
> it is pretty easy to detect that option not being given.

Depends on whether you think that 'qemu-img amend -o cluster_size=0' is
a valid way of saying that you don't want to change the cluster size. I
would prefer to error out.

> The TODO rather also applies to encryption; however, since the worst
> it does there is generate an error message, it isn't as bad as here
> (were the code might actually change the image if the flag is not
> given).

Agreed, it "only" requires that the user specify the encryption flag
explicitly when changing options of an encrypted image. No disaster
happens, it's just unfriendly.

Kevin

Re: [Qemu-devel] [PATCH 2/3] qcow2: Implement bdrv_amend_options

[Max Reitz]

Hi,

Am 28.08.2013 13:48, schrieb Kevin Wolf:
> Am 28.08.2013 um 13:39 hat Max Reitz geschrieben:
>>>> +    /* clear incompatible features */
>>>> +    if (s->incompatible_features & QCOW2_INCOMPAT_DIRTY) {
>>>> +        BdrvCheckResult result;
>>>> +        ret = qcow2_check(bs, &result, BDRV_FIX_ERRORS);
>>>> +        if (ret < 0) {
>>>> +            return ret;
>>>> +        }
>>> This is unnecessary: The image could be opened, so we know that it was
>>> clean when we started. We also know that we haven't crashed yet, so if we
>>> flush all in-memory data, we'll have a consistent on-disk state again.
>>>
>>> qcow2_mark_clean() already calls bdrv_flush(bs), so it does everything
>>> that is needed in this respect.
>> So that flag should always be already cleared at this point anyway?
> The qcow2_mark_clean() call is on the next line (which you removed from
> the quote), so not at this point but one line later. But yeah, it's done
> by other code.
Yes, I was referring to other code (which cleans the image right at 
opening).

>>>> +        } else if (!strcmp(options[i].name, "encryption")) {
>>>> +            if (options[i].value.n != !!s->crypt_method) {
>>>> +                fprintf(stderr, "Changing the encryption flag is not "
>>>> +                        "supported.\n");
>>>> +                return -ENOTSUP;
>>>> +            }
>>>> +        } else if (!strcmp(options[i].name, "cluster_size")) {
>>>> +            if (options[i].value.n && (options[i].value.n != s->cluster_size)) {
>>>> +                fprintf(stderr, "Changing the cluster size is not "
>>>> +                        "supported.\n");
>>>> +                return -ENOTSUP;
>>>> +            }
>>>> +        } else if (!strcmp(options[i].name, "lazy_refcounts")) {
>>>> +            /* TODO: detect whether this flag was indeed explicitly given */
>>>> +            lazy_refcounts = options[i].value.n;
>>> I can see two ways to achieve this:
>>>
>>> 1. Add a new field 'bool assigned' to QEMUOptionParameter, which would
>>>     be cleared before parsing an option string and set for each option in
>>>     set_option_parameter()
>>>
>>> 2. Get the QemuOpts conversion series in and add a function that tells
>>>     whether a given option was specified or not.
>>>
>>> The same TODO should actually apply to encryption and cluster_size as
>>> well, shouldn't it?
>> Kind of; however, a cluster_size of 0 seems invalid to me, therefore
>> it is pretty easy to detect that option not being given.
> Depends on whether you think that 'qemu-img amend -o cluster_size=0' is
> a valid way of saying that you don't want to change the cluster size. I
> would prefer to error out.
No, I just missed the default for that option not being zero. Sorry, my 
fault.


Max

Re: [Qemu-devel] [PATCH 2/3] qcow2: Implement bdrv_amend_options

[Kevin Wolf]

Am 28.08.2013 um 14:05 hat Max Reitz geschrieben:
> Hi,
> 
> Am 28.08.2013 13:48, schrieb Kevin Wolf:
> >Am 28.08.2013 um 13:39 hat Max Reitz geschrieben:
> >>>>+    /* clear incompatible features */
> >>>>+    if (s->incompatible_features & QCOW2_INCOMPAT_DIRTY) {
> >>>>+        BdrvCheckResult result;
> >>>>+        ret = qcow2_check(bs, &result, BDRV_FIX_ERRORS);
> >>>>+        if (ret < 0) {
> >>>>+            return ret;
> >>>>+        }
> >>>This is unnecessary: The image could be opened, so we know that it was
> >>>clean when we started. We also know that we haven't crashed yet, so if we
> >>>flush all in-memory data, we'll have a consistent on-disk state again.
> >>>
> >>>qcow2_mark_clean() already calls bdrv_flush(bs), so it does everything
> >>>that is needed in this respect.
> >>So that flag should always be already cleared at this point anyway?
> >The qcow2_mark_clean() call is on the next line (which you removed from
> >the quote), so not at this point but one line later. But yeah, it's done
> >by other code.
> Yes, I was referring to other code (which cleans the image right at
> opening).

Well, yes and no then.

Yes, qcow2_open() checks the dirty flag and when it's set, it repairs
the image, which in turn clears the dirty flag.

No, this does not mean that the flag is clear at this point. The next
cluster allocation makes the image dirty again, but we have all
information about the cluster allocation in memory, so a simple flush
makes the image consistent. That's why qcow2_mark_clean() must be called
here, and why calling only this is sufficient.

> >>>>+        } else if (!strcmp(options[i].name, "encryption")) {
> >>>>+            if (options[i].value.n != !!s->crypt_method) {
> >>>>+                fprintf(stderr, "Changing the encryption flag is not "
> >>>>+                        "supported.\n");
> >>>>+                return -ENOTSUP;
> >>>>+            }
> >>>>+        } else if (!strcmp(options[i].name, "cluster_size")) {
> >>>>+            if (options[i].value.n && (options[i].value.n != s->cluster_size)) {
> >>>>+                fprintf(stderr, "Changing the cluster size is not "
> >>>>+                        "supported.\n");
> >>>>+                return -ENOTSUP;
> >>>>+            }
> >>>>+        } else if (!strcmp(options[i].name, "lazy_refcounts")) {
> >>>>+            /* TODO: detect whether this flag was indeed explicitly given */
> >>>>+            lazy_refcounts = options[i].value.n;
> >>>I can see two ways to achieve this:
> >>>
> >>>1. Add a new field 'bool assigned' to QEMUOptionParameter, which would
> >>>    be cleared before parsing an option string and set for each option in
> >>>    set_option_parameter()
> >>>
> >>>2. Get the QemuOpts conversion series in and add a function that tells
> >>>    whether a given option was specified or not.
> >>>
> >>>The same TODO should actually apply to encryption and cluster_size as
> >>>well, shouldn't it?
> >>Kind of; however, a cluster_size of 0 seems invalid to me, therefore
> >>it is pretty easy to detect that option not being given.
> >Depends on whether you think that 'qemu-img amend -o cluster_size=0' is
> >a valid way of saying that you don't want to change the cluster size. I
> >would prefer to error out.
> No, I just missed the default for that option not being zero. Sorry,
> my fault.

Doesn't really change my point: Even if the default was 0, deciding that
the option wasn't given by checking against an invalid value that could
in theory also be passed on the command line is awkward, because it
prevents proper error handling for this specific invalid value.

Kevin

[Qemu-devel] [PATCH 3/3] qemu-iotest: qcow2 image option amendment

[Max Reitz]

Add tests for qemu-img amend on qcow2 image files.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
This test is numbered 061 for not interfering with my "metadata overlap
checks" series (although I will have to adapt my code to those new checks
anyway when/if they are included in master).
---
 tests/qemu-iotests/061     | 121 +++++++++++++++++++
 tests/qemu-iotests/061.out | 289 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 411 insertions(+)
 create mode 100755 tests/qemu-iotests/061
 create mode 100644 tests/qemu-iotests/061.out

diff --git a/tests/qemu-iotests/061 b/tests/qemu-iotests/061
new file mode 100755
index 0000000..2dd5d9f
--- /dev/null
+++ b/tests/qemu-iotests/061
@@ -0,0 +1,121 @@
+#!/bin/bash
+#
+# Test case for image option amendment in qcow2.
+#
+# Copyright (C) 2013 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=mreitz@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+	_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+# This tests qocw2-specific low-level functionality
+_supported_fmt qcow2
+_supported_proto generic
+_supported_os Linux
+
+echo
+echo "=== Testing version downgrade with zero expansion ==="
+echo
+IMGOPTS="compat=1.1,lazy_refcounts=on" _make_test_img 64M
+$QEMU_IO -c "write -z 0 128k" "$TEST_IMG" | _filter_qemu_io
+./qcow2.py "$TEST_IMG" dump-header
+$QEMU_IMG amend -o "compat=0.10" "$TEST_IMG"
+./qcow2.py "$TEST_IMG" dump-header
+$QEMU_IO -c "read -P 0 0 128k" "$TEST_IMG" | _filter_qemu_io
+
+echo
+echo "=== Testing dirty version downgrade ==="
+echo
+IMGOPTS="compat=1.1,lazy_refcounts=on" _make_test_img 64M
+$QEMU_IO -c "write -P 0x2a 0 128k" -c flush -c abort "$TEST_IMG" | _filter_qemu_io
+./qcow2.py "$TEST_IMG" dump-header
+$QEMU_IMG amend -o "compat=0.10" "$TEST_IMG"
+./qcow2.py "$TEST_IMG" dump-header
+$QEMU_IO -c "read -P 0x2a 0 128k" "$TEST_IMG" | _filter_qemu_io
+
+echo
+echo "=== Testing version downgrade with unknown compat/autoclear flags ==="
+echo
+IMGOPTS="compat=1.1" _make_test_img 64M
+./qcow2.py "$TEST_IMG" set-feature-bit compatible 42
+./qcow2.py "$TEST_IMG" set-feature-bit autoclear 42
+./qcow2.py "$TEST_IMG" dump-header
+$QEMU_IMG amend -o "compat=0.10" "$TEST_IMG"
+./qcow2.py "$TEST_IMG" dump-header
+
+echo
+echo "=== Testing version upgrade and resize ==="
+echo
+IMGOPTS="compat=0.10" _make_test_img 64M
+$QEMU_IO -c "write -P 0x2a 42M 64k" "$TEST_IMG" | _filter_qemu_io
+./qcow2.py "$TEST_IMG" dump-header
+$QEMU_IMG amend -o "compat=1.1,lazy_refcounts=on,size=128M" "$TEST_IMG"
+./qcow2.py "$TEST_IMG" dump-header
+$QEMU_IO -c "read -P 0x2a 42M 64k" "$TEST_IMG" | _filter_qemu_io
+
+echo
+echo "=== Testing dirty lazy_refcounts=off ==="
+echo
+IMGOPTS="compat=1.1,lazy_refcounts=on" _make_test_img 64M
+$QEMU_IO -c "write -P 0x2a 0 128k" -c flush -c abort "$TEST_IMG" | _filter_qemu_io
+./qcow2.py "$TEST_IMG" dump-header
+$QEMU_IMG amend -o "lazy_refcounts=off" "$TEST_IMG"
+./qcow2.py "$TEST_IMG" dump-header
+$QEMU_IO -c "read -P 0x2a 0 128k" "$TEST_IMG" | _filter_qemu_io
+
+echo
+echo "=== Testing backing file ==="
+echo
+IMGOPTS="compat=1.1" _make_test_img 64M
+IMGOPTS="compat=1.1" TEST_IMG="$TEST_IMG.base" _make_test_img 64M
+$QEMU_IO -c "write -P 0x2a 0 128k" "$TEST_IMG.base" | _filter_qemu_io
+$QEMU_IO -c "read -P 0 0 128k" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IMG amend -o "backing_file=$TEST_IMG.base,backing_fmt=qcow2" "$TEST_IMG"
+$QEMU_IO -c "read -P 0x2a 0 128k" "$TEST_IMG" | _filter_qemu_io
+
+echo
+echo "=== Testing invalid configurations ==="
+IMGOPTS="compat=0.10" _make_test_img 64M
+$QEMU_IMG amend -o "lazy_refcounts=on" "$TEST_IMG"
+$QEMU_IMG amend -o "compat=1.1" "$TEST_IMG" # actually valid
+$QEMU_IMG amend -o "compat=0.10,lazy_refcounts=on" "$TEST_IMG"
+$QEMU_IMG amend -o "compat=0.42" "$TEST_IMG"
+$QEMU_IMG amend -o "foo=bar" "$TEST_IMG"
+$QEMU_IMG amend -o "cluster_size=1k" "$TEST_IMG"
+$QEMU_IMG amend -o "encryption=on" "$TEST_IMG"
+$QEMU_IMG amend -o "preallocation=on" "$TEST_IMG"
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/061.out b/tests/qemu-iotests/061.out
new file mode 100644
index 0000000..42ae3f1
--- /dev/null
+++ b/tests/qemu-iotests/061.out
@@ -0,0 +1,289 @@
+QA output created by 061
+
+=== Testing version downgrade with zero expansion ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
+wrote 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+magic                     0x514649fb
+version                   3
+backing_file_offset       0x0
+backing_file_size         0x0
+cluster_bits              16
+size                      67108864
+crypt_method              0
+l1_size                   1
+l1_table_offset           0x30000
+refcount_table_offset     0x10000
+refcount_table_clusters   1
+nb_snapshots              0
+snapshot_offset           0x0
+incompatible_features     0x0
+compatible_features       0x1
+autoclear_features        0x0
+refcount_order            4
+header_length             104
+
+magic                     0x514649fb
+version                   2
+backing_file_offset       0x0
+backing_file_size         0x0
+cluster_bits              16
+size                      67108864
+crypt_method              0
+l1_size                   1
+l1_table_offset           0x30000
+refcount_table_offset     0x10000
+refcount_table_clusters   1
+nb_snapshots              0
+snapshot_offset           0x0
+incompatible_features     0x0
+compatible_features       0x0
+autoclear_features        0x0
+refcount_order            4
+header_length             72
+
+Header extension:
+magic                     0x6803f857
+length                    96
+data                      <binary>
+
+read 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Testing dirty version downgrade ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
+wrote 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+magic                     0x514649fb
+version                   3
+backing_file_offset       0x0
+backing_file_size         0x0
+cluster_bits              16
+size                      67108864
+crypt_method              0
+l1_size                   1
+l1_table_offset           0x30000
+refcount_table_offset     0x10000
+refcount_table_clusters   1
+nb_snapshots              0
+snapshot_offset           0x0
+incompatible_features     0x1
+compatible_features       0x1
+autoclear_features        0x0
+refcount_order            4
+header_length             104
+
+ERROR OFLAG_COPIED: offset=8000000000050000 refcount=0
+ERROR OFLAG_COPIED: offset=8000000000060000 refcount=0
+Repairing cluster 5 refcount=0 reference=1
+Repairing cluster 6 refcount=0 reference=1
+magic                     0x514649fb
+version                   2
+backing_file_offset       0x0
+backing_file_size         0x0
+cluster_bits              16
+size                      67108864
+crypt_method              0
+l1_size                   1
+l1_table_offset           0x30000
+refcount_table_offset     0x10000
+refcount_table_clusters   1
+nb_snapshots              0
+snapshot_offset           0x0
+incompatible_features     0x0
+compatible_features       0x0
+autoclear_features        0x0
+refcount_order            4
+header_length             72
+
+Header extension:
+magic                     0x6803f857
+length                    96
+data                      <binary>
+
+read 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Testing version downgrade with unknown compat/autoclear flags ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
+magic                     0x514649fb
+version                   3
+backing_file_offset       0x0
+backing_file_size         0x0
+cluster_bits              16
+size                      67108864
+crypt_method              0
+l1_size                   1
+l1_table_offset           0x30000
+refcount_table_offset     0x10000
+refcount_table_clusters   1
+nb_snapshots              0
+snapshot_offset           0x0
+incompatible_features     0x0
+compatible_features       0x40000000000
+autoclear_features        0x40000000000
+refcount_order            4
+header_length             104
+
+magic                     0x514649fb
+version                   2
+backing_file_offset       0x0
+backing_file_size         0x0
+cluster_bits              16
+size                      67108864
+crypt_method              0
+l1_size                   1
+l1_table_offset           0x30000
+refcount_table_offset     0x10000
+refcount_table_clusters   1
+nb_snapshots              0
+snapshot_offset           0x0
+incompatible_features     0x0
+compatible_features       0x0
+autoclear_features        0x0
+refcount_order            4
+header_length             72
+
+Header extension:
+magic                     0x6803f857
+length                    96
+data                      <binary>
+
+
+=== Testing version upgrade and resize ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
+wrote 65536/65536 bytes at offset 44040192
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+magic                     0x514649fb
+version                   2
+backing_file_offset       0x0
+backing_file_size         0x0
+cluster_bits              16
+size                      67108864
+crypt_method              0
+l1_size                   1
+l1_table_offset           0x30000
+refcount_table_offset     0x10000
+refcount_table_clusters   1
+nb_snapshots              0
+snapshot_offset           0x0
+incompatible_features     0x0
+compatible_features       0x0
+autoclear_features        0x0
+refcount_order            4
+header_length             72
+
+magic                     0x514649fb
+version                   3
+backing_file_offset       0x0
+backing_file_size         0x0
+cluster_bits              16
+size                      67108864
+crypt_method              0
+l1_size                   1
+l1_table_offset           0x30000
+refcount_table_offset     0x10000
+refcount_table_clusters   1
+nb_snapshots              0
+snapshot_offset           0x0
+incompatible_features     0x0
+compatible_features       0x1
+autoclear_features        0x0
+refcount_order            4
+header_length             104
+
+Header extension:
+magic                     0x6803f857
+length                    96
+data                      <binary>
+
+read 65536/65536 bytes at offset 44040192
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Testing dirty lazy_refcounts=off ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
+wrote 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+magic                     0x514649fb
+version                   3
+backing_file_offset       0x0
+backing_file_size         0x0
+cluster_bits              16
+size                      67108864
+crypt_method              0
+l1_size                   1
+l1_table_offset           0x30000
+refcount_table_offset     0x10000
+refcount_table_clusters   1
+nb_snapshots              0
+snapshot_offset           0x0
+incompatible_features     0x1
+compatible_features       0x1
+autoclear_features        0x0
+refcount_order            4
+header_length             104
+
+ERROR OFLAG_COPIED: offset=8000000000050000 refcount=0
+ERROR OFLAG_COPIED: offset=8000000000060000 refcount=0
+Repairing cluster 5 refcount=0 reference=1
+Repairing cluster 6 refcount=0 reference=1
+magic                     0x514649fb
+version                   3
+backing_file_offset       0x0
+backing_file_size         0x0
+cluster_bits              16
+size                      67108864
+crypt_method              0
+l1_size                   1
+l1_table_offset           0x30000
+refcount_table_offset     0x10000
+refcount_table_clusters   1
+nb_snapshots              0
+snapshot_offset           0x0
+incompatible_features     0x0
+compatible_features       0x0
+autoclear_features        0x0
+refcount_order            4
+header_length             104
+
+Header extension:
+magic                     0x6803f857
+length                    96
+data                      <binary>
+
+read 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Testing backing file ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864 
+wrote 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Testing invalid configurations ===
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
+Lazy refcounts only supported with compatibility level 1.1 and above (use compat=1.1 or greater)
+qemu-img: Error while amending options: Invalid argument
+Lazy refcounts only supported with compatibility level 1.1 and above (use compat=1.1 or greater)
+qemu-img: Error while amending options: Invalid argument
+Unknown compatibility level 0.42.
+qemu-img: Error while amending options: Invalid argument
+Unknown option 'foo'
+qemu-img: Invalid options for file format 'qcow2'.
+Changing the cluster size is not supported.
+qemu-img: Error while amending options: Operation not supported
+Changing the encryption flag is not supported.
+qemu-img: Error while amending options: Operation not supported
+Cannot change preallocation mode.
+qemu-img: Error while amending options: Operation not supported
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 43c05d6..48723f4 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -64,3 +64,4 @@
 055 rw auto
 056 rw auto backing
 059 rw auto
+061 rw auto
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 3/3] qemu-iotest: qcow2 image option amendment

[Kevin Wolf]

Am 28.08.2013 um 10:08 hat Max Reitz geschrieben:
> Add tests for qemu-img amend on qcow2 image files.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> This test is numbered 061 for not interfering with my "metadata overlap
> checks" series (although I will have to adapt my code to those new checks
> anyway when/if they are included in master).
> ---
>  tests/qemu-iotests/061     | 121 +++++++++++++++++++
>  tests/qemu-iotests/061.out | 289 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 411 insertions(+)
>  create mode 100755 tests/qemu-iotests/061
>  create mode 100644 tests/qemu-iotests/061.out

The existing cases look good. I would add some _check_test_img calls
after each amendment, and perhaps include a few cases with non-standard
cluster_size etc. during image creation (which would show that your TODO
isn't addressed yet).

Kevin

Re: [Qemu-devel] [PATCH 3/3] qemu-iotest: qcow2 image option amendment

[Max Reitz]

Am 28.08.2013 13:40, schrieb Kevin Wolf:
> Am 28.08.2013 um 10:08 hat Max Reitz geschrieben:
>> Add tests for qemu-img amend on qcow2 image files.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> This test is numbered 061 for not interfering with my "metadata overlap
>> checks" series (although I will have to adapt my code to those new checks
>> anyway when/if they are included in master).
>> ---
>>   tests/qemu-iotests/061     | 121 +++++++++++++++++++
>>   tests/qemu-iotests/061.out | 289 +++++++++++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/group   |   1 +
>>   3 files changed, 411 insertions(+)
>>   create mode 100755 tests/qemu-iotests/061
>>   create mode 100644 tests/qemu-iotests/061.out
> The existing cases look good. I would add some _check_test_img calls
> after each amendment, and perhaps include a few cases with non-standard
> cluster_size etc. during image creation (which would show that your TODO
> isn't addressed yet).
>
> Kevin
>
Ah, right, because it won't be 0 but DEFAULT_CLUSTER_SIZE then… Hm. So, 
generally, right now I can just check whether the value differs and is 
not DEFAULT_CLUSTER_SIZE (which would obviously silently ignore the flag 
when cluster_size=$DEFAULT_CLUSTER_SIZE was given)?

Max

Re: [Qemu-devel] [PATCH 3/3] qemu-iotest: qcow2 image option amendment

[Kevin Wolf]

Am 28.08.2013 um 13:47 hat Max Reitz geschrieben:
> Am 28.08.2013 13:40, schrieb Kevin Wolf:
> >Am 28.08.2013 um 10:08 hat Max Reitz geschrieben:
> >>Add tests for qemu-img amend on qcow2 image files.
> >>
> >>Signed-off-by: Max Reitz <mreitz@redhat.com>
> >>---
> >>This test is numbered 061 for not interfering with my "metadata overlap
> >>checks" series (although I will have to adapt my code to those new checks
> >>anyway when/if they are included in master).
> >>---
> >>  tests/qemu-iotests/061     | 121 +++++++++++++++++++
> >>  tests/qemu-iotests/061.out | 289 +++++++++++++++++++++++++++++++++++++++++++++
> >>  tests/qemu-iotests/group   |   1 +
> >>  3 files changed, 411 insertions(+)
> >>  create mode 100755 tests/qemu-iotests/061
> >>  create mode 100644 tests/qemu-iotests/061.out
> >The existing cases look good. I would add some _check_test_img calls
> >after each amendment, and perhaps include a few cases with non-standard
> >cluster_size etc. during image creation (which would show that your TODO
> >isn't addressed yet).
> >
> >Kevin
> >
> Ah, right, because it won't be 0 but DEFAULT_CLUSTER_SIZE then… Hm.
> So, generally, right now I can just check whether the value differs
> and is not DEFAULT_CLUSTER_SIZE (which would obviously silently
> ignore the flag when cluster_size=$DEFAULT_CLUSTER_SIZE was given)?

I think the TODO needs to be addressed before the series can be merged,
so discussing what you could do or not right now is moot.

Kevin

Re: [Qemu-devel] [PATCH 3/3] qemu-iotest: qcow2 image option amendment

[Max Reitz]

Hi,

Am 28.08.2013 13:54, schrieb Kevin Wolf:
> Am 28.08.2013 um 13:47 hat Max Reitz geschrieben:
>> Am 28.08.2013 13:40, schrieb Kevin Wolf:
>>> Am 28.08.2013 um 10:08 hat Max Reitz geschrieben:
>>>> Add tests for qemu-img amend on qcow2 image files.
>>>>
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>> ---
>>>> This test is numbered 061 for not interfering with my "metadata overlap
>>>> checks" series (although I will have to adapt my code to those new checks
>>>> anyway when/if they are included in master).
>>>> ---
>>>>   tests/qemu-iotests/061     | 121 +++++++++++++++++++
>>>>   tests/qemu-iotests/061.out | 289 +++++++++++++++++++++++++++++++++++++++++++++
>>>>   tests/qemu-iotests/group   |   1 +
>>>>   3 files changed, 411 insertions(+)
>>>>   create mode 100755 tests/qemu-iotests/061
>>>>   create mode 100644 tests/qemu-iotests/061.out
>>> The existing cases look good. I would add some _check_test_img calls
>>> after each amendment, and perhaps include a few cases with non-standard
>>> cluster_size etc. during image creation (which would show that your TODO
>>> isn't addressed yet).
>>>
>>> Kevin
>>>
>> Ah, right, because it won't be 0 but DEFAULT_CLUSTER_SIZE then… Hm.
>> So, generally, right now I can just check whether the value differs
>> and is not DEFAULT_CLUSTER_SIZE (which would obviously silently
>> ignore the flag when cluster_size=$DEFAULT_CLUSTER_SIZE was given)?
> I think the TODO needs to be addressed before the series can be merged,
> so discussing what you could do or not right now is moot.
>
> Kevin
Agreed.


Max