[Qemu-block] [PATCH for-2.10] qemu-img: Sort sub-command names in --help

[Eric Blake]

'amend' was the only sub-command not listed alphabetically; hoist
it earlier, and separate the @end table block to make it easier
to copy-and-paste the addition of future sub-commands.

Signed-off-by: Eric Blake 
---
 qemu-img-cmds.hx | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 3763f13625..8bd6f748c3 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -9,6 +9,12 @@ STEXI
 @table @option
 ETEXI

+DEF("amend", img_amend,
+"amend [--object objectdef] [--image-opts] [-p] [-q] [-f fmt] [-t cache] 
-o options filename")
+STEXI
+@item amend [--object @var{objectdef}] [--image-opts] [-p] [-q] [-f @var{fmt}] 
[-t @var{cache}] -o @var{options} @var{filename}
+ETEXI
+
 DEF("bench", img_bench,
 "bench [-c count] [-d depth] [-f fmt] [--flush-interval=flush_interval] 
[-n] [--no-drain] [-o offset] [--pattern=pattern] [-q] [-s buffer_size] [-S 
step_size] [-t cache] [-w] [-U] filename")
 STEXI
@@ -87,9 +93,6 @@ STEXI
 @item resize [--object @var{objectdef}] [--image-opts] [-q] @var{filename} [+ 
| -]@var{size}
 ETEXI

-DEF("amend", img_amend,
-"amend [--object objectdef] [--image-opts] [-p] [-q] [-f fmt] [-t cache] 
-o options filename")
 STEXI
-@item amend [--object @var{objectdef}] [--image-opts] [-p] [-q] [-f @var{fmt}] 
[-t @var{cache}] -o @var{options} @var{filename}
 @end table
 ETEXI
-- 
2.13.3

Re: [Qemu-block] [PATCH V5 10/10] block/qcow2: add compress info to image specific info

[Eric Blake]

On 07/25/2017 09:41 AM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven 
> ---
>  block/qcow2.c| 7 +++
>  qapi/block-core.json | 6 +-
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> +++ b/qapi/block-core.json
> @@ -68,6 +68,9 @@
>  # @encrypt: details about encryption parameters; only set if image
>  #   is encrypted (since 2.10)
>  #
> +# @compress: details about parameters for compressed clusters; only set if
> +#the compress format header extension is present (since 2.11)

Thinking aloud:

I still think this is better as "only set if the image uses compressed
headers" (similar to encrypt).  That is, I would like this output to be
present even when opening legacy deflate/0/12 images.

But thinking more, what I am REALLY asking for is "if any cluster is
compressed, then this information is present; if nothing is compressed,
the choice of compression header doesn't matter".  But we don't have a
quick-and-dirty way to tell if an image has any compressed clusters,
short of examining every L2 map (including maps inside internal snapshots).

Hmm - we already have 'Dirty bit' and 'Corrupt bit' as
incompatible_features that can quickly identify certain image
properties; do we want another bit set to 1 for images known to use
compressed clusters?  Or even make it an auto-clear bit (or even a pair
of auto-clear bits: one to designate that this image was written by a
new-enough qemu that promises to mark the image if any compressed
clusters exist, and the other is set only if such clusters do exist;
that way, if both bits are clear, we know we have to walk L2 tables to
look for compressed clusters, but if the first bit is set, then the
second bit is reliable)?

So maybe this means we are still debating spec ideas.  Kevin, do you
want to chime in?

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH V5 09/10] block/qcow2: add lzo compress format

[Eric Blake]

On 07/25/2017 09:41 AM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven 
> ---
>  block/qcow2-cluster.c  | 15 +++
>  block/qcow2.c  | 25 -
>  configure  |  2 +-
>  docs/interop/qcow2.txt |  2 ++
>  qapi/block-core.json   | 15 ---
>  qemu-img.texi  |  1 +
>  6 files changed, 55 insertions(+), 5 deletions(-)
> 

> +++ b/docs/interop/qcow2.txt
> @@ -335,6 +335,8 @@ The fields of the compress format extension are:
> deflate: Standard zlib deflate compression without
>  compression header
>  
> +   lzo: LZO1X compression without additional header
> +
>14:  compress_level (uint8_t)
>  
> 0 = default compress level (valid for all formats, 
> default)

What do compress_level and compress_window have to be for lzo?  (If they
are not useful, document that it must be 0 - which you've already done
for compress_level).


> +# @Qcow2CompressLZO:
> +#
> +# Since: 2.10

2.11

> +##
> +{ 'struct': 'Qcow2CompressLZO',
> +  'data': { } }
> +
> +##
>  # @Qcow2Compress:
>  #
>  # Specifies the compression format and compression level that should
> @@ -2491,8 +2500,8 @@
>  { 'union': 'Qcow2Compress',
>'base': { 'format': 'Qcow2CompressFormat' },
>'discriminator': 'format',
> -  'data': { 'deflate': 'Qcow2CompressDeflate' } }
> -
> +  'data': { 'deflate': 'Qcow2CompressDeflate',
> +'lzo': 'Qcow2CompressLZO' } }
>  

Was the loss of the blank line intentional?

> +++ b/qemu-img.texi
> @@ -682,6 +682,7 @@ The following options are available if support for the 
> respective libraries
>  has been enabled at compile time:
>  
> deflateUses standard zlib defalte compression
> +   lzoUses LZO1X compression

The .json file spelled it lzo1x; is there a difference implied by the
choice of capitalization?

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 1/2] dirty-bitmap: Report BlockDirtyInfo.count in bytes, as documented

[Eric Blake]

On 07/25/2017 04:28 PM, John Snow wrote:
> 
> 
> On 07/21/2017 02:32 PM, Eric Blake wrote:
>> We've been documenting the value in bytes since its introduction
>> in commit b9a9b3a4 (v1.3), where it was actually reported in bytes.
>>
>> Commit e4654d2 (v2.0) then removed things from block/qapi.c, in
>> preparation for a rewrite to a list of dirty sectors in the next
>> commit 21b5683 in block.c, but the new code mistakenly started
>> reporting in sectors.
>>
>> Fixes: https://bugzilla.redhat.com/1441460
>>
>> CC: qemu-sta...@nongnu.org
>> Signed-off-by: Eric Blake 
>> Reviewed-by: John Snow 
>>
> 
> I must have forgotten about this -- Didn't this get reviewed as part of
> your byte series? Is this a resend for a stable branch?

Yes, it was reviewed during my part 2-of-4 series on byte-base block
status (the dirty bitmap series).  Series 1 made it into 2.10, but
series 2-4 did not; ergo, I filtered out the true bug-fixes that were
still worthy post-freeze.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH V5 04/10] qemu-img: add documentation for compress settings

[Eric Blake]

On 07/25/2017 09:41 AM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven 
> ---
>  qemu-img.texi | 26 ++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/qemu-img.texi b/qemu-img.texi
> index 72dabd6..3612c59 100644
> --- a/qemu-img.texi
> +++ b/qemu-img.texi
> @@ -676,6 +676,32 @@ 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:
> +
> +   deflateUses standard zlib defalte compression
> +
> +The compression algorithm can only be defined at image create time and cannot
> +be changed later.

We can't change it via 'qemu-img amend'?  Might be an interesting
feature to add later: in-place re-compression to a new algorithm - but
since compression is global to the file, you have to be careful that
aborting the operation in the middle doesn't mix the two compressions.
Or put another way, we have to use at least twice the disk space for all
compressed clusters, and only at the end of things update the headers to
point to the new clusters and away from the old (so that an early abort
just treats all the new clusters as leaked); possibly by amending from
compressed to uncompressed back to compressed.

Oh, and why is 'qemu-img --help' mostly alphabetical, except that
'amend' comes after 'resize'?  Guess I'll do a quickie patch for that.


> +
> +Note: defining compression format settings which are different from the old
> +  default (format=deflate, level=0, window-size=12) will result in the
> +  compression format extension being written to the Qcow2 image. Versions
> +  of QEMU before 2.11 will not be able to open images with this 
> extension.
> +
> +@item compress.level
> +Valid for compress.format=deflate defines the compression level to use for
> +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 9 for the best compression.
> +
> +@item compress.window-size
> +Valid for compress.format=deflate defines the compression window size used
> +during compression. Valid window sizes for deflate compression range from 8 
> to
> +15 inclusively. The default is @code{compress.window-size=15}.

Maybe emphasize that larger is generally better (smaller exists for the
long-gone days of small machines, and for back-compat usage of 12 in
older qemu).  In fact, if we wanted, we could declare that qcow2's use
of deflate can ONLY use 12 or 15, instead of anything in the range 8-15,
if we though that would be easier to maintain.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH V5 03/10] block/qcow2: parse compress create options

[Eric Blake]

On 07/25/2017 09:41 AM, 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 
> ---
>  block/qcow2.c | 86 
> +--
>  include/block/block_int.h | 39 +++--
>  2 files changed, 105 insertions(+), 20 deletions(-)
> 

> +static void
> +qcow2_check_compress_settings(Qcow2Compress *compress, Error **errp)
> +{
> +if (compress->format == QCOW2_COMPRESS_FORMAT_DEFLATE) {
> +if (!compress->u.deflate.has_level) {
> +compress->u.deflate.has_level = true;
> +compress->u.deflate.level = 0;
> +}
> +if (compress->u.deflate.level > 9) {
> +error_setg(errp, "Compress level %" PRIu8 " is not supported for"
> +   " format '%s'", compress->u.deflate.level,
> +   Qcow2CompressFormat_lookup[compress->format]);
> +return;
> +}
> +if (!compress->u.deflate.has_window_size) {
> +compress->u.deflate.has_window_size = true;
> +compress->u.deflate.window_size = 15;

Maybe for 2.11 we should finally get around to adding parameter defaults
directly in QMP (you would then not need a has_FOO for any parameter FOO
that can specify its own default).  Not your problem to solve, though.

> +}
> +if (compress->u.deflate.window_size < 8 ||
> +compress->u.deflate.window_size > 15) {
> +error_setg(errp, "Compress window size %" PRIu8 " is not 
> supported"
> +   " for format '%s'", compress->u.deflate.window_size,
> +   Qcow2CompressFormat_lookup[compress->format]);

Should we allow a window_size of 0 in the UI (meaning pick a sane
non-zero default)?  It's odd that you allow 0 for level but not for
window-size.

> +return;
> +}
> +}
> +}

Dead return statement, since the end of the function occurs anyways.
But arguably it helps future maintenance if more tests were to be added
later?

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 1/2] dirty-bitmap: Report BlockDirtyInfo.count in bytes, as documented

[John Snow]

On 07/21/2017 02:32 PM, Eric Blake wrote:

We've been documenting the value in bytes since its introduction
in commit b9a9b3a4 (v1.3), where it was actually reported in bytes.

Commit e4654d2 (v2.0) then removed things from block/qapi.c, in
preparation for a rewrite to a list of dirty sectors in the next
commit 21b5683 in block.c, but the new code mistakenly started
reporting in sectors.

Fixes: https://bugzilla.redhat.com/1441460

CC: qemu-sta...@nongnu.org
Signed-off-by: Eric Blake 
Reviewed-by: John Snow 



I must have forgotten about this -- Didn't this get reviewed as part of 
your byte series? Is this a resend for a stable branch?



---
Too late for 2.9, since the regression has been unnoticed for
nine releases. But worth putting in 2.9.1 and 2.10.

v2-v4: no change
---
  block/dirty-bitmap.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 543bddb9b5..30462d4f9a 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -461,7 +461,7 @@ BlockDirtyInfoList 
*bdrv_query_dirty_bitmaps(BlockDriverState *bs)
  QLIST_FOREACH(bm, >dirty_bitmaps, list) {
  BlockDirtyInfo *info = g_new0(BlockDirtyInfo, 1);
  BlockDirtyInfoList *entry = g_new0(BlockDirtyInfoList, 1);
-info->count = bdrv_get_dirty_count(bm);
+info->count = bdrv_get_dirty_count(bm) << BDRV_SECTOR_BITS;
  info->granularity = bdrv_dirty_bitmap_granularity(bm);
  info->has_name = !!bm->name;
  info->name = g_strdup(bm->name);



--
—js

[Qemu-block] [PATCH v3 09/12] tests/libqos/pci: Clean up string interpolation into QMP input

[Eric Blake]

From: Markus Armbruster 

Leaving interpolation into JSON to qmp() is more robust than building
QMP input manually, as explained in the commit before previous.

The case in qpci_plug_device_test() is a bit complicated: it
interpolates several JSON object members, not just a value.  Clean it
up by passing them in as QDict rather than string, so we can leave
interpolation to qmp() here and to qobject_from_jsonf() in callers.

Signed-off-by: Markus Armbruster 
Message-Id: <1500645206-13559-7-git-send-email-arm...@redhat.com>
[use qmp_cmd for a slightly smaller diff]
Signed-off-by: Eric Blake 
---
 tests/libqos/pci.h  |  2 +-
 tests/ivshmem-test.c| 10 +-
 tests/libqos/pci.c  | 32 +---
 tests/virtio-blk-test.c |  5 -
 4 files changed, 27 insertions(+), 22 deletions(-)

diff --git a/tests/libqos/pci.h b/tests/libqos/pci.h
index ed480614ff..c981061703 100644
--- a/tests/libqos/pci.h
+++ b/tests/libqos/pci.h
@@ -109,6 +109,6 @@ void qpci_iounmap(QPCIDevice *dev, QPCIBar addr);
 QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr);

 void qpci_plug_device_test(const char *driver, const char *id,
-   uint8_t slot, const char *opts);
+   uint8_t slot, QDict *extra_args);
 void qpci_unplug_acpi_device_test(const char *id, uint8_t slot);
 #endif
diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c
index 37763425ee..38044bb01c 100644
--- a/tests/ivshmem-test.c
+++ b/tests/ivshmem-test.c
@@ -14,6 +14,7 @@
 #include "libqos/libqos-pc.h"
 #include "libqos/libqos-spapr.h"
 #include "libqtest.h"
+#include "qapi/qmp/qjson.h"
 #include "qemu-common.h"

 #define TMPSHMSIZE (1 << 20)
@@ -419,19 +420,18 @@ static void test_ivshmem_server_irq(void)
 static void test_ivshmem_hotplug(void)
 {
 const char *arch = qtest_get_arch();
-gchar *opts;
+QObject *extra_args = qobject_from_jsonf("{ 'shm': '%s', 'size': '1M' }",
+ tmpshm);

 qtest_start("");

-opts = g_strdup_printf("'shm': '%s', 'size': '1M'", tmpshm);
-
-qpci_plug_device_test("ivshmem", "iv1", PCI_SLOT_HP, opts);
+qpci_plug_device_test("ivshmem", "iv1", PCI_SLOT_HP,
+  qobject_to_qdict(extra_args));
 if (strcmp(arch, "ppc64") != 0) {
 qpci_unplug_acpi_device_test("iv1", PCI_SLOT_HP);
 }

 qtest_end();
-g_free(opts);
 }

 static void test_ivshmem_memdev(void)
diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
index 2dcdeade2a..2754412340 100644
--- a/tests/libqos/pci.c
+++ b/tests/libqos/pci.c
@@ -14,6 +14,7 @@
 #include "libqos/pci.h"

 #include "hw/pci/pci_regs.h"
+#include "qapi/qmp/qjson.h"
 #include "qemu/host-utils.h"

 void qpci_device_foreach(QPCIBus *bus, int vendor_id, int device_id,
@@ -392,22 +393,23 @@ QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr)
 }

 void qpci_plug_device_test(const char *driver, const char *id,
-   uint8_t slot, const char *opts)
+   uint8_t slot, QDict *extra_args)
 {
-QDict *response;
-char *cmd;
-
-cmd = g_strdup_printf("{'execute': 'device_add',"
-  " 'arguments': {"
-  "   'driver': '%s',"
-  "   'addr': '%d',"
-  "   %s%s"
-  "   'id': '%s'"
-  "}}", driver, slot,
-  opts ? opts : "", opts ? "," : "",
-  id);
-response = qmp(cmd);
-g_free(cmd);
+char addr[8];
+QDict *args, *response;
+
+sprintf(addr, "%d", slot);
+args = qobject_to_qdict(
+qobject_from_jsonf("{ 'driver': %s, 'addr': %s, 'id': %s}",
+   driver, addr, id));
+
+if (extra_args) {
+qdict_join(args, extra_args, true);
+QDECREF(extra_args);
+}
+
+response = qmp_cmd("device_add", QOBJECT(args));
+
 g_assert(response);
 g_assert(!qdict_haskey(response, "error"));
 QDECREF(response);
diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index 0576cb16ba..64a48f40b2 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -16,6 +16,7 @@
 #include "libqos/virtio-pci.h"
 #include "libqos/virtio-mmio.h"
 #include "libqos/malloc-generic.h"
+#include "qapi/qmp/qjson.h"
 #include "qemu/bswap.h"
 #include "standard-headers/linux/virtio_ids.h"
 #include "standard-headers/linux/virtio_config.h"
@@ -658,12 +659,13 @@ static void pci_hotplug(void)
 QVirtioPCIDevice *dev;
 QOSState *qs;
 const char *arch = qtest_get_arch();
+QObject *extra_args = qobject_from_jsonf("{ 'drive': 'drive1' }");

 qs = pci_test_start();

 /* plug secondary disk */
 qpci_plug_device_test("virtio-blk-pci", "drv1", PCI_SLOT_HP,
-  "'drive': 'drive1'");
+  qobject_to_qdict(extra_args));

Re: [Qemu-block] [PATCH V5 02/10] qapi/block-core: add Qcow2Compress parameters

[Eric Blake]

On 07/25/2017 09:41 AM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven 
> ---
>  qapi/block-core.json | 40 
>  1 file changed, 40 insertions(+)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 833c602..f652206 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2455,6 +2455,46 @@
>  '*encrypt': 'BlockdevQcow2Encryption' } }
>  
>  ##
> +# @Qcow2CompressFormat:
> +#
> +# @deflate: standard zlib deflate compression
> +#
> +# Since: 2.11
> +##
> +{ 'enum': 'Qcow2CompressFormat',
> +  'data': [ 'deflate' ] }
> +
> +##
> +# @Qcow2CompressDeflate:
> +#
> +# @level: specifies the compression level. 0 = default compression,
> +# 1 = fastest compression, 9 = best compresion

By putting level here instead of in the common base type, you'll have to
re-implement it for each compression type - but that's also okay since
they (might) have different ranges of valid levels.

s/compresion/compression/

maybe: 0 = default compression (same as level 6), 1=...


> +# @window-size: specifies the window size used for deflate compression.
> +#   8...15 = window size of 2^8 to 2^15 byte (default)

15 is the new default, but it might be worth an explicit mention that a
window of 12 is required for back-compat to older images.

> +#
> +# Since: 2.11
> +##
> +{ 'struct': 'Qcow2CompressDeflate',
> +  'data': { '*level': 'uint8',
> +'*window-size': 'uint8' } }
> +
> +##
> +# @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)

s/zlib/deflate/

> +#
> +# Since: 2.11
> +##
> +{ 'union': 'Qcow2Compress',
> +  'base': { 'format': 'Qcow2CompressFormat' },
> +  'discriminator': 'format',
> +  'data': { 'deflate': 'Qcow2CompressDeflate' } }
> +
> +
> +##
>  # @BlockdevOptionsSsh:
>  #
>  # @server:  host address
> 

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



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH v3 08/12] qtests: convert tests to use qmp_cmd

[Eric Blake]

Now that we have the qmp_cmd() helper, we can further simplify
some of the tests by using it.

Signed-off-by: Eric Blake 
---
 tests/device-introspect-test.c |  3 +--
 tests/ide-test.c   |  2 +-
 tests/libqos/libqos.c  |  5 +++--
 tests/libqos/pci-pc.c  |  4 ++--
 tests/libqos/usb.c | 18 +-
 tests/pc-cpu-test.c| 10 +-
 tests/postcopy-test.c  |  9 +
 tests/vhost-user-test.c| 12 ++--
 8 files changed, 32 insertions(+), 31 deletions(-)

diff --git a/tests/device-introspect-test.c b/tests/device-introspect-test.c
index f7162c023f..fc6d559e14 100644
--- a/tests/device-introspect-test.c
+++ b/tests/device-introspect-test.c
@@ -36,8 +36,7 @@ static QList *qom_list_types(const char *implements, bool 
abstract)
 if (implements) {
 qdict_put_str(args, "implements", implements);
 }
-resp = qmp("{'execute': 'qom-list-types',"
-   " 'arguments': %p }", args);
+resp = qmp_cmd("qom-list-types", QOBJECT(args));
 g_assert(qdict_haskey(resp, "return"));
 ret = qdict_get_qlist(resp, "return");
 QINCREF(ret);
diff --git a/tests/ide-test.c b/tests/ide-test.c
index ea2657d3d1..75dc472e6a 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -651,7 +651,7 @@ static void test_retry_flush(const char *machine)
 qmp_eventwait("STOP");

 /* Complete the command */
-qmp_discard_response("{'execute':'cont' }");
+qmp_cmd_discard_response("cont", NULL);

 /* Check registers */
 data = qpci_io_readb(dev, ide_bar, reg_device);
diff --git a/tests/libqos/libqos.c b/tests/libqos/libqos.c
index 42c5315423..18844617ae 100644
--- a/tests/libqos/libqos.c
+++ b/tests/libqos/libqos.c
@@ -4,6 +4,7 @@
 #include "libqtest.h"
 #include "libqos/libqos.h"
 #include "libqos/pci.h"
+#include "qapi/qmp/qjson.h"

 /*** Test Setup & Teardown ***/

@@ -86,7 +87,7 @@ void set_context(QOSState *s)

 static QDict *qmp_execute(const char *command)
 {
-return qmp("{ 'execute': %s }", command);
+return qmp_cmd(command, NULL);
 }

 void migrate(QOSState *from, QOSState *to, const char *uri)
@@ -106,7 +107,7 @@ void migrate(QOSState *from, QOSState *to, const char *uri)
 QDECREF(rsp);

 /* Issue the migrate command. */
-rsp = qmp("{ 'execute': 'migrate', 'arguments': { 'uri': %s } }", uri);
+rsp = qmp_cmd("migrate", qobject_from_jsonf("{ 'uri': %s }", uri));
 g_assert(qdict_haskey(rsp, "return"));
 QDECREF(rsp);

diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
index d40aa9dffd..8494671290 100644
--- a/tests/libqos/pci-pc.c
+++ b/tests/libqos/pci-pc.c
@@ -17,6 +17,7 @@
 #include "hw/pci/pci_regs.h"

 #include "qemu-common.h"
+#include "qapi/qmp/qjson.h"


 #define ACPI_PCIHP_ADDR 0xae00
@@ -160,8 +161,7 @@ void qpci_unplug_acpi_device_test(const char *id, uint8_t 
slot)
 {
 QDict *response;

-response = qmp("{'execute': 'device_del', 'arguments': {'id': %s}}",
-   id);
+response = qmp_cmd("device_del", qobject_from_jsonf("{'id': %s}", id));
 g_assert(response);
 g_assert(!qdict_haskey(response, "error"));
 QDECREF(response);
diff --git a/tests/libqos/usb.c b/tests/libqos/usb.c
index f88d4a6a3a..a96f5ebd6a 100644
--- a/tests/libqos/usb.c
+++ b/tests/libqos/usb.c
@@ -15,6 +15,7 @@
 #include "libqtest.h"
 #include "hw/usb/uhci-regs.h"
 #include "libqos/usb.h"
+#include "qapi/qmp/qjson.h"

 void qusb_pci_init_one(QPCIBus *pcibus, struct qhc *hc, uint32_t devfn, int 
bar)
 {
@@ -46,13 +47,13 @@ void usb_test_hotplug(const char *hcd_id, const int port,

 sprintf(id, "usbdev%d", port);
 bus = g_strdup_printf("%s.0", hcd_id);
-response = qmp("{'execute': 'device_add',"
-   " 'arguments': {"
-   "   'driver': 'usb-tablet',"
-   "   'port': %s,"
-   "   'bus': %s,"
-   "   'id': %s"
-   " }}", id + 6, bus, id);
+response = qmp_cmd("device_add",
+   qobject_from_jsonf("{"
+  "   'driver': 'usb-tablet',"
+  "   'port': %s,"
+  "   'bus': %s,"
+  "   'id': %s"
+  "}", id + 6, bus, id));
 g_free(bus);
 g_assert(response);
 g_assert(!qdict_haskey(response, "error"));
@@ -62,8 +63,7 @@ void usb_test_hotplug(const char *hcd_id, const int port,
 port_check();
 }

-response = qmp("{'execute': 'device_del', 'arguments': { 'id': %s }}",
-   id);
+response = qmp_cmd("device_del", qobject_from_jsonf("{ 'id': %s }", id));
 g_assert(response);
 g_assert(qdict_haskey(response, "event"));
 g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
diff --git a/tests/pc-cpu-test.c b/tests/pc-cpu-test.c
index 

[Qemu-block] [PATCH v3 04/12] tests: Pass literal format strings directly to qmp_FOO()

[Eric Blake]

From: Markus Armbruster 

The qmp_FOO() take a printf-like format string.  In a few places, we
assign a string literal to a variable and pass that instead of simply
passing the literal.  Clean that up.

Bonus: gets rid of non-literal format strings.  A step towards
compile-time format string checking without triggering
-Wformat-nonliteral.

Signed-off-by: Markus Armbruster 
Message-Id: <1500645206-13559-4-git-send-email-arm...@redhat.com>
Signed-off-by: Eric Blake 
Reviewed-by: Stefan Hajnoczi 
---
 tests/ahci-test.c | 4 +---
 tests/ide-test.c  | 4 +---
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 999121bb7c..97622e0044 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -1350,7 +1350,6 @@ static void test_flush_migrate(void)
 AHCIQState *src, *dst;
 AHCICommand *cmd;
 uint8_t px;
-const char *s;
 char *uri = g_strdup_printf("unix:%s", mig_socket);

 prepare_blkdebug_script(debug_path, "flush_to_disk");
@@ -1386,8 +1385,7 @@ static void test_flush_migrate(void)
 ahci_migrate(src, dst, uri);

 /* Complete the command */
-s = "{'execute':'cont' }";
-qmp_async(s);
+qmp_async("{'execute':'cont' }");
 qmp_eventwait("RESUME");
 ahci_command_wait(dst, cmd);
 ahci_command_verify(dst, cmd);
diff --git a/tests/ide-test.c b/tests/ide-test.c
index bfd79ddbdc..ea2657d3d1 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -624,7 +624,6 @@ static void test_retry_flush(const char *machine)
 QPCIDevice *dev;
 QPCIBar bmdma_bar, ide_bar;
 uint8_t data;
-const char *s;

 prepare_blkdebug_script(debug_path, "flush_to_disk");

@@ -652,8 +651,7 @@ static void test_retry_flush(const char *machine)
 qmp_eventwait("STOP");

 /* Complete the command */
-s = "{'execute':'cont' }";
-qmp_discard_response(s);
+qmp_discard_response("{'execute':'cont' }");

 /* Check registers */
 data = qpci_io_readb(dev, ide_bar, reg_device);
-- 
2.13.3

Re: [Qemu-block] [PATCH V5 01/10] specs/qcow2: add compress format extension

[Eric Blake]

On 07/25/2017 03:29 PM, Peter Lieven wrote:

>>> +   deflate: Standard zlib deflate compression without
>>> +compression header
>> Why did you name it "deflate" instead of "zlib" again?
> 
> zlib provides raw deflate encoding and gzip encoding both with and without 
> header afaik.
> We use raw deflate encoding without header in QEMU. And the name of the 
> algorithm is deflate.
> I found it more appropriate than zlib.

Okay, I can live with that.

> 
>>
>>> +
>>> +  14:  compress_level (uint8_t)
>>> +
>>> +   0 = default compress level (valid for all formats, 
>>> default)
>>> +
>>> +   Additional valid compression levels for deflate 
>>> compression:
>>> +
>>> +   All values between 1 and 9. 1 gives best speed, 9 gives 
>>> best
>>> +   compression. The default compression level is defined 
>>> by zlib
>>> +   and currently defaults to 6.
>>> +
>>> +  15:  compress_window_size (uint8_t)
>>> +
>>> +   Window or dictionary size used by the compression 
>>> format.
>>> +   Currently only used by the deflate compression 
>>> algorithm.
>> What must this be set to for other algorithms?  I guess we get to that
>> in later patches.
> 
> Its only used by the deflate algorithm. At this point we only have that 
> algorithm.
> lzo has no such encoding. If we add e.g. lzma it has a dictionary size which 
> is
> a power of two (typcial value would be 2^20).

So for lzo, we should document that the field must be 0 (when we get to
adding it).

> 
> Can you also have a look at the remainder of the series?

It's on my list for 2.11 reviews, although 2.10 freeze stuff is taking
priority, so it may be a few days yet.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH V5 01/10] specs/qcow2: add compress format extension

[Peter Lieven]

Am 25.07.2017 um 17:03 schrieb Eric Blake:
> On 07/25/2017 09:41 AM, Peter Lieven wrote:
>> Signed-off-by: Peter Lieven 
>> ---
>>  docs/interop/qcow2.txt | 51 
>> +-
>>  roms/ipxe  |  2 +-
>>  2 files changed, 51 insertions(+), 2 deletions(-)
>>
>> +
>> +== Compress format extension ==
>> +
>> +The compress format extension is an optional header extension. It provides
>> +the ability to specify the compress algorithm and compress parameters
> s/the compress algorithm/the compression algorithm/
>
>> +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 - 13:  compress_format_name (padded with zeros, but not
>> +   necessarily null terminated if it has full length).
>> +   Valid compression format names currently are:
>> +
>> +   deflate: Standard zlib deflate compression without
>> +compression header
> Why did you name it "deflate" instead of "zlib" again?

zlib provides raw deflate encoding and gzip encoding both with and without 
header afaik.
We use raw deflate encoding without header in QEMU. And the name of the 
algorithm is deflate.
I found it more appropriate than zlib.

>
>> +
>> +  14:  compress_level (uint8_t)
>> +
>> +   0 = default compress level (valid for all formats, 
>> default)
>> +
>> +   Additional valid compression levels for deflate 
>> compression:
>> +
>> +   All values between 1 and 9. 1 gives best speed, 9 gives 
>> best
>> +   compression. The default compression level is defined by 
>> zlib
>> +   and currently defaults to 6.
>> +
>> +  15:  compress_window_size (uint8_t)
>> +
>> +   Window or dictionary size used by the compression format.
>> +   Currently only used by the deflate compression algorithm.
> What must this be set to for other algorithms?  I guess we get to that
> in later patches.

Its only used by the deflate algorithm. At this point we only have that 
algorithm.
lzo has no such encoding. If we add e.g. lzma it has a dictionary size which is
a power of two (typcial value would be 2^20).

>
>> +
>> +   Valid window sizes for deflate compression range from 8 
>> to
>> +   15 inclusively.
>> +
>> +Note: Omitting the incompatible "Compress format bit" results in the usage
>> +of deflate compression with default compression level and a window size of 
>> 12
>> +(which was default before QEMU 2.11). If exactly these parameters are 
>> choosen
> s/choosen/chosen,/
>
>> +it is free to the implementation to omit the "Compress format bit" and the
> s/it is free to the implementation to omit/the implementation may omit/
>
>> +compress format extension when updating the QCOW2 header.
>> +
>> +
>>  == Host cluster management ==
>>  
>>  qcow2 manages the allocation of host clusters by maintaining a reference 
>> count
>> diff --git a/roms/ipxe b/roms/ipxe
>> index 0600d3a..b991c67 16
>> --- a/roms/ipxe
>> +++ b/roms/ipxe
>> @@ -1 +1 @@
>> -Subproject commit 0600d3ae94f93efd10fc6b3c7420a9557a3a1670
>> +Subproject commit b991c67c1d91574ef22336cc3a5944d1e63230c9
> Oops.

Yes, oops. But I guess I have to respin anyway ;-)

Can you also have a look at the remainder of the series?

Thanks,
Peter

Re: [Qemu-block] [PATCH 2/2] iotests: Redirect stderr to stdout in 186

[Eric Blake]

On 07/25/2017 10:56 AM, Max Reitz wrote:
> Without redirecting qemu's stderr to stdout, _filter_qemu will not apply
> to warnings.  This results in $QEMU_PROG not being replaced by QEMU_PROG
> which is not great if your qemu executable is not called
> qemu-system-x86_64 (e.g. qemu-system-i386).
> 
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/186 |  2 +-
>  tests/qemu-iotests/186.out | 12 ++--
>  2 files changed, 7 insertions(+), 7 deletions(-)

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 1/2] iotests: Fix test 156

[Eric Blake]

On 07/25/2017 10:56 AM, Max Reitz wrote:
> On one hand, the _make_test_img invocation for creating the target image
> was missing a -u because its backing file is not supposed to exist at
> that point.
> 
> On the other hand, nobody noticed probably because the backing file is
> created later on and _cleanup failed to remove it: The quotation marks
> were misplaced so bash tried to deleted a file literally called

s/deleted/delete/

> "$TEST_IMG{,.target}..." instead of resolving the globs.  Thus, the

Technically brace expansion, not globs.

> files stayed around after the first run and qemu-img create did not
> complain about a missing backing file on any run but the first.
> 
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/156 | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH 0/3] build configuration query tool and conditional (qemu-io)test skip

[Cleber Rosa]

On 07/25/2017 12:24 PM, Daniel P. Berrange wrote:
>>
>> OK, let's abstract a bit more.  Let's take this part of your statement:
>>
>>  "if qemu-io in this environment cannot do aio=native"
>>
>> Let's call that a feature check.  Depending on how the *feature check*
>> is written, a negative result may hide a test failure, because it would
>> now be skipped.
>>
>> Suppose that a feature check for "SDL display" is such that you run
>> "qemu -display sdl".  A *feature failure* here (SDL init is broken), or
>> an environment issue (DISPLAY=), will cause a SDL test skip.
> 
> You could have a way to statically define what features any given run
> of the test suite should enable, then report failure if they were not
> detected.
> 

You hit a key point here: statically define(d).  As a said before,
feature statements are a safer place upon which to base tests.  Ad hoc
checks, as suggested by Stefan, are definitely not.

> This is a similar situation to that seen with configure scripts. If invoked
> with no --enable-xxx flags, it will probe for features & enable them if
> found.  This means you can accidentally build without expected features if
> you have a missing -devel package, or a header/library is broken in some
> way. This is why configure prints a summary of which features it actually
> found. It is also why when building binary packages like RPMs, it is common
> to explicitly give --enable-xxx flags for all features you expect to see.
> Automatic enablement is none the less still useful for people in general.
> 
> So if we applied this kind of approach for testing, then any automated
> test systems at least, ought to provide a fixed list of features they
> expect to be present for tests. So if any features accidentally broke
> the tests would error.
> 
> Regards,
> Daniel
> 

Right.  The key question here seems to be the distance of the "fixed
list of features" from the test itself.  For instance, think of this
workflow/approach:

 1) ./scripts/configured-features-to-feature-list.sh > ~/feature_list
 2) tweak feature_list
 3) ./rpm -e SDL-devel
 4) ./configure  --enable-sdl
 5) ./make
 6) ./scripts/run-test-suite.sh --only-features=~/feature_list

This would only run tests that are expected to PASS within the given
feature list.  The test runner (run-test-suite.sh) would select only
tests that match the features given.  No SKIPs would be expected as the
outcome of *any test*.

The other approach is to let the feature match to the test, and SKIPs
would then be OK.  The downside to this is that a "--enable-xxx" with
missing "-devel" package, as you exemplified, would not show up as ERRORs.

Makes sense?

Regards!

-- 
Cleber Rosa
[ Sr Software Engineer - Virtualization Team - Red Hat ]
[ Avocado Test Framework - avocado-framework.github.io ]
[  7ABB 96EB 8B46 B94D 5E0F  E9BB 657E 8D33 A5F2 09F3  ]



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v2] qemu-iotests: add a "how to" to ./README

[John Snow]

On 07/25/2017 11:36 AM, Stefan Hajnoczi wrote:

There is not much getting started documentation for qemu-iotests.  This
patch explains how to create a new test and covers the overall testing
approach.

Cc: Ishani Chugh 
Reviewed-by: Eric Blake 
Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Stefan Hajnoczi 
---
v2:
  * Added missing "touch .out" step [Kevin]
  * Added reference to SubmitAPatch wiki page [Eric & Philippe]
---
  tests/qemu-iotests/README | 94 +++
  1 file changed, 94 insertions(+)

diff --git a/tests/qemu-iotests/README b/tests/qemu-iotests/README
index 6079b401ae..245f509b14 100644
--- a/tests/qemu-iotests/README
+++ b/tests/qemu-iotests/README
@@ -14,8 +14,102 @@ Just run ./check to run all tests for the raw image format, 
or ./check
  -qcow2 to test the qcow2 image format.  The output of ./check -h explains
  additional options to test further image formats or I/O methods.
  
+* Testing approach

+
+Each test is an executable file (usually a bash script) that is run by the
+./check test harness.  Standard out and standard error are captured to an
+output file.  If the output file differs from the "golden master" output file
+for the test then it fails.
+
+Tests are simply a sequence of commands that produce output and the test itself
+does not judge whether it passed or failed.  If you find yourself writing
+checks to determine success or failure then you should rethink the test and
+rely on output diffing instead.
+
+** Filtering volatile output
+
+When output contains absolute file paths, timestamps, process IDs, hostnames,
+or other volatile strings, the diff against golden master output will fail.
+Such output must be filtered to replace volatile strings with fixed
+placeholders.
+
+For example, the path to the temporary working directory changes between test
+runs so it must be filtered:
+
+  sed -e "s#$TEST_DIR/#TEST_DIR/#g"
+
+Commonly needed filters are available in ./common.filter.
+
+** Python tests
+
+Most tests are implemented in bash but it is difficult to interact with the QMP
+monitor.  A Python module called 'iotests' is available for tests that require
+JSON and interacting with QEMU.
+
+* How to create a test
+
+1. Choose an unused test number
+
+Tests are identified by a unique number.  Look for the highest test case number
+by looking at the test files.  Then search the qemu-de...@nongnu.org mailing
+list to check if anyone has already sent patches using the next available
+number.  You may need to increment the number a few times to reach an unused
+number.
+
+2. Create the test file
+
+Copy an existing test (one that most closely resembles what you wish to test)
+to the new test number:
+
+  cp 001 
+
+3. Assign groups to the test
+
+Add your test to the ./group file.  This file is the index of tests and assigns
+them to functional groups like "rw" for read-write tests.  Most tests belong to
+the "rw" and "auto" groups.  "auto" means the test runs when ./check is invoked
+without a -g argument.
+
+Consider adding your test to the "quick" group if it executes quickly (<1s).
+This group is run by "make check-block" and is often included as part of build
+tests in continuous integration systems.
+
+4. Write the test
+
+Edit the test script.  Look at existing tests for examples.
+
+5. Generate the golden master file
+
+Create an empty golden master file:
+
+  touch .out
+
+Then run your test:
+
+  ./check 
+
+You may need additional command-line options to use an image format or
+protocol like -qcow2.
+
+The test will fail because there is no golden master yet.  Inspect the output
+that your test generated with "cat .out.bad".
+
+Verify that the output is as expected and contains no volatile strings like
+timestamps.  You may need to add filters to your test to remove volatile
+strings.
+
+Once you are happy with the test output it can be used as the golden master
+with "mv .out.bad .out".  Rerun the test to verify
+that it passes.
+
+Congratulations, you've created a new test!
+
+To contribute your test to qemu.git please follow the guidelines here:
+http://wiki.qemu.org/Contribute/SubmitAPatch
+
  * Feedback and patches
  
  Please send improvements to the test suite, general feedback or just

  reports of failing tests cases to qemu-de...@nongnu.org with a CC:
  to qemu-block@nongnu.org.
+



Nice!

Unfortunately this does highlight how ridiculous our test number 
procurement process is, but at least it's documented.


Reviewed-by: John Snow 

Re: [Qemu-block] [Qemu-devel] [PATCH 0/3] build configuration query tool and conditional (qemu-io)test skip

[Daniel P. Berrange]

On Tue, Jul 25, 2017 at 12:16:13PM -0400, Cleber Rosa wrote:
> 
> 
> On 07/25/2017 11:49 AM, Stefan Hajnoczi wrote:
> > On Fri, Jul 21, 2017 at 10:21:24AM -0400, Cleber Rosa wrote:
> >> On 07/21/2017 10:01 AM, Daniel P. Berrange wrote:
> >>> On Fri, Jul 21, 2017 at 01:33:25PM +0100, Stefan Hajnoczi wrote:
>  On Thu, Jul 20, 2017 at 11:47:27PM -0400, Cleber Rosa wrote:
> >> Without the static capabilities defined, the dynamic check would be
> >> influenced by the run time environment.  It would really mean "qemu-io
> >> running on this environment (filesystem?) can do native aio".  Again,
> >> that's not the best type of information to depend on when writing tests.
> > 
> > Can you explain this more?
> > 
> > It seems logical to me that if qemu-io in this environment cannot do
> > aio=native then we must skip those tests.
> > 
> > Stefan
> > 
> 
> OK, let's abstract a bit more.  Let's take this part of your statement:
> 
>  "if qemu-io in this environment cannot do aio=native"
> 
> Let's call that a feature check.  Depending on how the *feature check*
> is written, a negative result may hide a test failure, because it would
> now be skipped.
> 
> Suppose that a feature check for "SDL display" is such that you run
> "qemu -display sdl".  A *feature failure* here (SDL init is broken), or
> an environment issue (DISPLAY=), will cause a SDL test skip.

You could have a way to statically define what features any given run
of the test suite should enable, then report failure if they were not
detected.

This is a similar situation to that seen with configure scripts. If invoked
with no --enable-xxx flags, it will probe for features & enable them if
found.  This means you can accidentally build without expected features if
you have a missing -devel package, or a header/library is broken in some
way. This is why configure prints a summary of which features it actually
found. It is also why when building binary packages like RPMs, it is common
to explicitly give --enable-xxx flags for all features you expect to see.
Automatic enablement is none the less still useful for people in general.

So if we applied this kind of approach for testing, then any automated
test systems at least, ought to provide a fixed list of features they
expect to be present for tests. So if any features accidentally broke
the tests would error.

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-block] [PATCH 4/7] block: convert ThrottleGroup to object with QOM

[Manos Pitsidianakis]

On Tue, Jul 25, 2017 at 05:09:41PM +0100, Stefan Hajnoczi wrote:

On Tue, Jul 25, 2017 at 01:29:08PM +0300, Manos Pitsidianakis wrote:

On Mon, Jul 24, 2017 at 04:12:47PM +0100, Stefan Hajnoczi wrote:
> On Fri, Jul 14, 2017 at 12:45:18PM +0300, Manos Pitsidianakis wrote:
> > ThrottleGroup is converted to an object. This will allow the future
> > throttle block filter drive easy creation and configuration of throttle
> > groups in QMP and cli.
> >
> > A new QAPI struct, ThrottleLimits, is introduced to provide a shared
> > struct for all throttle configuration needs in QMP.
> >
> > ThrottleGroups can be created via CLI as
> > -object throttling-group,id=foo,x-iops-total=100,x-..
>
> Please make the QOM name and struct name consistent.  Either
> ThrottleGroup/throttle-group or ThrottlingGroup/throttling-group but not
> ThrottleGroup/throttling-group.

I did this on purpose because current throttling has ThrottleGroup
internally and throttling.group in the command line. Should we keep this
only in legacy and make it throttle-group everywhere?


I don't see a compatibility issue because -drive throttling.group= is a
-drive property while THROTTLE_GROUP ("throttling-group") is a QOM class
name.  They are unrelated and the QOM convention is for the
typedef/struct name (ThrottleGroup) to be consistent with the QOM class
name.

Therefore it should be safe to use "throttle-group" as the QOM class
name instead of "throttling-group".


I meant for consistency not compatibility. Otherwise it probably would 
be better to keep throttle-group/ThrottleGroup in the new interfaces.



> > +visit_type_int64(v, name, , _err);
> > +if (local_err) {
> > +goto fail;
> > +}
> > +if (value < 0) {
> > +error_setg(_err, "Property value must be in range "
> > +   "[0, %"PRId64"]", INT64_MAX);
>
> Please print the maximum value relevant to the actual field type instead
> of INT64_MAX.

This checks the limits of the int64 field you give to QOM. I think you mean
in the value assignment to each field that follows? In any case, since
unsigned is the only smaller field we could convert it to uint64_t/uint32_t
internally.


I'm saying that UNSIGNED fields are silently truncated if the value is
larger than UINT_MAX, and also that the error message is misleading
since UNSIGNED fields cannot take values in the whole range we print.


Yes, wouldn't it be better to convert the unsigned field burst_length to 
uint64_t and take care of the overflow case? The field describes 
seconds, but there's no reason to keep it that small.


signature.asc
Description: PGP signature

Re: [Qemu-block] [Qemu-devel] [PATCH 0/3] build configuration query tool and conditional (qemu-io)test skip

[Cleber Rosa]

On 07/25/2017 11:49 AM, Stefan Hajnoczi wrote:
> On Fri, Jul 21, 2017 at 10:21:24AM -0400, Cleber Rosa wrote:
>> On 07/21/2017 10:01 AM, Daniel P. Berrange wrote:
>>> On Fri, Jul 21, 2017 at 01:33:25PM +0100, Stefan Hajnoczi wrote:
 On Thu, Jul 20, 2017 at 11:47:27PM -0400, Cleber Rosa wrote:
>> Without the static capabilities defined, the dynamic check would be
>> influenced by the run time environment.  It would really mean "qemu-io
>> running on this environment (filesystem?) can do native aio".  Again,
>> that's not the best type of information to depend on when writing tests.
> 
> Can you explain this more?
> 
> It seems logical to me that if qemu-io in this environment cannot do
> aio=native then we must skip those tests.
> 
> Stefan
> 

OK, let's abstract a bit more.  Let's take this part of your statement:

 "if qemu-io in this environment cannot do aio=native"

Let's call that a feature check.  Depending on how the *feature check*
is written, a negative result may hide a test failure, because it would
now be skipped.

Suppose that a feature check for "SDL display" is such that you run
"qemu -display sdl".  A *feature failure* here (SDL init is broken), or
an environment issue (DISPLAY=), will cause a SDL test skip.

If you base the test skip decision to a simple lookup on a list of
features (not calling them build configuration anymore, as this is clear
not attractive), this won't happen.  A "feature statement check" will
make the test proceed, and the *failure* will be presented.

I hope the pattern is visible.

-- 
Cleber Rosa
[ Sr Software Engineer - Virtualization Team - Red Hat ]
[ Avocado Test Framework - avocado-framework.github.io ]
[  7ABB 96EB 8B46 B94D 5E0F  E9BB 657E 8D33 A5F2 09F3  ]



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PULL 0/3] Block patches for 2.10-rc0

[Peter Maydell]

On 25 July 2017 at 16:12, Max Reitz  wrote:
> The following changes since commit 4c4414a4388f902b7ae2814f9a64898dd0e426a5:
>
>   hw/display/sm501: Don't use vmstate_register_ram_global() (2017-07-25 
> 13:04:28 +0100)
>
> are available in the git repository at:
>
>   git://github.com/XanClic/qemu.git tags/pull-block-2017-07-25
>
> for you to fetch changes up to bd998d7cc8ced211def90e4225042d63dddecc54:
>
>   qemu-iotests: Fix reference output for 186 (2017-07-25 16:33:58 +0200)
>
> 
> Block patches for 2.10-rc0
>
> 
> Daniel P. Berrange (1):
>   qcow: fix memory leaks related to encryption
>
> Kevin Wolf (1):
>   qemu-iotests: Fix reference output for 186
>
> Vladimir Sementsov-Ogievskiy (1):
>   qcow2-bitmap: fix bitmap_free
>
>  block/qcow.c   | 5 +++--
>  block/qcow2-bitmap.c   | 4 
>  block/qcow2.c  | 7 ---
>  tests/qemu-iotests/186.out | 6 +++---
>  4 files changed, 14 insertions(+), 8 deletions(-)

Applied, thanks.

-- PMM

Re: [Qemu-block] [PATCH 4/7] block: convert ThrottleGroup to object with QOM

[Stefan Hajnoczi]

On Tue, Jul 25, 2017 at 01:29:08PM +0300, Manos Pitsidianakis wrote:
> On Mon, Jul 24, 2017 at 04:12:47PM +0100, Stefan Hajnoczi wrote:
> > On Fri, Jul 14, 2017 at 12:45:18PM +0300, Manos Pitsidianakis wrote:
> > > ThrottleGroup is converted to an object. This will allow the future
> > > throttle block filter drive easy creation and configuration of throttle
> > > groups in QMP and cli.
> > > 
> > > A new QAPI struct, ThrottleLimits, is introduced to provide a shared
> > > struct for all throttle configuration needs in QMP.
> > > 
> > > ThrottleGroups can be created via CLI as
> > > -object throttling-group,id=foo,x-iops-total=100,x-..
> > 
> > Please make the QOM name and struct name consistent.  Either
> > ThrottleGroup/throttle-group or ThrottlingGroup/throttling-group but not
> > ThrottleGroup/throttling-group.
> 
> I did this on purpose because current throttling has ThrottleGroup
> internally and throttling.group in the command line. Should we keep this
> only in legacy and make it throttle-group everywhere?

I don't see a compatibility issue because -drive throttling.group= is a
-drive property while THROTTLE_GROUP ("throttling-group") is a QOM class
name.  They are unrelated and the QOM convention is for the
typedef/struct name (ThrottleGroup) to be consistent with the QOM class
name.

Therefore it should be safe to use "throttle-group" as the QOM class
name instead of "throttling-group".

> > > +visit_type_int64(v, name, , _err);
> > > +if (local_err) {
> > > +goto fail;
> > > +}
> > > +if (value < 0) {
> > > +error_setg(_err, "Property value must be in range "
> > > +   "[0, %"PRId64"]", INT64_MAX);
> > 
> > Please print the maximum value relevant to the actual field type instead
> > of INT64_MAX.
> 
> This checks the limits of the int64 field you give to QOM. I think you mean
> in the value assignment to each field that follows? In any case, since
> unsigned is the only smaller field we could convert it to uint64_t/uint32_t
> internally.

I'm saying that UNSIGNED fields are silently truncated if the value is
larger than UINT_MAX, and also that the error message is misleading
since UNSIGNED fields cannot take values in the whole range we print.


signature.asc
Description: PGP signature

[Qemu-block] [PATCH 2/2] iotests: Redirect stderr to stdout in 186

[Max Reitz]

Without redirecting qemu's stderr to stdout, _filter_qemu will not apply
to warnings.  This results in $QEMU_PROG not being replaced by QEMU_PROG
which is not great if your qemu executable is not called
qemu-system-x86_64 (e.g. qemu-system-i386).

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/186 |  2 +-
 tests/qemu-iotests/186.out | 12 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/tests/qemu-iotests/186 b/tests/qemu-iotests/186
index ab83ee4..2b9f618 100755
--- a/tests/qemu-iotests/186
+++ b/tests/qemu-iotests/186
@@ -56,7 +56,7 @@ function do_run_qemu()
 done
 fi
 echo quit
-) | $QEMU -S -nodefaults -display none -device virtio-scsi-pci -monitor 
stdio "$@"
+) | $QEMU -S -nodefaults -display none -device virtio-scsi-pci -monitor 
stdio "$@" 2>&1
 echo
 }
 
diff --git a/tests/qemu-iotests/186.out b/tests/qemu-iotests/186.out
index b8bf9a2..c8377fe 100644
--- a/tests/qemu-iotests/186.out
+++ b/tests/qemu-iotests/186.out
@@ -442,28 +442,28 @@ ide0-cd0 (NODE_NAME): null-co:// (null-co, read-only)
 Cache mode:   writeback
 (qemu) quit
 
-qemu-system-x86_64: -drive if=scsi,driver=null-co: warning: bus=0,unit=0 is 
deprecated with this machine type
 Testing: -drive if=scsi,driver=null-co
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) info block
+(qemu) QEMU_PROG: -drive if=scsi,driver=null-co: warning: bus=0,unit=0 is 
deprecated with this machine type
+info block
 scsi0-hd0 (NODE_NAME): null-co:// (null-co)
 Attached to:  /machine/unattached/device[27]/scsi.0/legacy[0]
 Cache mode:   writeback
 (qemu) quit
 
-qemu-system-x86_64: -drive if=scsi,media=cdrom: warning: bus=0,unit=0 is 
deprecated with this machine type
 Testing: -drive if=scsi,media=cdrom
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) info block
+(qemu) QEMU_PROG: -drive if=scsi,media=cdrom: warning: bus=0,unit=0 is 
deprecated with this machine type
+info block
 scsi0-cd0: [not inserted]
 Attached to:  /machine/unattached/device[27]/scsi.0/legacy[0]
 Removable device: not locked, tray closed
 (qemu) quit
 
-qemu-system-x86_64: -drive if=scsi,driver=null-co,media=cdrom: warning: 
bus=0,unit=0 is deprecated with this machine type
 Testing: -drive if=scsi,driver=null-co,media=cdrom
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) info block
+(qemu) QEMU_PROG: -drive if=scsi,driver=null-co,media=cdrom: warning: 
bus=0,unit=0 is deprecated with this machine type
+info block
 scsi0-cd0 (NODE_NAME): null-co:// (null-co, read-only)
 Attached to:  /machine/unattached/device[27]/scsi.0/legacy[0]
 Removable device: not locked, tray closed
-- 
2.9.4

[Qemu-block] [PATCH 0/2] iotests: Fixes to 156 and 186

[Max Reitz]

This series contains some small fixes for issues I stumbled upon when
preparing my pull request.


Max Reitz (2):
  iotests: Fix test 156
  iotests: Redirect stderr to stdout in 186

 tests/qemu-iotests/156 |  4 ++--
 tests/qemu-iotests/186 |  2 +-
 tests/qemu-iotests/186.out | 12 ++--
 3 files changed, 9 insertions(+), 9 deletions(-)

-- 
2.9.4

[Qemu-block] [PATCH 1/2] iotests: Fix test 156

[Max Reitz]

On one hand, the _make_test_img invocation for creating the target image
was missing a -u because its backing file is not supposed to exist at
that point.

On the other hand, nobody noticed probably because the backing file is
created later on and _cleanup failed to remove it: The quotation marks
were misplaced so bash tried to deleted a file literally called
"$TEST_IMG{,.target}..." instead of resolving the globs.  Thus, the
files stayed around after the first run and qemu-img create did not
complain about a missing backing file on any run but the first.

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/156 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/156 b/tests/qemu-iotests/156
index 2c4a06e..e75dc4d 100755
--- a/tests/qemu-iotests/156
+++ b/tests/qemu-iotests/156
@@ -38,7 +38,7 @@ status=1  # failure is the default!
 _cleanup()
 {
 _cleanup_qemu
-rm -f "$TEST_IMG{,.target}{,.backing,.overlay}"
+rm -f "$TEST_IMG"{,.target}{,.backing,.overlay}
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
@@ -83,7 +83,7 @@ _send_qemu_cmd $QEMU_HANDLE \
 'return'
 
 # Create target image
-TEST_IMG="$TEST_IMG.target.overlay" _make_test_img -b "$TEST_IMG.target" 1M
+TEST_IMG="$TEST_IMG.target.overlay" _make_test_img -u -b "$TEST_IMG.target" 1M
 
 # Mirror snapshot
 _send_qemu_cmd $QEMU_HANDLE \
-- 
2.9.4

Re: [Qemu-block] [Qemu-devel] [PATCH 0/3] build configuration query tool and conditional (qemu-io)test skip

[Stefan Hajnoczi]

On Fri, Jul 21, 2017 at 10:21:24AM -0400, Cleber Rosa wrote:
> On 07/21/2017 10:01 AM, Daniel P. Berrange wrote:
> > On Fri, Jul 21, 2017 at 01:33:25PM +0100, Stefan Hajnoczi wrote:
> >> On Thu, Jul 20, 2017 at 11:47:27PM -0400, Cleber Rosa wrote:
> Without the static capabilities defined, the dynamic check would be
> influenced by the run time environment.  It would really mean "qemu-io
> running on this environment (filesystem?) can do native aio".  Again,
> that's not the best type of information to depend on when writing tests.

Can you explain this more?

It seems logical to me that if qemu-io in this environment cannot do
aio=native then we must skip those tests.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH 3/3] qemu-iotests: require CONFIG_LINUX_AIO for test 087

[Daniel P. Berrange]

On Tue, Jul 25, 2017 at 04:45:46PM +0100, Stefan Hajnoczi wrote:
> On Mon, Jul 24, 2017 at 02:44:13PM +0800, Jing Liu wrote:
> > On 2017/7/21 上午11:47, Cleber Rosa wrote:
> > > One of the "sub-"tests of test 087 requires CONFIG_LINUX_AIO.
> > > 
> > > As a PoC/RFC, this goes the easy route and skips the test as a whole
> > > when that feature is missing.  Other approaches include splitting
> > > the test and adding extra filtering.
> > > 
> > > Signed-off-by: Cleber Rosa 
> > > ---
> > >   tests/qemu-iotests/087 | 1 +
> > >   1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
> > > index f8e4903..a2fb7de 100755
> > > --- a/tests/qemu-iotests/087
> > > +++ b/tests/qemu-iotests/087
> > > @@ -34,6 +34,7 @@ status=1# failure is the default!
> > >   _supported_fmt qcow2
> > >   _supported_proto file
> > >   _supported_os Linux
> > > +_require_feature CONFIG_LINUX_AIO
> > I tested that CONFIG_NETTLE_KDF is also a necessary for 087.
> > 
> > +_require_feature CONFIG_NETTLE_KDF
> 
> Are you sure?  Looks like either nettle or gcrypt is needed:

Correct, it works with either.

> crypto/Makefile.objs:crypto-obj-$(CONFIG_NETTLE_KDF) += pbkdf-nettle.o
> crypto/Makefile.objs:crypto-obj-$(if 
> $(CONFIG_NETTLE_KDF),n,$(CONFIG_GCRYPT_KDF)) += pbkdf-gcrypt.o
> 
> But this shows why the compile-time testing of features is ugly:
> 
> 1. It duplicates build dependency logic into the test cases.
> 
> 2. The test cases don't care about nettle vs gcrypt and they shouldn't
>have to know about it.  They just care whether LUKS is available or
>not.

Yeah that knowledge is messy and fragile wrt future changes.


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-block] [PATCH 3/3] qemu-iotests: require CONFIG_LINUX_AIO for test 087

[Stefan Hajnoczi]

On Mon, Jul 24, 2017 at 02:44:13PM +0800, Jing Liu wrote:
> On 2017/7/21 上午11:47, Cleber Rosa wrote:
> > One of the "sub-"tests of test 087 requires CONFIG_LINUX_AIO.
> > 
> > As a PoC/RFC, this goes the easy route and skips the test as a whole
> > when that feature is missing.  Other approaches include splitting
> > the test and adding extra filtering.
> > 
> > Signed-off-by: Cleber Rosa 
> > ---
> >   tests/qemu-iotests/087 | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
> > index f8e4903..a2fb7de 100755
> > --- a/tests/qemu-iotests/087
> > +++ b/tests/qemu-iotests/087
> > @@ -34,6 +34,7 @@ status=1  # failure is the default!
> >   _supported_fmt qcow2
> >   _supported_proto file
> >   _supported_os Linux
> > +_require_feature CONFIG_LINUX_AIO
> I tested that CONFIG_NETTLE_KDF is also a necessary for 087.
> 
> +_require_feature CONFIG_NETTLE_KDF

Are you sure?  Looks like either nettle or gcrypt is needed:

crypto/Makefile.objs:crypto-obj-$(CONFIG_NETTLE_KDF) += pbkdf-nettle.o
crypto/Makefile.objs:crypto-obj-$(if 
$(CONFIG_NETTLE_KDF),n,$(CONFIG_GCRYPT_KDF)) += pbkdf-gcrypt.o

But this shows why the compile-time testing of features is ugly:

1. It duplicates build dependency logic into the test cases.

2. The test cases don't care about nettle vs gcrypt and they shouldn't
   have to know about it.  They just care whether LUKS is available or
   not.

As I mentioned earlier, let's test whether the QEMU binaries offer
_features_, not which _config options_ they were built with.


signature.asc
Description: PGP signature

[Qemu-block] [PATCH v2] qemu-iotests: add a "how to" to ./README

[Stefan Hajnoczi]

There is not much getting started documentation for qemu-iotests.  This
patch explains how to create a new test and covers the overall testing
approach.

Cc: Ishani Chugh 
Reviewed-by: Eric Blake 
Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Stefan Hajnoczi 
---
v2:
 * Added missing "touch .out" step [Kevin]
 * Added reference to SubmitAPatch wiki page [Eric & Philippe]
---
 tests/qemu-iotests/README | 94 +++
 1 file changed, 94 insertions(+)

diff --git a/tests/qemu-iotests/README b/tests/qemu-iotests/README
index 6079b401ae..245f509b14 100644
--- a/tests/qemu-iotests/README
+++ b/tests/qemu-iotests/README
@@ -14,8 +14,102 @@ Just run ./check to run all tests for the raw image format, 
or ./check
 -qcow2 to test the qcow2 image format.  The output of ./check -h explains
 additional options to test further image formats or I/O methods.
 
+* Testing approach
+
+Each test is an executable file (usually a bash script) that is run by the
+./check test harness.  Standard out and standard error are captured to an
+output file.  If the output file differs from the "golden master" output file
+for the test then it fails.
+
+Tests are simply a sequence of commands that produce output and the test itself
+does not judge whether it passed or failed.  If you find yourself writing
+checks to determine success or failure then you should rethink the test and
+rely on output diffing instead.
+
+** Filtering volatile output
+
+When output contains absolute file paths, timestamps, process IDs, hostnames,
+or other volatile strings, the diff against golden master output will fail.
+Such output must be filtered to replace volatile strings with fixed
+placeholders.
+
+For example, the path to the temporary working directory changes between test
+runs so it must be filtered:
+
+  sed -e "s#$TEST_DIR/#TEST_DIR/#g"
+
+Commonly needed filters are available in ./common.filter.
+
+** Python tests
+
+Most tests are implemented in bash but it is difficult to interact with the QMP
+monitor.  A Python module called 'iotests' is available for tests that require
+JSON and interacting with QEMU.
+
+* How to create a test
+
+1. Choose an unused test number
+
+Tests are identified by a unique number.  Look for the highest test case number
+by looking at the test files.  Then search the qemu-de...@nongnu.org mailing
+list to check if anyone has already sent patches using the next available
+number.  You may need to increment the number a few times to reach an unused
+number.
+
+2. Create the test file
+
+Copy an existing test (one that most closely resembles what you wish to test)
+to the new test number:
+
+  cp 001 
+
+3. Assign groups to the test
+
+Add your test to the ./group file.  This file is the index of tests and assigns
+them to functional groups like "rw" for read-write tests.  Most tests belong to
+the "rw" and "auto" groups.  "auto" means the test runs when ./check is invoked
+without a -g argument.
+
+Consider adding your test to the "quick" group if it executes quickly (<1s).
+This group is run by "make check-block" and is often included as part of build
+tests in continuous integration systems.
+
+4. Write the test
+
+Edit the test script.  Look at existing tests for examples.
+
+5. Generate the golden master file
+
+Create an empty golden master file:
+
+  touch .out
+
+Then run your test:
+
+  ./check 
+
+You may need additional command-line options to use an image format or
+protocol like -qcow2.
+
+The test will fail because there is no golden master yet.  Inspect the output
+that your test generated with "cat .out.bad".
+
+Verify that the output is as expected and contains no volatile strings like
+timestamps.  You may need to add filters to your test to remove volatile
+strings.
+
+Once you are happy with the test output it can be used as the golden master
+with "mv .out.bad .out".  Rerun the test to verify
+that it passes.
+
+Congratulations, you've created a new test!
+
+To contribute your test to qemu.git please follow the guidelines here:
+http://wiki.qemu.org/Contribute/SubmitAPatch
+
 * Feedback and patches
 
 Please send improvements to the test suite, general feedback or just
 reports of failing tests cases to qemu-de...@nongnu.org with a CC:
 to qemu-block@nongnu.org.
+
-- 
2.13.3

Re: [Qemu-block] [Qemu-devel] [PATCH] qemu-iotests: add a "how to" to ./README

[Peter Maydell]

On 25 July 2017 at 16:20, Stefan Hajnoczi  wrote:
> On Mon, Jul 24, 2017 at 11:20:44AM +0100, Peter Maydell wrote:
>> Should ./check be run from the source tree, or the build tree? The
>> existing README text doesn't say and I don't think your additions
>> do either.
>
> It doesn't matter, both should work.  The script detects both
> possibilities and rejigs itself to compensate.

Does that mean
 if you run it from the source tree in a tree configured
 for out of tree build it will:
 (a) pollute your source tree with test output and binaries
 (b) give you a helpful message about what to do
 (c) magically find the build tree somehow
 (d) not need to write any binaries/test output/temp files at all

?

thanks
-- PMM

Re: [Qemu-block] [Qemu-devel] [PATCH for 2.10 06/35] qcow2: remove inconsistent check

[Paolo Bonzini]

On 24/07/2017 20:42, Eric Blake wrote:
> On 07/24/2017 01:27 PM, Philippe Mathieu-Daudé wrote:
>> This is equivalent to assert(russian roulette) so better remove it.
>>
>> block/qcow2-bitmap.c:259:29: warning: The left operand of '==' is a garbage 
>> value
>> assert(bitmap_table == NULL);
>> ^
>>
>> Reported-by: Clang Static Analyzer
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>>  block/qcow2-bitmap.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 3e8735a20d..fe72df5057 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -254,7 +254,6 @@ static int free_bitmap_clusters(BlockDriverState *bs, 
>> Qcow2BitmapTable *tb)
>>  
>>  ret = bitmap_table_load(bs, tb, _table);
>>  if (ret < 0) {
>> -assert(bitmap_table == NULL);
> 
> Rather, we should fix bitmap_table_load() to ensure that bitmap_table is
> always assigned, even on error.

I think it's even better to initialize bitmap_table to NULL in
free_bitmap_clusters, as all other callers do.

Paolo

Re: [Qemu-block] [Qemu-devel] [PATCH] qcow2-bitmap: fix bitmap_free

[Eric Blake]

On 07/25/2017 10:02 AM, Philippe Mathieu-Daudé wrote:
> Maybe worth adding "Coverity: CID 1377700"

At this point, the PULL request is already posted, so it may be too
late.  But it's not the end of the world if we miss the extra comment.

> 
> On 07/14/2017 01:00 PM, Max Reitz wrote:
>> On 2017-07-14 14:33, Vladimir Sementsov-Ogievskiy wrote:
>>> Fix possible crash on error path in
>>> qcow2_remove_persistent_dirty_bitmap. Although bitmap_free was added in
>>> 88ddffae8fc the bug was introduced later in commit 469c71edc72 (when
>>> qcow2_remove_persistent_dirty_bitmap was added).
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>> Reviewed-by: Eric Blake 
>>> ---
>>>   block/qcow2-bitmap.c | 4 
>>>   1 file changed, 4 insertions(+)
>>
>> Thanks, applied to my block branch:
>>
>> https://github.com/XanClic/qemu/commits/block
>>
>> Max
>>
> 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH] qemu-iotests: add a "how to" to ./README

[Stefan Hajnoczi]

On Mon, Jul 24, 2017 at 11:11:28AM +0200, Kevin Wolf wrote:
> Am 21.07.2017 um 11:34 hat Stefan Hajnoczi geschrieben:
> > +5. Generate the golden master file
> > +
> > +Run your test with "./check ".  You may need to pass 
> > additional
> > +options to use an image format or protocol.
> 
> ./check refuses to even run a test if the reference output is missing.
> So in practice you need a 'touch .out' first.

Oops, will fix!

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-block] [Qemu-devel] [PATCH] qemu-iotests: add a "how to" to ./README

[Stefan Hajnoczi]

On Mon, Jul 24, 2017 at 11:20:44AM +0100, Peter Maydell wrote:
> On 21 July 2017 at 10:34, Stefan Hajnoczi  wrote:
> > There is not much getting started documentation for qemu-iotests.  This
> > patch explains how to create a new test and covers the overall testing
> > approach.
> >
> > Cc: Ishani Chugh 
> > Signed-off-by: Stefan Hajnoczi 
> > ---
> >  tests/qemu-iotests/README | 83 
> > +++
> >  1 file changed, 83 insertions(+)
> >
> > diff --git a/tests/qemu-iotests/README b/tests/qemu-iotests/README
> > index 6079b40..8259b9f 100644
> > --- a/tests/qemu-iotests/README
> > +++ b/tests/qemu-iotests/README
> > @@ -14,8 +14,91 @@ Just run ./check to run all tests for the raw image 
> > format, or ./check
> >  -qcow2 to test the qcow2 image format.  The output of ./check -h explains
> >  additional options to test further image formats or I/O methods.
> >
> > +* Testing approach
> > +
> > +Each test is an executable file (usually a bash script) that is run by the
> > +./check test harness.  Standard out and standard error are captured to an
> > +output file.  If the output file differs from the "golden master" output 
> > file
> > +for the test then it fails.
> 
> Should ./check be run from the source tree, or the build tree? The
> existing README text doesn't say and I don't think your additions
> do either.

It doesn't matter, both should work.  The script detects both
possibilities and rejigs itself to compensate.

Stefan


signature.asc
Description: PGP signature

[Qemu-block] [PULL 3/3] qemu-iotests: Fix reference output for 186

[Max Reitz]

From: Kevin Wolf 

Commits 70f17a1 ('error: Revert unwanted change of warning messages')
and e1824e5 ('qemu-iotests: Test 'info block'') had a semantic merge
conflict, which results in failure for qemu-iotests case 186. Fix the
reference output to consider the changes of 70f17a1.

Signed-off-by: Kevin Wolf 
Message-id: 1500973176-29235-1-git-send-email-kw...@redhat.com
Reviewed-by: Markus Armbruster 
Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/186.out | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/186.out b/tests/qemu-iotests/186.out
index b963b12..b8bf9a2 100644
--- a/tests/qemu-iotests/186.out
+++ b/tests/qemu-iotests/186.out
@@ -442,7 +442,7 @@ ide0-cd0 (NODE_NAME): null-co:// (null-co, read-only)
 Cache mode:   writeback
 (qemu) quit
 
-warning: qemu-system-x86_64: -drive if=scsi,driver=null-co: bus=0,unit=0 is 
deprecated with this machine type
+qemu-system-x86_64: -drive if=scsi,driver=null-co: warning: bus=0,unit=0 is 
deprecated with this machine type
 Testing: -drive if=scsi,driver=null-co
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) info block
@@ -451,7 +451,7 @@ scsi0-hd0 (NODE_NAME): null-co:// (null-co)
 Cache mode:   writeback
 (qemu) quit
 
-warning: qemu-system-x86_64: -drive if=scsi,media=cdrom: bus=0,unit=0 is 
deprecated with this machine type
+qemu-system-x86_64: -drive if=scsi,media=cdrom: warning: bus=0,unit=0 is 
deprecated with this machine type
 Testing: -drive if=scsi,media=cdrom
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) info block
@@ -460,7 +460,7 @@ scsi0-cd0: [not inserted]
 Removable device: not locked, tray closed
 (qemu) quit
 
-warning: qemu-system-x86_64: -drive if=scsi,driver=null-co,media=cdrom: 
bus=0,unit=0 is deprecated with this machine type
+qemu-system-x86_64: -drive if=scsi,driver=null-co,media=cdrom: warning: 
bus=0,unit=0 is deprecated with this machine type
 Testing: -drive if=scsi,driver=null-co,media=cdrom
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) info block
-- 
2.9.4

[Qemu-block] [PULL 2/3] qcow2-bitmap: fix bitmap_free

[Max Reitz]

From: Vladimir Sementsov-Ogievskiy 

Fix possible crash on error path in
qcow2_remove_persistent_dirty_bitmap. Although bitmap_free was added in
88ddffae8fc the bug was introduced later in commit 469c71edc72 (when
qcow2_remove_persistent_dirty_bitmap was added).

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
Message-id: 20170714123341.373857-1-vsement...@virtuozzo.com
Signed-off-by: Max Reitz 
---
 block/qcow2-bitmap.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 3e8735a..e8d3bdb 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -487,6 +487,10 @@ static inline void bitmap_directory_to_be(uint8_t *dir, 
size_t size)
 
 static void bitmap_free(Qcow2Bitmap *bm)
 {
+if (bm == NULL) {
+return;
+}
+
 g_free(bm->name);
 g_free(bm);
 }
-- 
2.9.4

[Qemu-block] [PULL 0/3] Block patches for 2.10-rc0

[Max Reitz]

The following changes since commit 4c4414a4388f902b7ae2814f9a64898dd0e426a5:

  hw/display/sm501: Don't use vmstate_register_ram_global() (2017-07-25 
13:04:28 +0100)

are available in the git repository at:

  git://github.com/XanClic/qemu.git tags/pull-block-2017-07-25

for you to fetch changes up to bd998d7cc8ced211def90e4225042d63dddecc54:

  qemu-iotests: Fix reference output for 186 (2017-07-25 16:33:58 +0200)


Block patches for 2.10-rc0


Daniel P. Berrange (1):
  qcow: fix memory leaks related to encryption

Kevin Wolf (1):
  qemu-iotests: Fix reference output for 186

Vladimir Sementsov-Ogievskiy (1):
  qcow2-bitmap: fix bitmap_free

 block/qcow.c   | 5 +++--
 block/qcow2-bitmap.c   | 4 
 block/qcow2.c  | 7 ---
 tests/qemu-iotests/186.out | 6 +++---
 4 files changed, 14 insertions(+), 8 deletions(-)

-- 
2.9.4

[Qemu-block] [PULL 1/3] qcow: fix memory leaks related to encryption

[Max Reitz]

From: "Daniel P. Berrange" 

Fix leak of the 'encryptopts' string, which was mistakenly
declared const.

Fix leak of QemuOpts entry which should not have been deleted
from the opts array.

Reported by: coverity
Signed-off-by: Daniel P. Berrange 
Message-id: 20170714103105.5781-1-berra...@redhat.com
Reviewed-by: Eric Blake 
Signed-off-by: Max Reitz 
---
 block/qcow.c  | 5 +++--
 block/qcow2.c | 7 ---
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index 66827d6..c08cdc4 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -768,7 +768,7 @@ static int qcow_create(const char *filename, QemuOpts 
*opts, Error **errp)
 Error *local_err = NULL;
 int ret;
 BlockBackend *qcow_blk;
-const char *encryptfmt = NULL;
+char *encryptfmt = NULL;
 QDict *options;
 QDict *encryptopts = NULL;
 QCryptoBlockCreateOptions *crypto_opts = NULL;
@@ -793,7 +793,7 @@ static int qcow_create(const char *filename, QemuOpts 
*opts, Error **errp)
 goto cleanup;
 }
 } else if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
-encryptfmt = "aes";
+encryptfmt = g_strdup("aes");
 }
 
 ret = bdrv_create_file(filename, opts, _err);
@@ -908,6 +908,7 @@ exit:
 blk_unref(qcow_blk);
 cleanup:
 QDECREF(encryptopts);
+g_free(encryptfmt);
 qcrypto_block_free(crypto);
 qapi_free_QCryptoBlockCreateOptions(crypto_opts);
 g_free(backing_file);
diff --git a/block/qcow2.c b/block/qcow2.c
index 90efa44..d7c600b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2905,7 +2905,7 @@ static int qcow2_create(const char *filename, QemuOpts 
*opts, Error **errp)
 int version;
 uint64_t refcount_bits;
 int refcount_order;
-const char *encryptfmt = NULL;
+char *encryptfmt = NULL;
 Error *local_err = NULL;
 int ret;
 
@@ -2916,14 +2916,14 @@ static int qcow2_create(const char *filename, QemuOpts 
*opts, Error **errp)
 backing_fmt = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FMT);
 encryptfmt = qemu_opt_get_del(opts, BLOCK_OPT_ENCRYPT_FORMAT);
 if (encryptfmt) {
-if (qemu_opt_get_del(opts, BLOCK_OPT_ENCRYPT)) {
+if (qemu_opt_get(opts, BLOCK_OPT_ENCRYPT)) {
 error_setg(errp, "Options " BLOCK_OPT_ENCRYPT " and "
BLOCK_OPT_ENCRYPT_FORMAT " are mutually exclusive");
 ret = -EINVAL;
 goto finish;
 }
 } else if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
-encryptfmt = "aes";
+encryptfmt = g_strdup("aes");
 }
 cluster_size = qcow2_opt_get_cluster_size_del(opts, _err);
 if (local_err) {
@@ -2983,6 +2983,7 @@ static int qcow2_create(const char *filename, QemuOpts 
*opts, Error **errp)
 finish:
 g_free(backing_file);
 g_free(backing_fmt);
+g_free(encryptfmt);
 g_free(buf);
 return ret;
 }
-- 
2.9.4

Re: [Qemu-block] [PATCH] qemu-iotests: Fix reference output for 186

[Max Reitz]

On 2017-07-25 10:59, Kevin Wolf wrote:
> Commits 70f17a1 ('error: Revert unwanted change of warning messages')
> and e1824e5 ('qemu-iotests: Test 'info block'') had a semantic merge
> conflict, which results in failure for qemu-iotests case 186. Fix the
> reference output to consider the changes of 70f17a1.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/186.out | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

I've noticed that test 186 fails when not using qemu-system-x86_64,
though (just build with --target-list=i386-softmmu, for example). But
this patch doesn't make it worse, so I'll keep it.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH] qcow2-bitmap: fix bitmap_free

[Philippe Mathieu-Daudé]

Maybe worth adding "Coverity: CID 1377700"

On 07/14/2017 01:00 PM, Max Reitz wrote:

On 2017-07-14 14:33, Vladimir Sementsov-Ogievskiy wrote:

Fix possible crash on error path in
qcow2_remove_persistent_dirty_bitmap. Although bitmap_free was added in
88ddffae8fc the bug was introduced later in commit 469c71edc72 (when
qcow2_remove_persistent_dirty_bitmap was added).

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
---
  block/qcow2-bitmap.c | 4 
  1 file changed, 4 insertions(+)


Thanks, applied to my block branch:

https://github.com/XanClic/qemu/commits/block

Max

Re: [Qemu-block] [PATCH] qemu-iotests: Fix reference output for 186

[Max Reitz]

On 2017-07-25 10:59, Kevin Wolf wrote:
> Commits 70f17a1 ('error: Revert unwanted change of warning messages')
> and e1824e5 ('qemu-iotests: Test 'info block'') had a semantic merge
> conflict, which results in failure for qemu-iotests case 186. Fix the
> reference output to consider the changes of 70f17a1.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/186.out | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Thanks, applied to my block tree:

https://github.com/XanClic/qemu/commits/block

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH V5 01/10] specs/qcow2: add compress format extension

[Eric Blake]

On 07/25/2017 09:41 AM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven 
> ---
>  docs/interop/qcow2.txt | 51 
> +-
>  roms/ipxe  |  2 +-
>  2 files changed, 51 insertions(+), 2 deletions(-)
> 

> +
> +== Compress format extension ==
> +
> +The compress format extension is an optional header extension. It provides
> +the ability to specify the compress algorithm and compress parameters

s/the compress algorithm/the compression algorithm/

> +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 - 13:  compress_format_name (padded with zeros, but not
> +   necessarily null terminated if it has full length).
> +   Valid compression format names currently are:
> +
> +   deflate: Standard zlib deflate compression without
> +compression header

Why did you name it "deflate" instead of "zlib" again?

> +
> +  14:  compress_level (uint8_t)
> +
> +   0 = default compress level (valid for all formats, 
> default)
> +
> +   Additional valid compression levels for deflate 
> compression:
> +
> +   All values between 1 and 9. 1 gives best speed, 9 gives 
> best
> +   compression. The default compression level is defined by 
> zlib
> +   and currently defaults to 6.
> +
> +  15:  compress_window_size (uint8_t)
> +
> +   Window or dictionary size used by the compression format.
> +   Currently only used by the deflate compression algorithm.

What must this be set to for other algorithms?  I guess we get to that
in later patches.

> +
> +   Valid window sizes for deflate compression range from 8 to
> +   15 inclusively.
> +
> +Note: Omitting the incompatible "Compress format bit" results in the usage
> +of deflate compression with default compression level and a window size of 12
> +(which was default before QEMU 2.11). If exactly these parameters are choosen

s/choosen/chosen,/

> +it is free to the implementation to omit the "Compress format bit" and the

s/it is free to the implementation to omit/the implementation may omit/

> +compress format extension when updating the QCOW2 header.
> +
> +
>  == Host cluster management ==
>  
>  qcow2 manages the allocation of host clusters by maintaining a reference 
> count
> diff --git a/roms/ipxe b/roms/ipxe
> index 0600d3a..b991c67 16
> --- a/roms/ipxe
> +++ b/roms/ipxe
> @@ -1 +1 @@
> -Subproject commit 0600d3ae94f93efd10fc6b3c7420a9557a3a1670
> +Subproject commit b991c67c1d91574ef22336cc3a5944d1e63230c9

Oops.

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



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH V5 04/10] qemu-img: add documentation for compress settings

[Peter Lieven]

Signed-off-by: Peter Lieven 
---
 qemu-img.texi | 26 ++
 1 file changed, 26 insertions(+)

diff --git a/qemu-img.texi b/qemu-img.texi
index 72dabd6..3612c59 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -676,6 +676,32 @@ 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:
+
+   deflateUses standard zlib defalte compression
+
+The compression algorithm can only be defined at image create time and cannot
+be changed later.
+
+Note: defining compression format settings which are different from the old
+  default (format=deflate, level=0, window-size=12) will result in the
+  compression format extension being written to the Qcow2 image. Versions
+  of QEMU before 2.11 will not be able to open images with this extension.
+
+@item compress.level
+Valid for compress.format=deflate defines the compression level to use for
+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 9 for the best compression.
+
+@item compress.window-size
+Valid for compress.format=deflate defines the compression window size used
+during compression. Valid window sizes for deflate compression range from 8 to
+15 inclusively. The default is @code{compress.window-size=15}.
+
 @end table
 
 @item Other
-- 
1.9.1

[Qemu-block] [PATCH V5 08/10] 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 
---
 block/qcow2-cluster.c | 58 ++-
 block/qcow2.c | 53 +++---
 2 files changed, 61 insertions(+), 50 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index f06c08f..345feec 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1479,30 +1479,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,
+ Qcow2Compress *compress)
 {
-z_stream strm1, *strm = 
-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) {
+case QCOW2_COMPRESS_FORMAT_DEFLATE:
+{
+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(_strm, -compress->u.deflate.window_size);
+if (ret != Z_OK) {
+return -1;
+}
+ret = inflate(_strm, Z_FINISH);
+out_len = z_strm.next_out - out_buf;
+ret = -(ret != Z_STREAM_END);
+inflateEnd(_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)
@@ -1523,7 +1532,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,
+  >compress) < 0) {
 return -EIO;
 }
 s->cluster_cache_offset = coffset;
diff --git a/block/qcow2.c b/block/qcow2.c
index a12b3d7..6031963 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3426,9 +3426,9 @@ 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 ret, out_len = 0;
+uint8_t *buf, *out_buf = NULL, *local_buf = NULL;
 uint64_t cluster_offset;
 
 if (bytes == 0) {
@@ -3453,34 +3453,35 @@ 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) {
+case QCOW2_COMPRESS_FORMAT_DEFLATE:
+out_buf = g_malloc(s->cluster_size);
+ret = deflateInit2(
+_strm, s->compress.u.deflate.level ?
+ Z_DEFAULT_COMPRESSION : s->compress.u.deflate.level,
+ Z_DEFLATED, -s->compress.u.deflate.window_size,
+ 9, Z_DEFAULT_STRATEGY);
+if (ret != Z_OK) {
+ret = -EINVAL;
+goto fail;
+}
 
-/* best compression, small window, no zlib header */
-memset(, 0, sizeof(strm));
-ret = deflateInit2(, Z_DEFAULT_COMPRESSION,
-   Z_DEFLATED, -12,
-   9, Z_DEFAULT_STRATEGY);
-if (ret != 0) {
-ret = -EINVAL;
-goto fail;
-}
+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;
 
-strm.avail_in = s->cluster_size;
-strm.next_in = (uint8_t *)buf;
-strm.avail_out = s->cluster_size;
-strm.next_out = out_buf;
+ret = deflate(_strm, Z_FINISH);
+out_len = z_strm.next_out - out_buf;
+deflateEnd(_strm);
 
-ret = deflate(, Z_FINISH);
-if (ret != Z_STREAM_END && ret != Z_OK) {
-deflateEnd();
-ret = -EINVAL;
-goto fail;
+ret = ret != Z_STREAM_END;
+break;
+default:
+abort(); /* should never reach this point */
 }
-out_len = strm.next_out - out_buf;
-
-

[Qemu-block] [PATCH V5 07/10] 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 
---
 block/qcow2.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index ffe609d..a12b3d7 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3428,7 +3428,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) {
@@ -3438,8 +3438,8 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, 
uint64_t offset,
 return bdrv_truncate(bs->file, cluster_offset, PREALLOC_MODE_OFF, 
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)
 {
@@ -3448,8 +3448,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);
 
@@ -3517,7 +3519,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-block] [PATCH V5 05/10] 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 
---
 block/qcow2.c | 92 +++
 block/qcow2.h | 21 ++
 2 files changed, 108 insertions(+), 5 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index a5270a6..7fd52e1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -70,6 +70,7 @@ typedef struct {
 #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
 #define  QCOW2_EXT_MAGIC_CRYPTO_HEADER 0x0537be77
 #define  QCOW2_EXT_MAGIC_BITMAPS 0x23852875
+#define  QCOW2_EXT_MAGIC_COMPRESS_FORMAT 0xC03183A3
 
 static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
 {
@@ -269,6 +270,57 @@ static int qcow2_read_extensions(BlockDriverState *bs, 
uint64_t start_offset,
 #endif
 break;
 
+case QCOW2_EXT_MAGIC_COMPRESS_FORMAT:
+{
+Qcow2CompressFormatExt compress_ext;
+Error *local_err = NULL;
+if (ext.len != sizeof(compress_ext)) {
+error_setg(errp, "ERROR: ext_compress_format: len=%"
+   PRIu32 " invalid (!=%zu)", ext.len,
+   sizeof(compress_ext));
+return 2;
+}
+ret = bdrv_pread(bs->file, offset, _ext,
+ ext.len);
+if (ret < 0) {
+error_setg_errno(errp, -ret, "ERROR: ext_compress_fromat:"
+ " Could not read extension");
+return 3;
+}
+
+s->compress.format =
+qapi_enum_parse(Qcow2CompressFormat_lookup,
+compress_ext.name, QCOW2_COMPRESS_FORMAT__MAX,
+-1, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+return 4;
+}
+
+if (s->compress.format == QCOW2_COMPRESS_FORMAT_DEFLATE) {
+s->compress.u.deflate.has_level = true;
+s->compress.u.deflate.level = compress_ext.level;
+s->compress.u.deflate.has_window_size = true;
+s->compress.u.deflate.window_size = compress_ext.window_size;
+}
+
+qcow2_check_compress_settings(>compress, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+return 5;
+}
+
+#ifdef DEBUG_EXT
+if (s->compress.format == QCOW2_COMPRESS_FORMAT_DEFLATE) {
+printf("Qcow2: Got compress format %s with level %" PRIu8
+   " window_size %" PRIu8 "\n",
+   Qcow2CompressFormat_lookup[s->compress.format],
+   s->compress.u.deflate.level,
+   s->compress.u.deflate.window_size);
+}
+#endif
+break;
+}
 case QCOW2_EXT_MAGIC_FEATURE_TABLE:
 if (p_feature_table != NULL) {
 void* feature_table = g_malloc0(ext.len + 2 * 
sizeof(Qcow2Feature));
@@ -1403,6 +1455,12 @@ static int qcow2_do_open(BlockDriverState *bs, QDict 
*options, int flags,
 s->cluster_cache_offset = -1;
 s->flags = flags;
 
+s->compress.format = QCOW2_COMPRESS_FORMAT_DEFLATE;
+s->compress.u.deflate.has_level = true;
+s->compress.u.deflate.level = 0;
+s->compress.u.deflate.has_window_size = true;
+s->compress.u.deflate.window_size = 12;
+
 ret = qcow2_refcount_init(bs);
 if (ret != 0) {
 error_setg_errno(errp, -ret, "Could not initialize refcount handling");
@@ -2320,6 +2378,30 @@ int qcow2_update_header(BlockDriverState *bs)
 buflen -= ret;
 }
 
+/* Compress Format header extension */
+if (s->compress.format != QCOW2_COMPRESS_FORMAT_DEFLATE ||
+s->compress.u.deflate.level != 0 ||
+s->compress.u.deflate.window_size != 12) {
+Qcow2CompressFormatExt ext = {};
+assert(s->compress.format < QCOW2_COMPRESS_FORMAT__MAX);
+strncpy((char *) ,
+Qcow2CompressFormat_lookup[s->compress.format],
+sizeof(ext.name));
+if (s->compress.format == QCOW2_COMPRESS_FORMAT_DEFLATE) {
+ext.level = s->compress.u.deflate.level;
+ext.window_size = s->compress.u.deflate.window_size;
+}
+ret = header_ext_add(buf, QCOW2_EXT_MAGIC_COMPRESS_FORMAT,
+ , sizeof(ext),
+ 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[] = {
@@ -2334,6 +2416,11 @@ int qcow2_update_header(BlockDriverState *bs)
 .name = "corrupt bit",
 },
 {
+.type = 

[Qemu-block] [PATCH V5 10/10] block/qcow2: add compress info to image specific info

[Peter Lieven]

Signed-off-by: Peter Lieven 
---
 block/qcow2.c| 7 +++
 qapi/block-core.json | 6 +-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 0e9c2b8..7e03877 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3968,6 +3968,13 @@ static ImageInfoSpecific 
*qcow2_get_specific_info(BlockDriverState *bs)
 spec_info->u.qcow2.data->encrypt = qencrypt;
 }
 
+if (s->incompatible_features & QCOW2_INCOMPAT_COMPRESS) {
+spec_info->u.qcow2.data->compress = g_new0(Qcow2Compress, 1);
+memcpy(spec_info->u.qcow2.data->compress, >compress,
+   sizeof(Qcow2Compress));
+spec_info->u.qcow2.data->has_compress = true;
+}
+
 return spec_info;
 }
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index d655bb1..3b338d5 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -68,6 +68,9 @@
 # @encrypt: details about encryption parameters; only set if image
 #   is encrypted (since 2.10)
 #
+# @compress: details about parameters for compressed clusters; only set if
+#the compress format header extension is present (since 2.11)
+#
 # Since: 1.7
 ##
 { 'struct': 'ImageInfoSpecificQCow2',
@@ -76,7 +79,8 @@
   '*lazy-refcounts': 'bool',
   '*corrupt': 'bool',
   'refcount-bits': 'int',
-  '*encrypt': 'ImageInfoSpecificQCow2Encryption'
+  '*encrypt': 'ImageInfoSpecificQCow2Encryption',
+  '*compress': 'Qcow2Compress'
   } }
 
 ##
-- 
1.9.1

[Qemu-block] [PATCH V5 09/10] block/qcow2: add lzo compress format

[Peter Lieven]

Signed-off-by: Peter Lieven 
---
 block/qcow2-cluster.c  | 15 +++
 block/qcow2.c  | 25 -
 configure  |  2 +-
 docs/interop/qcow2.txt |  2 ++
 qapi/block-core.json   | 15 ---
 qemu-img.texi  |  1 +
 6 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 345feec..f48cded 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -24,6 +24,9 @@
 
 #include "qemu/osdep.h"
 #include 
+#ifdef CONFIG_LZO
+#include 
+#endif
 
 #include "qapi/error.h"
 #include "qemu-common.h"
@@ -1504,6 +1507,18 @@ static int decompress_buffer(uint8_t *out_buf, int 
out_buf_size,
 inflateEnd(_strm);
 break;
 }
+#ifdef CONFIG_LZO
+case QCOW2_COMPRESS_FORMAT_LZO:
+out_len = out_buf_size;
+ret = lzo1x_decompress_safe(buf, buf_size, out_buf,
+(lzo_uint *) _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 6031963..0e9c2b8 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -26,6 +26,9 @@
 #include "sysemu/block-backend.h"
 #include "qemu/module.h"
 #include 
+#ifdef CONFIG_LZO
+#include 
+#endif
 #include "block/qcow2.h"
 #include "qemu/error-report.h"
 #include "qapi/qmp/qerror.h"
@@ -310,6 +313,14 @@ static int qcow2_read_extensions(BlockDriverState *bs, 
uint64_t start_offset,
 return 5;
 }
 
+#ifdef CONFIG_LZO
+if (s->compress.format == QCOW2_COMPRESS_FORMAT_LZO &&
+lzo_init() != LZO_E_OK) {
+error_setg(errp, "ERROR: internal error - lzo_init() failed");
+return 6;
+}
+#endif
+
 #ifdef DEBUG_EXT
 if (s->compress.format == QCOW2_COMPRESS_FORMAT_DEFLATE) {
 printf("Qcow2: Got compress format %s with level %" PRIu8
@@ -317,6 +328,9 @@ static int qcow2_read_extensions(BlockDriverState *bs, 
uint64_t start_offset,
Qcow2CompressFormat_lookup[s->compress.format],
s->compress.u.deflate.level,
s->compress.u.deflate.window_size);
+} else {
+printf("Qcow2: Got compress format %s\n",
+   Qcow2CompressFormat_lookup[s->compress.format]);
 }
 #endif
 break;
@@ -3428,7 +3442,7 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, 
uint64_t offset,
 struct iovec iov;
 z_stream z_strm = {};
 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) {
@@ -3477,6 +3491,14 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, 
uint64_t offset,
 
 ret = ret != Z_STREAM_END;
 break;
+#ifdef CONFIG_LZO
+case QCOW2_COMPRESS_FORMAT_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 *) _len, work_buf);
+break;
+#endif
 default:
 abort(); /* should never reach this point */
 }
@@ -3522,6 +3544,7 @@ success:
 fail:
 qemu_vfree(local_buf);
 g_free(out_buf);
+g_free(work_buf);
 return ret;
 }
 
diff --git a/configure b/configure
index 987f59b..c1cee88 100755
--- a/configure
+++ b/configure
@@ -1958,7 +1958,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/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index d0d2a8f..3262667 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -335,6 +335,8 @@ The fields of the compress format extension are:
deflate: Standard zlib deflate compression without
 compression header
 
+   lzo: LZO1X compression without additional header
+
   14:  compress_level (uint8_t)
 
0 = default compress level (valid for all formats, default)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index f652206..d655bb1 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2458,11 +2458,12 @@
 # @Qcow2CompressFormat:
 #
 # @deflate: standard zlib deflate compression
+# @lzo: lzo1x compression
 #
 # Since: 2.11
 ##
 { 'enum': 'Qcow2CompressFormat',
-  'data': [ 'deflate' ] }

[Qemu-block] [PATCH V5 01/10] specs/qcow2: add compress format extension

[Peter Lieven]

Signed-off-by: Peter Lieven 
---
 docs/interop/qcow2.txt | 51 +-
 roms/ipxe  |  2 +-
 2 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index d7fdb1f..d0d2a8f 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -86,7 +86,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
@@ -137,6 +142,7 @@ be stored. Each extension has a structure like the 
following:
 0x6803f857 - Feature name table
 0x23852875 - Bitmaps extension
 0x0537be77 - Full disk encryption header pointer
+0xC03183A3 - Compress format extension
 other  - Unknown header extension, can be safely
  ignored
 
@@ -311,6 +317,49 @@ The algorithms used for encryption vary depending on the 
method
in the LUKS header, with the physical disk sector as the
input tweak.
 
+
+== 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 - 13:  compress_format_name (padded with zeros, but not
+   necessarily null terminated if it has full length).
+   Valid compression format names currently are:
+
+   deflate: Standard zlib deflate compression without
+compression header
+
+  14:  compress_level (uint8_t)
+
+   0 = default compress level (valid for all formats, default)
+
+   Additional valid compression levels for deflate compression:
+
+   All values between 1 and 9. 1 gives best speed, 9 gives best
+   compression. The default compression level is defined by 
zlib
+   and currently defaults to 6.
+
+  15:  compress_window_size (uint8_t)
+
+   Window or dictionary size used by the compression format.
+   Currently only used by the deflate compression algorithm.
+
+   Valid window sizes for deflate compression range from 8 to
+   15 inclusively.
+
+Note: Omitting the incompatible "Compress format bit" results in the usage
+of deflate compression with default compression level and a window size of 12
+(which was default before QEMU 2.11). If exactly these parameters are choosen
+it is free to the implementation to omit the "Compress format bit" and the
+compress format extension when updating the QCOW2 header.
+
+
 == Host cluster management ==
 
 qcow2 manages the allocation of host clusters by maintaining a reference count
diff --git a/roms/ipxe b/roms/ipxe
index 0600d3a..b991c67 16
--- a/roms/ipxe
+++ b/roms/ipxe
@@ -1 +1 @@
-Subproject commit 0600d3ae94f93efd10fc6b3c7420a9557a3a1670
+Subproject commit b991c67c1d91574ef22336cc3a5944d1e63230c9
-- 
1.9.1

[Qemu-block] [PATCH V5 00/10] 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 timecompressed size   decompress time
none35.7s339MB 3.4s
zlib (default)  30.5s320MB 3.2s
zlib (level 1)  12.8s348MB 3.2s
lzo  4.2s429MB 1.6s

Changes V4->V5: (thanks to Eric for his various comments)
  patch 1: rename format from zlib -> deflate
   add windows size to header
   define default values (deflate, level 0, window_size 12)
  patch 2: move level param to Qcow2CompressDeflate
   add window-size to Qcow2CompressDeflate
  patch 3: set default values in the parameter check function, pass 
Qcow2Compress struct
  patch 4: mention compress.window-size and the old default values
  patch 5: use Qcow2Compress struct inside BDRVQcow2State
   set default values (deflate/0/12) in qcow2_do_open
  patch 9: fix docs/interop/qcow2.txt

Changes V3->V4:
  - reduce the compress format name to 15 bytes [Eric]
  - explain the default compression level for zlib [Eric]
  - explain the difference between zlib with default compression
and the old default [Eric]
  - added Patch 10

Changes V2->V3:
  - rebase to current master (qcow2 crypto extension)
  - patch 1: explicitly list valid format names, list valid compression levels,
 remove extra_data and extra_data_size for now [Kevin]
  - patch 2: don't hook the compression options in the blockdev-add options 
[Kevin, Daniel]
  - patch 3: parse compress settings from the qapi struct [Daniel]
  - patch 4: mention valid values for zlib, mention incompatiblity for QEMU < 
2.10 [Kevin]
  - patch 5: use qapi to parse the compress format from header [Daniel]
  - added patch 6

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 (10):
  specs/qcow2: add compress format extension
  qapi/block-core: add Qcow2Compress parameters
  block/qcow2: parse compress create options
  qemu-img: add documentation for compress settings
  block/qcow2: read and write the compress format extension
  block/qcow2: simplify ret usage in qcow2_create
  block/qcow2: optimize qcow2_co_pwritev_compressed
  block/qcow2: start using the compress format extension
  block/qcow2: add lzo compress format
  block/qcow2: add compress info to image specific info

 block/qcow2-cluster.c |  73 
 block/qcow2.c | 282 +++---
 block/qcow2.h |  21 +++-
 configure |   2 +-
 docs/interop/qcow2.txt|  53 -
 include/block/block_int.h |  39 ---
 qapi/block-core.json  |  55 -
 qemu-img.texi |  27 +
 roms/ipxe |   2 +-
 9 files changed, 461 insertions(+), 93 deletions(-)

-- 
1.9.1

[Qemu-block] [PATCH V5 03/10] 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 
---
 block/qcow2.c | 86 +--
 include/block/block_int.h | 39 +++--
 2 files changed, 105 insertions(+), 20 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 90efa44..a5270a6 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -31,6 +31,8 @@
 #include "qapi/qmp/qerror.h"
 #include "qapi/qmp/qbool.h"
 #include "qapi/util.h"
+#include "qapi-visit.h"
+#include "qapi/qobject-input-visitor.h"
 #include "qapi/qmp/types.h"
 #include "qapi-event.h"
 #include "trace.h"
@@ -161,6 +163,34 @@ static ssize_t qcow2_crypto_hdr_write_func(QCryptoBlock 
*block, size_t offset,
 return ret;
 }
 
+static void
+qcow2_check_compress_settings(Qcow2Compress *compress, Error **errp)
+{
+if (compress->format == QCOW2_COMPRESS_FORMAT_DEFLATE) {
+if (!compress->u.deflate.has_level) {
+compress->u.deflate.has_level = true;
+compress->u.deflate.level = 0;
+}
+if (compress->u.deflate.level > 9) {
+error_setg(errp, "Compress level %" PRIu8 " is not supported for"
+   " format '%s'", compress->u.deflate.level,
+   Qcow2CompressFormat_lookup[compress->format]);
+return;
+}
+if (!compress->u.deflate.has_window_size) {
+compress->u.deflate.has_window_size = true;
+compress->u.deflate.window_size = 15;
+}
+if (compress->u.deflate.window_size < 8 ||
+compress->u.deflate.window_size > 15) {
+error_setg(errp, "Compress window size %" PRIu8 " is not supported"
+   " for format '%s'", compress->u.deflate.window_size,
+   Qcow2CompressFormat_lookup[compress->format]);
+return;
+}
+}
+}
+
 
 /* 
  * read qcow2 extension and fill bs
@@ -2706,7 +2736,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,
- const char *encryptfmt, Error **errp)
+ const char *encryptfmt, Qcow2Compress *compress,
+ Error **errp)
 {
 QDict *options;
 
@@ -2898,6 +2929,7 @@ static int qcow2_create(const char *filename, QemuOpts 
*opts, Error **errp)
 char *backing_file = NULL;
 char *backing_fmt = NULL;
 char *buf = NULL;
+Qcow2Compress compress, *compress_ptr = NULL;
 uint64_t size = 0;
 int flags = 0;
 size_t cluster_size = DEFAULT_CLUSTER_SIZE;
@@ -2975,9 +3007,42 @@ static int qcow2_create(const char *filename, QemuOpts 
*opts, Error **errp)
 
 refcount_order = ctz32(refcount_bits);
 
+if (qemu_opt_get(opts, BLOCK_OPT_COMPRESS_FORMAT) ||
+qemu_opt_get(opts, BLOCK_OPT_COMPRESS_LEVEL) ||
+qemu_opt_get(opts, BLOCK_OPT_COMPRESS_WINDOW_SIZE)) {
+QDict *options, *compressopts = NULL;
+Visitor *v;
+options = qemu_opts_to_qdict(opts, NULL);
+qdict_extract_subqdict(options, , "compress.");
+v = qobject_input_visitor_new_keyval(QOBJECT(compressopts));
+visit_start_struct(v, NULL, NULL, 0, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+ret = -EINVAL;
+goto finish;
+}
+visit_type_Qcow2Compress_members(v, , _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);
+qcow2_check_compress_settings(, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+ret = -EINVAL;
+goto finish;
+}
+compress_ptr = 
+}
+
 ret = qcow2_create2(filename, size, backing_file, backing_fmt, flags,
 cluster_size, prealloc, opts, version, refcount_order,
-encryptfmt, _err);
+encryptfmt, compress_ptr, _err);
 error_propagate(errp, local_err);
 
 finish:
@@ -4287,6 +4352,23 @@ 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 (deflate)",
+},
+{
+.name = BLOCK_OPT_COMPRESS_LEVEL,
+.type = QEMU_OPT_NUMBER,
+.help = "Compress level used for compressed clusters 

[Qemu-block] [PATCH V5 06/10] block/qcow2: simplify ret usage in qcow2_create

[Peter Lieven]

Signed-off-by: Peter Lieven 
---
 block/qcow2.c | 13 ++---
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 7fd52e1..ffe609d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3031,7 +3031,7 @@ static int qcow2_create(const char *filename, QemuOpts 
*opts, Error **errp)
 int refcount_order;
 const char *encryptfmt = NULL;
 Error *local_err = NULL;
-int ret;
+int ret = -EINVAL;
 
 /* Read out options */
 size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
@@ -3043,7 +3043,6 @@ static int qcow2_create(const char *filename, QemuOpts 
*opts, Error **errp)
 if (qemu_opt_get_del(opts, BLOCK_OPT_ENCRYPT)) {
 error_setg(errp, "Options " BLOCK_OPT_ENCRYPT " and "
BLOCK_OPT_ENCRYPT_FORMAT " are mutually exclusive");
-ret = -EINVAL;
 goto finish;
 }
 } else if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
@@ -3052,7 +3051,6 @@ static int qcow2_create(const char *filename, QemuOpts 
*opts, Error **errp)
 cluster_size = qcow2_opt_get_cluster_size_del(opts, _err);
 if (local_err) {
 error_propagate(errp, local_err);
-ret = -EINVAL;
 goto finish;
 }
 buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC);
@@ -3061,14 +3059,12 @@ static int qcow2_create(const char *filename, QemuOpts 
*opts, Error **errp)
_err);
 if (local_err) {
 error_propagate(errp, local_err);
-ret = -EINVAL;
 goto finish;
 }
 
 version = qcow2_opt_get_version_del(opts, _err);
 if (local_err) {
 error_propagate(errp, local_err);
-ret = -EINVAL;
 goto finish;
 }
 
@@ -3079,21 +3075,18 @@ static int qcow2_create(const char *filename, QemuOpts 
*opts, Error **errp)
 if (backing_file && prealloc != PREALLOC_MODE_OFF) {
 error_setg(errp, "Backing file and preallocation cannot be used at "
"the same time");
-ret = -EINVAL;
 goto finish;
 }
 
 if (version < 3 && (flags & BLOCK_FLAG_LAZY_REFCOUNTS)) {
 error_setg(errp, "Lazy refcounts only supported with compatibility "
"level 1.1 and above (use compat=1.1 or greater)");
-ret = -EINVAL;
 goto finish;
 }
 
 refcount_bits = qcow2_opt_get_refcount_bits_del(opts, version, _err);
 if (local_err) {
 error_propagate(errp, local_err);
-ret = -EINVAL;
 goto finish;
 }
 
@@ -3110,13 +3103,11 @@ static int qcow2_create(const char *filename, QemuOpts 
*opts, Error **errp)
 visit_start_struct(v, NULL, NULL, 0, _err);
 if (local_err) {
 error_propagate(errp, local_err);
-ret = -EINVAL;
 goto finish;
 }
 visit_type_Qcow2Compress_members(v, , _err);
 if (local_err) {
 error_propagate(errp, local_err);
-ret = -EINVAL;
 goto finish;
 }
 visit_end_struct(v, NULL);
@@ -3126,12 +3117,12 @@ static int qcow2_create(const char *filename, QemuOpts 
*opts, Error **errp)
 qcow2_check_compress_settings(, _err);
 if (local_err) {
 error_propagate(errp, local_err);
-ret = -EINVAL;
 goto finish;
 }
 compress_ptr = 
 }
 
+/* ret is still -EINVAL until here */
 ret = qcow2_create2(filename, size, backing_file, backing_fmt, flags,
 cluster_size, prealloc, opts, version, refcount_order,
 encryptfmt, compress_ptr, _err);
-- 
1.9.1

[Qemu-block] [PATCH V5 02/10] qapi/block-core: add Qcow2Compress parameters

[Peter Lieven]

Signed-off-by: Peter Lieven 
---
 qapi/block-core.json | 40 
 1 file changed, 40 insertions(+)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 833c602..f652206 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2455,6 +2455,46 @@
 '*encrypt': 'BlockdevQcow2Encryption' } }
 
 ##
+# @Qcow2CompressFormat:
+#
+# @deflate: standard zlib deflate compression
+#
+# Since: 2.11
+##
+{ 'enum': 'Qcow2CompressFormat',
+  'data': [ 'deflate' ] }
+
+##
+# @Qcow2CompressDeflate:
+#
+# @level: specifies the compression level. 0 = default compression,
+# 1 = fastest compression, 9 = best compresion
+# @window-size: specifies the window size used for deflate compression.
+#   8...15 = window size of 2^8 to 2^15 byte (default)
+#
+# Since: 2.11
+##
+{ 'struct': 'Qcow2CompressDeflate',
+  'data': { '*level': 'uint8',
+'*window-size': 'uint8' } }
+
+##
+# @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)
+#
+# Since: 2.11
+##
+{ 'union': 'Qcow2Compress',
+  'base': { 'format': 'Qcow2CompressFormat' },
+  'discriminator': 'format',
+  'data': { 'deflate': 'Qcow2CompressDeflate' } }
+
+
+##
 # @BlockdevOptionsSsh:
 #
 # @server:  host address
-- 
1.9.1

Re: [Qemu-block] [PATCH for-2.10 0/2] Test for bug fixes from byte-based block status

[Stefan Hajnoczi]

On Mon, Jul 24, 2017 at 10:39:50AM -0500, Eric Blake wrote:
> Kevin is correct, and I need tests of my recent bug fixes :)
> 
> https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg07235.html
> 
> Eric Blake (2):
>   iotests: Check dirty bitmap statistics in 124
>   iotests: Add test of recent fix to 'qemu-img measure'
> 
>  tests/qemu-iotests/124 |  7 +-
>  tests/qemu-iotests/190 | 59 
> ++
>  tests/qemu-iotests/190.out | 11 +
>  tests/qemu-iotests/group   |  1 +
>  4 files changed, 77 insertions(+), 1 deletion(-)
>  create mode 100755 tests/qemu-iotests/190
>  create mode 100644 tests/qemu-iotests/190.out
> 
> -- 
> 2.13.3
> 
> 

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-block] [Qemu-devel] [PATCH] qemu-iotests: Fix reference output for 186

[Stefan Hajnoczi]

On Tue, Jul 25, 2017 at 10:59:36AM +0200, Kevin Wolf wrote:
> Commits 70f17a1 ('error: Revert unwanted change of warning messages')
> and e1824e5 ('qemu-iotests: Test 'info block'') had a semantic merge
> conflict, which results in failure for qemu-iotests case 186. Fix the
> reference output to consider the changes of 70f17a1.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/186.out | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-block] [Qemu-devel] [PATCH] qemu-iotests: Fix reference output for 186

[Markus Armbruster]

Kevin Wolf  writes:

> Commits 70f17a1 ('error: Revert unwanted change of warning messages')
> and e1824e5 ('qemu-iotests: Test 'info block'') had a semantic merge
> conflict, which results in failure for qemu-iotests case 186. Fix the
> reference output to consider the changes of 70f17a1.
>
> Signed-off-by: Kevin Wolf 

Reviewed-by: Markus Armbruster 

Re: [Qemu-block] [PATCH 4/7] block: convert ThrottleGroup to object with QOM

[Manos Pitsidianakis]

On Mon, Jul 24, 2017 at 04:12:47PM +0100, Stefan Hajnoczi wrote:

On Fri, Jul 14, 2017 at 12:45:18PM +0300, Manos Pitsidianakis wrote:

ThrottleGroup is converted to an object. This will allow the future
throttle block filter drive easy creation and configuration of throttle
groups in QMP and cli.

A new QAPI struct, ThrottleLimits, is introduced to provide a shared
struct for all throttle configuration needs in QMP.

ThrottleGroups can be created via CLI as
-object throttling-group,id=foo,x-iops-total=100,x-..


Please make the QOM name and struct name consistent.  Either
ThrottleGroup/throttle-group or ThrottlingGroup/throttling-group but not
ThrottleGroup/throttling-group.


I did this on purpose because current throttling has ThrottleGroup 
internally and throttling.group in the command line. Should we keep this 
only in legacy and make it throttle-group everywhere?



where x-* are individual limit properties. Since we can't add non-scalar
properties in -object this interface must be used instead. However,
setting these properties must be disabled after initialization because
certain combinations of limits are forbidden and thus configuration
changes should be done in one transaction. The individual properties
will go away when support for non-scalar values in CLI is implemented
and thus are marked as experimental.

ThrottleGroup also has a `limits` property that uses the ThrottleLimits
struct.  It can be used to create ThrottleGroups or set the
configuration in existing groups as follows:

{ "execute": "object-add",
  "arguments": {
"qom-type": "throttling-group",
"id": "foo",
"props" : {
  "limits": {
  "iops-total": 100
  }
}
  }
}
{ "execute" : "qom-set",
"arguments" : {
"path" : "foo",
"property" : "limits",
"value" : {
"iops-total" : 99
}
}
}

This also means a group's configuration can be fetched with qom-get.

ThrottleGroups can be anonymous which means they can't get accessed by
other users ie they will always be units instead of group (Because they
have one ThrottleGroupMember).

Signed-off-by: Manos Pitsidianakis 
---

Notes:
Note: I tested Markus Armbruster's patch in 
<87wp7fghi9@dusky.pond.sub.org>
on master and I can use this syntax successfuly:
-object '{ "qom-type" : "throttling-group", "id" : "foo", "props" : { 
"limits" \
: { "iops-total" : 1000 } } }'
If this gets merged using -object will be a little more verbose but at 
least we
won't have seperate properties, which is a good thing, so the x-* should be
dropped.

 block/throttle-groups.c | 507 +---
 include/block/throttle-groups.h |   3 +
 include/qemu/throttle-options.h |  59 +++--
 include/qemu/throttle.h |   3 +
 qapi/block-core.json|  45 
 tests/test-throttle.c   |   1 +
 util/throttle.c | 121 ++
 7 files changed, 689 insertions(+), 50 deletions(-)

diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index f711a3dc62..4d1f82ec06 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -25,9 +25,17 @@
 #include "qemu/osdep.h"
 #include "sysemu/block-backend.h"
 #include "block/throttle-groups.h"
+#include "qemu/throttle-options.h"
 #include "qemu/queue.h"
 #include "qemu/thread.h"
 #include "sysemu/qtest.h"
+#include "qapi/error.h"
+#include "qapi-visit.h"
+#include "qom/object.h"
+#include "qom/object_interfaces.h"
+
+static void throttle_group_obj_init(Object *obj);
+static void throttle_group_obj_complete(UserCreatable *obj, Error **errp);

 /* The ThrottleGroup structure (with its ThrottleState) is shared
  * among different ThrottleGroupMembers and it's independent from
@@ -54,6 +62,10 @@
  * that ThrottleGroupMember has throttled requests in the queue.
  */
 typedef struct ThrottleGroup {
+Object parent_obj;
+
+/* refuse individual property change if initialization is complete */
+bool is_initialized;
 char *name; /* This is constant during the lifetime of the group */

 QemuMutex lock; /* This lock protects the following four fields */
@@ -63,8 +75,7 @@ typedef struct ThrottleGroup {
 bool any_timer_armed[2];
 QEMUClockType clock_type;

-/* These two are protected by the global throttle_groups_lock */
-unsigned refcount;
+/* This is protected by the global throttle_groups_lock */
 QTAILQ_ENTRY(ThrottleGroup) list;
 } ThrottleGroup;

@@ -77,7 +88,8 @@ static QTAILQ_HEAD(, ThrottleGroup) throttle_groups =
  * If no ThrottleGroup is found with the given name a new one is
  * created.
  *
- * @name: the name of the ThrottleGroup
+ * @name: the name of the ThrottleGroup, NULL means a new anonymous group will
+ *be created.
  * @ret:  the ThrottleState member of the ThrottleGroup
  */
 ThrottleState *throttle_group_incref(const char *name)
@@ -87,32 +99,30 @@ ThrottleState *throttle_group_incref(const char 

Re: [Qemu-block] [Qemu-devel] [PATCH v3] util: remove the obsolete non-blocking connect

[Daniel P. Berrange]

On Tue, Jul 25, 2017 at 08:18:31AM +0200, Markus Armbruster wrote:
> Mao Zhongyi  writes:
> 
> > On 07/25/2017 06:07 AM, John Snow wrote:
> >> This was posted over a month ago with two R-Bs, did it get merged or 
> >> dropped?
> >>
> >> --js
> >
> > Not yet, I hope that it will.
> 
> get_maintainers.pl blames Daniel, Gerd and Paolo :)

Yeah, I totally forgot that I was listed as a maintainer of the
util/qemu-sockets.c file when I reviewed this & assumed Gerd/Paolo
would take it.

I'll put it on my merge queue right now, but unfortunately I can't
justify sending this during freeze as it isn't a bugfix, so it'll
have to wait until 2.11 opens.

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 :|

[Qemu-block] [PATCH] qemu-iotests: Fix reference output for 186

[Kevin Wolf]

Commits 70f17a1 ('error: Revert unwanted change of warning messages')
and e1824e5 ('qemu-iotests: Test 'info block'') had a semantic merge
conflict, which results in failure for qemu-iotests case 186. Fix the
reference output to consider the changes of 70f17a1.

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/186.out | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/186.out b/tests/qemu-iotests/186.out
index b963b12..b8bf9a2 100644
--- a/tests/qemu-iotests/186.out
+++ b/tests/qemu-iotests/186.out
@@ -442,7 +442,7 @@ ide0-cd0 (NODE_NAME): null-co:// (null-co, read-only)
 Cache mode:   writeback
 (qemu) quit
 
-warning: qemu-system-x86_64: -drive if=scsi,driver=null-co: bus=0,unit=0 is 
deprecated with this machine type
+qemu-system-x86_64: -drive if=scsi,driver=null-co: warning: bus=0,unit=0 is 
deprecated with this machine type
 Testing: -drive if=scsi,driver=null-co
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) info block
@@ -451,7 +451,7 @@ scsi0-hd0 (NODE_NAME): null-co:// (null-co)
 Cache mode:   writeback
 (qemu) quit
 
-warning: qemu-system-x86_64: -drive if=scsi,media=cdrom: bus=0,unit=0 is 
deprecated with this machine type
+qemu-system-x86_64: -drive if=scsi,media=cdrom: warning: bus=0,unit=0 is 
deprecated with this machine type
 Testing: -drive if=scsi,media=cdrom
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) info block
@@ -460,7 +460,7 @@ scsi0-cd0: [not inserted]
 Removable device: not locked, tray closed
 (qemu) quit
 
-warning: qemu-system-x86_64: -drive if=scsi,driver=null-co,media=cdrom: 
bus=0,unit=0 is deprecated with this machine type
+qemu-system-x86_64: -drive if=scsi,driver=null-co,media=cdrom: warning: 
bus=0,unit=0 is deprecated with this machine type
 Testing: -drive if=scsi,driver=null-co,media=cdrom
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) info block
-- 
1.8.3.1

Re: [Qemu-block] [Qemu-devel] [PATCH v3] util: remove the obsolete non-blocking connect

[Markus Armbruster]

Mao Zhongyi  writes:

> On 07/25/2017 06:07 AM, John Snow wrote:
>> This was posted over a month ago with two R-Bs, did it get merged or dropped?
>>
>> --js
>
> Not yet, I hope that it will.

get_maintainers.pl blames Daniel, Gerd and Paolo :)