Re: [Qemu-block] [Qemu-devel] [PATCH for-2.12 1/3] qapi: Add qdict_is_null()

[Markus Armbruster]

Max Reitz  writes:

> On 2017-11-14 15:57, Markus Armbruster wrote:
>> Max Reitz  writes:
>> 
>>> Signed-off-by: Max Reitz 
>>> ---
>>>  include/qapi/qmp/qdict.h |  1 +
>>>  qobject/qdict.c  | 10 ++
>>>  2 files changed, 11 insertions(+)
>>>
>>> diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
>>> index fc218e7be6..c65ebfc748 100644
>>> --- a/include/qapi/qmp/qdict.h
>>> +++ b/include/qapi/qmp/qdict.h
>>> @@ -76,6 +76,7 @@ int64_t qdict_get_try_int(const QDict *qdict, const char 
>>> *key,
>>>int64_t def_value);
>>>  bool qdict_get_try_bool(const QDict *qdict, const char *key, bool 
>>> def_value);
>>>  const char *qdict_get_try_str(const QDict *qdict, const char *key);
>>> +bool qdict_is_qnull(const QDict *qdict, const char *key);
>>>  
>>>  void qdict_copy_default(QDict *dst, QDict *src, const char *key);
>>>  void qdict_set_default_str(QDict *dst, const char *key, const char *val);
>>> diff --git a/qobject/qdict.c b/qobject/qdict.c
>>> index e8f15f1132..a032ea629a 100644
>>> --- a/qobject/qdict.c
>>> +++ b/qobject/qdict.c
>>> @@ -294,6 +294,16 @@ const char *qdict_get_try_str(const QDict *qdict, 
>>> const char *key)
>>>  }
>>>  
>>>  /**
>>> + * qdict_is_qnull(): Return true if the value for 'key' is QNull
>>> + */
>>> +bool qdict_is_qnull(const QDict *qdict, const char *key)
>>> +{
>>> +QObject *value = qdict_get(qdict, key);
>>> +
>>> +return value && value->type == QTYPE_QNULL;
>>> +}
>>> +
>>> +/**
>>>   * qdict_iter(): Iterate over all the dictionary's stored values.
>>>   *
>>>   * This function allows the user to provide an iterator, which will be
>> 
>> As far as I can tell, the new helper function is going to be used just
>> once, by bdrv_open_inherit() in PATCH 2:
>> 
>> qdict_is_qnull(options, "backing")
>> 
>> I dislike abstracting from just one concrete instance.
>> 
>> Perhaps a more general helper could be more generally useful.  Something
>> like:
>> 
>> qobject_is(qdict_get(options, "backing", QTYPE_QNULL))
>> 
>> There are numerous instances of
>> 
>> !obj || qobject_type(obj) == T
>> 
>> in the tree, which could then be replaced by
>> 
>> qobject_is(obj, T)
>> 
>> An alternative helper: macro qobject_dynamic_cast(obj, T) that returns
>> (T *)obj if obj is a T, else null.  Leads to something like
>> 
>> qobject_dynamic_cast(qdict_get(options, "backing", QNull))
>
> If you think that's good, then that's good -- you know the QAPI code
> better then me, after all.

I'll play with it today.

> To explain myself: I thought it would be the natural extension of the
> qdict_get_try_*() functions for the QNull type.

I see now.  The name qdict_get_try_null() would make it obvious, but
what to return on success...

Re: [Qemu-block] [Qemu-devel] [PATCH v8 10/14] migration: add postcopy migration of dirty bitmaps

[John Snow]

On 10/30/2017 12:33 PM, Vladimir Sementsov-Ogievskiy wrote:
> Postcopy migration of dirty bitmaps. Only named dirty bitmaps,
> associated with root nodes and non-root named nodes are migrated.
> 
> If destination qemu is already containing a dirty bitmap with the same name
> as a migrated bitmap (for the same node), then, if their granularities are
> the same the migration will be done, otherwise the error will be generated.
> 
> If destination qemu doesn't contain such bitmap it will be created.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  include/migration/misc.h   |   3 +
>  migration/migration.h  |   3 +
>  migration/block-dirty-bitmap.c | 734 
> +

Ouch :\

>  migration/migration.c  |   3 +
>  migration/savevm.c |   2 +
>  vl.c   |   1 +
>  migration/Makefile.objs|   1 +
>  migration/trace-events |  14 +
>  8 files changed, 761 insertions(+)
>  create mode 100644 migration/block-dirty-bitmap.c
> 

Organizationally, you introduce three new 'public' prototypes:

dirty_bitmap_mig_init
dirty_bitmap_mig_before_vm_start
init_dirty_bitmap_incoming_migration

mig_init is advertised in migration/misc.h, the other two are in
migration/migration.h.
The definitions for all three are in migration/block-dirty-bitmap.c

In pure naivety, I find it weird to have something that you use in
migration.c and advertised in migration.h actually exist separately in
block-dirty-bitmap.c; but maybe this is the sanest thing to do.

> diff --git a/include/migration/misc.h b/include/migration/misc.h
> index c079b7771b..9cc539e232 100644
> --- a/include/migration/misc.h
> +++ b/include/migration/misc.h
> @@ -55,4 +55,7 @@ bool migration_has_failed(MigrationState *);
>  bool migration_in_postcopy_after_devices(MigrationState *);
>  void migration_global_dump(Monitor *mon);
>  
> +/* migration/block-dirty-bitmap.c */
> +void dirty_bitmap_mig_init(void);
> +
>  #endif
> diff --git a/migration/migration.h b/migration/migration.h
> index 50d1f01346..4e3ad04664 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -211,4 +211,7 @@ void migrate_send_rp_pong(MigrationIncomingState *mis,
>  void migrate_send_rp_req_pages(MigrationIncomingState *mis, const char* 
> rbname,
>ram_addr_t start, size_t len);
>  
> +void dirty_bitmap_mig_before_vm_start(void);
> +void init_dirty_bitmap_incoming_migration(void);
> +
>  #endif
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> new file mode 100644
> index 00..53cb20045d
> --- /dev/null
> +++ b/migration/block-dirty-bitmap.c
> @@ -0,0 +1,734 @@
> +/*
> + * Block dirty bitmap postcopy migration
> + *
> + * Copyright IBM, Corp. 2009
> + * Copyright (c) 2016-2017 Parallels International GmbH
> + *
> + * Authors:
> + *  Liran Schour   
> + *  Vladimir Sementsov-Ogievskiy 
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + * This file is derived from migration/block.c, so it's author and IBM 
> copyright
> + * are here, although content is quite different.
> + *
> + * Contributions after 2012-01-13 are licensed under the terms of the
> + * GNU GPL, version 2 or (at your option) any later version.
> + *
> + ****
> + *
> + * Here postcopy migration of dirty bitmaps is realized. Only named dirty
> + * bitmaps, associated with root nodes and non-root named nodes are migrated.

Put another way, only QMP-addressable bitmaps. Nodes without a name that
are not the root have no way to be addressed.

> + *
> + * If destination qemu is already containing a dirty bitmap with the same 
> name

"If the destination QEMU already contains a dirty bitmap with the same name"

> + * as a migrated bitmap (for the same node), then, if their granularities are
> + * the same the migration will be done, otherwise the error will be 
> generated.

"an error"

> + *
> + * If destination qemu doesn't contain such bitmap it will be created.

"If the destination QEMU doesn't contain such a bitmap, it will be created."

> + *
> + * format of migration:
> + *
> + * # Header (shared for different chunk types)
> + * 1, 2 or 4 bytes: flags (see qemu_{put,put}_flags)
> + * [ 1 byte: node name size ] \  flags & DEVICE_NAME
> + * [ n bytes: node name ] /
> + * [ 1 byte: bitmap name size ] \  flags & BITMAP_NAME
> + * [ n bytes: bitmap name ] /
> + *
> + * # Start of bitmap migration (flags & START)
> + * header
> + * be64: granularity
> + * 1 byte: bitmap flags (corresponds to BdrvDirtyBitmap)
> + *   bit 0-  bitmap is enabled
> + *   bit 1-  bitmap is persistent
> + *   bit 2-  bitmap is autoloading
> + *   bits 3-7 - reserved, must be zero
> + *
> + * # Complete of bitmap migration (flags & COMPLETE)
> + * header
> + *
> + 

Re: [Qemu-block] [PATCH for-2.11] util/stats64: Fix min/max comparisons

[Paolo Bonzini]

- Max Reitz  ha scritto:
> stat64_min_slow() and stat64_max_slow() compare the wrong way.  This
> makes iotest 136 fail with clang and -m32.

Queued, thanks.

Cc: qemu-sta...@nongnu.org

Paolo

> Signed-off-by: Max Reitz 
> ---
>  util/stats64.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/util/stats64.c b/util/stats64.c
> index 9968fcceac..389c365a9e 100644
> --- a/util/stats64.c
> +++ b/util/stats64.c
> @@ -91,7 +91,7 @@ bool stat64_min_slow(Stat64 *s, uint64_t value)
>  low = atomic_read(>low);
>  
>  orig = ((uint64_t)high << 32) | low;
> -if (orig < value) {
> +if (value < orig) {
>  /* We have to set low before high, just like stat64_min reads
>   * high before low.  The value may become higher temporarily, but
>   * stat64_get does not notice (it takes the lock) and the only ill
> @@ -120,7 +120,7 @@ bool stat64_max_slow(Stat64 *s, uint64_t value)
>  low = atomic_read(>low);
>  
>  orig = ((uint64_t)high << 32) | low;
> -if (orig > value) {
> +if (value > orig) {
>  /* We have to set low before high, just like stat64_max reads
>   * high before low.  The value may become lower temporarily, but
>   * stat64_get does not notice (it takes the lock) and the only ill
> -- 
> 2.13.6
> 

Re: [Qemu-block] [PATCH for-2.11? v7 0/6] block: Don't compare strings in bdrv_reopen_prepare()

[Max Reitz]

On 2017-11-14 19:01, Max Reitz wrote:
> bdrv_reopen_prepare() assumes that all BDS options are strings, which is
> not necessarily correct. This series introduces a new qobject_is_equal()
> function which can be used to test whether any options have changed,
> independently of their type.

Aaand once again applied to my block branch.

(As always, thank you for reviewing, Eric -- I'm happy to have expanded
your knowledge on obscure C behavior. :-))

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH] qapi: block-core: Clarify events emitted by 'block-job-cancel'

[no-reply]

Hi,

This series failed build test on ppc host. Please find the details below.

Type: series
Subject: [Qemu-devel] [PATCH] qapi: block-core: Clarify events emitted by 
'block-job-cancel'
Message-id: 20171114191605.22349-1-kcham...@redhat.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
# Testing script will be invoked under the git checkout with
# HEAD pointing to a commit that has the patches applied on top of "base"
# branch
set -e
echo "=== ENV ==="
env
echo "=== PACKAGES ==="
rpm -qa
echo "=== TEST BEGIN ==="
INSTALL=$PWD/install
BUILD=$PWD/build
mkdir -p $BUILD $INSTALL
SRC=$PWD
cd $BUILD
$SRC/configure --prefix=$INSTALL
make -j100
# XXX: we need reliable clean up
# make check -j100 V=1
make install
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag] patchew/20171114191605.22349-1-kcham...@redhat.com -> 
patchew/20171114191605.22349-1-kcham...@redhat.com
Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc'
Submodule 'pixman' (git://anongit.freedesktop.org/pixman) registered for path 
'pixman'
Submodule 'roms/SLOF' (git://git.qemu-project.org/SLOF.git) registered for path 
'roms/SLOF'
Submodule 'roms/ipxe' (git://git.qemu-project.org/ipxe.git) registered for path 
'roms/ipxe'
Submodule 'roms/openbios' (git://git.qemu-project.org/openbios.git) registered 
for path 'roms/openbios'
Submodule 'roms/openhackware' (git://git.qemu-project.org/openhackware.git) 
registered for path 'roms/openhackware'
Submodule 'roms/qemu-palcode' (git://github.com/rth7680/qemu-palcode.git) 
registered for path 'roms/qemu-palcode'
Submodule 'roms/seabios' (git://git.qemu-project.org/seabios.git/) registered 
for path 'roms/seabios'
Submodule 'roms/sgabios' (git://git.qemu-project.org/sgabios.git) registered 
for path 'roms/sgabios'
Submodule 'roms/u-boot' (git://git.qemu-project.org/u-boot.git) registered for 
path 'roms/u-boot'
Submodule 'roms/vgabios' (git://git.qemu-project.org/vgabios.git/) registered 
for path 'roms/vgabios'
Cloning into 'dtc'...
Submodule path 'dtc': checked out '65cc4d2748a2c2e6f27f1cf39e07a5dbabd80ebf'
Cloning into 'pixman'...
Submodule path 'pixman': checked out '87eea99e443b389c978cf37efc52788bf03a0ee0'
Cloning into 'roms/SLOF'...
Submodule path 'roms/SLOF': checked out 
'e3d05727a074619fc12d0a67f05cf2c42c875cce'
Cloning into 'roms/ipxe'...
Submodule path 'roms/ipxe': checked out 
'04186319181298083ef28695a8309028b26fe83c'
Cloning into 'roms/openbios'...
Submodule path 'roms/openbios': checked out 
'e79bca64838c96ec44fd7acd508879c5284233dd'
Cloning into 'roms/openhackware'...
Submodule path 'roms/openhackware': checked out 
'c559da7c8eec5e45ef1f67978827af6f0b9546f5'
Cloning into 'roms/qemu-palcode'...
Submodule path 'roms/qemu-palcode': checked out 
'c87a92639b28ac42bc8f6c67443543b405dc479b'
Cloning into 'roms/seabios'...
Submodule path 'roms/seabios': checked out 
'e2fc41e24ee0ada60fc511d60b15a41b294538be'
Cloning into 'roms/sgabios'...
Submodule path 'roms/sgabios': checked out 
'23d474943dcd55d0550a3d20b3d30e9040a4f15b'
Cloning into 'roms/u-boot'...
Submodule path 'roms/u-boot': checked out 
'2072e7262965bb48d7fffb1e283101e6ed8b21a8'
Cloning into 'roms/vgabios'...
Submodule path 'roms/vgabios': checked out 
'19ea12c230ded95928ecaef0db47a82231c2e485'
warning: unable to rmdir pixman: Directory not empty
Switched to a new branch 'test'
M   dtc
M   roms/SLOF
M   roms/ipxe
M   roms/openbios
M   roms/qemu-palcode
M   roms/seabios
M   roms/sgabios
M   roms/u-boot
375c460 qapi: block-core: Clarify events emitted by 'block-job-cancel'

=== OUTPUT BEGIN ===
=== ENV ===
XDG_SESSION_ID=31750
SHELL=/bin/sh
USER=patchew
PATCHEW=/home/patchew/patchew/patchew-cli -s http://patchew.org --nodebug
PATH=/usr/bin:/bin
PWD=/var/tmp/patchew-tester-tmp-x1o6jiab/src
LANG=en_US.UTF-8
HOME=/home/patchew
SHLVL=2
LOGNAME=patchew
XDG_RUNTIME_DIR=/run/user/1000
_=/usr/bin/env
=== PACKAGES ===
plymouth-core-libs-0.8.9-0.28.20140113.el7.centos.ppc64le
vim-common-7.4.160-2.el7.ppc64le
perl-Test-Simple-0.98-243.el7.noarch
hplip-common-3.15.9-3.el7.ppc64le
valgrind-3.12.0-8.el7.ppc64le
gamin-0.1.10-16.el7.ppc64le
libpeas-loader-python-1.20.0-1.el7.ppc64le
telepathy-filesystem-0.0.2-6.el7.noarch
colord-libs-1.3.4-1.el7.ppc64le
kbd-legacy-1.15.5-13.el7.noarch
perl-CPAN-Meta-YAML-0.008-14.el7.noarch
libvirt-daemon-driver-nwfilter-3.2.0-14.el7.ppc64le
ntsysv-1.7.4-1.el7.ppc64le
kernel-bootwrapper-3.10.0-693.el7.ppc64le
telepathy-farstream-0.6.0-5.el7.ppc64le
kdenetwork-common-4.10.5-8.el7_0.noarch
elfutils-devel-0.168-8.el7.ppc64le
pm-utils-1.4.1-27.el7.ppc64le
perl-Error-0.17020-2.el7.noarch
usbmuxd-1.1.0-1.el7.ppc64le
bzip2-devel-1.0.6-13.el7.ppc64le
blktrace-1.0.5-8.el7.ppc64le
gnome-keyring-pam-3.20.0-3.el7.ppc64le
tzdata-java-2017b-1.el7.noarch
perl-devel-5.16.3-292.el7.ppc64le
gnome-getting-started-docs-3.22.0-1.el7.noarch
perl-Log-Message-Simple-0.10-2.el7.noarch

[Qemu-block] [PATCH for-2.11] util/stats64: Fix min/max comparisons

[Max Reitz]

stat64_min_slow() and stat64_max_slow() compare the wrong way.  This
makes iotest 136 fail with clang and -m32.

Signed-off-by: Max Reitz 
---
 util/stats64.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/util/stats64.c b/util/stats64.c
index 9968fcceac..389c365a9e 100644
--- a/util/stats64.c
+++ b/util/stats64.c
@@ -91,7 +91,7 @@ bool stat64_min_slow(Stat64 *s, uint64_t value)
 low = atomic_read(>low);
 
 orig = ((uint64_t)high << 32) | low;
-if (orig < value) {
+if (value < orig) {
 /* We have to set low before high, just like stat64_min reads
  * high before low.  The value may become higher temporarily, but
  * stat64_get does not notice (it takes the lock) and the only ill
@@ -120,7 +120,7 @@ bool stat64_max_slow(Stat64 *s, uint64_t value)
 low = atomic_read(>low);
 
 orig = ((uint64_t)high << 32) | low;
-if (orig > value) {
+if (value > orig) {
 /* We have to set low before high, just like stat64_max reads
  * high before low.  The value may become lower temporarily, but
  * stat64_get does not notice (it takes the lock) and the only ill
-- 
2.13.6

Re: [Qemu-block] [Qemu-devel] [PATCH] qapi: block-core: Clarify events emitted by 'block-job-cancel'

[no-reply]

Hi,

This series failed build test on s390x host. Please find the details below.

Type: series
Subject: [Qemu-devel] [PATCH] qapi: block-core: Clarify events emitted by 
'block-job-cancel'
Message-id: 20171114191605.22349-1-kcham...@redhat.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
# Testing script will be invoked under the git checkout with
# HEAD pointing to a commit that has the patches applied on top of "base"
# branch
set -e
echo "=== ENV ==="
env
echo "=== PACKAGES ==="
rpm -qa
echo "=== TEST BEGIN ==="
CC=$HOME/bin/cc
INSTALL=$PWD/install
BUILD=$PWD/build
echo -n "Using CC: "
realpath $CC
mkdir -p $BUILD $INSTALL
SRC=$PWD
cd $BUILD
$SRC/configure --cc=$CC --prefix=$INSTALL
make -j4
# XXX: we need reliable clean up
# make check -j4 V=1
make install
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag] patchew/20171114191605.22349-1-kcham...@redhat.com -> 
patchew/20171114191605.22349-1-kcham...@redhat.com
Switched to a new branch 'test'
375c460 qapi: block-core: Clarify events emitted by 'block-job-cancel'

=== OUTPUT BEGIN ===
=== ENV ===
XDG_SESSION_ID=91315
SHELL=/bin/sh
USER=fam
PATCHEW=/home/fam/patchew/patchew-cli -s http://patchew.org --nodebug
PATH=/usr/bin:/bin
PWD=/var/tmp/patchew-tester-tmp-ki994nis/src
LANG=en_US.UTF-8
HOME=/home/fam
SHLVL=2
LOGNAME=fam
DBUS_SESSION_BUS_ADDRESS=unix:path=/run/user/1012/bus
XDG_RUNTIME_DIR=/run/user/1012
_=/usr/bin/env
=== PACKAGES ===
gpg-pubkey-873529b8-54e386ff
xz-libs-5.2.2-2.fc24.s390x
libxshmfence-1.2-3.fc24.s390x
giflib-4.1.6-15.fc24.s390x
trousers-lib-0.3.13-6.fc24.s390x
ncurses-base-6.0-6.20160709.fc25.noarch
gmp-6.1.1-1.fc25.s390x
libidn-1.33-1.fc25.s390x
slang-2.3.0-7.fc25.s390x
pkgconfig-0.29.1-1.fc25.s390x
alsa-lib-1.1.1-2.fc25.s390x
yum-metadata-parser-1.1.4-17.fc25.s390x
python3-slip-dbus-0.6.4-4.fc25.noarch
python2-cssselect-0.9.2-1.fc25.noarch
createrepo_c-libs-0.10.0-6.fc25.s390x
initscripts-9.69-1.fc25.s390x
parted-3.2-21.fc25.s390x
flex-2.6.0-3.fc25.s390x
colord-libs-1.3.4-1.fc25.s390x
python-osbs-client-0.33-3.fc25.noarch
perl-Pod-Simple-3.35-1.fc25.noarch
python2-simplejson-3.10.0-1.fc25.s390x
brltty-5.4-2.fc25.s390x
librados2-10.2.4-2.fc25.s390x
tcp_wrappers-7.6-83.fc25.s390x
libcephfs_jni1-10.2.4-2.fc25.s390x
nettle-devel-3.3-1.fc25.s390x
bzip2-devel-1.0.6-21.fc25.s390x
libuuid-2.28.2-2.fc25.s390x
python3-dnf-1.1.10-6.fc25.noarch
texlive-kpathsea-doc-svn41139-33.fc25.1.noarch
openssh-7.4p1-4.fc25.s390x
texlive-kpathsea-bin-svn40473-33.20160520.fc25.1.s390x
texlive-graphics-svn41015-33.fc25.1.noarch
texlive-dvipdfmx-def-svn40328-33.fc25.1.noarch
texlive-mfware-svn40768-33.fc25.1.noarch
texlive-texlive-scripts-svn41433-33.fc25.1.noarch
texlive-euro-svn22191.1.1-33.fc25.1.noarch
texlive-etex-svn37057.0-33.fc25.1.noarch
texlive-iftex-svn29654.0.2-33.fc25.1.noarch
texlive-palatino-svn31835.0-33.fc25.1.noarch
texlive-texlive-docindex-svn41430-33.fc25.1.noarch
texlive-xunicode-svn30466.0.981-33.fc25.1.noarch
texlive-koma-script-svn41508-33.fc25.1.noarch
texlive-pst-grad-svn15878.1.06-33.fc25.1.noarch
texlive-pst-blur-svn15878.2.0-33.fc25.1.noarch
texlive-jknapltx-svn19440.0-33.fc25.1.noarch
texinfo-6.1-4.fc25.s390x
openssl-devel-1.0.2k-1.fc25.s390x
jansson-2.10-2.fc25.s390x
fedora-repos-25-4.noarch
perl-Errno-1.25-387.fc25.s390x
acl-2.2.52-13.fc25.s390x
systemd-pam-231-17.fc25.s390x
NetworkManager-libnm-1.4.4-5.fc25.s390x
poppler-0.45.0-5.fc25.s390x
ccache-3.3.4-1.fc25.s390x
valgrind-3.12.0-9.fc25.s390x
perl-open-1.10-387.fc25.noarch
libgcc-6.4.1-1.fc25.s390x
libsoup-2.56.1-1.fc25.s390x
libstdc++-devel-6.4.1-1.fc25.s390x
libobjc-6.4.1-1.fc25.s390x
python2-rpm-4.13.0.1-2.fc25.s390x
python2-gluster-3.10.5-1.fc25.s390x
rpm-build-4.13.0.1-2.fc25.s390x
glibc-static-2.24-10.fc25.s390x
lz4-1.8.0-1.fc25.s390x
xapian-core-libs-1.2.24-1.fc25.s390x
elfutils-libelf-devel-0.169-1.fc25.s390x
nss-softokn-3.32.0-1.2.fc25.s390x
pango-1.40.9-1.fc25.s390x
glibc-debuginfo-common-2.24-10.fc25.s390x
libaio-0.3.110-6.fc24.s390x
libfontenc-1.1.3-3.fc24.s390x
lzo-2.08-8.fc24.s390x
isl-0.14-5.fc24.s390x
libXau-1.0.8-6.fc24.s390x
linux-atm-libs-2.5.1-14.fc24.s390x
libXext-1.3.3-4.fc24.s390x
libXxf86vm-1.1.4-3.fc24.s390x
bison-3.0.4-4.fc24.s390x
perl-srpm-macros-1-20.fc25.noarch
gawk-4.1.3-8.fc25.s390x
libwayland-client-1.12.0-1.fc25.s390x
perl-Exporter-5.72-366.fc25.noarch
perl-version-0.99.17-1.fc25.s390x
fftw-libs-double-3.3.5-3.fc25.s390x
libssh2-1.8.0-1.fc25.s390x
ModemManager-glib-1.6.4-1.fc25.s390x
newt-python3-0.52.19-2.fc25.s390x
python-munch-2.0.4-3.fc25.noarch
python-bugzilla-1.2.2-4.fc25.noarch
libedit-3.1-16.20160618cvs.fc25.s390x
createrepo_c-0.10.0-6.fc25.s390x
device-mapper-multipath-libs-0.4.9-83.fc25.s390x
yum-3.4.3-510.fc25.noarch
mozjs17-17.0.0-16.fc25.s390x
libselinux-2.5-13.fc25.s390x
python2-pyparsing-2.1.10-1.fc25.noarch
cairo-gobject-1.14.8-1.fc25.s390x
xorg-x11-proto-devel-7.7-20.fc25.noarch
brlapi-0.6.5-2.fc25.s390x
librados-devel-10.2.4-2.fc25.s390x

Re: [Qemu-block] [Qemu-devel] using "qemu-img convert -O qcow2" to convert qcow v1 to v2 creates a qcow v3 file?

[Max Reitz]

On 2017-11-14 21:38, John Snow wrote:
> 
> 
> On 11/14/2017 03:35 PM, Max Reitz wrote:
>> On 2017-11-14 21:30, John Snow wrote:
>>>
>>>
>>> On 11/14/2017 01:46 PM, Max Reitz wrote:
 On 2017-11-14 19:45, Thomas Huth wrote:
> On 14.11.2017 14:32, Max Reitz wrote:
> [...]
>> Well, do you want to document it?  I'd rather deprecate it altogether.
>
> Maybe a first step could be to change qemu-img so that it refuses to
> create new qcow1 images (but still can convert them into other formats).
> So basically make qcow1 read-only?

 Yep, and the actual first step to that is to make it issue a deprecation
 warning when creating qcow v1 images (which is what I proposed). :-)

 Max

>>>
>>> Deprecation warning is good.
>>>
>>> In future versions you can shimmy it behind a
>>> --no-really-I-want-this-old-format option, I think we ought to support
>>> creating the images for as long as is technologically convenient.
>>
>> Well, at some point you can also demand from users to just dig out some
>> old version of qemu-img to convert their qcow v1 images to qcow2.  It's
>> not like they are going to miss out on anything.
>>
> 
> As long is convenient. I won't throw a fit that it needs to be around
> forever, but as long as it's sufficiently guarded from use and isn't
> hard to keep around I'd prefer to do that.
> 
> I suppose it's just a weak preference.

I agree that sufficiently guarding it (albeit our definitions on what is
sufficient may differ) serves all the purpose I need, that is, make
users aware of the fact that they are doing something for which I can
see no reason.

But on the other hand, I want those guards to make it dead code,
effectively.  And if something is dead code... There is no reason to
keep it around.

>> (If you deprecate emulated hardware, users may complain that they don't
>> get the newest qemu features/bugfixes/... while continuing to use that
>> hardware, so I can see that it's a tough decision whether to deprecate
>> that.  But it's not like you are going to lose any features or anything
>> if you convert your dusty images to qcow2.  On the contrary, we're
>> helping you to get more performance out of them.  Maybe qemu should just
>> silently convert qcow v1 images to qcow2 without asking the user, like
>> Apple did...)
>>
>> Max
>>
> 
> "Like Apple did" seems sufficient justification to never do that, but
> maybe that's just my own opinion.

Sorry, forgot the emoticon. :-)

Yes, that was meant to be a joke.  Although adding a qemu-img amend for
amending qcow v1 to v2/v3 images is probably mostly an issue of creating
the right internal interface for cross-format amendments.

(Silently storing qcow v1 as v2/v3 images is actually something where
users could have a problem because maybe they have some tool that only
works on qcow v1 images; and then they can't use that together with an
auto-amending qemu...)

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] using "qemu-img convert -O qcow2" to convert qcow v1 to v2 creates a qcow v3 file?

[John Snow]

On 11/14/2017 03:35 PM, Max Reitz wrote:
> On 2017-11-14 21:30, John Snow wrote:
>>
>>
>> On 11/14/2017 01:46 PM, Max Reitz wrote:
>>> On 2017-11-14 19:45, Thomas Huth wrote:
 On 14.11.2017 14:32, Max Reitz wrote:
 [...]
> Well, do you want to document it?  I'd rather deprecate it altogether.

 Maybe a first step could be to change qemu-img so that it refuses to
 create new qcow1 images (but still can convert them into other formats).
 So basically make qcow1 read-only?
>>>
>>> Yep, and the actual first step to that is to make it issue a deprecation
>>> warning when creating qcow v1 images (which is what I proposed). :-)
>>>
>>> Max
>>>
>>
>> Deprecation warning is good.
>>
>> In future versions you can shimmy it behind a
>> --no-really-I-want-this-old-format option, I think we ought to support
>> creating the images for as long as is technologically convenient.
> 
> Well, at some point you can also demand from users to just dig out some
> old version of qemu-img to convert their qcow v1 images to qcow2.  It's
> not like they are going to miss out on anything.
> 

As long is convenient. I won't throw a fit that it needs to be around
forever, but as long as it's sufficiently guarded from use and isn't
hard to keep around I'd prefer to do that.

I suppose it's just a weak preference.

> (If you deprecate emulated hardware, users may complain that they don't
> get the newest qemu features/bugfixes/... while continuing to use that
> hardware, so I can see that it's a tough decision whether to deprecate
> that.  But it's not like you are going to lose any features or anything
> if you convert your dusty images to qcow2.  On the contrary, we're
> helping you to get more performance out of them.  Maybe qemu should just
> silently convert qcow v1 images to qcow2 without asking the user, like
> Apple did...)
> 
> Max
> 

"Like Apple did" seems sufficient justification to never do that, but
maybe that's just my own opinion.

Re: [Qemu-block] [Qemu-devel] using "qemu-img convert -O qcow2" to convert qcow v1 to v2 creates a qcow v3 file?

[Max Reitz]

On 2017-11-14 21:30, John Snow wrote:
> 
> 
> On 11/14/2017 01:46 PM, Max Reitz wrote:
>> On 2017-11-14 19:45, Thomas Huth wrote:
>>> On 14.11.2017 14:32, Max Reitz wrote:
>>> [...]
 Well, do you want to document it?  I'd rather deprecate it altogether.
>>>
>>> Maybe a first step could be to change qemu-img so that it refuses to
>>> create new qcow1 images (but still can convert them into other formats).
>>> So basically make qcow1 read-only?
>>
>> Yep, and the actual first step to that is to make it issue a deprecation
>> warning when creating qcow v1 images (which is what I proposed). :-)
>>
>> Max
>>
> 
> Deprecation warning is good.
> 
> In future versions you can shimmy it behind a
> --no-really-I-want-this-old-format option, I think we ought to support
> creating the images for as long as is technologically convenient.

Well, at some point you can also demand from users to just dig out some
old version of qemu-img to convert their qcow v1 images to qcow2.  It's
not like they are going to miss out on anything.

(If you deprecate emulated hardware, users may complain that they don't
get the newest qemu features/bugfixes/... while continuing to use that
hardware, so I can see that it's a tough decision whether to deprecate
that.  But it's not like you are going to lose any features or anything
if you convert your dusty images to qcow2.  On the contrary, we're
helping you to get more performance out of them.  Maybe qemu should just
silently convert qcow v1 images to qcow2 without asking the user, like
Apple did...)

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.11] qcow2: Fix overly broad madvise()

[Eric Blake]

On 11/14/2017 12:41 PM, Max Reitz wrote:
> @mem_size and @offset are both size_t, thus subtracting them from one
> another will just return a big size_t if mem_size < offset -- even more
> obvious here because the result is stored in another size_t.
> 
> Checking that result to be positive is therefore not sufficient to
> excluse the case that offset > mem_size.  Thus, we currently sometimes

s/excluse/exclude/

> issue an madvise() over a very large address range.
> 
> This is triggered by iotest 163, but with -m64, this does not result in
> tangible problems.  But with -m32, this test produces three segfaults,
> all of which are fixed by this patch.
> 
> Signed-off-by: Max Reitz 
> ---
>  block/qcow2-cache.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

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] qapi: block-core: Clarify events emitted by 'block-job-cancel'

[no-reply]

Hi,

This series failed automatic build test. Please find the testing commands and
their output below. If you have docker installed, you can probably reproduce it
locally.

Subject: [Qemu-devel] [PATCH] qapi: block-core: Clarify events emitted by 
'block-job-cancel'
Type: series
Message-id: 20171114191605.22349-1-kcham...@redhat.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
set -e
git submodule update --init dtc
# Let docker tests dump environment info
export SHOW_ENV=1
export J=8
time make docker-test-quick@centos6
time make docker-test-build@min-glib
time make docker-test-mingw@fedora
time make docker-test-block@fedora
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
375c460e8a qapi: block-core: Clarify events emitted by 'block-job-cancel'

=== OUTPUT BEGIN ===
Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc'
Cloning into '/var/tmp/patchew-tester-tmp-q1izxxh8/src/dtc'...
Submodule path 'dtc': checked out '558cd81bdd432769b59bff01240c44f82cfb1a9d'
  BUILD   centos6
make[1]: Entering directory '/var/tmp/patchew-tester-tmp-q1izxxh8/src'
  GEN 
/var/tmp/patchew-tester-tmp-q1izxxh8/src/docker-src.2017-11-14-14.25.09.27009/qemu.tar
Cloning into 
'/var/tmp/patchew-tester-tmp-q1izxxh8/src/docker-src.2017-11-14-14.25.09.27009/qemu.tar.vroot'...
done.
Checking out files:  27% (1530/5654)   
Checking out files:  28% (1584/5654)   
Checking out files:  29% (1640/5654)   
Checking out files:  30% (1697/5654)   
Checking out files:  31% (1753/5654)   
Checking out files:  32% (1810/5654)   
Checking out files:  33% (1866/5654)   
Checking out files:  34% (1923/5654)   
Checking out files:  35% (1979/5654)   
Checking out files:  36% (2036/5654)   
Checking out files:  37% (2092/5654)   
Checking out files:  38% (2149/5654)   
Checking out files:  39% (2206/5654)   
Checking out files:  40% (2262/5654)   
Checking out files:  40% (2278/5654)   
Checking out files:  41% (2319/5654)   
Checking out files:  42% (2375/5654)   
Checking out files:  43% (2432/5654)   
Checking out files:  44% (2488/5654)   
Checking out files:  45% (2545/5654)   
Checking out files:  46% (2601/5654)   
Checking out files:  47% (2658/5654)   
Checking out files:  48% (2714/5654)   
Checking out files:  49% (2771/5654)   
Checking out files:  50% (2827/5654)   
Checking out files:  51% (2884/5654)   
Checking out files:  52% (2941/5654)   
Checking out files:  53% (2997/5654)   
Checking out files:  54% (3054/5654)   
Checking out files:  55% (3110/5654)   
Checking out files:  56% (3167/5654)   
Checking out files:  57% (3223/5654)   
Checking out files:  58% (3280/5654)   
Checking out files:  59% (3336/5654)   
Checking out files:  60% (3393/5654)   
Checking out files:  61% (3449/5654)   
Checking out files:  62% (3506/5654)   
Checking out files:  63% (3563/5654)   
Checking out files:  64% (3619/5654)   
Checking out files:  65% (3676/5654)   
Checking out files:  66% (3732/5654)   
Checking out files:  67% (3789/5654)   
Checking out files:  68% (3845/5654)   
Checking out files:  69% (3902/5654)   
Checking out files:  70% (3958/5654)   
Checking out files:  71% (4015/5654)   
Checking out files:  72% (4071/5654)   
Checking out files:  73% (4128/5654)   
Checking out files:  74% (4184/5654)   
Checking out files:  75% (4241/5654)   
Checking out files:  76% (4298/5654)   
Checking out files:  77% (4354/5654)   
Checking out files:  78% (4411/5654)   
Checking out files:  79% (4467/5654)   
Checking out files:  80% (4524/5654)   
Checking out files:  81% (4580/5654)   
Checking out files:  82% (4637/5654)   
Checking out files:  83% (4693/5654)   
Checking out files:  84% (4750/5654)   
Checking out files:  85% (4806/5654)   
Checking out files:  86% (4863/5654)   
Checking out files:  87% (4919/5654)   
Checking out files:  88% (4976/5654)   
Checking out files:  89% (5033/5654)   
Checking out files:  90% (5089/5654)   
Checking out files:  91% (5146/5654)   
Checking out files:  92% (5202/5654)   
Checking out files:  93% (5259/5654)   
Checking out files:  94% (5315/5654)   
Checking out files:  95% (5372/5654)   
Checking out files:  96% (5428/5654)   
Checking out files:  97% (5485/5654)   
Checking out files:  98% (5541/5654)   
Checking out files:  99% (5598/5654)   
Checking out files: 100% (5654/5654)   
Checking out files: 100% (5654/5654), done.
Your branch is up-to-date with 'origin/test'.
Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc'
Cloning into 
'/var/tmp/patchew-tester-tmp-q1izxxh8/src/docker-src.2017-11-14-14.25.09.27009/qemu.tar.vroot/dtc'...
Submodule path 'dtc': checked out '558cd81bdd432769b59bff01240c44f82cfb1a9d'
Submodule 'ui/keycodemapdb' (git://git.qemu.org/keycodemapdb.git) registered 
for path 'ui/keycodemapdb'
Cloning into 
'/var/tmp/patchew-tester-tmp-q1izxxh8/src/docker-src.2017-11-14-14.25.09.27009/qemu.tar.vroot/ui/keycodemapdb'...
Submodule path 'ui/keycodemapdb': checked out 

Re: [Qemu-block] [PATCH 1/5 for-2.11?] qcow2: reject unaligned offsets in write compressed

[Eric Blake]

On 11/14/2017 12:30 PM, Anton Nefedov wrote:
> On 14/11/2017 7:50 PM, Eric Blake wrote:
>> On 11/14/2017 04:16 AM, Anton Nefedov wrote:
>>> Misaligned compressed write is not supported.
>>>
>>> Signed-off-by: Anton Nefedov 
>>> ---
>>>   block/qcow2.c | 4 
>>>   1 file changed, 4 insertions(+)
>>
>> Should this one be applied in 2.11?
>>
> 
> For the record, this one is pretty hard to trigger; backup and qemu-img
> convert currently use compressed write, both make sure they operate in
> clusters.
> 
> qemu-io is almighty though

Okay, then we definitely have a bug, and this patch is definitely 2.11
material, especially if you update the commit message to show the
trigger case:

> 
> qemu-io> write -c -P 7 512 64k
> wrote 65536/65536 bytes at offset 512
> 64 KiB, 1 ops; 0.0187 sec (3.329 MiB/sec and 53.2566 ops/sec)
> qemu-io> read -P 7 512 64k
> Pattern verification failed at offset 512, 65536 bytes
> read 65536/65536 bytes at offset 512
> 64 KiB, 1 ops; 0.0002 sec (248.016 MiB/sec and 3968.2540 ops/sec)
> qemu-io> read -P 7 0 64k
> read 65536/65536 bytes at offset 0
> 64 KiB, 1 ops; 0. sec (1.606 GiB/sec and 26315.7895 ops/sec)
> 
> /Anton
> 

-- 
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 for-2.11? v7 6/6] tests: Add check-qobject for equality tests

[Eric Blake]

On 11/14/2017 12:01 PM, Max Reitz wrote:
> Add a new test file (check-qobject.c) for unit tests that concern
> QObjects as a whole.
> 
> Its only purpose for now is to test the qobject_is_equal() function.
> 

> + * Note that qobject_is_equal() is not really an equivalence relation,
> + * so this function may not be used for all objects (reflexivity is
> + * not guaranteed, e.g. in the case of a QNum containing NaN).
> + *
> + * The @_ argument is required because a boolean may not be the last
> + * argument before a variadic argument list (C11 7.16.1.4 para. 4).

C99 7.15.1.4 (did C11 add a section? /me goes and looks... Oh, it did.)

Okay, so it's not a typo after all.  Ignore my comment in the cover
letter, and

Reviewed-by: Eric Blake 

(And soon, I'll have to start quoting C17 instead of C99 or C11...)

-- 
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 for-2.11? v7 0/6] block: Don't compare strings in bdrv_reopen_prepare()

[Eric Blake]

On 11/14/2017 12:01 PM, Max Reitz wrote:
> bdrv_reopen_prepare() assumes that all BDS options are strings, which is
> not necessarily correct. This series introduces a new qobject_is_equal()
> function which can be used to test whether any options have changed,
> independently of their type.
> 
> 
> v7:
> - Patch 6: Fix a clang warning:
> tests/check-qobject.c:39:24: error: passing an object that undergoes
>  default argument promotion to
>  'va_start' has undefined behavior
>   TIL: You cannot use va_start(ap, foo) if @foo is a bool.  An int
>works, however.

I knew that va_arg(ap, bool) was undefined behavior, but didn't realize
va_start(ap, bool_param) was also undefined.  But sure enough, reading
C99 7.15.1.4:

4 The parameter parmN is the identifier of the rightmost parameter in
the variable
parameter list in the function definition (the one just before the ,
...). If the parameter
parmN is declared with the register storage class, with a function or
array type, or
with a type that is not compatible with the type that results after
application of the default
argument promotions, the behavior is undefined.

>Feel free to explain the long version to me, because I don't
>think I have fully understood the issue, but it's something like
>"Using bools for variadic arguments results in their promotion to
>an int, but you have to use a type that is promoted to itself
>(like int)."

So it must be that C99 is trying to cater to platforms that have special
ABI for passing multiple bool on the stack, by stating that the moment
argument promotion is in effect, va_list adjacent to a bool may cause
problems with that special packing in the ABI.  Weird.

> 
> 
> git-backport-diff against v6:
> 
> Key:
> [] : patches are identical
> [] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, 
> respectively
> 
> 001/6:[] [--] 'qapi/qnull: Add own header'
> 002/6:[] [--] 'qapi/qlist: Add qlist_append_null() macro'
> 003/6:[] [--] 'qapi: Add qobject_is_equal()'
> 004/6:[] [--] 'block: qobject_is_equal() in bdrv_reopen_prepare()'
> 005/6:[] [--] 'iotests: Add test for non-string option reopening'
> 006/6:[0011] [FC] 'tests: Add check-qobject for equality tests'

I still think this is 2.11 material.  Once you fix the typo I point out
separately on 6/6, the changes since v6 look reasonable, so series:
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] [Nbd] [Qemu-devel] How to online resize qemu disk with nbd protocol?

[Eric Blake]

On 11/14/2017 11:37 AM, Wouter Verhelst wrote:
> On Tue, Nov 14, 2017 at 10:41:39AM -0600, Eric Blake wrote:
>> Another thought - with structured replies, we finally have a way to let
>> the client ask for the server to send resize information whenever the
>> server wants, rather than having to be polled by a new client request
>> all the time.  This is possible by having the server reply with a chunk
>> without the NBD_REPLY_FLAG_DONE bit, for as many times as it wants,
>> (that is, the server never officially ends the response to the single
>> client request for on-going status, until the client sends an
>> NBD_CMD_DISC).
> 
> Hrm, yeah, that could work.
> 
> Minor downside of this would be that a client would now be expected to
> continue listening "forever" (probably needs to do a blocking read() or
> a select() on the socket), whereas with the current situation a client
> could get away with only reading for as long as it expects data.
> 
> I don't think that should be a blocker, but it might be something we
> might want to document.
> 
>> I don't think the server should go into this mode without a flag bit
>> from the client requesting it (as it potentially ties up a thread that
>> could otherwise be used for parallel processing of other requests),
> 
> Yeah. I think we should probably initiate this with a BLOCK_STATUS
> message that has a flag with which we mean "don't stop sending data on
> the given region for contexts that support it".

Now we're mixing NBD_CMD_BLOCK_STATUS and NBD_CMD_RESIZE; I was thinking
of the open-ended command for being informed of all
server-side-initiated size changes in response to RESIZE; but your
mention of an open-ended BLOCK_STATUS has an interesting connotation of
being able to get live updates as sections of a file are dirtied.

I also remember from talking with Vladimir during KVM Forum last month
that one of the shortfalls of the NBD protocol is that you can only ever
send a length of up to 32 bits on the command side (unless we introduce
structured commands in addition to our current work to add structured
replies); even querying the full status of a 1TB volume would require at
least 256 NBD_CMD_BLOCK_STATUS calls.  But having a special case of a
length of 0 meaning to report status as long as the server is interested
could reduce the number of round trips by letting the server report more
than 4G of status in response to one client query.  There was also a
thread a while ago about the possibility of a per-export flag denoting a
volume is known to contain all-zeroes on startup, which can be used as a
handy shortcut to having to query block status over each slice of the
file; we weren't sure about burning a per-export flag on that
information, but having it be a special mode of NBD_CMD_BLOCK_STATUS may
be easy enough to codify.

> 
> However, I could imagine that there might be some cases wherein a server
> might be able to go into such a mode for two or more metadata contexts,
> and where a client might want to go into that mode for one of them but
> not all of them, while still wanting some information from them.
> 
> This could be covered with metadata context syntax, but it's annoying
> and shouldn't be necessary.
> 
> I'm starting to think I made a mistake when I said NBD_CMD_BLOCK_STATUS
> can't take a metadata context ID. Okay, there's no space for it, but
> that shouldn't have been a blocker.
> 
> Thoughts?

Nothing says the server has to reply the same length of information when
replying for multiple selected metadata contexts; but if we allow
different reply sizes all in one query, we may also need some way to
easily tell that the server has stopped sending metadata for one context
even though it is still providing additional replies for another context.

And maybe we do want to someday start thinking about structured
requests; where being able to do per-command selection of metadata
contexts (instead of per-export selection) may indeed be the first use case.

> 
>> and that the server could reject a repeat command with the flag if it
>> is already serving a previous open-ended request.
> 
> Right.
> 
> On the other hand, I can imagine that a client might also want to tell
> the server that it is no longer interested in an outstanding request. In
> such a case, it should be able to cancel it.

Good point - if we allow the client to request an open-ended reply, it's
also nice to let the client decide how long that open-endedness should last.

-- 
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] using "qemu-img convert -O qcow2" to convert qcow v1 to v2 creates a qcow v3 file?

[Max Reitz]

On 2017-11-14 19:45, Thomas Huth wrote:
> On 14.11.2017 14:32, Max Reitz wrote:
> [...]
>> Well, do you want to document it?  I'd rather deprecate it altogether.
> 
> Maybe a first step could be to change qemu-img so that it refuses to
> create new qcow1 images (but still can convert them into other formats).
> So basically make qcow1 read-only?

Yep, and the actual first step to that is to make it issue a deprecation
warning when creating qcow v1 images (which is what I proposed). :-)

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] using "qemu-img convert -O qcow2" to convert qcow v1 to v2 creates a qcow v3 file?

[Thomas Huth]

On 14.11.2017 14:32, Max Reitz wrote:
[...]
> Well, do you want to document it?  I'd rather deprecate it altogether.

Maybe a first step could be to change qemu-img so that it refuses to
create new qcow1 images (but still can convert them into other formats).
So basically make qcow1 read-only?

 Thomas



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH for-2.11] qcow2: Fix overly broad madvise()

[Max Reitz]

@mem_size and @offset are both size_t, thus subtracting them from one
another will just return a big size_t if mem_size < offset -- even more
obvious here because the result is stored in another size_t.

Checking that result to be positive is therefore not sufficient to
excluse the case that offset > mem_size.  Thus, we currently sometimes
issue an madvise() over a very large address range.

This is triggered by iotest 163, but with -m64, this does not result in
tangible problems.  But with -m32, this test produces three segfaults,
all of which are fixed by this patch.

Signed-off-by: Max Reitz 
---
 block/qcow2-cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
index 75746a7f43..5222a7b94d 100644
--- a/block/qcow2-cache.c
+++ b/block/qcow2-cache.c
@@ -73,7 +73,7 @@ static void qcow2_cache_table_release(BlockDriverState *bs, 
Qcow2Cache *c,
 size_t mem_size = (size_t) s->cluster_size * num_tables;
 size_t offset = QEMU_ALIGN_UP((uintptr_t) t, align) - (uintptr_t) t;
 size_t length = QEMU_ALIGN_DOWN(mem_size - offset, align);
-if (length > 0) {
+if (mem_size > offset && length > 0) {
 madvise((uint8_t *) t + offset, length, MADV_DONTNEED);
 }
 #endif
-- 
2.13.6

Re: [Qemu-block] [PATCH 1/5 for-2.11?] qcow2: reject unaligned offsets in write compressed

[Anton Nefedov]

On 14/11/2017 7:50 PM, Eric Blake wrote:

On 11/14/2017 04:16 AM, Anton Nefedov wrote:

Misaligned compressed write is not supported.

Signed-off-by: Anton Nefedov 
---
  block/qcow2.c | 4 
  1 file changed, 4 insertions(+)


Should this one be applied in 2.11?



For the record, this one is pretty hard to trigger; backup and qemu-img
convert currently use compressed write, both make sure they operate in
clusters.

qemu-io is almighty though

qemu-io> write -c -P 7 512 64k
wrote 65536/65536 bytes at offset 512
64 KiB, 1 ops; 0.0187 sec (3.329 MiB/sec and 53.2566 ops/sec)
qemu-io> read -P 7 512 64k
Pattern verification failed at offset 512, 65536 bytes
read 65536/65536 bytes at offset 512
64 KiB, 1 ops; 0.0002 sec (248.016 MiB/sec and 3968.2540 ops/sec)
qemu-io> read -P 7 0 64k
read 65536/65536 bytes at offset 0
64 KiB, 1 ops; 0. sec (1.606 GiB/sec and 26315.7895 ops/sec)

/Anton

Re: [Qemu-block] [PULL 00/20] Block patches for 2.11.0-rc1

[Peter Maydell]

On 14 November 2017 at 17:23, Max Reitz  wrote:
> The following changes since commit 191b5fbfa66e5b23e2150f3c6981d30eb84418a9:
>
>   Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' 
> into staging (2017-11-14 16:11:19 +)
>
> are available in the git repository at:
>
>   git://github.com/XanClic/qemu.git tags/pull-block-2017-11-14
>
> for you to fetch changes up to 8b2d7c364d9a2491f7501f6688cd722045cf808a:
>
>   qemu-iotests: update unsupported image formats in 194 (2017-11-14 18:06:26 
> +0100)
>
> 
> Block patches for 2.11.0-rc1


Applied, thanks.

-- PMM

[Qemu-block] [PATCH for-2.11? v7 6/6] tests: Add check-qobject for equality tests

[Max Reitz]

Add a new test file (check-qobject.c) for unit tests that concern
QObjects as a whole.

Its only purpose for now is to test the qobject_is_equal() function.

Signed-off-by: Max Reitz 
---
 tests/Makefile.include |   4 +-
 tests/check-qobject.c  | 328 +
 tests/.gitignore   |   1 +
 3 files changed, 332 insertions(+), 1 deletion(-)
 create mode 100644 tests/check-qobject.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 434a2ce868..c002352134 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -41,6 +41,7 @@ check-unit-y += tests/check-qlist$(EXESUF)
 gcov-files-check-qlist-y = qobject/qlist.c
 check-unit-y += tests/check-qnull$(EXESUF)
 gcov-files-check-qnull-y = qobject/qnull.c
+check-unit-y += tests/check-qobject$(EXESUF)
 check-unit-y += tests/check-qjson$(EXESUF)
 gcov-files-check-qjson-y = qobject/qjson.c
 check-unit-y += tests/check-qlit$(EXESUF)
@@ -546,7 +547,7 @@ GENERATED_FILES += tests/test-qapi-types.h 
tests/test-qapi-visit.h \
tests/test-qmp-introspect.h
 
 test-obj-y = tests/check-qnum.o tests/check-qstring.o tests/check-qdict.o \
-   tests/check-qlist.o tests/check-qnull.o \
+   tests/check-qlist.o tests/check-qnull.o tests/check-qobject.o \
tests/check-qjson.o tests/check-qlit.o \
tests/test-coroutine.o tests/test-string-output-visitor.o \
tests/test-string-input-visitor.o tests/test-qobject-output-visitor.o \
@@ -580,6 +581,7 @@ tests/check-qstring$(EXESUF): tests/check-qstring.o 
$(test-util-obj-y)
 tests/check-qdict$(EXESUF): tests/check-qdict.o $(test-util-obj-y)
 tests/check-qlist$(EXESUF): tests/check-qlist.o $(test-util-obj-y)
 tests/check-qnull$(EXESUF): tests/check-qnull.o $(test-util-obj-y)
+tests/check-qobject$(EXESUF): tests/check-qobject.o $(test-util-obj-y)
 tests/check-qjson$(EXESUF): tests/check-qjson.o $(test-util-obj-y)
 tests/check-qlit$(EXESUF): tests/check-qlit.o $(test-util-obj-y)
 tests/check-qom-interface$(EXESUF): tests/check-qom-interface.o 
$(test-qom-obj-y)
diff --git a/tests/check-qobject.c b/tests/check-qobject.c
new file mode 100644
index 00..03e9175113
--- /dev/null
+++ b/tests/check-qobject.c
@@ -0,0 +1,328 @@
+/*
+ * Generic QObject unit-tests.
+ *
+ * Copyright (C) 2017 Red Hat Inc.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ */
+#include "qemu/osdep.h"
+
+#include "qapi/qmp/types.h"
+#include "qemu-common.h"
+
+#include 
+
+/* Marks the end of the test_equality() argument list.
+ * We cannot use NULL there because that is a valid argument. */
+static QObject test_equality_end_of_arguments;
+
+/**
+ * Test whether all variadic QObject *arguments are equal (@expected
+ * is true) or whether they are all not equal (@expected is false).
+ * Every QObject is tested to be equal to itself (to test
+ * reflexivity), all tests are done both ways (to test symmetry), and
+ * transitivity is not assumed but checked (each object is compared to
+ * every other one).
+ *
+ * Note that qobject_is_equal() is not really an equivalence relation,
+ * so this function may not be used for all objects (reflexivity is
+ * not guaranteed, e.g. in the case of a QNum containing NaN).
+ *
+ * The @_ argument is required because a boolean may not be the last
+ * argument before a variadic argument list (C11 7.16.1.4 para. 4).
+ */
+static void do_test_equality(bool expected, int _, ...)
+{
+va_list ap_count, ap_extract;
+QObject **args;
+int arg_count = 0;
+int i, j;
+
+va_start(ap_count, _);
+va_copy(ap_extract, ap_count);
+while (va_arg(ap_count, QObject *) != _equality_end_of_arguments) {
+arg_count++;
+}
+va_end(ap_count);
+
+args = g_new(QObject *, arg_count);
+for (i = 0; i < arg_count; i++) {
+args[i] = va_arg(ap_extract, QObject *);
+}
+va_end(ap_extract);
+
+for (i = 0; i < arg_count; i++) {
+g_assert(qobject_is_equal(args[i], args[i]) == true);
+
+for (j = i + 1; j < arg_count; j++) {
+g_assert(qobject_is_equal(args[i], args[j]) == expected);
+}
+}
+}
+
+#define check_equal(...) \
+do_test_equality(true, 0, __VA_ARGS__, _equality_end_of_arguments)
+#define check_unequal(...) \
+do_test_equality(false, 0, __VA_ARGS__, _equality_end_of_arguments)
+
+static void do_free_all(int _, ...)
+{
+va_list ap;
+QObject *obj;
+
+va_start(ap, _);
+while ((obj = va_arg(ap, QObject *)) != NULL) {
+qobject_decref(obj);
+}
+va_end(ap);
+}
+
+#define free_all(...) \
+do_free_all(0, __VA_ARGS__, NULL)
+
+static void qobject_is_equal_null_test(void)
+{
+check_unequal(qnull(), NULL);
+}
+
+static void qobject_is_equal_num_test(void)
+{
+QNum *u0, *i0, *d0, *dnan, *um42, *im42, *dm42;
+
+u0 = qnum_from_uint(0u);
+i0 = qnum_from_int(0);
+d0 = qnum_from_double(0.0);
+

[Qemu-block] [PATCH for-2.11? v7 3/6] qapi: Add qobject_is_equal()

[Max Reitz]

This generic function (along with its implementations for different
types) determines whether two QObjects are equal.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Alberto Garcia 
Reviewed-by: Markus Armbruster 
---
 include/qapi/qmp/qbool.h   |  1 +
 include/qapi/qmp/qdict.h   |  1 +
 include/qapi/qmp/qlist.h   |  1 +
 include/qapi/qmp/qnull.h   |  2 ++
 include/qapi/qmp/qnum.h|  1 +
 include/qapi/qmp/qobject.h |  9 
 include/qapi/qmp/qstring.h |  1 +
 qobject/qbool.c|  8 +++
 qobject/qdict.c| 29 +
 qobject/qlist.c| 32 +++
 qobject/qnull.c|  9 
 qobject/qnum.c | 54 ++
 qobject/qobject.c  | 29 +
 qobject/qstring.c  |  9 
 14 files changed, 186 insertions(+)

diff --git a/include/qapi/qmp/qbool.h b/include/qapi/qmp/qbool.h
index a4c309..f77ea86c4e 100644
--- a/include/qapi/qmp/qbool.h
+++ b/include/qapi/qmp/qbool.h
@@ -24,6 +24,7 @@ typedef struct QBool {
 QBool *qbool_from_bool(bool value);
 bool qbool_get_bool(const QBool *qb);
 QBool *qobject_to_qbool(const QObject *obj);
+bool qbool_is_equal(const QObject *x, const QObject *y);
 void qbool_destroy_obj(QObject *obj);
 
 #endif /* QBOOL_H */
diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
index 7ea5120c4a..fc218e7be6 100644
--- a/include/qapi/qmp/qdict.h
+++ b/include/qapi/qmp/qdict.h
@@ -43,6 +43,7 @@ void qdict_del(QDict *qdict, const char *key);
 int qdict_haskey(const QDict *qdict, const char *key);
 QObject *qdict_get(const QDict *qdict, const char *key);
 QDict *qobject_to_qdict(const QObject *obj);
+bool qdict_is_equal(const QObject *x, const QObject *y);
 void qdict_iter(const QDict *qdict,
 void (*iter)(const char *key, QObject *obj, void *opaque),
 void *opaque);
diff --git a/include/qapi/qmp/qlist.h b/include/qapi/qmp/qlist.h
index 59d209bbae..ec3fcc1a4c 100644
--- a/include/qapi/qmp/qlist.h
+++ b/include/qapi/qmp/qlist.h
@@ -61,6 +61,7 @@ QObject *qlist_peek(QList *qlist);
 int qlist_empty(const QList *qlist);
 size_t qlist_size(const QList *qlist);
 QList *qobject_to_qlist(const QObject *obj);
+bool qlist_is_equal(const QObject *x, const QObject *y);
 void qlist_destroy_obj(QObject *obj);
 
 static inline const QListEntry *qlist_first(const QList *qlist)
diff --git a/include/qapi/qmp/qnull.h b/include/qapi/qmp/qnull.h
index d075549283..c992ee2ae1 100644
--- a/include/qapi/qmp/qnull.h
+++ b/include/qapi/qmp/qnull.h
@@ -27,4 +27,6 @@ static inline QNull *qnull(void)
 return _;
 }
 
+bool qnull_is_equal(const QObject *x, const QObject *y);
+
 #endif /* QNULL_H */
diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
index d6b0791139..c3d86794bb 100644
--- a/include/qapi/qmp/qnum.h
+++ b/include/qapi/qmp/qnum.h
@@ -69,6 +69,7 @@ double qnum_get_double(QNum *qn);
 char *qnum_to_string(QNum *qn);
 
 QNum *qobject_to_qnum(const QObject *obj);
+bool qnum_is_equal(const QObject *x, const QObject *y);
 void qnum_destroy_obj(QObject *obj);
 
 #endif /* QNUM_H */
diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index ef1d1a9237..38ac68845c 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -68,6 +68,15 @@ static inline void qobject_incref(QObject *obj)
 }
 
 /**
+ * qobject_is_equal(): Return whether the two objects are equal.
+ *
+ * Any of the pointers may be NULL; return true if both are.  Always
+ * return false if only one is (therefore a QNull object is not
+ * considered equal to a NULL pointer).
+ */
+bool qobject_is_equal(const QObject *x, const QObject *y);
+
+/**
  * qobject_destroy(): Free resources used by the object
  */
 void qobject_destroy(QObject *obj);
diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
index 10076b7c8c..65c05a9be5 100644
--- a/include/qapi/qmp/qstring.h
+++ b/include/qapi/qmp/qstring.h
@@ -31,6 +31,7 @@ void qstring_append_int(QString *qstring, int64_t value);
 void qstring_append(QString *qstring, const char *str);
 void qstring_append_chr(QString *qstring, int c);
 QString *qobject_to_qstring(const QObject *obj);
+bool qstring_is_equal(const QObject *x, const QObject *y);
 void qstring_destroy_obj(QObject *obj);
 
 #endif /* QSTRING_H */
diff --git a/qobject/qbool.c b/qobject/qbool.c
index 0606bbd2a3..ac825fc5a2 100644
--- a/qobject/qbool.c
+++ b/qobject/qbool.c
@@ -52,6 +52,14 @@ QBool *qobject_to_qbool(const QObject *obj)
 }
 
 /**
+ * qbool_is_equal(): Test whether the two QBools are equal
+ */
+bool qbool_is_equal(const QObject *x, const QObject *y)
+{
+return qobject_to_qbool(x)->value == qobject_to_qbool(y)->value;
+}
+
+/**
  * qbool_destroy_obj(): Free all memory allocated by a
  * QBool object
  */
diff --git a/qobject/qdict.c b/qobject/qdict.c
index 

[Qemu-block] [PATCH for-2.11? v7 4/6] block: qobject_is_equal() in bdrv_reopen_prepare()

[Max Reitz]

Currently, bdrv_reopen_prepare() assumes that all BDS options are
strings. However, this is not the case if the BDS has been created
through the json: pseudo-protocol or blockdev-add.

Note that the user-invokable reopen command is an HMP command, so you
can only specify strings there. Therefore, specifying a non-string
option with the "same" value as it was when originally created will now
return an error because the values are supposedly similar (and there is
no way for the user to circumvent this but to just not specify the
option again -- however, this is still strictly better than just
crashing).

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 block.c | 29 ++---
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/block.c b/block.c
index 684cb018da..28889a2690 100644
--- a/block.c
+++ b/block.c
@@ -3069,19 +3069,26 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
BlockReopenQueue *queue,
 const QDictEntry *entry = qdict_first(reopen_state->options);
 
 do {
-QString *new_obj = qobject_to_qstring(entry->value);
-const char *new = qstring_get_str(new_obj);
+QObject *new = entry->value;
+QObject *old = qdict_get(reopen_state->bs->options, entry->key);
+
 /*
- * Caution: while qdict_get_try_str() is fine, getting
- * non-string types would require more care.  When
- * bs->options come from -blockdev or blockdev_add, its
- * members are typed according to the QAPI schema, but
- * when they come from -drive, they're all QString.
+ * TODO: When using -drive to specify blockdev options, all values
+ * will be strings; however, when using -blockdev, blockdev-add or
+ * filenames using the json:{} pseudo-protocol, they will be
+ * correctly typed.
+ * In contrast, reopening options are (currently) always strings
+ * (because you can only specify them through qemu-io; all other
+ * callers do not specify any options).
+ * Therefore, when using anything other than -drive to create a 
BDS,
+ * this cannot detect non-string options as unchanged, because
+ * qobject_is_equal() always returns false for objects of different
+ * type.  In the future, this should be remedied by correctly 
typing
+ * all options.  For now, this is not too big of an issue because
+ * the user can simply omit options which cannot be changed anyway,
+ * so they will stay unchanged.
  */
-const char *old = qdict_get_try_str(reopen_state->bs->options,
-entry->key);
-
-if (!old || strcmp(new, old)) {
+if (!qobject_is_equal(new, old)) {
 error_setg(errp, "Cannot change the option '%s'", entry->key);
 ret = -EINVAL;
 goto error;
-- 
2.13.6

[Qemu-block] [PATCH for-2.11? v7 5/6] iotests: Add test for non-string option reopening

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Kevin Wolf 
Reviewed-by: Eric Blake 
---
 tests/qemu-iotests/133 | 9 +
 tests/qemu-iotests/133.out | 5 +
 2 files changed, 14 insertions(+)

diff --git a/tests/qemu-iotests/133 b/tests/qemu-iotests/133
index 9d35a6a1ca..af6b3e1dd4 100755
--- a/tests/qemu-iotests/133
+++ b/tests/qemu-iotests/133
@@ -83,6 +83,15 @@ $QEMU_IO -c 'reopen -o driver=qcow2' $TEST_IMG
 $QEMU_IO -c 'reopen -o file.driver=file' $TEST_IMG
 $QEMU_IO -c 'reopen -o backing.driver=qcow2' $TEST_IMG
 
+echo
+echo "=== Check that reopening works with non-string options ==="
+echo
+
+# Using the json: pseudo-protocol we can create non-string options
+# (Invoke 'info' just so we get some output afterwards)
+IMGOPTSSYNTAX=false $QEMU_IO -f null-co -c 'reopen' -c 'info' \
+"json:{'driver': 'null-co', 'size': 65536}"
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/133.out b/tests/qemu-iotests/133.out
index cc86b94880..f4a85aeb63 100644
--- a/tests/qemu-iotests/133.out
+++ b/tests/qemu-iotests/133.out
@@ -19,4 +19,9 @@ Cannot change the option 'driver'
 
 === Check that unchanged driver is okay ===
 
+
+=== Check that reopening works with non-string options ===
+
+format name: null-co
+format name: null-co
 *** done
-- 
2.13.6

[Qemu-block] [PATCH for-2.11? v7 0/6] block: Don't compare strings in bdrv_reopen_prepare()

[Max Reitz]

bdrv_reopen_prepare() assumes that all BDS options are strings, which is
not necessarily correct. This series introduces a new qobject_is_equal()
function which can be used to test whether any options have changed,
independently of their type.


v7:
- Patch 6: Fix a clang warning:
tests/check-qobject.c:39:24: error: passing an object that undergoes
 default argument promotion to
 'va_start' has undefined behavior
  TIL: You cannot use va_start(ap, foo) if @foo is a bool.  An int
   works, however.
   Feel free to explain the long version to me, because I don't
   think I have fully understood the issue, but it's something like
   "Using bools for variadic arguments results in their promotion to
   an int, but you have to use a type that is promoted to itself
   (like int)."


git-backport-diff against v6:

Key:
[] : patches are identical
[] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/6:[] [--] 'qapi/qnull: Add own header'
002/6:[] [--] 'qapi/qlist: Add qlist_append_null() macro'
003/6:[] [--] 'qapi: Add qobject_is_equal()'
004/6:[] [--] 'block: qobject_is_equal() in bdrv_reopen_prepare()'
005/6:[] [--] 'iotests: Add test for non-string option reopening'
006/6:[0011] [FC] 'tests: Add check-qobject for equality tests'


Max Reitz (6):
  qapi/qnull: Add own header
  qapi/qlist: Add qlist_append_null() macro
  qapi: Add qobject_is_equal()
  block: qobject_is_equal() in bdrv_reopen_prepare()
  iotests: Add test for non-string option reopening
  tests: Add check-qobject for equality tests

 tests/Makefile.include   |   4 +-
 include/qapi/qmp/qbool.h |   1 +
 include/qapi/qmp/qdict.h |   2 +
 include/qapi/qmp/qlist.h |   4 +
 include/qapi/qmp/qnull.h |  32 
 include/qapi/qmp/qnum.h  |   1 +
 include/qapi/qmp/qobject.h   |  21 ++-
 include/qapi/qmp/qstring.h   |   1 +
 include/qapi/qmp/types.h |   1 +
 block.c  |  29 ++--
 qapi/qapi-clone-visitor.c|   1 +
 qapi/string-input-visitor.c  |   1 +
 qobject/qbool.c  |   8 +
 qobject/qdict.c  |  29 
 qobject/qlist.c  |  32 
 qobject/qnull.c  |  11 +-
 qobject/qnum.c   |  54 +++
 qobject/qobject.c|  29 
 qobject/qstring.c|   9 ++
 tests/check-qnull.c  |   2 +-
 tests/check-qobject.c| 328 +++
 scripts/coccinelle/qobject.cocci |   3 +
 tests/.gitignore |   1 +
 tests/qemu-iotests/133   |   9 ++
 tests/qemu-iotests/133.out   |   5 +
 25 files changed, 592 insertions(+), 26 deletions(-)
 create mode 100644 include/qapi/qmp/qnull.h
 create mode 100644 tests/check-qobject.c

-- 
2.13.6

[Qemu-block] [PATCH for-2.11? v7 1/6] qapi/qnull: Add own header

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Alberto Garcia 
Reviewed-by: Markus Armbruster 
---
 include/qapi/qmp/qdict.h|  1 +
 include/qapi/qmp/qnull.h| 30 ++
 include/qapi/qmp/qobject.h  | 12 
 include/qapi/qmp/types.h|  1 +
 qapi/qapi-clone-visitor.c   |  1 +
 qapi/string-input-visitor.c |  1 +
 qobject/qnull.c |  2 +-
 tests/check-qnull.c |  2 +-
 8 files changed, 36 insertions(+), 14 deletions(-)
 create mode 100644 include/qapi/qmp/qnull.h

diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
index 6588c7f0c8..7ea5120c4a 100644
--- a/include/qapi/qmp/qdict.h
+++ b/include/qapi/qmp/qdict.h
@@ -15,6 +15,7 @@
 
 #include "qapi/qmp/qobject.h"
 #include "qapi/qmp/qlist.h"
+#include "qapi/qmp/qnull.h"
 #include "qapi/qmp/qnum.h"
 #include "qemu/queue.h"
 
diff --git a/include/qapi/qmp/qnull.h b/include/qapi/qmp/qnull.h
new file mode 100644
index 00..d075549283
--- /dev/null
+++ b/include/qapi/qmp/qnull.h
@@ -0,0 +1,30 @@
+/*
+ * QNull
+ *
+ * Copyright (C) 2015 Red Hat, Inc.
+ *
+ * Authors:
+ *  Markus Armbruster 
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1
+ * or later.  See the COPYING.LIB file in the top-level directory.
+ */
+
+#ifndef QNULL_H
+#define QNULL_H
+
+#include "qapi/qmp/qobject.h"
+
+struct QNull {
+QObject base;
+};
+
+extern QNull qnull_;
+
+static inline QNull *qnull(void)
+{
+QINCREF(_);
+return _;
+}
+
+#endif /* QNULL_H */
diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index eab29edd12..ef1d1a9237 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -93,16 +93,4 @@ static inline QType qobject_type(const QObject *obj)
 return obj->type;
 }
 
-struct QNull {
-QObject base;
-};
-
-extern QNull qnull_;
-
-static inline QNull *qnull(void)
-{
-QINCREF(_);
-return _;
-}
-
 #endif /* QOBJECT_H */
diff --git a/include/qapi/qmp/types.h b/include/qapi/qmp/types.h
index a4bc662bfb..749ac44dcb 100644
--- a/include/qapi/qmp/types.h
+++ b/include/qapi/qmp/types.h
@@ -19,5 +19,6 @@
 #include "qapi/qmp/qstring.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qlist.h"
+#include "qapi/qmp/qnull.h"
 
 #endif /* QAPI_QMP_TYPES_H */
diff --git a/qapi/qapi-clone-visitor.c b/qapi/qapi-clone-visitor.c
index d8b62792bc..daab6819b4 100644
--- a/qapi/qapi-clone-visitor.c
+++ b/qapi/qapi-clone-visitor.c
@@ -12,6 +12,7 @@
 #include "qapi/clone-visitor.h"
 #include "qapi/visitor-impl.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qnull.h"
 
 struct QapiCloneVisitor {
 Visitor visitor;
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index 67a0a4a58b..b3fdd0827d 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -16,6 +16,7 @@
 #include "qapi/string-input-visitor.h"
 #include "qapi/visitor-impl.h"
 #include "qapi/qmp/qerror.h"
+#include "qapi/qmp/qnull.h"
 #include "qemu/option.h"
 #include "qemu/queue.h"
 #include "qemu/range.h"
diff --git a/qobject/qnull.c b/qobject/qnull.c
index 69a21d1059..bc9fd31626 100644
--- a/qobject/qnull.c
+++ b/qobject/qnull.c
@@ -12,7 +12,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu-common.h"
-#include "qapi/qmp/qobject.h"
+#include "qapi/qmp/qnull.h"
 
 QNull qnull_ = {
 .base = {
diff --git a/tests/check-qnull.c b/tests/check-qnull.c
index 5c6eb0adc8..afa4400da1 100644
--- a/tests/check-qnull.c
+++ b/tests/check-qnull.c
@@ -8,7 +8,7 @@
  */
 #include "qemu/osdep.h"
 
-#include "qapi/qmp/qobject.h"
+#include "qapi/qmp/qnull.h"
 #include "qemu-common.h"
 #include "qapi/qobject-input-visitor.h"
 #include "qapi/qobject-output-visitor.h"
-- 
2.13.6

[Qemu-block] [PATCH for-2.11? v7 2/6] qapi/qlist: Add qlist_append_null() macro

[Max Reitz]

Besides the macro itself, this patch also adds a corresponding
Coccinelle rule.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Alberto Garcia 
---
 include/qapi/qmp/qlist.h | 3 +++
 scripts/coccinelle/qobject.cocci | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/include/qapi/qmp/qlist.h b/include/qapi/qmp/qlist.h
index c4b5fdad9b..59d209bbae 100644
--- a/include/qapi/qmp/qlist.h
+++ b/include/qapi/qmp/qlist.h
@@ -15,6 +15,7 @@
 
 #include "qapi/qmp/qobject.h"
 #include "qapi/qmp/qnum.h"
+#include "qapi/qmp/qnull.h"
 #include "qemu/queue.h"
 
 typedef struct QListEntry {
@@ -37,6 +38,8 @@ typedef struct QList {
 qlist_append(qlist, qbool_from_bool(value))
 #define qlist_append_str(qlist, value) \
 qlist_append(qlist, qstring_from_str(value))
+#define qlist_append_null(qlist) \
+qlist_append(qlist, qnull())
 
 #define QLIST_FOREACH_ENTRY(qlist, var) \
 for ((var) = ((qlist)->head.tqh_first); \
diff --git a/scripts/coccinelle/qobject.cocci b/scripts/coccinelle/qobject.cocci
index 1120eb1a42..47bcafe9a9 100644
--- a/scripts/coccinelle/qobject.cocci
+++ b/scripts/coccinelle/qobject.cocci
@@ -41,4 +41,7 @@ expression Obj, E;
 |
 - qlist_append(Obj, qstring_from_str(E));
 + qlist_append_str(Obj, E);
+|
+- qlist_append(Obj, qnull());
++ qlist_append_null(Obj);
 )
-- 
2.13.6

Re: [Qemu-block] [Nbd] [Qemu-devel] How to online resize qemu disk with nbd protocol?

[Wouter Verhelst]

On Tue, Nov 14, 2017 at 10:41:39AM -0600, Eric Blake wrote:
> Another thought - with structured replies, we finally have a way to let
> the client ask for the server to send resize information whenever the
> server wants, rather than having to be polled by a new client request
> all the time.  This is possible by having the server reply with a chunk
> without the NBD_REPLY_FLAG_DONE bit, for as many times as it wants,
> (that is, the server never officially ends the response to the single
> client request for on-going status, until the client sends an
> NBD_CMD_DISC).

Hrm, yeah, that could work.

Minor downside of this would be that a client would now be expected to
continue listening "forever" (probably needs to do a blocking read() or
a select() on the socket), whereas with the current situation a client
could get away with only reading for as long as it expects data.

I don't think that should be a blocker, but it might be something we
might want to document.

> I don't think the server should go into this mode without a flag bit
> from the client requesting it (as it potentially ties up a thread that
> could otherwise be used for parallel processing of other requests),

Yeah. I think we should probably initiate this with a BLOCK_STATUS
message that has a flag with which we mean "don't stop sending data on
the given region for contexts that support it".

However, I could imagine that there might be some cases wherein a server
might be able to go into such a mode for two or more metadata contexts,
and where a client might want to go into that mode for one of them but
not all of them, while still wanting some information from them.

This could be covered with metadata context syntax, but it's annoying
and shouldn't be necessary.

I'm starting to think I made a mistake when I said NBD_CMD_BLOCK_STATUS
can't take a metadata context ID. Okay, there's no space for it, but
that shouldn't have been a blocker.

Thoughts?

> and that the server could reject a repeat command with the flag if it
> is already serving a previous open-ended request.

Right.

On the other hand, I can imagine that a client might also want to tell
the server that it is no longer interested in an outstanding request. In
such a case, it should be able to cancel it.

-- 
Could you people please use IRC like normal people?!?

  -- Amaya Rodrigo Sastre, trying to quiet down the buzz in the DebConf 2008
 Hacklab

[Qemu-block] [PULL 19/20] block/parallels: add migration blocker

[Max Reitz]

From: Jeff Cody 

Migration does not work for parallels, and has been broken for a while
(see patch 'block/parallels: Do not update header or truncate image when
 INMIGRATE').  The bdrv_invalidate_cache() method needs to be added for
migration to be supported.  Until this is done, prohibit migration.

Signed-off-by: Jeff Cody 
Reviewed-by: Fam Zheng 
Message-id: 
5e04a7c8a3089913fa58d484af42dab7993984ad.1510059970.git.jc...@redhat.com
Reviewed-by: Stefan Hajnoczi 
Reviewed-by: Denis V. Lunev 
Signed-off-by: Max Reitz 
---
 block/parallels.c | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/block/parallels.c b/block/parallels.c
index 7b7a3efa1d..9545761f49 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -35,6 +35,7 @@
 #include "qemu/module.h"
 #include "qemu/bswap.h"
 #include "qemu/bitmap.h"
+#include "migration/blocker.h"
 
 /**/
 
@@ -100,6 +101,7 @@ typedef struct BDRVParallelsState {
 unsigned int tracks;
 
 unsigned int off_multiplier;
+Error *migration_blocker;
 } BDRVParallelsState;
 
 
@@ -720,6 +722,16 @@ static int parallels_open(BlockDriverState *bs, QDict 
*options, int flags,
 s->bat_dirty_bmap =
 bitmap_new(DIV_ROUND_UP(s->header_size, s->bat_dirty_block));
 
+/* Disable migration until bdrv_invalidate_cache method is added */
+error_setg(>migration_blocker, "The Parallels format used by node '%s' "
+   "does not support live migration",
+   bdrv_get_device_or_node_name(bs));
+ret = migrate_add_blocker(s->migration_blocker, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+error_free(s->migration_blocker);
+goto fail;
+}
 qemu_co_mutex_init(>lock);
 return 0;
 
@@ -750,6 +762,9 @@ static void parallels_close(BlockDriverState *bs)
 
 g_free(s->bat_dirty_bmap);
 qemu_vfree(s->header);
+
+migrate_del_blocker(s->migration_blocker);
+error_free(s->migration_blocker);
 }
 
 static QemuOptsList parallels_create_opts = {
-- 
2.13.6

[Qemu-block] [PULL 12/20] iotests: Make 136 less flaky

[Max Reitz]

136 executes some AIO requests without a final aio_flush; then it
advances the virtual clock and thus expects the last access time of the
device to be less than the current time when queried (i.e. idle_time_ns
to be greater than 0).  However, without the aio_flush, some requests
may be settled after the clock_step invocation.  In that case,
idle_time_ns would be 0 and the test fails.

Fix this by adding an aio_flush if any AIO request other than some other
aio_flush has been executed.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Stefan Hajnoczi 
Message-id: 20171109203025.27493-6-mre...@redhat.com
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/136 | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/136 b/tests/qemu-iotests/136
index 4b994897af..88b97ea7c6 100644
--- a/tests/qemu-iotests/136
+++ b/tests/qemu-iotests/136
@@ -238,6 +238,18 @@ sector = "%d"
 for i in range(failed_wr_ops):
 ops.append("aio_write %d 512" % bad_offset)
 
+# We need an extra aio_flush to settle all outstanding AIO
+# operations before we can advance the virtual clock, so that
+# the last access happens before clock_step and idle_time_ns
+# will be greater than 0
+extra_flush = 0
+if rd_ops + wr_ops + invalid_rd_ops + invalid_wr_ops + \
+failed_rd_ops + failed_wr_ops > 0:
+extra_flush = 1
+
+if extra_flush > 0:
+ops.append("aio_flush")
+
 if failed_wr_ops > 0:
 highest_offset = max(highest_offset, bad_offset + 512)
 
@@ -251,7 +263,7 @@ sector = "%d"
 self.total_wr_bytes += wr_ops * wr_size
 self.total_wr_ops += wr_ops
 self.total_wr_merged += wr_merged
-self.total_flush_ops += flush_ops
+self.total_flush_ops += flush_ops + extra_flush
 self.invalid_rd_ops += invalid_rd_ops
 self.invalid_wr_ops += invalid_wr_ops
 self.failed_rd_ops += failed_rd_ops
-- 
2.13.6

[Qemu-block] [PULL 11/20] iotests: Make 083 less flaky

[Max Reitz]

083 has (at least) two issues:

1. By launching the nbd-fault-injector in background, it may not be
   scheduled until the first grep on its output file is executed.
   However, until then, that file may not have been created yet -- so it
   either does not exist yet (thus making the grep emit an error), or it
   does exist but contains stale data (thus making the rest of the test
   case work connect to a wrong address).
   Fix this by explicitly overwriting the output file before executing
   nbd-fault-injector.

2. The nbd-fault-injector prints things other than "Listening on...".
   It also prints a "Closing connection" message from time to time.  We
   currently invoke sed on the whole file in the hope of it only
   containing the "Listening on..." line yet.  That hope is sometimes
   shattered by the brutal reality of race conditions, so make the sed
   script more robust.

Signed-off-by: Max Reitz 
Message-id: 20171109203025.27493-5-mre...@redhat.com
Reviewed-by: Eric Blake 
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/083 | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/083 b/tests/qemu-iotests/083
index 0306f112da..3c1adbf0fb 100755
--- a/tests/qemu-iotests/083
+++ b/tests/qemu-iotests/083
@@ -86,6 +86,7 @@ EOF
 
rm -f "$TEST_DIR/nbd.sock"
 
+echo > "$TEST_DIR/nbd-fault-injector.out"
$PYTHON nbd-fault-injector.py $extra_args "$nbd_addr" 
"$TEST_DIR/nbd-fault-injector.conf" >"$TEST_DIR/nbd-fault-injector.out" 2>&1 &
 
# Wait for server to be ready
@@ -94,7 +95,8 @@ EOF
done
 
# Extract the final address (port number has now been assigned in tcp 
case)
-   nbd_addr=$(sed 's/Listening on \(.*\)$/\1/' 
"$TEST_DIR/nbd-fault-injector.out")
+nbd_addr=$(sed -n 's/^Listening on //p' \
+   "$TEST_DIR/nbd-fault-injector.out")
 
if [ "$proto" = "tcp" ]; then
nbd_url="nbd+tcp://$nbd_addr/$export_name"
-- 
2.13.6

[Qemu-block] [PULL 09/20] iotests: Add missing 'blkdebug::' in 040

[Max Reitz]

040 tries to invoke pause_drive() on a drive that does not use blkdebug.
Good idea, but let's use blkdebug to make it actually work.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Stefan Hajnoczi 
Message-id: 20171109203025.27493-3-mre...@redhat.com
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/040 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index c284d08796..90b5b4f2ad 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -289,7 +289,7 @@ class TestSetSpeed(ImageCommitTestCase):
 qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % 
mid_img, test_img)
 qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0x1 0 512', test_img)
 qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0xef 524288 524288', 
mid_img)
-self.vm = iotests.VM().add_drive(test_img)
+self.vm = iotests.VM().add_drive('blkdebug::' + test_img)
 self.vm.launch()
 
 def tearDown(self):
-- 
2.13.6

Re: [Qemu-block] [PULL 00/20] Block patches for 2.11.0-rc1

[Peter Maydell]

On 14 November 2017 at 17:23, Max Reitz  wrote:
> The following changes since commit 191b5fbfa66e5b23e2150f3c6981d30eb84418a9:
>
>   Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' 
> into staging (2017-11-14 16:11:19 +)
>
> are available in the git repository at:
>
>   git://github.com/XanClic/qemu.git tags/pull-block-2017-11-14
>
> for you to fetch changes up to 8b2d7c364d9a2491f7501f6688cd722045cf808a:
>
>   qemu-iotests: update unsupported image formats in 194 (2017-11-14 18:06:26 
> +0100)
>

I can probably squeeze this into rc1, but 17:30 GMT on rc day is at least
an hour later than you can reasonably rely on being able to get a pull
request into the rc

thanks
-- PMM

Re: [Qemu-block] [PULL 00/20] Block patches for 2.11.0-rc1

[Max Reitz]

On 2017-11-14 18:28, Peter Maydell wrote:
> On 14 November 2017 at 17:23, Max Reitz  wrote:
>> The following changes since commit 191b5fbfa66e5b23e2150f3c6981d30eb84418a9:
>>
>>   Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' 
>> into staging (2017-11-14 16:11:19 +)
>>
>> are available in the git repository at:
>>
>>   git://github.com/XanClic/qemu.git tags/pull-block-2017-11-14
>>
>> for you to fetch changes up to 8b2d7c364d9a2491f7501f6688cd722045cf808a:
>>
>>   qemu-iotests: update unsupported image formats in 194 (2017-11-14 18:06:26 
>> +0100)
>>
> 
> I can probably squeeze this into rc1, but 17:30 GMT on rc day is at least
> an hour later than you can reasonably rely on being able to get a pull
> request into the rc

Sorry, I understand.

(I got two errors when testing the build, so I had to debug them and
drop a series -- which is one reason why it got delayed so much.  The
other is that I wanted to see if Kevin would be around today so that he
could do a combined pull request.)

I promise to do my build testing earlier next time...

Max



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PULL 20/20] qemu-iotests: update unsupported image formats in 194

[Max Reitz]

From: Jeff Cody 

Test 194 checks for 'luks' to exclude as an unsupported format,
However, most formats are unsupported, due to migration blockers.

Rather than specifying a blacklist of unsupported formats, whitelist
supported formats (specifically, qcow2, qed, raw, dmg).

Tested-by: Alexey Kardashevskiy 
Signed-off-by: Jeff Cody 
Message-id: 
23ca18c7f843c86a28b1529ca9ac6db4b35ca0e4.1510059970.git.jc...@redhat.com
Reviewed-by: Stefan Hajnoczi 
Reviewed-by: Denis V. Lunev 
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/194 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/194 b/tests/qemu-iotests/194
index 8d973b440f..1d4214aca3 100755
--- a/tests/qemu-iotests/194
+++ b/tests/qemu-iotests/194
@@ -21,7 +21,7 @@
 
 import iotests
 
-iotests.verify_image_format(unsupported_fmts=['luks'])
+iotests.verify_image_format(supported_fmts=['qcow2', 'qed', 'raw', 'dmg'])
 iotests.verify_platform(['linux'])
 
 with iotests.FilePath('source.img') as source_img_path, \
-- 
2.13.6

[Qemu-block] [PULL 17/20] block/vhdx.c: Don't blindly update the header

[Max Reitz]

From: Jeff Cody 

The VHDX specification requires that before user data modification of
the vhdx image, the VHDX header file and data GUIDs need to be updated.
In vhdx_open(), if the image is set to RDWR, we go ahead and update the
header.

However, just because the image is set to RDWR does not mean we can go
ahead and write at this point - specifically, if the QEMU run state is
INMIGRATE, the underlying file BS may be set to inactive via the BDS
open flag of BDRV_O_INACTIVE.  Attempting to write under this condition
will cause an assert in bdrv_co_pwritev().

We can alternatively latch the first time the image is written.  And lo
and behold, we do just that, via vhdx_user_visible_write() in
vhdx_co_writev().  This means the call to vhdx_update_headers() in
vhdx_open() is likely just vestigial, and can be removed.

Reported-by: Alexey Kardashevskiy 
Tested-by: Alexey Kardashevskiy 
Signed-off-by: Jeff Cody 
Message-id: 
659e4cdba6ef4c651737852777c8c93d27b38040.1510059970.git.jc...@redhat.com
Reviewed-by: Stefan Hajnoczi 
Reviewed-by: Denis V. Lunev 
Signed-off-by: Max Reitz 
---
 block/vhdx.c | 7 ---
 1 file changed, 7 deletions(-)

diff --git a/block/vhdx.c b/block/vhdx.c
index 7ae4589879..9956933da6 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1008,13 +1008,6 @@ static int vhdx_open(BlockDriverState *bs, QDict 
*options, int flags,
 goto fail;
 }
 
-if (flags & BDRV_O_RDWR) {
-ret = vhdx_update_headers(bs, s, false, NULL);
-if (ret < 0) {
-goto fail;
-}
-}
-
 /* TODO: differencing files */
 
 return 0;
-- 
2.13.6

[Qemu-block] [PULL 13/20] iotests: Use new-style NBD connections

[Max Reitz]

From: Eric Blake 

Old-style NBD is deprecated upstream (it is documented, but no
longer implemented in the reference implementation), and it is
severely limited (it cannot support structured replies, which
means it cannot support efficient handling of zeroes), when
compared to new-style NBD.  We are better off having our iotests
favor new-style everywhere (although some explicit tests,
particularly 83, still cover old-style for back-compat reasons);
this is as simple as supplying the empty string as the default
export name, as it does not change the URI needed to connect a
client to the server.  This also gives us more coverage of the
just-added structured reply code, when not overriding $QEMU_NBD
to intentionally point to an older server.

Signed-off-by: Eric Blake 
Message-id: 20171109221216.10248-1-ebl...@redhat.com
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/common.rc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 0e8a33c696..dbae7d74ba 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -242,7 +242,7 @@ _make_test_img()
 if [ $IMGPROTO = "nbd" ]; then
 # Pass a sufficiently high number to -e that should be enough for all
 # tests
-eval "$QEMU_NBD -v -t -b 127.0.0.1 -p 10810 -f $IMGFMT -e 42  
$TEST_IMG_FILE >/dev/null &"
+eval "$QEMU_NBD -v -t -b 127.0.0.1 -p 10810 -f $IMGFMT -e 42 -x '' 
$TEST_IMG_FILE >/dev/null &"
 sleep 1 # FIXME: qemu-nbd needs to be listening before we continue
 fi
 
-- 
2.13.6

[Qemu-block] [PULL 18/20] block/parallels: Do not update header or truncate image when INMIGRATE

[Max Reitz]

From: Jeff Cody 

If we write or modify the image file while the QEMU run state is
INMIGRATE, then the BDRV_O_INACTIVE BDS flag is set.  This will cause
an assert, since the image is marked inactive.  Make sure we obey this
flag.

Tested-by: Alexey Kardashevskiy 
Signed-off-by: Jeff Cody 
Message-id: 
3996c930fa8cde8570b7a63032720d76a28fd78b.1510059970.git.jc...@redhat.com
Reviewed-by: Stefan Hajnoczi 
Reviewed-by: Denis V. Lunev 
Signed-off-by: Max Reitz 
---
 block/parallels.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 2b6c6e5709..7b7a3efa1d 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -708,7 +708,7 @@ static int parallels_open(BlockDriverState *bs, QDict 
*options, int flags,
 s->prealloc_mode = PRL_PREALLOC_MODE_FALLOCATE;
 }
 
-if (flags & BDRV_O_RDWR) {
+if ((flags & BDRV_O_RDWR) && !(flags & BDRV_O_INACTIVE)) {
 s->header->inuse = cpu_to_le32(HEADER_INUSE_MAGIC);
 ret = parallels_update_header(bs);
 if (ret < 0) {
@@ -741,12 +741,9 @@ static void parallels_close(BlockDriverState *bs)
 {
 BDRVParallelsState *s = bs->opaque;
 
-if (bs->open_flags & BDRV_O_RDWR) {
+if ((bs->open_flags & BDRV_O_RDWR) && !(bs->open_flags & BDRV_O_INACTIVE)) 
{
 s->header->inuse = 0;
 parallels_update_header(bs);
-}
-
-if (bs->open_flags & BDRV_O_RDWR) {
 bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS,
   PREALLOC_MODE_OFF, NULL);
 }
-- 
2.13.6

[Qemu-block] [PULL 15/20] block/snapshot: dirty all dirty bitmaps on snapshot-switch

[Max Reitz]

From: Vladimir Sementsov-Ogievskiy 

Snapshot-switch actually changes active state of disk so it should
reflect on dirty bitmaps. Otherwise next incremental backup using
these bitmaps will be invalid.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Message-id: 20171023092945.54532-1-vsement...@virtuozzo.com
Reviewed-by: Eric Blake 
Reviewed-by: John Snow 
Signed-off-by: Max Reitz 
---
 block/snapshot.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/block/snapshot.c b/block/snapshot.c
index a46564e7b7..1d5ab5f90f 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -181,10 +181,24 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
 {
 BlockDriver *drv = bs->drv;
 int ret, open_ret;
+int64_t len;
 
 if (!drv) {
 return -ENOMEDIUM;
 }
+
+len = bdrv_getlength(bs);
+if (len < 0) {
+return len;
+}
+/* We should set all bits in all enabled dirty bitmaps, because dirty
+ * bitmaps reflect active state of disk and snapshot switch operation
+ * actually dirties active state.
+ * TODO: It may make sense not to set all bits but analyze block status of
+ * current state and destination snapshot and do not set bits corresponding
+ * to both-zero or both-unallocated areas. */
+bdrv_set_dirty(bs, 0, len);
+
 if (drv->bdrv_snapshot_goto) {
 return drv->bdrv_snapshot_goto(bs, snapshot_id);
 }
-- 
2.13.6

[Qemu-block] [PULL 10/20] iotests: Make 055 less flaky

[Max Reitz]

First of all, test 055 does a valiant job of invoking pause_drive()
sometimes, but that is worth nothing without blkdebug.  So the first
thing to do is to sprinkle a couple of "blkdebug::" in there -- with the
exception of the transaction tests, because the blkdebug break points
make the transaction QMP command hang (which is bad).  In that case, we
can get away with throttling the block job that it effectively is
paused.

Then, 055 usually does not pause the drive before starting a block job
that should be cancelled.  This means that the backup job might be
completed already before block-job-cancel is invoked; thus making the
test either fail (currently) or moot if cancel_and_wait() ignored this
condition.  Fix this by pausing the drive before starting the job.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Stefan Hajnoczi 
Message-id: 20171109203025.27493-4-mre...@redhat.com
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/055 | 25 -
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index e1206caf9b..8a5d9fd269 100755
--- a/tests/qemu-iotests/055
+++ b/tests/qemu-iotests/055
@@ -48,7 +48,7 @@ class TestSingleDrive(iotests.QMPTestCase):
 def setUp(self):
 qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img, 
str(image_len))
 
-self.vm = iotests.VM().add_drive(test_img)
+self.vm = iotests.VM().add_drive('blkdebug::' + test_img)
 self.vm.add_drive(blockdev_target_img, interface="none")
 if iotests.qemu_default_machine == 'pc':
 self.vm.add_drive(None, 'media=cdrom', 'ide')
@@ -65,10 +65,11 @@ class TestSingleDrive(iotests.QMPTestCase):
 def do_test_cancel(self, cmd, target):
 self.assert_no_active_block_jobs()
 
+self.vm.pause_drive('drive0')
 result = self.vm.qmp(cmd, device='drive0', target=target, sync='full')
 self.assert_qmp(result, 'return', {})
 
-event = self.cancel_and_wait()
+event = self.cancel_and_wait(resume=True)
 self.assert_qmp(event, 'data/type', 'backup')
 
 def test_cancel_drive_backup(self):
@@ -166,7 +167,7 @@ class TestSetSpeed(iotests.QMPTestCase):
 def setUp(self):
 qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img, 
str(image_len))
 
-self.vm = iotests.VM().add_drive(test_img)
+self.vm = iotests.VM().add_drive('blkdebug::' + test_img)
 self.vm.add_drive(blockdev_target_img, interface="none")
 self.vm.launch()
 
@@ -246,6 +247,8 @@ class TestSetSpeed(iotests.QMPTestCase):
 def test_set_speed_invalid_blockdev_backup(self):
 self.do_test_set_speed_invalid('blockdev-backup',  'drive1')
 
+# Note: We cannot use pause_drive() here, or the transaction command
+#   would stall.  Instead, we limit the block job speed here.
 class TestSingleTransaction(iotests.QMPTestCase):
 def setUp(self):
 qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img, 
str(image_len))
@@ -271,7 +274,8 @@ class TestSingleTransaction(iotests.QMPTestCase):
 'type': cmd,
 'data': { 'device': 'drive0',
   'target': target,
-  'sync': 'full' },
+  'sync': 'full',
+  'speed': 64 * 1024 },
 }
 ])
 
@@ -289,12 +293,12 @@ class TestSingleTransaction(iotests.QMPTestCase):
 def do_test_pause(self, cmd, target, image):
 self.assert_no_active_block_jobs()
 
-self.vm.pause_drive('drive0')
 result = self.vm.qmp('transaction', actions=[{
 'type': cmd,
 'data': { 'device': 'drive0',
   'target': target,
-  'sync': 'full' },
+  'sync': 'full',
+  'speed': 64 * 1024 },
 }
 ])
 self.assert_qmp(result, 'return', {})
@@ -302,7 +306,9 @@ class TestSingleTransaction(iotests.QMPTestCase):
 result = self.vm.qmp('block-job-pause', device='drive0')
 self.assert_qmp(result, 'return', {})
 
-self.vm.resume_drive('drive0')
+result = self.vm.qmp('block-job-set-speed', device='drive0', speed=0)
+self.assert_qmp(result, 'return', {})
+
 self.pause_job('drive0')
 
 result = self.vm.qmp('query-block-jobs')
@@ -461,7 +467,7 @@ class TestDriveCompression(iotests.QMPTestCase):
 pass
 
 def do_prepare_drives(self, fmt, args, attach_target):
-self.vm = iotests.VM().add_drive(test_img)
+self.vm = iotests.VM().add_drive('blkdebug::' + test_img)
 
 qemu_img('create', '-f', fmt, blockdev_target_img,
  str(TestDriveCompression.image_len), *args)
@@ -500,10 +506,11 @@ class TestDriveCompression(iotests.QMPTestCase):
 
 

[Qemu-block] [PULL 16/20] iotests: 077: Filter out 'resume' lines

[Max Reitz]

From: Fam Zheng 

In the "Overlapping multiple requests" cases, the 3rd reqs (the break
point B) doesn't wait for the 2nd, and once resumed the I/O will just
continue.  This is because the 2nd is already waiting for the 1st, and
in wait_serialising_requests() there is:

/* If the request is already (indirectly) waiting for us, or
 * will wait for us as soon as it wakes up, then just go on
 * (instead of producing a deadlock in the former case). */
if (!req->waiting_for) {
/* actually break */
...
}

Consequently, the following "sleep 100; resume A" command races with the
completion of that request, and sometimes results in an unexpected
order of output:

> @@ -56,9 +56,9 @@
>  wrote XXX/XXX bytes at offset XXX
>  XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  blkdebug: Resuming request 'B'
> +blkdebug: Resuming request 'A'
>  wrote XXX/XXX bytes at offset XXX
>  XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> -blkdebug: Resuming request 'A'
>  wrote XXX/XXX bytes at offset XXX
>  XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  wrote XXX/XXX bytes at offset XXX

Filter out the "Resuming request" lines to make the output
deterministic.

Reported-by: Patchew 
Signed-off-by: Fam Zheng 
Message-id: 20171113150026.4743-1-f...@redhat.com
Reviewed-by: Eric Blake 
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/077 |  3 ++-
 tests/qemu-iotests/077.out | 16 
 2 files changed, 2 insertions(+), 17 deletions(-)

diff --git a/tests/qemu-iotests/077 b/tests/qemu-iotests/077
index d2d2a2d687..b3c6fb1370 100755
--- a/tests/qemu-iotests/077
+++ b/tests/qemu-iotests/077
@@ -188,7 +188,8 @@ EOF
 test_io | $QEMU_IO  | _filter_qemu_io | \
 sed -e 's,[0-9/]* bytes at offset [0-9]*,XXX/XXX bytes at offset XXX,g' \
 -e 's/^[0-9]* \(bytes\|KiB\)/XXX bytes/' \
--e '/Suspended/d'
+-e '/Suspended/d' \
+-e '/blkdebug: Resuming request/d'
 
 echo
 echo "== Verify image content =="
diff --git a/tests/qemu-iotests/077.out b/tests/qemu-iotests/077.out
index 16f951fd3d..4aae82f2e2 100644
--- a/tests/qemu-iotests/077.out
+++ b/tests/qemu-iotests/077.out
@@ -4,17 +4,14 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
 == Some concurrent requests involving RMW ==
 wrote XXX/XXX bytes at offset XXX
 XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-blkdebug: Resuming request 'A'
 wrote XXX/XXX bytes at offset XXX
 XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote XXX/XXX bytes at offset XXX
 XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-blkdebug: Resuming request 'A'
 wrote XXX/XXX bytes at offset XXX
 XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote XXX/XXX bytes at offset XXX
 XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-blkdebug: Resuming request 'A'
 wrote XXX/XXX bytes at offset XXX
 XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote XXX/XXX bytes at offset XXX
@@ -31,51 +28,38 @@ wrote XXX/XXX bytes at offset XXX
 XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote XXX/XXX bytes at offset XXX
 XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-blkdebug: Resuming request 'A'
 wrote XXX/XXX bytes at offset XXX
 XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-blkdebug: Resuming request 'B'
 wrote XXX/XXX bytes at offset XXX
 XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote XXX/XXX bytes at offset XXX
 XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-blkdebug: Resuming request 'B'
 wrote XXX/XXX bytes at offset XXX
 XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-blkdebug: Resuming request 'A'
 wrote XXX/XXX bytes at offset XXX
 XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote XXX/XXX bytes at offset XXX
 XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-blkdebug: Resuming request 'A'
 wrote XXX/XXX bytes at offset XXX
 XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-blkdebug: Resuming request 'B'
 wrote XXX/XXX bytes at offset XXX
 XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote XXX/XXX bytes at offset XXX
 XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-blkdebug: Resuming request 'B'
 wrote XXX/XXX bytes at offset XXX
 XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-blkdebug: Resuming request 'A'
 wrote XXX/XXX bytes at offset XXX
 XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote XXX/XXX bytes at offset XXX
 XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-blkdebug: Resuming request 'A'
 wrote XXX/XXX bytes at offset XXX
 XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote XXX/XXX bytes at offset XXX
 XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-blkdebug: Resuming request 'A'
-blkdebug: Resuming request 'C'
 

[Qemu-block] [PULL 07/20] qcow2: Assert that the crypto header does not overlap other metadata

[Max Reitz]

From: Alberto Garcia 

The crypto header is initialized only when QEMU is creating a new
image, so there's no chance of this happening on a corrupted image.

If QEMU is really trying to allocate the header overlapping other
existing metadata sections then this is a serious bug in QEMU itself
so let's add an assertion.

Signed-off-by: Alberto Garcia 
Message-id: 
ae3d77f312fc0c5e0ac2bbd71676c0112eebe2e5.1509718618.git.be...@igalia.com
Reviewed-by: Daniel P. Berrange 
Signed-off-by: Max Reitz 
---
 block/qcow2.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index defc1fe49f..b3d66a0e88 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -126,6 +126,7 @@ static ssize_t qcow2_crypto_hdr_init_func(QCryptoBlock 
*block, size_t headerlen,
 /* Zero fill remaining space in cluster so it has predictable
  * content in case of future spec changes */
 clusterlen = size_to_clusters(s, headerlen) * s->cluster_size;
+assert(qcow2_pre_write_overlap_check(bs, 0, ret, clusterlen) == 0);
 ret = bdrv_pwrite_zeroes(bs->file,
  ret + headerlen,
  clusterlen - headerlen, 0);
-- 
2.13.6

[Qemu-block] [PULL 06/20] qcow2: Add iotest for an empty refcount table

[Max Reitz]

From: Alberto Garcia 

This patch adds a simple iotest in which we try to write to an image
with an empty refcount table (i.e. with all entries set to 0).

This scenario was already handled by the existing consistency checks,
but we add an explicit test case for completeness.

Signed-off-by: Alberto Garcia 
Reviewed-by: Max Reitz 
Message-id: 
7e48b0e2ae1a0a18e0ee303b3045f130feec0474.1509718618.git.be...@igalia.com
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/060 | 7 +++
 tests/qemu-iotests/060.out | 6 ++
 2 files changed, 13 insertions(+)

diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index dc5a517673..66a8fa4aea 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -243,6 +243,13 @@ poke_file "$TEST_IMG" "$(($l2_offset+8))" 
"\x80\x00\x00\x00\x00\x06\x2a\x00"
 $QEMU_IO -c "discard 0 64k" -c "read 64k 64k" "$TEST_IMG" | _filter_qemu_io
 
 echo
+echo "=== Testing empty refcount table ==="
+echo
+_make_test_img 64M
+poke_file "$TEST_IMG" "$rt_offset""\x00\x00\x00\x00\x00\x00\x00\x00"
+$QEMU_IO -c "write 0 64k" "$TEST_IMG" | _filter_qemu_io
+
+echo
 echo "=== Testing empty refcount table with valid L1 and L2 tables ==="
 echo
 _make_test_img 64M
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index 98f314c16d..cfd78f87a9 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -182,6 +182,12 @@ discard 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read failed: Input/output error
 
+=== Testing empty refcount table ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+qcow2: Marking image as corrupt: Preventing invalid write on metadata 
(overlaps with refcount table); further corruption events will be suppressed
+write failed: Input/output error
+
 === Testing empty refcount table with valid L1 and L2 tables ===
 
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-- 
2.13.6

[Qemu-block] [PULL 08/20] iotests: Make 030 less flaky

[Max Reitz]

This patch fixes two race conditions in 030:

1. The first is in TestENOSPC.test_enospc().  After resuming the job,
   querying it to confirm it is no longer paused may fail because in the
   meantime it might have completed already.  The same was fixed in
   TestEIO.test_ignore() already (in commit
   2c3b44da07d341557a8203cc509ea07fe3605e11).

2. The second is in TestSetSpeed.test_set_speed_invalid(): Here, a
   stream job is started on a drive without any break points, with a
   block-job-set-speed invoked subsequently.  However, without any break
   points, the job might have completed in the meantime (on tmpfs at
   least); or it might complete before cancel_and_wait() which expects
   the job to still exist.  This can be fixed like everywhere else by
   pausing the drive (installing break points) before starting the job
   and letting cancel_and_wait() resume it.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Stefan Hajnoczi 
Message-id: 20171109203025.27493-2-mre...@redhat.com
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/030 | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 18838948fa..457984b8e9 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -666,6 +666,7 @@ class TestENOSPC(TestErrors):
 if event['event'] == 'BLOCK_JOB_ERROR':
 self.assert_qmp(event, 'data/device', 'drive0')
 self.assert_qmp(event, 'data/operation', 'read')
+error = True
 
 result = self.vm.qmp('query-block-jobs')
 self.assert_qmp(result, 'return[0]/paused', True)
@@ -676,9 +677,11 @@ class TestENOSPC(TestErrors):
 self.assert_qmp(result, 'return', {})
 
 result = self.vm.qmp('query-block-jobs')
+if result == {'return': []}:
+# Race; likely already finished. Check.
+continue
 self.assert_qmp(result, 'return[0]/paused', False)
 self.assert_qmp(result, 'return[0]/io-status', 'ok')
-error = True
 elif event['event'] == 'BLOCK_JOB_COMPLETED':
 self.assertTrue(error, 'job completed unexpectedly')
 self.assert_qmp(event, 'data/type', 'stream')
@@ -792,13 +795,14 @@ class TestSetSpeed(iotests.QMPTestCase):
 
 self.assert_no_active_block_jobs()
 
+self.vm.pause_drive('drive0')
 result = self.vm.qmp('block-stream', device='drive0')
 self.assert_qmp(result, 'return', {})
 
 result = self.vm.qmp('block-job-set-speed', device='drive0', speed=-1)
 self.assert_qmp(result, 'error/class', 'GenericError')
 
-self.cancel_and_wait()
+self.cancel_and_wait(resume=True)
 
 if __name__ == '__main__':
 iotests.main(supported_fmts=['qcow2', 'qed'])
-- 
2.13.6

[Qemu-block] [PULL 04/20] qcow2: Don't open images with header.refcount_table_clusters == 0

[Max Reitz]

From: Alberto Garcia 

qcow2_do_open() is checking that header.refcount_table_clusters is not
too large, but it doesn't check that it's greater than zero. Apart
from the fact that an image like that is obviously corrupted, trying
to use it crashes QEMU since we end up with a null s->refcount_table
after qcow2_refcount_init().

These images can however be repaired, so allow opening them if the
BDRV_O_CHECK flag is set.

Signed-off-by: Alberto Garcia 
Reviewed-by: Max Reitz 
Message-id: 
f9750f50c80359babba11062e88f5075a47e8e16.1509718618.git.be...@igalia.com
Signed-off-by: Max Reitz 
---
 block/qcow2.c  | 6 ++
 tests/qemu-iotests/060 | 7 +++
 tests/qemu-iotests/060.out | 5 +
 3 files changed, 18 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index 92cb9f9bfa..defc1fe49f 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1280,6 +1280,12 @@ static int qcow2_do_open(BlockDriverState *bs, QDict 
*options, int flags,
 goto fail;
 }
 
+if (header.refcount_table_clusters == 0 && !(flags & BDRV_O_CHECK)) {
+error_setg(errp, "Image does not contain a reference count table");
+ret = -EINVAL;
+goto fail;
+}
+
 ret = validate_table_offset(bs, s->refcount_table_offset,
 s->refcount_table_size, sizeof(uint64_t));
 if (ret < 0) {
diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index c3bce27b33..656af50883 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -270,6 +270,13 @@ poke_file "$TEST_IMG" "$rb_offset"
"\x00\x00\x00\x00\x00\x00\x00\x00"
 # write will try to allocate a compressed data cluster at offset 0.
 $QEMU_IO -c "write -c 0k 64k" "$TEST_IMG" | _filter_qemu_io
 
+echo
+echo "=== Testing zero refcount table size ==="
+echo
+_make_test_img 64M
+poke_file "$TEST_IMG" "56""\x00\x00\x00\x00"
+$QEMU_IO -c "write 0 64k" "$TEST_IMG" 2>&1 | _filter_testdir | _filter_imgfmt
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index cf8790ff57..58456e8487 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -203,4 +203,9 @@ wrote 65536/65536 bytes at offset 65536
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 qcow2: Marking image as corrupt: Preventing invalid allocation of compressed 
cluster at offset 0; further corruption events will be suppressed
 write failed: Input/output error
+
+=== Testing zero refcount table size ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+can't open device TEST_DIR/t.IMGFMT: Image does not contain a reference count 
table
 *** done
-- 
2.13.6

[Qemu-block] [PULL 01/20] qcow2: Prevent allocating refcount blocks at offset 0

[Max Reitz]

From: Alberto Garcia 

Each entry in the qcow2 cache contains an offset field indicating the
location of the data in the qcow2 image. If the offset is 0 then it
means that the entry contains no data and is available to be used when
needed.

Because of that it is not possible to store in the cache the first
cluster of the qcow2 image (offset = 0). This is not a problem because
that cluster always contains the qcow2 header and we're not using this
cache for that.

However, if the qcow2 image is corrupted it can happen that we try to
allocate a new refcount block at offset 0, triggering this assertion
and crashing QEMU:

  qcow2_cache_entry_mark_dirty: Assertion `c->entries[i].offset != 0' failed

This patch adds an explicit check for this scenario and a new test
case.

This problem was originally reported here:

   https://bugs.launchpad.net/qemu/+bug/1728615

Reported-by: R.Nageswara Sastry 
Signed-off-by: Alberto Garcia 
Reviewed-by: Max Reitz 
Message-id: 
92a2fadd10d58b423f269c1d1a309af161cdc73f.1509718618.git.be...@igalia.com
Signed-off-by: Max Reitz 
---
 block/qcow2-refcount.c |  7 +++
 tests/qemu-iotests/060 | 11 +++
 tests/qemu-iotests/060.out |  8 
 3 files changed, 26 insertions(+)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index aa3fd6cf17..9059996c4b 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -367,6 +367,13 @@ static int alloc_refcount_block(BlockDriverState *bs,
 return new_block;
 }
 
+/* If we're allocating the block at offset 0 then something is wrong */
+if (new_block == 0) {
+qcow2_signal_corruption(bs, true, -1, -1, "Preventing invalid "
+"allocation of refcount block at offset 0");
+return -EIO;
+}
+
 #ifdef DEBUG_ALLOC2
 fprintf(stderr, "qcow2: Allocate refcount block %d for %" PRIx64
 " at %" PRIx64 "\n",
diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index 8e95c450eb..dead26aeaf 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -242,6 +242,17 @@ poke_file "$TEST_IMG" "$(($l2_offset+8))" 
"\x80\x00\x00\x00\x00\x06\x2a\x00"
 # Should emit two error messages
 $QEMU_IO -c "discard 0 64k" -c "read 64k 64k" "$TEST_IMG" | _filter_qemu_io
 
+echo
+echo "=== Testing empty refcount table with valid L1 and L2 tables ==="
+echo
+_make_test_img 64M
+$QEMU_IO -c "write 0 64k" "$TEST_IMG" | _filter_qemu_io
+poke_file "$TEST_IMG" "$rt_offset""\x00\x00\x00\x00\x00\x00\x00\x00"
+# Since the first data cluster is already allocated this triggers an
+# allocation with an explicit offset (using qcow2_alloc_clusters_at())
+# causing a refcount block to be allocated at offset 0
+$QEMU_IO -c "write 0 128k" "$TEST_IMG" | _filter_qemu_io
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index 5ca3af491f..872719009c 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -181,4 +181,12 @@ qcow2: Marking image as corrupt: Cluster allocation offset 
0x62a00 unaligned (L2
 discard 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read failed: Input/output error
+
+=== Testing empty refcount table with valid L1 and L2 tables ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+qcow2: Marking image as corrupt: Preventing invalid allocation of refcount 
block at offset 0; further corruption events will be suppressed
+write failed: Input/output error
 *** done
-- 
2.13.6

[Qemu-block] [PULL 05/20] qcow2: Add iotest for an image with header.refcount_table_offset == 0

[Max Reitz]

From: Alberto Garcia 

This patch adds a simple iotest in which we try to write to an image
with the refcount table offset set to 0.

This scenario was already handled by the existing consistency checks,
but we add an explicit test case for completeness.

Signed-off-by: Alberto Garcia 
Message-id: 
feeceada92486bb8790b90f303fc9fe82a27391a.1509718618.git.be...@igalia.com
Reviewed-by: Max Reitz 
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/060 | 7 +++
 tests/qemu-iotests/060.out | 6 ++
 2 files changed, 13 insertions(+)

diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index 656af50883..dc5a517673 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -277,6 +277,13 @@ _make_test_img 64M
 poke_file "$TEST_IMG" "56""\x00\x00\x00\x00"
 $QEMU_IO -c "write 0 64k" "$TEST_IMG" 2>&1 | _filter_testdir | _filter_imgfmt
 
+echo
+echo "=== Testing incorrect refcount table offset ==="
+echo
+_make_test_img 64M
+poke_file "$TEST_IMG" "48""\x00\x00\x00\x00\x00\x00\x00\x00"
+$QEMU_IO -c "write 0 64k" "$TEST_IMG" | _filter_qemu_io
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index 58456e8487..98f314c16d 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -208,4 +208,10 @@ write failed: Input/output error
 
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
 can't open device TEST_DIR/t.IMGFMT: Image does not contain a reference count 
table
+
+=== Testing incorrect refcount table offset ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+qcow2: Marking image as corrupt: Preventing invalid allocation of L2 table at 
offset 0; further corruption events will be suppressed
+write failed: Input/output error
 *** done
-- 
2.13.6

[Qemu-block] [PULL 03/20] qcow2: Prevent allocating compressed clusters at offset 0

[Max Reitz]

From: Alberto Garcia 

If the refcount data is corrupted then we can end up trying to
allocate a new compressed cluster at offset 0 in the image, triggering
an assertion in qcow2_alloc_bytes() that would crash QEMU:

  qcow2_alloc_bytes: Assertion `offset' failed.

This patch adds an explicit check for this scenario and a new test
case.

Signed-off-by: Alberto Garcia 
Message-id: 
fb53467cf48e95ff3330def1cf1003a5b862b7d9.1509718618.git.be...@igalia.com
Reviewed-by: Max Reitz 
Signed-off-by: Max Reitz 
---
 block/qcow2-refcount.c |  7 +++
 tests/qemu-iotests/060 | 10 ++
 tests/qemu-iotests/060.out |  8 
 3 files changed, 25 insertions(+)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 9059996c4b..60b8eef3e8 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1082,6 +1082,13 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size)
 return new_cluster;
 }
 
+if (new_cluster == 0) {
+qcow2_signal_corruption(bs, true, -1, -1, "Preventing invalid "
+"allocation of compressed cluster "
+"at offset 0");
+return -EIO;
+}
+
 if (!offset || ROUND_UP(offset, s->cluster_size) != new_cluster) {
 offset = new_cluster;
 free_in_cluster = s->cluster_size;
diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index 40f85cc216..c3bce27b33 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -260,6 +260,16 @@ _make_test_img 64M
 poke_file "$TEST_IMG" "$rb_offset""\x00\x00\x00\x00\x00\x00\x00\x00"
 $QEMU_IO -c "write 0 64k" "$TEST_IMG" | _filter_qemu_io
 
+echo
+echo "=== Testing empty refcount block with compressed write ==="
+echo
+_make_test_img 64M
+$QEMU_IO -c "write 64k 64k" "$TEST_IMG" | _filter_qemu_io
+poke_file "$TEST_IMG" "$rb_offset""\x00\x00\x00\x00\x00\x00\x00\x00"
+# The previous write already allocated an L2 table, so now this new
+# write will try to allocate a compressed data cluster at offset 0.
+$QEMU_IO -c "write -c 0k 64k" "$TEST_IMG" | _filter_qemu_io
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index 5b8b518486..cf8790ff57 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -195,4 +195,12 @@ write failed: Input/output error
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
 qcow2: Marking image as corrupt: Preventing invalid allocation of L2 table at 
offset 0; further corruption events will be suppressed
 write failed: Input/output error
+
+=== Testing empty refcount block with compressed write ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 65536/65536 bytes at offset 65536
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+qcow2: Marking image as corrupt: Preventing invalid allocation of compressed 
cluster at offset 0; further corruption events will be suppressed
+write failed: Input/output error
 *** done
-- 
2.13.6

[Qemu-block] [PULL 02/20] qcow2: Prevent allocating L2 tables at offset 0

[Max Reitz]

From: Alberto Garcia 

If the refcount data is corrupted then we can end up trying to
allocate a new L2 table at offset 0 in the image, triggering an
assertion in the qcow2 cache that would crash QEMU:

  qcow2_cache_entry_mark_dirty: Assertion `c->entries[i].offset != 0' failed

This patch adds an explicit check for this scenario and a new test
case.

Signed-off-by: Alberto Garcia 
Reviewed-by: Max Reitz 
Message-id: 
92dac37191ae7844a2da22c122204eb493cc3133.1509718618.git.be...@igalia.com
Signed-off-by: Max Reitz 
---
 block/qcow2-cluster.c  | 8 
 tests/qemu-iotests/060 | 7 +++
 tests/qemu-iotests/060.out | 6 ++
 3 files changed, 21 insertions(+)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index fb10e26068..2e072ed155 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -278,6 +278,14 @@ static int l2_allocate(BlockDriverState *bs, int l1_index, 
uint64_t **table)
 goto fail;
 }
 
+/* If we're allocating the table at offset 0 then something is wrong */
+if (l2_offset == 0) {
+qcow2_signal_corruption(bs, true, -1, -1, "Preventing invalid "
+"allocation of L2 table at offset 0");
+ret = -EIO;
+goto fail;
+}
+
 ret = qcow2_cache_flush(bs, s->refcount_block_cache);
 if (ret < 0) {
 goto fail;
diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index dead26aeaf..40f85cc216 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -253,6 +253,13 @@ poke_file "$TEST_IMG" "$rt_offset"
"\x00\x00\x00\x00\x00\x00\x00\x00"
 # causing a refcount block to be allocated at offset 0
 $QEMU_IO -c "write 0 128k" "$TEST_IMG" | _filter_qemu_io
 
+echo
+echo "=== Testing empty refcount block ==="
+echo
+_make_test_img 64M
+poke_file "$TEST_IMG" "$rb_offset""\x00\x00\x00\x00\x00\x00\x00\x00"
+$QEMU_IO -c "write 0 64k" "$TEST_IMG" | _filter_qemu_io
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index 872719009c..5b8b518486 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -189,4 +189,10 @@ wrote 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 qcow2: Marking image as corrupt: Preventing invalid allocation of refcount 
block at offset 0; further corruption events will be suppressed
 write failed: Input/output error
+
+=== Testing empty refcount block ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+qcow2: Marking image as corrupt: Preventing invalid allocation of L2 table at 
offset 0; further corruption events will be suppressed
+write failed: Input/output error
 *** done
-- 
2.13.6

[Qemu-block] [PULL 00/20] Block patches for 2.11.0-rc1

[Max Reitz]

The following changes since commit 191b5fbfa66e5b23e2150f3c6981d30eb84418a9:

  Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' into 
staging (2017-11-14 16:11:19 +)

are available in the git repository at:

  git://github.com/XanClic/qemu.git tags/pull-block-2017-11-14

for you to fetch changes up to 8b2d7c364d9a2491f7501f6688cd722045cf808a:

  qemu-iotests: update unsupported image formats in 194 (2017-11-14 18:06:26 
+0100)


Block patches for 2.11.0-rc1


Alberto Garcia (8):
  qcow2: Prevent allocating refcount blocks at offset 0
  qcow2: Prevent allocating L2 tables at offset 0
  qcow2: Prevent allocating compressed clusters at offset 0
  qcow2: Don't open images with header.refcount_table_clusters == 0
  qcow2: Add iotest for an image with header.refcount_table_offset == 0
  qcow2: Add iotest for an empty refcount table
  qcow2: Assert that the crypto header does not overlap other metadata
  qcow2: Check that corrupted images can be repaired in iotest 060

Eric Blake (1):
  iotests: Use new-style NBD connections

Fam Zheng (1):
  iotests: 077: Filter out 'resume' lines

Jeff Cody (4):
  block/vhdx.c: Don't blindly update the header
  block/parallels: Do not update header or truncate image when INMIGRATE
  block/parallels: add migration blocker
  qemu-iotests: update unsupported image formats in 194

Max Reitz (5):
  iotests: Make 030 less flaky
  iotests: Add missing 'blkdebug::' in 040
  iotests: Make 055 less flaky
  iotests: Make 083 less flaky
  iotests: Make 136 less flaky

Vladimir Sementsov-Ogievskiy (1):
  block/snapshot: dirty all dirty bitmaps on snapshot-switch

 block/parallels.c|  22 ++---
 block/qcow2-cluster.c|   8 
 block/qcow2-refcount.c   |  14 ++
 block/qcow2.c|   7 +++
 block/snapshot.c |  14 ++
 block/vhdx.c |   7 ---
 tests/qemu-iotests/030   |   8 +++-
 tests/qemu-iotests/040   |   2 +-
 tests/qemu-iotests/055   |  25 +++
 tests/qemu-iotests/060   |  59 +
 tests/qemu-iotests/060.out   | 103 +++
 tests/qemu-iotests/077   |   3 +-
 tests/qemu-iotests/077.out   |  16 ---
 tests/qemu-iotests/083   |   4 +-
 tests/qemu-iotests/136   |  14 +-
 tests/qemu-iotests/194   |   2 +-
 tests/qemu-iotests/common.rc |   2 +-
 17 files changed, 265 insertions(+), 45 deletions(-)

-- 
2.13.6

Re: [Qemu-block] [PATCH 1/5 for-2.11?] qcow2: reject unaligned offsets in write compressed

[Eric Blake]

On 11/14/2017 04:16 AM, Anton Nefedov wrote:
> Misaligned compressed write is not supported.
> 
> Signed-off-by: Anton Nefedov 
> ---
>  block/qcow2.c | 4 
>  1 file changed, 4 insertions(+)

Should this one be applied in 2.11?

Reviewed-by: Eric Blake 

> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 92cb9f9..45c5651 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3349,6 +3349,10 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, 
> uint64_t offset,
>  return bdrv_truncate(bs->file, cluster_offset, PREALLOC_MODE_OFF, 
> NULL);
>  }
>  
> +if (offset_into_cluster(s, offset)) {
> +return -EINVAL;
> +}
> +
>  buf = qemu_blockalign(bs, s->cluster_size);
>  if (bytes != s->cluster_size) {
>  if (bytes > s->cluster_size ||
> 

-- 
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] [Nbd] [Qemu-devel] How to online resize qemu disk with nbd protocol?

[Eric Blake]

[reviving an old thread]

On 01/23/2017 08:54 AM, Eric Blake wrote:

> I'm still thinking that allowing the client to query the current size is
> useful.  Over the weekend, I was thinking of SEEK_SET/SEEK_END semantics
> (SEEK_CUR doesn't really make sense, since we don't maintain a current
> offset), and wondering if we could improve the command as follows:
> 
> If invoked without flags, it operates with SEEK_SET semantics (the
> offset is the new requested size); if invoked with
> NBD_CMD_FLAG_RELATIVE, it operates with SEEK_END semantics (the offset
> is added to the existing size, and can be treated as a signed 64-bit
> negative number to shrink).  Either way, on success, the command replies
> with a structured reply containing the new 64-bit total size of the
> disk.  This would require the reply to be a structured reply, and the
> semantics of NBD_CMD_FLAG_RELATIVE with an offset of 0 becomes a way to
> probe the existing disk size (thus enabling the server to resize without
> client request, and perhaps used with some out-of-band means for the
> client to know that it should query the server for an updated size).
> Also note that the server's reply of the current size may be slightly
> different than what was requested by the client (that is, we should
> allow the server to round the client's request up to an appropriate
> granularity) - we should probably require that the server can only round
> up (not down).
> 
> Thoughts?

Another thought - with structured replies, we finally have a way to let
the client ask for the server to send resize information whenever the
server wants, rather than having to be polled by a new client request
all the time.  This is possible by having the server reply with a chunk
without the NBD_REPLY_FLAG_DONE bit, for as many times as it wants,
(that is, the server never officially ends the response to the single
client request for on-going status, until the client sends an
NBD_CMD_DISC).  I don't think the server should go into this mode
without a flag bit from the client requesting it (as it potentially ties
up a thread that could otherwise be used for parallel processing of
other requests), and that the server could reject a repeat command with
the flag if it is already serving a previous open-ended request.

-- 
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] iotests: Add test for failing qemu-img commit

[Max Reitz]

On 2017-11-10 22:21, Max Reitz wrote:
> On 2017-06-16 15:58, Max Reitz wrote:
>> Signed-off-by: Max Reitz 
>> ---
>> In order to pass, this depends on "fix: avoid an infinite loop or a
>> dangling pointer problem in img_commit"
>> (http://lists.nongnu.org/archive/html/qemu-block/2017-06/msg00443.html)
>> and on the "block: Don't compare strings in bdrv_reopen_prepare()"
>> series
>> (http://lists.nongnu.org/archive/html/qemu-block/2017-06/msg00424.html).
>> ---
>>  tests/qemu-iotests/020 | 27 +++
>>  tests/qemu-iotests/020.out | 17 +
>>  2 files changed, 44 insertions(+)
> 
> Finally applied to my block tree:
> 
> https://github.com/XanClic/qemu/commits/block

...and dropped again because I had to drop the series this depends on.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v6 0/6] block: Don't compare strings in bdrv_reopen_prepare()

[Max Reitz]

On 2017-10-04 17:25, Max Reitz wrote:
> bdrv_reopen_prepare() assumes that all BDS options are strings, which is
> not necessarily correct. This series introduces a new qobject_is_equal()
> function which can be used to test whether any options have changed,
> independently of their type.
> 
> 
> v6:
> - Patch 2: Added corresponding Coccinelle rule [Eric]
> - Patch 6:
>   - Added .gitignore entry [Eric]
>   - Removed leading underscore [Markus]
>   - Replaced test_equality() by check_equal()+check_unequal() [Markus]
>   - Put type conversion (or rather non-conversion) tests into an own
> function [Markus]
>   - Fixed a line > 80 characters

Dropping from my queue because of a clang compiler warning in
tests/check-qobject.c.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH for-2.11 4/5] qcow2: Add bounds check to get_refblock_offset()

[Max Reitz]

On 2017-11-14 16:38, Alberto Garcia wrote:
> On Tue 14 Nov 2017 04:27:56 PM CET, Max Reitz wrote:
 +static int64_t get_refblock_offset(BlockDriverState *bs, uint64_t offset)
 +{
 +BDRVQcow2State *s = bs->opaque;
 +uint32_t index = offset_to_reftable_index(s, offset);
 +int64_t covering_refblock_offset = 0;
 +
 +if (index < s->refcount_table_size) {
 +covering_refblock_offset = s->refcount_table[index] & 
 REFT_OFFSET_MASK;
 +}
 +if (!covering_refblock_offset) {
 +qcow2_signal_corruption(bs, true, -1, -1, "Refblock at %#" PRIx64 
 " is "
 +"not covered by the refcount structures",
 +offset);
 +return -EIO;
 +}
 +
 +return covering_refblock_offset;
 +}
>>>
>>> Isn't it simpler to do something like this instead?
>>>
>>>if (index >= s->refcount_table_size) {
>>>qcow2_signal_corruption(...);
>>>return -EIO;
>>>}
>>>return s->refcount_table[index] & REFT_OFFSET_MASK;
>>
>> But that doesn't cover the case were s->refcount_table[index] &
>> REFT_OFFSET_MASK is 0.
> 
> Ah, you're right. That would be detected by the qcow2_cache_get() call
> though, but it's fine to do the check here as well.

After patch 5, yes, but it would lead to a failed assertion.

Before this series, there is no protection in place; the cache will
happily serve you any empty entry (because offset 0 is used for empty
entries) and pretend it's the correct block.

Only when you try to dirty it is when you run into problems (because
then you'll get an assertion failure).

Max

> Reviewed-by: Alberto Garcia 
> 
> Berto
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.11 3/5] block: Guard against NULL bs->drv

[Max Reitz]

On 2017-11-10 22:46, Eric Blake wrote:
> On 11/10/2017 02:31 PM, Max Reitz wrote:
>> We currently do not guard everywhere against a NULL bs->drv where we
>> should be doing so.  Most of the places fixed here just do not care
>> about that case at all.
>>
>> Some care implicitly, e.g. through a prior function call to
>> bdrv_getlength() which would always fail for an ejected BDS.  Add an
>> assert there to make it more obvious.
>>
>> Other places seem to care, but do so insufficiently: Freeing clusters in
>> a qcow2 image is an error-free operation, but it may leave the image in
>> an unusable state anyway.  Giving qcow2_free_clusters() an error code is
>> not really viable, it is much easier to note that bs->drv may be NULL
>> even after a successful driver call.  This concerns bdrv_co_flush(), and
>> the way the check is added to bdrv_co_pdiscard() (in every iteration
>> instead of only once).
>>
>> Finally, some places employ at least an assert(bs->drv); somewhere, that
>> may be reasonable (such as in the reopen code), but in
>> bdrv_has_zero_init(), it is definitely not.  Returning 0 there in case
>> of an ejected BDS saves us much headache instead.
>>
>> Reported-by: R. Nageswara Sastry 
>> Buglink: https://bugs.launchpad.net/qemu/+bug/1728660
>> Signed-off-by: Max Reitz 
>> ---
> 
>> +++ b/block/replication.c
> 
>>  
>> +if (!s->hidden_disk->bs->drv) {
>> +error_setg(errp, "Hidden disk %s is ejected",
>> +   s->hidden_disk->bs->node_name);
>> +return;
>> +}
> 
> How would the hidden disk ever be ejected?  Could this be an assert instead?

Maybe? :-)

Isn't the hidden disk usually a qcow2 file?  As such I guess there can
be corruptions in it that make the qcow2 driver eject it (even though
qemu isn't writing to it).

Max

> But what you have is equally safe, so
> Reviewed-by: Eric Blake 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH for-2.11 5/5] qcow2: Refuse to get unaligned offsets from cache

[Alberto Garcia]

On Tue 14 Nov 2017 04:09:16 PM CET, Max Reitz wrote:
>>> +static inline const char *qcow2_cache_get_name(BDRVQcow2State *s, 
>>> Qcow2Cache *c)
>>> +{
>>> +if (c == s->refcount_block_cache) {
>>> +return "refcount block";
>>> +} else if (c == s->l2_table_cache) {
>>> +return "L2 table";
>>> +} else {
>>> +/* Do not abort, because this is not critical */
>>> +return "unknown";
>>> +}
>>> +}
>> 
>> Why is an unknown cache not critical?
>
> Because this is debugging information.

Ah, right, this is only used for qcow2_signal_corruption().

> I know others disagree with my opinion that I'd rather not abort qemu
> just because someone forgot to add a 'return "foo";' here when adding
> a new cache, but that's my opinion so I wanted to at least be told by
> someone that we should abort here before doing it.

I just checked the code and at least qcow2_cache_entry_flush() assumes
that there can be other caches, so this problem is not new.

Perhaps we should rethink this in the future and add a stronger
assertion somewhere else instead of having dead code, but for now this
is OK I guess.

Reviewed-by: Alberto Garcia 

Berto

Re: [Qemu-block] [PATCH v4 0/4] Don't write headers if BDS is INACTIVE

[Max Reitz]

On 2017-11-07 14:10, Jeff Cody wrote:
> Changes from v3->v4:
> 
> Patch 3: Add migrate_del_blocker and error_free (Thanks Stefan)

Thanks, applied to my block branch:

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

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH for-2.11 4/5] qcow2: Add bounds check to get_refblock_offset()

[Max Reitz]

On 2017-11-14 16:02, Alberto Garcia wrote:
> On Fri 10 Nov 2017 09:31:10 PM CET, Max Reitz wrote:
>> +static int64_t get_refblock_offset(BlockDriverState *bs, uint64_t offset)
>> +{
>> +BDRVQcow2State *s = bs->opaque;
>> +uint32_t index = offset_to_reftable_index(s, offset);
>> +int64_t covering_refblock_offset = 0;
>> +
>> +if (index < s->refcount_table_size) {
>> +covering_refblock_offset = s->refcount_table[index] & 
>> REFT_OFFSET_MASK;
>> +}
>> +if (!covering_refblock_offset) {
>> +qcow2_signal_corruption(bs, true, -1, -1, "Refblock at %#" PRIx64 " 
>> is "
>> +"not covered by the refcount structures",
>> +offset);
>> +return -EIO;
>> +}
>> +
>> +return covering_refblock_offset;
>> +}
> 
> Isn't it simpler to do something like this instead?
> 
>if (index >= s->refcount_table_size) {
>qcow2_signal_corruption(...);
>return -EIO;
>}
>return s->refcount_table[index] & REFT_OFFSET_MASK;

But that doesn't cover the case were s->refcount_table[index] &
REFT_OFFSET_MASK is 0.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH for-2.12 0/4] blockdev: Mark BD-{remove, insert}-medium stable

[Alberto Garcia]

On Fri 10 Nov 2017 11:42:58 PM CET, Max Reitz wrote:
> Berto's "Test I/O limits with removable media" patch proves that
> throttling survives a blockdev-remove-medium/blockdev-insert-medium pair
> now, so let's mark them stable (because that was the reason they were
> considered experimental, see commit
> 6e0abc251dd4f8eba1f53656dfede12e5840e83b for more).
>
> But before we do that, let's use the chance and drop the @device
> parameter.
>
>
> Based-on: 
> ("Fix throttling crashes in BlockBackend with no BlockDriverState",
>  because of the test case added there)
>
>
> Max Reitz (4):
>   iotests: Make BD-{remove,insert}-medium use @id
>   tests/ahci: Switch tray and medium commands to @id
>   blockdev: Drop BD-{remove,insert}-medium's @device
>   blockdev: Mark BD-{remove,insert}-medium stable

This series,

Reviewed-by: Alberto Garcia 

Berto

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.12 2/3] block: Handle null backing link

[Max Reitz]

On 2017-11-14 16:17, Markus Armbruster wrote:
> Max Reitz  writes:
> 
>> Instead of converting all "backing": null instances into "backing": "",
>> handle a null value directly in bdrv_open_inherit().
>>
>> This enables explicitly null backing links for json:{} filenames.
>>
>> Signed-off-by: Max Reitz 
>> ---
>>  block.c|  2 +-
>>  blockdev.c | 14 --
>>  tests/qemu-iotests/089 | 20 
>>  tests/qemu-iotests/089.out |  8 
>>  4 files changed, 29 insertions(+), 15 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 85427df577..bc92ddd5a0 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2558,7 +2558,7 @@ static BlockDriverState *bdrv_open_inherit(const char 
>> *filename,
>>  
>>  /* See cautionary note on accessing @options above */
>>  backing = qdict_get_try_str(options, "backing");
>> -if (backing && *backing == '\0') {
>> +if (qdict_is_qnull(options, "backing") || (backing && *backing == 
>> '\0')) {
>>  flags |= BDRV_O_NO_BACKING;
>>  qdict_del(options, "backing");
>>  }
> 
> Consider a comment pointing out "" is deprecated.

Would not really be necessary after patch 3, but let's see how well
patch 3 is received. :-)

(And in any case, a comment certainly won't hurt.)

Max

> See also my reply to PATCH 1/3.
> 
>> diff --git a/blockdev.c b/blockdev.c
>> index 56a6b24a0b..99ef618b39 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -3885,7 +3885,6 @@ void qmp_blockdev_add(BlockdevOptions *options, Error 
>> **errp)
>>  QObject *obj;
>>  Visitor *v = qobject_output_visitor_new();
>>  QDict *qdict;
>> -const QDictEntry *ent;
>>  Error *local_err = NULL;
>>  
>>  visit_type_BlockdevOptions(v, NULL, , _err);
>> @@ -3899,19 +3898,6 @@ void qmp_blockdev_add(BlockdevOptions *options, Error 
>> **errp)
>>  
>>  qdict_flatten(qdict);
>>  
>> -/*
>> - * Rewrite "backing": null to "backing": ""
>> - * TODO Rewrite "" to null instead, and perhaps not even here
>> - */
>> -for (ent = qdict_first(qdict); ent; ent = qdict_next(qdict, ent)) {
>> -char *dot = strrchr(ent->key, '.');
>> -
>> -if (!strcmp(dot ? dot + 1 : ent->key, "backing")
>> -&& qobject_type(ent->value) == QTYPE_QNULL) {
>> -qdict_put(qdict, ent->key, qstring_new());
>> -}
>> -}
>> -
>>  if (!qdict_get_try_str(qdict, "node-name")) {
>>  error_setg(errp, "'node-name' must be specified for the root node");
>>  goto fail;
>> diff --git a/tests/qemu-iotests/089 b/tests/qemu-iotests/089
>> index 9bfe2307b3..832eac1248 100755
>> --- a/tests/qemu-iotests/089
>> +++ b/tests/qemu-iotests/089
>> @@ -82,6 +82,26 @@ $QEMU_IO_PROG --cache $CACHEMODE \
>>  $QEMU_IO -c 'read -P 42 0 512' "$TEST_IMG" | _filter_qemu_io
>>  
>>  
>> +echo
>> +echo "=== Testing correct handling of 'backing':null ==="
>> +echo
>> +
>> +_make_test_img -b "$TEST_IMG.base" $IMG_SIZE
>> +
>> +# This should read 42
>> +$QEMU_IO -c 'read -P 42 0 512' "$TEST_IMG" | _filter_qemu_io
>> +
>> +# This should read 0
>> +$QEMU_IO -c 'read -P 0 0 512' "json:{\
>> +'driver': '$IMGFMT',
>> +'file': {
>> +'driver': 'file',
>> +'filename': '$TEST_IMG'
>> +},
>> +'backing': null
>> +}" | _filter_qemu_io
>> +
>> +
>>  # Taken from test 071
>>  echo
>>  echo "=== Testing blkdebug ==="
>> diff --git a/tests/qemu-iotests/089.out b/tests/qemu-iotests/089.out
>> index 18f5fdda7a..607a9c6d9c 100644
>> --- a/tests/qemu-iotests/089.out
>> +++ b/tests/qemu-iotests/089.out
>> @@ -19,6 +19,14 @@ Pattern verification failed at offset 0, 512 bytes
>>  read 512/512 bytes at offset 0
>>  512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>  
>> +=== Testing correct handling of 'backing':null ===
>> +
>> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
>> backing_file=TEST_DIR/t.IMGFMT.base
>> +read 512/512 bytes at offset 0
>> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +read 512/512 bytes at offset 0
>> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +
>>  === Testing blkdebug ===
>>  
>>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> 
> Reviewed-by: Markus Armbruster 
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.12 3/3] block: Deprecate "backing": ""

[Markus Armbruster]

Max Reitz  writes:

> We have a clear replacement, so let's deprecate it.
>
> Signed-off-by: Max Reitz 
> ---
>  qapi/block-core.json | 4 ++--
>  block.c  | 4 
>  qemu-doc.texi| 7 +++
>  qemu-options.hx  | 4 ++--
>  4 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 76bf50f813..dfe4d3650c 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1090,7 +1090,7 @@
>  # @overlay: reference to the existing block device that will become
>  #   the overlay of @node, as part of creating the snapshot.
>  #   It must not have a current backing file (this can be
> -#   achieved by passing "backing": "" to blockdev-add).
> +#   achieved by passing "backing": null to blockdev-add).
>  #
>  # Since: 2.5
>  ##
> @@ -1238,7 +1238,7 @@
>  # "node-name": "node1534",
>  # "file": { "driver": "file",
>  #   "filename": "hd1.qcow2" },
> -# "backing": "" } }
> +# "backing": null } }
>  #
>  # <- { "return": {} }
>  #
> diff --git a/block.c b/block.c
> index bc92ddd5a0..463c4de25b 100644
> --- a/block.c
> +++ b/block.c
> @@ -2559,6 +2559,10 @@ static BlockDriverState *bdrv_open_inherit(const char 
> *filename,
>  /* See cautionary note on accessing @options above */
>  backing = qdict_get_try_str(options, "backing");
>  if (qdict_is_qnull(options, "backing") || (backing && *backing == '\0')) 
> {
> +if (backing) {
> +warn_report("Use of \"backing\": \"\" is deprecated; "
> +"use \"backing\": null instead");
> +}
>  flags |= BDRV_O_NO_BACKING;
>  qdict_del(options, "backing");
>  }
> diff --git a/qemu-doc.texi b/qemu-doc.texi
> index 8c10956a66..8f57d9ad21 100644
> --- a/qemu-doc.texi
> +++ b/qemu-doc.texi
> @@ -2537,6 +2537,13 @@ or ``ivshmem-doorbell`` device types.
>  The ``spapr-pci-vfio-host-bridge'' device type is replaced by
>  the ``spapr-pci-host-bridge'' device type.
>  
> +@section Block device options
> +
> +@subsection "backing": "" (since 2.12.0)
> +
> +In order to prevent QEMU from automatically opening an image's backing
> +chain, use ``"backing": null'' instead.
> +
>  @node License
>  @appendix License
>  
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 3728e9b4dd..0ee1a04d00 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -731,8 +731,8 @@ Reference to or definition of the data source block 
> driver node
>  
>  @item backing
>  Reference to or definition of the backing file block device (default is taken
> -from the image file). It is allowed to pass an empty string here in order to
> -disable the default backing file.
> +from the image file). It is allowed to pass @code{null} here in order to 
> disable
> +the default backing file.
>  
>  @item lazy-refcounts
>  Whether to enable the lazy refcounts feature (on/off; default is taken from 
> the

Missed in commit c42e8742f, I guess.

Reviewed-by: Markus Armbruster 

Re: [Qemu-block] [PATCH v4 1/4] block/vhdx.c: Don't blindly update the header

[Max Reitz]

On 2017-11-07 14:10, Jeff Cody wrote:
> The VHDX specification requires that before user data modification of
> the vhdx image, the VHDX header file and data GUIDs need to be updated.
> In vhdx_open(), if the image is set to RDWR, we go ahead and update the
> header.
> 
> However, just because the image is set to RDWR does not mean we can go
> ahead and write at this point - specifically, if the QEMU run state is
> INMIGRATE, the underlying file BS may be set to inactive via the BDS
> open flag of BDRV_O_INACTIVE.  Attempting to write under this condition
> will cause an assert in bdrv_co_pwritev().
> 
> We can alternatively latch the first time the image is written.  And lo
> and behold, we do just that, via vhdx_user_visible_write() in
> vhdx_co_writev().  This means the call to vhdx_update_headers() in
> vhdx_open() is likely just vestigial, and can be removed.
> 
> Reported-by: Alexey Kardashevskiy 
> Tested-by: Alexey Kardashevskiy 
> Signed-off-by: Jeff Cody 
> ---
>  block/vhdx.c | 7 ---
>  1 file changed, 7 deletions(-)
> 
> diff --git a/block/vhdx.c b/block/vhdx.c
> index 7ae4589..9956933 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -1008,13 +1008,6 @@ static int vhdx_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  goto fail;
>  }
>  
> -if (flags & BDRV_O_RDWR) {
> -ret = vhdx_update_headers(bs, s, false, NULL);

And this doesn't even update the data GUID...

Max

> -if (ret < 0) {
> -goto fail;
> -}
> -}
> -
>  /* TODO: differencing files */
>  
>  return 0;
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.12 2/3] block: Handle null backing link

[Markus Armbruster]

Max Reitz  writes:

> Instead of converting all "backing": null instances into "backing": "",
> handle a null value directly in bdrv_open_inherit().
>
> This enables explicitly null backing links for json:{} filenames.
>
> Signed-off-by: Max Reitz 
> ---
>  block.c|  2 +-
>  blockdev.c | 14 --
>  tests/qemu-iotests/089 | 20 
>  tests/qemu-iotests/089.out |  8 
>  4 files changed, 29 insertions(+), 15 deletions(-)
>
> diff --git a/block.c b/block.c
> index 85427df577..bc92ddd5a0 100644
> --- a/block.c
> +++ b/block.c
> @@ -2558,7 +2558,7 @@ static BlockDriverState *bdrv_open_inherit(const char 
> *filename,
>  
>  /* See cautionary note on accessing @options above */
>  backing = qdict_get_try_str(options, "backing");
> -if (backing && *backing == '\0') {
> +if (qdict_is_qnull(options, "backing") || (backing && *backing == '\0')) 
> {
>  flags |= BDRV_O_NO_BACKING;
>  qdict_del(options, "backing");
>  }

Consider a comment pointing out "" is deprecated.

See also my reply to PATCH 1/3.

> diff --git a/blockdev.c b/blockdev.c
> index 56a6b24a0b..99ef618b39 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3885,7 +3885,6 @@ void qmp_blockdev_add(BlockdevOptions *options, Error 
> **errp)
>  QObject *obj;
>  Visitor *v = qobject_output_visitor_new();
>  QDict *qdict;
> -const QDictEntry *ent;
>  Error *local_err = NULL;
>  
>  visit_type_BlockdevOptions(v, NULL, , _err);
> @@ -3899,19 +3898,6 @@ void qmp_blockdev_add(BlockdevOptions *options, Error 
> **errp)
>  
>  qdict_flatten(qdict);
>  
> -/*
> - * Rewrite "backing": null to "backing": ""
> - * TODO Rewrite "" to null instead, and perhaps not even here
> - */
> -for (ent = qdict_first(qdict); ent; ent = qdict_next(qdict, ent)) {
> -char *dot = strrchr(ent->key, '.');
> -
> -if (!strcmp(dot ? dot + 1 : ent->key, "backing")
> -&& qobject_type(ent->value) == QTYPE_QNULL) {
> -qdict_put(qdict, ent->key, qstring_new());
> -}
> -}
> -
>  if (!qdict_get_try_str(qdict, "node-name")) {
>  error_setg(errp, "'node-name' must be specified for the root node");
>  goto fail;
> diff --git a/tests/qemu-iotests/089 b/tests/qemu-iotests/089
> index 9bfe2307b3..832eac1248 100755
> --- a/tests/qemu-iotests/089
> +++ b/tests/qemu-iotests/089
> @@ -82,6 +82,26 @@ $QEMU_IO_PROG --cache $CACHEMODE \
>  $QEMU_IO -c 'read -P 42 0 512' "$TEST_IMG" | _filter_qemu_io
>  
>  
> +echo
> +echo "=== Testing correct handling of 'backing':null ==="
> +echo
> +
> +_make_test_img -b "$TEST_IMG.base" $IMG_SIZE
> +
> +# This should read 42
> +$QEMU_IO -c 'read -P 42 0 512' "$TEST_IMG" | _filter_qemu_io
> +
> +# This should read 0
> +$QEMU_IO -c 'read -P 0 0 512' "json:{\
> +'driver': '$IMGFMT',
> +'file': {
> +'driver': 'file',
> +'filename': '$TEST_IMG'
> +},
> +'backing': null
> +}" | _filter_qemu_io
> +
> +
>  # Taken from test 071
>  echo
>  echo "=== Testing blkdebug ==="
> diff --git a/tests/qemu-iotests/089.out b/tests/qemu-iotests/089.out
> index 18f5fdda7a..607a9c6d9c 100644
> --- a/tests/qemu-iotests/089.out
> +++ b/tests/qemu-iotests/089.out
> @@ -19,6 +19,14 @@ Pattern verification failed at offset 0, 512 bytes
>  read 512/512 bytes at offset 0
>  512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  
> +=== Testing correct handling of 'backing':null ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
> backing_file=TEST_DIR/t.IMGFMT.base
> +read 512/512 bytes at offset 0
> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +read 512/512 bytes at offset 0
> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
>  === Testing blkdebug ===
>  
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864

Reviewed-by: Markus Armbruster 

Re: [Qemu-block] [PATCH for-2.11 5/5] qcow2: Refuse to get unaligned offsets from cache

[Max Reitz]

On 2017-11-14 16:06, Alberto Garcia wrote:
> On Fri 10 Nov 2017 09:31:11 PM CET, Max Reitz wrote:
>> +static inline const char *qcow2_cache_get_name(BDRVQcow2State *s, 
>> Qcow2Cache *c)
>> +{
>> +if (c == s->refcount_block_cache) {
>> +return "refcount block";
>> +} else if (c == s->l2_table_cache) {
>> +return "L2 table";
>> +} else {
>> +/* Do not abort, because this is not critical */
>> +return "unknown";
>> +}
>> +}
> 
> Why is an unknown cache not critical?

Because this is debugging information.  I know others disagree with my
opinion that I'd rather not abort qemu just because someone forgot to
add a 'return "foo";' here when adding a new cache, but that's my
opinion so I wanted to at least be told by someone that we should abort
here before doing it.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH] iotests: 077: Filter out 'resume' lines

[Max Reitz]

On 2017-11-13 16:00, Fam Zheng wrote:
> In the "Overlapping multiple requests" cases, the 3rd reqs (the break
> point B) doesn't wait for the 2nd, and once resumed the I/O will just
> continue.  This is because the 2nd is already waiting for the 1st, and
> in wait_serialising_requests() there is:
> 
> /* If the request is already (indirectly) waiting for us, or
>  * will wait for us as soon as it wakes up, then just go on
>  * (instead of producing a deadlock in the former case). */
> if (!req->waiting_for) {
> /* actually break */
> ...
> }
> 
> Consequently, the following "sleep 100; resume A" command races with the
> completion of that request, and sometimes results in an unexpected
> order of output:
> 
>> @@ -56,9 +56,9 @@
>>  wrote XXX/XXX bytes at offset XXX
>>  XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>  blkdebug: Resuming request 'B'
>> +blkdebug: Resuming request 'A'
>>  wrote XXX/XXX bytes at offset XXX
>>  XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> -blkdebug: Resuming request 'A'
>>  wrote XXX/XXX bytes at offset XXX
>>  XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>  wrote XXX/XXX bytes at offset XXX
> 
> Filter out the "Resuming request" lines to make the output
> deterministic.
> 
> Reported-by: Patchew 
> Signed-off-by: Fam Zheng 
> ---
>  tests/qemu-iotests/077 |  3 ++-
>  tests/qemu-iotests/077.out | 48 
> --
>  2 files changed, 18 insertions(+), 33 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 for-2.11 5/5] qcow2: Refuse to get unaligned offsets from cache

[Alberto Garcia]

On Fri 10 Nov 2017 09:31:11 PM CET, Max Reitz wrote:
> +static inline const char *qcow2_cache_get_name(BDRVQcow2State *s, Qcow2Cache 
> *c)
> +{
> +if (c == s->refcount_block_cache) {
> +return "refcount block";
> +} else if (c == s->l2_table_cache) {
> +return "L2 table";
> +} else {
> +/* Do not abort, because this is not critical */
> +return "unknown";
> +}
> +}

Why is an unknown cache not critical?

Berto

Re: [Qemu-block] [PATCH for-2.11 4/5] qcow2: Add bounds check to get_refblock_offset()

[Alberto Garcia]

On Fri 10 Nov 2017 09:31:10 PM CET, Max Reitz wrote:
> +static int64_t get_refblock_offset(BlockDriverState *bs, uint64_t offset)
> +{
> +BDRVQcow2State *s = bs->opaque;
> +uint32_t index = offset_to_reftable_index(s, offset);
> +int64_t covering_refblock_offset = 0;
> +
> +if (index < s->refcount_table_size) {
> +covering_refblock_offset = s->refcount_table[index] & 
> REFT_OFFSET_MASK;
> +}
> +if (!covering_refblock_offset) {
> +qcow2_signal_corruption(bs, true, -1, -1, "Refblock at %#" PRIx64 " 
> is "
> +"not covered by the refcount structures",
> +offset);
> +return -EIO;
> +}
> +
> +return covering_refblock_offset;
> +}

Isn't it simpler to do something like this instead?

   if (index >= s->refcount_table_size) {
   qcow2_signal_corruption(...);
   return -EIO;
   }
   return s->refcount_table[index] & REFT_OFFSET_MASK;

Berto

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.12 1/3] qapi: Add qdict_is_null()

[Max Reitz]

On 2017-11-14 15:57, Markus Armbruster wrote:
> Max Reitz  writes:
> 
>> Signed-off-by: Max Reitz 
>> ---
>>  include/qapi/qmp/qdict.h |  1 +
>>  qobject/qdict.c  | 10 ++
>>  2 files changed, 11 insertions(+)
>>
>> diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
>> index fc218e7be6..c65ebfc748 100644
>> --- a/include/qapi/qmp/qdict.h
>> +++ b/include/qapi/qmp/qdict.h
>> @@ -76,6 +76,7 @@ int64_t qdict_get_try_int(const QDict *qdict, const char 
>> *key,
>>int64_t def_value);
>>  bool qdict_get_try_bool(const QDict *qdict, const char *key, bool 
>> def_value);
>>  const char *qdict_get_try_str(const QDict *qdict, const char *key);
>> +bool qdict_is_qnull(const QDict *qdict, const char *key);
>>  
>>  void qdict_copy_default(QDict *dst, QDict *src, const char *key);
>>  void qdict_set_default_str(QDict *dst, const char *key, const char *val);
>> diff --git a/qobject/qdict.c b/qobject/qdict.c
>> index e8f15f1132..a032ea629a 100644
>> --- a/qobject/qdict.c
>> +++ b/qobject/qdict.c
>> @@ -294,6 +294,16 @@ const char *qdict_get_try_str(const QDict *qdict, const 
>> char *key)
>>  }
>>  
>>  /**
>> + * qdict_is_qnull(): Return true if the value for 'key' is QNull
>> + */
>> +bool qdict_is_qnull(const QDict *qdict, const char *key)
>> +{
>> +QObject *value = qdict_get(qdict, key);
>> +
>> +return value && value->type == QTYPE_QNULL;
>> +}
>> +
>> +/**
>>   * qdict_iter(): Iterate over all the dictionary's stored values.
>>   *
>>   * This function allows the user to provide an iterator, which will be
> 
> As far as I can tell, the new helper function is going to be used just
> once, by bdrv_open_inherit() in PATCH 2:
> 
> qdict_is_qnull(options, "backing")
> 
> I dislike abstracting from just one concrete instance.
> 
> Perhaps a more general helper could be more generally useful.  Something
> like:
> 
> qobject_is(qdict_get(options, "backing", QTYPE_QNULL))
> 
> There are numerous instances of
> 
> !obj || qobject_type(obj) == T
> 
> in the tree, which could then be replaced by
> 
> qobject_is(obj, T)
> 
> An alternative helper: macro qobject_dynamic_cast(obj, T) that returns
> (T *)obj if obj is a T, else null.  Leads to something like
> 
> qobject_dynamic_cast(qdict_get(options, "backing", QNull))

If you think that's good, then that's good -- you know the QAPI code
better then me, after all.

To explain myself: I thought it would be the natural extension of the
qdict_get_try_*() functions for the QNull type.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH] block/snapshot: dirty all dirty bitmaps on snapshot-switch

[Max Reitz]

On 2017-10-23 11:29, Vladimir Sementsov-Ogievskiy wrote:
> Snapshot-switch actually changes active state of disk so it should
> reflect on dirty bitmaps. Otherwise next incremental backup using
> these bitmaps will be invalid.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/snapshot.c | 14 ++
>  1 file changed, 14 insertions(+)

Thanks, applied to my block tree:

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

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.12 1/3] qapi: Add qdict_is_null()

[Markus Armbruster]

Max Reitz  writes:

> Signed-off-by: Max Reitz 
> ---
>  include/qapi/qmp/qdict.h |  1 +
>  qobject/qdict.c  | 10 ++
>  2 files changed, 11 insertions(+)
>
> diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
> index fc218e7be6..c65ebfc748 100644
> --- a/include/qapi/qmp/qdict.h
> +++ b/include/qapi/qmp/qdict.h
> @@ -76,6 +76,7 @@ int64_t qdict_get_try_int(const QDict *qdict, const char 
> *key,
>int64_t def_value);
>  bool qdict_get_try_bool(const QDict *qdict, const char *key, bool def_value);
>  const char *qdict_get_try_str(const QDict *qdict, const char *key);
> +bool qdict_is_qnull(const QDict *qdict, const char *key);
>  
>  void qdict_copy_default(QDict *dst, QDict *src, const char *key);
>  void qdict_set_default_str(QDict *dst, const char *key, const char *val);
> diff --git a/qobject/qdict.c b/qobject/qdict.c
> index e8f15f1132..a032ea629a 100644
> --- a/qobject/qdict.c
> +++ b/qobject/qdict.c
> @@ -294,6 +294,16 @@ const char *qdict_get_try_str(const QDict *qdict, const 
> char *key)
>  }
>  
>  /**
> + * qdict_is_qnull(): Return true if the value for 'key' is QNull
> + */
> +bool qdict_is_qnull(const QDict *qdict, const char *key)
> +{
> +QObject *value = qdict_get(qdict, key);
> +
> +return value && value->type == QTYPE_QNULL;
> +}
> +
> +/**
>   * qdict_iter(): Iterate over all the dictionary's stored values.
>   *
>   * This function allows the user to provide an iterator, which will be

As far as I can tell, the new helper function is going to be used just
once, by bdrv_open_inherit() in PATCH 2:

qdict_is_qnull(options, "backing")

I dislike abstracting from just one concrete instance.

Perhaps a more general helper could be more generally useful.  Something
like:

qobject_is(qdict_get(options, "backing", QTYPE_QNULL))

There are numerous instances of

!obj || qobject_type(obj) == T

in the tree, which could then be replaced by

qobject_is(obj, T)

An alternative helper: macro qobject_dynamic_cast(obj, T) that returns
(T *)obj if obj is a T, else null.  Leads to something like

qobject_dynamic_cast(qdict_get(options, "backing", QNull))

Re: [Qemu-block] [PATCH for-2.11 2/5] qcow2: Unaligned zero cluster in handle_alloc()

[Alberto Garcia]

On Fri 10 Nov 2017 09:31:08 PM CET, Max Reitz wrote:
> We should check whether the cluster offset we are about to use is
> actually valid; that is, whether it is aligned to cluster boundaries.
>
> Reported-by: R. Nageswara Sastry 
> Buglink: https://bugs.launchpad.net/qemu/+bug/1728643
> Buglink: https://bugs.launchpad.net/qemu/+bug/1728657
> Signed-off-by: Max Reitz 

Reviewed-by: Alberto Garcia 

Berto

Re: [Qemu-block] [PATCH for 2.9] qcow2: avoid nb_sectors int overflow at zero/discard

[Max Reitz]

On 2017-10-24 10:56, Anton Nefedov wrote:
> during discard, signed integer overflow leads to end_offset possibly
> be less than offset,
> that in turn makes nb_clusters (to discard) much bigger than expected,
> leading to a possible data loss, operation taking very long and
> image growing large as discard will allocate L2 tables for all that
> huge amount of clusters
> 
> One possibility where nb_sectors is large enough (>=2^21) is vm_state
> discard at snapshot creation.
> 
> Fixed on master in 2.10 with:
> 
> d2cb36af2b0040d421b347e6e4e803e07220f78d
> Author: Eric Blake 
> Date:   Sat May 6 19:05:52 2017 -0500
> 
> qcow2: Discard/zero clusters by byte count
> 
> Signed-off-by: Anton Nefedov 
> Signed-off-by: Denis V. Lunev 
> ---
>  block/qcow2.h | 6 +++---
>  block/qcow2-cluster.c | 6 +++---
>  2 files changed, 6 insertions(+), 6 deletions(-)

FWIW:
Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH for-2.12 1/3] qapi: Add qdict_is_null()

[Max Reitz]

On 2017-11-14 15:07, Alberto Garcia wrote:
> On Fri 10 Nov 2017 11:13:27 PM CET, Max Reitz wrote:
>> Signed-off-by: Max Reitz 
>> ---
>>  include/qapi/qmp/qdict.h |  1 +
>>  qobject/qdict.c  | 10 ++
>>  2 files changed, 11 insertions(+)
>>
>> diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
>> index fc218e7be6..c65ebfc748 100644
>> --- a/include/qapi/qmp/qdict.h
>> +++ b/include/qapi/qmp/qdict.h
>> @@ -76,6 +76,7 @@ int64_t qdict_get_try_int(const QDict *qdict, const char 
>> *key,
>>int64_t def_value);
>>  bool qdict_get_try_bool(const QDict *qdict, const char *key, bool 
>> def_value);
>>  const char *qdict_get_try_str(const QDict *qdict, const char *key);
>> +bool qdict_is_qnull(const QDict *qdict, const char *key);
> 
> I found this name a bit confusing, how about something like
> qdict_entry_is_qnull() ?

Yes, that sounds better.  Thanks!

Max

> I'm fine if you want to keep it like this though,
> 
> Reviewed-by: Alberto Garcia 
> 
> Berto
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH for-2.12 1/3] qapi: Add qdict_is_null()

[Alberto Garcia]

On Fri 10 Nov 2017 11:13:27 PM CET, Max Reitz wrote:
> Signed-off-by: Max Reitz 
> ---
>  include/qapi/qmp/qdict.h |  1 +
>  qobject/qdict.c  | 10 ++
>  2 files changed, 11 insertions(+)
>
> diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
> index fc218e7be6..c65ebfc748 100644
> --- a/include/qapi/qmp/qdict.h
> +++ b/include/qapi/qmp/qdict.h
> @@ -76,6 +76,7 @@ int64_t qdict_get_try_int(const QDict *qdict, const char 
> *key,
>int64_t def_value);
>  bool qdict_get_try_bool(const QDict *qdict, const char *key, bool def_value);
>  const char *qdict_get_try_str(const QDict *qdict, const char *key);
> +bool qdict_is_qnull(const QDict *qdict, const char *key);

I found this name a bit confusing, how about something like
qdict_entry_is_qnull() ?

I'm fine if you want to keep it like this though,

Reviewed-by: Alberto Garcia 

Berto

Re: [Qemu-block] [PATCH for-2.12 2/3] block: Handle null backing link

[Alberto Garcia]

On Fri 10 Nov 2017 11:13:28 PM CET, Max Reitz wrote:
> Instead of converting all "backing": null instances into "backing": "",
> handle a null value directly in bdrv_open_inherit().
>
> This enables explicitly null backing links for json:{} filenames.
>
> Signed-off-by: Max Reitz 

Reviewed-by: Alberto Garcia 

Berto

Re: [Qemu-block] [Qemu-devel] [PATCH] qcow2: fix image corruption after committing qcow2 image into base

[Max Reitz]

On 2017-11-10 18:22, Daniel P. Berrange wrote:
> On Fri, Nov 10, 2017 at 10:34:59AM -0600, Eric Blake wrote:
>> On 11/03/2017 09:41 AM, Daniel P. Berrange wrote:
>>> After committing the qcow2 image contents into the base image, qemu-img
>>> will call bdrv_make_empty to drop the payload in the layered image.
>>>
>>> When this is done for qcow2 images, it blows away the LUKS encryption
>>> header, making the resulting image unusable. There are two codepaths
>>> for emptying a qcow2 image, and the second (slower) codepaths leaves
>>> the LUKS header intact, so force use of that codepath.
>>>
>>> Signed-off-by: Daniel P. Berrange 
>>> ---
>>>
>>> NB, ideally we would fix the faster codepath in make_completely_empty, but
>>> having looked at the code, I've really no idea how to even start on fixing 
>>> that
>>> to not kill the LUKS header clusters.
>>
>> Hmm - I wonder if persistent bitmaps are also corrupted in the fast path.
> 
> I also wonder if there's anything better we can do to make us safer by
> default, so we default to the slow & safe path, unless we can provide
> we *only* have the subset of features that are safe for the fast path ?

I have wondered the same but I can't think of any.  The only thing that
comes close would be to check for which header extensions there are; but
at the same time, we could just add a comment to qcow2_read_extensions()
("If you add a new feature to qcow2, note that you may want to adjust
the qcow2_make_empty() fastpath conditions").

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] using "qemu-img convert -O qcow2" to convert qcow v1 to v2 creates a qcow v3 file?

[Max Reitz]

On 2017-11-13 19:08, Eric Blake wrote:
> On 11/13/2017 11:58 AM, Eric Blake wrote:
> 
 qemu-system-aarch64: -drive if=none,file=hda.qcow2,format=qcow,id=hd:
 Unsupported qcow version 3
>>>
>>> ah, this means it wants "format=qcow2".
>>
>> Oh, I should have read this followup before writing my other reply.
>>
>>>
>>> This is pretty confusing, especially the error message, the
>>> output of "file", and the fact that "format=qcow" can't just
>>> DTRT if it gets a qcow version 3 (2?), since it can clearly
>>> identify what it's got.
>>
>> Indeed, making the qcow driver smart enough to reopen with the qcow2
>> driver (for both v2 and v3 images) might be an interesting ease-of-use hack.
> 
> And having the qcow2 driverautomatically be able to open a qcow v1 image
> read-only might also be nice - although v1 is lacking so many features
> that we'd be insane to let a read-write request of the qcow2 driver
> downgrade to qcow.

Both seem to be hacks to me, though, which needlessly complicate everything.

Just issuing a deprecation warning when creating qcow v1 images and
improving the warning when opening qcow2 (v2/v3) images with the qcow
driver should be enough.

> Another observation: is there any official documentation for the qcow
> (v1) format?  I know it's an outdated format that we no longer recommend
> for new image creation, but it's sad when I have to resort to a google
> search to find old posts like this:
> 
> https://people.gnome.org/~markmc/qcow-image-format-version-1.html
> 
> rather than being able to see the documentation of v1 alongside v2 in
> the qemu.git repository.

Well, do you want to document it?  I'd rather deprecate it altogether.

Max

> At any rate, since qcow and qcow2 share the same 4-byte magic number and
> version field, it explains why both drivers are able to identify (but
> fail to open) the files of the other version number.



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH 5/5] iotest 030: add compressed block-stream test

[Anton Nefedov]

Signed-off-by: Anton Nefedov 
---
 tests/qemu-iotests/030 | 69 +-
 tests/qemu-iotests/030.out |  4 +--
 2 files changed, 70 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 1883894..52cbe5d 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -21,7 +21,7 @@
 import time
 import os
 import iotests
-from iotests import qemu_img, qemu_io
+from iotests import qemu_img, qemu_img_pipe, qemu_io
 
 backing_img = os.path.join(iotests.test_dir, 'backing.img')
 mid_img = os.path.join(iotests.test_dir, 'mid.img')
@@ -800,5 +800,72 @@ class TestSetSpeed(iotests.QMPTestCase):
 
 self.cancel_and_wait()
 
+class TestCompressed(iotests.QMPTestCase):
+supported_fmts = ['qcow2']
+cluster_size = 64 * 1024;
+image_len = 1 * 1024 * 1024;
+
+def setUp(self):
+qemu_img('create', backing_img, str(TestCompressed.image_len))
+qemu_io('-f', 'raw', '-c', 'write -P 1 0 ' + 
str(TestCompressed.image_len), backing_img)
+qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % 
backing_img, test_img)
+
+# write '4' in every 4th cluster
+step=4
+for i in range(TestCompressed.image_len / TestCompressed.cluster_size 
/ step + 1):
+qemu_io('-c', 'write -P %d %d %d' %
+(step, step * i * TestCompressed.cluster_size,
+ TestCompressed.cluster_size),
+test_img)
+
+self.vm = iotests.VM().add_drive(test_img)
+self.vm.launch()
+
+def tearDown(self):
+os.remove(test_img)
+os.remove(backing_img)
+
+def _pattern(self, x, divs):
+return divs[-1] if not x%divs[-1] else self._pattern(x, divs[:-1])
+
+def test_compressed(self):
+self.assert_no_active_block_jobs()
+
+result = self.vm.qmp('block-stream', device='drive0', compress=True)
+if iotests.imgfmt not in TestCompressed.supported_fmts:
+self.assert_qmp(
+result, 'error/desc',
+'Compression is not supported for this drive drive0')
+return
+self.assert_qmp(result, 'return', {})
+
+# write '2' in every 2nd cluster
+step = 2
+for i in range(TestCompressed.image_len / TestCompressed.cluster_size 
/ step + 1):
+result = self.vm.qmp('human-monitor-command',
+ command_line=
+ 'qemu-io drive0 \"write -P %d %d %d\"' %
+ (step, step * i * TestCompressed.cluster_size,
+  TestCompressed.cluster_size))
+self.assert_qmp(result, 'return', "")
+
+self.wait_until_completed()
+self.assert_no_active_block_jobs()
+
+self.vm.shutdown()
+
+for i in range(TestCompressed.image_len / TestCompressed.cluster_size):
+outp = qemu_io('-c', 'read -P %d %d %d' %
+   (self._pattern(i, [1,4,2]),
+i * TestCompressed.cluster_size,
+TestCompressed.cluster_size),
+   test_img)
+self.assertTrue(not 'fail' in outp)
+self.assertTrue('read' in outp and 'at offset' in outp)
+
+self.assertTrue(
+"File contains external, encrypted or compressed clusters."
+in qemu_img_pipe('map', test_img))
+
 if __name__ == '__main__':
 iotests.main(supported_fmts=['qcow2', 'qed'])
diff --git a/tests/qemu-iotests/030.out b/tests/qemu-iotests/030.out
index 391c857..42314e9 100644
--- a/tests/qemu-iotests/030.out
+++ b/tests/qemu-iotests/030.out
@@ -1,5 +1,5 @@
-...
+
 --
-Ran 23 tests
+Ran 24 tests
 
 OK
-- 
2.7.4

[Qemu-block] [PATCH 3/5] block: support compressed write for copy-on-read

[Anton Nefedov]

Signed-off-by: Anton Nefedov 
---
 block/io.c | 30 --
 block/trace-events |  2 +-
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/block/io.c b/block/io.c
index 3d5ef2c..93c6b24 100644
--- a/block/io.c
+++ b/block/io.c
@@ -953,7 +953,7 @@ bdrv_driver_pwritev_compressed(BlockDriverState *bs, 
uint64_t offset,
 }
 
 static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
-int64_t offset, unsigned int bytes, QEMUIOVector *qiov)
+int64_t offset, unsigned int bytes, QEMUIOVector *qiov, int flags)
 {
 BlockDriverState *bs = child->bs;
 
@@ -988,12 +988,13 @@ static int coroutine_fn 
bdrv_co_do_copy_on_readv(BdrvChild *child,
  * allocating cluster in the image file.  Note that this value may exceed
  * BDRV_REQUEST_MAX_BYTES (even when the original read did not), which
  * is one reason we loop rather than doing it all at once.
+ * Also this is crucial for compressed copy-on-read.
  */
 bdrv_round_to_clusters(bs, offset, bytes, _offset, _bytes);
 skip_bytes = offset - cluster_offset;
 
 trace_bdrv_co_do_copy_on_readv(bs, offset, bytes,
-   cluster_offset, cluster_bytes);
+   cluster_offset, cluster_bytes, flags);
 
 bounce_buffer = qemu_try_blockalign(bs,
 MIN(MIN(max_transfer, cluster_bytes),
@@ -1041,8 +1042,13 @@ static int coroutine_fn 
bdrv_co_do_copy_on_readv(BdrvChild *child,
 /* This does not change the data on the disk, it is not
  * necessary to flush even in cache=writethrough mode.
  */
-ret = bdrv_driver_pwritev(bs, cluster_offset, pnum,
-  _qiov, 0);
+if (flags & BDRV_REQ_WRITE_COMPRESSED) {
+ret = bdrv_driver_pwritev_compressed(bs, cluster_offset,
+ pnum, _qiov);
+} else {
+ret = bdrv_driver_pwritev(bs, cluster_offset, pnum,
+  _qiov, 0);
+}
 }
 
 if (ret < 0) {
@@ -1107,7 +1113,12 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild 
*child,
  * potential fallback support, if we ever implement any read flags
  * to pass through to drivers.  For now, there aren't any
  * passthrough flags.  */
-assert(!(flags & ~(BDRV_REQ_NO_SERIALISING | BDRV_REQ_COPY_ON_READ)));
+assert(!(flags & ~(BDRV_REQ_NO_SERIALISING | BDRV_REQ_COPY_ON_READ |
+   BDRV_REQ_WRITE_COMPRESSED)));
+
+/* write compressed only makes sense with copy on read */
+assert(!(flags & BDRV_REQ_WRITE_COMPRESSED) ||
+   (flags & BDRV_REQ_COPY_ON_READ));
 
 /* Handle Copy on Read and associated serialisation */
 if (flags & BDRV_REQ_COPY_ON_READ) {
@@ -1132,7 +1143,7 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild 
*child,
 }
 
 if (!ret || pnum != bytes) {
-ret = bdrv_co_do_copy_on_readv(child, offset, bytes, qiov);
+ret = bdrv_co_do_copy_on_readv(child, offset, bytes, qiov, flags);
 goto out;
 }
 }
@@ -1209,6 +1220,13 @@ int coroutine_fn bdrv_co_preadv(BdrvChild *child,
 return ret;
 }
 
+/* write compressed only makes sense with copy on read */
+if ((flags & BDRV_REQ_WRITE_COMPRESSED) &&
+!(flags & BDRV_REQ_COPY_ON_READ))
+{
+return -EINVAL;
+}
+
 bdrv_inc_in_flight(bs);
 
 /* Don't do copy-on-read if we read data before write operation */
diff --git a/block/trace-events b/block/trace-events
index 11c8d5f..12fe188 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -12,7 +12,7 @@ blk_co_pwritev(void *blk, void *bs, int64_t offset, unsigned 
int bytes, int flag
 bdrv_co_preadv(void *bs, int64_t offset, int64_t nbytes, unsigned int flags) 
"bs %p offset %"PRId64" nbytes %"PRId64" flags 0x%x"
 bdrv_co_pwritev(void *bs, int64_t offset, int64_t nbytes, unsigned int flags) 
"bs %p offset %"PRId64" nbytes %"PRId64" flags 0x%x"
 bdrv_co_pwrite_zeroes(void *bs, int64_t offset, int count, int flags) "bs %p 
offset %"PRId64" count %d flags 0x%x"
-bdrv_co_do_copy_on_readv(void *bs, int64_t offset, unsigned int bytes, int64_t 
cluster_offset, int64_t cluster_bytes) "bs %p offset %"PRId64" bytes %u 
cluster_offset %"PRId64" cluster_bytes %"PRId64
+bdrv_co_do_copy_on_readv(void *bs, int64_t offset, unsigned int bytes, int64_t 
cluster_offset, int64_t cluster_bytes, int flags) "bs %p offset %"PRId64" bytes 
%u cluster_offset %"PRId64" cluster_bytes %"PRId64" flags 0x%x"
 
 # block/stream.c
 stream_one_iteration(void *s, int64_t offset, uint64_t bytes, int 
is_allocated) "s %p offset %" PRId64 " bytes %" PRIu64 " is_allocated %d"
-- 
2.7.4

[Qemu-block] [PATCH 4/5] block-stream: add compress option

[Anton Nefedov]

Signed-off-by: Anton Nefedov 
---
 qapi/block-core.json  |  4 
 include/block/block_int.h |  4 +++-
 block/stream.c| 16 
 blockdev.c| 13 -
 hmp.c |  2 ++
 hmp-commands.hx   |  4 ++--
 6 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index ab96e34..b0d022f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2007,6 +2007,9 @@
 #
 # @speed:  the maximum speed, in bytes per second
 #
+# @compress: true to compress data, if the target format supports it.
+#(default: false). Since 2.12.
+#
 # @on-error: the action to take on an error (default report).
 #'stop' and 'enospc' can only be used if the block device
 #supports io-status (see BlockInfo).  Since 1.3.
@@ -2026,6 +2029,7 @@
 { 'command': 'block-stream',
   'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
 '*base-node': 'str', '*backing-file': 'str', '*speed': 'int',
+'*compress': 'bool',
 '*on-error': 'BlockdevOnError' } }
 
 ##
diff --git a/include/block/block_int.h b/include/block/block_int.h
index a548277..093bf9b 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -863,6 +863,7 @@ int is_windows_drive(const char *filename);
  * @backing_file_str: The file name that will be written to @bs as the
  * the new backing file if the job completes. Ignored if @base is %NULL.
  * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
+ * @compress: True to compress data.
  * @on_error: The action to take upon error.
  * @errp: Error object.
  *
@@ -875,7 +876,8 @@ int is_windows_drive(const char *filename);
  */
 void stream_start(const char *job_id, BlockDriverState *bs,
   BlockDriverState *base, const char *backing_file_str,
-  int64_t speed, BlockdevOnError on_error, Error **errp);
+  int64_t speed, bool compress,
+  BlockdevOnError on_error, Error **errp);
 
 /**
  * commit_start:
diff --git a/block/stream.c b/block/stream.c
index e6f7234..75c9d66 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -38,23 +38,29 @@ typedef struct StreamBlockJob {
 BlockdevOnError on_error;
 char *backing_file_str;
 int bs_flags;
+bool compress;
 } StreamBlockJob;
 
 static int coroutine_fn stream_populate(BlockBackend *blk,
 int64_t offset, uint64_t bytes,
-void *buf)
+void *buf, bool compress)
 {
 struct iovec iov = {
 .iov_base = buf,
 .iov_len  = bytes,
 };
 QEMUIOVector qiov;
+int flags = BDRV_REQ_COPY_ON_READ;
+
+if (compress) {
+flags |= BDRV_REQ_WRITE_COMPRESSED;
+}
 
 assert(bytes < SIZE_MAX);
 qemu_iovec_init_external(, , 1);
 
 /* Copy-on-read the unallocated clusters */
-return blk_co_preadv(blk, offset, qiov.size, , BDRV_REQ_COPY_ON_READ);
+return blk_co_preadv(blk, offset, qiov.size, , flags);
 }
 
 typedef struct {
@@ -166,7 +172,7 @@ static void coroutine_fn stream_run(void *opaque)
 }
 trace_stream_one_iteration(s, offset, n, ret);
 if (copy) {
-ret = stream_populate(blk, offset, n, buf);
+ret = stream_populate(blk, offset, n, buf, s->compress);
 }
 if (ret < 0) {
 BlockErrorAction action =
@@ -227,7 +233,8 @@ static const BlockJobDriver stream_job_driver = {
 
 void stream_start(const char *job_id, BlockDriverState *bs,
   BlockDriverState *base, const char *backing_file_str,
-  int64_t speed, BlockdevOnError on_error, Error **errp)
+  int64_t speed, bool compress,
+  BlockdevOnError on_error, Error **errp)
 {
 StreamBlockJob *s;
 BlockDriverState *iter;
@@ -267,6 +274,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
 s->base = base;
 s->backing_file_str = g_strdup(backing_file_str);
 s->bs_flags = orig_bs_flags;
+s->compress = compress;
 
 s->on_error = on_error;
 trace_stream_start(bs, base, s);
diff --git a/blockdev.c b/blockdev.c
index 56a6b24..18a56d9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2968,6 +2968,7 @@ void qmp_block_stream(bool has_job_id, const char 
*job_id, const char *device,
   bool has_base_node, const char *base_node,
   bool has_backing_file, const char *backing_file,
   bool has_speed, int64_t speed,
+  bool has_compress, bool compress,
   bool has_on_error, BlockdevOnError on_error,
   Error **errp)
 {
@@ -2981,6 +2982,10 @@ void qmp_block_stream(bool has_job_id, const char 
*job_id, const char *device,
 on_error = BLOCKDEV_ON_ERROR_REPORT;
 }
 

[Qemu-block] [PATCH 2/5] qcow2: multiple clusters write compressed

[Anton Nefedov]

From: Pavel Butsykin 

At the moment, qcow2_co_pwritev_compressed can process the requests size
less than or equal to one cluster. This patch added possibility to write
compressed data in the QCOW2 more than one cluster. The implementation
is simple, we just split large requests into separate clusters and write
using existing functionality. For unaligned requests we use a workaround
and do write data without compression.

Signed-off-by: Anton Nefedov 
---
 block/qcow2.c | 77 +++
 1 file changed, 56 insertions(+), 21 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 45c5651..3d5f17d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3325,11 +3325,9 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t 
offset,
 return 0;
 }
 
-/* XXX: put compressed sectors first, then all the cluster aligned
-   tables to avoid losing bytes in alignment */
 static coroutine_fn int
-qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
-uint64_t bytes, QEMUIOVector *qiov)
+qcow2_co_pwritev_cluster_compressed(BlockDriverState *bs, uint64_t offset,
+uint64_t bytes, QEMUIOVector *qiov)
 {
 BDRVQcow2State *s = bs->opaque;
 QEMUIOVector hd_qiov;
@@ -3339,25 +3337,12 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, 
uint64_t offset,
 uint8_t *buf, *out_buf;
 int64_t cluster_offset;
 
-if (bytes == 0) {
-/* align end of file to a sector boundary to ease reading with
-   sector based I/Os */
-cluster_offset = bdrv_getlength(bs->file->bs);
-if (cluster_offset < 0) {
-return cluster_offset;
-}
-return bdrv_truncate(bs->file, cluster_offset, PREALLOC_MODE_OFF, 
NULL);
-}
-
-if (offset_into_cluster(s, offset)) {
-return -EINVAL;
-}
+assert(bytes <= s->cluster_size);
+assert(!offset_into_cluster(s, offset));
 
 buf = qemu_blockalign(bs, s->cluster_size);
-if (bytes != s->cluster_size) {
-if (bytes > s->cluster_size ||
-offset + bytes != bs->total_sectors << BDRV_SECTOR_BITS)
-{
+if (bytes < s->cluster_size) {
+if (offset + bytes != bs->total_sectors << BDRV_SECTOR_BITS) {
 qemu_vfree(buf);
 return -EINVAL;
 }
@@ -3437,6 +3422,56 @@ fail:
 return ret;
 }
 
+/* XXX: put compressed sectors first, then all the cluster aligned
+   tables to avoid losing bytes in alignment */
+static coroutine_fn int
+qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
+uint64_t bytes, QEMUIOVector *qiov)
+{
+BDRVQcow2State *s = bs->opaque;
+QEMUIOVector hd_qiov;
+uint64_t curr_off = 0;
+int ret;
+
+if (bytes == 0) {
+/* align end of file to a sector boundary to ease reading with
+   sector based I/Os */
+int64_t cluster_offset = bdrv_getlength(bs->file->bs);
+if (cluster_offset < 0) {
+return cluster_offset;
+}
+return bdrv_truncate(bs->file, cluster_offset, PREALLOC_MODE_OFF, 
NULL);
+}
+
+if (offset_into_cluster(s, offset)) {
+return -EINVAL;
+}
+
+qemu_iovec_init(_qiov, qiov->niov);
+do {
+uint32_t chunk_size;
+
+qemu_iovec_reset(_qiov);
+chunk_size = MIN(bytes, s->cluster_size);
+qemu_iovec_concat(_qiov, qiov, curr_off, chunk_size);
+
+ret = qcow2_co_pwritev_cluster_compressed(bs, offset + curr_off,
+  chunk_size, _qiov);
+if (ret == -ENOTSUP) {
+ret = qcow2_co_pwritev(bs, offset + curr_off, chunk_size,
+   _qiov, 0);
+}
+if (ret < 0) {
+break;
+}
+curr_off += chunk_size;
+bytes -= chunk_size;
+} while (bytes);
+qemu_iovec_destroy(_qiov);
+
+return ret;
+}
+
 static int make_completely_empty(BlockDriverState *bs)
 {
 BDRVQcow2State *s = bs->opaque;
-- 
2.7.4

[Qemu-block] [PATCH 0/5] compressed block-stream

[Anton Nefedov]

It might be useful to compress images during block-stream;
this way the user can merge compressed images of a backing chain and
the result will remain compressed.

Anton Nefedov (4):
  qcow2: reject unaligned offsets in write compressed
  block: support compressed write for copy-on-read
  block-stream: add compress option
  iotest 030: add compressed block-stream test

Pavel Butsykin (1):
  qcow2: multiple clusters write compressed

 qapi/block-core.json   |  4 +++
 include/block/block_int.h  |  4 ++-
 block/io.c | 30 +++
 block/qcow2.c  | 73 +++---
 block/stream.c | 16 +++---
 blockdev.c | 13 -
 hmp.c  |  2 ++
 block/trace-events |  2 +-
 hmp-commands.hx|  4 +--
 tests/qemu-iotests/030 | 69 ++-
 tests/qemu-iotests/030.out |  4 +--
 11 files changed, 186 insertions(+), 35 deletions(-)

-- 
2.7.4

[Qemu-block] [PATCH 1/5] qcow2: reject unaligned offsets in write compressed

[Anton Nefedov]

Misaligned compressed write is not supported.

Signed-off-by: Anton Nefedov 
---
 block/qcow2.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index 92cb9f9..45c5651 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3349,6 +3349,10 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, 
uint64_t offset,
 return bdrv_truncate(bs->file, cluster_offset, PREALLOC_MODE_OFF, 
NULL);
 }
 
+if (offset_into_cluster(s, offset)) {
+return -EINVAL;
+}
+
 buf = qemu_blockalign(bs, s->cluster_size);
 if (bytes != s->cluster_size) {
 if (bytes > s->cluster_size ||
-- 
2.7.4

Re: [Qemu-block] [PATCH v5 02/29] Replace all occurances of __FUNCTION__ with __func__

[Gerd Hoffmann]

On Mon, Nov 13, 2017 at 02:34:42PM -0800, Alistair Francis wrote:
> Replace all occurs of __FUNCTION__ except for the check in checkpatch
> with the non GCC specific __func__.
> 
> One line in hcd-musb.c was manually tweaked to pass checkpatch.
> 
> Signed-off-by: Alistair Francis 
> Cc: Gerd Hoffmann 
> Cc: Andrzej Zaborowski 
> Cc: Stefano Stabellini 
> Cc: Anthony Perard 
> Cc: John Snow 
> Cc: Aurelien Jarno 
> Cc: Yongbok Kim 
> Cc: Peter Crosthwaite 
> Cc: Stefan Hajnoczi 
> Cc: Fam Zheng 
> Cc: Juan Quintela 
> Cc: "Dr. David Alan Gilbert" 
> Cc: qemu-...@nongnu.org
> Cc: qemu-block@nongnu.org
> Cc: xen-de...@lists.xenproject.org
> Reviewed-by: Eric Blake 
> Reviewed-by: Stefan Hajnoczi 
> Reviewed-by: Anthony PERARD 
> Reviewed-by: Juan Quintela 

Reviewed-by: Gerd Hoffmann