Re: [PATCH 02/23] iotests.py: Store socket files in $SOCK_DIR

[Thomas Huth]

On 10/10/2019 17.24, Max Reitz wrote:
> iotests.py itself does not store socket files, but it machine.py and
> qtest.py do.  iotests.py needs to pass the respective path to them, and
> they need to adhere to it.
> 
> Signed-off-by: Max Reitz 
> ---
>  python/qemu/machine.py| 15 ---
>  python/qemu/qtest.py  |  9 ++---
>  tests/qemu-iotests/iotests.py |  4 +++-
>  3 files changed, 21 insertions(+), 7 deletions(-)

Reviewed-by: Thomas Huth 

Re: [PATCH 00/23] iotests: Add and use $SOCK_DIR

[Thomas Huth]

On 10/10/2019 17.24, Max Reitz wrote:
> Hi,
> 
> Perhaps the main reason we cannot run important tests such as 041 in CI
> is that when they care Unix sockets in $TEST_DIR, the path may become
> too long to connect to them.
> 
> To get by this problem, this series lets the check script create a new
> temporary directory (mktemp -d) and then makes the iotests use it for
> all Unix sockets.

Thanks a lot for tackling this!

I gave it a try, and most tests work fine now indeed when I run them in
a directory with a vry long file name.

I still get an error with 028 though:


$ ./check -qcow2 028
QEMU  --
"/home/thuth/tmp/qemu-with-very-long-directory-name-01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789/tests/qemu-iotests/../../x86_64-softmmu/qemu-system-x86_64"
-nodefaults -display none -machine accel=qtest
QEMU_IMG  --
"/home/thuth/tmp/qemu-with-very-long-directory-name-01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789/tests/qemu-iotests/../../qemu-img"

QEMU_IO   --
"/home/thuth/tmp/qemu-with-very-long-directory-name-01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789/tests/qemu-iotests/../../qemu-io"
 --cache writeback -f qcow2
QEMU_NBD  --
"/home/thuth/tmp/qemu-with-very-long-directory-name-01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789/tests/qemu-iotests/../../qemu-nbd"

IMGFMT-- qcow2 (compat=1.1)
IMGPROTO  -- file
PLATFORM  -- Linux/x86_64 thuth 4.18.0-80.11.2.el8_0.x86_64
TEST_DIR  --
/home/thuth/tmp/qemu-with-very-long-directory-name-01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789/tests/qemu-iotests/scratch
SOCK_DIR  -- /tmp/tmp.YEmubpCxRH
SOCKET_SCM_HELPER --
/home/thuth/tmp/qemu-with-very-long-directory-name-01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789/tests/qemu-iotests/socket_scm_helper

028  fail   [09:11:52] [09:11:53]output
mismatch (see 028.out.bad)
--- /home/thuth/devel/qemu/tests/qemu-iotests/028.out   2019-08-16
18:00:39.258741027 +0200
+++
/home/thuth/tmp/qemu-with-very-long-directory-name-01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789/tests/qemu-iotests/028.out.bad
2019-10-11 09:11:53.822111901 +0200
@@ -468,7 +468,8 @@

 block-backup

-Formatting 'TEST_DIR/t.IMGFMT.copy', fmt=IMGFMT size=4294968832
backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
+(qemu)
+Timeout waiting for Formatting on handle 0
 (qemu) info block-jobs
 No active jobs
 === IO: pattern 195
Failures: 028
Failed 1 of 1 iotests


028 works fine if I start it from a directory with a short filename.


I also saw an error with "./check -raw 055" once, but it does not seem
to be reproducible:


055  fail   [09:08:28] [09:10:57]output
mismatch (see 055.out.bad)
--- /home/thuth/devel/qemu/tests/qemu-iotests/055.out   2019-08-16
18:00:39.262741079 +0200
+++
/home/thuth/tmp/qemu-with-very-long-directory-name-01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789/tests/qemu-iotests/055.out.bad
2019-10-11 09:10:57.059254617 +0200
@@ -1,5 +1,35 @@
-..
+WARNING:qemu.machine:qemu received signal 9:
/home/thuth/tmp/qemu-with-very-long-directory-name-01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789/tests/qemu-iotests/../../x86_64-softmmu/qemu-system-x86_64
-chardev socket,id=mon,path=/tmp/tmp.3mKmQDU7Ft/qemu-24878-monitor.sock
-mon chardev=mon,mode=control -display none -vga none -qtest
unix:path=/tmp/tmp.3mKmQDU7Ft/qemu-24878-qtest.sock -accel qtest
-nodefaults -display none -machine accel=qtest -drive
if=virtio,id=drive0,file=blkdebug::/home/thuth/tmp/qemu-with-very-long-directory-name-01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789/tests/qemu-iotests/scratch/test.img,format=raw,cache=writeback
-drive
if=none,id=drive1,file=/home/thuth/tmp/qemu-with-very-long-directory-name-01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789/tests/qemu-iotests/scratch/blockdev-target.img,format=vmdk,cache=writeback
+E.
+==
+ERROR: test_complete_compress_blockdev_backup
(__main__.TestDriveCompression)
+--
+Traceback (most recent call last):
+  File
"/home/thuth/devel/qemu/tests/qemu-iotests/../../python/qemu/qmp.py",
line 122, in __get_events
+ret = self.__json_read(only_event=True)
+  

Re: [PATCH 06/23] iotests/083: Create socket in $SOCK_DIR

[Thomas Huth]

On 10/10/2019 17.24, Max Reitz wrote:
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/083 |  6 +++---
>  tests/qemu-iotests/083.out | 34 +-
>  2 files changed, 20 insertions(+), 20 deletions(-)

Reviewed-by: Thomas Huth 

Re: [PATCH v2 1/2] nbd: Don't send oversize strings

[Vladimir Sementsov-Ogievskiy]

11.10.2019 0:00, Eric Blake wrote:
> Qemu as server currently won't accept export names larger than 256
> bytes, nor create dirty bitmap names longer than 1023 bytes, so most
> uses of qemu as client or server have no reason to get anywhere near
> the NBD spec maximum of a 4k limit per string.
> 
> However, we weren't actually enforcing things, ignoring when the
> remote side violates the protocol on input, and also having several
> code paths where we send oversize strings on output (for example,
> qemu-nbd --description could easily send more than 4k).  Tighten
> things up as follows:
> 
> client:
> - Perform bounds check on export name and dirty bitmap request prior
>to handing it to server
> - Validate that copied server replies are not too long (ignoring
>NBD_INFO_* replies that are not copied is not too bad)
> server:
> - Perform bounds check on export name and description prior to
>advertising it to client
> - Reject client name or metadata query that is too long
> 
> Signed-off-by: Eric Blake 
> ---
>   include/block/nbd.h |  1 +
>   block/nbd.c |  9 +
>   blockdev-nbd.c  |  5 +
>   nbd/client.c| 16 +---
>   nbd/server.c| 14 --
>   qemu-nbd.c  |  9 +
>   6 files changed, 49 insertions(+), 5 deletions(-)
> 
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 316fd705a9e4..fcabdf0f37c3 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -232,6 +232,7 @@ enum {
>* going larger would require an audit of more code to make sure we
>* aren't overflowing some other buffer. */

This comment says, that we restrict export name to 256...

>   #define NBD_MAX_NAME_SIZE 256
> +#define NBD_MAX_STRING_SIZE 4096
> 
>   /* Two types of reply structures */
>   #define NBD_SIMPLE_REPLY_MAGIC  0x67446698
> diff --git a/block/nbd.c b/block/nbd.c
> index 813c40d8f067..76eb1dbe04df 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -1621,6 +1621,10 @@ static int nbd_process_options(BlockDriverState *bs, 
> QDict *options,
>   }
> 
>   s->export = g_strdup(qemu_opt_get(opts, "export"));
> +if (s->export && strlen(s->export) > NBD_MAX_STRING_SIZE) {
> +error_setg(errp, "export name too long to send to server");
> +goto error;
> +}
> 
>   s->tlscredsid = g_strdup(qemu_opt_get(opts, "tls-creds"));
>   if (s->tlscredsid) {
> @@ -1638,6 +1642,11 @@ static int nbd_process_options(BlockDriverState *bs, 
> QDict *options,
>   }
> 
>   s->x_dirty_bitmap = g_strdup(qemu_opt_get(opts, "x-dirty-bitmap"));
> +if (s->x_dirty_bitmap && strlen(s->x_dirty_bitmap) > 
> NBD_MAX_STRING_SIZE) {
> +error_setg(errp, "x_dirty_bitmap query too long to send to server");
> +goto error;

this is new latest "goto error", you should add g_free(s->x_dirty_bitmap) in 
following "if (ret < 0)"

> +}
> +
>   s->reconnect_delay = qemu_opt_get_number(opts, "reconnect-delay", 0);
> 
>   ret = 0;
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index 6a8b206e1d74..8c20baa4a4b9 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -162,6 +162,11 @@ void qmp_nbd_server_add(const char *device, bool 
> has_name, const char *name,
>   name = device;
>   }
> 
> +if (strlen(name) > NBD_MAX_STRING_SIZE) {
> +error_setg(errp, "export name '%s' too long", name);
> +return;
> +}

Hmmm, no, so here we restrict to 4096, but, we will not allow client to request 
more than
256. Seems, to correctly update server-part, we should drop NBD_MAX_NAME_SIZE 
and do the
audit mentioned in the comment above its definition.

> +
>   if (nbd_export_find(name)) {
>   error_setg(errp, "NBD server already has export named '%s'", name);
>   return;
> diff --git a/nbd/client.c b/nbd/client.c
> index f6733962b49b..d6e29daced63 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -289,8 +289,8 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, 
> char **description,
>   return -1;
>   }
>   len -= sizeof(namelen);
> -if (len < namelen) {
> -error_setg(errp, "incorrect option name length");
> +if (len < namelen || namelen > NBD_MAX_STRING_SIZE) {
> +error_setg(errp, "incorrect list name length");

New wording made me go above and read the comment, what functions does. Comment 
is good, but without
it, it sounds like name of the list for me...

>   nbd_send_opt_abort(ioc);
>   return -1;
>   }
> @@ -303,6 +303,11 @@ static int nbd_receive_list(QIOChannel *ioc, char 
> **name, char **description,
>   local_name[namelen] = '\0';
>   len -= namelen;
>   if (len) {
> +if (len > NBD_MAX_STRING_SIZE) {
> +error_setg(errp, "incorrect list description length");
> +nbd_send_opt_abort(ioc);
> +return -1;
> +}
>   local_desc = g_malloc(len + 1);
>   if (nbd_read(ioc, local_desc, len, "expo

Re: [PATCH 07/23] iotests/140: Create socket in $SOCK_DIR

[Thomas Huth]

On 10/10/2019 17.24, Max Reitz wrote:
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/140 | 8 
>  tests/qemu-iotests/140.out | 2 +-
>  2 files changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Thomas Huth 

Re: [PATCH 08/23] iotests/143: Create socket in $SOCK_DIR

[Thomas Huth]

On 10/10/2019 17.24, Max Reitz wrote:
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/143 | 6 +++---
>  tests/qemu-iotests/143.out | 2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Thomas Huth 

Re: [PATCH 09/23] iotests/147: Create socket in $SOCK_DIR

[Thomas Huth]

On 10/10/2019 17.24, Max Reitz wrote:
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/147 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Thomas Huth 

Re: [PATCH 10/23] iotests/181: Create socket in $SOCK_DIR

[Thomas Huth]

On 10/10/2019 17.24, Max Reitz wrote:
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/181 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Thomas Huth 

Re: [PATCH 11/23] iotests/182: Create socket in $SOCK_DIR

[Thomas Huth]

On 10/10/2019 17.24, Max Reitz wrote:
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/182 | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Thomas Huth 

Re: [PATCH v2 2/2] nbd: Allow description when creating NBD blockdev

[Vladimir Sementsov-Ogievskiy]

11.10.2019 0:00, Eric Blake wrote:
> Allow blockdevs to match the feature already present in qemu-nbd -D.
> Enhance iotest 223 to cover it.
> 
> Signed-off-by: Eric Blake 

Reviewed-by: Vladimir Sementsov-Ogievskiy 

> ---
>   qapi/block.json| 8 +---
>   blockdev-nbd.c | 9 -
>   monitor/hmp-cmds.c | 4 ++--
>   tests/qemu-iotests/223 | 2 +-
>   tests/qemu-iotests/223.out | 1 +
>   5 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/qapi/block.json b/qapi/block.json
> index 145c268bb646..a6617b5bd03a 100644
> --- a/qapi/block.json
> +++ b/qapi/block.json
> @@ -250,9 +250,11 @@
>   # @name: Export name. If unspecified, the @device parameter is used as the
>   #export name. (Since 2.12)
>   #
> +# @description: Free-form description of the export. (Since 4.2)

Worth mention maximum of 4096?

> +#
>   # @writable: Whether clients should be able to write to the device via the
>   # NBD connection (default false).
> -
> +#
>   # @bitmap: Also export the dirty bitmap reachable from @device, so the
>   #  NBD client can use NBD_OPT_SET_META_CONTEXT with
>   #  "qemu:dirty-bitmap:NAME" to inspect the bitmap. (since 4.0)
> @@ -263,8 +265,8 @@
>   # Since: 1.3.0
>   ##

[..]


-- 
Best regards,
Vladimir

Re: [PATCH 12/23] iotests/183: Create socket in $SOCK_DIR

[Thomas Huth]

On 10/10/2019 17.24, Max Reitz wrote:
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/183 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Thomas Huth 

Re: Problems with c8bb23cbdbe3 on ppc64le

[Max Reitz]

On 10.10.19 18:15, Anton Nefedov wrote:
> On 10/10/2019 6:17 PM, Max Reitz wrote:
>> Hi everyone,
>>
>> (CCs just based on tags in the commit in question)
>>
>> I have two bug reports which claim problems of qcow2 on XFS on ppc64le
>> machines since qemu 4.1.0.  One of those is about bad performance
>> (sorry, is isn’t public :-/), the other about data corruption
>> (https://bugzilla.redhat.com/show_bug.cgi?id=1751934).
>>
>> It looks like in both cases reverting c8bb23cbdbe3 solves the problem
>> (which optimized COW of unallocated areas).
>>
>> I think I’ve looked at every angle but can‘t find what could be wrong
>> with it.  Do any of you have any idea? :-/
>>
> 
> hi,
> 
> oh, that patch strikes again..
> 
> I don't quite follow, was this bug confirmed to happen on x86? Comment 8
> (https://bugzilla.redhat.com/show_bug.cgi?id=1751934#c8) mentioned that
> (or was that mixed up with the old xfsctl bug?)

I think that was mixed up with the xfsctl bug, yes.

> Regardless of the platform, does it reproduce? That's comforting
> already; worst case we can trace each and every request then (unless it
> will stop to reproduce this way).

I haven’t been able to reproduce it yet (wrestling with the test system
and getting ppc64 machines provisioned), but as far as I know it
reproduces reliably on ppc64, but only there.

> Also, perhaps it's worth to try to replace fallocate with write(0)?
> Either in qcow2 (in the patch, bdrv_co_pwrite_zeroes -> bdrv_co_pwritev)
> or in the file driver. It might hint whether it's misbehaving fallocate
> (in qemu or in kernel) or something else.

Good idea, that should at least tell us something about the corruption.

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH 01/23] iotests: Introduce $SOCK_DIR

[Max Reitz]

On 10.10.19 20:18, Eric Blake wrote:
> On 10/10/19 10:24 AM, Max Reitz wrote:
>> Unix sockets generally have a maximum path length.  Depending on your
>> $TEST_DIR, it may be exceeded and then all tests that create and use
>> Unix sockets there may fail.
>>
>> Circumvent this by adding a new scratch directory specifically for
>> Unix socket files.  It defaults to a temporary directory (mktemp -d)
>> that is completely removed after the iotests are done.
>>
>> (By default, mktemp -d creates a /tmp/tmp.XX directory, which
>> should be short enough for our use cases.)
>>
>> Signed-off-by: Max Reitz 
>> ---
>>   tests/qemu-iotests/check | 17 +
> 
>> +tmp_sock_dir=false
>> +if [ -z "$SOCK_DIR" ]; then
>> +    SOCK_DIR=$(mktemp -d)
>> +    tmp_sock_dir=true
>> +fi
>> +
>> +if [ ! -d "$SOCK_DIR" ]; then
>> +    mkdir "$SOCK_DIR"
>> +fi
> 
> Should this use mkdir -p, in case two parallel processes compete with
> the same SOCK_DIR?

I would have used mkdir -p, but I saw we used this construct for
TEST_DIR, so I thought I‘d just go for the same.

> What if SOCK_DIR is set to something that is not a directory (say a
> file), at which point mkdir fails, but you don't seem to be catching
> that failure.

Well, the same applies to TEST_DIR.  And technically, as long as we
don’t use mkdir -p for either, not catching the error at least helps
circumvent the potential race. O:-)

(I’ll convert both to mkdir -p with error handling.)

Max

> Otherwise looks good.




signature.asc
Description: OpenPGP digital signature

Re: [PATCH 04/23] iotests: Filter $SOCK_DIR

[Max Reitz]

On 10.10.19 20:42, Eric Blake wrote:
> On 10/10/19 10:24 AM, Max Reitz wrote:
>> Signed-off-by: Max Reitz 
>> ---
>>   tests/qemu-iotests/common.filter | 8 ++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/common.filter
>> b/tests/qemu-iotests/common.filter
>> index 9f418b4881..cd42f5e7e3 100644
>> --- a/tests/qemu-iotests/common.filter
>> +++ b/tests/qemu-iotests/common.filter
>> @@ -43,7 +43,8 @@ _filter_qom_path()
>>   # replace occurrences of the actual TEST_DIR value with TEST_DIR
>>   _filter_testdir()
>>   {
>> -    $SED -e "s#$TEST_DIR/#TEST_DIR/#g"
>> +    $SED -e "s#$TEST_DIR/#TEST_DIR/#g" \
>> + -e "s#$SOCK_DIR/#SOCK_DIR/#g"
> 
> Do we want to output a literal 'SOCK_DIR' (every test that uses it has
> to update their expected output), or can we make this also output a
> literal 'TEST_DIR' (output is a bit more confusing on which dir to look
> in, but fewer files to touch)?  Your preference.

There’s another advantage to filtering it to be TEST_DIR, and that’s the
fact that if $TEST_DIR and $SOCK_DIR are the same, we will always
replace $SOCK_DIR by TEST_DIR.

But I still preferred filtering it to be SOCK_DIR, because that seemed
to me like we would have done it had we had a SOCK_DIR from the start.

> Reviewed-by: Eric Blake 

Thanks for reviewing!

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH 14/23] iotests/194: Create sockets in $SOCK_DIR

[Max Reitz]

On 10.10.19 21:43, Eric Blake wrote:
> On 10/10/19 10:24 AM, Max Reitz wrote:
>> Signed-off-by: Max Reitz 
>> ---
>>   tests/qemu-iotests/194 | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/194 b/tests/qemu-iotests/194
>> index d746ab1e21..72e47e8833 100755
>> --- a/tests/qemu-iotests/194
>> +++ b/tests/qemu-iotests/194
>> @@ -26,8 +26,8 @@ iotests.verify_platform(['linux'])
>>     with iotests.FilePath('source.img') as source_img_path, \
>>    iotests.FilePath('dest.img') as dest_img_path, \
> 
> Any reason these are still two iotests.FilePath(),
> 
>> - iotests.FilePath('migration.sock') as migration_sock_path, \
>> - iotests.FilePath('nbd.sock') as nbd_sock_path, \
>> + iotests.FilePaths(['migration.sock', 'nbd.sock'],
>> iotests.sock_dir) as \
>> + [migration_sock_path, nbd_sock_path], \
> 
> while you joined this into one iotests.FilePaths()?  Doesn't affect
> correctness, but does raise a consistency issue (I noticed it again in
> the untouched part of patch 17).

The migration.sock FilePath line would have exceeded 80 characters, so I
would’ve had to wrap it.  Thus I decided I might as well make it a
FilePaths.

I didn’t dare touch the surrounding code, because that would have
required me to explain myself in the commit message. O:-)

Max

>>    iotests.VM('source') as source_vm, \
>>    iotests.VM('dest') as dest_vm:
>>  
> 




signature.asc
Description: OpenPGP digital signature

Re: [PATCH 04/23] iotests: Filter $SOCK_DIR

[Max Reitz]

On 10.10.19 21:50, Eric Blake wrote:
> On 10/10/19 10:24 AM, Max Reitz wrote:
>> Signed-off-by: Max Reitz 
>> ---
>>   tests/qemu-iotests/common.filter | 8 ++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
> 
>> @@ -218,7 +221,8 @@ _filter_nbd()
>>   # Filter out the TCP port number since this changes between runs.
>>   $SED -e '/nbd\/.*\.c:/d' \
>>   -e 's#127\.0\.0\.1:[0-9]*#127.0.0.1:PORT#g' \
>> -    -e "s#?socket=$TEST_DIR#?socket=TEST_DIR#g" \
>> +    -e "s#?socket=$SOCK_DIR#?socket=TEST_DIR#g" \
>> +    -e "s#?socket=$SOCK_DIR#?socket=SOCK_DIR#g" \
>>   -e 's#\(foo\|PORT/\?\|.sock\): Failed to .*$#\1#'
> 
> This goes away in 23, but this looks crazy.  Don't you really want the
> first line to replace $TEST_DIR with TEST_DIR (not $SOCK_DIR with
> TEST_DIR)?  Otherwise, bisection is likely to break until all the
> intermediate patches have made the conversion to stop using TEST_DIR.
> 
> I already gave R-b, but you'll need to fix this one.

Oops, yes.  I messed it up.  I only intended to add the SOCK_DIR
replacement line.

(Originally I had 23 merged into this one, and then I noticed it would
break bisection, so I tried to pull it out.  And failed, as you can see.)

Max




signature.asc
Description: OpenPGP digital signature

Re: [PATCH 04/23] iotests: Filter $SOCK_DIR

[Thomas Huth]

On 11/10/2019 09.54, Max Reitz wrote:
> On 10.10.19 20:42, Eric Blake wrote:
>> On 10/10/19 10:24 AM, Max Reitz wrote:
>>> Signed-off-by: Max Reitz 
>>> ---
>>>   tests/qemu-iotests/common.filter | 8 ++--
>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/common.filter
>>> b/tests/qemu-iotests/common.filter
>>> index 9f418b4881..cd42f5e7e3 100644
>>> --- a/tests/qemu-iotests/common.filter
>>> +++ b/tests/qemu-iotests/common.filter
>>> @@ -43,7 +43,8 @@ _filter_qom_path()
>>>   # replace occurrences of the actual TEST_DIR value with TEST_DIR
>>>   _filter_testdir()
>>>   {
>>> -    $SED -e "s#$TEST_DIR/#TEST_DIR/#g"
>>> +    $SED -e "s#$TEST_DIR/#TEST_DIR/#g" \
>>> + -e "s#$SOCK_DIR/#SOCK_DIR/#g"
>>
>> Do we want to output a literal 'SOCK_DIR' (every test that uses it has
>> to update their expected output), or can we make this also output a
>> literal 'TEST_DIR' (output is a bit more confusing on which dir to look
>> in, but fewer files to touch)?  Your preference.
> 
> There’s another advantage to filtering it to be TEST_DIR, and that’s the
> fact that if $TEST_DIR and $SOCK_DIR are the same, we will always
> replace $SOCK_DIR by TEST_DIR.
> 
> But I still preferred filtering it to be SOCK_DIR, because that seemed
> to me like we would have done it had we had a SOCK_DIR from the start.

I also think that using SOCK_DIR is the better choice. It's a little bit
more churn now, but in the long run, it will help to avoid confusion,
and I think that's more important.

 Thomas




signature.asc
Description: OpenPGP digital signature

Re: [PATCH 00/23] iotests: Add and use $SOCK_DIR

[Max Reitz]

On 11.10.19 09:27, Thomas Huth wrote:
> On 10/10/2019 17.24, Max Reitz wrote:
>> Hi,
>>
>> Perhaps the main reason we cannot run important tests such as 041 in CI
>> is that when they care Unix sockets in $TEST_DIR, the path may become
>> too long to connect to them.
>>
>> To get by this problem, this series lets the check script create a new
>> temporary directory (mktemp -d) and then makes the iotests use it for
>> all Unix sockets.
> 
> Thanks a lot for tackling this!
> 
> I gave it a try, and most tests work fine now indeed when I run them in
> a directory with a vry long file name.
> 
> I still get an error with 028 though:

Hm, I didn’t see any error for 028 or 055 myself.  028 makes use of
common.qemu, which uses FIFOs, and I thought there were exempt from this
problem.  And for 055 I have no idea.

Maybe just bugs in qemu? :-)

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH 00/23] iotests: Add and use $SOCK_DIR

[Thomas Huth]

On 11/10/2019 10.03, Max Reitz wrote:
> On 11.10.19 09:27, Thomas Huth wrote:
>> On 10/10/2019 17.24, Max Reitz wrote:
>>> Hi,
>>>
>>> Perhaps the main reason we cannot run important tests such as 041 in CI
>>> is that when they care Unix sockets in $TEST_DIR, the path may become
>>> too long to connect to them.
>>>
>>> To get by this problem, this series lets the check script create a new
>>> temporary directory (mktemp -d) and then makes the iotests use it for
>>> all Unix sockets.
>>
>> Thanks a lot for tackling this!
>>
>> I gave it a try, and most tests work fine now indeed when I run them in
>> a directory with a vry long file name.
>>
>> I still get an error with 028 though:
> 
> Hm, I didn’t see any error for 028 or 055 myself.  028 makes use of
> common.qemu, which uses FIFOs, and I thought there were exempt from this
> problem.  And for 055 I have no idea.
> 
> Maybe just bugs in qemu? :-)

Yeah, maybe... anyway, both, 028 and 055, are not in the auto group, so
I think we simply could ignore these bugs for now.

 Thomas



signature.asc
Description: OpenPGP digital signature

CfP: Software Defined Storage devroom at FOSDEM 2020

[Niels de Vos]

FOSDEM is a free software event that offers open source communities a
place to meet, share ideas and collaborate.  It is renown for being
highly developer- oriented and brings together 8000+ participants from
all over the world.  It is held in the city of Brussels (Belgium).

FOSDEM 2020 will take place during the weekend of February 1st-2nd 2020.
More details about the event can be found at http://fosdem.org/

** Call For Participation

The Software Defined Storage devroom will go into it's fourth round for
talks around Open Source Software Defined Storage projects, management
tools and real world deployments.

Presentation topics could include but are not limited too:

- Your work on a SDS project like Ceph, Gluster, OpenEBS or LizardFS
- Your work on or with SDS related projects like SWIFT or Container
  Storage Interface
- Management tools for SDS deployments
- Monitoring tools for SDS clusters

** Important dates:

- Nov 24th 2019:  submission deadline for talk proposals
- Dec 15th 2019:  announcement of the final schedule
- Feb  2nd 2020:  Software Defined Storage dev room

Talk proposals will be reviewed by a steering committee:
- Niels de Vos (OpenShift Container Storage Developer - Red Hat)
- Jan Fajerski (Ceph Developer - SUSE)
- Kai Wagner (SUSE)
- Mike Perez (Ceph Community Manager, Red Hat)

Use the FOSDEM 'pentabarf' tool to submit your proposal:
https://penta.fosdem.org/submission/FOSDEM20

- If necessary, create a Pentabarf account and activate it.  Please
  reuse your account from previous years if you have already created it.
  https://penta.fosdem.org/user/new_account/FOSDEM20

- In the "Person" section, provide First name, Last name (in the
  "General" tab), Email (in the "Contact" tab) and Bio ("Abstract" field
  in the "Description" tab).

- Submit a proposal by clicking on "Create event".

- Important! Select the "Software Defined Storage devroom" track (on the
  "General" tab).

- Provide the title of your talk ("Event title" in the "General" tab).

- Provide a description of the subject of the talk and the intended
  audience (in the "Abstract" field of the "Description" tab)

- Provide a rough outline of the talk or goals of the session (a short
  list of bullet points covering topics that will be discussed) in the
  "Full description" field in the "Description" tab

- Provide an expected length of your talk in the "Duration" field.
  Please consider at least 5 minutes of discussion into your proposal
  plus allow 5 minutes for the handover to the next presenter.
  Suggested talk length would be 20+5+5 and 45+10+5 minutes. Note that
  short talks have a preference so that more topics can be presented
  during the day.

** Recording of talks

The FOSDEM organizers plan to have live streaming and recording fully
working, both for remote/later viewing of talks, and so that people can
watch streams in the hallways when rooms are full. This requires
speakers to consent to being recorded and streamed. If you plan to be a
speaker, please understand that by doing so you implicitly give consent
for your talk to be recorded and streamed. The recordings will be
published under the same license as all FOSDEM content (CC-BY).

Hope to hear from you soon! And please forward this announcement.

If you have any further questions, please write to the mailinglist at
storage-devr...@lists.fosdem.org and we will try to answer as soon as
possible.

Thanks!


signature.asc
Description: PGP signature

Re: [PATCH 3/4] block/mirror: support unaligned write in active mirror

[Vladimir Sementsov-Ogievskiy]

04.10.2019 19:31, Max Reitz wrote:
> On 12.09.19 17:13, Vladimir Sementsov-Ogievskiy wrote:
>> Prior 9adc1cb49af8d do_sync_target_write had a bug: it reset aligned-up
>> region in the dirty bitmap, which means that we may not copy some bytes
>> and assume them copied, which actually leads to producing corrupted
>> target.
>>
>> So 9adc1cb49af8d forced dirty bitmap granularity to be
>> request_alignment for mirror-top filter, so we are not working with
>> unaligned requests. However forcing large alignment obviously decreases
>> performance of unaligned requests.
>>
>> This commit provides another solution for the problem: if unaligned
>> padding is already dirty, we can safely ignore it, as
>> 1. It's dirty, it will be copied by mirror_iteration anyway
>> 2. It's dirty, so skipping it now we don't increase dirtiness of the
>> bitmap and therefore don't damage "synchronicity" of the
>> write-blocking mirror.
>>
>> If unaligned padding is not dirty, we just write it, no reason to touch
>> dirty bitmap if we succeed (on failure we'll set the whole region
>> ofcourse, but we loss "synchronicity" on failure anyway).
>>
>> Note: we need to disable dirty_bitmap, otherwise we will not be able to
>> see in do_sync_target_write bitmap state before current operation. We
>> may of course check dirty bitmap before the operation in
>> bdrv_mirror_top_do_write and remember it, but we don't need active
>> dirty bitmap for write-blocking mirror anyway.
>>
>> New code-path is unused until the following commit reverts
>> 9adc1cb49af8d.
>>
>> Suggested-by: Denis V. Lunev 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>> ---
>>   block/mirror.c | 39 ++-
>>   1 file changed, 38 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index d176bf5920..d192f6a96b 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -1204,6 +1204,39 @@ do_sync_target_write(MirrorBlockJob *job, 
>> MirrorMethod method,
>>QEMUIOVector *qiov, int flags)
>>   {
>>   int ret;
>> +size_t qiov_offset = 0;
>> +
>> +if (!QEMU_IS_ALIGNED(offset, job->granularity) &&
>> +bdrv_dirty_bitmap_get(job->dirty_bitmap, offset)) {
>> +/*
>> + * Dirty unaligned padding
>> + * 1. It's already dirty, no damage to "actively_synced" if we 
>> just
>> + *skip unaligned part.
>> + * 2. If we copy it, we can't reset corresponding bit in
>> + *dirty_bitmap as there may be some "dirty" bytes still not
>> + *copied.
>> + * So, just ignore it.
>> + */
>> +qiov_offset = QEMU_ALIGN_UP(offset, job->granularity) - offset;
>> +if (bytes <= qiov_offset) {
>> +/* nothing to do after shrink */
>> +return;
>> +}
>> +offset += qiov_offset;
>> +bytes -= qiov_offset;
>> +}
>> +
>> +if (!QEMU_IS_ALIGNED(offset + bytes, job->granularity) &&
>> +bdrv_dirty_bitmap_get(job->dirty_bitmap, offset + bytes - 1))
>> +{
>> +uint64_t tail = (offset + bytes) % job->granularity;
>> +
>> +if (bytes <= tail) {
>> +/* nothing to do after shrink */
>> +return;
>> +}
>> +bytes -= tail;
>> +}
>>   
>>   bdrv_reset_dirty_bitmap(job->dirty_bitmap, offset, bytes);
>>   
> 
> The bdrv_set_dirty_bitmap() in the error case below needs to use the
> original offset/bytes, I suppose.

No, because we shrink tail only if it is already dirty. And we've locked the
region for in-flight operation, so nobody can clear the bitmap in a meantime.

But still, here is something to do:

for not-shrinked tails, if any, we should:
1. align down for reset
2. align up for set on failure


> 
> Apart from that, looks good to me.
> 
> Max
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PULL 5/5] multifd: Use number of channels as listen backlog

[Wei Yang]

On Wed, Sep 04, 2019 at 08:29:15AM +0200, Juan Quintela wrote:
>Reviewed-by: Daniel P. Berrangé 
>Signed-off-by: Juan Quintela 
>---
> migration/socket.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
>diff --git a/migration/socket.c b/migration/socket.c
>index e63f5e1612..97c9efde59 100644
>--- a/migration/socket.c
>+++ b/migration/socket.c
>@@ -178,10 +178,15 @@ static void 
>socket_start_incoming_migration(SocketAddress *saddr,
> {
> QIONetListener *listener = qio_net_listener_new();
> size_t i;
>+int num = 1;
> 
> qio_net_listener_set_name(listener, "migration-socket-listener");
> 
>-if (qio_net_listener_open_sync(listener, saddr, 1, errp) < 0) {
>+if (migrate_use_multifd()) {
>+num = migrate_multifd_channels();
>+}
>+
>+if (qio_net_listener_open_sync(listener, saddr, num, errp) < 0) {
> object_unref(OBJECT(listener));
> return;
> }

My confusion is this function is called at the beginning of the program, which
means we didn't set multifd on or change the multifd channel parameter.

They are the default value at this point.

Am I right?

>-- 
>2.21.0
>

-- 
Wei Yang
Help you, Help me

Re: [PATCH 3/4] block/mirror: support unaligned write in active mirror

[Max Reitz]

On 11.10.19 10:33, Vladimir Sementsov-Ogievskiy wrote:
> 04.10.2019 19:31, Max Reitz wrote:
>> On 12.09.19 17:13, Vladimir Sementsov-Ogievskiy wrote:
>>> Prior 9adc1cb49af8d do_sync_target_write had a bug: it reset aligned-up
>>> region in the dirty bitmap, which means that we may not copy some bytes
>>> and assume them copied, which actually leads to producing corrupted
>>> target.
>>>
>>> So 9adc1cb49af8d forced dirty bitmap granularity to be
>>> request_alignment for mirror-top filter, so we are not working with
>>> unaligned requests. However forcing large alignment obviously decreases
>>> performance of unaligned requests.
>>>
>>> This commit provides another solution for the problem: if unaligned
>>> padding is already dirty, we can safely ignore it, as
>>> 1. It's dirty, it will be copied by mirror_iteration anyway
>>> 2. It's dirty, so skipping it now we don't increase dirtiness of the
>>> bitmap and therefore don't damage "synchronicity" of the
>>> write-blocking mirror.
>>>
>>> If unaligned padding is not dirty, we just write it, no reason to touch
>>> dirty bitmap if we succeed (on failure we'll set the whole region
>>> ofcourse, but we loss "synchronicity" on failure anyway).
>>>
>>> Note: we need to disable dirty_bitmap, otherwise we will not be able to
>>> see in do_sync_target_write bitmap state before current operation. We
>>> may of course check dirty bitmap before the operation in
>>> bdrv_mirror_top_do_write and remember it, but we don't need active
>>> dirty bitmap for write-blocking mirror anyway.
>>>
>>> New code-path is unused until the following commit reverts
>>> 9adc1cb49af8d.
>>>
>>> Suggested-by: Denis V. Lunev 
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>> ---
>>>   block/mirror.c | 39 ++-
>>>   1 file changed, 38 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/block/mirror.c b/block/mirror.c
>>> index d176bf5920..d192f6a96b 100644
>>> --- a/block/mirror.c
>>> +++ b/block/mirror.c
>>> @@ -1204,6 +1204,39 @@ do_sync_target_write(MirrorBlockJob *job, 
>>> MirrorMethod method,
>>>QEMUIOVector *qiov, int flags)
>>>   {
>>>   int ret;
>>> +size_t qiov_offset = 0;
>>> +
>>> +if (!QEMU_IS_ALIGNED(offset, job->granularity) &&
>>> +bdrv_dirty_bitmap_get(job->dirty_bitmap, offset)) {
>>> +/*
>>> + * Dirty unaligned padding
>>> + * 1. It's already dirty, no damage to "actively_synced" if we 
>>> just
>>> + *skip unaligned part.
>>> + * 2. If we copy it, we can't reset corresponding bit in
>>> + *dirty_bitmap as there may be some "dirty" bytes still not
>>> + *copied.
>>> + * So, just ignore it.
>>> + */
>>> +qiov_offset = QEMU_ALIGN_UP(offset, job->granularity) - offset;
>>> +if (bytes <= qiov_offset) {
>>> +/* nothing to do after shrink */
>>> +return;
>>> +}
>>> +offset += qiov_offset;
>>> +bytes -= qiov_offset;
>>> +}
>>> +
>>> +if (!QEMU_IS_ALIGNED(offset + bytes, job->granularity) &&
>>> +bdrv_dirty_bitmap_get(job->dirty_bitmap, offset + bytes - 1))
>>> +{
>>> +uint64_t tail = (offset + bytes) % job->granularity;
>>> +
>>> +if (bytes <= tail) {
>>> +/* nothing to do after shrink */
>>> +return;
>>> +}
>>> +bytes -= tail;
>>> +}
>>>   
>>>   bdrv_reset_dirty_bitmap(job->dirty_bitmap, offset, bytes);
>>>   
>>
>> The bdrv_set_dirty_bitmap() in the error case below needs to use the
>> original offset/bytes, I suppose.
> 
> No, because we shrink tail only if it is already dirty. And we've locked the
> region for in-flight operation, so nobody can clear the bitmap in a meantime.

True.  But wouldn’t it be simpler to understand to just use the original
offsets?

> But still, here is something to do:
> 
> for not-shrinked tails, if any, we should:
> 1. align down for reset
> 2. align up for set on failure

Well, the align up is done automatically, and I think that’s pretty
self-explanatory.

Max

>>
>> Apart from that, looks good to me.
>>
>> Max
>>
> 
> 




signature.asc
Description: OpenPGP digital signature

Re: [PATCH 00/23] iotests: Add and use $SOCK_DIR

[Max Reitz]

On 11.10.19 09:27, Thomas Huth wrote:
> On 10/10/2019 17.24, Max Reitz wrote:
>> Hi,
>>
>> Perhaps the main reason we cannot run important tests such as 041 in CI
>> is that when they care Unix sockets in $TEST_DIR, the path may become
>> too long to connect to them.
>>
>> To get by this problem, this series lets the check script create a new
>> temporary directory (mktemp -d) and then makes the iotests use it for
>> all Unix sockets.
> 
> Thanks a lot for tackling this!
> 
> I gave it a try, and most tests work fine now indeed when I run them in
> a directory with a vry long file name.
> 
> I still get an error with 028 though:

I still don’t know about 055, but for 028 it looks like a race.

We have this:

> _send_qemu_cmd $h "drive_backup disk ${TEST_IMG}.copy" "(qemu)" >/dev/null
> _send_qemu_cmd $h "" "Formatting" | _filter_img_create

But it looks to me like the “Formatting” line comes earlier when the
path is longer.  No, I don’t know.

What I do know is that this looks wrong altogether.  Why would the
(qemu) prompt necessarily appear before the “Formatting” message?  I
think the drive-backup job creates the image before it is guaranteed to
yield.

So I think the solution is to s/(qemu)/Formatting/ in the expected
return value, replace the “"" "Formatting"” line by “"" "(qemu)"”, and
drop the “Formatting” output from the reference output.  (And add an
empty “(qemu)” line to the reference output.)

Max



signature.asc
Description: OpenPGP digital signature

[PATCH v2 2/5] block/mirror: simplify do_sync_target_write

[Vladimir Sementsov-Ogievskiy]

do_sync_target_write is called from bdrv_mirror_top_do_write after
write/discard operation, all inside active_write/active_write_settle
protecting us from mirror iteration. So the whole area is dirty for
sure, no reason to examine dirty bitmap.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
---
 block/mirror.c | 95 +++---
 1 file changed, 28 insertions(+), 67 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index fe984efb90..1ae57a5131 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1181,84 +1181,45 @@ do_sync_target_write(MirrorBlockJob *job, MirrorMethod 
method,
  uint64_t offset, uint64_t bytes,
  QEMUIOVector *qiov, int flags)
 {
-QEMUIOVector target_qiov;
-uint64_t dirty_offset = offset;
-uint64_t dirty_bytes;
-
-if (qiov) {
-qemu_iovec_init(&target_qiov, qiov->niov);
-}
-
-while (true) {
-bool valid_area;
-int ret;
-
-bdrv_dirty_bitmap_lock(job->dirty_bitmap);
-dirty_bytes = MIN(offset + bytes - dirty_offset, INT_MAX);
-valid_area = bdrv_dirty_bitmap_next_dirty_area(job->dirty_bitmap,
-   &dirty_offset,
-   &dirty_bytes);
-if (!valid_area) {
-bdrv_dirty_bitmap_unlock(job->dirty_bitmap);
-break;
-}
+int ret;
 
-bdrv_reset_dirty_bitmap_locked(job->dirty_bitmap,
-   dirty_offset, dirty_bytes);
-bdrv_dirty_bitmap_unlock(job->dirty_bitmap);
+bdrv_reset_dirty_bitmap(job->dirty_bitmap, offset, bytes);
 
-job_progress_increase_remaining(&job->common.job, dirty_bytes);
+job_progress_increase_remaining(&job->common.job, bytes);
 
-assert(dirty_offset - offset <= SIZE_MAX);
-if (qiov) {
-qemu_iovec_reset(&target_qiov);
-qemu_iovec_concat(&target_qiov, qiov,
-  dirty_offset - offset, dirty_bytes);
-}
-
-switch (method) {
-case MIRROR_METHOD_COPY:
-ret = blk_co_pwritev(job->target, dirty_offset, dirty_bytes,
- qiov ? &target_qiov : NULL, flags);
-break;
+switch (method) {
+case MIRROR_METHOD_COPY:
+ret = blk_co_pwritev(job->target, offset, bytes, qiov, flags);
+break;
 
-case MIRROR_METHOD_ZERO:
-assert(!qiov);
-ret = blk_co_pwrite_zeroes(job->target, dirty_offset, dirty_bytes,
-   flags);
-break;
+case MIRROR_METHOD_ZERO:
+assert(!qiov);
+ret = blk_co_pwrite_zeroes(job->target, offset, bytes, flags);
+break;
 
-case MIRROR_METHOD_DISCARD:
-assert(!qiov);
-ret = blk_co_pdiscard(job->target, dirty_offset, dirty_bytes);
-break;
+case MIRROR_METHOD_DISCARD:
+assert(!qiov);
+ret = blk_co_pdiscard(job->target, offset, bytes);
+break;
 
-default:
-abort();
-}
+default:
+abort();
+}
 
-if (ret >= 0) {
-job_progress_update(&job->common.job, dirty_bytes);
-} else {
-BlockErrorAction action;
+if (ret >= 0) {
+job_progress_update(&job->common.job, bytes);
+} else {
+BlockErrorAction action;
 
-bdrv_set_dirty_bitmap(job->dirty_bitmap, dirty_offset, 
dirty_bytes);
-job->actively_synced = false;
+bdrv_set_dirty_bitmap(job->dirty_bitmap, offset, bytes);
+job->actively_synced = false;
 
-action = mirror_error_action(job, false, -ret);
-if (action == BLOCK_ERROR_ACTION_REPORT) {
-if (!job->ret) {
-job->ret = ret;
-}
-break;
+action = mirror_error_action(job, false, -ret);
+if (action == BLOCK_ERROR_ACTION_REPORT) {
+if (!job->ret) {
+job->ret = ret;
 }
 }
-
-dirty_offset += dirty_bytes;
-}
-
-if (qiov) {
-qemu_iovec_destroy(&target_qiov);
 }
 }
 
-- 
2.21.0

[PATCH v2 5/5] Revert "mirror: Only mirror granularity-aligned chunks"

[Vladimir Sementsov-Ogievskiy]

This reverts commit 9adc1cb49af8d4e54f57980b1eed5c0a4b2dafa6.
"mirror: Only mirror granularity-aligned chunks"

Since previous commit unaligned chunks are supported by
do_sync_target_write.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
---
 block/mirror.c | 29 -
 1 file changed, 29 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 1ed1d79ff8..eecb2d5793 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1488,15 +1488,6 @@ static void bdrv_mirror_top_child_perm(BlockDriverState 
*bs, BdrvChild *c,
 *nshared = BLK_PERM_ALL;
 }
 
-static void bdrv_mirror_top_refresh_limits(BlockDriverState *bs, Error **errp)
-{
-MirrorBDSOpaque *s = bs->opaque;
-
-if (s && s->job && s->job->copy_mode == MIRROR_COPY_MODE_WRITE_BLOCKING) {
-bs->bl.request_alignment = s->job->granularity;
-}
-}
-
 /* Dummy node that provides consistent read to its users without requiring it
  * from its backing file and that allows writes on the backing file chain. */
 static BlockDriver bdrv_mirror_top = {
@@ -1509,7 +1500,6 @@ static BlockDriver bdrv_mirror_top = {
 .bdrv_co_block_status   = bdrv_co_block_status_from_backing,
 .bdrv_refresh_filename  = bdrv_mirror_top_refresh_filename,
 .bdrv_child_perm= bdrv_mirror_top_child_perm,
-.bdrv_refresh_limits= bdrv_mirror_top_refresh_limits,
 };
 
 static BlockJob *mirror_start_job(
@@ -1657,25 +1647,6 @@ static BlockJob *mirror_start_job(
 s->should_complete = true;
 }
 
-/*
- * Must be called before we start tracking writes, but after
- *
- * ((MirrorBlockJob *)
- * ((MirrorBDSOpaque *)
- * mirror_top_bs->opaque
- * )->job
- * )->copy_mode
- *
- * has the correct value.
- * (We start tracking writes as of the following
- * bdrv_create_dirty_bitmap() call.)
- */
-bdrv_refresh_limits(mirror_top_bs, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
-goto fail;
-}
-
 s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
 if (!s->dirty_bitmap) {
 goto fail;
-- 
2.21.0

[PATCH v2 1/5] hbitmap: handle set/reset with zero length

[Vladimir Sementsov-Ogievskiy]

Passing zero length to these functions leads to unpredicted results.
Zero-length set/reset may occur in active-mirror, on zero-length write
(which is unlikely, but not guaranteed to never happen).

Let's just do nothing on zero-length request.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 util/hbitmap.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/util/hbitmap.c b/util/hbitmap.c
index fd44c897ab..86b0231046 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -387,6 +387,10 @@ void hbitmap_set(HBitmap *hb, uint64_t start, uint64_t 
count)
 uint64_t first, n;
 uint64_t last = start + count - 1;
 
+if (count == 0) {
+return;
+}
+
 trace_hbitmap_set(hb, start, count,
   start >> hb->granularity, last >> hb->granularity);
 
@@ -477,6 +481,10 @@ void hbitmap_reset(HBitmap *hb, uint64_t start, uint64_t 
count)
 uint64_t first;
 uint64_t last = start + count - 1;
 
+if (count == 0) {
+return;
+}
+
 trace_hbitmap_reset(hb, start, count,
 start >> hb->granularity, last >> hb->granularity);
 
-- 
2.21.0

[PATCH v2 0/5] active-mirror: support unaligned guest operations

[Vladimir Sementsov-Ogievskiy]

Commit 9adc1cb49af8d fixed a bug about unaligned (to dirty bitmap
granularity) guest writes (and discards) by simply requesting
corresponding alignment on mirror-top filter. However forcing large
alignment obviously decreases performance of unaligned requests.

So it's time for a new solution which is in 04. And 05 reverts
9adc1cb49af8d.

v2:
01: new fix (do we need it for stable?)
02,03,05: add Max's r-b
04: fix bitmap handling
improve comments

Vladimir Sementsov-Ogievskiy (5):
  hbitmap: handle set/reset with zero length
  block/mirror: simplify do_sync_target_write
  block/block-backend: add blk_co_pwritev_part
  block/mirror: support unaligned write in active mirror
  Revert "mirror: Only mirror granularity-aligned chunks"

 include/sysemu/block-backend.h |   4 +
 block/block-backend.c  |  17 +++-
 block/mirror.c | 181 -
 util/hbitmap.c |   8 ++
 4 files changed, 114 insertions(+), 96 deletions(-)

-- 
2.21.0

[PATCH] iotests/028: Fix for long $TEST_DIRs

[Max Reitz]

For long test image paths, the order of the "Formatting" line and the
"(qemu)" prompt after a drive_backup HMP command may be reversed.  It is
not well-defined anyway, so just silence everything and wait until we
get a prompt.

Reported-by: Thomas Huth 
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/028 | 6 --
 tests/qemu-iotests/028.out | 1 -
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/028 b/tests/qemu-iotests/028
index 71301ec6e5..da3187719a 100755
--- a/tests/qemu-iotests/028
+++ b/tests/qemu-iotests/028
@@ -119,8 +119,10 @@ fi
 # Silence output since it contains the disk image path and QEMU's readline
 # character echoing makes it very hard to filter the output. Plus, there
 # is no telling how many times the command will repeat before succeeding.
-_send_qemu_cmd $h "drive_backup disk ${TEST_IMG}.copy" "(qemu)" >/dev/null
-_send_qemu_cmd $h "" "Formatting" | _filter_img_create
+# Furthermore, the order of the "Formatting" line and the "(qemu)" prompt
+# is undefined.
+_send_qemu_cmd $h "drive_backup disk ${TEST_IMG}.copy" "Formatting" >/dev/null
+_send_qemu_cmd $h "" "(qemu)" >/dev/null
 qemu_cmd_repeat=20 _send_qemu_cmd $h "info block-jobs" "No active jobs" 
>/dev/null
 _send_qemu_cmd $h "info block-jobs" "No active jobs"
 _send_qemu_cmd $h 'quit' ""
diff --git a/tests/qemu-iotests/028.out b/tests/qemu-iotests/028.out
index 7d54aeb003..37aed84436 100644
--- a/tests/qemu-iotests/028.out
+++ b/tests/qemu-iotests/028.out
@@ -468,7 +468,6 @@ No errors were found on the image.
 
 block-backup
 
-Formatting 'TEST_DIR/t.IMGFMT.copy', fmt=IMGFMT size=4294968832 
backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
 (qemu) info block-jobs
 No active jobs
 === IO: pattern 195
-- 
2.21.0

[PATCH v2 4/5] block/mirror: support unaligned write in active mirror

[Vladimir Sementsov-Ogievskiy]

Prior 9adc1cb49af8d do_sync_target_write had a bug: it reset aligned-up
region in the dirty bitmap, which means that we may not copy some bytes
and assume them copied, which actually leads to producing corrupted
target.

So 9adc1cb49af8d forced dirty bitmap granularity to be
request_alignment for mirror-top filter, so we are not working with
unaligned requests. However forcing large alignment obviously decreases
performance of unaligned requests.

This commit provides another solution for the problem: if unaligned
padding is already dirty, we can safely ignore it, as
1. It's dirty, it will be copied by mirror_iteration anyway
2. It's dirty, so skipping it now we don't increase dirtiness of the
   bitmap and therefore don't damage "synchronicity" of the
   write-blocking mirror.

If unaligned padding is not dirty, we just write it, no reason to touch
dirty bitmap if we succeed (on failure we'll set the whole region
ofcourse, but we loss "synchronicity" on failure anyway).

Note: we need to disable dirty_bitmap, otherwise we will not be able to
see in do_sync_target_write bitmap state before current operation. We
may of course check dirty bitmap before the operation in
bdrv_mirror_top_do_write and remember it, but we don't need active
dirty bitmap for write-blocking mirror anyway.

New code-path is unused until the following commit reverts
9adc1cb49af8d.

Suggested-by: Denis V. Lunev 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/mirror.c | 71 +++---
 1 file changed, 68 insertions(+), 3 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 1ae57a5131..1ed1d79ff8 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1182,14 +1182,67 @@ do_sync_target_write(MirrorBlockJob *job, MirrorMethod 
method,
  QEMUIOVector *qiov, int flags)
 {
 int ret;
+size_t qiov_offset = 0;
+int64_t bitmap_offset, bitmap_end;
 
-bdrv_reset_dirty_bitmap(job->dirty_bitmap, offset, bytes);
+if (!QEMU_IS_ALIGNED(offset, job->granularity) &&
+bdrv_dirty_bitmap_get(job->dirty_bitmap, offset))
+{
+/*
+ * Dirty unaligned padding: ignore it.
+ *
+ * Reasoning:
+ * 1. If we copy it, we can't reset corresponding bit in
+ *dirty_bitmap as there may be some "dirty" bytes still not
+ *copied.
+ * 2. It's already dirty, so skipping it we don't diverge mirror
+ *progress.
+ *
+ * Note, that because of this, guest write may have no contribution
+ * into mirror converge, but that's not bad, as we have background
+ * process of mirroring. If under some bad circumstances (high 
guest
+ * IO load) background process starve, we will not converge anyway,
+ * even if each write will contribute, as guest is not guaranteed 
to
+ * rewrite the whole disk.
+ */
+qiov_offset = QEMU_ALIGN_UP(offset, job->granularity) - offset;
+if (bytes <= qiov_offset) {
+/* nothing to do after shrink */
+return;
+}
+offset += qiov_offset;
+bytes -= qiov_offset;
+}
+
+if (!QEMU_IS_ALIGNED(offset + bytes, job->granularity) &&
+bdrv_dirty_bitmap_get(job->dirty_bitmap, offset + bytes - 1))
+{
+uint64_t tail = (offset + bytes) % job->granularity;
+
+if (bytes <= tail) {
+/* nothing to do after shrink */
+return;
+}
+bytes -= tail;
+}
+
+/*
+ * Tails are either clean or shrunk, so for bitmap resetting
+ * we safely align the range down.
+ */
+bitmap_offset = QEMU_ALIGN_UP(offset, job->granularity);
+bitmap_end = QEMU_ALIGN_DOWN(offset + bytes, job->granularity);
+if (bitmap_offset < bitmap_end) {
+bdrv_reset_dirty_bitmap(job->dirty_bitmap, bitmap_offset,
+bitmap_end - bitmap_offset);
+}
 
 job_progress_increase_remaining(&job->common.job, bytes);
 
 switch (method) {
 case MIRROR_METHOD_COPY:
-ret = blk_co_pwritev(job->target, offset, bytes, qiov, flags);
+ret = blk_co_pwritev_part(job->target, offset, bytes,
+  qiov, qiov_offset, flags);
 break;
 
 case MIRROR_METHOD_ZERO:
@@ -1211,7 +1264,16 @@ do_sync_target_write(MirrorBlockJob *job, MirrorMethod 
method,
 } else {
 BlockErrorAction action;
 
-bdrv_set_dirty_bitmap(job->dirty_bitmap, offset, bytes);
+/*
+ * We failed, so we should mark dirty the whole area, aligned up.
+ * Note that we don't care about shrunk tails if any: they were dirty
+ * at function start, and they must be still dirty, as we've locked
+ * the region for in-flight op.
+ */
+bitmap_offset = QEMU_ALIGN_DOWN(offset, job->granularity);
+bitmap

[PATCH v2 3/5] block/block-backend: add blk_co_pwritev_part

[Vladimir Sementsov-Ogievskiy]

Add blk write function with qiov_offset parameter. It's needed for the
following commit.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
---
 include/sysemu/block-backend.h |  4 
 block/block-backend.c  | 17 +
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 368d53af77..73f2cef7fe 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -121,6 +121,10 @@ void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps 
*ops, void *opaque);
 int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
unsigned int bytes, QEMUIOVector *qiov,
BdrvRequestFlags flags);
+int coroutine_fn blk_co_pwritev_part(BlockBackend *blk, int64_t offset,
+ unsigned int bytes,
+ QEMUIOVector *qiov, size_t qiov_offset,
+ BdrvRequestFlags flags);
 int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
unsigned int bytes, QEMUIOVector *qiov,
BdrvRequestFlags flags);
diff --git a/block/block-backend.c b/block/block-backend.c
index 1c605d5444..b3d00478af 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1176,9 +1176,10 @@ int coroutine_fn blk_co_preadv(BlockBackend *blk, 
int64_t offset,
 return ret;
 }
 
-int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
-unsigned int bytes, QEMUIOVector *qiov,
-BdrvRequestFlags flags)
+int coroutine_fn blk_co_pwritev_part(BlockBackend *blk, int64_t offset,
+ unsigned int bytes,
+ QEMUIOVector *qiov, size_t qiov_offset,
+ BdrvRequestFlags flags)
 {
 int ret;
 BlockDriverState *bs;
@@ -1205,11 +1206,19 @@ int coroutine_fn blk_co_pwritev(BlockBackend *blk, 
int64_t offset,
 flags |= BDRV_REQ_FUA;
 }
 
-ret = bdrv_co_pwritev(blk->root, offset, bytes, qiov, flags);
+ret = bdrv_co_pwritev_part(blk->root, offset, bytes, qiov, qiov_offset,
+   flags);
 bdrv_dec_in_flight(bs);
 return ret;
 }
 
+int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
+unsigned int bytes, QEMUIOVector *qiov,
+BdrvRequestFlags flags)
+{
+return blk_co_pwritev_part(blk, offset, bytes, qiov, 0, flags);
+}
+
 typedef struct BlkRwCo {
 BlockBackend *blk;
 int64_t offset;
-- 
2.21.0

Re: [PATCH 3/4] block/mirror: support unaligned write in active mirror

[Vladimir Sementsov-Ogievskiy]

11.10.2019 11:58, Max Reitz wrote:
> On 11.10.19 10:33, Vladimir Sementsov-Ogievskiy wrote:
>> 04.10.2019 19:31, Max Reitz wrote:
>>> On 12.09.19 17:13, Vladimir Sementsov-Ogievskiy wrote:
 Prior 9adc1cb49af8d do_sync_target_write had a bug: it reset aligned-up
 region in the dirty bitmap, which means that we may not copy some bytes
 and assume them copied, which actually leads to producing corrupted
 target.

 So 9adc1cb49af8d forced dirty bitmap granularity to be
 request_alignment for mirror-top filter, so we are not working with
 unaligned requests. However forcing large alignment obviously decreases
 performance of unaligned requests.

 This commit provides another solution for the problem: if unaligned
 padding is already dirty, we can safely ignore it, as
 1. It's dirty, it will be copied by mirror_iteration anyway
 2. It's dirty, so skipping it now we don't increase dirtiness of the
  bitmap and therefore don't damage "synchronicity" of the
  write-blocking mirror.

 If unaligned padding is not dirty, we just write it, no reason to touch
 dirty bitmap if we succeed (on failure we'll set the whole region
 ofcourse, but we loss "synchronicity" on failure anyway).

 Note: we need to disable dirty_bitmap, otherwise we will not be able to
 see in do_sync_target_write bitmap state before current operation. We
 may of course check dirty bitmap before the operation in
 bdrv_mirror_top_do_write and remember it, but we don't need active
 dirty bitmap for write-blocking mirror anyway.

 New code-path is unused until the following commit reverts
 9adc1cb49af8d.

 Suggested-by: Denis V. Lunev 
 Signed-off-by: Vladimir Sementsov-Ogievskiy 
 ---
block/mirror.c | 39 ++-
1 file changed, 38 insertions(+), 1 deletion(-)

 diff --git a/block/mirror.c b/block/mirror.c
 index d176bf5920..d192f6a96b 100644
 --- a/block/mirror.c
 +++ b/block/mirror.c
 @@ -1204,6 +1204,39 @@ do_sync_target_write(MirrorBlockJob *job, 
 MirrorMethod method,
 QEMUIOVector *qiov, int flags)
{
int ret;
 +size_t qiov_offset = 0;
 +
 +if (!QEMU_IS_ALIGNED(offset, job->granularity) &&
 +bdrv_dirty_bitmap_get(job->dirty_bitmap, offset)) {
 +/*
 + * Dirty unaligned padding
 + * 1. It's already dirty, no damage to "actively_synced" if 
 we just
 + *skip unaligned part.
 + * 2. If we copy it, we can't reset corresponding bit in
 + *dirty_bitmap as there may be some "dirty" bytes still 
 not
 + *copied.
 + * So, just ignore it.
 + */
 +qiov_offset = QEMU_ALIGN_UP(offset, job->granularity) - 
 offset;
 +if (bytes <= qiov_offset) {
 +/* nothing to do after shrink */
 +return;
 +}
 +offset += qiov_offset;
 +bytes -= qiov_offset;
 +}
 +
 +if (!QEMU_IS_ALIGNED(offset + bytes, job->granularity) &&
 +bdrv_dirty_bitmap_get(job->dirty_bitmap, offset + bytes - 1))
 +{
 +uint64_t tail = (offset + bytes) % job->granularity;
 +
 +if (bytes <= tail) {
 +/* nothing to do after shrink */
 +return;
 +}
 +bytes -= tail;
 +}

bdrv_reset_dirty_bitmap(job->dirty_bitmap, offset, bytes);

>>>
>>> The bdrv_set_dirty_bitmap() in the error case below needs to use the
>>> original offset/bytes, I suppose.
>>
>> No, because we shrink tail only if it is already dirty. And we've locked the
>> region for in-flight operation, so nobody can clear the bitmap in a meantime.
> 
> True.  But wouldn’t it be simpler to understand to just use the original
> offsets?
> 
>> But still, here is something to do:
>>
>> for not-shrinked tails, if any, we should:
>> 1. align down for reset
>> 2. align up for set on failure
> 
> Well, the align up is done automatically, and I think that’s pretty
> self-explanatory.

Patch to make hbitmap_reset very strict (assert on analigned except for the
very end of the bitmap, if orig_size is unaligned) is queued by John.
So, I've made explicit alignment in v2.

> 
>>>
>>> Apart from that, looks good to me.
>>>
>>> Max
>>>
>>
>>
> 
> 


-- 
Best regards,
Vladimir

Re: [PATCH 3/4] block/mirror: support unaligned write in active mirror

[Vladimir Sementsov-Ogievskiy]

11.10.2019 11:58, Max Reitz wrote:
> On 11.10.19 10:33, Vladimir Sementsov-Ogievskiy wrote:
>> 04.10.2019 19:31, Max Reitz wrote:
>>> On 12.09.19 17:13, Vladimir Sementsov-Ogievskiy wrote:
 Prior 9adc1cb49af8d do_sync_target_write had a bug: it reset aligned-up
 region in the dirty bitmap, which means that we may not copy some bytes
 and assume them copied, which actually leads to producing corrupted
 target.

 So 9adc1cb49af8d forced dirty bitmap granularity to be
 request_alignment for mirror-top filter, so we are not working with
 unaligned requests. However forcing large alignment obviously decreases
 performance of unaligned requests.

 This commit provides another solution for the problem: if unaligned
 padding is already dirty, we can safely ignore it, as
 1. It's dirty, it will be copied by mirror_iteration anyway
 2. It's dirty, so skipping it now we don't increase dirtiness of the
  bitmap and therefore don't damage "synchronicity" of the
  write-blocking mirror.

 If unaligned padding is not dirty, we just write it, no reason to touch
 dirty bitmap if we succeed (on failure we'll set the whole region
 ofcourse, but we loss "synchronicity" on failure anyway).

 Note: we need to disable dirty_bitmap, otherwise we will not be able to
 see in do_sync_target_write bitmap state before current operation. We
 may of course check dirty bitmap before the operation in
 bdrv_mirror_top_do_write and remember it, but we don't need active
 dirty bitmap for write-blocking mirror anyway.

 New code-path is unused until the following commit reverts
 9adc1cb49af8d.

 Suggested-by: Denis V. Lunev 
 Signed-off-by: Vladimir Sementsov-Ogievskiy 
 ---
block/mirror.c | 39 ++-
1 file changed, 38 insertions(+), 1 deletion(-)

 diff --git a/block/mirror.c b/block/mirror.c
 index d176bf5920..d192f6a96b 100644
 --- a/block/mirror.c
 +++ b/block/mirror.c
 @@ -1204,6 +1204,39 @@ do_sync_target_write(MirrorBlockJob *job, 
 MirrorMethod method,
 QEMUIOVector *qiov, int flags)
{
int ret;
 +size_t qiov_offset = 0;
 +
 +if (!QEMU_IS_ALIGNED(offset, job->granularity) &&
 +bdrv_dirty_bitmap_get(job->dirty_bitmap, offset)) {
 +/*
 + * Dirty unaligned padding
 + * 1. It's already dirty, no damage to "actively_synced" if 
 we just
 + *skip unaligned part.
 + * 2. If we copy it, we can't reset corresponding bit in
 + *dirty_bitmap as there may be some "dirty" bytes still 
 not
 + *copied.
 + * So, just ignore it.
 + */
 +qiov_offset = QEMU_ALIGN_UP(offset, job->granularity) - 
 offset;
 +if (bytes <= qiov_offset) {
 +/* nothing to do after shrink */
 +return;
 +}
 +offset += qiov_offset;
 +bytes -= qiov_offset;
 +}
 +
 +if (!QEMU_IS_ALIGNED(offset + bytes, job->granularity) &&
 +bdrv_dirty_bitmap_get(job->dirty_bitmap, offset + bytes - 1))
 +{
 +uint64_t tail = (offset + bytes) % job->granularity;
 +
 +if (bytes <= tail) {
 +/* nothing to do after shrink */
 +return;
 +}
 +bytes -= tail;
 +}

bdrv_reset_dirty_bitmap(job->dirty_bitmap, offset, bytes);

>>>
>>> The bdrv_set_dirty_bitmap() in the error case below needs to use the
>>> original offset/bytes, I suppose.
>>
>> No, because we shrink tail only if it is already dirty. And we've locked the
>> region for in-flight operation, so nobody can clear the bitmap in a meantime.
> 
> True.  But wouldn’t it be simpler to understand to just use the original
> offsets?

then we need to store them.. And if we do it, I'd prefer to add an assertion 
that
shrunk tails are still dirty instead.

> 
>> But still, here is something to do:
>>
>> for not-shrinked tails, if any, we should:
>> 1. align down for reset
>> 2. align up for set on failure
> 
> Well, the align up is done automatically, and I think that’s pretty
> self-explanatory.
> 
> Max
> 
>>>
>>> Apart from that, looks good to me.
>>>
>>> Max
>>>
>>
>>
> 
> 


-- 
Best regards,
Vladimir

Re: [PATCH v2 1/1] IDE: deprecate ide-drive

[Peter Krempa]

On Thu, Oct 10, 2019 at 14:08:12 -0400, John Snow wrote:
> On 10/10/19 7:54 AM, Peter Krempa wrote:
> > On Thu, Oct 10, 2019 at 13:42:26 +0200, Philippe Mathieu-Daudé wrote:
> >> On 10/10/19 1:26 PM, Peter Krempa wrote:

[...]

> > To be honest, I didn't really review the code nor the documentation.
> > I actually reviewed only the idea itself in the context of integration
> > with libvirt and that's why I didn't go for 'Reviewed-by:'.
> > 
> > The gist of the citation above is that we should stick to well known
> > tags with their well known meanings and I think that considering this a
> > 'review' would be a stretch of the definiton.
> > 
> 
> I wasn't aware that PMM wanted to avoid non-standard tags; I consider
> them to be for human use, but I can change that behavior.
> 
> Peter, I'll change it to an ACK (as suggested by Kevin) is that's OK by you.

Sure! I'll spell it out even for compliance:

ACKed-by: Peter Krempa 


signature.asc
Description: PGP signature

Re: [PATCH] iotests/028: Fix for long $TEST_DIRs

[Thomas Huth]

On 11/10/2019 11.07, Max Reitz wrote:
> For long test image paths, the order of the "Formatting" line and the
> "(qemu)" prompt after a drive_backup HMP command may be reversed.  It is
> not well-defined anyway, so just silence everything and wait until we
> get a prompt.
> 
> Reported-by: Thomas Huth 
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/028 | 6 --
>  tests/qemu-iotests/028.out | 1 -
>  2 files changed, 4 insertions(+), 3 deletions(-)

Thanks, now it works most of the time. But sometimes I now get a new
error - seems there is another race left:

$ ./check -qcow2 028
QEMU  --
"/home/thuth/tmp/qemu-with-very-long-directory-name-01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789/tests/qemu-iotests/../../x86_64-softmmu/qemu-system-x86_64"
-nodefaults -display none -machine accel=qtest
QEMU_IMG  --
"/home/thuth/tmp/qemu-with-very-long-directory-name-01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789/tests/qemu-iotests/../../qemu-img"

QEMU_IO   --
"/home/thuth/tmp/qemu-with-very-long-directory-name-01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789/tests/qemu-iotests/../../qemu-io"
 --cache writeback -f qcow2
QEMU_NBD  --
"/home/thuth/tmp/qemu-with-very-long-directory-name-01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789/tests/qemu-iotests/../../qemu-nbd"

IMGFMT-- qcow2 (compat=1.1)
IMGPROTO  -- file
PLATFORM  -- Linux/x86_64 thuth 4.18.0-80.11.2.el8_0.x86_64
TEST_DIR  --
/home/thuth/tmp/qemu-with-very-long-directory-name-01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789/tests/qemu-iotests/scratch
SOCK_DIR  -- /tmp/tmp.u4ofcBgQHm
SOCKET_SCM_HELPER --
/home/thuth/tmp/qemu-with-very-long-directory-name-01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789/tests/qemu-iotests/socket_scm_helper

028  fail   [11:25:46] [11:25:48]  (last: 0s)output
mismatch (see 028.out.bad)
--- /home/thuth/devel/qemu/tests/qemu-iotests/028.out   2019-10-11
11:24:49.853096709 +0200
+++
/home/thuth/tmp/qemu-with-very-long-directory-name-01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789/tests/qemu-iotests/028.out.bad
2019-10-11 11:25:46.976888134 +0200
@@ -468,266 +468,3 @@

 block-backup

-(qemu) info block-jobs
-No active jobs
-=== IO: pattern 195
-read 512/512 bytes at offset 3221194240
-512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 512/512 bytes at offset 3221195264
-512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 512/512 bytes at offset 3221196288
-512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 512/512 bytes at offset 3221197312
-512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 512/512 bytes at offset 3221198336
-512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 512/512 bytes at offset 3221199360
-512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 512/512 bytes at offset 3221200384
-512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 512/512 bytes at offset 3221201408
-512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 512/512 bytes at offset 3221202432
-512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 512/512 bytes at offset 3221203456
-512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 512/512 bytes at offset 3221204480
-512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 512/512 bytes at offset 3221205504
-512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 512/512 bytes at offset 3221206528
-512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 512/512 bytes at offset 3221207552
-512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 512/512 bytes at offset 3221208576
-512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 512/512 bytes at offset 3221209600
-512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 512/512 bytes at offset 3221210624
-512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 512/512 bytes at offset 3221211648
-512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 512/512 bytes at offset 3221212672
-512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 512/512 bytes at offset 3221213696
-512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 512/512 bytes at offset 3221214720
-512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 512/512 bytes at offset 3221215744
-512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 512/512 bytes at offset 3221216768
-512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 512

Re: [PATCH v2 1/4] qemu-iotests: remove bash shebang from library files

[Kevin Wolf]

Am 09.10.2019 um 21:47 hat Cleber Rosa geschrieben:
> Due to not being able to find a reason to have shebangs on files that
> are not executable.
> 
> While at it, add a mode hint to emacs, which would be clueless or
> plain wrong about these containing shell code.

vim still doesn't like the change.

Of course, we could also add another line for vim and for every other
editor in use, but actually, I think I'd prefer just dropping this
patch. It even makes each file a few bytes larger instead of saving
something. Shebang lines are a shorter and more portable format
indicator than the alternatives.

So I think in the end we have found a good reason to keep them. :-)

Kevin

Re: [PATCH] qcow2-bitmaps: fix qcow2_can_store_new_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

10.10.2019 21:46, John Snow wrote:
> 
> 
> On 10/10/19 11:39 AM, Eric Blake wrote:
>> On 6/7/19 1:53 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> 07.06.2019 21:48, Vladimir Sementsov-Ogievskiy wrote:
 qcow2_can_store_new_dirty_bitmap works wrong, as it considers only
 bitmaps already stored in the qcow2 image and ignores persistent
 BdrvDirtyBitmap objects.

 So, let's instead count persistent BdrvDirtyBitmaps. We load all qcow2
 bitmaps on open, so there should not be any bitmap in the image for
 which we don't have BdrvDirtyBitmaps version. If it is - it's a kind of
 corruption, and no reason to check for corruptions here (open() and
 close() are better places for it).

 Signed-off-by: Vladimir Sementsov-Ogievskiy 
 ---

 Hi!

 Patch is better than discussing I thing, so here is my
 counter-suggestion for
 "[PATCH 0/5] block/dirty-bitmap: check number and size constraints
 against queued bitmaps"
 by John.

 It's of course needs some additional refactoring, as first assert
 shows bad design,
 I just wrote it in such manner to make minimum changes to fix the bug.

>>
 +    assert(!bdrv_find_dirty_bitmap(bs, name));
>>>
>>> exactly bad, this should be checked in qmp_block_dirty_bitmap_add(),
>>> before checks around
>>> persistence. and aio_context_acquire may be dropped..
>>>
>>> But anyway, idea is clear I think, consider it as RFC patch
>>
>> Are you planning to post a v2, as this affects at least
>> https://bugzilla.redhat.com/show_bug.cgi?id=1712636
>>
> 
> I suppose we ought to do it this way for now as it's less SLOC than my
> idea, but I have to admit I would still prefer to get rid of "can_store"
> altogether and provide concrete bitmap_store() and bitmap_remove()
> callbacks for purpose of symmetry and model simplicity.
> 
> I guess I'll worry about that discussion some other time.
> 
> --js
> 

Should I base it on master or on you bitmaps branch? Do we want it for stable?

-- 
Best regards,
Vladimir

Re: [PATCH 1/2] qcow2: Limit total allocation range to INT_MAX

[Philippe Mathieu-Daudé]

On 10/10/19 12:08 PM, Max Reitz wrote:

When the COW areas are included, the size of an allocation can exceed
INT_MAX.  This is kind of limited by handle_alloc() in that it already
caps avail_bytes at INT_MAX, but the number of clusters still reflects
the original length.

This can have all sorts of effects, ranging from the storage layer write
call failing to image corruption.  (If there were no image corruption,
then I suppose there would be data loss because the .cow_end area is
forced to be empty, even though there might be something we need to
COW.)

Fix all of it by limiting nb_clusters so the equivalent number of bytes
will not exceed INT_MAX.

Cc: qemu-sta...@nongnu.org
Signed-off-by: Max Reitz 


Reviewed-by: Philippe Mathieu-Daudé 


---
  block/qcow2-cluster.c | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 8d5fa1539c..8982b7b762 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1330,6 +1330,9 @@ static int handle_alloc(BlockDriverState *bs, uint64_t 
guest_offset,
  nb_clusters = MIN(nb_clusters, s->l2_slice_size - l2_index);
  assert(nb_clusters <= INT_MAX);
  
+/* Limit total allocation byte count to INT_MAX */

+nb_clusters = MIN(nb_clusters, INT_MAX >> s->cluster_bits);
+
  /* Find L2 entry for the first involved cluster */
  ret = get_cluster_table(bs, guest_offset, &l2_slice, &l2_index);
  if (ret < 0) {
@@ -1412,7 +1415,7 @@ static int handle_alloc(BlockDriverState *bs, uint64_t 
guest_offset,
   * request actually writes to (excluding COW at the end)
   */
  uint64_t requested_bytes = *bytes + offset_into_cluster(s, guest_offset);
-int avail_bytes = MIN(INT_MAX, nb_clusters << s->cluster_bits);
+int avail_bytes = nb_clusters << s->cluster_bits;
  int nb_bytes = MIN(requested_bytes, avail_bytes);
  QCowL2Meta *old_m = *m;
  

Re: [PULL 5/5] multifd: Use number of channels as listen backlog

[Juan Quintela]

Wei Yang  wrote:
> On Wed, Sep 04, 2019 at 08:29:15AM +0200, Juan Quintela wrote:
>>Reviewed-by: Daniel P. Berrangé 
>>Signed-off-by: Juan Quintela 
>>---
>> migration/socket.c | 7 ++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>>diff --git a/migration/socket.c b/migration/socket.c
>>index e63f5e1612..97c9efde59 100644
>>--- a/migration/socket.c
>>+++ b/migration/socket.c
>>@@ -178,10 +178,15 @@ static void 
>>socket_start_incoming_migration(SocketAddress *saddr,
>> {
>> QIONetListener *listener = qio_net_listener_new();
>> size_t i;
>>+int num = 1;
>> 
>> qio_net_listener_set_name(listener, "migration-socket-listener");
>> 
>>-if (qio_net_listener_open_sync(listener, saddr, 1, errp) < 0) {
>>+if (migrate_use_multifd()) {
>>+num = migrate_multifd_channels();
>>+}
>>+
>>+if (qio_net_listener_open_sync(listener, saddr, num, errp) < 0) {
>> object_unref(OBJECT(listener));
>> return;
>> }
>
> My confusion is this function is called at the beginning of the program, which
> means we didn't set multifd on or change the multifd channel parameter.
>
> They are the default value at this point.
>
> Am I right?

Hi

good catch!

You are right.  The fix worked for me because I always use on the
command line:

--global migration.multifd-channels=10

or whatever number I want to avoid typing.  I can only see two
solutions:
- increase the number always
- require "defer" when using multifd to be able to setup parameters.

Any other good ideas?

Thanks, Juan.

PD.  I was having problem reproducing this issue because I use the
command line for the parameter.

Re: [PATCH v2 3/4] qemu-iotests: 044: pass is actually a noop, so remove it

[Kevin Wolf]

Am 09.10.2019 um 21:47 hat Cleber Rosa geschrieben:
> Reviewed-by: Eric Blake 
> Signed-off-by: Cleber Rosa 

Reviewed-by: Kevin Wolf 

Re: [PATCH v2 4/4] qemu-iotests: 044: remove inaccurate docstring class description

[Kevin Wolf]

Am 09.10.2019 um 21:47 hat Cleber Rosa geschrieben:
> Reviewed-by: Eric Blake 
> Reviewed-by: John Snow 
> Signed-off-by: Cleber Rosa 

Reviewed-by: Kevin Wolf 

Re: [PATCH v2 2/4] qemu-iotests: remove forceful execution success from library files

[Kevin Wolf]

Am 09.10.2019 um 21:47 hat Cleber Rosa geschrieben:
> Should not be necessary on files that are not executed standalone.
> 
> Reviewed-by: Eric Blake 
> Signed-off-by: Cleber Rosa 

Looks fine for common.filter and common.rc, nobody ever checks their
return value.

common.config is included like this:

if ! . "$source_iotests/common.config"
then
_init_error "failed to source common.config"
fi

So as long as we keep this, don't we want to make sure that it returns
success?

Of course, we never really want to return an error from common.config,
so instead of keeping the final 'true' statement, we might consider
changing its inclusion to not check for errors. The case that
potentially changes is when common.config doesn't exist or isn't
readable, but this isn't supposed to ever happen anyway.

Kevin

Re: [PATCH v2 1/4] qemu-iotests: remove bash shebang from library files

[Nir Soffer]

On Fri, Oct 11, 2019, 12:36 Kevin Wolf  wrote:

> Am 09.10.2019 um 21:47 hat Cleber Rosa geschrieben:
> > Due to not being able to find a reason to have shebangs on files that
> > are not executable.
> >
> > While at it, add a mode hint to emacs, which would be clueless or
> > plain wrong about these containing shell code.
>
> vim still doesn't like the change.
>
> Of course, we could also add another line for vim and for every other
> editor in use, but actually, I think I'd prefer just dropping this
> patch. It even makes each file a few bytes larger instead of saving
> something. Shebang lines are a shorter and more portable format
> indicator than the alternatives.
>
> So I think in the end we have found a good reason to keep them. :-)
>

What about .sh suffix? Should be most portable way.

>

[PATCH v2] iotests/028: Fix for long $TEST_DIRs

[Max Reitz]

For long test image paths, the order of the "Formatting" line and the
"(qemu)" prompt after a drive_backup HMP command may be reversed.  In
fact, the interaction between the prompt and the line may lead to the
"Formatting" to being greppable at all after "read"-ing it (if the
prompt injects an IFS character into the "Formatting" string).

So just wait until we get a prompt.  At that point, the block job must
have been started, so "info block-jobs" will only return "No active
jobs" once it is done.

Reported-by: Thomas Huth 
Signed-off-by: Max Reitz 
---
v2:
- Fix another kind of race...
---
 tests/qemu-iotests/028 | 11 ---
 tests/qemu-iotests/028.out |  1 -
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/028 b/tests/qemu-iotests/028
index 71301ec6e5..bba1ee59ae 100755
--- a/tests/qemu-iotests/028
+++ b/tests/qemu-iotests/028
@@ -119,9 +119,14 @@ fi
 # Silence output since it contains the disk image path and QEMU's readline
 # character echoing makes it very hard to filter the output. Plus, there
 # is no telling how many times the command will repeat before succeeding.
-_send_qemu_cmd $h "drive_backup disk ${TEST_IMG}.copy" "(qemu)" >/dev/null
-_send_qemu_cmd $h "" "Formatting" | _filter_img_create
-qemu_cmd_repeat=20 _send_qemu_cmd $h "info block-jobs" "No active jobs" 
>/dev/null
+# (Note that creating the image results in a "Formatting..." message over
+# stdout, which is the same channel the monitor uses.  We cannot reliably
+# wait for it because the monitor output may interact with it in such a
+# way that _timed_wait_for cannot read it.  However, once the block job is
+# done, we know that the "Formatting..." message must have appeared
+# already, so the output is still deterministic.)
+silent=y _send_qemu_cmd $h "drive_backup disk ${TEST_IMG}.copy" "(qemu)"
+silent=y qemu_cmd_repeat=20 _send_qemu_cmd $h "info block-jobs" "No active 
jobs"
 _send_qemu_cmd $h "info block-jobs" "No active jobs"
 _send_qemu_cmd $h 'quit' ""
 
diff --git a/tests/qemu-iotests/028.out b/tests/qemu-iotests/028.out
index 7d54aeb003..37aed84436 100644
--- a/tests/qemu-iotests/028.out
+++ b/tests/qemu-iotests/028.out
@@ -468,7 +468,6 @@ No errors were found on the image.
 
 block-backup
 
-Formatting 'TEST_DIR/t.IMGFMT.copy', fmt=IMGFMT size=4294968832 
backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
 (qemu) info block-jobs
 No active jobs
 === IO: pattern 195
-- 
2.21.0

Re: [PATCH v2] iotests/028: Fix for long $TEST_DIRs

[Thomas Huth]

On 11/10/2019 14.18, Max Reitz wrote:
> For long test image paths, the order of the "Formatting" line and the
> "(qemu)" prompt after a drive_backup HMP command may be reversed.  In
> fact, the interaction between the prompt and the line may lead to the
> "Formatting" to being greppable at all after "read"-ing it (if the
> prompt injects an IFS character into the "Formatting" string).
> 
> So just wait until we get a prompt.  At that point, the block job must
> have been started, so "info block-jobs" will only return "No active
> jobs" once it is done.
> 
> Reported-by: Thomas Huth 
> Signed-off-by: Max Reitz 
> ---
> v2:
> - Fix another kind of race...

Thanks, that fixes the issue for me!

Tested-by: Thomas Huth 

Re: [PULL 5/5] multifd: Use number of channels as listen backlog

[Wei Yang]

On Fri, Oct 11, 2019 at 12:40:03PM +0200, Juan Quintela wrote:
>Wei Yang  wrote:
>> On Wed, Sep 04, 2019 at 08:29:15AM +0200, Juan Quintela wrote:
>>>Reviewed-by: Daniel P. Berrang?? 
>>>Signed-off-by: Juan Quintela 
>>>---
>>> migration/socket.c | 7 ++-
>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>>diff --git a/migration/socket.c b/migration/socket.c
>>>index e63f5e1612..97c9efde59 100644
>>>--- a/migration/socket.c
>>>+++ b/migration/socket.c
>>>@@ -178,10 +178,15 @@ static void 
>>>socket_start_incoming_migration(SocketAddress *saddr,
>>> {
>>> QIONetListener *listener = qio_net_listener_new();
>>> size_t i;
>>>+int num = 1;
>>> 
>>> qio_net_listener_set_name(listener, "migration-socket-listener");
>>> 
>>>-if (qio_net_listener_open_sync(listener, saddr, 1, errp) < 0) {
>>>+if (migrate_use_multifd()) {
>>>+num = migrate_multifd_channels();
>>>+}
>>>+
>>>+if (qio_net_listener_open_sync(listener, saddr, num, errp) < 0) {
>>> object_unref(OBJECT(listener));
>>> return;
>>> }
>>
>> My confusion is this function is called at the beginning of the program, 
>> which
>> means we didn't set multifd on or change the multifd channel parameter.
>>
>> They are the default value at this point.
>>
>> Am I right?
>
>Hi
>
>good catch!
>
>You are right.  The fix worked for me because I always use on the
>command line:
>
>--global migration.multifd-channels=10
>
>or whatever number I want to avoid typing.  I can only see two
>solutions:
>- increase the number always

You mean change default value? Then which one should we choose?

>- require "defer" when using multifd to be able to setup parameters.
>
>Any other good ideas?

Would you mind explaining more about "defer"? How this works?

>
>Thanks, Juan.
>
>PD.  I was having problem reproducing this issue because I use the
>command line for the parameter.

-- 
Wei Yang
Help you, Help me

Occasional VM soft lockup when a remote cdrom is attached

[Guoheyi]

Hi folks,

We observed Linux on VM occasionally (at very low rate) got soft lockup 
when a remote cdrom is attached. The guest hangs up at below call trace:


[Tue Oct8 23:02:53 2019]ata_scsi_queuecmd+0xe0/0x2a0 [libata]

[Tue Oct8 23:02:53 2019]scsi_dispatch_cmd+0xec/0x288

[Tue Oct8 23:02:53 2019]scsi_queue_rq+0x5f4/0x6b8

[Tue Oct8 23:02:53 2019]blk_mq_dispatch_rq_list+0xb0/0x690

[Tue Oct8 23:02:53 2019]blk_mq_do_dispatch_sched+0x8c/0x130

[Tue Oct8 23:02:53 2019]blk_mq_sched_dispatch_requests+0x128/0x1f0

[Tue Oct8 23:02:53 2019]__blk_mq_run_hw_queue+0x9c/0x128

[Tue Oct8 23:02:53 2019]__blk_mq_delay_run_hw_queue+0x198/0x1d8

[Tue Oct8 23:02:53 2019]blk_mq_run_hw_queue+0x68/0x180

[Tue Oct8 23:02:53 2019]blk_mq_sched_insert_request+0xbc/0x210

[Tue Oct8 23:02:53 2019]blk_execute_rq_nowait+0x118/0x168

[Tue Oct8 23:02:53 2019]blk_execute_rq+0x74/0xd8

[Tue Oct8 23:02:53 2019]__scsi_execute+0xd8/0x1e0

[Tue Oct8 23:02:53 2019]sr_check_events+0xd4/0x2c8 [sr_mod]

[Tue Oct8 23:02:53 2019]cdrom_check_events+0x34/0x50 [cdrom]

[Tue Oct8 23:02:53 2019]sr_block_check_events+0xdc/0x108 [sr_mod]

[Tue Oct8 23:02:53 2019]disk_check_events+0x60/0x198

[Tue Oct8 23:02:53 2019]disk_events_workfn+0x24/0x30

[Tue Oct8 23:02:53 2019]process_one_work+0x1b4/0x3f8

[Tue Oct8 23:02:53 2019]worker_thread+0x54/0x470

[Tue Oct8 23:02:53 2019]kthread+0x134/0x138

[Tue Oct8 23:02:53 2019]ret_from_fork+0x10/0x18


We are running the whole stack on ARM64 platforms, using rcdrom on host 
to connect a remote cdrom, which is appeared as "/dev/sr0" on the host. 
Our Linux kernel version is 4.19.36 and qemu version is 2.8.1, which is 
fairly old but I checked the mainline and found the work flow does not 
change much. And KVM is enabled.


We provide the remote cdrom to guest as a block device, attached under 
ICH SATA bus.



The work flow should be like this (please correct me if I was wrong):

1. There is a kworker thread in guest kernel which will check cdrom 
status periodically.


2. The call of "ata_scsi_queuecmd" in guest will write AHCI port 
register "PORT_CMD_ISSUE", so this VCPU thread is trapped out to qemu.


3. qemu will grab the BQL and then dispatch the access to ahci_port_write().

4. For this is a "get event status notification" command, qemu finally 
goes to cmd_get_event_status_notification() and then cdrom_is_inserted().


5. In cdrom_is_inserted(), an ioctl to cdrom fd is issued.


However, in the last step, we found the ioctl() may have large latency, 
for it is a virtual device of remote cdrom, when the remote server is 
busy and of poor performance. We have observed more than 8 seconds 
latency in half an hour test, and the latency might reach more than 20 
seconds when guest soft lockup occurred.



My question is, is there any way to get around of this issue? Does it 
make sense for qemu to setup an IO thread to issue this ioctl() and let 
the VCPU thread return to guest as soon as possible? Or it is kernel's 
responsibility to break up the long time ioctl() and return to user space?



Any comments or advice will be appreciated :)


HG

[PATCH 2/5] iotests: Test 041 does not work on macOS

[Thomas Huth]

041 works fine on Linux, FreeBSD and OpenBSD, so let's mark it as
only supported on these systems.

Signed-off-by: Thomas Huth 
---
 tests/qemu-iotests/041 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 8568426311..7211f1950a 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -1123,4 +1123,5 @@ class TestOrphanedSource(iotests.QMPTestCase):
 
 if __name__ == '__main__':
 iotests.main(supported_fmts=['qcow2', 'qed'],
- supported_protocols=['file'])
+ supported_protocols=['file'],
+ supported_platforms=['linux', 'freebsd', 'openbsd6'])
-- 
2.18.1

[PATCH 0/5] Enable more iotests during "make check-block"

[Thomas Huth]

As discussed here:

 https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg00697.html

and here:

 https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg01388.html

it would be good to have some more valuable iotests enabled in the
"auto" group to get better iotest coverage during "make check".

And once Max' "iotests: Add and use $SOCK_DIR" patch series has been
merged, we can indeed enable these Python-based tests, too.

There is just one small downside: Since these tests require a QEMU
that features a 'virtio-blk' device, we cannot run the iotests
with binaries like qemu-system-tricore anymore. But since the iotests
were not very useful with such binaries anyway, I think it's ok now
if we skip them there.

Based-on: 20191010152457.17713-1-mre...@redhat.com

John Snow (1):
  iotests: remove 'linux' from default supported platforms

Thomas Huth (4):
  iotests: Test 041 does not work on macOS
  iotests: Test 183 does not work on macOS and OpenBSD
  iotests: Skip "make check-block" if QEMU does not support virtio-blk
  iotests: Enable more tests in the 'auto' group to improve test
coverage

 tests/check-block.sh  | 16 +++-
 tests/qemu-iotests/041|  3 ++-
 tests/qemu-iotests/183|  1 +
 tests/qemu-iotests/group  | 18 +-
 tests/qemu-iotests/iotests.py | 16 +++-
 5 files changed, 38 insertions(+), 16 deletions(-)

-- 
2.18.1

[PATCH 1/5] iotests: remove 'linux' from default supported platforms

[Thomas Huth]

From: John Snow 

verify_platform will check an explicit whitelist and blacklist instead.
The default will now be assumed to be allowed to run anywhere.

For tests that do not specify their platforms explicitly, this has the effect of
enabling these tests on non-linux platforms. For tests that always specified
linux explicitly, there is no change.

For Python tests on FreeBSD at least; only seven python tests fail:
045 147 149 169 194 199 211

045 and 149 appear to be misconfigurations,
147 and 194 are the AF_UNIX path too long error,
169 and 199 are bitmap migration bugs, and
211 is a bug that shows up on Linux platforms, too.

This is at least good evidence that these tests are not Linux-only. If
they aren't suitable for other platforms, they should be disabled on a
per-platform basis as appropriate.

Therefore, let's switch these on and deal with the failures.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/iotests.py | 16 +++-
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 66090b70c1..d7b9a6f3ef 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -844,9 +844,14 @@ def verify_protocol(supported=[], unsupported=[]):
 if not_sup or (imgproto in unsupported):
 notrun('not suitable for this protocol: %s' % imgproto)
 
-def verify_platform(supported_oses=['linux']):
-if True not in [sys.platform.startswith(x) for x in supported_oses]:
-notrun('not suitable for this OS: %s' % sys.platform)
+def verify_platform(supported=None, unsupported=None):
+if unsupported is not None:
+if any((sys.platform.startswith(x) for x in unsupported)):
+notrun('not suitable for this OS: %s' % sys.platform)
+
+if supported is not None:
+if not any((sys.platform.startswith(x) for x in supported)):
+notrun('not suitable for this OS: %s' % sys.platform)
 
 def verify_cache_mode(supported_cache_modes=[]):
 if supported_cache_modes and (cachemode not in supported_cache_modes):
@@ -908,7 +913,8 @@ def execute_unittest(output, verbosity, debug):
 r'Ran \1 tests', output.getvalue()))
 
 def execute_test(test_function=None,
- supported_fmts=[], supported_oses=['linux'],
+ supported_fmts=[],
+ supported_platforms=None,
  supported_cache_modes=[], unsupported_fmts=[],
  supported_protocols=[], unsupported_protocols=[]):
 """Run either unittest or script-style tests."""
@@ -925,7 +931,7 @@ def execute_test(test_function=None,
 verbosity = 1
 verify_image_format(supported_fmts, unsupported_fmts)
 verify_protocol(supported_protocols, unsupported_protocols)
-verify_platform(supported_oses)
+verify_platform(supported=supported_platforms)
 verify_cache_mode(supported_cache_modes)
 
 if debug:
-- 
2.18.1

[PATCH 3/5] iotests: Test 183 does not work on macOS and OpenBSD

[Thomas Huth]

When running 183 in Cirrus-CI on macOS, or with our vm-build-openbsd
target, it fails with an "Timeout waiting for return on handle 0" error.

Let's mark it as supported only on systems where the test is working
fine (i.e. Linux and FreeBSD).

Signed-off-by: Thomas Huth 
---
 tests/qemu-iotests/183 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qemu-iotests/183 b/tests/qemu-iotests/183
index bced83fae0..feeadb74c8 100755
--- a/tests/qemu-iotests/183
+++ b/tests/qemu-iotests/183
@@ -42,6 +42,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.filter
 . ./common.qemu
 
+_supported_os Linux FreeBSD
 _supported_fmt qcow2 raw qed quorum
 _supported_proto file
 
-- 
2.18.1

[PATCH 4/5] iotests: Skip "make check-block" if QEMU does not support virtio-blk

[Thomas Huth]

The next patch is going to add some python-based tests to the "auto"
group, and these tests require virtio-blk to work properly. Running
iotests without virtio-blk likely does not make too much sense anyway,
so instead of adding a check for the availability of virtio-blk to each
and every test (which does not sound very appealing), let's rather add
a check for this at the top level in the check-block.sh script instead
(so that it is possible to run "make check" without the "check-block"
part for qemu-system-tricore for example).

Signed-off-by: Thomas Huth 
---
 tests/check-block.sh | 16 +++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/tests/check-block.sh b/tests/check-block.sh
index 679aedec50..7582347ec2 100755
--- a/tests/check-block.sh
+++ b/tests/check-block.sh
@@ -26,10 +26,24 @@ if grep -q "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null 
; then
 exit 0
 fi
 
-if [ -z "$(find . -name 'qemu-system-*' -print)" ]; then
+if [ -n "$QEMU_PROG" ]; then
+qemu_prog="$QEMU_PROG"
+else
+for binary in *-softmmu/qemu-system-* ; do
+if [ -x "$binary" ]; then
+qemu_prog="$binary"
+break
+fi
+done
+fi
+if [ -z "$qemu_prog" ]; then
 echo "No qemu-system binary available ==> Not running the qemu-iotests."
 exit 0
 fi
+if ! "$qemu_prog" -M none -device help | grep virtio-blk >/dev/null 2>&1 ; then
+echo "$qemu_prog does not support virtio-blk ==> Not running the 
qemu-iotests."
+exit 0
+fi
 
 if ! command -v bash >/dev/null 2>&1 ; then
 echo "bash not available ==> Not running the qemu-iotests."
-- 
2.18.1

[PATCH 5/5] iotests: Enable more tests in the 'auto' group to improve test coverage

[Thomas Huth]

According to Kevin, tests 030, 040 and 041 are among the most valuable
tests that we have, so we should always run them if possible, even if
they take a little bit longer.

According to Max, it would be good to have a test for iothreads and
migration. 127 and 256 seem to be good candidates for iothreads. For
migration, let's enable 091, 181, 183, and 203 (which also tests
iothreads).

Signed-off-by: Thomas Huth 
---
 tests/qemu-iotests/group | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 5805a79d9e..fa0d6143b0 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -51,7 +51,7 @@
 027 rw auto quick
 028 rw backing quick
 029 rw auto quick
-030 rw backing
+030 rw auto backing
 031 rw auto quick
 032 rw auto quick
 033 rw auto quick
@@ -61,8 +61,8 @@
 037 rw auto backing quick
 038 rw auto backing quick
 039 rw auto quick
-040 rw
-041 rw backing
+040 rw auto
+041 rw auto backing
 042 rw auto quick
 043 rw auto backing
 044 rw
@@ -112,7 +112,7 @@
 088 rw quick
 089 rw auto quick
 090 rw auto quick
-091 rw migration
+091 rw auto migration
 092 rw quick
 093 throttle
 094 rw quick
@@ -148,7 +148,7 @@
 124 rw backing
 125 rw
 126 rw auto backing
-127 rw backing quick
+127 rw auto backing quick
 128 rw quick
 129 rw quick
 130 rw auto quick
@@ -197,9 +197,9 @@
 177 rw auto quick
 178 img
 179 rw auto quick
-181 rw migration
+181 rw auto migration
 182 rw quick
-183 rw migration
+183 rw auto migration
 184 rw auto quick
 185 rw
 186 rw auto
@@ -218,7 +218,7 @@
 200 rw
 201 rw migration
 202 rw quick
-203 rw migration
+203 rw auto migration
 204 rw quick
 205 rw quick
 206 rw
@@ -270,7 +270,7 @@
 253 rw quick
 254 rw backing quick
 255 rw quick
-256 rw quick
+256 rw auto quick
 257 rw
 258 rw quick
 262 rw quick migration
-- 
2.18.1

Re: [PATCH v2 04/16] qcow2: Keep unknown extra snapshot data

[Max Reitz]

On 20.08.19 13:42, Max Reitz wrote:
> On 19.08.19 21:23, Eric Blake wrote:
>> On 8/19/19 1:55 PM, Max Reitz wrote:
>>> The qcow2 specification says to ignore unknown extra data fields in
>>> snapshot table entries.  Currently, we discard it whenever we update the
>>> image, which is a bit different from "ignore".
>>>
>>> This patch makes the qcow2 driver keep all unknown extra data fields
>>> when updating an image's snapshot table.
>>>
>>> Signed-off-by: Max Reitz 
>>> ---
>>>  block/qcow2.h  |  5 
>>>  block/qcow2-snapshot.c | 61 +++---
>>>  2 files changed, 56 insertions(+), 10 deletions(-)
>>>
>>
>>> @@ -162,7 +184,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
>>>  sn = s->snapshots + i;
>>>  offset = ROUND_UP(offset, 8);
>>>  offset += sizeof(h);
>>> -offset += sizeof(extra);
>>> +offset += MAX(sizeof(extra), sn->extra_data_size);
>>
>> Why would we ever have less than sizeof(extra) bytes to write on output,
>> since we always produce the fields on creation and synthesize the
>> missing fields of extra on read?  Can't you rewrite this as:
>>
>> assert(sn->extra_data_size >= sizeof(extra));
>> offset += sn->extra_data_size;
> 
> Hm, but I don’t prop up extra_data_size to be at least sizeof(extra).  I
> can do that, but it would add a few extra lines here and there.

On second thought, it doesn’t work at all because then there is no way
to later detect whether the image is compliant or not.

Max



signature.asc
Description: OpenPGP digital signature

[PATCH v3 04/16] qcow2: Keep unknown extra snapshot data

[Max Reitz]

The qcow2 specification says to ignore unknown extra data fields in
snapshot table entries.  Currently, we discard it whenever we update the
image, which is a bit different from "ignore".

This patch makes the qcow2 driver keep all unknown extra data fields
when updating an image's snapshot table.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 block/qcow2.h  |  5 
 block/qcow2-snapshot.c | 61 +++---
 2 files changed, 56 insertions(+), 10 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 8da1ad11d9..61e5d4ba94 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -61,6 +61,9 @@
  * space for snapshot names and IDs */
 #define QCOW_MAX_SNAPSHOTS_SIZE (1024 * QCOW_MAX_SNAPSHOTS)
 
+/* Maximum amount of extra data per snapshot table entry to accept */
+#define QCOW_MAX_SNAPSHOT_EXTRA_DATA 1024
+
 /* Bitmap header extension constraints */
 #define QCOW2_MAX_BITMAPS 65535
 #define QCOW2_MAX_BITMAP_DIRECTORY_SIZE (1024 * QCOW2_MAX_BITMAPS)
@@ -181,6 +184,8 @@ typedef struct QCowSnapshot {
 uint32_t date_sec;
 uint32_t date_nsec;
 uint64_t vm_clock_nsec;
+uint32_t extra_data_size;
+void *unknown_extra_data; /* Extra data past QCowSnapshotExtraData */
 } QCowSnapshot;
 
 struct Qcow2Cache;
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 80d32e4504..120cb7fa09 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -37,6 +37,7 @@ void qcow2_free_snapshots(BlockDriverState *bs)
 for(i = 0; i < s->nb_snapshots; i++) {
 g_free(s->snapshots[i].name);
 g_free(s->snapshots[i].id_str);
+g_free(s->snapshots[i].unknown_extra_data);
 }
 g_free(s->snapshots);
 s->snapshots = NULL;
@@ -51,7 +52,6 @@ int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
 QCowSnapshot *sn;
 int i, id_str_size, name_size;
 int64_t offset;
-uint32_t extra_data_size;
 int ret;
 
 if (!s->nb_snapshots) {
@@ -80,31 +80,53 @@ int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
 sn->date_sec = be32_to_cpu(h.date_sec);
 sn->date_nsec = be32_to_cpu(h.date_nsec);
 sn->vm_clock_nsec = be64_to_cpu(h.vm_clock_nsec);
-extra_data_size = be32_to_cpu(h.extra_data_size);
+sn->extra_data_size = be32_to_cpu(h.extra_data_size);
 
 id_str_size = be16_to_cpu(h.id_str_size);
 name_size = be16_to_cpu(h.name_size);
 
-/* Read extra data */
+if (sn->extra_data_size > QCOW_MAX_SNAPSHOT_EXTRA_DATA) {
+ret = -EFBIG;
+error_setg(errp, "Too much extra metadata in snapshot table "
+   "entry %i", i);
+goto fail;
+}
+
+/* Read known extra data */
 ret = bdrv_pread(bs->file, offset, &extra,
- MIN(sizeof(extra), extra_data_size));
+ MIN(sizeof(extra), sn->extra_data_size));
 if (ret < 0) {
 error_setg_errno(errp, -ret, "Failed to read snapshot table");
 goto fail;
 }
-offset += extra_data_size;
+offset += MIN(sizeof(extra), sn->extra_data_size);
 
-if (extra_data_size >= endof(QCowSnapshotExtraData,
- vm_state_size_large)) {
+if (sn->extra_data_size >= endof(QCowSnapshotExtraData,
+ vm_state_size_large)) {
 sn->vm_state_size = be64_to_cpu(extra.vm_state_size_large);
 }
 
-if (extra_data_size >= endof(QCowSnapshotExtraData, disk_size)) {
+if (sn->extra_data_size >= endof(QCowSnapshotExtraData, disk_size)) {
 sn->disk_size = be64_to_cpu(extra.disk_size);
 } else {
 sn->disk_size = bs->total_sectors * BDRV_SECTOR_SIZE;
 }
 
+if (sn->extra_data_size > sizeof(extra)) {
+/* Store unknown extra data */
+size_t unknown_extra_data_size =
+sn->extra_data_size - sizeof(extra);
+
+sn->unknown_extra_data = g_malloc(unknown_extra_data_size);
+ret = bdrv_pread(bs->file, offset, sn->unknown_extra_data,
+ unknown_extra_data_size);
+if (ret < 0) {
+error_setg_errno(errp, -ret, "Failed to read snapshot table");
+goto fail;
+}
+offset += unknown_extra_data_size;
+}
+
 /* Read snapshot ID */
 sn->id_str = g_malloc(id_str_size + 1);
 ret = bdrv_pread(bs->file, offset, sn->id_str, id_str_size);
@@ -162,7 +184,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
 sn = s->snapshots + i;
 offset = ROUND_UP(offset, 8);
 offset += sizeof(h);
-offset += sizeof(extra);
+offset += MAX(sizeof(extra), sn->extra_data_size);
 offset += strlen(sn->id_str);
 offset += strlen(sn->name);
 
@@ -209,7 +231,8 @@ static int qcow2_write_snapshots(BlockDrive

[PATCH v3 03/16] qcow2: Add Error ** to qcow2_read_snapshots()

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 block/qcow2.h  | 2 +-
 block/qcow2-snapshot.c | 7 ++-
 block/qcow2.c  | 3 +--
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index f51f478e34..8da1ad11d9 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -708,7 +708,7 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs,
 Error **errp);
 
 void qcow2_free_snapshots(BlockDriverState *bs);
-int qcow2_read_snapshots(BlockDriverState *bs);
+int qcow2_read_snapshots(BlockDriverState *bs, Error **errp);
 
 /* qcow2-cache.c functions */
 Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables,
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 752883e5c3..80d32e4504 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -43,7 +43,7 @@ void qcow2_free_snapshots(BlockDriverState *bs)
 s->nb_snapshots = 0;
 }
 
-int qcow2_read_snapshots(BlockDriverState *bs)
+int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
 {
 BDRVQcow2State *s = bs->opaque;
 QCowSnapshotHeader h;
@@ -68,6 +68,7 @@ int qcow2_read_snapshots(BlockDriverState *bs)
 offset = ROUND_UP(offset, 8);
 ret = bdrv_pread(bs->file, offset, &h, sizeof(h));
 if (ret < 0) {
+error_setg_errno(errp, -ret, "Failed to read snapshot table");
 goto fail;
 }
 
@@ -88,6 +89,7 @@ int qcow2_read_snapshots(BlockDriverState *bs)
 ret = bdrv_pread(bs->file, offset, &extra,
  MIN(sizeof(extra), extra_data_size));
 if (ret < 0) {
+error_setg_errno(errp, -ret, "Failed to read snapshot table");
 goto fail;
 }
 offset += extra_data_size;
@@ -107,6 +109,7 @@ int qcow2_read_snapshots(BlockDriverState *bs)
 sn->id_str = g_malloc(id_str_size + 1);
 ret = bdrv_pread(bs->file, offset, sn->id_str, id_str_size);
 if (ret < 0) {
+error_setg_errno(errp, -ret, "Failed to read snapshot table");
 goto fail;
 }
 offset += id_str_size;
@@ -116,6 +119,7 @@ int qcow2_read_snapshots(BlockDriverState *bs)
 sn->name = g_malloc(name_size + 1);
 ret = bdrv_pread(bs->file, offset, sn->name, name_size);
 if (ret < 0) {
+error_setg_errno(errp, -ret, "Failed to read snapshot table");
 goto fail;
 }
 offset += name_size;
@@ -123,6 +127,7 @@ int qcow2_read_snapshots(BlockDriverState *bs)
 
 if (offset - s->snapshots_offset > QCOW_MAX_SNAPSHOTS_SIZE) {
 ret = -EFBIG;
+error_setg(errp, "Snapshot table is too big");
 goto fail;
 }
 }
diff --git a/block/qcow2.c b/block/qcow2.c
index 7961c05783..d0135912d0 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1584,9 +1584,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
*bs, QDict *options,
 s->snapshots_offset = header.snapshots_offset;
 s->nb_snapshots = header.nb_snapshots;
 
-ret = qcow2_read_snapshots(bs);
+ret = qcow2_read_snapshots(bs, errp);
 if (ret < 0) {
-error_setg_errno(errp, -ret, "Could not read snapshots");
 goto fail;
 }
 
-- 
2.21.0

[PATCH v3 00/16] qcow2: Let check -r all repair some snapshot bits

[Max Reitz]

Hi,

The v1 cover letter explained this series’s purpose:
https://lists.nongnu.org/archive/html/qemu-block/2019-07/msg01290.html

The v2 cover letter explained the v2 changes:
https://lists.nongnu.org/archive/html/qemu-block/2019-08/msg00921.html


The only changes in v3 are:
- Patches 12 and 13: Added notes to the commit messages why it’s OK to
 not let the user choose which snapshots to drop

(I wanted to also address Eric’s idea of letting
QCowSnapshot.extra_data_size be always >= sizeof(QCowSnapshotExtraData)
and thus save the MAX(sizeof(extra), sn->extra_data_size) in
qcow2_write_snapshots(), but that doesn’t really work, because then we
have no way of knowing later whether the image is compliant and thus
needs fixing or not.  He gave me an R-b anyway, so I guess it’s fine.
O:-))

This series now has R-bs from Eric on all patches.  I’m only posting it
because I felt a bit bad about just taking the series as-is and add the
commit notes to 12 and 13 while applying it.
(Also, maybe there is someone who saw me have a bit of discussion with
Eric and thus assumed I would definitely send a v3 that they could then
review.)


Max Reitz (16):
  include: Move endof() up from hw/virtio/virtio.h
  qcow2: Use endof()
  qcow2: Add Error ** to qcow2_read_snapshots()
  qcow2: Keep unknown extra snapshot data
  qcow2: Make qcow2_write_snapshots() public
  qcow2: Put qcow2_upgrade() into its own function
  qcow2: Write v3-compliant snapshot list on upgrade
  qcow2: Separate qcow2_check_read_snapshot_table()
  qcow2: Add qcow2_check_fix_snapshot_table()
  qcow2: Fix broken snapshot table entries
  qcow2: Keep track of the snapshot table length
  qcow2: Fix overly long snapshot tables
  qcow2: Repair snapshot table with too many entries
  qcow2: Fix v3 snapshot table entry compliancy
  iotests: Add peek_file* functions
  iotests: Test qcow2's snapshot table handling

 block/qcow2.h|  15 +-
 include/hw/virtio/virtio.h   |   7 -
 include/qemu/compiler.h  |   7 +
 block/qcow2-snapshot.c   | 323 +++--
 block/qcow2.c| 155 +--
 hw/block/virtio-blk.c|   4 +-
 hw/net/virtio-net.c  |  10 +-
 tests/qemu-iotests/261   | 523 +++
 tests/qemu-iotests/261.out   | 346 +++
 tests/qemu-iotests/common.rc |  20 ++
 tests/qemu-iotests/group |   1 +
 11 files changed, 1354 insertions(+), 57 deletions(-)
 create mode 100755 tests/qemu-iotests/261
 create mode 100644 tests/qemu-iotests/261.out

-- 
2.21.0

[PATCH v3 01/16] include: Move endof() up from hw/virtio/virtio.h

[Max Reitz]

endof() is a useful macro, we can make use of it outside of virtio.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 include/hw/virtio/virtio.h |  7 ---
 include/qemu/compiler.h|  7 +++
 hw/block/virtio-blk.c  |  4 ++--
 hw/net/virtio-net.c| 10 +-
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 48e8d04ff6..ef083af550 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -37,13 +37,6 @@ static inline hwaddr vring_align(hwaddr addr,
 return QEMU_ALIGN_UP(addr, align);
 }
 
-/*
- * Calculate the number of bytes up to and including the given 'field' of
- * 'container'.
- */
-#define virtio_endof(container, field) \
-(offsetof(container, field) + sizeof_field(container, field))
-
 typedef struct VirtIOFeature {
 uint64_t flags;
 size_t end;
diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index 7b93c73340..85c02c16d3 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -60,6 +60,13 @@
 
 #define sizeof_field(type, field) sizeof(((type *)0)->field)
 
+/*
+ * Calculate the number of bytes up to and including the given 'field' of
+ * 'container'.
+ */
+#define endof(container, field) \
+(offsetof(container, field) + sizeof_field(container, field))
+
 /* Convert from a base type to a parent type, with compile time checking.  */
 #ifdef __GNUC__
 #define DO_UPCAST(type, field, dev) ( __extension__ ( { \
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index ed2ddebd2b..ac857edc73 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -42,9 +42,9 @@
  */
 static VirtIOFeature feature_sizes[] = {
 {.flags = 1ULL << VIRTIO_BLK_F_DISCARD,
- .end = virtio_endof(struct virtio_blk_config, discard_sector_alignment)},
+ .end = endof(struct virtio_blk_config, discard_sector_alignment)},
 {.flags = 1ULL << VIRTIO_BLK_F_WRITE_ZEROES,
- .end = virtio_endof(struct virtio_blk_config, write_zeroes_may_unmap)},
+ .end = endof(struct virtio_blk_config, write_zeroes_may_unmap)},
 {}
 };
 
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 9f11422337..2c4909c5f9 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -90,15 +90,15 @@ static inline __virtio16 *virtio_net_rsc_ext_num_dupacks(
 
 static VirtIOFeature feature_sizes[] = {
 {.flags = 1ULL << VIRTIO_NET_F_MAC,
- .end = virtio_endof(struct virtio_net_config, mac)},
+ .end = endof(struct virtio_net_config, mac)},
 {.flags = 1ULL << VIRTIO_NET_F_STATUS,
- .end = virtio_endof(struct virtio_net_config, status)},
+ .end = endof(struct virtio_net_config, status)},
 {.flags = 1ULL << VIRTIO_NET_F_MQ,
- .end = virtio_endof(struct virtio_net_config, max_virtqueue_pairs)},
+ .end = endof(struct virtio_net_config, max_virtqueue_pairs)},
 {.flags = 1ULL << VIRTIO_NET_F_MTU,
- .end = virtio_endof(struct virtio_net_config, mtu)},
+ .end = endof(struct virtio_net_config, mtu)},
 {.flags = 1ULL << VIRTIO_NET_F_SPEED_DUPLEX,
- .end = virtio_endof(struct virtio_net_config, duplex)},
+ .end = endof(struct virtio_net_config, duplex)},
 {}
 };
 
-- 
2.21.0

[PATCH v3 09/16] qcow2: Add qcow2_check_fix_snapshot_table()

[Max Reitz]

qcow2_check_read_snapshot_table() can perform consistency checks, but it
cannot fix everything.  Specifically, it cannot allocate new clusters,
because that should wait until the refcount structures are known to be
consistent (i.e., after qcow2_check_refcounts()).  Thus, it cannot call
qcow2_write_snapshots().

Do that in qcow2_check_fix_snapshot_table(), which is called after
qcow2_check_refcounts().

Currently, there is nothing that would set result->corruptions, so this
is a no-op.  A follow-up patch will change that.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 block/qcow2.h  |  3 +++
 block/qcow2-snapshot.c | 25 +
 block/qcow2.c  |  9 -
 3 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 6c2a38c98d..b6125899b8 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -719,6 +719,9 @@ int qcow2_write_snapshots(BlockDriverState *bs);
 int coroutine_fn qcow2_check_read_snapshot_table(BlockDriverState *bs,
  BdrvCheckResult *result,
  BdrvCheckMode fix);
+int coroutine_fn qcow2_check_fix_snapshot_table(BlockDriverState *bs,
+BdrvCheckResult *result,
+BdrvCheckMode fix);
 
 /* qcow2-cache.c functions */
 Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables,
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index d667bfd935..b526a8f819 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -380,6 +380,31 @@ int coroutine_fn 
qcow2_check_read_snapshot_table(BlockDriverState *bs,
 return 0;
 }
 
+int coroutine_fn qcow2_check_fix_snapshot_table(BlockDriverState *bs,
+BdrvCheckResult *result,
+BdrvCheckMode fix)
+{
+BDRVQcow2State *s = bs->opaque;
+int ret;
+
+if (result->corruptions && (fix & BDRV_FIX_ERRORS)) {
+qemu_co_mutex_unlock(&s->lock);
+ret = qcow2_write_snapshots(bs);
+qemu_co_mutex_lock(&s->lock);
+if (ret < 0) {
+result->check_errors++;
+fprintf(stderr, "ERROR failed to update snapshot table: %s\n",
+strerror(-ret));
+return ret;
+}
+
+result->corruptions_fixed += result->corruptions;
+result->corruptions = 0;
+}
+
+return 0;
+}
+
 static void find_new_snapshot_id(BlockDriverState *bs,
  char *id_str, int id_str_size)
 {
diff --git a/block/qcow2.c b/block/qcow2.c
index 7f47444ab7..c8b3726dff 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -597,13 +597,20 @@ static int coroutine_fn 
qcow2_co_check_locked(BlockDriverState *bs,
 memset(result, 0, sizeof(*result));
 
 ret = qcow2_check_read_snapshot_table(bs, &snapshot_res, fix);
-qcow2_add_check_result(result, &snapshot_res, false);
 if (ret < 0) {
+qcow2_add_check_result(result, &snapshot_res, false);
 return ret;
 }
 
 ret = qcow2_check_refcounts(bs, &refcount_res, fix);
 qcow2_add_check_result(result, &refcount_res, true);
+if (ret < 0) {
+qcow2_add_check_result(result, &snapshot_res, false);
+return ret;
+}
+
+ret = qcow2_check_fix_snapshot_table(bs, &snapshot_res, fix);
+qcow2_add_check_result(result, &snapshot_res, false);
 if (ret < 0) {
 return ret;
 }
-- 
2.21.0

[PATCH v3 02/16] qcow2: Use endof()

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 block/qcow2-snapshot.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index d0e7fa9311..752883e5c3 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -92,11 +92,12 @@ int qcow2_read_snapshots(BlockDriverState *bs)
 }
 offset += extra_data_size;
 
-if (extra_data_size >= 8) {
+if (extra_data_size >= endof(QCowSnapshotExtraData,
+ vm_state_size_large)) {
 sn->vm_state_size = be64_to_cpu(extra.vm_state_size_large);
 }
 
-if (extra_data_size >= 16) {
+if (extra_data_size >= endof(QCowSnapshotExtraData, disk_size)) {
 sn->disk_size = be64_to_cpu(extra.disk_size);
 } else {
 sn->disk_size = bs->total_sectors * BDRV_SECTOR_SIZE;
@@ -251,7 +252,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
 }
 
 QEMU_BUILD_BUG_ON(offsetof(QCowHeader, snapshots_offset) !=
-offsetof(QCowHeader, nb_snapshots) + sizeof(header_data.nb_snapshots));
+  endof(QCowHeader, nb_snapshots));
 
 header_data.nb_snapshots= cpu_to_be32(s->nb_snapshots);
 header_data.snapshots_offset= cpu_to_be64(snapshots_offset);
-- 
2.21.0

[PATCH v3 05/16] qcow2: Make qcow2_write_snapshots() public

[Max Reitz]

Updating the snapshot list will be useful when upgrading a v2 image to
v3, so we will need to call this function in qcow2.c.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 block/qcow2.h  | 1 +
 block/qcow2-snapshot.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 61e5d4ba94..02d629a3d8 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -714,6 +714,7 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs,
 
 void qcow2_free_snapshots(BlockDriverState *bs);
 int qcow2_read_snapshots(BlockDriverState *bs, Error **errp);
+int qcow2_write_snapshots(BlockDriverState *bs);
 
 /* qcow2-cache.c functions */
 Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables,
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 120cb7fa09..e3bf4c9776 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -164,7 +164,7 @@ fail:
 }
 
 /* add at the end of the file a new list of snapshots */
-static int qcow2_write_snapshots(BlockDriverState *bs)
+int qcow2_write_snapshots(BlockDriverState *bs)
 {
 BDRVQcow2State *s = bs->opaque;
 QCowSnapshot *sn;
-- 
2.21.0

[PATCH v3 12/16] qcow2: Fix overly long snapshot tables

[Max Reitz]

We currently refuse to open qcow2 images with overly long snapshot
tables.  This patch makes qemu-img check -r all drop all offending
entries past what we deem acceptable.

The user cannot choose which snapshots are removed.  This is fine
because we have chosen the maximum snapshot table size to be so large
(64 MB) that it cannot be reasonably reached.  If the snapshot table
exceeds this size, the image has probably been corrupted in some way; in
this case, it is most important to just make the image usable such that
the user can copy off at least the active layer.
(Also note that the snapshots will be removed only with "-r all", so a
plain "check" or "check -r leaks" will not delete any data.)

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 block/qcow2-snapshot.c | 88 +-
 1 file changed, 78 insertions(+), 10 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 582eb3386a..366d9f574c 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -29,15 +29,24 @@
 #include "qemu/error-report.h"
 #include "qemu/cutils.h"
 
+static void qcow2_free_single_snapshot(BlockDriverState *bs, int i)
+{
+BDRVQcow2State *s = bs->opaque;
+
+assert(i >= 0 && i < s->nb_snapshots);
+g_free(s->snapshots[i].name);
+g_free(s->snapshots[i].id_str);
+g_free(s->snapshots[i].unknown_extra_data);
+memset(&s->snapshots[i], 0, sizeof(s->snapshots[i]));
+}
+
 void qcow2_free_snapshots(BlockDriverState *bs)
 {
 BDRVQcow2State *s = bs->opaque;
 int i;
 
 for(i = 0; i < s->nb_snapshots; i++) {
-g_free(s->snapshots[i].name);
-g_free(s->snapshots[i].id_str);
-g_free(s->snapshots[i].unknown_extra_data);
+qcow2_free_single_snapshot(bs, i);
 }
 g_free(s->snapshots);
 s->snapshots = NULL;
@@ -48,6 +57,14 @@ void qcow2_free_snapshots(BlockDriverState *bs)
  * If @repair is true, try to repair a broken snapshot table instead
  * of just returning an error:
  *
+ * - If the snapshot table was too long, set *nb_clusters_reduced to
+ *   the number of snapshots removed off the end.
+ *   The caller will update the on-disk nb_snapshots accordingly;
+ *   this leaks clusters, but is safe.
+ *   (The on-disk information must be updated before
+ *   qcow2_check_refcounts(), because that function relies on
+ *   s->nb_snapshots to reflect the on-disk value.)
+ *
  * - If there were snapshots with too much extra metadata, increment
  *   *extra_data_dropped for each.
  *   This requires the caller to eventually rewrite the whole snapshot
@@ -59,6 +76,7 @@ void qcow2_free_snapshots(BlockDriverState *bs)
  *   extra data.)
  */
 static int qcow2_do_read_snapshots(BlockDriverState *bs, bool repair,
+   int *nb_clusters_reduced,
int *extra_data_dropped,
Error **errp)
 {
@@ -67,7 +85,7 @@ static int qcow2_do_read_snapshots(BlockDriverState *bs, bool 
repair,
 QCowSnapshotExtraData extra;
 QCowSnapshot *sn;
 int i, id_str_size, name_size;
-int64_t offset;
+int64_t offset, pre_sn_offset;
 uint64_t table_length = 0;
 int ret;
 
@@ -83,6 +101,7 @@ static int qcow2_do_read_snapshots(BlockDriverState *bs, 
bool repair,
 for(i = 0; i < s->nb_snapshots; i++) {
 bool truncate_unknown_extra_data = false;
 
+pre_sn_offset = offset;
 table_length = ROUND_UP(table_length, 8);
 
 /* Read statically sized part of the snapshot header */
@@ -197,9 +216,31 @@ static int qcow2_do_read_snapshots(BlockDriverState *bs, 
bool repair,
 if (table_length > QCOW_MAX_SNAPSHOTS_SIZE ||
 offset - s->snapshots_offset > INT_MAX)
 {
-ret = -EFBIG;
-error_setg(errp, "Snapshot table is too big");
-goto fail;
+if (!repair) {
+ret = -EFBIG;
+error_setg(errp, "Snapshot table is too big");
+error_append_hint(errp, "You can force-remove all %u "
+  "overhanging snapshots with qemu-img check "
+  "-r all\n", s->nb_snapshots - i);
+goto fail;
+}
+
+fprintf(stderr, "Discarding %u overhanging snapshots (snapshot "
+"table is too big)\n", s->nb_snapshots - i);
+
+*nb_clusters_reduced += (s->nb_snapshots - i);
+
+/* Discard current snapshot also */
+qcow2_free_single_snapshot(bs, i);
+
+/*
+ * This leaks all the rest of the snapshot table and the
+ * snapshots' clusters, but we run in check -r all mode,
+ * so qcow2_check_refcounts() will take care of it.
+ */
+s->nb_snapshots = i;
+offset = pre_sn_offset;
+break;
 }
 }
 
@@ -214,7 +255,7 @@ fail:
 
 int qcow2_read_snapshots(BlockDriverState *bs

[PATCH v3 06/16] qcow2: Put qcow2_upgrade() into its own function

[Max Reitz]

This does not make sense right now, but it will make sense once we need
to do more than to just update s->qcow_version.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 block/qcow2.c | 43 ++-
 1 file changed, 38 insertions(+), 5 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index d0135912d0..d43064dca2 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4897,12 +4897,46 @@ static int qcow2_downgrade(BlockDriverState *bs, int 
target_version,
 return 0;
 }
 
+/*
+ * Upgrades an image's version.  While newer versions encompass all
+ * features of older versions, some things may have to be presented
+ * differently.
+ */
+static int qcow2_upgrade(BlockDriverState *bs, int target_version,
+ BlockDriverAmendStatusCB *status_cb, void *cb_opaque,
+ Error **errp)
+{
+BDRVQcow2State *s = bs->opaque;
+int current_version = s->qcow_version;
+int ret;
+
+/* This is qcow2_upgrade(), not qcow2_downgrade() */
+assert(target_version > current_version);
+
+/* There are no other versions (yet) that you can upgrade to */
+assert(target_version == 3);
+
+status_cb(bs, 0, 1, cb_opaque);
+
+s->qcow_version = target_version;
+ret = qcow2_update_header(bs);
+if (ret < 0) {
+s->qcow_version = current_version;
+error_setg_errno(errp, -ret, "Failed to update the image header");
+return ret;
+}
+status_cb(bs, 1, 1, cb_opaque);
+
+return 0;
+}
+
 typedef enum Qcow2AmendOperation {
 /* This is the value Qcow2AmendHelperCBInfo::last_operation will be
  * statically initialized to so that the helper CB can discern the first
  * invocation from an operation change */
 QCOW2_NO_OPERATION = 0,
 
+QCOW2_UPGRADING,
 QCOW2_CHANGING_REFCOUNT_ORDER,
 QCOW2_DOWNGRADING,
 } Qcow2AmendOperation;
@@ -5085,17 +5119,16 @@ static int qcow2_amend_options(BlockDriverState *bs, 
QemuOpts *opts,
 helper_cb_info = (Qcow2AmendHelperCBInfo){
 .original_status_cb = status_cb,
 .original_cb_opaque = cb_opaque,
-.total_operations = (new_version < old_version)
+.total_operations = (new_version != old_version)
   + (s->refcount_bits != refcount_bits)
 };
 
 /* Upgrade first (some features may require compat=1.1) */
 if (new_version > old_version) {
-s->qcow_version = new_version;
-ret = qcow2_update_header(bs);
+helper_cb_info.current_operation = QCOW2_UPGRADING;
+ret = qcow2_upgrade(bs, new_version, &qcow2_amend_helper_cb,
+&helper_cb_info, errp);
 if (ret < 0) {
-s->qcow_version = old_version;
-error_setg_errno(errp, -ret, "Failed to update the image header");
 return ret;
 }
 }
-- 
2.21.0

[PATCH v3 10/16] qcow2: Fix broken snapshot table entries

[Max Reitz]

The only case where we currently reject snapshot table entries is when
they have too much extra data.  Fix them with qemu-img check -r all by
counting it as a corruption, reducing their extra_data_size, and then
letting qcow2_check_fix_snapshot_table() do the rest.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 block/qcow2-snapshot.c | 67 +++---
 1 file changed, 56 insertions(+), 11 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index b526a8f819..53dc1635ec 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -44,7 +44,23 @@ void qcow2_free_snapshots(BlockDriverState *bs)
 s->nb_snapshots = 0;
 }
 
-int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
+/*
+ * If @repair is true, try to repair a broken snapshot table instead
+ * of just returning an error:
+ *
+ * - If there were snapshots with too much extra metadata, increment
+ *   *extra_data_dropped for each.
+ *   This requires the caller to eventually rewrite the whole snapshot
+ *   table, which requires cluster allocation.  Therefore, this should
+ *   be done only after qcow2_check_refcounts() made sure the refcount
+ *   structures are valid.
+ *   (In the meantime, the image is still valid because
+ *   qcow2_check_refcounts() does not do anything with snapshots'
+ *   extra data.)
+ */
+static int qcow2_do_read_snapshots(BlockDriverState *bs, bool repair,
+   int *extra_data_dropped,
+   Error **errp)
 {
 BDRVQcow2State *s = bs->opaque;
 QCowSnapshotHeader h;
@@ -64,6 +80,8 @@ int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
 s->snapshots = g_new0(QCowSnapshot, s->nb_snapshots);
 
 for(i = 0; i < s->nb_snapshots; i++) {
+bool truncate_unknown_extra_data = false;
+
 /* Read statically sized part of the snapshot header */
 offset = ROUND_UP(offset, 8);
 ret = bdrv_pread(bs->file, offset, &h, sizeof(h));
@@ -86,10 +104,21 @@ int qcow2_read_snapshots(BlockDriverState *bs, Error 
**errp)
 name_size = be16_to_cpu(h.name_size);
 
 if (sn->extra_data_size > QCOW_MAX_SNAPSHOT_EXTRA_DATA) {
-ret = -EFBIG;
-error_setg(errp, "Too much extra metadata in snapshot table "
-   "entry %i", i);
-goto fail;
+if (!repair) {
+ret = -EFBIG;
+error_setg(errp, "Too much extra metadata in snapshot table "
+   "entry %i", i);
+error_append_hint(errp, "You can force-remove this extra "
+  "metadata with qemu-img check -r all\n");
+goto fail;
+}
+
+fprintf(stderr, "Discarding too much extra metadata in snapshot "
+"table entry %i (%" PRIu32 " > %u)\n",
+i, sn->extra_data_size, QCOW_MAX_SNAPSHOT_EXTRA_DATA);
+
+(*extra_data_dropped)++;
+truncate_unknown_extra_data = true;
 }
 
 /* Read known extra data */
@@ -113,18 +142,26 @@ int qcow2_read_snapshots(BlockDriverState *bs, Error 
**errp)
 }
 
 if (sn->extra_data_size > sizeof(extra)) {
-/* Store unknown extra data */
-size_t unknown_extra_data_size =
-sn->extra_data_size - sizeof(extra);
+uint64_t extra_data_end;
+size_t unknown_extra_data_size;
+
+extra_data_end = offset + sn->extra_data_size - sizeof(extra);
 
+if (truncate_unknown_extra_data) {
+sn->extra_data_size = QCOW_MAX_SNAPSHOT_EXTRA_DATA;
+}
+
+/* Store unknown extra data */
+unknown_extra_data_size = sn->extra_data_size - sizeof(extra);
 sn->unknown_extra_data = g_malloc(unknown_extra_data_size);
 ret = bdrv_pread(bs->file, offset, sn->unknown_extra_data,
  unknown_extra_data_size);
 if (ret < 0) {
-error_setg_errno(errp, -ret, "Failed to read snapshot table");
+error_setg_errno(errp, -ret,
+ "Failed to read snapshot table");
 goto fail;
 }
-offset += unknown_extra_data_size;
+offset = extra_data_end;
 }
 
 /* Read snapshot ID */
@@ -163,6 +200,11 @@ fail:
 return ret;
 }
 
+int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
+{
+return qcow2_do_read_snapshots(bs, false, NULL, errp);
+}
+
 /* add at the end of the file a new list of snapshots */
 int qcow2_write_snapshots(BlockDriverState *bs)
 {
@@ -328,6 +370,7 @@ int coroutine_fn 
qcow2_check_read_snapshot_table(BlockDriverState *bs,
 {
 BDRVQcow2State *s = bs->opaque;
 Error *local_err = NULL;
+int extra_data_dropped = 0;
 int ret;
 struct {
 uint32_t nb_snapshots;
@@ -363,7 +406,8 @@ int corout

[PATCH v3 08/16] qcow2: Separate qcow2_check_read_snapshot_table()

[Max Reitz]

Reading the snapshot table can fail.  That is a problem when we want to
repair the image.

Therefore, stop reading the snapshot table in qcow2_do_open() in check
mode.  Instead, add a new function qcow2_check_read_snapshot_table()
that reads the snapshot table at a later point.  In the future, we want
to handle errors here and fix them.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 block/qcow2.h  |  4 +++
 block/qcow2-snapshot.c | 58 
 block/qcow2.c  | 76 --
 3 files changed, 120 insertions(+), 18 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 02d629a3d8..6c2a38c98d 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -716,6 +716,10 @@ void qcow2_free_snapshots(BlockDriverState *bs);
 int qcow2_read_snapshots(BlockDriverState *bs, Error **errp);
 int qcow2_write_snapshots(BlockDriverState *bs);
 
+int coroutine_fn qcow2_check_read_snapshot_table(BlockDriverState *bs,
+ BdrvCheckResult *result,
+ BdrvCheckMode fix);
+
 /* qcow2-cache.c functions */
 Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables,
unsigned table_size);
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index e3bf4c9776..d667bfd935 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -322,6 +322,64 @@ fail:
 return ret;
 }
 
+int coroutine_fn qcow2_check_read_snapshot_table(BlockDriverState *bs,
+ BdrvCheckResult *result,
+ BdrvCheckMode fix)
+{
+BDRVQcow2State *s = bs->opaque;
+Error *local_err = NULL;
+int ret;
+struct {
+uint32_t nb_snapshots;
+uint64_t snapshots_offset;
+} QEMU_PACKED snapshot_table_pointer;
+
+/* qcow2_do_open() discards this information in check mode */
+ret = bdrv_pread(bs->file, offsetof(QCowHeader, nb_snapshots),
+ &snapshot_table_pointer, sizeof(snapshot_table_pointer));
+if (ret < 0) {
+result->check_errors++;
+fprintf(stderr, "ERROR failed to read the snapshot table pointer from "
+"the image header: %s\n", strerror(-ret));
+return ret;
+}
+
+s->snapshots_offset = be64_to_cpu(snapshot_table_pointer.snapshots_offset);
+s->nb_snapshots = be32_to_cpu(snapshot_table_pointer.nb_snapshots);
+
+ret = qcow2_validate_table(bs, s->snapshots_offset, s->nb_snapshots,
+   sizeof(QCowSnapshotHeader),
+   sizeof(QCowSnapshotHeader) * QCOW_MAX_SNAPSHOTS,
+   "snapshot table", &local_err);
+if (ret < 0) {
+result->check_errors++;
+error_reportf_err(local_err, "ERROR ");
+
+/* We did not read the snapshot table, so invalidate this information 
*/
+s->snapshots_offset = 0;
+s->nb_snapshots = 0;
+
+return ret;
+}
+
+qemu_co_mutex_unlock(&s->lock);
+ret = qcow2_read_snapshots(bs, &local_err);
+qemu_co_mutex_lock(&s->lock);
+if (ret < 0) {
+result->check_errors++;
+error_reportf_err(local_err,
+  "ERROR failed to read the snapshot table: ");
+
+/* We did not read the snapshot table, so invalidate this information 
*/
+s->snapshots_offset = 0;
+s->nb_snapshots = 0;
+
+return ret;
+}
+
+return 0;
+}
+
 static void find_new_snapshot_id(BlockDriverState *bs,
  char *id_str, int id_str_size)
 {
diff --git a/block/qcow2.c b/block/qcow2.c
index a2e46ce589..7f47444ab7 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -570,11 +570,40 @@ int qcow2_mark_consistent(BlockDriverState *bs)
 return 0;
 }
 
+static void qcow2_add_check_result(BdrvCheckResult *out,
+   const BdrvCheckResult *src,
+   bool set_allocation_info)
+{
+out->corruptions += src->corruptions;
+out->leaks += src->leaks;
+out->check_errors += src->check_errors;
+out->corruptions_fixed += src->corruptions_fixed;
+out->leaks_fixed += src->leaks_fixed;
+
+if (set_allocation_info) {
+out->image_end_offset = src->image_end_offset;
+out->bfi = src->bfi;
+}
+}
+
 static int coroutine_fn qcow2_co_check_locked(BlockDriverState *bs,
   BdrvCheckResult *result,
   BdrvCheckMode fix)
 {
-int ret = qcow2_check_refcounts(bs, result, fix);
+BdrvCheckResult snapshot_res = {};
+BdrvCheckResult refcount_res = {};
+int ret;
+
+memset(result, 0, sizeof(*result));
+
+ret = qcow2_check_read_snapshot_table(bs, &snapshot_res, fix);
+qcow2_add_check_result(result, &snapshot_res, false);
+if (ret < 0) {
+ret

[PATCH v3 07/16] qcow2: Write v3-compliant snapshot list on upgrade

[Max Reitz]

qcow2 v3 requires every snapshot table entry to have two extra data
fields: The 64-bit VM state size, and the virtual disk size.  Both are
optional for v2 images, so they may not be present.

qcow2_upgrade() therefore should update the snapshot table to ensure all
entries have these extra data fields.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1727347
Reported-by: Eric Blake 
Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 block/qcow2.c | 32 ++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index d43064dca2..a2e46ce589 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4907,7 +4907,9 @@ static int qcow2_upgrade(BlockDriverState *bs, int 
target_version,
  Error **errp)
 {
 BDRVQcow2State *s = bs->opaque;
+bool need_snapshot_update;
 int current_version = s->qcow_version;
+int i;
 int ret;
 
 /* This is qcow2_upgrade(), not qcow2_downgrade() */
@@ -4916,7 +4918,33 @@ static int qcow2_upgrade(BlockDriverState *bs, int 
target_version,
 /* There are no other versions (yet) that you can upgrade to */
 assert(target_version == 3);
 
-status_cb(bs, 0, 1, cb_opaque);
+status_cb(bs, 0, 2, cb_opaque);
+
+/*
+ * In v2, snapshots do not need to have extra data.  v3 requires
+ * the 64-bit VM state size and the virtual disk size to be
+ * present.
+ * qcow2_write_snapshots() will always write the list in the
+ * v3-compliant format.
+ */
+need_snapshot_update = false;
+for (i = 0; i < s->nb_snapshots; i++) {
+if (s->snapshots[i].extra_data_size <
+sizeof_field(QCowSnapshotExtraData, vm_state_size_large) +
+sizeof_field(QCowSnapshotExtraData, disk_size))
+{
+need_snapshot_update = true;
+break;
+}
+}
+if (need_snapshot_update) {
+ret = qcow2_write_snapshots(bs);
+if (ret < 0) {
+error_setg_errno(errp, -ret, "Failed to update the snapshot 
table");
+return ret;
+}
+}
+status_cb(bs, 1, 2, cb_opaque);
 
 s->qcow_version = target_version;
 ret = qcow2_update_header(bs);
@@ -4925,7 +4953,7 @@ static int qcow2_upgrade(BlockDriverState *bs, int 
target_version,
 error_setg_errno(errp, -ret, "Failed to update the image header");
 return ret;
 }
-status_cb(bs, 1, 1, cb_opaque);
+status_cb(bs, 2, 2, cb_opaque);
 
 return 0;
 }
-- 
2.21.0

[PATCH v3 11/16] qcow2: Keep track of the snapshot table length

[Max Reitz]

When repairing the snapshot table, we truncate entries that have too
much extra data.  This frees up space that we do not have to count
towards the snapshot table size.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 block/qcow2-snapshot.c | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 53dc1635ec..582eb3386a 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -68,6 +68,7 @@ static int qcow2_do_read_snapshots(BlockDriverState *bs, bool 
repair,
 QCowSnapshot *sn;
 int i, id_str_size, name_size;
 int64_t offset;
+uint64_t table_length = 0;
 int ret;
 
 if (!s->nb_snapshots) {
@@ -82,6 +83,8 @@ static int qcow2_do_read_snapshots(BlockDriverState *bs, bool 
repair,
 for(i = 0; i < s->nb_snapshots; i++) {
 bool truncate_unknown_extra_data = false;
 
+table_length = ROUND_UP(table_length, 8);
+
 /* Read statically sized part of the snapshot header */
 offset = ROUND_UP(offset, 8);
 ret = bdrv_pread(bs->file, offset, &h, sizeof(h));
@@ -184,7 +187,16 @@ static int qcow2_do_read_snapshots(BlockDriverState *bs, 
bool repair,
 offset += name_size;
 sn->name[name_size] = '\0';
 
-if (offset - s->snapshots_offset > QCOW_MAX_SNAPSHOTS_SIZE) {
+/* Note that the extra data may have been truncated */
+table_length += sizeof(h) + sn->extra_data_size + id_str_size +
+name_size;
+if (!repair) {
+assert(table_length == offset - s->snapshots_offset);
+}
+
+if (table_length > QCOW_MAX_SNAPSHOTS_SIZE ||
+offset - s->snapshots_offset > INT_MAX)
+{
 ret = -EFBIG;
 error_setg(errp, "Snapshot table is too big");
 goto fail;
-- 
2.21.0

[PATCH v3 14/16] qcow2: Fix v3 snapshot table entry compliancy

[Max Reitz]

qcow2 v3 images require every snapshot table entry to have at least 16
bytes of extra data.  If they do not, let qemu-img check -r all fix it.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 block/qcow2-snapshot.c | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index dac8a778e4..5ab64da1ec 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -516,6 +516,24 @@ int coroutine_fn 
qcow2_check_read_snapshot_table(BlockDriverState *bs,
 result->corruptions -= nb_clusters_reduced;
 }
 
+/*
+ * All of v3 images' snapshot table entries need to have at least
+ * 16 bytes of extra data.
+ */
+if (s->qcow_version >= 3) {
+int i;
+for (i = 0; i < s->nb_snapshots; i++) {
+if (s->snapshots[i].extra_data_size <
+sizeof_field(QCowSnapshotExtraData, vm_state_size_large) +
+sizeof_field(QCowSnapshotExtraData, disk_size))
+{
+result->corruptions++;
+fprintf(stderr, "%s snapshot table entry %i is incomplete\n",
+fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
+}
+}
+}
+
 return 0;
 }
 
-- 
2.21.0

[PATCH v3 13/16] qcow2: Repair snapshot table with too many entries

[Max Reitz]

The user cannot choose which snapshots are removed.  This is fine
because we have chosen the maximum snapshot table size to be so large
(65536 entries) that it cannot be reasonably reached.  If the snapshot
table exceeds this size, the image has probably been corrupted in some
way; in this case, it is most important to just make the image usable
such that the user can copy off at least the active layer.
(Also note that the snapshots will be removed only with "-r all", so a
plain "check" or "check -r leaks" will not delete any data.)

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 block/qcow2-snapshot.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 366d9f574c..dac8a778e4 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -444,6 +444,14 @@ int coroutine_fn 
qcow2_check_read_snapshot_table(BlockDriverState *bs,
 s->snapshots_offset = be64_to_cpu(snapshot_table_pointer.snapshots_offset);
 s->nb_snapshots = be32_to_cpu(snapshot_table_pointer.nb_snapshots);
 
+if (s->nb_snapshots > QCOW_MAX_SNAPSHOTS && (fix & BDRV_FIX_ERRORS)) {
+fprintf(stderr, "Discarding %u overhanging snapshots\n",
+s->nb_snapshots - QCOW_MAX_SNAPSHOTS);
+
+nb_clusters_reduced += s->nb_snapshots - QCOW_MAX_SNAPSHOTS;
+s->nb_snapshots = QCOW_MAX_SNAPSHOTS;
+}
+
 ret = qcow2_validate_table(bs, s->snapshots_offset, s->nb_snapshots,
sizeof(QCowSnapshotHeader),
sizeof(QCowSnapshotHeader) * QCOW_MAX_SNAPSHOTS,
@@ -452,6 +460,12 @@ int coroutine_fn 
qcow2_check_read_snapshot_table(BlockDriverState *bs,
 result->check_errors++;
 error_reportf_err(local_err, "ERROR ");
 
+if (s->nb_snapshots > QCOW_MAX_SNAPSHOTS) {
+fprintf(stderr, "You can force-remove all %u overhanging snapshots 
"
+"with qemu-img check -r all\n",
+s->nb_snapshots - QCOW_MAX_SNAPSHOTS);
+}
+
 /* We did not read the snapshot table, so invalidate this information 
*/
 s->snapshots_offset = 0;
 s->nb_snapshots = 0;
-- 
2.21.0

[PATCH v3 15/16] iotests: Add peek_file* functions

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 tests/qemu-iotests/common.rc | 20 
 1 file changed, 20 insertions(+)

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 12b4751848..fa7bae2422 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -53,6 +53,26 @@ poke_file()
 printf "$3" | dd "of=$1" bs=1 "seek=$2" conv=notrunc &>/dev/null
 }
 
+# peek_file_le 'test.img' 512 2 => 65534
+peek_file_le()
+{
+# Wrap in echo $() to strip spaces
+echo $(od -j"$2" -N"$3" --endian=little -An -vtu"$3" "$1")
+}
+
+# peek_file_be 'test.img' 512 2 => 65279
+peek_file_be()
+{
+# Wrap in echo $() to strip spaces
+echo $(od -j"$2" -N"$3" --endian=big -An -vtu"$3" "$1")
+}
+
+# peek_file_raw 'test.img' 512 2 => '\xff\xfe'
+peek_file_raw()
+{
+dd if="$1" bs=1 skip="$2" count="$3" status=none
+}
+
 
 if ! . ./common.config
 then
-- 
2.21.0

[PATCH v3 16/16] iotests: Test qcow2's snapshot table handling

[Max Reitz]

Add a test how our qcow2 driver handles extra data in snapshot table
entries, and how it repairs overly long snapshot tables.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 tests/qemu-iotests/261 | 523 +
 tests/qemu-iotests/261.out | 346 
 tests/qemu-iotests/group   |   1 +
 3 files changed, 870 insertions(+)
 create mode 100755 tests/qemu-iotests/261
 create mode 100644 tests/qemu-iotests/261.out

diff --git a/tests/qemu-iotests/261 b/tests/qemu-iotests/261
new file mode 100755
index 00..fb96bcfbe2
--- /dev/null
+++ b/tests/qemu-iotests/261
@@ -0,0 +1,523 @@
+#!/usr/bin/env bash
+#
+# Test case for qcow2's handling of extra data in snapshot table entries
+# (and more generally, how certain cases of broken snapshot tables are
+# handled)
+#
+# Copyright (C) 2019 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+# creator
+owner=mre...@redhat.com
+
+seq=$(basename $0)
+echo "QA output created by $seq"
+
+status=1   # failure is the default!
+
+_cleanup()
+{
+_cleanup_test_img
+rm -f "$TEST_IMG".v{2,3}.orig
+rm -f "$TEST_DIR"/sn{0,1,2}{,-pre,-extra,-post}
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+# This tests qocw2-specific low-level functionality
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+# (1) We create a v2 image that supports nothing but refcount_bits=16
+# (2) We do some refcount management on our own which expects
+# refcount_bits=16
+_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
+
+# Parameters:
+#   $1: image filename
+#   $2: snapshot table entry offset in the image
+snapshot_table_entry_size()
+{
+id_len=$(peek_file_be "$1" $(($2 + 12)) 2)
+name_len=$(peek_file_be "$1" $(($2 + 14)) 2)
+extra_len=$(peek_file_be "$1" $(($2 + 36)) 4)
+
+full_len=$((40 + extra_len + id_len + name_len))
+echo $(((full_len + 7) / 8 * 8))
+}
+
+# Parameter:
+#   $1: image filename
+print_snapshot_table()
+{
+nb_entries=$(peek_file_be "$1" 60 4)
+offset=$(peek_file_be "$1" 64 8)
+
+echo "Snapshots in $1:" | _filter_testdir | _filter_imgfmt
+
+for ((i = 0; i < nb_entries; i++)); do
+id_len=$(peek_file_be "$1" $((offset + 12)) 2)
+name_len=$(peek_file_be "$1" $((offset + 14)) 2)
+extra_len=$(peek_file_be "$1" $((offset + 36)) 4)
+
+extra_ofs=$((offset + 40))
+id_ofs=$((extra_ofs + extra_len))
+name_ofs=$((id_ofs + id_len))
+
+echo "  [$i]"
+echo "ID: $(peek_file_raw "$1" $id_ofs $id_len)"
+echo "Name: $(peek_file_raw "$1" $name_ofs $name_len)"
+echo "Extra data size: $extra_len"
+if [ $extra_len -ge 8 ]; then
+echo "VM state size: $(peek_file_be "$1" $extra_ofs 8)"
+fi
+if [ $extra_len -ge 16 ]; then
+echo "Disk size: $(peek_file_be "$1" $((extra_ofs + 8)) 8)"
+fi
+if [ $extra_len -gt 16 ]; then
+echo 'Unknown extra data:' \
+"$(peek_file_raw "$1" $((extra_ofs + 16)) $((extra_len - 16)) \
+   | tr -d '\0')"
+fi
+
+offset=$((offset + $(snapshot_table_entry_size "$1" $offset)))
+done
+}
+
+# Mark clusters as allocated; works only in refblock 0 (i.e. before
+# cluster #32768).
+# Parameters:
+#   $1: Start offset of what to allocate
+#   $2: End offset (exclusive)
+refblock0_allocate()
+{
+reftable_ofs=$(peek_file_be "$TEST_IMG" 48 8)
+refblock_ofs=$(peek_file_be "$TEST_IMG" $reftable_ofs 8)
+
+cluster=$(($1 / 65536))
+ecluster=$((($2 + 65535) / 65536))
+
+while [ $cluster -lt $ecluster ]; do
+if [ $cluster -ge 32768 ]; then
+echo "*** Abort: Cluster $cluster exceeds refblock 0 ***"
+exit 1
+fi
+poke_file "$TEST_IMG" $((refblock_ofs + cluster * 2)) '\x00\x01'
+cluster=$((cluster + 1))
+done
+}
+
+
+echo
+echo '=== Create v2 template ==='
+echo
+
+# Create v2 image with a snapshot table with three entries:
+# [0]: No extra data (valid with v2, not valid with v3)
+# [1]: Has extra data unknown to qemu
+# [2]: Has the 64-bit VM state size, but not the disk size (again,
+#  valid with v2, not valid with v3)
+
+TEST_IMG="$TEST_IMG.v2.orig" IMGOPTS='compa

[RFC v5 011/126] block/snapshot: rename Error ** parameter to more common errp

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/snapshot.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index 8081616ae9..bd9fb01817 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -426,7 +426,7 @@ fail:
 }
 
 int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bad_bs,
- Error **err)
+ Error **errp)
 {
 int ret = 0;
 BlockDriverState *bs;
@@ -441,7 +441,7 @@ int bdrv_all_delete_snapshot(const char *name, 
BlockDriverState **first_bad_bs,
 bdrv_snapshot_find(bs, snapshot, name) >= 0)
 {
 ret = bdrv_snapshot_delete(bs, snapshot->id_str,
-   snapshot->name, err);
+   snapshot->name, errp);
 }
 aio_context_release(ctx);
 if (ret < 0) {
-- 
2.21.0

[RFC v5 039/126] IDE: introduce ERRP_AUTO_PROPAGATE

[Vladimir Sementsov-Ogievskiy]

If we want to add some info to errp (by error_prepend() or
error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
Otherwise, this info will not be added when errp == &fatal_err
(the program will exit prior to the error_append_hint() or
error_prepend() call).  Fix such cases.

If we want to check error after errp-function call, we need to
introduce local_err and than propagate it to errp. Instead, use
ERRP_AUTO_PROPAGATE macro, benefits are:
1. No need of explicit error_propagate call
2. No need of explicit local_err variable: use errp directly
3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
   &error_fatel, this means that we don't break error_abort
   (we'll abort on error_set, not on error_propagate)

This commit (together with its neighbors) was generated by

for f in $(git grep -l errp \*.[ch]); do \
spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
--macro-file scripts/cocci-macro-file.h --in-place --no-show-diff $f; \
done;

then fix a bit of compilation problems: coccinelle for some reason
leaves several
f() {
...
goto out;
...
out:
}
patterns, with "out:" at function end.

then
./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"

(auto-msg was a file with this commit message)

Still, for backporting it may be more comfortable to use only the first
command and then do one huge commit.

Reported-by: Kevin Wolf 
Reported-by: Greg Kurz 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 hw/ide/qdev.c | 16 +++-
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 6fba6b62b8..9e5ed9914f 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -233,18 +233,18 @@ static void ide_dev_get_bootindex(Object *obj, Visitor 
*v, const char *name,
 static void ide_dev_set_bootindex(Object *obj, Visitor *v, const char *name,
   void *opaque, Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 IDEDevice *d = IDE_DEVICE(obj);
 int32_t boot_index;
-Error *local_err = NULL;
 
-visit_type_int32(v, name, &boot_index, &local_err);
-if (local_err) {
-goto out;
+visit_type_int32(v, name, &boot_index, errp);
+if (*errp) {
+return;
 }
 /* check whether bootindex is present in fw_boot_order list  */
-check_boot_index(boot_index, &local_err);
-if (local_err) {
-goto out;
+check_boot_index(boot_index, errp);
+if (*errp) {
+return;
 }
 /* change bootindex to a new one */
 d->conf.bootindex = boot_index;
@@ -253,8 +253,6 @@ static void ide_dev_set_bootindex(Object *obj, Visitor *v, 
const char *name,
 add_boot_device_path(d->conf.bootindex, &d->qdev,
  d->unit ? "/disk@1" : "/disk@0");
 }
-out:
-error_propagate(errp, local_err);
 }
 
 static void ide_dev_instance_init(Object *obj)
-- 
2.21.0

[RFC v5 007/126] nbd: well form nbd_iter_channel_error errp handler

[Vladimir Sementsov-Ogievskiy]

Make nbd_iter_channel_error errp handler well formed:
rename errp to errp_in, as it is IN-parameter here (which is unusual
for errp).

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/nbd.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index 813c40d8f0..c66fdf54b9 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -684,18 +684,18 @@ typedef struct NBDReplyChunkIter {
 } NBDReplyChunkIter;
 
 static void nbd_iter_channel_error(NBDReplyChunkIter *iter,
-   int ret, Error **local_err)
+   int ret, Error **errp_in)
 {
-assert(ret < 0);
+assert(ret < 0 && errp_in && *errp_in);
 
 if (!iter->ret) {
 iter->ret = ret;
-error_propagate(&iter->err, *local_err);
+error_propagate(&iter->err, *errp_in);
 } else {
-error_free(*local_err);
+error_free(*errp_in);
 }
 
-*local_err = NULL;
+*errp_in = NULL;
 }
 
 static void nbd_iter_request_error(NBDReplyChunkIter *iter, int ret)
-- 
2.21.0

[RFC v5 040/126] Floppy: introduce ERRP_AUTO_PROPAGATE

[Vladimir Sementsov-Ogievskiy]

If we want to add some info to errp (by error_prepend() or
error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
Otherwise, this info will not be added when errp == &fatal_err
(the program will exit prior to the error_append_hint() or
error_prepend() call).  Fix such cases.

If we want to check error after errp-function call, we need to
introduce local_err and than propagate it to errp. Instead, use
ERRP_AUTO_PROPAGATE macro, benefits are:
1. No need of explicit error_propagate call
2. No need of explicit local_err variable: use errp directly
3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
   &error_fatel, this means that we don't break error_abort
   (we'll abort on error_set, not on error_propagate)

This commit (together with its neighbors) was generated by

for f in $(git grep -l errp \*.[ch]); do \
spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
--macro-file scripts/cocci-macro-file.h --in-place --no-show-diff $f; \
done;

then fix a bit of compilation problems: coccinelle for some reason
leaves several
f() {
...
goto out;
...
out:
}
patterns, with "out:" at function end.

then
./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"

(auto-msg was a file with this commit message)

Still, for backporting it may be more comfortable to use only the first
command and then do one huge commit.

Reported-by: Kevin Wolf 
Reported-by: Greg Kurz 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 hw/block/fdc.c | 19 ---
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index ac5d31e8c1..7590b63c19 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2522,11 +2522,11 @@ static void fdctrl_result_timer(void *opaque)
 static void fdctrl_connect_drives(FDCtrl *fdctrl, DeviceState *fdc_dev,
   Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 unsigned int i;
 FDrive *drive;
 DeviceState *dev;
 BlockBackend *blk;
-Error *local_err = NULL;
 
 for (i = 0; i < MAX_FD; i++) {
 drive = &fdctrl->drives[i];
@@ -2548,17 +2548,15 @@ static void fdctrl_connect_drives(FDCtrl *fdctrl, 
DeviceState *fdc_dev,
 blk_ref(blk);
 blk_detach_dev(blk, fdc_dev);
 fdctrl->qdev_for_drives[i].blk = NULL;
-qdev_prop_set_drive(dev, "drive", blk, &local_err);
+qdev_prop_set_drive(dev, "drive", blk, errp);
 blk_unref(blk);
 
-if (local_err) {
-error_propagate(errp, local_err);
+if (*errp) {
 return;
 }
 
-object_property_set_bool(OBJECT(dev), true, "realized", &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
+object_property_set_bool(OBJECT(dev), true, "realized", errp);
+if (*errp) {
 return;
 }
 }
@@ -2685,10 +2683,10 @@ static const MemoryRegionPortio fdc_portio_list[] = {
 
 static void isabus_fdc_realize(DeviceState *dev, Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 ISADevice *isadev = ISA_DEVICE(dev);
 FDCtrlISABus *isa = ISA_FDC(dev);
 FDCtrl *fdctrl = &isa->state;
-Error *err = NULL;
 
 isa_register_portio_list(isadev, &fdctrl->portio_list,
  isa->iobase, fdc_portio_list, fdctrl,
@@ -2705,9 +2703,8 @@ static void isabus_fdc_realize(DeviceState *dev, Error 
**errp)
 }
 
 qdev_set_legacy_instance_id(dev, isa->iobase, 2);
-fdctrl_realize_common(dev, fdctrl, &err);
-if (err != NULL) {
-error_propagate(errp, err);
+fdctrl_realize_common(dev, fdctrl, errp);
+if (*errp) {
 return;
 }
 }
-- 
2.21.0

[RFC v5 055/126] virtio-blk: introduce ERRP_AUTO_PROPAGATE

[Vladimir Sementsov-Ogievskiy]

If we want to add some info to errp (by error_prepend() or
error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
Otherwise, this info will not be added when errp == &fatal_err
(the program will exit prior to the error_append_hint() or
error_prepend() call).  Fix such cases.

If we want to check error after errp-function call, we need to
introduce local_err and than propagate it to errp. Instead, use
ERRP_AUTO_PROPAGATE macro, benefits are:
1. No need of explicit error_propagate call
2. No need of explicit local_err variable: use errp directly
3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
   &error_fatel, this means that we don't break error_abort
   (we'll abort on error_set, not on error_propagate)

This commit (together with its neighbors) was generated by

for f in $(git grep -l errp \*.[ch]); do \
spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
--macro-file scripts/cocci-macro-file.h --in-place --no-show-diff $f; \
done;

then fix a bit of compilation problems: coccinelle for some reason
leaves several
f() {
...
goto out;
...
out:
}
patterns, with "out:" at function end.

then
./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"

(auto-msg was a file with this commit message)

Still, for backporting it may be more comfortable to use only the first
command and then do one huge commit.

Reported-by: Kevin Wolf 
Reported-by: Greg Kurz 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 hw/block/dataplane/virtio-blk.c | 1 +
 hw/block/virtio-blk.c   | 7 +++
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 119906a5fe..f8a1e70886 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -85,6 +85,7 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, 
VirtIOBlkConf *conf,
   VirtIOBlockDataPlane **dataplane,
   Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 VirtIOBlockDataPlane *s;
 BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
 VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index ed2ddebd2b..788d346727 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1113,10 +1113,10 @@ static const BlockDevOps virtio_block_ops = {
 
 static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 VirtIODevice *vdev = VIRTIO_DEVICE(dev);
 VirtIOBlock *s = VIRTIO_BLK(dev);
 VirtIOBlkConf *conf = &s->conf;
-Error *err = NULL;
 unsigned i;
 
 if (!conf->conf.blk) {
@@ -1188,9 +1188,8 @@ static void virtio_blk_device_realize(DeviceState *dev, 
Error **errp)
 for (i = 0; i < conf->num_queues; i++) {
 virtio_add_queue(vdev, conf->queue_size, virtio_blk_handle_output);
 }
-virtio_blk_data_plane_create(vdev, conf, &s->dataplane, &err);
-if (err != NULL) {
-error_propagate(errp, err);
+virtio_blk_data_plane_create(vdev, conf, &s->dataplane, errp);
+if (*errp) {
 virtio_cleanup(vdev);
 return;
 }
-- 
2.21.0

[RFC v5 019/126] include/block/snapshot.h: rename Error ** parameter to more common errp

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 include/block/snapshot.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index b5d5084a12..2bfcd57578 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -78,7 +78,7 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
 
 bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs);
 int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bsd_bs,
- Error **err);
+ Error **errp);
 int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs,
Error **errp);
 int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs);
-- 
2.21.0

[RFC v5 052/126] vhost: introduce ERRP_AUTO_PROPAGATE

[Vladimir Sementsov-Ogievskiy]

If we want to add some info to errp (by error_prepend() or
error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
Otherwise, this info will not be added when errp == &fatal_err
(the program will exit prior to the error_append_hint() or
error_prepend() call).  Fix such cases.

If we want to check error after errp-function call, we need to
introduce local_err and than propagate it to errp. Instead, use
ERRP_AUTO_PROPAGATE macro, benefits are:
1. No need of explicit error_propagate call
2. No need of explicit local_err variable: use errp directly
3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
   &error_fatel, this means that we don't break error_abort
   (we'll abort on error_set, not on error_propagate)

This commit (together with its neighbors) was generated by

for f in $(git grep -l errp \*.[ch]); do \
spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
--macro-file scripts/cocci-macro-file.h --in-place --no-show-diff $f; \
done;

then fix a bit of compilation problems: coccinelle for some reason
leaves several
f() {
...
goto out;
...
out:
}
patterns, with "out:" at function end.

then
./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"

(auto-msg was a file with this commit message)

Still, for backporting it may be more comfortable to use only the first
command and then do one huge commit.

Reported-by: Kevin Wolf 
Reported-by: Greg Kurz 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 hw/block/vhost-user-blk.c |  6 +++---
 hw/scsi/vhost-scsi.c  | 12 +---
 hw/scsi/vhost-user-scsi.c |  7 +++
 hw/virtio/vhost-vsock.c   |  1 +
 4 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 63da9bb619..9dfe173c19 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -388,9 +388,9 @@ static void vhost_user_blk_event(void *opaque, int event)
 
 static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 VirtIODevice *vdev = VIRTIO_DEVICE(dev);
 VHostUserBlk *s = VHOST_USER_BLK(vdev);
-Error *err = NULL;
 int i, ret;
 
 if (!s->chardev.chr) {
@@ -429,8 +429,8 @@ static void vhost_user_blk_device_realize(DeviceState *dev, 
Error **errp)
  NULL, (void *)dev, NULL, true);
 
 reconnect:
-if (qemu_chr_fe_wait_connected(&s->chardev, &err) < 0) {
-error_report_err(err);
+if (qemu_chr_fe_wait_connected(&s->chardev, errp) < 0) {
+error_report_errp(errp);
 goto virtio_err;
 }
 
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index c693fc748a..2dca1f7fe2 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -165,9 +165,9 @@ static const VMStateDescription vmstate_virtio_vhost_scsi = 
{
 
 static void vhost_scsi_realize(DeviceState *dev, Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev);
 VHostSCSICommon *vsc = VHOST_SCSI_COMMON(dev);
-Error *err = NULL;
 int vhostfd = -1;
 int ret;
 
@@ -195,9 +195,8 @@ static void vhost_scsi_realize(DeviceState *dev, Error 
**errp)
vhost_dummy_handle_output,
vhost_dummy_handle_output,
vhost_dummy_handle_output,
-   &err);
-if (err != NULL) {
-error_propagate(errp, err);
+   errp);
+if (*errp) {
 goto close_fd;
 }
 
@@ -207,9 +206,8 @@ static void vhost_scsi_realize(DeviceState *dev, Error 
**errp)
 "When external environment supports it (Orchestrator migrates "
 "target SCSI device state or use shared storage over network), 
"
 "set 'migratable' property to true to enable migration.");
-migrate_add_blocker(vsc->migration_blocker, &err);
-if (err) {
-error_propagate(errp, err);
+migrate_add_blocker(vsc->migration_blocker, errp);
+if (*errp) {
 error_free(vsc->migration_blocker);
 goto free_virtio;
 }
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index 6a6c15dd32..ef4b25104f 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -68,11 +68,11 @@ static void vhost_dummy_handle_output(VirtIODevice *vdev, 
VirtQueue *vq)
 
 static void vhost_user_scsi_realize(DeviceState *dev, Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev);
 VHostUserSCSI *s = VHOST_USER_SCSI(dev);
 VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
 struct vhost_virtqueue *vqs = NULL;
-Error *err = NULL;
 int ret;
 
 if (!vs->conf.chardev.chr) {
@@ -82,9 +82,8 @@ static void vhost_user_scsi_realize(DeviceState *dev, Error 
**errp)
 
 virtio_scsi_common_realize(dev, vhost_dummy_handle_output,
vhost_dummy_handle

[RFC v5 068/126] scsi: introduce ERRP_AUTO_PROPAGATE

[Vladimir Sementsov-Ogievskiy]

If we want to add some info to errp (by error_prepend() or
error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
Otherwise, this info will not be added when errp == &fatal_err
(the program will exit prior to the error_append_hint() or
error_prepend() call).  Fix such cases.

If we want to check error after errp-function call, we need to
introduce local_err and than propagate it to errp. Instead, use
ERRP_AUTO_PROPAGATE macro, benefits are:
1. No need of explicit error_propagate call
2. No need of explicit local_err variable: use errp directly
3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
   &error_fatel, this means that we don't break error_abort
   (we'll abort on error_set, not on error_propagate)

This commit (together with its neighbors) was generated by

for f in $(git grep -l errp \*.[ch]); do \
spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
--macro-file scripts/cocci-macro-file.h --in-place --no-show-diff $f; \
done;

then fix a bit of compilation problems: coccinelle for some reason
leaves several
f() {
...
goto out;
...
out:
}
patterns, with "out:" at function end.

then
./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"

(auto-msg was a file with this commit message)

Still, for backporting it may be more comfortable to use only the first
command and then do one huge commit.

Reported-by: Kevin Wolf 
Reported-by: Greg Kurz 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 scsi/pr-manager-helper.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/scsi/pr-manager-helper.c b/scsi/pr-manager-helper.c
index ca27c93283..f41eaa8524 100644
--- a/scsi/pr-manager-helper.c
+++ b/scsi/pr-manager-helper.c
@@ -101,13 +101,13 @@ static int pr_manager_helper_write(PRManagerHelper 
*pr_mgr,
 static int pr_manager_helper_initialize(PRManagerHelper *pr_mgr,
 Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 char *path = g_strdup(pr_mgr->path);
 SocketAddress saddr = {
 .type = SOCKET_ADDRESS_TYPE_UNIX,
 .u.q_unix.path = path
 };
 QIOChannelSocket *sioc = qio_channel_socket_new();
-Error *local_err = NULL;
 
 uint32_t flags;
 int r;
@@ -116,11 +116,10 @@ static int pr_manager_helper_initialize(PRManagerHelper 
*pr_mgr,
 qio_channel_set_name(QIO_CHANNEL(sioc), "pr-manager-helper");
 qio_channel_socket_connect_sync(sioc,
 &saddr,
-&local_err);
+errp);
 g_free(path);
-if (local_err) {
+if (*errp) {
 object_unref(OBJECT(sioc));
-error_propagate(errp, local_err);
 return -ENOTCONN;
 }
 
-- 
2.21.0

[RFC v5 031/126] xen: introduce ERRP_AUTO_PROPAGATE

[Vladimir Sementsov-Ogievskiy]

If we want to add some info to errp (by error_prepend() or
error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
Otherwise, this info will not be added when errp == &fatal_err
(the program will exit prior to the error_append_hint() or
error_prepend() call).  Fix such cases.

If we want to check error after errp-function call, we need to
introduce local_err and than propagate it to errp. Instead, use
ERRP_AUTO_PROPAGATE macro, benefits are:
1. No need of explicit error_propagate call
2. No need of explicit local_err variable: use errp directly
3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
   &error_fatel, this means that we don't break error_abort
   (we'll abort on error_set, not on error_propagate)

This commit (together with its neighbors) was generated by

for f in $(git grep -l errp \*.[ch]); do \
spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
--macro-file scripts/cocci-macro-file.h --in-place --no-show-diff $f; \
done;

then fix a bit of compilation problems: coccinelle for some reason
leaves several
f() {
...
goto out;
...
out:
}
patterns, with "out:" at function end.

then
./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"

(auto-msg was a file with this commit message)

Still, for backporting it may be more comfortable to use only the first
command and then do one huge commit.

Reported-by: Kevin Wolf 
Reported-by: Greg Kurz 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 hw/block/dataplane/xen-block.c |  17 ++---
 hw/block/xen-block.c   | 119 ++---
 hw/xen/xen-backend.c   |   7 +-
 hw/xen/xen-bus.c   |  92 -
 hw/xen/xen-host-pci-device.c   |  27 
 hw/xen/xen_pt.c|  25 +++
 hw/xen/xen_pt_config_init.c|  20 +++---
 7 files changed, 139 insertions(+), 168 deletions(-)

diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c
index 3b9caeb2fa..c38e3c3d85 100644
--- a/hw/block/dataplane/xen-block.c
+++ b/hw/block/dataplane/xen-block.c
@@ -727,8 +727,8 @@ void xen_block_dataplane_start(XenBlockDataPlane *dataplane,
unsigned int protocol,
Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 XenDevice *xendev = dataplane->xendev;
-Error *local_err = NULL;
 unsigned int ring_size;
 unsigned int i;
 
@@ -764,9 +764,8 @@ void xen_block_dataplane_start(XenBlockDataPlane *dataplane,
 }
 
 xen_device_set_max_grant_refs(xendev, dataplane->nr_ring_ref,
-  &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
+  errp);
+if (*errp) {
 goto stop;
 }
 
@@ -774,9 +773,8 @@ void xen_block_dataplane_start(XenBlockDataPlane *dataplane,
   dataplane->ring_ref,
   dataplane->nr_ring_ref,
   PROT_READ | PROT_WRITE,
-  &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
+  errp);
+if (*errp) {
 goto stop;
 }
 
@@ -809,9 +807,8 @@ void xen_block_dataplane_start(XenBlockDataPlane *dataplane,
 dataplane->event_channel =
 xen_device_bind_event_channel(xendev, dataplane->ctx, event_channel,
   xen_block_dataplane_event, dataplane,
-  &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
+  errp);
+if (*errp) {
 goto stop;
 }
 
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index 879fc310a4..8f4165edd9 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -194,6 +194,7 @@ static const BlockDevOps xen_block_dev_ops = {
 
 static void xen_block_realize(XenDevice *xendev, Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 XenBlockDevice *blockdev = XEN_BLOCK_DEVICE(xendev);
 XenBlockDeviceClass *blockdev_class =
 XEN_BLOCK_DEVICE_GET_CLASS(xendev);
@@ -201,7 +202,6 @@ static void xen_block_realize(XenDevice *xendev, Error 
**errp)
 XenBlockVdev *vdev = &blockdev->props.vdev;
 BlockConf *conf = &blockdev->props.conf;
 BlockBackend *blk = conf->blk;
-Error *local_err = NULL;
 
 if (vdev->type == XEN_BLOCK_VDEV_TYPE_INVALID) {
 error_setg(errp, "vdev property not set");
@@ -211,9 +211,8 @@ static void xen_block_realize(XenDevice *xendev, Error 
**errp)
 trace_xen_block_realize(type, vdev->disk, vdev->partition);
 
 if (blockdev_class->realize) {
-blockdev_class->realize(blockdev, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
+blockdev_class->realize(blockdev, errp);
+if (*errp) {
 return;
   

[RFC v5 098/126] iSCSI: introduce ERRP_AUTO_PROPAGATE

[Vladimir Sementsov-Ogievskiy]

If we want to add some info to errp (by error_prepend() or
error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
Otherwise, this info will not be added when errp == &fatal_err
(the program will exit prior to the error_append_hint() or
error_prepend() call).  Fix such cases.

If we want to check error after errp-function call, we need to
introduce local_err and than propagate it to errp. Instead, use
ERRP_AUTO_PROPAGATE macro, benefits are:
1. No need of explicit error_propagate call
2. No need of explicit local_err variable: use errp directly
3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
   &error_fatel, this means that we don't break error_abort
   (we'll abort on error_set, not on error_propagate)

This commit (together with its neighbors) was generated by

for f in $(git grep -l errp \*.[ch]); do \
spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
--macro-file scripts/cocci-macro-file.h --in-place --no-show-diff $f; \
done;

then fix a bit of compilation problems: coccinelle for some reason
leaves several
f() {
...
goto out;
...
out:
}
patterns, with "out:" at function end.

then
./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"

(auto-msg was a file with this commit message)

Still, for backporting it may be more comfortable to use only the first
command and then do one huge commit.

Reported-by: Kevin Wolf 
Reported-by: Greg Kurz 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/iscsi.c | 36 +++-
 1 file changed, 15 insertions(+), 21 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 506bf5f875..bd825fe40e 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1779,6 +1779,7 @@ static void iscsi_save_designator(IscsiLun *lun,
 static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
   Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 IscsiLun *iscsilun = bs->opaque;
 struct iscsi_context *iscsi = NULL;
 struct scsi_task *task = NULL;
@@ -1786,7 +1787,6 @@ static int iscsi_open(BlockDriverState *bs, QDict 
*options, int flags,
 struct scsi_inquiry_supported_pages *inq_vpd;
 char *initiator_name = NULL;
 QemuOpts *opts;
-Error *local_err = NULL;
 const char *transport_name, *portal, *target;
 #if LIBISCSI_API_VERSION >= (20160603)
 enum iscsi_transport_type transport;
@@ -1794,9 +1794,8 @@ static int iscsi_open(BlockDriverState *bs, QDict 
*options, int flags,
 int i, ret = 0, timeout = 0, lun;
 
 opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
-qemu_opts_absorb_qdict(opts, options, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
+qemu_opts_absorb_qdict(opts, options, errp);
+if (*errp) {
 ret = -EINVAL;
 goto out;
 }
@@ -1850,9 +1849,8 @@ static int iscsi_open(BlockDriverState *bs, QDict 
*options, int flags,
 }
 
 /* check if we got CHAP username/password via the options */
-apply_chap(iscsi, opts, &local_err);
-if (local_err != NULL) {
-error_propagate(errp, local_err);
+apply_chap(iscsi, opts, errp);
+if (*errp) {
 ret = -EINVAL;
 goto out;
 }
@@ -1864,9 +1862,8 @@ static int iscsi_open(BlockDriverState *bs, QDict 
*options, int flags,
 }
 
 /* check if we got HEADER_DIGEST via the options */
-apply_header_digest(iscsi, opts, &local_err);
-if (local_err != NULL) {
-error_propagate(errp, local_err);
+apply_header_digest(iscsi, opts, errp);
+if (*errp) {
 ret = -EINVAL;
 goto out;
 }
@@ -1918,9 +1915,8 @@ static int iscsi_open(BlockDriverState *bs, QDict 
*options, int flags,
 flags &= ~BDRV_O_RDWR;
 }
 
-iscsi_readcapacity_sync(iscsilun, &local_err);
-if (local_err != NULL) {
-error_propagate(errp, local_err);
+iscsi_readcapacity_sync(iscsilun, errp);
+if (*errp) {
 ret = -EINVAL;
 goto out;
 }
@@ -2124,8 +2120,8 @@ static void iscsi_reopen_commit(BDRVReopenState 
*reopen_state)
 static int coroutine_fn iscsi_co_truncate(BlockDriverState *bs, int64_t offset,
   PreallocMode prealloc, Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 IscsiLun *iscsilun = bs->opaque;
-Error *local_err = NULL;
 
 if (prealloc != PREALLOC_MODE_OFF) {
 error_setg(errp, "Unsupported preallocation mode '%s'",
@@ -2138,9 +2134,8 @@ static int coroutine_fn 
iscsi_co_truncate(BlockDriverState *bs, int64_t offset,
 return -ENOTSUP;
 }
 
-iscsi_readcapacity_sync(iscsilun, &local_err);
-if (local_err != NULL) {
-error_propagate(errp, local_err);
+iscsi_readcapacity_sync(iscsilun, errp);
+if (*errp) {
 return -EIO;
 }
 
@@ -2159,12 +2154,12 @@ static int coroutine_fn 
iscsi_co_truncate(BlockDriverState *bs, int64_t offset,
 static int coroutine_fn iscsi_co_create_opts(const char *filename, QemuOpts 
*o

[RFC v5 097/126] VDI: introduce ERRP_AUTO_PROPAGATE

[Vladimir Sementsov-Ogievskiy]

If we want to add some info to errp (by error_prepend() or
error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
Otherwise, this info will not be added when errp == &fatal_err
(the program will exit prior to the error_append_hint() or
error_prepend() call).  Fix such cases.

If we want to check error after errp-function call, we need to
introduce local_err and than propagate it to errp. Instead, use
ERRP_AUTO_PROPAGATE macro, benefits are:
1. No need of explicit error_propagate call
2. No need of explicit local_err variable: use errp directly
3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
   &error_fatel, this means that we don't break error_abort
   (we'll abort on error_set, not on error_propagate)

This commit (together with its neighbors) was generated by

for f in $(git grep -l errp \*.[ch]); do \
spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
--macro-file scripts/cocci-macro-file.h --in-place --no-show-diff $f; \
done;

then fix a bit of compilation problems: coccinelle for some reason
leaves several
f() {
...
goto out;
...
out:
}
patterns, with "out:" at function end.

then
./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"

(auto-msg was a file with this commit message)

Still, for backporting it may be more comfortable to use only the first
command and then do one huge commit.

Reported-by: Kevin Wolf 
Reported-by: Greg Kurz 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/vdi.c | 15 +++
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index 806ba7f53c..0f2e0723f9 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -371,11 +371,11 @@ static int vdi_probe(const uint8_t *buf, int buf_size, 
const char *filename)
 static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
 Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 BDRVVdiState *s = bs->opaque;
 VdiHeader header;
 size_t bmap_size;
 int ret;
-Error *local_err = NULL;
 QemuUUID uuid_link, uuid_parent;
 
 bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
@@ -496,9 +496,8 @@ static int vdi_open(BlockDriverState *bs, QDict *options, 
int flags,
 error_setg(&s->migration_blocker, "The vdi format used by node '%s' "
"does not support live migration",
bdrv_get_device_or_node_name(bs));
-ret = migrate_add_blocker(s->migration_blocker, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
+ret = migrate_add_blocker(s->migration_blocker, errp);
+if (*errp) {
 error_free(s->migration_blocker);
 goto fail_free_bmap;
 }
@@ -735,6 +734,7 @@ nonallocating_write:
 static int coroutine_fn vdi_co_do_create(BlockdevCreateOptions *create_options,
  size_t block_size, Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 BlockdevCreateOptionsVdi *vdi_opts;
 int ret = 0;
 uint64_t bytes = 0;
@@ -899,13 +899,13 @@ static int coroutine_fn 
vdi_co_create(BlockdevCreateOptions *create_options,
 static int coroutine_fn vdi_co_create_opts(const char *filename, QemuOpts 
*opts,
Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 QDict *qdict = NULL;
 BlockdevCreateOptions *create_options = NULL;
 BlockDriverState *bs_file = NULL;
 uint64_t block_size = DEFAULT_CLUSTER_SIZE;
 bool is_static = false;
 Visitor *v;
-Error *local_err = NULL;
 int ret;
 
 /* Parse options and convert legacy syntax.
@@ -956,11 +956,10 @@ static int coroutine_fn vdi_co_create_opts(const char 
*filename, QemuOpts *opts,
 ret = -EINVAL;
 goto done;
 }
-visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
+visit_type_BlockdevCreateOptions(v, NULL, &create_options, errp);
 visit_free(v);
 
-if (local_err) {
-error_propagate(errp, local_err);
+if (*errp) {
 ret = -EINVAL;
 goto done;
 }
-- 
2.21.0

[RFC v5 099/126] nbd: introduce ERRP_AUTO_PROPAGATE

[Vladimir Sementsov-Ogievskiy]

If we want to add some info to errp (by error_prepend() or
error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
Otherwise, this info will not be added when errp == &fatal_err
(the program will exit prior to the error_append_hint() or
error_prepend() call).  Fix such cases.

If we want to check error after errp-function call, we need to
introduce local_err and than propagate it to errp. Instead, use
ERRP_AUTO_PROPAGATE macro, benefits are:
1. No need of explicit error_propagate call
2. No need of explicit local_err variable: use errp directly
3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
   &error_fatel, this means that we don't break error_abort
   (we'll abort on error_set, not on error_propagate)

This commit (together with its neighbors) was generated by

for f in $(git grep -l errp \*.[ch]); do \
spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
--macro-file scripts/cocci-macro-file.h --in-place --no-show-diff $f; \
done;

then fix a bit of compilation problems: coccinelle for some reason
leaves several
f() {
...
goto out;
...
out:
}
patterns, with "out:" at function end.

then
./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"

(auto-msg was a file with this commit message)

Still, for backporting it may be more comfortable to use only the first
command and then do one huge commit.

Reported-by: Kevin Wolf 
Reported-by: Greg Kurz 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 include/block/nbd.h |  1 +
 block/nbd.c | 49 +
 nbd/client.c|  5 +
 nbd/server.c|  5 +
 4 files changed, 34 insertions(+), 26 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 316fd705a9..330f40142a 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -360,6 +360,7 @@ void nbd_server_start(SocketAddress *addr, const char 
*tls_creds,
 static inline int nbd_read(QIOChannel *ioc, void *buffer, size_t size,
const char *desc, Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 int ret = qio_channel_read_all(ioc, buffer, size, errp) < 0 ? -EIO : 0;
 
 if (ret < 0) {
diff --git a/block/nbd.c b/block/nbd.c
index c66fdf54b9..2f8a924562 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -805,10 +805,10 @@ static int nbd_co_receive_cmdread_reply(BDRVNBDState *s, 
uint64_t handle,
 uint64_t offset, QEMUIOVector *qiov,
 int *request_ret, Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 NBDReplyChunkIter iter;
 NBDReply reply;
 void *payload = NULL;
-Error *local_err = NULL;
 
 NBD_FOREACH_REPLY_CHUNK(s, iter, handle, s->info.structured_reply,
 qiov, &reply, &payload)
@@ -827,20 +827,20 @@ static int nbd_co_receive_cmdread_reply(BDRVNBDState *s, 
uint64_t handle,
 break;
 case NBD_REPLY_TYPE_OFFSET_HOLE:
 ret = nbd_parse_offset_hole_payload(s, &reply.structured, payload,
-offset, qiov, &local_err);
+offset, qiov, errp);
 if (ret < 0) {
 nbd_channel_error(s, ret);
-nbd_iter_channel_error(&iter, ret, &local_err);
+nbd_iter_channel_error(&iter, ret, errp);
 }
 break;
 default:
 if (!nbd_reply_type_is_error(chunk->type)) {
 /* not allowed reply type */
 nbd_channel_error(s, -EINVAL);
-error_setg(&local_err,
+error_setg(errp,
"Unexpected reply type: %d (%s) for CMD_READ",
chunk->type, nbd_reply_type_lookup(chunk->type));
-nbd_iter_channel_error(&iter, -EINVAL, &local_err);
+nbd_iter_channel_error(&iter, -EINVAL, errp);
 }
 }
 
@@ -858,10 +858,10 @@ static int nbd_co_receive_blockstatus_reply(BDRVNBDState 
*s,
 NBDExtent *extent,
 int *request_ret, Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 NBDReplyChunkIter iter;
 NBDReply reply;
 void *payload = NULL;
-Error *local_err = NULL;
 bool received = false;
 
 assert(!extent->length);
@@ -875,27 +875,27 @@ static int nbd_co_receive_blockstatus_reply(BDRVNBDState 
*s,
 case NBD_REPLY_TYPE_BLOCK_STATUS:
 if (received) {
 nbd_channel_error(s, -EINVAL);
-error_setg(&local_err, "Several BLOCK_STATUS chunks in reply");
-nbd_iter_channel_error(&iter, -EINVAL, &local_err);
+error_setg(errp, "Several BLOCK_STATUS chunks in reply");
+nbd_iter_channel_error(&iter, -EINVAL, errp);
 }
 received = true;
 
 ret = nbd_parse_blockstatus_payload

[RFC v5 095/126] Sheepdog: introduce ERRP_AUTO_PROPAGATE

[Vladimir Sementsov-Ogievskiy]

If we want to add some info to errp (by error_prepend() or
error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
Otherwise, this info will not be added when errp == &fatal_err
(the program will exit prior to the error_append_hint() or
error_prepend() call).  Fix such cases.

If we want to check error after errp-function call, we need to
introduce local_err and than propagate it to errp. Instead, use
ERRP_AUTO_PROPAGATE macro, benefits are:
1. No need of explicit error_propagate call
2. No need of explicit local_err variable: use errp directly
3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
   &error_fatel, this means that we don't break error_abort
   (we'll abort on error_set, not on error_propagate)

This commit (together with its neighbors) was generated by

for f in $(git grep -l errp \*.[ch]); do \
spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
--macro-file scripts/cocci-macro-file.h --in-place --no-show-diff $f; \
done;

then fix a bit of compilation problems: coccinelle for some reason
leaves several
f() {
...
goto out;
...
out:
}
patterns, with "out:" at function end.

then
./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"

(auto-msg was a file with this commit message)

Still, for backporting it may be more comfortable to use only the first
command and then do one huge commit.

Reported-by: Kevin Wolf 
Reported-by: Greg Kurz 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/sheepdog.c | 73 ++--
 1 file changed, 33 insertions(+), 40 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 773dfc6ab1..396ec1be28 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -529,10 +529,10 @@ static void sd_aio_setup(SheepdogAIOCB *acb, 
BDRVSheepdogState *s,
 
 static SocketAddress *sd_server_config(QDict *options, Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 QDict *server = NULL;
 Visitor *iv = NULL;
 SocketAddress *saddr = NULL;
-Error *local_err = NULL;
 
 qdict_extract_subqdict(options, &server, "server.");
 
@@ -541,9 +541,8 @@ static SocketAddress *sd_server_config(QDict *options, 
Error **errp)
 goto done;
 }
 
-visit_type_SocketAddress(iv, NULL, &saddr, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
+visit_type_SocketAddress(iv, NULL, &saddr, errp);
+if (*errp) {
 goto done;
 }
 
@@ -1008,7 +1007,7 @@ static void sd_config_done(SheepdogConfig *cfg)
 static void sd_parse_uri(SheepdogConfig *cfg, const char *filename,
  Error **errp)
 {
-Error *err = NULL;
+ERRP_AUTO_PROPAGATE();
 QueryParams *qp = NULL;
 bool is_unix;
 URI *uri;
@@ -1017,7 +1016,7 @@ static void sd_parse_uri(SheepdogConfig *cfg, const char 
*filename,
 
 cfg->uri = uri = uri_parse(filename);
 if (!uri) {
-error_setg(&err, "invalid URI '%s'", filename);
+error_setg(errp, "invalid URI '%s'", filename);
 goto out;
 }
 
@@ -1029,18 +1028,18 @@ static void sd_parse_uri(SheepdogConfig *cfg, const 
char *filename,
 } else if (!g_strcmp0(uri->scheme, "sheepdog+unix")) {
 is_unix = true;
 } else {
-error_setg(&err, "URI scheme must be 'sheepdog', 'sheepdog+tcp',"
+error_setg(errp, "URI scheme must be 'sheepdog', 'sheepdog+tcp',"
" or 'sheepdog+unix'");
 goto out;
 }
 
 if (uri->path == NULL || !strcmp(uri->path, "/")) {
-error_setg(&err, "missing file path in URI");
+error_setg(errp, "missing file path in URI");
 goto out;
 }
 if (g_strlcpy(cfg->vdi, uri->path + 1, SD_MAX_VDI_LEN)
 >= SD_MAX_VDI_LEN) {
-error_setg(&err, "VDI name is too long");
+error_setg(errp, "VDI name is too long");
 goto out;
 }
 
@@ -1049,25 +1048,25 @@ static void sd_parse_uri(SheepdogConfig *cfg, const 
char *filename,
 if (is_unix) {
 /* sheepdog+unix:///vdiname?socket=path */
 if (uri->server || uri->port) {
-error_setg(&err, "URI scheme %s doesn't accept a server address",
+error_setg(errp, "URI scheme %s doesn't accept a server address",
uri->scheme);
 goto out;
 }
 if (!qp->n) {
-error_setg(&err,
+error_setg(errp,
"URI scheme %s requires query parameter 'socket'",
uri->scheme);
 goto out;
 }
 if (qp->n != 1 || strcmp(qp->p[0].name, "socket")) {
-error_setg(&err, "unexpected query parameters");
+error_setg(errp, "unexpected query parameters");
 goto out;
 }
 cfg->path = qp->p[0].value;
 } else {
 /* sheepdog[+tcp]://[host:port]/vdiname */
 if (qp->n) {
-error_setg(&err, "unexpected query parameters");
+error_setg(errp, "unexpected query par

[RFC v5 024/126] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
functions with errp OUT parameter.

It has three goals:

1. Fix issue with error_fatal & error_prepend/error_append_hint: user
can't see this additional information, because exit() happens in
error_setg earlier than information is added. [Reported by Greg Kurz]

2. Fix issue with error_abort & error_propagate: when we wrap
error_abort by local_err+error_propagate, resulting coredump will
refer to error_propagate and not to the place where error happened.
(the macro itself doesn't fix the issue, but it allows to [3.] drop all
local_err+error_propagate pattern, which will definitely fix the issue)
[Reported by Kevin Wolf]

3. Drop local_err+error_propagate pattern, which is used to workaround
void functions with errp parameter, when caller wants to know resulting
status. (Note: actually these functions could be merely updated to
return int error code).

To achieve these goals, we need to add invocation of the macro at start
of functions, which needs error_prepend/error_append_hint (1.); add
invocation of the macro at start of functions which do
local_err+error_propagate scenario the check errors, drop local errors
from them and just use *errp instead (2., 3.).

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
---

CC: Gerd Hoffmann 
CC: "Gonglei (Arei)" 
CC: Eduardo Habkost 
CC: Igor Mammedov 
CC: Laurent Vivier 
CC: Amit Shah 
CC: Kevin Wolf 
CC: Max Reitz 
CC: John Snow 
CC: Ari Sundholm 
CC: Pavel Dovgalyuk 
CC: Paolo Bonzini 
CC: Stefan Hajnoczi 
CC: Fam Zheng 
CC: Stefan Weil 
CC: Ronnie Sahlberg 
CC: Peter Lieven 
CC: Eric Blake 
CC: "Denis V. Lunev" 
CC: Markus Armbruster 
CC: Alberto Garcia 
CC: Jason Dillaman 
CC: Wen Congyang 
CC: Xie Changlong 
CC: Liu Yuan 
CC: "Richard W.M. Jones" 
CC: Jeff Cody 
CC: "Marc-André Lureau" 
CC: "Daniel P. Berrangé" 
CC: Richard Henderson 
CC: Greg Kurz 
CC: "Michael S. Tsirkin" 
CC: Marcel Apfelbaum 
CC: Beniamino Galvani 
CC: Peter Maydell 
CC: "Cédric Le Goater" 
CC: Andrew Jeffery 
CC: Joel Stanley 
CC: Andrew Baumann 
CC: "Philippe Mathieu-Daudé" 
CC: Antony Pavlov 
CC: Jean-Christophe Dubois 
CC: Peter Chubb 
CC: Subbaraya Sundeep 
CC: Eric Auger 
CC: Alistair Francis 
CC: "Edgar E. Iglesias" 
CC: Stefano Stabellini 
CC: Anthony Perard 
CC: Paul Durrant 
CC: Paul Burton 
CC: Aleksandar Rikalo 
CC: Chris Wulff 
CC: Marek Vasut 
CC: David Gibson 
CC: Cornelia Huck 
CC: Halil Pasic 
CC: Christian Borntraeger 
CC: "Hervé Poussineau" 
CC: Xiao Guangrong 
CC: Aurelien Jarno 
CC: Aleksandar Markovic 
CC: Mark Cave-Ayland 
CC: Jason Wang 
CC: Laszlo Ersek 
CC: Yuval Shaia 
CC: Palmer Dabbelt 
CC: Sagar Karandikar 
CC: Bastian Koppelmann 
CC: David Hildenbrand 
CC: Thomas Huth 
CC: Eric Farman 
CC: Matthew Rosato 
CC: Hannes Reinecke 
CC: Michael Walle 
CC: Artyom Tarasenko 
CC: Stefan Berger 
CC: Samuel Thibault 
CC: Alex Williamson 
CC: Tony Krowiak 
CC: Pierre Morel 
CC: Michael Roth 
CC: Hailiang Zhang 
CC: Juan Quintela 
CC: "Dr. David Alan Gilbert" 
CC: Luigi Rizzo 
CC: Giuseppe Lettieri 
CC: Vincenzo Maffione 
CC: Jan Kiszka 
CC: Anthony Green 
CC: Stafford Horne 
CC: Guan Xuetao 
CC: Max Filippov 
CC: qemu-block@nongnu.org
CC: integrat...@gluster.org
CC: sheep...@lists.wpkg.org
CC: qemu-...@nongnu.org
CC: xen-de...@lists.xenproject.org
CC: qemu-...@nongnu.org
CC: qemu-s3...@nongnu.org
CC: qemu-ri...@nongnu.org

 include/qapi/error.h | 38 ++
 1 file changed, 38 insertions(+)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index d6898d833b..47238d9065 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -345,6 +345,44 @@ void error_set_internal(Error **errp,
 ErrorClass err_class, const char *fmt, ...)
 GCC_FMT_ATTR(6, 7);
 
+typedef struct ErrorPropagator {
+Error *local_err;
+Error **errp;
+} ErrorPropagator;
+
+static inline void error_propagator_cleanup(ErrorPropagator *prop)
+{
+error_propagate(prop->errp, prop->local_err);
+}
+
+G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
+
+/*
+ * ERRP_AUTO_PROPAGATE
+ *
+ * This macro is created to be the first line of a function with Error **errp
+ * OUT parameter. It's needed only in cases where we want to use error_prepend,
+ * error_append_hint or dereference *errp. It's still safe (but useless) in
+ * other cases.
+ *
+ * If errp is NULL or points to error_fatal, it is rewritten to point to a
+ * local Error object, which will be automatically propagated to the original
+ * errp on function exit (see error_propagator_cleanup).
+ *
+ * After invocation of this macro it is always safe to dereference errp
+ * (as it's not NULL anymore) and to add information (by error_prepend or
+ * error_append_hint)
+ * (as, if it was error_fatal, we swapped it with a local_error to be
+ * propagated on cleanup).
+ *
+ * Note: we don't wrap the error_abort case, as we want resulting coredump
+ * to point to the place w

Re: [PATCH v6 4/4] colo: Update Documentation for continuous replication

[Lukas Straub]

On Thu, 10 Oct 2019 10:34:15 +
"Zhang, Chen"  wrote:

> > -Original Message-
> > From: Lukas Straub 
> > Sent: Wednesday, October 9, 2019 11:17 PM
> > To: Zhang, Chen 
> > Cc: qemu-devel ; Jason Wang
> > ; Wen Congyang ;
> > Xie Changlong ; Kevin Wolf
> > ; Max Reitz ; qemu-block
> > 
> > Subject: Re: [PATCH v6 4/4] colo: Update Documentation for continuous
> > replication
> > 
> > On Wed, 9 Oct 2019 08:36:52 +
> > "Zhang, Chen"  wrote:
> >   
> > > > -Original Message-
> > > > From: Lukas Straub 
> > > > Sent: Saturday, October 5, 2019 9:06 PM
> > > > To: qemu-devel 
> > > > Cc: Zhang, Chen ; Jason Wang
> > > > ; Wen Congyang  
> > ; Xie  
> > > > Changlong ; Kevin Wolf  
> > ;  
> > > > Max Reitz ; qemu-block  > bl...@nongnu.org>  
> > > > Subject: [PATCH v6 4/4] colo: Update Documentation for continuous
> > > > replication
> > > >
> > > > Document the qemu command-line and qmp commands for continuous
> > > > replication
> > > >
> > > > Signed-off-by: Lukas Straub 
> > > > ---
> > > >  docs/COLO-FT.txt   | 213 +++--
> > > >  docs/block-replication.txt |  28 +++--
> > > >  2 files changed, 174 insertions(+), 67 deletions(-)
> > > >
> > > > diff --git a/docs/COLO-FT.txt b/docs/COLO-FT.txt index
> > > > ad24680d13..bc1a0ccb99 100644
> > > > --- a/docs/COLO-FT.txt
> > > > +++ b/docs/COLO-FT.txt
> > > > @@ -145,35 +145,65 @@ The diagram just shows the main qmp  
> > command,  
> > > > you can get the detail  in test procedure.
> > > >
> > > > ...
> > > >
> > > > +Note: Here we are running both instances on the same Host for
> > > > +testing, change the IP Addresses if you want to run it on two
> > > > +Hosts. Initally
> > > > +127.0.0.1 is the Primary Host and 127.0.0.2 is the Secondary Host.
> > > > +
> > > > +== Startup qemu ==
> > > > +1. Primary:
> > > > +Note: Initally, $imagefolder/primary.qcow2 needs to be copied to all  
> > Hosts.  
> > > > +# imagefolder="/mnt/vms/colo-test-primary"
> > > > +
> > > > +# qemu-system-x86_64 -enable-kvm -cpu qemu64,+kvmclock -m 512 -  
> > smp  
> > > > 1 -qmp stdio \
> > > > +   -device piix3-usb-uhci -device usb-tablet -name primary \
> > > > +   -netdev
> > > > + tap,id=hn0,vhost=off,helper=/usr/lib/qemu/qemu-bridge-helper
> > > > \
> > > > +   -device rtl8139,id=e0,netdev=hn0 \
> > > > +   -chardev socket,id=mirror0,host=0.0.0.0,port=9003,server,nowait \
> > > > +   -chardev socket,id=compare1,host=0.0.0.0,port=9004,server,wait \  
> > >
> > > We should change the host=127.0.0.1 consistent with the expression below. 
> > >  
> > 
> > Hi,
> > This (and the IPs below in the QMP commands) needs to be this way,
> > because it's a listening port and with 127.0.0.1 it would only listen on the
> > loopback ip and wouldn't be reachable from another node for example. With
> > 0.0.0.0 it will listen on all Interfaces.  
> 
> Yes, I know.  For this command demo, maybe use 192.168.0.1/192.168.0.2 are 
> more clear.

Hmm,
the compare0 and compare_out actually can be replaced by unix sockets. 
So what do you think about the following?

   -chardev socket,id=mirror0,host=127.0.0.1,port=9003,server,nowait \
   -chardev socket,id=compare1,host=127.0.0.1,port=9004,server,wait \
   -chardev socket,id=compare0,path=/tmp/compare0.sock,server,nowait \
   -chardev socket,id=compare0-0,path=/tmp/compare0.sock \
   -chardev socket,id=compare_out,path=/tmp/compare_out.sock,server,nowait \
   -chardev socket,id=compare_out0,path=/tmp/compare_out.sock \

> >   
> > > > +   -chardev socket,id=compare0,host=127.0.0.1,port=9001,server,nowait  
> > \  
> > > > +   -chardev socket,id=compare0-0,host=127.0.0.1,port=9001 \
> > > > +   -chardev
> > > > + socket,id=compare_out,host=127.0.0.1,port=9005,server,nowait
> > > > \
> > > > +   -chardev socket,id=compare_out0,host=127.0.0.1,port=9005 \
> > > > +   -object filter-mirror,id=m0,netdev=hn0,queue=tx,outdev=mirror0 \
> > > > +   -object filter-
> > > > redirector,netdev=hn0,id=redire0,queue=rx,indev=compare_out \
> > > > +   -object filter-
> > > > redirector,netdev=hn0,id=redire1,queue=rx,outdev=compare0 \
> > > > +   -object iothread,id=iothread1 \
> > > > +   -object
> > > > +colo-compare,id=comp0,primary_in=compare0-
> > > > 0,secondary_in=compare1,\
> > > > +outdev=compare_out0,iothread=iothread1 \
> > > > +   -drive
> > > > +if=ide,id=colo-disk0,driver=quorum,read-pattern=fifo,vote-threshold
> > > > +=1,\
> > > > +children.0.file.filename=$imagefolder/primary.qcow2,children.0.driv
> > > > +er=q
> > > > +cow2 -S
> > > > +
> > > > +2. Secondary:
> > > > +# imagefolder="/mnt/vms/colo-test-secondary"
> > > > +# primary_ip=127.0.0.1
> > > > +
> > > > +# qemu-img create -f qcow2 $imagefolder/secondary-active.qcow2 10G
> > > > +
> > > > +# qemu-img create -f qcow2 $imagefolder/secondary-hidden.qcow2  
> > 10G  
> > > > +  
> > >
> > > The active disk and hidden disk just need create one time, we can note 
> > > that  
> > here.
> > 
> > Ok, I will Note that. But I will wait until the bl

[RFC v5 107/126] blklogwrites: introduce ERRP_AUTO_PROPAGATE

[Vladimir Sementsov-Ogievskiy]

If we want to add some info to errp (by error_prepend() or
error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
Otherwise, this info will not be added when errp == &fatal_err
(the program will exit prior to the error_append_hint() or
error_prepend() call).  Fix such cases.

If we want to check error after errp-function call, we need to
introduce local_err and than propagate it to errp. Instead, use
ERRP_AUTO_PROPAGATE macro, benefits are:
1. No need of explicit error_propagate call
2. No need of explicit local_err variable: use errp directly
3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
   &error_fatel, this means that we don't break error_abort
   (we'll abort on error_set, not on error_propagate)

This commit (together with its neighbors) was generated by

for f in $(git grep -l errp \*.[ch]); do \
spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
--macro-file scripts/cocci-macro-file.h --in-place --no-show-diff $f; \
done;

then fix a bit of compilation problems: coccinelle for some reason
leaves several
f() {
...
goto out;
...
out:
}
patterns, with "out:" at function end.

then
./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"

(auto-msg was a file with this commit message)

Still, for backporting it may be more comfortable to use only the first
command and then do one huge commit.

Reported-by: Kevin Wolf 
Reported-by: Greg Kurz 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/blklogwrites.c | 22 +-
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/block/blklogwrites.c b/block/blklogwrites.c
index 04d8b33607..841cfeef95 100644
--- a/block/blklogwrites.c
+++ b/block/blklogwrites.c
@@ -141,36 +141,33 @@ static uint64_t 
blk_log_writes_find_cur_log_sector(BdrvChild *log,
 static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 BDRVBlkLogWritesState *s = bs->opaque;
 QemuOpts *opts;
-Error *local_err = NULL;
 int ret;
 uint64_t log_sector_size;
 bool log_append;
 
 opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
-qemu_opts_absorb_qdict(opts, options, &local_err);
-if (local_err) {
+qemu_opts_absorb_qdict(opts, options, errp);
+if (*errp) {
 ret = -EINVAL;
-error_propagate(errp, local_err);
 goto fail;
 }
 
 /* Open the file */
 bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, false,
-   &local_err);
-if (local_err) {
+   errp);
+if (*errp) {
 ret = -EINVAL;
-error_propagate(errp, local_err);
 goto fail;
 }
 
 /* Open the log file */
 s->log_file = bdrv_open_child(NULL, options, "log", bs, &child_file, false,
-  &local_err);
-if (local_err) {
+  errp);
+if (*errp) {
 ret = -EINVAL;
-error_propagate(errp, local_err);
 goto fail;
 }
 
@@ -220,10 +217,9 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict 
*options, int flags,
 if (blk_log_writes_sector_size_valid(log_sector_size)) {
 s->cur_log_sector =
 blk_log_writes_find_cur_log_sector(s->log_file, 
log_sector_size,
-le64_to_cpu(log_sb.nr_entries), 
&local_err);
-if (local_err) {
+le64_to_cpu(log_sb.nr_entries), errp);
+if (*errp) {
 ret = -EINVAL;
-error_propagate(errp, local_err);
 goto fail_log;
 }
 
-- 
2.21.0

[RFC v5 111/126] raw: introduce ERRP_AUTO_PROPAGATE

[Vladimir Sementsov-Ogievskiy]

If we want to add some info to errp (by error_prepend() or
error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
Otherwise, this info will not be added when errp == &fatal_err
(the program will exit prior to the error_append_hint() or
error_prepend() call).  Fix such cases.

If we want to check error after errp-function call, we need to
introduce local_err and than propagate it to errp. Instead, use
ERRP_AUTO_PROPAGATE macro, benefits are:
1. No need of explicit error_propagate call
2. No need of explicit local_err variable: use errp directly
3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
   &error_fatel, this means that we don't break error_abort
   (we'll abort on error_set, not on error_propagate)

This commit (together with its neighbors) was generated by

for f in $(git grep -l errp \*.[ch]); do \
spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
--macro-file scripts/cocci-macro-file.h --in-place --no-show-diff $f; \
done;

then fix a bit of compilation problems: coccinelle for some reason
leaves several
f() {
...
goto out;
...
out:
}
patterns, with "out:" at function end.

then
./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"

(auto-msg was a file with this commit message)

Still, for backporting it may be more comfortable to use only the first
command and then do one huge commit.

Reported-by: Kevin Wolf 
Reported-by: Greg Kurz 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/file-posix.c | 79 --
 block/file-win32.c | 29 +++--
 block/raw-format.c |  7 ++--
 3 files changed, 50 insertions(+), 65 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index f12c06de2d..fa75232713 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -320,6 +320,7 @@ static bool raw_is_io_aligned(int fd, void *buf, size_t len)
 
 static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 BDRVRawState *s = bs->opaque;
 char *buf;
 size_t max_align = MAX(MAX_BLOCKSIZE, getpagesize());
@@ -473,9 +474,9 @@ static int raw_open_common(BlockDriverState *bs, QDict 
*options,
int bdrv_flags, int open_flags,
bool device, Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 BDRVRawState *s = bs->opaque;
 QemuOpts *opts;
-Error *local_err = NULL;
 const char *filename = NULL;
 const char *str;
 BlockdevAioOptions aio, aio_default;
@@ -484,9 +485,8 @@ static int raw_open_common(BlockDriverState *bs, QDict 
*options,
 OnOffAuto locking;
 
 opts = qemu_opts_create(&raw_runtime_opts, NULL, 0, &error_abort);
-qemu_opts_absorb_qdict(opts, options, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
+qemu_opts_absorb_qdict(opts, options, errp);
+if (*errp) {
 ret = -EINVAL;
 goto fail;
 }
@@ -503,9 +503,8 @@ static int raw_open_common(BlockDriverState *bs, QDict 
*options,
   : BLOCKDEV_AIO_OPTIONS_THREADS;
 aio = qapi_enum_parse(&BlockdevAioOptions_lookup,
   qemu_opt_get(opts, "aio"),
-  aio_default, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
+  aio_default, errp);
+if (*errp) {
 ret = -EINVAL;
 goto fail;
 }
@@ -513,9 +512,8 @@ static int raw_open_common(BlockDriverState *bs, QDict 
*options,
 
 locking = qapi_enum_parse(&OnOffAuto_lookup,
   qemu_opt_get(opts, "locking"),
-  ON_OFF_AUTO_AUTO, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
+  ON_OFF_AUTO_AUTO, errp);
+if (*errp) {
 ret = -EINVAL;
 goto fail;
 }
@@ -541,9 +539,8 @@ static int raw_open_common(BlockDriverState *bs, QDict 
*options,
 
 str = qemu_opt_get(opts, "pr-manager");
 if (str) {
-s->pr_mgr = pr_manager_lookup(str, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
+s->pr_mgr = pr_manager_lookup(str, errp);
+if (*errp) {
 ret = -EINVAL;
 goto fail;
 }
@@ -817,9 +814,9 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
 uint64_t new_perm, uint64_t new_shared,
 Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 BDRVRawState *s = bs->opaque;
 int ret = 0;
-Error *local_err = NULL;
 
 if (!s->use_lock) {
 return 0;
@@ -859,22 +856,22 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
 /* fall through to unlock bytes. */
 case RAW_PL_ABORT:
 raw_apply_lock_bytes(s, s->fd, s->perm, ~s->shared_perm,
- true, &local_err);
-if (local_err) {
+ true, errp);
+   

[RFC v5 108/126] blkverify: introduce ERRP_AUTO_PROPAGATE

[Vladimir Sementsov-Ogievskiy]

If we want to add some info to errp (by error_prepend() or
error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
Otherwise, this info will not be added when errp == &fatal_err
(the program will exit prior to the error_append_hint() or
error_prepend() call).  Fix such cases.

If we want to check error after errp-function call, we need to
introduce local_err and than propagate it to errp. Instead, use
ERRP_AUTO_PROPAGATE macro, benefits are:
1. No need of explicit error_propagate call
2. No need of explicit local_err variable: use errp directly
3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
   &error_fatel, this means that we don't break error_abort
   (we'll abort on error_set, not on error_propagate)

This commit (together with its neighbors) was generated by

for f in $(git grep -l errp \*.[ch]); do \
spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
--macro-file scripts/cocci-macro-file.h --in-place --no-show-diff $f; \
done;

then fix a bit of compilation problems: coccinelle for some reason
leaves several
f() {
...
goto out;
...
out:
}
patterns, with "out:" at function end.

then
./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"

(auto-msg was a file with this commit message)

Still, for backporting it may be more comfortable to use only the first
command and then do one huge commit.

Reported-by: Kevin Wolf 
Reported-by: Greg Kurz 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/blkverify.c | 17 +++--
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/block/blkverify.c b/block/blkverify.c
index 304b0a1368..96e54f8439 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -110,35 +110,32 @@ static QemuOptsList runtime_opts = {
 static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
   Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 BDRVBlkverifyState *s = bs->opaque;
 QemuOpts *opts;
-Error *local_err = NULL;
 int ret;
 
 opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
-qemu_opts_absorb_qdict(opts, options, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
+qemu_opts_absorb_qdict(opts, options, errp);
+if (*errp) {
 ret = -EINVAL;
 goto fail;
 }
 
 /* Open the raw file */
 bs->file = bdrv_open_child(qemu_opt_get(opts, "x-raw"), options, "raw",
-   bs, &child_file, false, &local_err);
-if (local_err) {
+   bs, &child_file, false, errp);
+if (*errp) {
 ret = -EINVAL;
-error_propagate(errp, local_err);
 goto fail;
 }
 
 /* Open the test file */
 s->test_file = bdrv_open_child(qemu_opt_get(opts, "x-image"), options,
"test", bs, &child_format, false,
-   &local_err);
-if (local_err) {
+   errp);
+if (*errp) {
 ret = -EINVAL;
-error_propagate(errp, local_err);
 goto fail;
 }
 
-- 
2.21.0

Re: [PATCH v3 04/16] qcow2: Keep unknown extra snapshot data

[Eric Blake]

On 10/11/19 10:28 AM, Max Reitz wrote:

The qcow2 specification says to ignore unknown extra data fields in
snapshot table entries.  Currently, we discard it whenever we update the
image, which is a bit different from "ignore".

This patch makes the qcow2 driver keep all unknown extra data fields
when updating an image's snapshot table.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---



  /* Bitmap header extension constraints */
  #define QCOW2_MAX_BITMAPS 65535
  #define QCOW2_MAX_BITMAP_DIRECTORY_SIZE (1024 * QCOW2_MAX_BITMAPS)
@@ -181,6 +184,8 @@ typedef struct QCowSnapshot {
  uint32_t date_sec;
  uint32_t date_nsec;
  uint64_t vm_clock_nsec;
+uint32_t extra_data_size;
+void *unknown_extra_data; /* Extra data past QCowSnapshotExtraData */


Would it be worth a comment change:

uint32_t extra_data_size; /* Size of all extra data, including 
QCowSnapshotExtraData */

void *unknown_extra_data; /* Data beyond QCowSnapshotExtraData, if any */

Either way, R-b stands.

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

[RFC v5 045/126] pflash: introduce ERRP_AUTO_PROPAGATE

[Vladimir Sementsov-Ogievskiy]

If we want to add some info to errp (by error_prepend() or
error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
Otherwise, this info will not be added when errp == &fatal_err
(the program will exit prior to the error_append_hint() or
error_prepend() call).  Fix such cases.

If we want to check error after errp-function call, we need to
introduce local_err and than propagate it to errp. Instead, use
ERRP_AUTO_PROPAGATE macro, benefits are:
1. No need of explicit error_propagate call
2. No need of explicit local_err variable: use errp directly
3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
   &error_fatel, this means that we don't break error_abort
   (we'll abort on error_set, not on error_propagate)

This commit (together with its neighbors) was generated by

for f in $(git grep -l errp \*.[ch]); do \
spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
--macro-file scripts/cocci-macro-file.h --in-place --no-show-diff $f; \
done;

then fix a bit of compilation problems: coccinelle for some reason
leaves several
f() {
...
goto out;
...
out:
}
patterns, with "out:" at function end.

then
./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"

(auto-msg was a file with this commit message)

Still, for backporting it may be more comfortable to use only the first
command and then do one huge commit.

Reported-by: Kevin Wolf 
Reported-by: Greg Kurz 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 hw/block/pflash_cfi01.c | 7 +++
 hw/block/pflash_cfi02.c | 7 +++
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 566c0acb77..37571b6efb 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -700,12 +700,12 @@ static const MemoryRegionOps pflash_cfi01_ops = {
 
 static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 PFlashCFI01 *pfl = PFLASH_CFI01(dev);
 uint64_t total_len;
 int ret;
 uint64_t blocks_per_device, sector_len_per_device, device_len;
 int num_devices;
-Error *local_err = NULL;
 
 if (pfl->sector_len == 0) {
 error_setg(errp, "attribute \"sector-length\" not specified or zero.");
@@ -739,9 +739,8 @@ static void pflash_cfi01_realize(DeviceState *dev, Error 
**errp)
 &pfl->mem, OBJECT(dev),
 &pflash_cfi01_ops,
 pfl,
-pfl->name, total_len, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
+pfl->name, total_len, errp);
+if (*errp) {
 return;
 }
 
diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 4baca701b7..9dcdc13289 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -719,9 +719,9 @@ static const MemoryRegionOps pflash_cfi02_ops = {
 
 static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 PFlashCFI02 *pfl = PFLASH_CFI02(dev);
 int ret;
-Error *local_err = NULL;
 
 if (pfl->uniform_sector_len == 0 && pfl->sector_len[0] == 0) {
 error_setg(errp, "attribute \"sector-length\" not specified or zero.");
@@ -787,9 +787,8 @@ static void pflash_cfi02_realize(DeviceState *dev, Error 
**errp)
 
 memory_region_init_rom_device(&pfl->orig_mem, OBJECT(pfl),
   &pflash_cfi02_ops, pfl, pfl->name,
-  pfl->chip_len, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
+  pfl->chip_len, errp);
+if (*errp) {
 return;
 }
 
-- 
2.21.0

[RFC v5 103/126] GLUSTER: introduce ERRP_AUTO_PROPAGATE

[Vladimir Sementsov-Ogievskiy]

If we want to add some info to errp (by error_prepend() or
error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
Otherwise, this info will not be added when errp == &fatal_err
(the program will exit prior to the error_append_hint() or
error_prepend() call).  Fix such cases.

If we want to check error after errp-function call, we need to
introduce local_err and than propagate it to errp. Instead, use
ERRP_AUTO_PROPAGATE macro, benefits are:
1. No need of explicit error_propagate call
2. No need of explicit local_err variable: use errp directly
3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
   &error_fatel, this means that we don't break error_abort
   (we'll abort on error_set, not on error_propagate)

This commit (together with its neighbors) was generated by

for f in $(git grep -l errp \*.[ch]); do \
spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
--macro-file scripts/cocci-macro-file.h --in-place --no-show-diff $f; \
done;

then fix a bit of compilation problems: coccinelle for some reason
leaves several
f() {
...
goto out;
...
out:
}
patterns, with "out:" at function end.

then
./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"

(auto-msg was a file with this commit message)

Still, for backporting it may be more comfortable to use only the first
command and then do one huge commit.

Reported-by: Kevin Wolf 
Reported-by: Greg Kurz 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/gluster.c | 69 -
 1 file changed, 34 insertions(+), 35 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index 64028b2cba..683101612e 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -419,6 +419,7 @@ out:
 static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 struct glfs *glfs;
 int ret;
 int old_errno;
@@ -512,38 +513,38 @@ out:
 static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
   QDict *options, Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 QemuOpts *opts;
 SocketAddress *gsconf = NULL;
 SocketAddressList *curr = NULL;
 QDict *backing_options = NULL;
-Error *local_err = NULL;
 char *str = NULL;
 const char *ptr;
 int i, type, num_servers;
 
 /* create opts info from runtime_json_opts list */
 opts = qemu_opts_create(&runtime_json_opts, NULL, 0, &error_abort);
-qemu_opts_absorb_qdict(opts, options, &local_err);
-if (local_err) {
+qemu_opts_absorb_qdict(opts, options, errp);
+if (*errp) {
 goto out;
 }
 
 num_servers = qdict_array_entries(options, GLUSTER_OPT_SERVER_PATTERN);
 if (num_servers < 1) {
-error_setg(&local_err, QERR_MISSING_PARAMETER, "server");
+error_setg(errp, QERR_MISSING_PARAMETER, "server");
 goto out;
 }
 
 ptr = qemu_opt_get(opts, GLUSTER_OPT_VOLUME);
 if (!ptr) {
-error_setg(&local_err, QERR_MISSING_PARAMETER, GLUSTER_OPT_VOLUME);
+error_setg(errp, QERR_MISSING_PARAMETER, GLUSTER_OPT_VOLUME);
 goto out;
 }
 gconf->volume = g_strdup(ptr);
 
 ptr = qemu_opt_get(opts, GLUSTER_OPT_PATH);
 if (!ptr) {
-error_setg(&local_err, QERR_MISSING_PARAMETER, GLUSTER_OPT_PATH);
+error_setg(errp, QERR_MISSING_PARAMETER, GLUSTER_OPT_PATH);
 goto out;
 }
 gconf->path = g_strdup(ptr);
@@ -555,15 +556,15 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster 
*gconf,
 
 /* create opts info from runtime_type_opts list */
 opts = qemu_opts_create(&runtime_type_opts, NULL, 0, &error_abort);
-qemu_opts_absorb_qdict(opts, backing_options, &local_err);
-if (local_err) {
+qemu_opts_absorb_qdict(opts, backing_options, errp);
+if (*errp) {
 goto out;
 }
 
 ptr = qemu_opt_get(opts, GLUSTER_OPT_TYPE);
 if (!ptr) {
-error_setg(&local_err, QERR_MISSING_PARAMETER, GLUSTER_OPT_TYPE);
-error_append_hint(&local_err, GERR_INDEX_HINT, i);
+error_setg(errp, QERR_MISSING_PARAMETER, GLUSTER_OPT_TYPE);
+error_append_hint(errp, GERR_INDEX_HINT, i);
 goto out;
 
 }
@@ -574,10 +575,10 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster 
*gconf,
 type = qapi_enum_parse(&SocketAddressType_lookup, ptr, -1, NULL);
 if (type != SOCKET_ADDRESS_TYPE_INET
 && type != SOCKET_ADDRESS_TYPE_UNIX) {
-error_setg(&local_err,
+error_setg(errp,
"Parameter '%s' may be 'inet' or 'unix'",
GLUSTER_OPT_TYPE);
-error_append_hint(&local_err, GERR_INDEX_HINT, i);
+error_append_hint(errp, GERR_INDEX_HINT, i);
 goto out;
 }
 gsconf->type = type;
@@ -586,24 +587,24 @@ static

[RFC v5 113/126] qcow: introduce ERRP_AUTO_PROPAGATE

[Vladimir Sementsov-Ogievskiy]

If we want to add some info to errp (by error_prepend() or
error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
Otherwise, this info will not be added when errp == &fatal_err
(the program will exit prior to the error_append_hint() or
error_prepend() call).  Fix such cases.

If we want to check error after errp-function call, we need to
introduce local_err and than propagate it to errp. Instead, use
ERRP_AUTO_PROPAGATE macro, benefits are:
1. No need of explicit error_propagate call
2. No need of explicit local_err variable: use errp directly
3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
   &error_fatel, this means that we don't break error_abort
   (we'll abort on error_set, not on error_propagate)

This commit (together with its neighbors) was generated by

for f in $(git grep -l errp \*.[ch]); do \
spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
--macro-file scripts/cocci-macro-file.h --in-place --no-show-diff $f; \
done;

then fix a bit of compilation problems: coccinelle for some reason
leaves several
f() {
...
goto out;
...
out:
}
patterns, with "out:" at function end.

then
./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"

(auto-msg was a file with this commit message)

Still, for backporting it may be more comfortable to use only the first
command and then do one huge commit.

Reported-by: Kevin Wolf 
Reported-by: Greg Kurz 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow.c | 17 +++--
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index 5bdf72ba33..c58d3e36be 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -117,11 +117,11 @@ static QemuOptsList qcow_runtime_opts = {
 static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
  Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 BDRVQcowState *s = bs->opaque;
 unsigned int len, i, shift;
 int ret;
 QCowHeader header;
-Error *local_err = NULL;
 QCryptoBlockOpenOptions *crypto_opts = NULL;
 unsigned int cflags = 0;
 QDict *encryptopts = NULL;
@@ -314,9 +314,8 @@ static int qcow_open(BlockDriverState *bs, QDict *options, 
int flags,
 error_setg(&s->migration_blocker, "The qcow format used by node '%s' "
"does not support live migration",
bdrv_get_device_or_node_name(bs));
-ret = migrate_add_blocker(s->migration_blocker, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
+ret = migrate_add_blocker(s->migration_blocker, errp);
+if (*errp) {
 error_free(s->migration_blocker);
 goto fail;
 }
@@ -942,12 +941,12 @@ exit:
 static int coroutine_fn qcow_co_create_opts(const char *filename,
 QemuOpts *opts, Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 BlockdevCreateOptions *create_options = NULL;
 BlockDriverState *bs = NULL;
 QDict *qdict;
 Visitor *v;
 const char *val;
-Error *local_err = NULL;
 int ret;
 
 static const QDictRenames opt_renames[] = {
@@ -977,9 +976,8 @@ static int coroutine_fn qcow_co_create_opts(const char 
*filename,
 }
 
 /* Create and open the file (protocol layer) */
-ret = bdrv_create_file(filename, opts, &local_err);
+ret = bdrv_create_file(filename, opts, errp);
 if (ret < 0) {
-error_propagate(errp, local_err);
 goto fail;
 }
 
@@ -1000,11 +998,10 @@ static int coroutine_fn qcow_co_create_opts(const char 
*filename,
 goto fail;
 }
 
-visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
+visit_type_BlockdevCreateOptions(v, NULL, &create_options, errp);
 visit_free(v);
 
-if (local_err) {
-error_propagate(errp, local_err);
+if (*errp) {
 ret = -EINVAL;
 goto fail;
 }
-- 
2.21.0

[RFC v5 100/126] NFS: introduce ERRP_AUTO_PROPAGATE

[Vladimir Sementsov-Ogievskiy]

If we want to add some info to errp (by error_prepend() or
error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
Otherwise, this info will not be added when errp == &fatal_err
(the program will exit prior to the error_append_hint() or
error_prepend() call).  Fix such cases.

If we want to check error after errp-function call, we need to
introduce local_err and than propagate it to errp. Instead, use
ERRP_AUTO_PROPAGATE macro, benefits are:
1. No need of explicit error_propagate call
2. No need of explicit local_err variable: use errp directly
3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
   &error_fatel, this means that we don't break error_abort
   (we'll abort on error_set, not on error_propagate)

This commit (together with its neighbors) was generated by

for f in $(git grep -l errp \*.[ch]); do \
spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
--macro-file scripts/cocci-macro-file.h --in-place --no-show-diff $f; \
done;

then fix a bit of compilation problems: coccinelle for some reason
leaves several
f() {
...
goto out;
...
out:
}
patterns, with "out:" at function end.

then
./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"

(auto-msg was a file with this commit message)

Still, for backporting it may be more comfortable to use only the first
command and then do one huge commit.

Reported-by: Kevin Wolf 
Reported-by: Greg Kurz 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/nfs.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/block/nfs.c b/block/nfs.c
index f39acfdb28..ec0dbe3385 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -561,21 +561,20 @@ out:
 static BlockdevOptionsNfs *nfs_options_qdict_to_qapi(QDict *options,
  Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 BlockdevOptionsNfs *opts = NULL;
 Visitor *v;
 const QDictEntry *e;
-Error *local_err = NULL;
 
 v = qobject_input_visitor_new_flat_confused(options, errp);
 if (!v) {
 return NULL;
 }
 
-visit_type_BlockdevOptionsNfs(v, NULL, &opts, &local_err);
+visit_type_BlockdevOptionsNfs(v, NULL, &opts, errp);
 visit_free(v);
 
-if (local_err) {
-error_propagate(errp, local_err);
+if (*errp) {
 return NULL;
 }
 
-- 
2.21.0

Re: [PATCH v3 07/16] qcow2: Write v3-compliant snapshot list on upgrade

[Eric Blake]

On 10/11/19 10:28 AM, Max Reitz wrote:

qcow2 v3 requires every snapshot table entry to have two extra data
fields: The 64-bit VM state size, and the virtual disk size.  Both are
optional for v2 images, so they may not be present.

qcow2_upgrade() therefore should update the snapshot table to ensure all
entries have these extra data fields.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1727347
Reported-by: Eric Blake 
Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
  block/qcow2.c | 32 ++--
  1 file changed, 30 insertions(+), 2 deletions(-)




+need_snapshot_update = false;
+for (i = 0; i < s->nb_snapshots; i++) {
+if (s->snapshots[i].extra_data_size <
+sizeof_field(QCowSnapshotExtraData, vm_state_size_large) +
+sizeof_field(QCowSnapshotExtraData, disk_size))


Shorter as:
if (s->snapshots[i].extra_data_size < sizeof(QCowSnapshotExtraData))

but that's stylistic, so R-b still stands.

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

[RFC v5 060/126] megasas: introduce ERRP_AUTO_PROPAGATE

[Vladimir Sementsov-Ogievskiy]

If we want to add some info to errp (by error_prepend() or
error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
Otherwise, this info will not be added when errp == &fatal_err
(the program will exit prior to the error_append_hint() or
error_prepend() call).  Fix such cases.

If we want to check error after errp-function call, we need to
introduce local_err and than propagate it to errp. Instead, use
ERRP_AUTO_PROPAGATE macro, benefits are:
1. No need of explicit error_propagate call
2. No need of explicit local_err variable: use errp directly
3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
   &error_fatel, this means that we don't break error_abort
   (we'll abort on error_set, not on error_propagate)

This commit (together with its neighbors) was generated by

for f in $(git grep -l errp \*.[ch]); do \
spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
--macro-file scripts/cocci-macro-file.h --in-place --no-show-diff $f; \
done;

then fix a bit of compilation problems: coccinelle for some reason
leaves several
f() {
...
goto out;
...
out:
}
patterns, with "out:" at function end.

then
./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"

(auto-msg was a file with this commit message)

Still, for backporting it may be more comfortable to use only the first
command and then do one huge commit.

Reported-by: Kevin Wolf 
Reported-by: Greg Kurz 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 hw/scsi/megasas.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index de9bd20887..007cfcff88 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -2311,11 +2311,11 @@ static const struct SCSIBusInfo megasas_scsi_info = {
 
 static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 MegasasState *s = MEGASAS(dev);
 MegasasBaseClass *b = MEGASAS_DEVICE_GET_CLASS(s);
 uint8_t *pci_conf;
 int i, bar_type;
-Error *err = NULL;
 int ret;
 
 pci_conf = dev->config;
@@ -2326,20 +2326,19 @@ static void megasas_scsi_realize(PCIDevice *dev, Error 
**errp)
 pci_conf[PCI_INTERRUPT_PIN] = 0x01;
 
 if (s->msi != ON_OFF_AUTO_OFF) {
-ret = msi_init(dev, 0x50, 1, true, false, &err);
+ret = msi_init(dev, 0x50, 1, true, false, errp);
 /* Any error other than -ENOTSUP(board's MSI support is broken)
  * is a programming error */
 assert(!ret || ret == -ENOTSUP);
 if (ret && s->msi == ON_OFF_AUTO_ON) {
 /* Can't satisfy user's explicit msi=on request, fail */
-error_append_hint(&err, "You have to use msi=auto (default) or "
-"msi=off with this machine type.\n");
-error_propagate(errp, err);
+error_append_hint(errp, "You have to use msi=auto (default) or "
+  "msi=off with this machine type.\n");
 return;
 } else if (ret) {
 /* With msi=auto, we fall back to MSI off silently */
 s->msi = ON_OFF_AUTO_OFF;
-error_free(err);
+error_free_errp(errp);
 }
 }
 
-- 
2.21.0

Re: [PATCH v3 14/16] qcow2: Fix v3 snapshot table entry compliancy

[Eric Blake]

On 10/11/19 10:28 AM, Max Reitz wrote:

qcow2 v3 images require every snapshot table entry to have at least 16
bytes of extra data.  If they do not, let qemu-img check -r all fix it.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
  block/qcow2-snapshot.c | 18 ++
  1 file changed, 18 insertions(+)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index dac8a778e4..5ab64da1ec 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -516,6 +516,24 @@ int coroutine_fn 
qcow2_check_read_snapshot_table(BlockDriverState *bs,
  result->corruptions -= nb_clusters_reduced;
  }
  
+/*

+ * All of v3 images' snapshot table entries need to have at least
+ * 16 bytes of extra data.
+ */
+if (s->qcow_version >= 3) {
+int i;
+for (i = 0; i < s->nb_snapshots; i++) {
+if (s->snapshots[i].extra_data_size <
+sizeof_field(QCowSnapshotExtraData, vm_state_size_large) +
+sizeof_field(QCowSnapshotExtraData, disk_size))
+{


Another stylistic place where sizeof(QCowSnapshotExtraData) would be 
more compact.  Doesn't change R-b


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

[RFC v5 101/126] SSH: introduce ERRP_AUTO_PROPAGATE

[Vladimir Sementsov-Ogievskiy]

If we want to add some info to errp (by error_prepend() or
error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
Otherwise, this info will not be added when errp == &fatal_err
(the program will exit prior to the error_append_hint() or
error_prepend() call).  Fix such cases.

If we want to check error after errp-function call, we need to
introduce local_err and than propagate it to errp. Instead, use
ERRP_AUTO_PROPAGATE macro, benefits are:
1. No need of explicit error_propagate call
2. No need of explicit local_err variable: use errp directly
3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
   &error_fatel, this means that we don't break error_abort
   (we'll abort on error_set, not on error_propagate)

This commit (together with its neighbors) was generated by

for f in $(git grep -l errp \*.[ch]); do \
spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
--macro-file scripts/cocci-macro-file.h --in-place --no-show-diff $f; \
done;

then fix a bit of compilation problems: coccinelle for some reason
leaves several
f() {
...
goto out;
...
out:
}
patterns, with "out:" at function end.

then
./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"

(auto-msg was a file with this commit message)

Still, for backporting it may be more comfortable to use only the first
command and then do one huge commit.

Reported-by: Kevin Wolf 
Reported-by: Greg Kurz 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/ssh.c | 12 +---
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/block/ssh.c b/block/ssh.c
index 84d01e892b..98df18ecb7 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -614,17 +614,16 @@ static bool ssh_process_legacy_options(QDict *output_opts,
 
 static BlockdevOptionsSsh *ssh_parse_options(QDict *options, Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 BlockdevOptionsSsh *result = NULL;
 QemuOpts *opts = NULL;
-Error *local_err = NULL;
 const QDictEntry *e;
 Visitor *v;
 
 /* Translate legacy options */
 opts = qemu_opts_create(&ssh_runtime_opts, NULL, 0, &error_abort);
-qemu_opts_absorb_qdict(opts, options, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
+qemu_opts_absorb_qdict(opts, options, errp);
+if (*errp) {
 goto fail;
 }
 
@@ -638,11 +637,10 @@ static BlockdevOptionsSsh *ssh_parse_options(QDict 
*options, Error **errp)
 goto fail;
 }
 
-visit_type_BlockdevOptionsSsh(v, NULL, &result, &local_err);
+visit_type_BlockdevOptionsSsh(v, NULL, &result, errp);
 visit_free(v);
 
-if (local_err) {
-error_propagate(errp, local_err);
+if (*errp) {
 goto fail;
 }
 
-- 
2.21.0

[RFC v5 109/126] parallels: introduce ERRP_AUTO_PROPAGATE

[Vladimir Sementsov-Ogievskiy]

If we want to add some info to errp (by error_prepend() or
error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
Otherwise, this info will not be added when errp == &fatal_err
(the program will exit prior to the error_append_hint() or
error_prepend() call).  Fix such cases.

If we want to check error after errp-function call, we need to
introduce local_err and than propagate it to errp. Instead, use
ERRP_AUTO_PROPAGATE macro, benefits are:
1. No need of explicit error_propagate call
2. No need of explicit local_err variable: use errp directly
3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
   &error_fatel, this means that we don't break error_abort
   (we'll abort on error_set, not on error_propagate)

This commit (together with its neighbors) was generated by

for f in $(git grep -l errp \*.[ch]); do \
spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
--macro-file scripts/cocci-macro-file.h --in-place --no-show-diff $f; \
done;

then fix a bit of compilation problems: coccinelle for some reason
leaves several
f() {
...
goto out;
...
out:
}
patterns, with "out:" at function end.

then
./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"

(auto-msg was a file with this commit message)

Still, for backporting it may be more comfortable to use only the first
command and then do one huge commit.

Reported-by: Kevin Wolf 
Reported-by: Greg Kurz 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/parallels.c | 30 +-
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 7cd2714b69..c8ba23bbd5 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -613,8 +613,8 @@ static int coroutine_fn parallels_co_create_opts(const char 
*filename,
  QemuOpts *opts,
  Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 BlockdevCreateOptions *create_options = NULL;
-Error *local_err = NULL;
 BlockDriverState *bs = NULL;
 QDict *qdict;
 Visitor *v;
@@ -635,9 +635,8 @@ static int coroutine_fn parallels_co_create_opts(const char 
*filename,
 }
 
 /* Create and open the file (protocol layer) */
-ret = bdrv_create_file(filename, opts, &local_err);
+ret = bdrv_create_file(filename, opts, errp);
 if (ret < 0) {
-error_propagate(errp, local_err);
 goto done;
 }
 
@@ -658,11 +657,10 @@ static int coroutine_fn parallels_co_create_opts(const 
char *filename,
 goto done;
 }
 
-visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
+visit_type_BlockdevCreateOptions(v, NULL, &create_options, errp);
 visit_free(v);
 
-if (local_err) {
-error_propagate(errp, local_err);
+if (*errp) {
 ret = -EINVAL;
 goto done;
 }
@@ -721,11 +719,11 @@ static int parallels_update_header(BlockDriverState *bs)
 static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
   Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 BDRVParallelsState *s = bs->opaque;
 ParallelsHeader ph;
 int ret, size, i;
 QemuOpts *opts = NULL;
-Error *local_err = NULL;
 char *buf;
 
 bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
@@ -813,13 +811,13 @@ static int parallels_open(BlockDriverState *bs, QDict 
*options, int flags,
 }
 }
 
-opts = qemu_opts_create(¶llels_runtime_opts, NULL, 0, &local_err);
-if (local_err != NULL) {
+opts = qemu_opts_create(¶llels_runtime_opts, NULL, 0, errp);
+if (*errp) {
 goto fail_options;
 }
 
-qemu_opts_absorb_qdict(opts, options, &local_err);
-if (local_err != NULL) {
+qemu_opts_absorb_qdict(opts, options, errp);
+if (*errp) {
 goto fail_options;
 }
 
@@ -829,9 +827,9 @@ static int parallels_open(BlockDriverState *bs, QDict 
*options, int flags,
 buf = qemu_opt_get_del(opts, PARALLELS_OPT_PREALLOC_MODE);
 s->prealloc_mode = qapi_enum_parse(&prealloc_mode_lookup, buf,
PRL_PREALLOC_MODE_FALLOCATE,
-   &local_err);
+   errp);
 g_free(buf);
-if (local_err != NULL) {
+if (*errp) {
 goto fail_options;
 }
 
@@ -855,9 +853,8 @@ static int parallels_open(BlockDriverState *bs, QDict 
*options, int flags,
 error_setg(&s->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, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
+ret = migrate_add_blocker(s->migration_blocker, errp);
+if (*errp) {
 error_free(s->migration_blocker);
 goto fail;
 }
@@ -872,7 +869,6 @@ fail:
 retu

[RFC v5 114/126] blkdebug: introduce ERRP_AUTO_PROPAGATE

[Vladimir Sementsov-Ogievskiy]

If we want to add some info to errp (by error_prepend() or
error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
Otherwise, this info will not be added when errp == &fatal_err
(the program will exit prior to the error_append_hint() or
error_prepend() call).  Fix such cases.

If we want to check error after errp-function call, we need to
introduce local_err and than propagate it to errp. Instead, use
ERRP_AUTO_PROPAGATE macro, benefits are:
1. No need of explicit error_propagate call
2. No need of explicit local_err variable: use errp directly
3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
   &error_fatel, this means that we don't break error_abort
   (we'll abort on error_set, not on error_propagate)

This commit (together with its neighbors) was generated by

for f in $(git grep -l errp \*.[ch]); do \
spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
--macro-file scripts/cocci-macro-file.h --in-place --no-show-diff $f; \
done;

then fix a bit of compilation problems: coccinelle for some reason
leaves several
f() {
...
goto out;
...
out:
}
patterns, with "out:" at function end.

then
./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"

(auto-msg was a file with this commit message)

Still, for backporting it may be more comfortable to use only the first
command and then do one huge commit.

Reported-by: Kevin Wolf 
Reported-by: Greg Kurz 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/blkdebug.c | 36 +++-
 1 file changed, 15 insertions(+), 21 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 5ae96c52b0..7c3fc222f3 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -164,6 +164,7 @@ struct add_rule_data {
 
 static int add_rule(void *opaque, QemuOpts *opts, Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 struct add_rule_data *d = opaque;
 BDRVBlkdebugState *s = d->s;
 const char* event_name;
@@ -171,7 +172,6 @@ static int add_rule(void *opaque, QemuOpts *opts, Error 
**errp)
 struct BlkdebugRule *rule;
 int64_t sector;
 BlkdebugIOType iotype;
-Error *local_error = NULL;
 
 /* Find the right event for the rule */
 event_name = qemu_opt_get(opts, "event");
@@ -205,9 +205,8 @@ static int add_rule(void *opaque, QemuOpts *opts, Error 
**errp)
 
 iotype = qapi_enum_parse(&BlkdebugIOType_lookup,
  qemu_opt_get(opts, "iotype"),
- BLKDEBUG_IO_TYPE__MAX, &local_error);
-if (local_error) {
-error_propagate(errp, local_error);
+ BLKDEBUG_IO_TYPE__MAX, errp);
+if (*errp) {
 return -1;
 }
 if (iotype != BLKDEBUG_IO_TYPE__MAX) {
@@ -259,10 +258,10 @@ static void remove_rule(BlkdebugRule *rule)
 static int read_config(BDRVBlkdebugState *s, const char *filename,
QDict *options, Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 FILE *f = NULL;
 int ret;
 struct add_rule_data d;
-Error *local_err = NULL;
 
 if (filename) {
 f = fopen(filename, "r");
@@ -278,26 +277,23 @@ static int read_config(BDRVBlkdebugState *s, const char 
*filename,
 }
 }
 
-qemu_config_parse_qdict(options, config_groups, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
+qemu_config_parse_qdict(options, config_groups, errp);
+if (*errp) {
 ret = -EINVAL;
 goto fail;
 }
 
 d.s = s;
 d.action = ACTION_INJECT_ERROR;
-qemu_opts_foreach(&inject_error_opts, add_rule, &d, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
+qemu_opts_foreach(&inject_error_opts, add_rule, &d, errp);
+if (*errp) {
 ret = -EINVAL;
 goto fail;
 }
 
 d.action = ACTION_SET_STATE;
-qemu_opts_foreach(&set_state_opts, add_rule, &d, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
+qemu_opts_foreach(&set_state_opts, add_rule, &d, errp);
+if (*errp) {
 ret = -EINVAL;
 goto fail;
 }
@@ -395,16 +391,15 @@ static QemuOptsList runtime_opts = {
 static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
  Error **errp)
 {
+ERRP_AUTO_PROPAGATE();
 BDRVBlkdebugState *s = bs->opaque;
 QemuOpts *opts;
-Error *local_err = NULL;
 int ret;
 uint64_t align;
 
 opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
-qemu_opts_absorb_qdict(opts, options, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
+qemu_opts_absorb_qdict(opts, options, errp);
+if (*errp) {
 ret = -EINVAL;
 goto out;
 }
@@ -421,10 +416,9 @@ static int blkdebug_open(BlockDriverState *bs, QDict 
*options, int flags,
 
 /* Open the image file */
 bs->file = bdrv_open_child(qemu_opt_get(opts, "x-image"), options, "image",
-