[Qemu-devel] [PATCH V2 0/8] add Qcow2 compress format extension

[Qemu-devel] [PATCH V2 0/8] add Qcow2 compress format extension

[Peter Lieven]

this adds a create option for Qcow2 images to specify the compression format
and level for compressed clusters. The series adds 2 algorithms to choose from:
zlib and lzo. zlib is the current default, but with unoptimal settings.
If no compress.format option is specified the old zlib with the old parameters
is used and the created images are backwards compatible with older QEMU version.
As soon as a compression format is specified a new compress format header extension
is written and the Qcow2 images are incompatible with older QEMU versions.

Some numbers for an uncompressed Debian 9 QCOW2 image (size 1148MB):

compress.format     compress time    compressed size   decompress time
none                35.7s            339MB             3.4s
zlib (default)      30.5s            320MB             3.2s
zlib (level 1)      12.8s            348MB             3.2s
lzo                  4.2s            429MB             1.6s

Changes V1->V2:
  - split the series into more patches
  - added an qapi scheme for the compression settings
  - renamed compression_algorithm to compress.format and added compress.level+
  - updated the header extension to carry a variable extra payload and compress
    level.
  - removed extra reservations for header extensions
  - added missing lzo_init and fixed compress overhead for lzo

Peter Lieven (8):
  docs: add compress format extension to qcow2 spec
  qapi: add compress parameters to Qcow2 Blockdev options
  block/qcow2: parse compress create options
  qemu-img: add documentation for compress settings
  block/qcow2: read and write the compress format extension
  block/qcow2: optimize qcow2_co_pwritev_compressed
  block/qcow2: start using the compress format extension
  block/qcow2: add lzo compress format

 block/qcow2-cluster.c     |  73 +++++++++++-----
 block/qcow2.c             | 219 +++++++++++++++++++++++++++++++++++++++-------
 block/qcow2.h             |  33 +++++--
 configure                 |   2 +-
 docs/interop/qcow2.txt    |  43 ++++++++-
 include/block/block_int.h |   2 +
 qapi/block-core.json      |  54 +++++++++++-
 qemu-img.texi             |  22 +++++
 8 files changed, 384 insertions(+), 64 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH V2 1/8] docs: add compress format extension to qcow2 spec

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 docs/interop/qcow2.txt | 43 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 80cdfd0..c01daf3 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -85,7 +85,12 @@ in the description of a field.
                                 be written to (unless for regaining
                                 consistency).
 
-                    Bits 2-63:  Reserved (set to 0)
+                    Bit 2:      Compress format bit.  If and only if this bit
+                                is set then the compress format extension
+                                MUST be present and MUST be parsed and checked
+                                for compatibility.
+
+                    Bits 3-63:  Reserved (set to 0)
 
          80 -  87:  compatible_features
                     Bitmask of compatible features. An implementation can
@@ -135,6 +140,7 @@ be stored. Each extension has a structure like the following:
                         0xE2792ACA - Backing file format name
                         0x6803f857 - Feature name table
                         0x23852875 - Bitmaps extension
+                        0xC03183A3 - Compress format extension
                         other      - Unknown header extension, can be safely
                                      ignored
 
@@ -208,6 +214,41 @@ The fields of the bitmaps extension are:
                    starts. Must be aligned to a cluster boundary.
 
 
+== Compress format extension ==
+
+The compress format extension is an optional header extension. It provides
+the ability to specify the compress algorithm and compress parameters
+that are used for compressed clusters. This new header MUST be present if
+the incompatible-feature bit "compress format bit" is set and MUST be absent
+otherwise.
+
+The fields of the compress format extension are:
+
+    Byte  0 - 15:  compress_format_name (padded with zeros, but not
+                   necessarily null terminated if it has full length)
+
+              16:  compress_level (uint8_t)
+                   0 = default compress level
+                   1 = lowest compress level
+                   x = highest compress level (the highest compress
+                       level may vary for different compress formats)
+
+         17 - 19:  Reserved for future use, must be zero.
+
+         20 - 23:  extra_data_size
+                   Size of compress format specific extra data.
+                   For now, as no format specifies extra data,
+                   extra_data_size is reserved and should be zero.
+
+        variable:  extra_data
+                   Extra data with additional parameters for the compress
+                   format, occupying extra_data_size bytes.
+
+        variable:  Padding to round up the size of compress format extension
+                   to the next multiple of 8. All bytes of the padding must be
+                   zero.
+
+
 == Host cluster management ==
 
 qcow2 manages the allocation of host clusters by maintaining a reference count
-- 
1.9.1

[Qemu-devel] [PATCH V2 2/8] qapi: add compress parameters to Qcow2 Blockdev options

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 qapi/block-core.json | 44 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index f85c223..1574ffb 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2282,6 +2282,43 @@
             'mode':  'Qcow2OverlapCheckMode' } }
 
 ##
+# @Qcow2CompressFormat:
+# @zlib: standard zlib deflate compression
+#
+# Since: 2.10
+##
+{ 'enum': 'Qcow2CompressFormat',
+  'data': [ 'zlib' ] }
+
+##
+# @Qcow2CompressZLib:
+#
+# Since: 2.10
+##
+{ 'struct': 'Qcow2CompressZLib',
+  'data': { } }
+
+##
+# @Qcow2Compress:
+#
+# Specifies the compression format and compression level that should
+# be used for compressed Qcow2 clusters.
+#
+# @format: specifies the compression format to use. (defaults to zlib)
+#
+# @level: specifies the compression level. 0 = default compression,
+#         1 = fastest compression, x = highest compresion (x may very between
+#         different compression formats)
+#
+# Since: 2.10
+##
+{ 'union': 'Qcow2Compress',
+  'base': { 'format': 'Qcow2CompressFormat',
+            'level': 'uint8' },
+  'discriminator': 'format',
+  'data': { 'zlib': 'Qcow2CompressZLib' } }
+
+##
 # @BlockdevOptionsQcow2:
 #
 # Driver specific block device options for qcow2.
@@ -2316,6 +2353,10 @@
 #                         caches. The interval is in seconds. The default value
 #                         is 0 and it disables this feature (since 2.5)
 #
+# @compress:              which format and compression level to use for
+#                         compressed clusters. Defaults to zlib with default
+#                         compression level (since 2.10)
+#
 # Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsQcow2',
@@ -2328,7 +2369,8 @@
             '*cache-size': 'int',
             '*l2-cache-size': 'int',
             '*refcount-cache-size': 'int',
-            '*cache-clean-interval': 'int' } }
+            '*cache-clean-interval': 'int',
+            '*compress': 'Qcow2Compress' } }
 
 
 ##
-- 
1.9.1

[Qemu-devel] [PATCH V2 3/8] block/qcow2: parse compress create options

[Peter Lieven]

this adds parsing and validation for the compress create
options. They are only validated but not yet used.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/qcow2.c             | 56 +++++++++++++++++++++++++++++++++++++++++++++--
 block/qcow2.h             |  9 ++++++++
 include/block/block_int.h |  2 ++
 3 files changed, 65 insertions(+), 2 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 2f94f03..308121a 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2144,7 +2144,8 @@ static int qcow2_create2(const char *filename, int64_t total_size,
                          const char *backing_file, const char *backing_format,
                          int flags, size_t cluster_size, PreallocMode prealloc,
                          QemuOpts *opts, int version, int refcount_order,
-                         Error **errp)
+                         const char *compress_format_name,
+                         uint8_t compress_level, Error **errp)
 {
     int cluster_bits;
     QDict *options;
@@ -2390,11 +2391,24 @@ out:
     return ret;
 }
 
+static int qcow2_compress_format_from_name(char *fmt)
+{
+    if (!fmt || !fmt[0]) {
+        return QCOW2_COMPRESS_ZLIB_COMPAT;
+    } else if (g_str_equal(fmt, "zlib")) {
+        return QCOW2_COMPRESS_ZLIB;
+    } else {
+        return -EINVAL;
+    }
+}
+
 static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
 {
     char *backing_file = NULL;
     char *backing_fmt = NULL;
     char *buf = NULL;
+    char *compress_format_name = NULL;
+    uint64_t compress_level = 0;
     uint64_t size = 0;
     int flags = 0;
     size_t cluster_size = DEFAULT_CLUSTER_SIZE;
@@ -2475,15 +2489,40 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
 
     refcount_order = ctz32(refcount_bits);
 
+    compress_format_name = qemu_opt_get_del(opts,
+                                            BLOCK_OPT_COMPRESS_FORMAT);
+    ret = qcow2_compress_format_from_name(compress_format_name);
+    if (ret < 0) {
+        error_setg(errp, "Compress format '%s' is not supported",
+                   compress_format_name);
+        goto finish;
+    }
+    compress_level = qemu_opt_get_number_del(opts, BLOCK_OPT_COMPRESS_LEVEL,
+                                             compress_level);
+    if (ret == QCOW2_COMPRESS_ZLIB_COMPAT && compress_level > 0) {
+        error_setg(errp, "Compress level can only be defined in conjunction"
+                   " with compress format");
+        ret = -EINVAL;
+        goto finish;
+    }
+    if ((ret == QCOW2_COMPRESS_ZLIB && compress_level > 9) ||
+        compress_level > 0xff) {
+        error_setg(errp, "Compress level %" PRIu64 " is not supported for"
+                   " format '%s'", compress_level, compress_format_name);
+        ret = -EINVAL;
+        goto finish;
+    }
+
     ret = qcow2_create2(filename, size, backing_file, backing_fmt, flags,
                         cluster_size, prealloc, opts, version, refcount_order,
-                        &local_err);
+                        compress_format_name, compress_level, &local_err);
     error_propagate(errp, local_err);
 
 finish:
     g_free(backing_file);
     g_free(backing_fmt);
     g_free(buf);
+    g_free(compress_format_name);
     return ret;
 }
 
@@ -3458,6 +3497,19 @@ static QemuOptsList qcow2_create_opts = {
             .help = "Width of a reference count entry in bits",
             .def_value_str = "16"
         },
+        {
+            .name = BLOCK_OPT_COMPRESS_FORMAT,
+            .type = QEMU_OPT_STRING,
+            .help = "Compress format used for compressed clusters (zlib)",
+            .def_value_str = ""
+        },
+        {
+            .name = BLOCK_OPT_COMPRESS_LEVEL,
+            .type = QEMU_OPT_NUMBER,
+            .help = "Compress level used for compressed clusters (0 = default,"
+                    " 1=fastest, x=best; x varies depending on compress.format)",
+            .def_value_str = "0"
+        },
         { /* end of list */ }
     }
 };
diff --git a/block/qcow2.h b/block/qcow2.h
index 87b15eb..d21da33 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -171,6 +171,15 @@ typedef struct Qcow2UnknownHeaderExtension {
 } Qcow2UnknownHeaderExtension;
 
 enum {
+    /* QCOW2_COMPRESS_ZLIB_COMPAT specifies to use the old standard
+     * zlib compression with a smaller window size that is compatible with
+     * old QEMU versions. This compression is used if no compression format
+     * is specified at create time */
+    QCOW2_COMPRESS_ZLIB_COMPAT = 0,
+    QCOW2_COMPRESS_ZLIB        = 1,
+};
+
+enum {
     QCOW2_FEAT_TYPE_INCOMPATIBLE    = 0,
     QCOW2_FEAT_TYPE_COMPATIBLE      = 1,
     QCOW2_FEAT_TYPE_AUTOCLEAR       = 2,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 15fa602..49811b0 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -57,6 +57,8 @@
 #define BLOCK_OPT_NOCOW             "nocow"
 #define BLOCK_OPT_OBJECT_SIZE       "object_size"
 #define BLOCK_OPT_REFCOUNT_BITS     "refcount_bits"
+#define BLOCK_OPT_COMPRESS_FORMAT   "compress.format"
+#define BLOCK_OPT_COMPRESS_LEVEL    "compress.level"
 
 #define BLOCK_PROBE_BUF_SIZE        512
 
-- 
1.9.1

[Qemu-devel] [PATCH V2 4/8] qemu-img: add documentation for compress settings

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 qemu-img.texi | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/qemu-img.texi b/qemu-img.texi
index 5b925ec..430f0b9 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -621,6 +621,27 @@ file which is COW and has data blocks already, it couldn't be changed to NOCOW
 by setting @code{nocow=on}. One can issue @code{lsattr filename} to check if
 the NOCOW flag is set or not (Capital 'C' is NOCOW flag).
 
+@item compress.format
+Defines which compression algorithm is should be used for compressed clusters.
+The following options are available if support for the respective libraries
+has been enabled at compile time:
+
+   zlib            Uses standard zlib compression
+
+The compression algorithm can only be defined at image create time and cannot
+be changed later.
+
+Note: defining a compression format will result in the compression format
+      extension being written to the Qcow2 image. Older versions of QEMU will
+      not be able to open images with this extension.
+
+@item compress.level
+Defines which compression level is used for the selected compression format.
+The default of @code{compress.level=0} will use the default compression level
+for the format. Alternate values range from 1 for fastest compression to
+x for the best compression (x max vary between compression formats). This is
+always a trade in of compression speed against compressed size.
+
 @end table
 
 @item Other
-- 
1.9.1

[Qemu-devel] [PATCH V2 5/8] block/qcow2: read and write the compress format extension

[Peter Lieven]

we now read the extension on open and write it on update, but
do not yet use it.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/qcow2.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
 block/qcow2.h |  23 +++++++++++---
 2 files changed, 104 insertions(+), 19 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 308121a..39a8afc 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -63,6 +63,7 @@ typedef struct {
 #define  QCOW2_EXT_MAGIC_END 0
 #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
 #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
+#define  QCOW2_EXT_MAGIC_COMPRESS_FORMAT 0xC03183A3
 
 static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
 {
@@ -76,6 +77,26 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
         return 0;
 }
 
+static int qcow2_compress_format_from_name(char *fmt)
+{
+    if (!fmt || !fmt[0]) {
+        return QCOW2_COMPRESS_ZLIB_COMPAT;
+    } else if (g_str_equal(fmt, "zlib")) {
+        return QCOW2_COMPRESS_ZLIB;
+    } else {
+        return -EINVAL;
+    }
+}
+
+static int qcow2_compress_level_supported(int id, uint64_t level)
+{
+    if ((id == QCOW2_COMPRESS_ZLIB_COMPAT && level > 0) ||
+        (id == QCOW2_COMPRESS_ZLIB && level > 9) ||
+        level > 0xff) {
+        return -EINVAL;
+    }
+    return 0;
+}
 
 /* 
  * read qcow2 extension and fill bs
@@ -148,6 +169,43 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
 #endif
             break;
 
+        case QCOW2_EXT_MAGIC_COMPRESS_FORMAT:
+            if (ext.len != sizeof(s->compress_format)) {
+                error_setg(errp, "ERROR: ext_compress_format: len=%"
+                           PRIu32 " invalid (!=%zu)", ext.len,
+                           sizeof(s->compress_format));
+                return 2;
+            }
+            ret = bdrv_pread(bs->file, offset, &s->compress_format,
+                             ext.len);
+            if (ret < 0) {
+                error_setg_errno(errp, -ret, "ERROR: ext_compress_fromat:"
+                                 " Could not read extension");
+                return 3;
+            }
+            s->compress_format_id =
+                qcow2_compress_format_from_name(s->compress_format.name);
+            if (s->compress_format_id < 0) {
+                error_setg(errp, "ERROR: compression algorithm '%s' is "
+                           " unsupported", s->compress_format.name);
+                return 4;
+            }
+            if (qcow2_compress_level_supported(s->compress_format_id,
+                                               s->compress_format.level) < 0) {
+                error_setg(errp, "ERROR: compress level %" PRIu8 " is not"
+                           " supported for format '%s'",
+                           s->compress_format.level, s->compress_format.name);
+                return 5;
+            }
+
+#ifdef DEBUG_EXT
+            printf("Qcow2: Got compress format %s with compress level %"
+                   PRIu8 "\n", s->compress_format.name,
+                   s->compress_format.level);
+#endif
+            break;
+
+
         case QCOW2_EXT_MAGIC_FEATURE_TABLE:
             if (p_feature_table != NULL) {
                 void* feature_table = g_malloc0(ext.len + 2 * sizeof(Qcow2Feature));
@@ -1981,6 +2039,20 @@ int qcow2_update_header(BlockDriverState *bs)
         buflen -= ret;
     }
 
+    /* Compress Format header extension */
+    if (s->compress_format.name[0]) {
+        assert(!s->compress_format.extra_data_size);
+        ret = header_ext_add(buf, QCOW2_EXT_MAGIC_COMPRESS_FORMAT,
+                             &s->compress_format, sizeof(s->compress_format),
+                             buflen);
+        if (ret < 0) {
+            goto fail;
+        }
+        buf += ret;
+        buflen -= ret;
+        header->incompatible_features |= cpu_to_be64(QCOW2_INCOMPAT_COMPRESS);
+    }
+
     /* Feature table */
     if (s->qcow_version >= 3) {
         Qcow2Feature features[] = {
@@ -1995,6 +2067,11 @@ int qcow2_update_header(BlockDriverState *bs)
                 .name = "corrupt bit",
             },
             {
+                .type = QCOW2_FEAT_TYPE_INCOMPATIBLE,
+                .bit  = QCOW2_INCOMPAT_COMPRESS_BITNR,
+                .name = "compress format bit",
+            },
+            {
                 .type = QCOW2_FEAT_TYPE_COMPATIBLE,
                 .bit  = QCOW2_COMPAT_LAZY_REFCOUNTS_BITNR,
                 .name = "lazy refcounts",
@@ -2333,6 +2410,13 @@ static int qcow2_create2(const char *filename, int64_t total_size,
         abort();
     }
 
+    if (compress_format_name[0]) {
+        BDRVQcow2State *s = blk_bs(blk)->opaque;
+        memcpy(s->compress_format.name, compress_format_name,
+               strlen(compress_format_name));
+        s->compress_format.level = compress_level;
+    }
+
     /* Create a full header (including things like feature table) */
     ret = qcow2_update_header(blk_bs(blk));
     if (ret < 0) {
@@ -2391,17 +2475,6 @@ out:
     return ret;
 }
 
-static int qcow2_compress_format_from_name(char *fmt)
-{
-    if (!fmt || !fmt[0]) {
-        return QCOW2_COMPRESS_ZLIB_COMPAT;
-    } else if (g_str_equal(fmt, "zlib")) {
-        return QCOW2_COMPRESS_ZLIB;
-    } else {
-        return -EINVAL;
-    }
-}
-
 static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
 {
     char *backing_file = NULL;
@@ -2505,11 +2578,10 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
         ret = -EINVAL;
         goto finish;
     }
-    if ((ret == QCOW2_COMPRESS_ZLIB && compress_level > 9) ||
-        compress_level > 0xff) {
+    ret = qcow2_compress_level_supported(ret, compress_level);
+    if (ret < 0) {
         error_setg(errp, "Compress level %" PRIu64 " is not supported for"
                    " format '%s'", compress_level, compress_format_name);
-        ret = -EINVAL;
         goto finish;
     }
 
diff --git a/block/qcow2.h b/block/qcow2.h
index d21da33..4ceaba1 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -187,13 +187,16 @@ enum {
 
 /* Incompatible feature bits */
 enum {
-    QCOW2_INCOMPAT_DIRTY_BITNR   = 0,
-    QCOW2_INCOMPAT_CORRUPT_BITNR = 1,
-    QCOW2_INCOMPAT_DIRTY         = 1 << QCOW2_INCOMPAT_DIRTY_BITNR,
-    QCOW2_INCOMPAT_CORRUPT       = 1 << QCOW2_INCOMPAT_CORRUPT_BITNR,
+    QCOW2_INCOMPAT_DIRTY_BITNR    = 0,
+    QCOW2_INCOMPAT_CORRUPT_BITNR  = 1,
+    QCOW2_INCOMPAT_COMPRESS_BITNR = 2,
+    QCOW2_INCOMPAT_DIRTY          = 1 << QCOW2_INCOMPAT_DIRTY_BITNR,
+    QCOW2_INCOMPAT_CORRUPT        = 1 << QCOW2_INCOMPAT_CORRUPT_BITNR,
+    QCOW2_INCOMPAT_COMPRESS       = 1 << QCOW2_INCOMPAT_COMPRESS_BITNR,
 
     QCOW2_INCOMPAT_MASK          = QCOW2_INCOMPAT_DIRTY
-                                 | QCOW2_INCOMPAT_CORRUPT,
+                                 | QCOW2_INCOMPAT_CORRUPT
+                                 | QCOW2_INCOMPAT_COMPRESS,
 };
 
 /* Compatible feature bits */
@@ -219,6 +222,13 @@ typedef struct Qcow2Feature {
     char    name[46];
 } QEMU_PACKED Qcow2Feature;
 
+typedef struct Qcow2CompressFormatExt {
+    char name[16];
+    uint8_t level;
+    char res[3];
+    uint32_t extra_data_size;
+} QEMU_PACKED Qcow2CompressFormatExt;
+
 typedef struct Qcow2DiscardRegion {
     BlockDriverState *bs;
     uint64_t offset;
@@ -303,6 +313,9 @@ typedef struct BDRVQcow2State {
      * override) */
     char *image_backing_file;
     char *image_backing_format;
+
+    Qcow2CompressFormatExt compress_format;
+    int compress_format_id;
 } BDRVQcow2State;
 
 typedef struct Qcow2COWRegion {
-- 
1.9.1

[Qemu-devel] [PATCH V2 6/8] block/qcow2: optimize qcow2_co_pwritev_compressed

[Peter Lieven]

if we specify exactly one iov of s->cluster_size bytes we can avoid
the bounce buffer.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/qcow2.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 39a8afc..0a7202a 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2750,7 +2750,7 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
     struct iovec iov;
     z_stream strm;
     int ret, out_len;
-    uint8_t *buf, *out_buf;
+    uint8_t *buf, *out_buf, *local_buf = NULL;
     uint64_t cluster_offset;
 
     if (bytes == 0) {
@@ -2760,8 +2760,8 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
         return bdrv_truncate(bs->file, cluster_offset, NULL);
     }
 
-    buf = qemu_blockalign(bs, s->cluster_size);
-    if (bytes != s->cluster_size) {
+    if (bytes != s->cluster_size || qiov->niov != 1) {
+        buf = local_buf = qemu_blockalign(bs, s->cluster_size);
         if (bytes > s->cluster_size ||
             offset + bytes != bs->total_sectors << BDRV_SECTOR_BITS)
         {
@@ -2770,8 +2770,10 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
         }
         /* Zero-pad last write if image size is not cluster aligned */
         memset(buf + bytes, 0, s->cluster_size - bytes);
+        qemu_iovec_to_buf(qiov, 0, buf, bytes);
+    } else {
+        buf = qiov->iov[0].iov_base;
     }
-    qemu_iovec_to_buf(qiov, 0, buf, bytes);
 
     out_buf = g_malloc(s->cluster_size);
 
@@ -2839,7 +2841,7 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
 success:
     ret = 0;
 fail:
-    qemu_vfree(buf);
+    qemu_vfree(local_buf);
     g_free(out_buf);
     return ret;
 }
-- 
1.9.1

[Qemu-devel] [PATCH V2 7/8] block/qcow2: start using the compress format extension

[Peter Lieven]

we now pass the parameters to the zlib compressor if the
extension is present and use the old default values if
the extension is absent.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/qcow2-cluster.c | 58 ++++++++++++++++++++++++++++++---------------------
 block/qcow2.c         | 57 +++++++++++++++++++++++++++-----------------------
 2 files changed, 65 insertions(+), 50 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 3d341fd..353ac87 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1521,30 +1521,39 @@ again:
 }
 
 static int decompress_buffer(uint8_t *out_buf, int out_buf_size,
-                             const uint8_t *buf, int buf_size)
+                             const uint8_t *buf, int buf_size,
+                             uint32_t compress_format_id)
 {
-    z_stream strm1, *strm = &strm1;
-    int ret, out_len;
-
-    memset(strm, 0, sizeof(*strm));
-
-    strm->next_in = (uint8_t *)buf;
-    strm->avail_in = buf_size;
-    strm->next_out = out_buf;
-    strm->avail_out = out_buf_size;
-
-    ret = inflateInit2(strm, -12);
-    if (ret != Z_OK)
-        return -1;
-    ret = inflate(strm, Z_FINISH);
-    out_len = strm->next_out - out_buf;
-    if ((ret != Z_STREAM_END && ret != Z_BUF_ERROR) ||
-        out_len != out_buf_size) {
-        inflateEnd(strm);
-        return -1;
-    }
-    inflateEnd(strm);
-    return 0;
+    int ret = 0, out_len;
+
+    switch (compress_format_id) {
+    case QCOW2_COMPRESS_ZLIB:
+    case QCOW2_COMPRESS_ZLIB_COMPAT: {
+        z_stream z_strm = {};
+
+        z_strm.next_in = (uint8_t *)buf;
+        z_strm.avail_in = buf_size;
+        z_strm.next_out = out_buf;
+        z_strm.avail_out = out_buf_size;
+
+        ret = inflateInit2(&z_strm, -15);
+        if (ret != Z_OK) {
+            return -1;
+        }
+        ret = inflate(&z_strm, Z_FINISH);
+        out_len = z_strm.next_out - out_buf;
+        ret = -(ret != Z_STREAM_END);
+        inflateEnd(&z_strm);
+        break;
+    }
+    default:
+        abort(); /* should never reach this point */
+    }
+
+    if (out_len != out_buf_size) {
+        ret = -1;
+    }
+    return ret;
 }
 
 int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset)
@@ -1565,7 +1574,8 @@ int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset)
             return ret;
         }
         if (decompress_buffer(s->cluster_cache, s->cluster_size,
-                              s->cluster_data + sector_offset, csize) < 0) {
+                              s->cluster_data + sector_offset, csize,
+                              s->compress_format_id) < 0) {
             return -EIO;
         }
         s->cluster_cache_offset = coffset;
diff --git a/block/qcow2.c b/block/qcow2.c
index 0a7202a..b41e58d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2748,9 +2748,10 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
     BDRVQcow2State *s = bs->opaque;
     QEMUIOVector hd_qiov;
     struct iovec iov;
-    z_stream strm;
-    int ret, out_len;
-    uint8_t *buf, *out_buf, *local_buf = NULL;
+    z_stream z_strm = {};
+    int z_windowBits = -15, z_level = Z_DEFAULT_COMPRESSION;
+    int ret, out_len = 0;
+    uint8_t *buf, *out_buf = NULL, *local_buf = NULL;
     uint64_t cluster_offset;
 
     if (bytes == 0) {
@@ -2775,34 +2776,38 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
         buf = qiov->iov[0].iov_base;
     }
 
-    out_buf = g_malloc(s->cluster_size);
+    switch (s->compress_format_id) {
+    case QCOW2_COMPRESS_ZLIB_COMPAT:
+        z_windowBits = -12;
+    case QCOW2_COMPRESS_ZLIB:
+        out_buf = g_malloc(s->cluster_size);
+        if (s->compress_format.level > 0) {
+            z_level = s->compress_format.level;
+        }
 
-    /* best compression, small window, no zlib header */
-    memset(&strm, 0, sizeof(strm));
-    ret = deflateInit2(&strm, Z_DEFAULT_COMPRESSION,
-                       Z_DEFLATED, -12,
-                       9, Z_DEFAULT_STRATEGY);
-    if (ret != 0) {
-        ret = -EINVAL;
-        goto fail;
-    }
+        ret = deflateInit2(&z_strm, z_level, Z_DEFLATED, z_windowBits, 9,
+                           Z_DEFAULT_STRATEGY);
+        if (ret != Z_OK) {
+            ret = -EINVAL;
+            goto fail;
+        }
 
-    strm.avail_in = s->cluster_size;
-    strm.next_in = (uint8_t *)buf;
-    strm.avail_out = s->cluster_size;
-    strm.next_out = out_buf;
+        z_strm.avail_in = s->cluster_size;
+        z_strm.next_in = (uint8_t *)buf;
+        z_strm.avail_out = s->cluster_size;
+        z_strm.next_out = out_buf;
 
-    ret = deflate(&strm, Z_FINISH);
-    if (ret != Z_STREAM_END && ret != Z_OK) {
-        deflateEnd(&strm);
-        ret = -EINVAL;
-        goto fail;
-    }
-    out_len = strm.next_out - out_buf;
+        ret = deflate(&z_strm, Z_FINISH);
+        out_len = z_strm.next_out - out_buf;
+        deflateEnd(&z_strm);
 
-    deflateEnd(&strm);
+        ret = ret != Z_STREAM_END;
+        break;
+    default:
+        abort(); /* should never reach this point */
+    }
 
-    if (ret != Z_STREAM_END || out_len >= s->cluster_size) {
+    if (ret || out_len >= s->cluster_size) {
         /* could not compress: write normal cluster */
         ret = qcow2_co_pwritev(bs, offset, bytes, qiov, 0);
         if (ret < 0) {
-- 
1.9.1

[Qemu-devel] [PATCH V2 8/8] block/qcow2: add lzo compress format

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/qcow2-cluster.c | 15 +++++++++++++++
 block/qcow2.c         | 26 +++++++++++++++++++++++++-
 block/qcow2.h         |  1 +
 configure             |  2 +-
 qapi/block-core.json  | 14 ++++++++++++--
 qemu-img.texi         |  1 +
 6 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 353ac87..666d090 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -24,6 +24,9 @@
 
 #include "qemu/osdep.h"
 #include <zlib.h>
+#ifdef CONFIG_LZO
+#include <lzo/lzo1x.h>
+#endif
 
 #include "qapi/error.h"
 #include "qemu-common.h"
@@ -1546,6 +1549,18 @@ static int decompress_buffer(uint8_t *out_buf, int out_buf_size,
         inflateEnd(&z_strm);
         break;
     }
+#ifdef CONFIG_LZO
+    case QCOW2_COMPRESS_LZO:
+        out_len = out_buf_size;
+        ret = lzo1x_decompress_safe(buf, buf_size, out_buf,
+                                    (lzo_uint *) &out_len, NULL);
+        if (ret == LZO_E_INPUT_NOT_CONSUMED) {
+            /* We always read up to the next sector boundary. Thus
+             * buf_size may be larger than the original compressed size. */
+            ret = 0;
+        }
+        break;
+#endif
     default:
         abort(); /* should never reach this point */
     }
diff --git a/block/qcow2.c b/block/qcow2.c
index b41e58d..ef94193 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -26,6 +26,9 @@
 #include "sysemu/block-backend.h"
 #include "qemu/module.h"
 #include <zlib.h>
+#ifdef CONFIG_LZO
+#include <lzo/lzo1x.h>
+#endif
 #include "block/qcow2.h"
 #include "qemu/error-report.h"
 #include "qapi/qmp/qerror.h"
@@ -83,6 +86,10 @@ static int qcow2_compress_format_from_name(char *fmt)
         return QCOW2_COMPRESS_ZLIB_COMPAT;
     } else if (g_str_equal(fmt, "zlib")) {
         return QCOW2_COMPRESS_ZLIB;
+#ifdef CONFIG_LZO
+    } else if (g_str_equal(fmt, "lzo")) {
+        return QCOW2_COMPRESS_LZO;
+#endif
     } else {
         return -EINVAL;
     }
@@ -92,6 +99,7 @@ static int qcow2_compress_level_supported(int id, uint64_t level)
 {
     if ((id == QCOW2_COMPRESS_ZLIB_COMPAT && level > 0) ||
         (id == QCOW2_COMPRESS_ZLIB && level > 9) ||
+        (id == QCOW2_COMPRESS_LZO && level > 0) ||
         level > 0xff) {
         return -EINVAL;
     }
@@ -197,6 +205,13 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
                            s->compress_format.level, s->compress_format.name);
                 return 5;
             }
+#ifdef CONFIG_LZO
+            if (s->compress_format_id == QCOW2_COMPRESS_LZO &&
+                lzo_init() != LZO_E_OK) {
+                error_setg(errp, "ERROR: internal error - lzo_init() failed");
+                return 6;
+            }
+#endif
 
 #ifdef DEBUG_EXT
             printf("Qcow2: Got compress format %s with compress level %"
@@ -2751,7 +2766,7 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
     z_stream z_strm = {};
     int z_windowBits = -15, z_level = Z_DEFAULT_COMPRESSION;
     int ret, out_len = 0;
-    uint8_t *buf, *out_buf = NULL, *local_buf = NULL;
+    uint8_t *buf, *out_buf = NULL, *local_buf = NULL, *work_buf = NULL;
     uint64_t cluster_offset;
 
     if (bytes == 0) {
@@ -2803,6 +2818,14 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
 
         ret = ret != Z_STREAM_END;
         break;
+#ifdef CONFIG_LZO
+    case QCOW2_COMPRESS_LZO:
+        out_buf = g_malloc(s->cluster_size + s->cluster_size / 16 + 64 + 3);
+        work_buf = g_malloc(LZO1X_1_MEM_COMPRESS);
+        ret = lzo1x_1_compress(buf, s->cluster_size, out_buf,
+                               (lzo_uint *) &out_len, work_buf);
+        break;
+#endif
     default:
         abort(); /* should never reach this point */
     }
@@ -2848,6 +2871,7 @@ success:
 fail:
     qemu_vfree(local_buf);
     g_free(out_buf);
+    g_free(work_buf);
     return ret;
 }
 
diff --git a/block/qcow2.h b/block/qcow2.h
index 4ceaba1..038688d 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -177,6 +177,7 @@ enum {
      * is specified at create time */
     QCOW2_COMPRESS_ZLIB_COMPAT = 0,
     QCOW2_COMPRESS_ZLIB        = 1,
+    QCOW2_COMPRESS_LZO         = 2,
 };
 
 enum {
diff --git a/configure b/configure
index c571ad1..81d3286 100755
--- a/configure
+++ b/configure
@@ -1890,7 +1890,7 @@ if test "$lzo" != "no" ; then
 int main(void) { lzo_version(); return 0; }
 EOF
     if compile_prog "" "-llzo2" ; then
-        libs_softmmu="$libs_softmmu -llzo2"
+        LIBS="$LIBS -llzo2"
         lzo="yes"
     else
         if test "$lzo" = "yes"; then
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 1574ffb..736073a 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2284,11 +2284,12 @@
 ##
 # @Qcow2CompressFormat:
 # @zlib: standard zlib deflate compression
+# @lzo: lzo1x compression
 #
 # Since: 2.10
 ##
 { 'enum': 'Qcow2CompressFormat',
-  'data': [ 'zlib' ] }
+  'data': [ 'zlib', 'lzo' ] }
 
 ##
 # @Qcow2CompressZLib:
@@ -2299,6 +2300,14 @@
   'data': { } }
 
 ##
+# @Qcow2CompressLZO:
+#
+# Since: 2.10
+##
+{ 'struct': 'Qcow2CompressLZO',
+  'data': { } }
+
+##
 # @Qcow2Compress:
 #
 # Specifies the compression format and compression level that should
@@ -2316,7 +2325,8 @@
   'base': { 'format': 'Qcow2CompressFormat',
             'level': 'uint8' },
   'discriminator': 'format',
-  'data': { 'zlib': 'Qcow2CompressZLib' } }
+  'data': { 'zlib': 'Qcow2CompressZLib',
+            'lzo': 'Qcow2CompressLZO' } }
 
 ##
 # @BlockdevOptionsQcow2:
diff --git a/qemu-img.texi b/qemu-img.texi
index 430f0b9..fda0e50 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -627,6 +627,7 @@ The following options are available if support for the respective libraries
 has been enabled at compile time:
 
    zlib            Uses standard zlib compression
+   lzo             Uses LZO1X compression
 
 The compression algorithm can only be defined at image create time and cannot
 be changed later.
-- 
1.9.1

Re: [Qemu-devel] [PATCH V2 0/8] add Qcow2 compress format extension

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH V2 0/8] add Qcow2 compress format extension
Message-id: 1498733831-15254-1-git-send-email-pl@kamp.de
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
fatal: Cannot update paths and switch to branch 'test' at the same time.
Did you intend to checkout 'origin/patchew/1498733831-15254-1-git-send-email-pl@kamp.de' which can not be resolved as commit?
Traceback (most recent call last):
  File "/home/fam/bin/patchew", line 440, in test_one
    git_clone_repo(clone, r["repo"], r["head"], logf)
  File "/home/fam/bin/patchew", line 53, in git_clone_repo
    cwd=clone)
  File "/usr/lib64/python3.5/subprocess.py", line 271, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['git', 'checkout', 'origin/patchew/1498733831-15254-1-git-send-email-pl@kamp.de', '-b', 'test']' returned non-zero exit status 128



---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH V2 0/8] add Qcow2 compress format extension

[Fam Zheng]

On Thu, 07/06 16:49, no-reply@patchew.org wrote:
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Subject: [Qemu-devel] [PATCH V2 0/8] add Qcow2 compress format extension
> Message-id: 1498733831-15254-1-git-send-email-pl@kamp.de
> Type: series
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> 
> BASE=base
> n=1
> total=$(git log --oneline $BASE.. | wc -l)
> failed=0
> 
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> 
> commits="$(git log --format=%H --reverse $BASE..)"
> for c in $commits; do
>     echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
>     if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
>         failed=1
>         echo
>     fi
>     n=$((n+1))
> done
> 
> exit $failed
> === TEST SCRIPT END ===
> 
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> fatal: Cannot update paths and switch to branch 'test' at the same time.
> Did you intend to checkout 'origin/patchew/1498733831-15254-1-git-send-email-pl@kamp.de' which can not be resolved as commit?
> Traceback (most recent call last):
>   File "/home/fam/bin/patchew", line 440, in test_one
>     git_clone_repo(clone, r["repo"], r["head"], logf)
>   File "/home/fam/bin/patchew", line 53, in git_clone_repo
>     cwd=clone)
>   File "/usr/lib64/python3.5/subprocess.py", line 271, in check_call
>     raise CalledProcessError(retcode, cmd)
> subprocess.CalledProcessError: Command '['git', 'checkout', 'origin/patchew/1498733831-15254-1-git-send-email-pl@kamp.de', '-b', 'test']' returned non-zero exit status 128

Ignore this please, patchew is recovering from a bad state.

Re: [Qemu-devel] [PATCH V2 0/8] add Qcow2 compress format extension

[Peter Lieven]

Am 29.06.2017 um 12:57 schrieb Peter Lieven:
> this adds a create option for Qcow2 images to specify the compression format
> and level for compressed clusters. The series adds 2 algorithms to choose from:
> zlib and lzo. zlib is the current default, but with unoptimal settings.
> If no compress.format option is specified the old zlib with the old parameters
> is used and the created images are backwards compatible with older QEMU version.
> As soon as a compression format is specified a new compress format header extension
> is written and the Qcow2 images are incompatible with older QEMU versions.
>
> Some numbers for an uncompressed Debian 9 QCOW2 image (size 1148MB):
>
> compress.format     compress time    compressed size   decompress time
> none                35.7s            339MB             3.4s
> zlib (default)      30.5s            320MB             3.2s
> zlib (level 1)      12.8s            348MB             3.2s
> lzo                  4.2s            429MB             1.6s
>
> Changes V1->V2:
>    - split the series into more patches
>    - added an qapi scheme for the compression settings
>    - renamed compression_algorithm to compress.format and added compress.level+
>    - updated the header extension to carry a variable extra payload and compress
>      level.
>    - removed extra reservations for header extensions
>    - added missing lzo_init and fixed compress overhead for lzo
>
> Peter Lieven (8):
>    docs: add compress format extension to qcow2 spec
>    qapi: add compress parameters to Qcow2 Blockdev options
>    block/qcow2: parse compress create options
>    qemu-img: add documentation for compress settings
>    block/qcow2: read and write the compress format extension
>    block/qcow2: optimize qcow2_co_pwritev_compressed
>    block/qcow2: start using the compress format extension
>    block/qcow2: add lzo compress format
>
>   block/qcow2-cluster.c     |  73 +++++++++++-----
>   block/qcow2.c             | 219 +++++++++++++++++++++++++++++++++++++++-------
>   block/qcow2.h             |  33 +++++--
>   configure                 |   2 +-
>   docs/interop/qcow2.txt    |  43 ++++++++-
>   include/block/block_int.h |   2 +
>   qapi/block-core.json      |  54 +++++++++++-
>   qemu-img.texi             |  22 +++++
>   8 files changed, 384 insertions(+), 64 deletions(-)
>

Hi,


had anyone had a chance to look at the updated specs?


Thanks,

Peter

Re: [Qemu-devel] [PATCH V2 1/8] docs: add compress format extension to qcow2 spec

[Kevin Wolf]

Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  docs/interop/qcow2.txt | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
> index 80cdfd0..c01daf3 100644
> --- a/docs/interop/qcow2.txt
> +++ b/docs/interop/qcow2.txt
> @@ -85,7 +85,12 @@ in the description of a field.
>                                  be written to (unless for regaining
>                                  consistency).
>  
> -                    Bits 2-63:  Reserved (set to 0)
> +                    Bit 2:      Compress format bit.  If and only if this bit
> +                                is set then the compress format extension
> +                                MUST be present and MUST be parsed and checked
> +                                for compatibility.
> +
> +                    Bits 3-63:  Reserved (set to 0)
>  
>           80 -  87:  compatible_features
>                      Bitmask of compatible features. An implementation can
> @@ -135,6 +140,7 @@ be stored. Each extension has a structure like the following:
>                          0xE2792ACA - Backing file format name
>                          0x6803f857 - Feature name table
>                          0x23852875 - Bitmaps extension
> +                        0xC03183A3 - Compress format extension
>                          other      - Unknown header extension, can be safely
>                                       ignored
>  
> @@ -208,6 +214,41 @@ The fields of the bitmaps extension are:
>                     starts. Must be aligned to a cluster boundary.
>  
>  
> +== Compress format extension ==
> +
> +The compress format extension is an optional header extension. It provides
> +the ability to specify the compress algorithm and compress parameters
> +that are used for compressed clusters. This new header MUST be present if
> +the incompatible-feature bit "compress format bit" is set and MUST be absent
> +otherwise.

Okay. Only one header extension type for all compression formats. I
think this does work because we have a variable header extension size.

> +The fields of the compress format extension are:
> +
> +    Byte  0 - 15:  compress_format_name (padded with zeros, but not
> +                   necessarily null terminated if it has full length)

We absolutely need to define the valid names and their meaning here.

> +              16:  compress_level (uint8_t)
> +                   0 = default compress level
> +                   1 = lowest compress level
> +                   x = highest compress level (the highest compress
> +                       level may vary for different compress formats)

Let's be explicit about the compression formats that this field is valid
for (i.e. make this already part of the format specific fields).

Then we can also be specific instead of writing "may vary", which is not
a very good thing to have in a specification.

> +         17 - 19:  Reserved for future use, must be zero.

We don't need this for now because byte 16 will be the final one in this
struct.

> +         20 - 23:  extra_data_size
> +                   Size of compress format specific extra data.
> +                   For now, as no format specifies extra data,
> +                   extra_data_size is reserved and should be zero.
> +
> +        variable:  extra_data
> +                   Extra data with additional parameters for the compress
> +                   format, occupying extra_data_size bytes.

extra_data_size isn't necessary because we already have the length of
the header extension, so we know how long the whole thing is.
extra_data isn't necessary because we'll just add new fields at the end
if we want to add something.

> +        variable:  Padding to round up the size of compress format extension
> +                   to the next multiple of 8. All bytes of the padding must be
> +                   zero.

This is already contained in the description of header extensions in
general, so we don't have to repeat it here.

Kevin

Re: [Qemu-devel] [PATCH V2 2/8] qapi: add compress parameters to Qcow2 Blockdev options

[Kevin Wolf]

Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  qapi/block-core.json | 44 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 43 insertions(+), 1 deletion(-)

How does this make sense as a runtime option? What would happen if the
image contains one compression format and I specify another one on the
command line or in blockdev-add?

Shouldn't it just be a create-time option and when you run qemu, it uses
whatever format that image has?

Kevin

Re: [Qemu-devel] [PATCH V2 4/8] qemu-img: add documentation for compress settings

[Kevin Wolf]

Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  qemu-img.texi | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/qemu-img.texi b/qemu-img.texi
> index 5b925ec..430f0b9 100644
> --- a/qemu-img.texi
> +++ b/qemu-img.texi
> @@ -621,6 +621,27 @@ file which is COW and has data blocks already, it couldn't be changed to NOCOW
>  by setting @code{nocow=on}. One can issue @code{lsattr filename} to check if
>  the NOCOW flag is set or not (Capital 'C' is NOCOW flag).
>  
> +@item compress.format
> +Defines which compression algorithm is should be used for compressed clusters.
> +The following options are available if support for the respective libraries
> +has been enabled at compile time:
> +
> +   zlib            Uses standard zlib compression
> +
> +The compression algorithm can only be defined at image create time and cannot
> +be changed later.
> +
> +Note: defining a compression format will result in the compression format
> +      extension being written to the Qcow2 image. Older versions of QEMU will
> +      not be able to open images with this extension.

Why not more specifically "QEMU versions before 2.10"?

> +@item compress.level
> +Defines which compression level is used for the selected compression format.
> +The default of @code{compress.level=0} will use the default compression level
> +for the format. Alternate values range from 1 for fastest compression to
> +x for the best compression (x max vary between compression formats). This is
> +always a trade in of compression speed against compressed size.

Start with something like "valid for compress.format=zlib" and then give
the exact range of valid values for zlib.

Kevin

Re: [Qemu-devel] [PATCH V2 2/8] qapi: add compress parameters to Qcow2 Blockdev options

[Peter Lieven]

Am 10.07.2017 um 15:10 schrieb Kevin Wolf:
> Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   qapi/block-core.json | 44 +++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 43 insertions(+), 1 deletion(-)
> How does this make sense as a runtime option? What would happen if the
> image contains one compression format and I specify another one on the
> command line or in blockdev-add?
>
> Shouldn't it just be a create-time option and when you run qemu, it uses
> whatever format that image has?

I was asked to add it here. It is indeed only a create option and has no other effect.

Peter

Re: [Qemu-devel] [PATCH V2 5/8] block/qcow2: read and write the compress format extension

[Kevin Wolf]

Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
> we now read the extension on open and write it on update, but
> do not yet use it.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/qcow2.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
>  block/qcow2.h |  23 +++++++++++---
>  2 files changed, 104 insertions(+), 19 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 308121a..39a8afc 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -63,6 +63,7 @@ typedef struct {
>  #define  QCOW2_EXT_MAGIC_END 0
>  #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
>  #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
> +#define  QCOW2_EXT_MAGIC_COMPRESS_FORMAT 0xC03183A3
>  
>  static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
>  {
> @@ -76,6 +77,26 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
>          return 0;
>  }
>  
> +static int qcow2_compress_format_from_name(char *fmt)
> +{
> +    if (!fmt || !fmt[0]) {
> +        return QCOW2_COMPRESS_ZLIB_COMPAT;
> +    } else if (g_str_equal(fmt, "zlib")) {
> +        return QCOW2_COMPRESS_ZLIB;
> +    } else {
> +        return -EINVAL;
> +    }
> +}

It might make sense to allow specifying the old compression format in
the compression header. But I'm not so sure about using the empty string
rather than a proper name for it, and even less about not documenting
it.

Kevin

Re: [Qemu-devel] [PATCH V2 1/8] docs: add compress format extension to qcow2 spec

[Peter Lieven]

Am 10.07.2017 um 14:58 schrieb Kevin Wolf:
> Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   docs/interop/qcow2.txt | 43 ++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 42 insertions(+), 1 deletion(-)
>>
>> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
>> index 80cdfd0..c01daf3 100644
>> --- a/docs/interop/qcow2.txt
>> +++ b/docs/interop/qcow2.txt
>> @@ -85,7 +85,12 @@ in the description of a field.
>>                                   be written to (unless for regaining
>>                                   consistency).
>>   
>> -                    Bits 2-63:  Reserved (set to 0)
>> +                    Bit 2:      Compress format bit.  If and only if this bit
>> +                                is set then the compress format extension
>> +                                MUST be present and MUST be parsed and checked
>> +                                for compatibility.
>> +
>> +                    Bits 3-63:  Reserved (set to 0)
>>   
>>            80 -  87:  compatible_features
>>                       Bitmask of compatible features. An implementation can
>> @@ -135,6 +140,7 @@ be stored. Each extension has a structure like the following:
>>                           0xE2792ACA - Backing file format name
>>                           0x6803f857 - Feature name table
>>                           0x23852875 - Bitmaps extension
>> +                        0xC03183A3 - Compress format extension
>>                           other      - Unknown header extension, can be safely
>>                                        ignored
>>   
>> @@ -208,6 +214,41 @@ The fields of the bitmaps extension are:
>>                      starts. Must be aligned to a cluster boundary.
>>   
>>   
>> +== Compress format extension ==
>> +
>> +The compress format extension is an optional header extension. It provides
>> +the ability to specify the compress algorithm and compress parameters
>> +that are used for compressed clusters. This new header MUST be present if
>> +the incompatible-feature bit "compress format bit" is set and MUST be absent
>> +otherwise.
> Okay. Only one header extension type for all compression formats. I
> think this does work because we have a variable header extension size.
>
>> +The fields of the compress format extension are:
>> +
>> +    Byte  0 - 15:  compress_format_name (padded with zeros, but not
>> +                   necessarily null terminated if it has full length)
> We absolutely need to define the valid names and their meaning here.
>
>> +              16:  compress_level (uint8_t)
>> +                   0 = default compress level
>> +                   1 = lowest compress level
>> +                   x = highest compress level (the highest compress
>> +                       level may vary for different compress formats)
> Let's be explicit about the compression formats that this field is valid
> for (i.e. make this already part of the format specific fields).
>
> Then we can also be specific instead of writing "may vary", which is not
> a very good thing to have in a specification.

The idea to keep it in the common header was that most formats will
only need a compression level and nothing more. So it is not necessary
to add specific headers for a setting that is quite common among
compression formats.


>
>> +         17 - 19:  Reserved for future use, must be zero.
> We don't need this for now because byte 16 will be the final one in this
> struct.
>
>> +         20 - 23:  extra_data_size
>> +                   Size of compress format specific extra data.
>> +                   For now, as no format specifies extra data,
>> +                   extra_data_size is reserved and should be zero.
>> +
>> +        variable:  extra_data
>> +                   Extra data with additional parameters for the compress
>> +                   format, occupying extra_data_size bytes.
> extra_data_size isn't necessary because we already have the length of
> the header extension, so we know how long the whole thing is.
> extra_data isn't necessary because we'll just add new fields at the end
> if we want to add something.

I was asked to add it altough it is redundant.


>
>> +        variable:  Padding to round up the size of compress format extension
>> +                   to the next multiple of 8. All bytes of the padding must be
>> +                   zero.
> This is already contained in the description of header extensions in
> general, so we don't have to repeat it here.

Its also in the bitmap extension...

Peter

Re: [Qemu-devel] [PATCH V2 2/8] qapi: add compress parameters to Qcow2 Blockdev options

[Daniel P. Berrange]

On Mon, Jul 10, 2017 at 03:24:02PM +0200, Peter Lieven wrote:
> Am 10.07.2017 um 15:10 schrieb Kevin Wolf:
> > Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
> > > Signed-off-by: Peter Lieven <pl@kamp.de>
> > > ---
> > >   qapi/block-core.json | 44 +++++++++++++++++++++++++++++++++++++++++++-
> > >   1 file changed, 43 insertions(+), 1 deletion(-)
> > How does this make sense as a runtime option? What would happen if the
> > image contains one compression format and I specify another one on the
> > command line or in blockdev-add?
> > 
> > Shouldn't it just be a create-time option and when you run qemu, it uses
> > whatever format that image has?
> 
> I was asked to add it here. It is indeed only a create option and has no
> other effect.

I think there was some mis-understanding. I suggested adding QAPI schema
to define the options needed during create, and then use QAPI functions
when processing the create. In patch 4 you've just used the traditional
qemu opts functions to query the parameters, so there's no validation
that what's being processed actually matches the QAPI schema. The QAPI
schema additions for create would need to be defined indepenantly of
the runtime options too.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH V2 5/8] block/qcow2: read and write the compress format extension

[Peter Lieven]

Am 10.07.2017 um 15:25 schrieb Kevin Wolf:
> Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
>> we now read the extension on open and write it on update, but
>> do not yet use it.
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   block/qcow2.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
>>   block/qcow2.h |  23 +++++++++++---
>>   2 files changed, 104 insertions(+), 19 deletions(-)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 308121a..39a8afc 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -63,6 +63,7 @@ typedef struct {
>>   #define  QCOW2_EXT_MAGIC_END 0
>>   #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
>>   #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
>> +#define  QCOW2_EXT_MAGIC_COMPRESS_FORMAT 0xC03183A3
>>   
>>   static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
>>   {
>> @@ -76,6 +77,26 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
>>           return 0;
>>   }
>>   
>> +static int qcow2_compress_format_from_name(char *fmt)
>> +{
>> +    if (!fmt || !fmt[0]) {
>> +        return QCOW2_COMPRESS_ZLIB_COMPAT;
>> +    } else if (g_str_equal(fmt, "zlib")) {
>> +        return QCOW2_COMPRESS_ZLIB;
>> +    } else {
>> +        return -EINVAL;
>> +    }
>> +}
> It might make sense to allow specifying the old compression format in
> the compression header. But I'm not so sure about using the empty string
> rather than a proper name for it, and even less about not documenting
> it.

The old format is used if and only if the compression header is absent.
It makes no sense to write a header and use the old format. The old
settings are suboptimal and old versions can't open a qcow2 with a
compression header anyway.

Peter

Re: [Qemu-devel] [PATCH V2 2/8] qapi: add compress parameters to Qcow2 Blockdev options

[Kevin Wolf]

Am 10.07.2017 um 15:24 hat Peter Lieven geschrieben:
> Am 10.07.2017 um 15:10 schrieb Kevin Wolf:
> >Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
> >>Signed-off-by: Peter Lieven <pl@kamp.de>
> >>---
> >>  qapi/block-core.json | 44 +++++++++++++++++++++++++++++++++++++++++++-
> >>  1 file changed, 43 insertions(+), 1 deletion(-)
> >How does this make sense as a runtime option? What would happen if the
> >image contains one compression format and I specify another one on the
> >command line or in blockdev-add?
> >
> >Shouldn't it just be a create-time option and when you run qemu, it uses
> >whatever format that image has?
> 
> I was asked to add it here. It is indeed only a create option and has
> no other effect.

But if it's only a create option, why is it added to blockdev-add?

Kevin

Re: [Qemu-devel] [PATCH V2 2/8] qapi: add compress parameters to Qcow2 Blockdev options

[Peter Lieven]

Am 10.07.2017 um 15:30 schrieb Kevin Wolf:
> Am 10.07.2017 um 15:24 hat Peter Lieven geschrieben:
>> Am 10.07.2017 um 15:10 schrieb Kevin Wolf:
>>> Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>> ---
>>>>   qapi/block-core.json | 44 +++++++++++++++++++++++++++++++++++++++++++-
>>>>   1 file changed, 43 insertions(+), 1 deletion(-)
>>> How does this make sense as a runtime option? What would happen if the
>>> image contains one compression format and I specify another one on the
>>> command line or in blockdev-add?
>>>
>>> Shouldn't it just be a create-time option and when you run qemu, it uses
>>> whatever format that image has?
>> I was asked to add it here. It is indeed only a create option and has
>> no other effect.
> But if it's only a create option, why is it added to blockdev-add?

As Daniel wrote its maybe a misunderstanding. He pointed me to the crypto extension
that was recently added and the structs there. As I said I might need help with the
QAPI stuff...

Peter

Re: [Qemu-devel] [PATCH V2 5/8] block/qcow2: read and write the compress format extension

[Kevin Wolf]

Am 10.07.2017 um 15:29 hat Peter Lieven geschrieben:
> Am 10.07.2017 um 15:25 schrieb Kevin Wolf:
> >Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
> >>we now read the extension on open and write it on update, but
> >>do not yet use it.
> >>
> >>Signed-off-by: Peter Lieven <pl@kamp.de>
> >>---
> >>  block/qcow2.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
> >>  block/qcow2.h |  23 +++++++++++---
> >>  2 files changed, 104 insertions(+), 19 deletions(-)
> >>
> >>diff --git a/block/qcow2.c b/block/qcow2.c
> >>index 308121a..39a8afc 100644
> >>--- a/block/qcow2.c
> >>+++ b/block/qcow2.c
> >>@@ -63,6 +63,7 @@ typedef struct {
> >>  #define  QCOW2_EXT_MAGIC_END 0
> >>  #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
> >>  #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
> >>+#define  QCOW2_EXT_MAGIC_COMPRESS_FORMAT 0xC03183A3
> >>  static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
> >>  {
> >>@@ -76,6 +77,26 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
> >>          return 0;
> >>  }
> >>+static int qcow2_compress_format_from_name(char *fmt)
> >>+{
> >>+    if (!fmt || !fmt[0]) {
> >>+        return QCOW2_COMPRESS_ZLIB_COMPAT;
> >>+    } else if (g_str_equal(fmt, "zlib")) {
> >>+        return QCOW2_COMPRESS_ZLIB;
> >>+    } else {
> >>+        return -EINVAL;
> >>+    }
> >>+}
> >It might make sense to allow specifying the old compression format in
> >the compression header. But I'm not so sure about using the empty string
> >rather than a proper name for it, and even less about not documenting
> >it.
> 
> The old format is used if and only if the compression header is absent.
> It makes no sense to write a header and use the old format. The old
> settings are suboptimal and old versions can't open a qcow2 with a
> compression header anyway.

Your code allows an empty string in the header extension. If you don't
want it there, you need to reject it.

Kevin

Re: [Qemu-devel] [PATCH V2 5/8] block/qcow2: read and write the compress format extension

[Daniel P. Berrange]

On Mon, Jul 10, 2017 at 03:34:59PM +0200, Kevin Wolf wrote:
> Am 10.07.2017 um 15:29 hat Peter Lieven geschrieben:
> > Am 10.07.2017 um 15:25 schrieb Kevin Wolf:
> > >Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
> > >>we now read the extension on open and write it on update, but
> > >>do not yet use it.
> > >>
> > >>Signed-off-by: Peter Lieven <pl@kamp.de>
> > >>---
> > >>  block/qcow2.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
> > >>  block/qcow2.h |  23 +++++++++++---
> > >>  2 files changed, 104 insertions(+), 19 deletions(-)
> > >>
> > >>diff --git a/block/qcow2.c b/block/qcow2.c
> > >>index 308121a..39a8afc 100644
> > >>--- a/block/qcow2.c
> > >>+++ b/block/qcow2.c
> > >>@@ -63,6 +63,7 @@ typedef struct {
> > >>  #define  QCOW2_EXT_MAGIC_END 0
> > >>  #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
> > >>  #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
> > >>+#define  QCOW2_EXT_MAGIC_COMPRESS_FORMAT 0xC03183A3
> > >>  static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
> > >>  {
> > >>@@ -76,6 +77,26 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
> > >>          return 0;
> > >>  }
> > >>+static int qcow2_compress_format_from_name(char *fmt)
> > >>+{
> > >>+    if (!fmt || !fmt[0]) {
> > >>+        return QCOW2_COMPRESS_ZLIB_COMPAT;
> > >>+    } else if (g_str_equal(fmt, "zlib")) {
> > >>+        return QCOW2_COMPRESS_ZLIB;
> > >>+    } else {
> > >>+        return -EINVAL;
> > >>+    }
> > >>+}
> > >It might make sense to allow specifying the old compression format in
> > >the compression header. But I'm not so sure about using the empty string
> > >rather than a proper name for it, and even less about not documenting
> > >it.
> > 
> > The old format is used if and only if the compression header is absent.
> > It makes no sense to write a header and use the old format. The old
> > settings are suboptimal and old versions can't open a qcow2 with a
> > compression header anyway.
> 
> Your code allows an empty string in the header extension. If you don't
> want it there, you need to reject it.

This is a good reason to use the QAPI schema to parse the create options
instead of doing it using qemu_opts - the QAPI schema will generate correct
code to parse & validate enum values 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH V2 5/8] block/qcow2: read and write the compress format extension

[Peter Lieven]

Am 10.07.2017 um 15:44 schrieb Daniel P. Berrange:
> On Mon, Jul 10, 2017 at 03:34:59PM +0200, Kevin Wolf wrote:
>> Am 10.07.2017 um 15:29 hat Peter Lieven geschrieben:
>>> Am 10.07.2017 um 15:25 schrieb Kevin Wolf:
>>>> Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
>>>>> we now read the extension on open and write it on update, but
>>>>> do not yet use it.
>>>>>
>>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>>> ---
>>>>>   block/qcow2.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
>>>>>   block/qcow2.h |  23 +++++++++++---
>>>>>   2 files changed, 104 insertions(+), 19 deletions(-)
>>>>>
>>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>>> index 308121a..39a8afc 100644
>>>>> --- a/block/qcow2.c
>>>>> +++ b/block/qcow2.c
>>>>> @@ -63,6 +63,7 @@ typedef struct {
>>>>>   #define  QCOW2_EXT_MAGIC_END 0
>>>>>   #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
>>>>>   #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
>>>>> +#define  QCOW2_EXT_MAGIC_COMPRESS_FORMAT 0xC03183A3
>>>>>   static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
>>>>>   {
>>>>> @@ -76,6 +77,26 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
>>>>>           return 0;
>>>>>   }
>>>>> +static int qcow2_compress_format_from_name(char *fmt)
>>>>> +{
>>>>> +    if (!fmt || !fmt[0]) {
>>>>> +        return QCOW2_COMPRESS_ZLIB_COMPAT;
>>>>> +    } else if (g_str_equal(fmt, "zlib")) {
>>>>> +        return QCOW2_COMPRESS_ZLIB;
>>>>> +    } else {
>>>>> +        return -EINVAL;
>>>>> +    }
>>>>> +}
>>>> It might make sense to allow specifying the old compression format in
>>>> the compression header. But I'm not so sure about using the empty string
>>>> rather than a proper name for it, and even less about not documenting
>>>> it.
>>> The old format is used if and only if the compression header is absent.
>>> It makes no sense to write a header and use the old format. The old
>>> settings are suboptimal and old versions can't open a qcow2 with a
>>> compression header anyway.
>> Your code allows an empty string in the header extension. If you don't
>> want it there, you need to reject it.
> This is a good reason to use the QAPI schema to parse the create options
> instead of doing it using qemu_opts - the QAPI schema will generate correct
> code to parse & validate enum values

Can you then please advise where to add the schematas and which functions
to use for parsing it? You wrote that it is a bit hackish at the moment as not
everything is available in qcow2_create path.

Thank you,
Peter

Re: [Qemu-devel] [PATCH V2 1/8] docs: add compress format extension to qcow2 spec

[Kevin Wolf]

Am 10.07.2017 um 15:27 hat Peter Lieven geschrieben:
> Am 10.07.2017 um 14:58 schrieb Kevin Wolf:
> >Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
> >>Signed-off-by: Peter Lieven <pl@kamp.de>
> >>---
> >>  docs/interop/qcow2.txt | 43 ++++++++++++++++++++++++++++++++++++++++++-
> >>  1 file changed, 42 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
> >>index 80cdfd0..c01daf3 100644
> >>--- a/docs/interop/qcow2.txt
> >>+++ b/docs/interop/qcow2.txt
> >>@@ -85,7 +85,12 @@ in the description of a field.
> >>                                  be written to (unless for regaining
> >>                                  consistency).
> >>-                    Bits 2-63:  Reserved (set to 0)
> >>+                    Bit 2:      Compress format bit.  If and only if this bit
> >>+                                is set then the compress format extension
> >>+                                MUST be present and MUST be parsed and checked
> >>+                                for compatibility.
> >>+
> >>+                    Bits 3-63:  Reserved (set to 0)
> >>           80 -  87:  compatible_features
> >>                      Bitmask of compatible features. An implementation can
> >>@@ -135,6 +140,7 @@ be stored. Each extension has a structure like the following:
> >>                          0xE2792ACA - Backing file format name
> >>                          0x6803f857 - Feature name table
> >>                          0x23852875 - Bitmaps extension
> >>+                        0xC03183A3 - Compress format extension
> >>                          other      - Unknown header extension, can be safely
> >>                                       ignored
> >>@@ -208,6 +214,41 @@ The fields of the bitmaps extension are:
> >>                     starts. Must be aligned to a cluster boundary.
> >>+== Compress format extension ==
> >>+
> >>+The compress format extension is an optional header extension. It provides
> >>+the ability to specify the compress algorithm and compress parameters
> >>+that are used for compressed clusters. This new header MUST be present if
> >>+the incompatible-feature bit "compress format bit" is set and MUST be absent
> >>+otherwise.
> >Okay. Only one header extension type for all compression formats. I
> >think this does work because we have a variable header extension size.
> >
> >>+The fields of the compress format extension are:
> >>+
> >>+    Byte  0 - 15:  compress_format_name (padded with zeros, but not
> >>+                   necessarily null terminated if it has full length)
> >We absolutely need to define the valid names and their meaning here.
> >
> >>+              16:  compress_level (uint8_t)
> >>+                   0 = default compress level
> >>+                   1 = lowest compress level
> >>+                   x = highest compress level (the highest compress
> >>+                       level may vary for different compress formats)
> >Let's be explicit about the compression formats that this field is valid
> >for (i.e. make this already part of the format specific fields).
> >
> >Then we can also be specific instead of writing "may vary", which is not
> >a very good thing to have in a specification.
> 
> The idea to keep it in the common header was that most formats will
> only need a compression level and nothing more. So it is not necessary
> to add specific headers for a setting that is quite common among
> compression formats.

I'm only talking about a change in the specification text, which would
give us a clean separation design-wise and a more specific explanation.

We have regretted enough things that were defined a bit sloppily just
for convenience that I'd rather not make this mistake again when we can
avoid it.

> >>+         17 - 19:  Reserved for future use, must be zero.
> >We don't need this for now because byte 16 will be the final one in this
> >struct.
> >
> >>+         20 - 23:  extra_data_size
> >>+                   Size of compress format specific extra data.
> >>+                   For now, as no format specifies extra data,
> >>+                   extra_data_size is reserved and should be zero.
> >>+
> >>+        variable:  extra_data
> >>+                   Extra data with additional parameters for the compress
> >>+                   format, occupying extra_data_size bytes.
> >extra_data_size isn't necessary because we already have the length of
> >the header extension, so we know how long the whole thing is.
> >extra_data isn't necessary because we'll just add new fields at the end
> >if we want to add something.
> 
> I was asked to add it altough it is redundant.

Who asked this, and did they have a good reason for it?

> >
> >>+        variable:  Padding to round up the size of compress format extension
> >>+                   to the next multiple of 8. All bytes of the padding must be
> >>+                   zero.
> >This is already contained in the description of header extensions in
> >general, so we don't have to repeat it here.
> 
> Its also in the bitmap extension...

The bitmap header extension is aligned, so it doesn't have padding.
Some other bitmap structures need padding, but they are not header
extensions, so the information isn't redundant for them

Kevin

Re: [Qemu-devel] [PATCH V2 5/8] block/qcow2: read and write the compress format extension

[Kevin Wolf]

Am 10.07.2017 um 15:44 hat Daniel P. Berrange geschrieben:
> On Mon, Jul 10, 2017 at 03:34:59PM +0200, Kevin Wolf wrote:
> > Am 10.07.2017 um 15:29 hat Peter Lieven geschrieben:
> > > Am 10.07.2017 um 15:25 schrieb Kevin Wolf:
> > > >Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
> > > >>we now read the extension on open and write it on update, but
> > > >>do not yet use it.
> > > >>
> > > >>Signed-off-by: Peter Lieven <pl@kamp.de>
> > > >>---
> > > >>  block/qcow2.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
> > > >>  block/qcow2.h |  23 +++++++++++---
> > > >>  2 files changed, 104 insertions(+), 19 deletions(-)
> > > >>
> > > >>diff --git a/block/qcow2.c b/block/qcow2.c
> > > >>index 308121a..39a8afc 100644
> > > >>--- a/block/qcow2.c
> > > >>+++ b/block/qcow2.c
> > > >>@@ -63,6 +63,7 @@ typedef struct {
> > > >>  #define  QCOW2_EXT_MAGIC_END 0
> > > >>  #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
> > > >>  #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
> > > >>+#define  QCOW2_EXT_MAGIC_COMPRESS_FORMAT 0xC03183A3
> > > >>  static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
> > > >>  {
> > > >>@@ -76,6 +77,26 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
> > > >>          return 0;
> > > >>  }
> > > >>+static int qcow2_compress_format_from_name(char *fmt)
> > > >>+{
> > > >>+    if (!fmt || !fmt[0]) {
> > > >>+        return QCOW2_COMPRESS_ZLIB_COMPAT;
> > > >>+    } else if (g_str_equal(fmt, "zlib")) {
> > > >>+        return QCOW2_COMPRESS_ZLIB;
> > > >>+    } else {
> > > >>+        return -EINVAL;
> > > >>+    }
> > > >>+}
> > > >It might make sense to allow specifying the old compression format in
> > > >the compression header. But I'm not so sure about using the empty string
> > > >rather than a proper name for it, and even less about not documenting
> > > >it.
> > > 
> > > The old format is used if and only if the compression header is absent.
> > > It makes no sense to write a header and use the old format. The old
> > > settings are suboptimal and old versions can't open a qcow2 with a
> > > compression header anyway.
> > 
> > Your code allows an empty string in the header extension. If you don't
> > want it there, you need to reject it.
> 
> This is a good reason to use the QAPI schema to parse the create options
> instead of doing it using qemu_opts - the QAPI schema will generate correct
> code to parse & validate enum values 

I'd really like to see QAPI used for bdrv_create(), but here we're in
the bdrv_open() path and parsing qcow2 headers. So either way this one
specifically wouldn't be fixed.

Kevin

Re: [Qemu-devel] [PATCH V2 3/8] block/qcow2: parse compress create options

[Daniel P. Berrange]

On Thu, Jun 29, 2017 at 12:57:06PM +0200, Peter Lieven wrote:
> this adds parsing and validation for the compress create
> options. They are only validated but not yet used.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/qcow2.c             | 56 +++++++++++++++++++++++++++++++++++++++++++++--
>  block/qcow2.h             |  9 ++++++++
>  include/block/block_int.h |  2 ++
>  3 files changed, 65 insertions(+), 2 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 2f94f03..308121a 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2144,7 +2144,8 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>                           const char *backing_file, const char *backing_format,
>                           int flags, size_t cluster_size, PreallocMode prealloc,
>                           QemuOpts *opts, int version, int refcount_order,
> -                         Error **errp)
> +                         const char *compress_format_name,
> +                         uint8_t compress_level, Error **errp)
>  {
>      int cluster_bits;
>      QDict *options;
> @@ -2390,11 +2391,24 @@ out:
>      return ret;
>  }
>  
> +static int qcow2_compress_format_from_name(char *fmt)
> +{
> +    if (!fmt || !fmt[0]) {
> +        return QCOW2_COMPRESS_ZLIB_COMPAT;
> +    } else if (g_str_equal(fmt, "zlib")) {
> +        return QCOW2_COMPRESS_ZLIB;
> +    } else {
> +        return -EINVAL;
> +    }
> +}
> +
>  static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
>  {
>      char *backing_file = NULL;
>      char *backing_fmt = NULL;
>      char *buf = NULL;
> +    char *compress_format_name = NULL;
> +    uint64_t compress_level = 0;
>      uint64_t size = 0;
>      int flags = 0;
>      size_t cluster_size = DEFAULT_CLUSTER_SIZE;
> @@ -2475,15 +2489,40 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
>  
>      refcount_order = ctz32(refcount_bits);
>  
> +    compress_format_name = qemu_opt_get_del(opts,
> +                                            BLOCK_OPT_COMPRESS_FORMAT);
> +    ret = qcow2_compress_format_from_name(compress_format_name);
> +    if (ret < 0) {
> +        error_setg(errp, "Compress format '%s' is not supported",
> +                   compress_format_name);
> +        goto finish;
> +    }
> +    compress_level = qemu_opt_get_number_del(opts, BLOCK_OPT_COMPRESS_LEVEL,
> +                                             compress_level);
> +    if (ret == QCOW2_COMPRESS_ZLIB_COMPAT && compress_level > 0) {
> +        error_setg(errp, "Compress level can only be defined in conjunction"
> +                   " with compress format");
> +        ret = -EINVAL;
> +        goto finish;
> +    }
> +    if ((ret == QCOW2_COMPRESS_ZLIB && compress_level > 9) ||
> +        compress_level > 0xff) {
> +        error_setg(errp, "Compress level %" PRIu64 " is not supported for"
> +                   " format '%s'", compress_level, compress_format_name);
> +        ret = -EINVAL;
> +        goto finish;
> +    }

This is where the QAPI schema bits would fit in. eg instead of calling the
various qemu_opts_get* functions, do something like this:

    QDict *options, *compressopts = NULL;
    Qcow2Compress *compress = NULL;

    options = qemu_opts_to_qdict(opts, NULL);
    qdict_extract_subqdict(options, &compressopts, "compress.");

    v = qobject_input_visitor_new_keyval(QOBJECT(compressopts));
    
    visit_type_Qcow2Compress(v, &compress, &local_err);
    if (local_err) {
        ret= -EINVAL;
	goto finish;
    }


    ..do whatever you need with the 'compress' object now...

    visit_free(v);
    QDECREF(encryptopts);
    QDECREF(options);


Eventually the entire of the qcow2 create codepath will be qemu opts based,
so all the subdict/visitor steps will be able to go away, leaving you to
just use the Qcow2Compress struct immediately.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH V2 5/8] block/qcow2: read and write the compress format extension

[Daniel P. Berrange]

On Mon, Jul 10, 2017 at 03:52:18PM +0200, Kevin Wolf wrote:
> Am 10.07.2017 um 15:44 hat Daniel P. Berrange geschrieben:
> > On Mon, Jul 10, 2017 at 03:34:59PM +0200, Kevin Wolf wrote:
> > > Am 10.07.2017 um 15:29 hat Peter Lieven geschrieben:
> > > > Am 10.07.2017 um 15:25 schrieb Kevin Wolf:
> > > > >Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
> > > > >>we now read the extension on open and write it on update, but
> > > > >>do not yet use it.
> > > > >>
> > > > >>Signed-off-by: Peter Lieven <pl@kamp.de>
> > > > >>---
> > > > >>  block/qcow2.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
> > > > >>  block/qcow2.h |  23 +++++++++++---
> > > > >>  2 files changed, 104 insertions(+), 19 deletions(-)
> > > > >>
> > > > >>diff --git a/block/qcow2.c b/block/qcow2.c
> > > > >>index 308121a..39a8afc 100644
> > > > >>--- a/block/qcow2.c
> > > > >>+++ b/block/qcow2.c
> > > > >>@@ -63,6 +63,7 @@ typedef struct {
> > > > >>  #define  QCOW2_EXT_MAGIC_END 0
> > > > >>  #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
> > > > >>  #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
> > > > >>+#define  QCOW2_EXT_MAGIC_COMPRESS_FORMAT 0xC03183A3
> > > > >>  static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
> > > > >>  {
> > > > >>@@ -76,6 +77,26 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
> > > > >>          return 0;
> > > > >>  }
> > > > >>+static int qcow2_compress_format_from_name(char *fmt)
> > > > >>+{
> > > > >>+    if (!fmt || !fmt[0]) {
> > > > >>+        return QCOW2_COMPRESS_ZLIB_COMPAT;
> > > > >>+    } else if (g_str_equal(fmt, "zlib")) {
> > > > >>+        return QCOW2_COMPRESS_ZLIB;
> > > > >>+    } else {
> > > > >>+        return -EINVAL;
> > > > >>+    }
> > > > >>+}
> > > > >It might make sense to allow specifying the old compression format in
> > > > >the compression header. But I'm not so sure about using the empty string
> > > > >rather than a proper name for it, and even less about not documenting
> > > > >it.
> > > > 
> > > > The old format is used if and only if the compression header is absent.
> > > > It makes no sense to write a header and use the old format. The old
> > > > settings are suboptimal and old versions can't open a qcow2 with a
> > > > compression header anyway.
> > > 
> > > Your code allows an empty string in the header extension. If you don't
> > > want it there, you need to reject it.
> > 
> > This is a good reason to use the QAPI schema to parse the create options
> > instead of doing it using qemu_opts - the QAPI schema will generate correct
> > code to parse & validate enum values 
> 
> I'd really like to see QAPI used for bdrv_create(), but here we're in
> the bdrv_open() path and parsing qcow2 headers. So either way this one
> specifically wouldn't be fixed.

Sorry, yes, I was getting mixed up in the two different patches with
similar bits of code.

We could still replace the qcow2_compress_format_from_name() method
though with a call to the qapi_enum_parse() passing in the
'QcowCompressFormat_lookup' table.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH V2 5/8] block/qcow2: read and write the compress format extension

[Daniel P. Berrange]

On Mon, Jul 10, 2017 at 03:46:21PM +0200, Peter Lieven wrote:
> Am 10.07.2017 um 15:44 schrieb Daniel P. Berrange:
> > On Mon, Jul 10, 2017 at 03:34:59PM +0200, Kevin Wolf wrote:
> > > Am 10.07.2017 um 15:29 hat Peter Lieven geschrieben:
> > > > Am 10.07.2017 um 15:25 schrieb Kevin Wolf:
> > > > > Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
> > > > > > we now read the extension on open and write it on update, but
> > > > > > do not yet use it.
> > > > > > 
> > > > > > Signed-off-by: Peter Lieven <pl@kamp.de>
> > > > > > ---
> > > > > >   block/qcow2.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
> > > > > >   block/qcow2.h |  23 +++++++++++---
> > > > > >   2 files changed, 104 insertions(+), 19 deletions(-)
> > > > > > 
> > > > > > diff --git a/block/qcow2.c b/block/qcow2.c
> > > > > > index 308121a..39a8afc 100644
> > > > > > --- a/block/qcow2.c
> > > > > > +++ b/block/qcow2.c
> > > > > > @@ -63,6 +63,7 @@ typedef struct {
> > > > > >   #define  QCOW2_EXT_MAGIC_END 0
> > > > > >   #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
> > > > > >   #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
> > > > > > +#define  QCOW2_EXT_MAGIC_COMPRESS_FORMAT 0xC03183A3
> > > > > >   static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
> > > > > >   {
> > > > > > @@ -76,6 +77,26 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
> > > > > >           return 0;
> > > > > >   }
> > > > > > +static int qcow2_compress_format_from_name(char *fmt)
> > > > > > +{
> > > > > > +    if (!fmt || !fmt[0]) {
> > > > > > +        return QCOW2_COMPRESS_ZLIB_COMPAT;
> > > > > > +    } else if (g_str_equal(fmt, "zlib")) {
> > > > > > +        return QCOW2_COMPRESS_ZLIB;
> > > > > > +    } else {
> > > > > > +        return -EINVAL;
> > > > > > +    }
> > > > > > +}
> > > > > It might make sense to allow specifying the old compression format in
> > > > > the compression header. But I'm not so sure about using the empty string
> > > > > rather than a proper name for it, and even less about not documenting
> > > > > it.
> > > > The old format is used if and only if the compression header is absent.
> > > > It makes no sense to write a header and use the old format. The old
> > > > settings are suboptimal and old versions can't open a qcow2 with a
> > > > compression header anyway.
> > > Your code allows an empty string in the header extension. If you don't
> > > want it there, you need to reject it.
> > This is a good reason to use the QAPI schema to parse the create options
> > instead of doing it using qemu_opts - the QAPI schema will generate correct
> > code to parse & validate enum values
> 
> Can you then please advise where to add the schematas and which functions
> to use for parsing it? You wrote that it is a bit hackish at the moment as not
> everything is available in qcow2_create path.

I just sent an (untested) example for patch 3 that should help. If you still
have problems let me know.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH V2 1/8] docs: add compress format extension to qcow2 spec

[Eric Blake]

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

On 07/10/2017 08:50 AM, Kevin Wolf wrote:

> We have regretted enough things that were defined a bit sloppily just
> for convenience that I'd rather not make this mistake again when we can
> avoid it.

Agreed.  Another consideration: defining redundant information is
trickier to maintain because correct implementations have to check that
both points are consistent with one another.

> 
>>>> +         17 - 19:  Reserved for future use, must be zero.
>>> We don't need this for now because byte 16 will be the final one in this
>>> struct.
>>>
>>>> +         20 - 23:  extra_data_size
>>>> +                   Size of compress format specific extra data.
>>>> +                   For now, as no format specifies extra data,
>>>> +                   extra_data_size is reserved and should be zero.
>>>> +
>>>> +        variable:  extra_data
>>>> +                   Extra data with additional parameters for the compress
>>>> +                   format, occupying extra_data_size bytes.
>>> extra_data_size isn't necessary because we already have the length of
>>> the header extension, so we know how long the whole thing is.
>>> extra_data isn't necessary because we'll just add new fields at the end
>>> if we want to add something.
>>
>> I was asked to add it altough it is redundant.
> 
> Who asked this, and did they have a good reason for it?

May have been me, or may have been implied by my comments, but I agree
that the overall extension header covers this, so we don't need a
redundant form.  So as long as the overall extension header length is
good enough, I'm okay avoiding redundancies.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH V2 5/8] block/qcow2: read and write the compress format extension

[Peter Lieven]

Am 10.07.2017 um 15:55 schrieb Daniel P. Berrange:
> On Mon, Jul 10, 2017 at 03:52:18PM +0200, Kevin Wolf wrote:
>> Am 10.07.2017 um 15:44 hat Daniel P. Berrange geschrieben:
>>> On Mon, Jul 10, 2017 at 03:34:59PM +0200, Kevin Wolf wrote:
>>>> Am 10.07.2017 um 15:29 hat Peter Lieven geschrieben:
>>>>> Am 10.07.2017 um 15:25 schrieb Kevin Wolf:
>>>>>> Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
>>>>>>> we now read the extension on open and write it on update, but
>>>>>>> do not yet use it.
>>>>>>>
>>>>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>>>>> ---
>>>>>>>   block/qcow2.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
>>>>>>>   block/qcow2.h |  23 +++++++++++---
>>>>>>>   2 files changed, 104 insertions(+), 19 deletions(-)
>>>>>>>
>>>>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>>>>> index 308121a..39a8afc 100644
>>>>>>> --- a/block/qcow2.c
>>>>>>> +++ b/block/qcow2.c
>>>>>>> @@ -63,6 +63,7 @@ typedef struct {
>>>>>>>   #define  QCOW2_EXT_MAGIC_END 0
>>>>>>>   #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
>>>>>>>   #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
>>>>>>> +#define  QCOW2_EXT_MAGIC_COMPRESS_FORMAT 0xC03183A3
>>>>>>>   static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
>>>>>>>   {
>>>>>>> @@ -76,6 +77,26 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
>>>>>>>           return 0;
>>>>>>>   }
>>>>>>> +static int qcow2_compress_format_from_name(char *fmt)
>>>>>>> +{
>>>>>>> +    if (!fmt || !fmt[0]) {
>>>>>>> +        return QCOW2_COMPRESS_ZLIB_COMPAT;
>>>>>>> +    } else if (g_str_equal(fmt, "zlib")) {
>>>>>>> +        return QCOW2_COMPRESS_ZLIB;
>>>>>>> +    } else {
>>>>>>> +        return -EINVAL;
>>>>>>> +    }
>>>>>>> +}
>>>>>> It might make sense to allow specifying the old compression format in
>>>>>> the compression header. But I'm not so sure about using the empty string
>>>>>> rather than a proper name for it, and even less about not documenting
>>>>>> it.
>>>>> The old format is used if and only if the compression header is absent.
>>>>> It makes no sense to write a header and use the old format. The old
>>>>> settings are suboptimal and old versions can't open a qcow2 with a
>>>>> compression header anyway.
>>>> Your code allows an empty string in the header extension. If you don't
>>>> want it there, you need to reject it.
>>> This is a good reason to use the QAPI schema to parse the create options
>>> instead of doing it using qemu_opts - the QAPI schema will generate correct
>>> code to parse & validate enum values
>> I'd really like to see QAPI used for bdrv_create(), but here we're in
>> the bdrv_open() path and parsing qcow2 headers. So either way this one
>> specifically wouldn't be fixed.
> Sorry, yes, I was getting mixed up in the two different patches with
> similar bits of code.
>
> We could still replace the qcow2_compress_format_from_name() method
> though with a call to the qapi_enum_parse() passing in the
> 'QcowCompressFormat_lookup' table.

Can you advise how exactly I would do that? Sorry, but this is new stuff for me.

Thanks,
Peter

Re: [Qemu-devel] [PATCH V2 2/8] qapi: add compress parameters to Qcow2 Blockdev options

[Peter Lieven]

Am 10.07.2017 um 15:30 schrieb Kevin Wolf:
> Am 10.07.2017 um 15:24 hat Peter Lieven geschrieben:
>> Am 10.07.2017 um 15:10 schrieb Kevin Wolf:
>>> Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>> ---
>>>>   qapi/block-core.json | 44 +++++++++++++++++++++++++++++++++++++++++++-
>>>>   1 file changed, 43 insertions(+), 1 deletion(-)
>>> How does this make sense as a runtime option? What would happen if the
>>> image contains one compression format and I specify another one on the
>>> command line or in blockdev-add?
>>>
>>> Shouldn't it just be a create-time option and when you run qemu, it uses
>>> whatever format that image has?
>> I was asked to add it here. It is indeed only a create option and has
>> no other effect.
> But if it's only a create option, why is it added to blockdev-add?

Is there already a separate schemata for the create options?

Thanks,
Peter

Re: [Qemu-devel] [PATCH V2 2/8] qapi: add compress parameters to Qcow2 Blockdev options

[Kevin Wolf]

Am 13.07.2017 um 10:45 hat Peter Lieven geschrieben:
> Am 10.07.2017 um 15:30 schrieb Kevin Wolf:
> >Am 10.07.2017 um 15:24 hat Peter Lieven geschrieben:
> >>Am 10.07.2017 um 15:10 schrieb Kevin Wolf:
> >>>Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
> >>>>Signed-off-by: Peter Lieven <pl@kamp.de>
> >>>>---
> >>>>  qapi/block-core.json | 44 +++++++++++++++++++++++++++++++++++++++++++-
> >>>>  1 file changed, 43 insertions(+), 1 deletion(-)
> >>>How does this make sense as a runtime option? What would happen if the
> >>>image contains one compression format and I specify another one on the
> >>>command line or in blockdev-add?
> >>>
> >>>Shouldn't it just be a create-time option and when you run qemu, it uses
> >>>whatever format that image has?
> >>I was asked to add it here. It is indeed only a create option and has
> >>no other effect.
> >But if it's only a create option, why is it added to blockdev-add?
> 
> Is there already a separate schemata for the create options?

If you mean a separate schema file, no. Putting it in the same file
isn't a problem either, but hooking it up in structs used by
blockdev-add (or other QMP commands) probably is.

So if you want to follow Dan's suggestion, I think you just need to put
the new type into the schema, without referencing it anywhere except in
qcow2's .bdrv_create C implementation.

Kevin

Re: [Qemu-devel] [PATCH V2 2/8] qapi: add compress parameters to Qcow2 Blockdev options

[Daniel P. Berrange]

On Thu, Jul 13, 2017 at 10:52:41AM +0200, Kevin Wolf wrote:
> Am 13.07.2017 um 10:45 hat Peter Lieven geschrieben:
> > Am 10.07.2017 um 15:30 schrieb Kevin Wolf:
> > >Am 10.07.2017 um 15:24 hat Peter Lieven geschrieben:
> > >>Am 10.07.2017 um 15:10 schrieb Kevin Wolf:
> > >>>Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
> > >>>>Signed-off-by: Peter Lieven <pl@kamp.de>
> > >>>>---
> > >>>>  qapi/block-core.json | 44 +++++++++++++++++++++++++++++++++++++++++++-
> > >>>>  1 file changed, 43 insertions(+), 1 deletion(-)
> > >>>How does this make sense as a runtime option? What would happen if the
> > >>>image contains one compression format and I specify another one on the
> > >>>command line or in blockdev-add?
> > >>>
> > >>>Shouldn't it just be a create-time option and when you run qemu, it uses
> > >>>whatever format that image has?
> > >>I was asked to add it here. It is indeed only a create option and has
> > >>no other effect.
> > >But if it's only a create option, why is it added to blockdev-add?
> > 
> > Is there already a separate schemata for the create options?
> 
> If you mean a separate schema file, no. Putting it in the same file
> isn't a problem either, but hooking it up in structs used by
> blockdev-add (or other QMP commands) probably is.
> 
> So if you want to follow Dan's suggestion, I think you just need to put
> the new type into the schema, without referencing it anywhere except in
> qcow2's .bdrv_create C implementation.

Yep, that's exactly what I intended by my suggestion

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH V2 5/8] block/qcow2: read and write the compress format extension

[Daniel P. Berrange]

On Thu, Jul 13, 2017 at 10:44:53AM +0200, Peter Lieven wrote:
> Am 10.07.2017 um 15:55 schrieb Daniel P. Berrange:
> > On Mon, Jul 10, 2017 at 03:52:18PM +0200, Kevin Wolf wrote:
> > > Am 10.07.2017 um 15:44 hat Daniel P. Berrange geschrieben:
> > > > On Mon, Jul 10, 2017 at 03:34:59PM +0200, Kevin Wolf wrote:
> > > > > Am 10.07.2017 um 15:29 hat Peter Lieven geschrieben:
> > > > > > Am 10.07.2017 um 15:25 schrieb Kevin Wolf:
> > > > > > > Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
> > > > > > > > we now read the extension on open and write it on update, but
> > > > > > > > do not yet use it.
> > > > > > > > 
> > > > > > > > Signed-off-by: Peter Lieven <pl@kamp.de>
> > > > > > > > ---
> > > > > > > >   block/qcow2.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
> > > > > > > >   block/qcow2.h |  23 +++++++++++---
> > > > > > > >   2 files changed, 104 insertions(+), 19 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/block/qcow2.c b/block/qcow2.c
> > > > > > > > index 308121a..39a8afc 100644
> > > > > > > > --- a/block/qcow2.c
> > > > > > > > +++ b/block/qcow2.c
> > > > > > > > @@ -63,6 +63,7 @@ typedef struct {
> > > > > > > >   #define  QCOW2_EXT_MAGIC_END 0
> > > > > > > >   #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
> > > > > > > >   #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
> > > > > > > > +#define  QCOW2_EXT_MAGIC_COMPRESS_FORMAT 0xC03183A3
> > > > > > > >   static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
> > > > > > > >   {
> > > > > > > > @@ -76,6 +77,26 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
> > > > > > > >           return 0;
> > > > > > > >   }
> > > > > > > > +static int qcow2_compress_format_from_name(char *fmt)
> > > > > > > > +{
> > > > > > > > +    if (!fmt || !fmt[0]) {
> > > > > > > > +        return QCOW2_COMPRESS_ZLIB_COMPAT;
> > > > > > > > +    } else if (g_str_equal(fmt, "zlib")) {
> > > > > > > > +        return QCOW2_COMPRESS_ZLIB;
> > > > > > > > +    } else {
> > > > > > > > +        return -EINVAL;
> > > > > > > > +    }
> > > > > > > > +}
> > > > > > > It might make sense to allow specifying the old compression format in
> > > > > > > the compression header. But I'm not so sure about using the empty string
> > > > > > > rather than a proper name for it, and even less about not documenting
> > > > > > > it.
> > > > > > The old format is used if and only if the compression header is absent.
> > > > > > It makes no sense to write a header and use the old format. The old
> > > > > > settings are suboptimal and old versions can't open a qcow2 with a
> > > > > > compression header anyway.
> > > > > Your code allows an empty string in the header extension. If you don't
> > > > > want it there, you need to reject it.
> > > > This is a good reason to use the QAPI schema to parse the create options
> > > > instead of doing it using qemu_opts - the QAPI schema will generate correct
> > > > code to parse & validate enum values
> > > I'd really like to see QAPI used for bdrv_create(), but here we're in
> > > the bdrv_open() path and parsing qcow2 headers. So either way this one
> > > specifically wouldn't be fixed.
> > Sorry, yes, I was getting mixed up in the two different patches with
> > similar bits of code.
> > 
> > We could still replace the qcow2_compress_format_from_name() method
> > though with a call to the qapi_enum_parse() passing in the
> > 'QcowCompressFormat_lookup' table.
> 
> Can you advise how exactly I would do that? Sorry, but this is new stuff
> for me.

In your line of code:

          s->compress_format_id =
               qcow2_compress_format_from_name(s->compress_format.name);

Instead do

           s->compress_format_id =
               qapi_enum_parse(QcowCompressFormat_lookup, s->compress_format.name);


The 'QcowCompressFormat_lookup' symbol is a global variable that is an
array of strings, automatically created from the 'enum' you define in
the QAPI schema.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH V2 5/8] block/qcow2: read and write the compress format extension

[Peter Lieven]

Am 13.07.2017 um 11:21 schrieb Daniel P. Berrange:
> On Thu, Jul 13, 2017 at 10:44:53AM +0200, Peter Lieven wrote:
>> Am 10.07.2017 um 15:55 schrieb Daniel P. Berrange:
>>> On Mon, Jul 10, 2017 at 03:52:18PM +0200, Kevin Wolf wrote:
>>>> Am 10.07.2017 um 15:44 hat Daniel P. Berrange geschrieben:
>>>>> On Mon, Jul 10, 2017 at 03:34:59PM +0200, Kevin Wolf wrote:
>>>>>> Am 10.07.2017 um 15:29 hat Peter Lieven geschrieben:
>>>>>>> Am 10.07.2017 um 15:25 schrieb Kevin Wolf:
>>>>>>>> Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
>>>>>>>>> we now read the extension on open and write it on update, but
>>>>>>>>> do not yet use it.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>>>>>>> ---
>>>>>>>>>    block/qcow2.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
>>>>>>>>>    block/qcow2.h |  23 +++++++++++---
>>>>>>>>>    2 files changed, 104 insertions(+), 19 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>>>>>>> index 308121a..39a8afc 100644
>>>>>>>>> --- a/block/qcow2.c
>>>>>>>>> +++ b/block/qcow2.c
>>>>>>>>> @@ -63,6 +63,7 @@ typedef struct {
>>>>>>>>>    #define  QCOW2_EXT_MAGIC_END 0
>>>>>>>>>    #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
>>>>>>>>>    #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
>>>>>>>>> +#define  QCOW2_EXT_MAGIC_COMPRESS_FORMAT 0xC03183A3
>>>>>>>>>    static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
>>>>>>>>>    {
>>>>>>>>> @@ -76,6 +77,26 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
>>>>>>>>>            return 0;
>>>>>>>>>    }
>>>>>>>>> +static int qcow2_compress_format_from_name(char *fmt)
>>>>>>>>> +{
>>>>>>>>> +    if (!fmt || !fmt[0]) {
>>>>>>>>> +        return QCOW2_COMPRESS_ZLIB_COMPAT;
>>>>>>>>> +    } else if (g_str_equal(fmt, "zlib")) {
>>>>>>>>> +        return QCOW2_COMPRESS_ZLIB;
>>>>>>>>> +    } else {
>>>>>>>>> +        return -EINVAL;
>>>>>>>>> +    }
>>>>>>>>> +}
>>>>>>>> It might make sense to allow specifying the old compression format in
>>>>>>>> the compression header. But I'm not so sure about using the empty string
>>>>>>>> rather than a proper name for it, and even less about not documenting
>>>>>>>> it.
>>>>>>> The old format is used if and only if the compression header is absent.
>>>>>>> It makes no sense to write a header and use the old format. The old
>>>>>>> settings are suboptimal and old versions can't open a qcow2 with a
>>>>>>> compression header anyway.
>>>>>> Your code allows an empty string in the header extension. If you don't
>>>>>> want it there, you need to reject it.
>>>>> This is a good reason to use the QAPI schema to parse the create options
>>>>> instead of doing it using qemu_opts - the QAPI schema will generate correct
>>>>> code to parse & validate enum values
>>>> I'd really like to see QAPI used for bdrv_create(), but here we're in
>>>> the bdrv_open() path and parsing qcow2 headers. So either way this one
>>>> specifically wouldn't be fixed.
>>> Sorry, yes, I was getting mixed up in the two different patches with
>>> similar bits of code.
>>>
>>> We could still replace the qcow2_compress_format_from_name() method
>>> though with a call to the qapi_enum_parse() passing in the
>>> 'QcowCompressFormat_lookup' table.
>> Can you advise how exactly I would do that? Sorry, but this is new stuff
>> for me.
> In your line of code:
>
>            s->compress_format_id =
>                 qcow2_compress_format_from_name(s->compress_format.name);
>
> Instead do
>
>             s->compress_format_id =
>                 qapi_enum_parse(QcowCompressFormat_lookup, s->compress_format.name);
>
>
> The 'QcowCompressFormat_lookup' symbol is a global variable that is an
> array of strings, automatically created from the 'enum' you define in
> the QAPI schema.

Is it possible to limit the valid integer values for an integer in the QAPI specification?

I would like to do that for the compress.level stuff.

Thanks,
Peter

Re: [Qemu-devel] [PATCH V2 5/8] block/qcow2: read and write the compress format extension

[Daniel P. Berrange]

On Thu, Jul 13, 2017 at 03:49:09PM +0200, Peter Lieven wrote:
> Am 13.07.2017 um 11:21 schrieb Daniel P. Berrange:
> > On Thu, Jul 13, 2017 at 10:44:53AM +0200, Peter Lieven wrote:
> > > Am 10.07.2017 um 15:55 schrieb Daniel P. Berrange:
> > > > On Mon, Jul 10, 2017 at 03:52:18PM +0200, Kevin Wolf wrote:
> > > > > Am 10.07.2017 um 15:44 hat Daniel P. Berrange geschrieben:
> > > > > > On Mon, Jul 10, 2017 at 03:34:59PM +0200, Kevin Wolf wrote:
> > > > > > > Am 10.07.2017 um 15:29 hat Peter Lieven geschrieben:
> > > > > > > > Am 10.07.2017 um 15:25 schrieb Kevin Wolf:
> > > > > > > > > Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
> > > > > > > > > > we now read the extension on open and write it on update, but
> > > > > > > > > > do not yet use it.
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Peter Lieven <pl@kamp.de>
> > > > > > > > > > ---
> > > > > > > > > >    block/qcow2.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
> > > > > > > > > >    block/qcow2.h |  23 +++++++++++---
> > > > > > > > > >    2 files changed, 104 insertions(+), 19 deletions(-)
> > > > > > > > > > 
> > > > > > > > > > diff --git a/block/qcow2.c b/block/qcow2.c
> > > > > > > > > > index 308121a..39a8afc 100644
> > > > > > > > > > --- a/block/qcow2.c
> > > > > > > > > > +++ b/block/qcow2.c
> > > > > > > > > > @@ -63,6 +63,7 @@ typedef struct {
> > > > > > > > > >    #define  QCOW2_EXT_MAGIC_END 0
> > > > > > > > > >    #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
> > > > > > > > > >    #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
> > > > > > > > > > +#define  QCOW2_EXT_MAGIC_COMPRESS_FORMAT 0xC03183A3
> > > > > > > > > >    static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
> > > > > > > > > >    {
> > > > > > > > > > @@ -76,6 +77,26 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
> > > > > > > > > >            return 0;
> > > > > > > > > >    }
> > > > > > > > > > +static int qcow2_compress_format_from_name(char *fmt)
> > > > > > > > > > +{
> > > > > > > > > > +    if (!fmt || !fmt[0]) {
> > > > > > > > > > +        return QCOW2_COMPRESS_ZLIB_COMPAT;
> > > > > > > > > > +    } else if (g_str_equal(fmt, "zlib")) {
> > > > > > > > > > +        return QCOW2_COMPRESS_ZLIB;
> > > > > > > > > > +    } else {
> > > > > > > > > > +        return -EINVAL;
> > > > > > > > > > +    }
> > > > > > > > > > +}
> > > > > > > > > It might make sense to allow specifying the old compression format in
> > > > > > > > > the compression header. But I'm not so sure about using the empty string
> > > > > > > > > rather than a proper name for it, and even less about not documenting
> > > > > > > > > it.
> > > > > > > > The old format is used if and only if the compression header is absent.
> > > > > > > > It makes no sense to write a header and use the old format. The old
> > > > > > > > settings are suboptimal and old versions can't open a qcow2 with a
> > > > > > > > compression header anyway.
> > > > > > > Your code allows an empty string in the header extension. If you don't
> > > > > > > want it there, you need to reject it.
> > > > > > This is a good reason to use the QAPI schema to parse the create options
> > > > > > instead of doing it using qemu_opts - the QAPI schema will generate correct
> > > > > > code to parse & validate enum values
> > > > > I'd really like to see QAPI used for bdrv_create(), but here we're in
> > > > > the bdrv_open() path and parsing qcow2 headers. So either way this one
> > > > > specifically wouldn't be fixed.
> > > > Sorry, yes, I was getting mixed up in the two different patches with
> > > > similar bits of code.
> > > > 
> > > > We could still replace the qcow2_compress_format_from_name() method
> > > > though with a call to the qapi_enum_parse() passing in the
> > > > 'QcowCompressFormat_lookup' table.
> > > Can you advise how exactly I would do that? Sorry, but this is new stuff
> > > for me.
> > In your line of code:
> > 
> >            s->compress_format_id =
> >                 qcow2_compress_format_from_name(s->compress_format.name);
> > 
> > Instead do
> > 
> >             s->compress_format_id =
> >                 qapi_enum_parse(QcowCompressFormat_lookup, s->compress_format.name);
> > 
> > 
> > The 'QcowCompressFormat_lookup' symbol is a global variable that is an
> > array of strings, automatically created from the 'enum' you define in
> > the QAPI schema.
> 
> Is it possible to limit the valid integer values for an integer in the QAPI specification?
> 
> I would like to do that for the compress.level stuff.

Not that I know of.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH V2 5/8] block/qcow2: read and write the compress format extension

[Peter Lieven]

Am 13.07.2017 um 16:00 schrieb Daniel P. Berrange:
> On Thu, Jul 13, 2017 at 03:49:09PM +0200, Peter Lieven wrote:
>> Am 13.07.2017 um 11:21 schrieb Daniel P. Berrange:
>>> On Thu, Jul 13, 2017 at 10:44:53AM +0200, Peter Lieven wrote:
>>>> Am 10.07.2017 um 15:55 schrieb Daniel P. Berrange:
>>>>> On Mon, Jul 10, 2017 at 03:52:18PM +0200, Kevin Wolf wrote:
>>>>>> Am 10.07.2017 um 15:44 hat Daniel P. Berrange geschrieben:
>>>>>>> On Mon, Jul 10, 2017 at 03:34:59PM +0200, Kevin Wolf wrote:
>>>>>>>> Am 10.07.2017 um 15:29 hat Peter Lieven geschrieben:
>>>>>>>>> Am 10.07.2017 um 15:25 schrieb Kevin Wolf:
>>>>>>>>>> Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
>>>>>>>>>>> we now read the extension on open and write it on update, but
>>>>>>>>>>> do not yet use it.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>>>>>>>>> ---
>>>>>>>>>>>     block/qcow2.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
>>>>>>>>>>>     block/qcow2.h |  23 +++++++++++---
>>>>>>>>>>>     2 files changed, 104 insertions(+), 19 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>>>>>>>>> index 308121a..39a8afc 100644
>>>>>>>>>>> --- a/block/qcow2.c
>>>>>>>>>>> +++ b/block/qcow2.c
>>>>>>>>>>> @@ -63,6 +63,7 @@ typedef struct {
>>>>>>>>>>>     #define  QCOW2_EXT_MAGIC_END 0
>>>>>>>>>>>     #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
>>>>>>>>>>>     #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
>>>>>>>>>>> +#define  QCOW2_EXT_MAGIC_COMPRESS_FORMAT 0xC03183A3
>>>>>>>>>>>     static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
>>>>>>>>>>>     {
>>>>>>>>>>> @@ -76,6 +77,26 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
>>>>>>>>>>>             return 0;
>>>>>>>>>>>     }
>>>>>>>>>>> +static int qcow2_compress_format_from_name(char *fmt)
>>>>>>>>>>> +{
>>>>>>>>>>> +    if (!fmt || !fmt[0]) {
>>>>>>>>>>> +        return QCOW2_COMPRESS_ZLIB_COMPAT;
>>>>>>>>>>> +    } else if (g_str_equal(fmt, "zlib")) {
>>>>>>>>>>> +        return QCOW2_COMPRESS_ZLIB;
>>>>>>>>>>> +    } else {
>>>>>>>>>>> +        return -EINVAL;
>>>>>>>>>>> +    }
>>>>>>>>>>> +}
>>>>>>>>>> It might make sense to allow specifying the old compression format in
>>>>>>>>>> the compression header. But I'm not so sure about using the empty string
>>>>>>>>>> rather than a proper name for it, and even less about not documenting
>>>>>>>>>> it.
>>>>>>>>> The old format is used if and only if the compression header is absent.
>>>>>>>>> It makes no sense to write a header and use the old format. The old
>>>>>>>>> settings are suboptimal and old versions can't open a qcow2 with a
>>>>>>>>> compression header anyway.
>>>>>>>> Your code allows an empty string in the header extension. If you don't
>>>>>>>> want it there, you need to reject it.
>>>>>>> This is a good reason to use the QAPI schema to parse the create options
>>>>>>> instead of doing it using qemu_opts - the QAPI schema will generate correct
>>>>>>> code to parse & validate enum values
>>>>>> I'd really like to see QAPI used for bdrv_create(), but here we're in
>>>>>> the bdrv_open() path and parsing qcow2 headers. So either way this one
>>>>>> specifically wouldn't be fixed.
>>>>> Sorry, yes, I was getting mixed up in the two different patches with
>>>>> similar bits of code.
>>>>>
>>>>> We could still replace the qcow2_compress_format_from_name() method
>>>>> though with a call to the qapi_enum_parse() passing in the
>>>>> 'QcowCompressFormat_lookup' table.
>>>> Can you advise how exactly I would do that? Sorry, but this is new stuff
>>>> for me.
>>> In your line of code:
>>>
>>>             s->compress_format_id =
>>>                  qcow2_compress_format_from_name(s->compress_format.name);
>>>
>>> Instead do
>>>
>>>              s->compress_format_id =
>>>                  qapi_enum_parse(QcowCompressFormat_lookup, s->compress_format.name);
>>>
>>>
>>> The 'QcowCompressFormat_lookup' symbol is a global variable that is an
>>> array of strings, automatically created from the 'enum' you define in
>>> the QAPI schema.
>> Is it possible to limit the valid integer values for an integer in the QAPI specification?
>>
>> I would like to do that for the compress.level stuff.
> Not that I know of.

If I specify an enum, what values are assigned to them? I am thinking of specifying an enum {0, 1, 2, 3, 4, 5, 6, 7, 8, 9} for the valid
compress levels of zlib.

Peter

Re: [Qemu-devel] [PATCH V2 5/8] block/qcow2: read and write the compress format extension

[Daniel P. Berrange]

On Thu, Jul 13, 2017 at 04:03:47PM +0200, Peter Lieven wrote:
> Am 13.07.2017 um 16:00 schrieb Daniel P. Berrange:
> > On Thu, Jul 13, 2017 at 03:49:09PM +0200, Peter Lieven wrote:
> > > Am 13.07.2017 um 11:21 schrieb Daniel P. Berrange:
> > > > On Thu, Jul 13, 2017 at 10:44:53AM +0200, Peter Lieven wrote:
> > > > > Am 10.07.2017 um 15:55 schrieb Daniel P. Berrange:
> > > > > > On Mon, Jul 10, 2017 at 03:52:18PM +0200, Kevin Wolf wrote:
> > > > > > > Am 10.07.2017 um 15:44 hat Daniel P. Berrange geschrieben:
> > > > > > > > On Mon, Jul 10, 2017 at 03:34:59PM +0200, Kevin Wolf wrote:
> > > > > > > > > Am 10.07.2017 um 15:29 hat Peter Lieven geschrieben:
> > > > > > > > > > Am 10.07.2017 um 15:25 schrieb Kevin Wolf:
> > > > > > > > > > > Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
> > > > > > > > > > > > we now read the extension on open and write it on update, but
> > > > > > > > > > > > do not yet use it.
> > > > > > > > > > > > 
> > > > > > > > > > > > Signed-off-by: Peter Lieven <pl@kamp.de>
> > > > > > > > > > > > ---
> > > > > > > > > > > >     block/qcow2.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
> > > > > > > > > > > >     block/qcow2.h |  23 +++++++++++---
> > > > > > > > > > > >     2 files changed, 104 insertions(+), 19 deletions(-)
> > > > > > > > > > > > 
> > > > > > > > > > > > diff --git a/block/qcow2.c b/block/qcow2.c
> > > > > > > > > > > > index 308121a..39a8afc 100644
> > > > > > > > > > > > --- a/block/qcow2.c
> > > > > > > > > > > > +++ b/block/qcow2.c
> > > > > > > > > > > > @@ -63,6 +63,7 @@ typedef struct {
> > > > > > > > > > > >     #define  QCOW2_EXT_MAGIC_END 0
> > > > > > > > > > > >     #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
> > > > > > > > > > > >     #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
> > > > > > > > > > > > +#define  QCOW2_EXT_MAGIC_COMPRESS_FORMAT 0xC03183A3
> > > > > > > > > > > >     static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
> > > > > > > > > > > >     {
> > > > > > > > > > > > @@ -76,6 +77,26 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
> > > > > > > > > > > >             return 0;
> > > > > > > > > > > >     }
> > > > > > > > > > > > +static int qcow2_compress_format_from_name(char *fmt)
> > > > > > > > > > > > +{
> > > > > > > > > > > > +    if (!fmt || !fmt[0]) {
> > > > > > > > > > > > +        return QCOW2_COMPRESS_ZLIB_COMPAT;
> > > > > > > > > > > > +    } else if (g_str_equal(fmt, "zlib")) {
> > > > > > > > > > > > +        return QCOW2_COMPRESS_ZLIB;
> > > > > > > > > > > > +    } else {
> > > > > > > > > > > > +        return -EINVAL;
> > > > > > > > > > > > +    }
> > > > > > > > > > > > +}
> > > > > > > > > > > It might make sense to allow specifying the old compression format in
> > > > > > > > > > > the compression header. But I'm not so sure about using the empty string
> > > > > > > > > > > rather than a proper name for it, and even less about not documenting
> > > > > > > > > > > it.
> > > > > > > > > > The old format is used if and only if the compression header is absent.
> > > > > > > > > > It makes no sense to write a header and use the old format. The old
> > > > > > > > > > settings are suboptimal and old versions can't open a qcow2 with a
> > > > > > > > > > compression header anyway.
> > > > > > > > > Your code allows an empty string in the header extension. If you don't
> > > > > > > > > want it there, you need to reject it.
> > > > > > > > This is a good reason to use the QAPI schema to parse the create options
> > > > > > > > instead of doing it using qemu_opts - the QAPI schema will generate correct
> > > > > > > > code to parse & validate enum values
> > > > > > > I'd really like to see QAPI used for bdrv_create(), but here we're in
> > > > > > > the bdrv_open() path and parsing qcow2 headers. So either way this one
> > > > > > > specifically wouldn't be fixed.
> > > > > > Sorry, yes, I was getting mixed up in the two different patches with
> > > > > > similar bits of code.
> > > > > > 
> > > > > > We could still replace the qcow2_compress_format_from_name() method
> > > > > > though with a call to the qapi_enum_parse() passing in the
> > > > > > 'QcowCompressFormat_lookup' table.
> > > > > Can you advise how exactly I would do that? Sorry, but this is new stuff
> > > > > for me.
> > > > In your line of code:
> > > > 
> > > >             s->compress_format_id =
> > > >                  qcow2_compress_format_from_name(s->compress_format.name);
> > > > 
> > > > Instead do
> > > > 
> > > >              s->compress_format_id =
> > > >                  qapi_enum_parse(QcowCompressFormat_lookup, s->compress_format.name);
> > > > 
> > > > 
> > > > The 'QcowCompressFormat_lookup' symbol is a global variable that is an
> > > > array of strings, automatically created from the 'enum' you define in
> > > > the QAPI schema.
> > > Is it possible to limit the valid integer values for an integer in the QAPI specification?
> > > 
> > > I would like to do that for the compress.level stuff.
> > Not that I know of.
> 
> If I specify an enum, what values are assigned to them? I am thinking of
> specifying an enum {0, 1, 2, 3, 4, 5, 6, 7, 8, 9} for the valid
> compress levels of zlib.

It should assume enum values starting from zero, but I think that is not
considered ABI from QAPI POV. IOW, you shouldn't persist the enum values
anywhere - only the enum string representation is considered ABI stable.

Also you can't have enum string names starting with a digit. So you would
need enum {level0, level1, level2, etc.... }, and store the string
'level0', etc in the qcow2 header if you went this way. I think this is
kind of ugly compared to just using an int and doing range checks at time
of use.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH V2 5/8] block/qcow2: read and write the compress format extension

[Peter Lieven]

Am 13.07.2017 um 16:07 schrieb Daniel P. Berrange:
> On Thu, Jul 13, 2017 at 04:03:47PM +0200, Peter Lieven wrote:
>> Am 13.07.2017 um 16:00 schrieb Daniel P. Berrange:
>>> On Thu, Jul 13, 2017 at 03:49:09PM +0200, Peter Lieven wrote:
>>>> Am 13.07.2017 um 11:21 schrieb Daniel P. Berrange:
>>>>> On Thu, Jul 13, 2017 at 10:44:53AM +0200, Peter Lieven wrote:
>>>>>> Am 10.07.2017 um 15:55 schrieb Daniel P. Berrange:
>>>>>>> On Mon, Jul 10, 2017 at 03:52:18PM +0200, Kevin Wolf wrote:
>>>>>>>> Am 10.07.2017 um 15:44 hat Daniel P. Berrange geschrieben:
>>>>>>>>> On Mon, Jul 10, 2017 at 03:34:59PM +0200, Kevin Wolf wrote:
>>>>>>>>>> Am 10.07.2017 um 15:29 hat Peter Lieven geschrieben:
>>>>>>>>>>> Am 10.07.2017 um 15:25 schrieb Kevin Wolf:
>>>>>>>>>>>> Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
>>>>>>>>>>>>> we now read the extension on open and write it on update, but
>>>>>>>>>>>>> do not yet use it.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>      block/qcow2.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
>>>>>>>>>>>>>      block/qcow2.h |  23 +++++++++++---
>>>>>>>>>>>>>      2 files changed, 104 insertions(+), 19 deletions(-)
>>>>>>>>>>>>>
>>>>>>>>>>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>>>>>>>>>>> index 308121a..39a8afc 100644
>>>>>>>>>>>>> --- a/block/qcow2.c
>>>>>>>>>>>>> +++ b/block/qcow2.c
>>>>>>>>>>>>> @@ -63,6 +63,7 @@ typedef struct {
>>>>>>>>>>>>>      #define  QCOW2_EXT_MAGIC_END 0
>>>>>>>>>>>>>      #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
>>>>>>>>>>>>>      #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
>>>>>>>>>>>>> +#define  QCOW2_EXT_MAGIC_COMPRESS_FORMAT 0xC03183A3
>>>>>>>>>>>>>      static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
>>>>>>>>>>>>>      {
>>>>>>>>>>>>> @@ -76,6 +77,26 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
>>>>>>>>>>>>>              return 0;
>>>>>>>>>>>>>      }
>>>>>>>>>>>>> +static int qcow2_compress_format_from_name(char *fmt)
>>>>>>>>>>>>> +{
>>>>>>>>>>>>> +    if (!fmt || !fmt[0]) {
>>>>>>>>>>>>> +        return QCOW2_COMPRESS_ZLIB_COMPAT;
>>>>>>>>>>>>> +    } else if (g_str_equal(fmt, "zlib")) {
>>>>>>>>>>>>> +        return QCOW2_COMPRESS_ZLIB;
>>>>>>>>>>>>> +    } else {
>>>>>>>>>>>>> +        return -EINVAL;
>>>>>>>>>>>>> +    }
>>>>>>>>>>>>> +}
>>>>>>>>>>>> It might make sense to allow specifying the old compression format in
>>>>>>>>>>>> the compression header. But I'm not so sure about using the empty string
>>>>>>>>>>>> rather than a proper name for it, and even less about not documenting
>>>>>>>>>>>> it.
>>>>>>>>>>> The old format is used if and only if the compression header is absent.
>>>>>>>>>>> It makes no sense to write a header and use the old format. The old
>>>>>>>>>>> settings are suboptimal and old versions can't open a qcow2 with a
>>>>>>>>>>> compression header anyway.
>>>>>>>>>> Your code allows an empty string in the header extension. If you don't
>>>>>>>>>> want it there, you need to reject it.
>>>>>>>>> This is a good reason to use the QAPI schema to parse the create options
>>>>>>>>> instead of doing it using qemu_opts - the QAPI schema will generate correct
>>>>>>>>> code to parse & validate enum values
>>>>>>>> I'd really like to see QAPI used for bdrv_create(), but here we're in
>>>>>>>> the bdrv_open() path and parsing qcow2 headers. So either way this one
>>>>>>>> specifically wouldn't be fixed.
>>>>>>> Sorry, yes, I was getting mixed up in the two different patches with
>>>>>>> similar bits of code.
>>>>>>>
>>>>>>> We could still replace the qcow2_compress_format_from_name() method
>>>>>>> though with a call to the qapi_enum_parse() passing in the
>>>>>>> 'QcowCompressFormat_lookup' table.
>>>>>> Can you advise how exactly I would do that? Sorry, but this is new stuff
>>>>>> for me.
>>>>> In your line of code:
>>>>>
>>>>>              s->compress_format_id =
>>>>>                   qcow2_compress_format_from_name(s->compress_format.name);
>>>>>
>>>>> Instead do
>>>>>
>>>>>               s->compress_format_id =
>>>>>                   qapi_enum_parse(QcowCompressFormat_lookup, s->compress_format.name);
>>>>>
>>>>>
>>>>> The 'QcowCompressFormat_lookup' symbol is a global variable that is an
>>>>> array of strings, automatically created from the 'enum' you define in
>>>>> the QAPI schema.
>>>> Is it possible to limit the valid integer values for an integer in the QAPI specification?
>>>>
>>>> I would like to do that for the compress.level stuff.
>>> Not that I know of.
>> If I specify an enum, what values are assigned to them? I am thinking of
>> specifying an enum {0, 1, 2, 3, 4, 5, 6, 7, 8, 9} for the valid
>> compress levels of zlib.
> It should assume enum values starting from zero, but I think that is not
> considered ABI from QAPI POV. IOW, you shouldn't persist the enum values
> anywhere - only the enum string representation is considered ABI stable.
>
> Also you can't have enum string names starting with a digit. So you would
> need enum {level0, level1, level2, etc.... }, and store the string
> 'level0', etc in the qcow2 header if you went this way. I think this is
> kind of ugly compared to just using an int and doing range checks at time
> of use.

Okay, so it has to be a mix of QAPI parsing and manual parameter checking, right?

I currently have the following:

     options = qemu_opts_to_qdict(opts, NULL);
     qdict_extract_subqdict(options, &compressopts, "compress.");
     v = qobject_input_visitor_new_keyval(QOBJECT(compressopts));
     visit_start_struct(v, NULL, NULL, 0, &local_err);
     if (local_err) {
         ret= -EINVAL;
         goto finish;
     }
     visit_type_Qcow2Compress_members(v, &compress, &local_err);
     if (local_err) {
         ret= -EINVAL;
         goto finish;
     }
     visit_end_struct(v, NULL);
     visit_free(v);
     QDECREF(compressopts);
     QDECREF(options);

And I have the following 2 questions:
  a) I have to specifiy compress.format and compress.level otherwise I will get an error. How can I fix that the settings are optional?
  b) If I just specify a compress.format can I default the compress.level to 0 without an error?

Thanks,
Peter

Re: [Qemu-devel] [PATCH V2 5/8] block/qcow2: read and write the compress format extension

[Daniel P. Berrange]

On Thu, Jul 13, 2017 at 04:18:13PM +0200, Peter Lieven wrote:
> 
> Okay, so it has to be a mix of QAPI parsing and manual parameter checking,
> right?

Yeah. It does feel like a valid RFE for QAPI to add a permitted range to
'int' types though, which would simplify the code in future.

> I currently have the following:
> 
>     options = qemu_opts_to_qdict(opts, NULL);
>     qdict_extract_subqdict(options, &compressopts, "compress.");
>     v = qobject_input_visitor_new_keyval(QOBJECT(compressopts));
>     visit_start_struct(v, NULL, NULL, 0, &local_err);
>     if (local_err) {
>         ret= -EINVAL;
>         goto finish;
>     }
>     visit_type_Qcow2Compress_members(v, &compress, &local_err);
>     if (local_err) {
>         ret= -EINVAL;
>         goto finish;
>     }
>     visit_end_struct(v, NULL);
>     visit_free(v);
>     QDECREF(compressopts);
>     QDECREF(options)

Looks good.

> And I have the following 2 questions:
>  a) I have to specifiy compress.format and compress.level otherwise I will get an error. How can I fix that the settings are optional?

Put an '*' as the first character of any field name if it should be optional.

>  b) If I just specify a compress.format can I default the compress.level to 0 without an error?

I believe you'd get compress.level as 0 automatically for an 'int' type.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH V2 5/8] block/qcow2: read and write the compress format extension

[Peter Lieven]

Am 13.07.2017 um 16:58 schrieb Daniel P. Berrange:
> On Thu, Jul 13, 2017 at 04:18:13PM +0200, Peter Lieven wrote:
>> Okay, so it has to be a mix of QAPI parsing and manual parameter checking,
>> right?
> Yeah. It does feel like a valid RFE for QAPI to add a permitted range to
> 'int' types though, which would simplify the code in future.
>
>> I currently have the following:
>>
>>      options = qemu_opts_to_qdict(opts, NULL);
>>      qdict_extract_subqdict(options, &compressopts, "compress.");
>>      v = qobject_input_visitor_new_keyval(QOBJECT(compressopts));
>>      visit_start_struct(v, NULL, NULL, 0, &local_err);
>>      if (local_err) {
>>          ret= -EINVAL;
>>          goto finish;
>>      }
>>      visit_type_Qcow2Compress_members(v, &compress, &local_err);
>>      if (local_err) {
>>          ret= -EINVAL;
>>          goto finish;
>>      }
>>      visit_end_struct(v, NULL);
>>      visit_free(v);
>>      QDECREF(compressopts);
>>      QDECREF(options)
> Looks good.
>
>> And I have the following 2 questions:
>>   a) I have to specifiy compress.format and compress.level otherwise I will get an error. How can I fix that the settings are optional?
> Put an '*' as the first character of any field name if it should be optional.
>
>>   b) If I just specify a compress.format can I default the compress.level to 0 without an error?
> I believe you'd get compress.level as 0 automatically for an 'int' type.

I still face the issue that I now always have to specify a compress.format. I tried to solve it like this:

     int compress_format = -1;
     uint8_t compress_level = 0;
     if (qemu_opt_find(opts, BLOCK_OPT_COMPRESS_FORMAT) ||
         qemu_opt_find(opts, BLOCK_OPT_COMPRESS_LEVEL)) {
         QDict *options, *compressopts = NULL;
         Qcow2Compress compress;
         Visitor *v;
         options = qemu_opts_to_qdict(opts, NULL);
         qdict_extract_subqdict(options, &compressopts, "compress.");
         v = qobject_input_visitor_new_keyval(QOBJECT(compressopts));
         visit_start_struct(v, NULL, NULL, 0, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
             ret = -EINVAL;
             goto finish;
         }
         visit_type_Qcow2Compress_members(v, &compress, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
             ret = -EINVAL;
             goto finish;
         }
         visit_end_struct(v, NULL);
         visit_free(v);
         QDECREF(compressopts);
         QDECREF(options);

         compress_format = compress.format;
         compress_level = compress.level;
         if (compress_format == QCOW2_COMPRESS_FORMAT_ZLIB &&
             compress_level > 9) {
             error_setg(errp, "Compress level %" PRIu8 " is not supported for"
                        " format '%s'", compress_level,
                        qemu_opt_get(opts, BLOCK_OPT_COMPRESS_FORMAT));
             ret = -EINVAL;
             goto finish;
         }
     }

Maybe there is a better option?

Peter

Re: [Qemu-devel] [PATCH V2 5/8] block/qcow2: read and write the compress format extension

[Daniel P. Berrange]

On Thu, Jul 13, 2017 at 05:00:39PM +0200, Peter Lieven wrote:
> Am 13.07.2017 um 16:58 schrieb Daniel P. Berrange:
> > On Thu, Jul 13, 2017 at 04:18:13PM +0200, Peter Lieven wrote:
> > > Okay, so it has to be a mix of QAPI parsing and manual parameter checking,
> > > right?
> > Yeah. It does feel like a valid RFE for QAPI to add a permitted range to
> > 'int' types though, which would simplify the code in future.
> > 
> > > I currently have the following:
> > > 
> > >      options = qemu_opts_to_qdict(opts, NULL);
> > >      qdict_extract_subqdict(options, &compressopts, "compress.");
> > >      v = qobject_input_visitor_new_keyval(QOBJECT(compressopts));
> > >      visit_start_struct(v, NULL, NULL, 0, &local_err);
> > >      if (local_err) {
> > >          ret= -EINVAL;
> > >          goto finish;
> > >      }
> > >      visit_type_Qcow2Compress_members(v, &compress, &local_err);
> > >      if (local_err) {
> > >          ret= -EINVAL;
> > >          goto finish;
> > >      }
> > >      visit_end_struct(v, NULL);
> > >      visit_free(v);
> > >      QDECREF(compressopts);
> > >      QDECREF(options)
> > Looks good.
> > 
> > > And I have the following 2 questions:
> > >   a) I have to specifiy compress.format and compress.level otherwise I will get an error. How can I fix that the settings are optional?
> > Put an '*' as the first character of any field name if it should be optional.
> > 
> > >   b) If I just specify a compress.format can I default the compress.level to 0 without an error?
> > I believe you'd get compress.level as 0 automatically for an 'int' type.
> 
> I still face the issue that I now always have to specify a compress.format.
> I tried to solve it like this:

[snip]

that's not needed if you name the parameter '*level' in the QAPI schema

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH V2 5/8] block/qcow2: read and write the compress format extension

[Peter Lieven]

Am 13.07.2017 um 17:01 schrieb Daniel P. Berrange:
> On Thu, Jul 13, 2017 at 05:00:39PM +0200, Peter Lieven wrote:
>> Am 13.07.2017 um 16:58 schrieb Daniel P. Berrange:
>>> On Thu, Jul 13, 2017 at 04:18:13PM +0200, Peter Lieven wrote:
>>>> Okay, so it has to be a mix of QAPI parsing and manual parameter checking,
>>>> right?
>>> Yeah. It does feel like a valid RFE for QAPI to add a permitted range to
>>> 'int' types though, which would simplify the code in future.
>>>
>>>> I currently have the following:
>>>>
>>>>       options = qemu_opts_to_qdict(opts, NULL);
>>>>       qdict_extract_subqdict(options, &compressopts, "compress.");
>>>>       v = qobject_input_visitor_new_keyval(QOBJECT(compressopts));
>>>>       visit_start_struct(v, NULL, NULL, 0, &local_err);
>>>>       if (local_err) {
>>>>           ret= -EINVAL;
>>>>           goto finish;
>>>>       }
>>>>       visit_type_Qcow2Compress_members(v, &compress, &local_err);
>>>>       if (local_err) {
>>>>           ret= -EINVAL;
>>>>           goto finish;
>>>>       }
>>>>       visit_end_struct(v, NULL);
>>>>       visit_free(v);
>>>>       QDECREF(compressopts);
>>>>       QDECREF(options)
>>> Looks good.
>>>
>>>> And I have the following 2 questions:
>>>>    a) I have to specifiy compress.format and compress.level otherwise I will get an error. How can I fix that the settings are optional?
>>> Put an '*' as the first character of any field name if it should be optional.
>>>
>>>>    b) If I just specify a compress.format can I default the compress.level to 0 without an error?
>>> I believe you'd get compress.level as 0 automatically for an 'int' type.
>> I still face the issue that I now always have to specify a compress.format.
>> I tried to solve it like this:
> [snip]
>
> that's not needed if you name the parameter '*level' in the QAPI schema

its not needed for the *level, but I still get the error that the format parameter is missing otherwise.
And I can't make format optional.

Peter

Re: [Qemu-devel] [PATCH V2 5/8] block/qcow2: read and write the compress format extension

[Daniel P. Berrange]

On Thu, Jul 13, 2017 at 05:02:51PM +0200, Peter Lieven wrote:
> Am 13.07.2017 um 17:01 schrieb Daniel P. Berrange:
> > On Thu, Jul 13, 2017 at 05:00:39PM +0200, Peter Lieven wrote:
> > > Am 13.07.2017 um 16:58 schrieb Daniel P. Berrange:
> > > > On Thu, Jul 13, 2017 at 04:18:13PM +0200, Peter Lieven wrote:
> > > > > Okay, so it has to be a mix of QAPI parsing and manual parameter checking,
> > > > > right?
> > > > Yeah. It does feel like a valid RFE for QAPI to add a permitted range to
> > > > 'int' types though, which would simplify the code in future.
> > > > 
> > > > > I currently have the following:
> > > > > 
> > > > >       options = qemu_opts_to_qdict(opts, NULL);
> > > > >       qdict_extract_subqdict(options, &compressopts, "compress.");
> > > > >       v = qobject_input_visitor_new_keyval(QOBJECT(compressopts));
> > > > >       visit_start_struct(v, NULL, NULL, 0, &local_err);
> > > > >       if (local_err) {
> > > > >           ret= -EINVAL;
> > > > >           goto finish;
> > > > >       }
> > > > >       visit_type_Qcow2Compress_members(v, &compress, &local_err);
> > > > >       if (local_err) {
> > > > >           ret= -EINVAL;
> > > > >           goto finish;
> > > > >       }
> > > > >       visit_end_struct(v, NULL);
> > > > >       visit_free(v);
> > > > >       QDECREF(compressopts);
> > > > >       QDECREF(options)
> > > > Looks good.
> > > > 
> > > > > And I have the following 2 questions:
> > > > >    a) I have to specifiy compress.format and compress.level otherwise I will get an error. How can I fix that the settings are optional?
> > > > Put an '*' as the first character of any field name if it should be optional.
> > > > 
> > > > >    b) If I just specify a compress.format can I default the compress.level to 0 without an error?
> > > > I believe you'd get compress.level as 0 automatically for an 'int' type.
> > > I still face the issue that I now always have to specify a compress.format.
> > > I tried to solve it like this:
> > [snip]
> > 
> > that's not needed if you name the parameter '*level' in the QAPI schema
> 
> its not needed for the *level, but I still get the error that the format
> parameter is missing otherwise. And I can't make format optional.

Surely specifying compress.format is how you turn on compression in the
first place, so making that optional should not be necessary. eg the
simple case becomes:

  qemu-img create -f qcow2 -o compress.format=zlib  foo.qcow2 1G

Yes, there is no notion of a "default" format, but IMHO that's a good
thing - same way with encryption - it requires an explict encrypt.format=luks
to turn on encryption

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH V2 5/8] block/qcow2: read and write the compress format extension

[Peter Lieven]

Am 13.07.2017 um 17:06 schrieb Daniel P. Berrange:
> On Thu, Jul 13, 2017 at 05:02:51PM +0200, Peter Lieven wrote:
>> Am 13.07.2017 um 17:01 schrieb Daniel P. Berrange:
>>> On Thu, Jul 13, 2017 at 05:00:39PM +0200, Peter Lieven wrote:
>>>> Am 13.07.2017 um 16:58 schrieb Daniel P. Berrange:
>>>>> On Thu, Jul 13, 2017 at 04:18:13PM +0200, Peter Lieven wrote:
>>>>>> Okay, so it has to be a mix of QAPI parsing and manual parameter checking,
>>>>>> right?
>>>>> Yeah. It does feel like a valid RFE for QAPI to add a permitted range to
>>>>> 'int' types though, which would simplify the code in future.
>>>>>
>>>>>> I currently have the following:
>>>>>>
>>>>>>        options = qemu_opts_to_qdict(opts, NULL);
>>>>>>        qdict_extract_subqdict(options, &compressopts, "compress.");
>>>>>>        v = qobject_input_visitor_new_keyval(QOBJECT(compressopts));
>>>>>>        visit_start_struct(v, NULL, NULL, 0, &local_err);
>>>>>>        if (local_err) {
>>>>>>            ret= -EINVAL;
>>>>>>            goto finish;
>>>>>>        }
>>>>>>        visit_type_Qcow2Compress_members(v, &compress, &local_err);
>>>>>>        if (local_err) {
>>>>>>            ret= -EINVAL;
>>>>>>            goto finish;
>>>>>>        }
>>>>>>        visit_end_struct(v, NULL);
>>>>>>        visit_free(v);
>>>>>>        QDECREF(compressopts);
>>>>>>        QDECREF(options)
>>>>> Looks good.
>>>>>
>>>>>> And I have the following 2 questions:
>>>>>>     a) I have to specifiy compress.format and compress.level otherwise I will get an error. How can I fix that the settings are optional?
>>>>> Put an '*' as the first character of any field name if it should be optional.
>>>>>
>>>>>>     b) If I just specify a compress.format can I default the compress.level to 0 without an error?
>>>>> I believe you'd get compress.level as 0 automatically for an 'int' type.
>>>> I still face the issue that I now always have to specify a compress.format.
>>>> I tried to solve it like this:
>>> [snip]
>>>
>>> that's not needed if you name the parameter '*level' in the QAPI schema
>> its not needed for the *level, but I still get the error that the format
>> parameter is missing otherwise. And I can't make format optional.
> Surely specifying compress.format is how you turn on compression in the
> first place, so making that optional should not be necessary. eg the
> simple case becomes:
>
>    qemu-img create -f qcow2 -o compress.format=zlib  foo.qcow2 1G
>
> Yes, there is no notion of a "default" format, but IMHO that's a good
> thing - same way with encryption - it requires an explict encrypt.format=luks
> to turn on encryption

Yes, but visit_type_Qcow2Compress_members always returns an error if compress.format
is missing from the options. So I should not call it an error out if compress.format is not in the
options. Thus the check if either compress.format or compress.level is specified at all.

Peter

Re: [Qemu-devel] [PATCH V2 5/8] block/qcow2: read and write the compress format extension

[Daniel P. Berrange]

On Thu, Jul 13, 2017 at 05:13:23PM +0200, Peter Lieven wrote:
> Am 13.07.2017 um 17:06 schrieb Daniel P. Berrange:
> > On Thu, Jul 13, 2017 at 05:02:51PM +0200, Peter Lieven wrote:
> > > Am 13.07.2017 um 17:01 schrieb Daniel P. Berrange:
> > > > On Thu, Jul 13, 2017 at 05:00:39PM +0200, Peter Lieven wrote:
> > > > > Am 13.07.2017 um 16:58 schrieb Daniel P. Berrange:
> > > > > > On Thu, Jul 13, 2017 at 04:18:13PM +0200, Peter Lieven wrote:
> > > > > > > Okay, so it has to be a mix of QAPI parsing and manual parameter checking,
> > > > > > > right?
> > > > > > Yeah. It does feel like a valid RFE for QAPI to add a permitted range to
> > > > > > 'int' types though, which would simplify the code in future.
> > > > > > 
> > > > > > > I currently have the following:
> > > > > > > 
> > > > > > >        options = qemu_opts_to_qdict(opts, NULL);
> > > > > > >        qdict_extract_subqdict(options, &compressopts, "compress.");
> > > > > > >        v = qobject_input_visitor_new_keyval(QOBJECT(compressopts));
> > > > > > >        visit_start_struct(v, NULL, NULL, 0, &local_err);
> > > > > > >        if (local_err) {
> > > > > > >            ret= -EINVAL;
> > > > > > >            goto finish;
> > > > > > >        }
> > > > > > >        visit_type_Qcow2Compress_members(v, &compress, &local_err);
> > > > > > >        if (local_err) {
> > > > > > >            ret= -EINVAL;
> > > > > > >            goto finish;
> > > > > > >        }
> > > > > > >        visit_end_struct(v, NULL);
> > > > > > >        visit_free(v);
> > > > > > >        QDECREF(compressopts);
> > > > > > >        QDECREF(options)
> > > > > > Looks good.
> > > > > > 
> > > > > > > And I have the following 2 questions:
> > > > > > >     a) I have to specifiy compress.format and compress.level otherwise I will get an error. How can I fix that the settings are optional?
> > > > > > Put an '*' as the first character of any field name if it should be optional.
> > > > > > 
> > > > > > >     b) If I just specify a compress.format can I default the compress.level to 0 without an error?
> > > > > > I believe you'd get compress.level as 0 automatically for an 'int' type.
> > > > > I still face the issue that I now always have to specify a compress.format.
> > > > > I tried to solve it like this:
> > > > [snip]
> > > > 
> > > > that's not needed if you name the parameter '*level' in the QAPI schema
> > > its not needed for the *level, but I still get the error that the format
> > > parameter is missing otherwise. And I can't make format optional.
> > Surely specifying compress.format is how you turn on compression in the
> > first place, so making that optional should not be necessary. eg the
> > simple case becomes:
> > 
> >    qemu-img create -f qcow2 -o compress.format=zlib  foo.qcow2 1G
> > 
> > Yes, there is no notion of a "default" format, but IMHO that's a good
> > thing - same way with encryption - it requires an explict encrypt.format=luks
> > to turn on encryption
> 
> Yes, but visit_type_Qcow2Compress_members always returns an error if compress.format
> is missing from the options. So I should not call it an error out if compress.format is not in the
> options. Thus the check if either compress.format or compress.level is specified at all.

Oh yes, I see what you mean now. Yes, just wrapping the whole qapi block
in an    if(qemu_opt_get(opts, "compress.format"))  is the right way to
handle that.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH V2 5/8] block/qcow2: read and write the compress format extension

[Peter Lieven]

Am 13.07.2017 um 17:17 schrieb Daniel P. Berrange:
> On Thu, Jul 13, 2017 at 05:13:23PM +0200, Peter Lieven wrote:
>> Am 13.07.2017 um 17:06 schrieb Daniel P. Berrange:
>>> On Thu, Jul 13, 2017 at 05:02:51PM +0200, Peter Lieven wrote:
>>>> Am 13.07.2017 um 17:01 schrieb Daniel P. Berrange:
>>>>> On Thu, Jul 13, 2017 at 05:00:39PM +0200, Peter Lieven wrote:
>>>>>> Am 13.07.2017 um 16:58 schrieb Daniel P. Berrange:
>>>>>>> On Thu, Jul 13, 2017 at 04:18:13PM +0200, Peter Lieven wrote:
>>>>>>>> Okay, so it has to be a mix of QAPI parsing and manual parameter checking,
>>>>>>>> right?
>>>>>>> Yeah. It does feel like a valid RFE for QAPI to add a permitted range to
>>>>>>> 'int' types though, which would simplify the code in future.
>>>>>>>
>>>>>>>> I currently have the following:
>>>>>>>>
>>>>>>>>         options = qemu_opts_to_qdict(opts, NULL);
>>>>>>>>         qdict_extract_subqdict(options, &compressopts, "compress.");
>>>>>>>>         v = qobject_input_visitor_new_keyval(QOBJECT(compressopts));
>>>>>>>>         visit_start_struct(v, NULL, NULL, 0, &local_err);
>>>>>>>>         if (local_err) {
>>>>>>>>             ret= -EINVAL;
>>>>>>>>             goto finish;
>>>>>>>>         }
>>>>>>>>         visit_type_Qcow2Compress_members(v, &compress, &local_err);
>>>>>>>>         if (local_err) {
>>>>>>>>             ret= -EINVAL;
>>>>>>>>             goto finish;
>>>>>>>>         }
>>>>>>>>         visit_end_struct(v, NULL);
>>>>>>>>         visit_free(v);
>>>>>>>>         QDECREF(compressopts);
>>>>>>>>         QDECREF(options)
>>>>>>> Looks good.
>>>>>>>
>>>>>>>> And I have the following 2 questions:
>>>>>>>>      a) I have to specifiy compress.format and compress.level otherwise I will get an error. How can I fix that the settings are optional?
>>>>>>> Put an '*' as the first character of any field name if it should be optional.
>>>>>>>
>>>>>>>>      b) If I just specify a compress.format can I default the compress.level to 0 without an error?
>>>>>>> I believe you'd get compress.level as 0 automatically for an 'int' type.
>>>>>> I still face the issue that I now always have to specify a compress.format.
>>>>>> I tried to solve it like this:
>>>>> [snip]
>>>>>
>>>>> that's not needed if you name the parameter '*level' in the QAPI schema
>>>> its not needed for the *level, but I still get the error that the format
>>>> parameter is missing otherwise. And I can't make format optional.
>>> Surely specifying compress.format is how you turn on compression in the
>>> first place, so making that optional should not be necessary. eg the
>>> simple case becomes:
>>>
>>>     qemu-img create -f qcow2 -o compress.format=zlib  foo.qcow2 1G
>>>
>>> Yes, there is no notion of a "default" format, but IMHO that's a good
>>> thing - same way with encryption - it requires an explict encrypt.format=luks
>>> to turn on encryption
>> Yes, but visit_type_Qcow2Compress_members always returns an error if compress.format
>> is missing from the options. So I should not call it an error out if compress.format is not in the
>> options. Thus the check if either compress.format or compress.level is specified at all.
> Oh yes, I see what you mean now. Yes, just wrapping the whole qapi block
> in an    if(qemu_opt_get(opts, "compress.format"))  is the right way to
> handle that.


qemu_opt_get doesn't work, because it seems to return an empty string. I now also always see
compress.format and compress.level in the qemu-img output even if I don't specify it. Have
to figure out why this is...

~/git/qemu$ ./qemu-img create -f qcow2 /tmp/1G.qcow2 1G
Formatting '/tmp/1G.qcow2', fmt=qcow2 size=1073741824 encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16 compress.format= compress.level=0

I hope I have the new patchset ready tomorrow. I would appreciate if you could have a look at it.

Thank you,
Peter

Re: [Qemu-devel] [PATCH V2 5/8] block/qcow2: read and write the compress format extension

[Eric Blake]

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

On 07/13/2017 10:13 AM, Peter Lieven wrote:
>>>>>>
>>>>>>> I currently have the following:
>>>>>>>
>>>>>>>        options = qemu_opts_to_qdict(opts, NULL);
>>>>>>>        qdict_extract_subqdict(options, &compressopts, "compress.");

Can you show your .json patches as well?

> Yes, but visit_type_Qcow2Compress_members always returns an error if
> compress.format
> is missing from the options. So I should not call it an error out if
> compress.format is not in the
> options. Thus the check if either compress.format or compress.level is
> specified at all.

In the JSON, you want '*compress':'CompressUnion' (or some such name)
(overall, the user does not have to specify compression); but within
CompressUnion, you want 'format' to be unconditional (it is the
discriminator), and then branches of the union to add whatever other
fields match the enum members covered by 'format'.

{ 'enum': 'CompressionType', 'data': [ 'gzip', 'lzo' ] }
{ 'union': 'CompressUnion', 'discriminator': 'format',
  'base': { 'format': 'CompressionType', '*level': 'int' },
  'data': { 'gzip': {...any gzip-specific extras}, 'lzo': {...any
lzo-specific extras} } }

[hmm, I think I'm STILL missing my patches to allow for an anonymous
base...  I should rebase and send those; but in the meantime, you'll
have to create a separate 'CompressBase' struct and use
'base':'CompressBase' instead]

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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