Re: Potential Null dereference

[Philippe Mathieu-Daudé]

On 3/24/20 4:05 AM, Mansour Ahmadi wrote:

Hi,

Nullness of  needs to be checked here:
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/commit.c#L221

pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),...


While it is done at 2 other locations:
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/backup-top.c#L113
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/mirror.c#L1477

if(bs->backing== NULL) {return}


Thanks for spotting this. Do you mind sending a patch?



Thanks,
Mansour

RE: [PATCH-for-5.0 v2 00/11] misc: Trivial static code analyzer fixes

[Chenqun (kuhn)]

>-Original Message-
>From: Qemu-devel [mailto:qemu-devel-
>bounces+kuhn.chenqun=huawei@nongnu.org] On Behalf Of Laurent Vivier
>Sent: Monday, March 23, 2020 10:56 PM
>To: Philippe Mathieu-Daudé ; qemu-de...@nongnu.org
>Cc: Fam Zheng ; Peter Maydell ;
>Michael S. Tsirkin ; Mark Cave-Ayland ayl...@ilande.co.uk>; qemu-block@nongnu.org; qemu-triv...@nongnu.org;
>Markus Armbruster ; Hervé Poussineau
>; Joel Stanley ; Michael Tokarev
>; Alistair Francis ; qemu-
>a...@nongnu.org; Cédric Le Goater ; John Snow
>; David Gibson ; Kevin Wolf
>; Andrew Jeffery ; Max Reitz
>; Igor Mitsyanko ; qemu-
>p...@nongnu.org; Paolo Bonzini 
>Subject: Re: [PATCH-for-5.0 v2 00/11] misc: Trivial static code analyzer fixes
>
>Le 23/03/2020 à 15:45, Philippe Mathieu-Daudé a écrit :
>> On 3/23/20 3:32 PM, Laurent Vivier wrote:
>>> Le 21/03/2020 à 15:40, Philippe Mathieu-Daudé a écrit :
 Fix trivial warnings reported by the Clang static code analyzer.

 Since v1:
 - Addressed Markus/Zoltan/Aleksandar review comments

 Philippe Mathieu-Daudé (11):
    block: Avoid dead assignment
    blockdev: Remove dead assignment
    hw/i2c/pm_smbus: Remove dead assignment
    hw/input/adb-kbd: Remove dead assignment
    hw/ide/sii3112: Remove dead assignment
    hw/isa/i82378: Remove dead assignment
    hw/gpio/aspeed_gpio: Remove dead assignment
    hw/timer/exynos4210_mct: Remove dead assignments

Same as this:
https://patchwork.kernel.org/patch/11415527/

    hw/timer/stm32f2xx_timer: Remove dead assignment
    hw/timer/pxa2xx_timer: Add assertion to silent static analyzer
 warning
    hw/scsi/esp-pci: Remove dead assignment

Same as this:
https://patchwork.kernel.org/patch/11415529/


   block.c    | 2 +-
   blockdev.c | 2 +-
   hw/gpio/aspeed_gpio.c  | 2 +-
   hw/i2c/pm_smbus.c  | 1 -
   hw/ide/sii3112.c   | 5 +++--
   hw/input/adb-kbd.c | 6 +-
   hw/isa/i82378.c    | 8 
   hw/scsi/esp-pci.c  | 1 -
   hw/timer/exynos4210_mct.c  | 3 ---
   hw/timer/pxa2xx_timer.c    | 1 +
   hw/timer/stm32f2xx_timer.c | 1 -
   11 files changed, 12 insertions(+), 20 deletions(-)

>>>
>>> I think your series covers cases already covered by:
>>>
>>> [PATCH v3 00/12] redundant code: Fix warnings reported by Clang
>>> static code analyzer
>>> https://patchew.org/QEMU/20200302130715.29440-1-kuhn.ch
>>
>> Unfortunately [for me...] I don't have v3 in my INBOX... *sigh* This
>> was 3 weeks ago. *sigh*.
>>
>> I can see the series in the archives:
>> https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg00219.html
>> But I can't find the outcome, was it queued in the trivial tree?
>> Any idea when this will be merged in the master tree?
>
>Some patches are already merged via trivial (1, 2 (should go by SCSI
>queue) 3, 5, 6, 7, 9, 11 (by USB queue), 12).
>
>But others needed R-b tags or new version. I didn't check which of your patches
>are already covered by this series.
>
Hi, Laurent and Philippe

  Yes, there are currently three patches( 4、8 、10) that are not merged.  
And, only patch4 and patch10  have the same points as this series.

I should update it again earlier.

Thanks.

[PATCH] iotests: drop group file

[Vladimir Sementsov-Ogievskiy]

When sending iotests to upstream or do patch porting from one branch
to another we very often have to resolve conflicts in group file, as
many absolutely independent features are intersecting by this file.
These conflicts are simple, but imagine how much time we all have
already spent on resolving them? Let's finally get rid of group file.

This patch transposes group info: instead of collecting it in one file,
let each test define its groups by itself.

Several steps are done to achive it:

1. Define groups in test files automatically:

grep '^[0-9]\{3\} ' group | while read line; do
file=$(awk '{print $1}' <<< "$line");
groups=$(sed -e 's/^... //' <<< "$line");
awk "NR==2{print \"# group: $groups\"}1" $file > tmp;
cat tmp > $file;
done

2. Copy groups documentation into docs/devel/testing.rst, which already
   has a section about iotests.

3. Modify check script to work without group file.

   Here is a logic change: before, even if test do not belong to any
   group (only iotest 142 currently) it should be defined in group
   file. Now, test is not forced to define any group. Instead check
   considers all files with names matching [0-9][0-9][0-9] as tests.

   check script is also refactored to make it simple to do next cool
   thing about iotests: allow meaningful names for test-case files.

4. Remove group file.

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

Hi all!

I understand, that it's 5.1 material, and I will have to regenerate
generated part and resend, but I just really want to share the idea
with you. Hope you like it:)

 docs/devel/testing.rst   |  30 +++-
 tests/qemu-iotests/001   |   1 +
 tests/qemu-iotests/002   |   1 +
 tests/qemu-iotests/003   |   1 +
 tests/qemu-iotests/004   |   1 +
 tests/qemu-iotests/005   |   1 +
 tests/qemu-iotests/007   |   1 +
 tests/qemu-iotests/008   |   1 +
 tests/qemu-iotests/009   |   1 +
 tests/qemu-iotests/010   |   1 +
 tests/qemu-iotests/011   |   1 +
 tests/qemu-iotests/012   |   1 +
 tests/qemu-iotests/013   |   1 +
 tests/qemu-iotests/014   |   1 +
 tests/qemu-iotests/015   |   1 +
 tests/qemu-iotests/017   |   1 +
 tests/qemu-iotests/018   |   1 +
 tests/qemu-iotests/019   |   1 +
 tests/qemu-iotests/020   |   1 +
 tests/qemu-iotests/021   |   1 +
 tests/qemu-iotests/022   |   1 +
 tests/qemu-iotests/023   |   1 +
 tests/qemu-iotests/024   |   1 +
 tests/qemu-iotests/025   |   1 +
 tests/qemu-iotests/026   |   1 +
 tests/qemu-iotests/027   |   1 +
 tests/qemu-iotests/028   |   1 +
 tests/qemu-iotests/029   |   1 +
 tests/qemu-iotests/030   |   1 +
 tests/qemu-iotests/031   |   1 +
 tests/qemu-iotests/032   |   1 +
 tests/qemu-iotests/033   |   1 +
 tests/qemu-iotests/034   |   1 +
 tests/qemu-iotests/035   |   1 +
 tests/qemu-iotests/036   |   1 +
 tests/qemu-iotests/037   |   1 +
 tests/qemu-iotests/038   |   1 +
 tests/qemu-iotests/039   |   1 +
 tests/qemu-iotests/040   |   1 +
 tests/qemu-iotests/041   |   1 +
 tests/qemu-iotests/042   |   1 +
 tests/qemu-iotests/043   |   1 +
 tests/qemu-iotests/044   |   1 +
 tests/qemu-iotests/045   |   1 +
 tests/qemu-iotests/046   |   1 +
 tests/qemu-iotests/047   |   1 +
 tests/qemu-iotests/048   |   1 +
 tests/qemu-iotests/049   |   1 +
 tests/qemu-iotests/050   |   1 +
 tests/qemu-iotests/051   |   1 +
 tests/qemu-iotests/052   |   1 +
 tests/qemu-iotests/053   |   1 +
 tests/qemu-iotests/054   |   1 +
 tests/qemu-iotests/055   |   1 +
 tests/qemu-iotests/056   |   1 +
 tests/qemu-iotests/057   |   1 +
 tests/qemu-iotests/058   |   1 +
 tests/qemu-iotests/059   |   1 +
 tests/qemu-iotests/060   |   1 +
 tests/qemu-iotests/061   |   1 +
 tests/qemu-iotests/062   |   1 +
 tests/qemu-iotests/063   |   1 +
 tests/qemu-iotests/064   |   1 +
 tests/qemu-iotests/065   |   1 +
 tests/qemu-iotests/066   |   1 +
 tests/qemu-iotests/067   |   1 +
 tests/qemu-iotests/068   |   1 +
 tests/qemu-iotests/069   |   1 +
 tests/qemu-iotests/070   |   1 +
 tests/qemu-iotests/071   |   1 +
 tests/qemu-iotests/072   |   1 +
 tests/qemu-iotests/073   |   1 +
 tests/qemu-iotests/074   |   1 +
 tests/qemu-iotests/075   |   1 +
 tests/qemu-iotests/076   |   1 +
 tests/qemu-iotests/077   |   1 +
 tests/qemu-iotests/078   |   1 +
 tests/qemu-iotests/079   |   1 +
 tests/qemu-iotests/080   |   1 +
 tests/qemu-iotests/081   |   1 +
 tests/qemu-iotests/082   |   1 +
 tests/qemu-iotests/083   |   1 +
 tests/qemu-iotests/084   |   1 +
 tests/qemu-iotests/085   |   1 +
 tests/qemu-iotests/086   |   1 +
 tests/qemu-iotests/087   |   1 +
 tests/qemu-iotests/088   |   1 +
 tests/qemu-iotests/089   |   1 +
 tests/qemu-iotests/090   |   1 +
 tests/qemu-iotests/091   |   1 +
 tests/qemu-iotests/092   |   1 +
 tests/qemu-iotests/093   |   1 +
 tests/qemu-iotests/094   |   1 +
 tests/qemu-iotests/095   |   1 +
 tests/qemu-iotests/096   |   1 +
 tests/qemu-iotests/097   |   1 +
 tests/qemu-iotests/098   |   1 +
 tests/qemu-iotests/099   |   1 +
 tests/qemu-iotests/101   |   1 +
 tests/qemu-iotests/102 

Re: [PATCH] iotests: drop group file

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/20200324074156.5330-1-vsement...@virtuozzo.com/



Hi,

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

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC  s_normRoundPackToF64.o
  CC  s_addMagsF64.o
  CC  s_subMagsF64.o
make: *** [check-tests/check-block.sh] Error 1
make: *** Waiting for unfinished jobs
  CC  s_mulAddF64.o
  CC  s_normSubnormalExtF80Sig.o
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', 
'--label', 'com.qemu.instance.uuid=2b582229add741f387fcf0a88ff2a3ff', '-u', 
'1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', 
'-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 
'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', 
'/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', 
'/var/tmp/patchew-tester-tmp-ot7uydwt/src/docker-src.2020-03-24-03.54.58.28935:/var/tmp/qemu:z,ro',
 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit 
status 2.
filter=--filter=label=com.qemu.instance.uuid=2b582229add741f387fcf0a88ff2a3ff
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-ot7uydwt/src'
make: *** [docker-run-test-quick@centos7] Error 2

real2m41.224s
user0m7.941s


The full log is available at
http://patchew.org/logs/20200324074156.5330-1-vsement...@virtuozzo.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

Re: [PATCH] iotests: drop group file

[Vladimir Sementsov-Ogievskiy]

24.03.2020 10:57, no-re...@patchew.org wrote:

Patchew URL: 
https://patchew.org/QEMU/20200324074156.5330-1-vsement...@virtuozzo.com/



Hi,

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

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

   CC  s_normRoundPackToF64.o
   CC  s_addMagsF64.o
   CC  s_subMagsF64.o
make: *** [check-tests/check-block.sh] Error 1
make: *** Waiting for unfinished jobs
   CC  s_mulAddF64.o
   CC  s_normSubnormalExtF80Sig.o
---
 raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', 
'--label', 'com.qemu.instance.uuid=2b582229add741f387fcf0a88ff2a3ff', '-u', 
'1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', 
'-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 
'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', 
'/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', 
'/var/tmp/patchew-tester-tmp-ot7uydwt/src/docker-src.2020-03-24-03.54.58.28935:/var/tmp/qemu:z,ro',
 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit 
status 2.
filter=--filter=label=com.qemu.instance.uuid=2b582229add741f387fcf0a88ff2a3ff
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-ot7uydwt/src'
make: *** [docker-run-test-quick@centos7] Error 2

real2m41.224s
user0m7.941s


The full log is available at
http://patchew.org/logs/20200324074156.5330-1-vsement...@virtuozzo.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com




Doesn't look as something related to my patch.


--
Best regards,
Vladimir

Re: [PATCH 3/5] block: add max_pwrite_zeroes_no_fallback to BlockLimits

[Vladimir Sementsov-Ogievskiy]

14.03.2020 0:07, Eric Blake wrote:

On 3/2/20 4:05 AM, Vladimir Sementsov-Ogievskiy wrote:

NBD spec is updated, so that max_block doesn't relate to


Maybe: The NBD spec was recently updated to clarify that max_block...


NBD_CMD_WRITE_ZEROES with NBD_CMD_FLAG_FAST_ZERO (which mirrors Qemu
flag BDRV_REQ_NO_FALLBACK). To drop the restriction we need new
max_pwrite_zeroes_no_fallback.


It feels odd to have two different pwrite_zeroes limits in the block layer, but 
I can live with it if other block layer gurus are also okay with it.



Default value of new max_pwrite_zeroes_no_fallback is zero and it means
no-restriction, so we are automatically done by this commit. Note that


Why not have the default value be set to the existing value of the normal 
pwrite_zeroes limit, rather than 0?


nbd and blkdebug are the only drivers which in the same time define
max_pwrite_zeroes limit and support BDRV_REQ_NO_FALLBACK, so we need to
update only blkdebug.


Grammar:

The default value for the new max_pwrite_zeroes_no_fallback is zero, meaning no 
restriction, which covers all drivers not touched by this commit.  Note that 
nbd and blkdebug are the only drivers which have a max_pwrite_zeroes limit 
while supporting BDRV_REQ_NO_FALLBACK, so we only need to update blkdebug.

Except that I think there IS still a limit in current NBD: you can't request 
anything larger than 32 bits (whereas some other drivers may allow a full 
63-bit request, as well as future NBD usage when we finally add 64-bit 
extensions to the protocol).  So I think this patch is incomplete; it should be 
updating the nbd code to set the proper limit.

(I still need to post v2 of my patches for bdrv_co_make_zero support, which is 
a case where knowing if there is a 32-bit limit when using BDRV_REQ_NO_FALLBACK 
for fast zeroing is important).



Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  include/block/block_int.h | 8 
  block/blkdebug.c  | 7 ++-
  block/io.c    | 4 +++-
  3 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 6f9fd5e20e..c167e887c6 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -618,6 +618,14 @@ typedef struct BlockLimits {
   * pwrite_zeroes_alignment. May be 0 if no inherent 32-bit limit */
  int32_t max_pwrite_zeroes;
+    /*
+ * Maximum number of bytes that can zeroized at once if flag


zeroed


+ * BDRV_REQ_NO_FALLBACK specified (since it is signed, it must be < 2G, if
+ * set).


Why must it be a signed 32-bit number?  Why not let it be a 64-bit number?


Must be multiple of pwrite_zeroes_alignment. May be 0 if no
+ * inherent 32-bit limit.
+ */
+    int32_t max_pwrite_zeroes_no_fallback;
+
  /* Optimal alignment for write zeroes requests in bytes. A power
   * of 2 is best but not mandatory.  Must be a multiple of
   * bl.request_alignment, and must be less than max_pwrite_zeroes
diff --git a/block/blkdebug.c b/block/blkdebug.c
index af44aa973f..7627fbcb3b 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -692,7 +692,11 @@ static int coroutine_fn 
blkdebug_co_pwrite_zeroes(BlockDriverState *bs,
  }
  assert(QEMU_IS_ALIGNED(offset, align));
  assert(QEMU_IS_ALIGNED(bytes, align));
-    if (bs->bl.max_pwrite_zeroes) {
+    if ((flags & BDRV_REQ_NO_FALLBACK) &&
+    bs->bl.max_pwrite_zeroes_no_fallback)
+    {
+    assert(bytes <= bs->bl.max_pwrite_zeroes_no_fallback);
+    } else if (bs->bl.max_pwrite_zeroes) {
  assert(bytes <= bs->bl.max_pwrite_zeroes);
  }
@@ -977,6 +981,7 @@ static void blkdebug_refresh_limits(BlockDriverState *bs, 
Error **errp)
  }
  if (s->max_write_zero) {
  bs->bl.max_pwrite_zeroes = s->max_write_zero;
+    bs->bl.max_pwrite_zeroes_no_fallback = s->max_write_zero;


Ah, so you DO default it to max_pwwrite_zeroes instead of to 0; the commit 
message does not quite match the code.


  }
  if (s->opt_discard) {
  bs->bl.pdiscard_alignment = s->opt_discard;
diff --git a/block/io.c b/block/io.c
index 7e4cb74cf4..75fd5600c2 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1752,7 +1752,9 @@ static int coroutine_fn 
bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
  int head = 0;
  int tail = 0;
-    int max_write_zeroes = MIN_NON_ZERO(bs->bl.max_pwrite_zeroes, INT_MAX);
+    int max_write_zeroes = MIN_NON_ZERO((flags & BDRV_REQ_NO_FALLBACK) ?
+    bs->bl.max_pwrite_zeroes_no_fallback :
+    bs->bl.max_pwrite_zeroes, INT_MAX);


I'd still like to get rid of this INT_MAX clamping.  If we can blank the entire 
image in one call, even when it is larger than 4G, then it is worth making that 
exposed to the user.  (Even in NBD, we might decide to add an extension that 
allows NBD_CMD_WRITE_ZEROES with a new flag and with offset/length == 0/0, as 
an official way to make the entire image zero, w

Re: [PATCH] block: make BlockConf.*_size properties 32-bit

[Roman Kagan]

On Mon, Mar 02, 2020 at 01:55:02PM +0300, Roman Kagan wrote:
> On Thu, Feb 13, 2020 at 04:55:44PM +0300, Roman Kagan wrote:
> > On Thu, Feb 13, 2020 at 06:47:10AM -0600, Eric Blake wrote:
> > > On 2/13/20 2:01 AM, Roman Kagan wrote:
> > > > On Wed, Feb 12, 2020 at 03:44:19PM -0600, Eric Blake wrote:
> > > > > On 2/11/20 5:54 AM, Roman Kagan wrote:
> > > > > > Devices (virtio-blk, scsi, etc.) and the block layer are happy to 
> > > > > > use
> > > > > > 32-bit for logical_block_size, physical_block_size, and min_io_size.
> > > > > > However, the properties in BlockConf are defined as uint16_t 
> > > > > > limiting
> > > > > > the values to 32768.
> > > > > > 
> > > > > > This appears unnecessary tight, and we've seen bigger block sizes 
> > > > > > handy
> > > > > > at times.
> > > > > 
> > > > > What larger sizes?  I could see 64k or maybe even 1M block sizes,...
> > > > 
> > > > We played exactly with these two :)
> > > > 
> > > > > > 
> > > > > > Make them 32 bit instead and lift the limitation.
> > > > > > 
> > > > > > Signed-off-by: Roman Kagan 
> > > > > > ---
> > > > > >hw/core/qdev-properties.c| 21 -
> > > > > >include/hw/block/block.h |  8 
> > > > > >include/hw/qdev-properties.h |  2 +-
> > > > > >3 files changed, 17 insertions(+), 14 deletions(-)
> > > > > > 
> > > > > > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> > > > > > index 7f93bfeb88..5f84e4a3b8 100644
> > > > > > --- a/hw/core/qdev-properties.c
> > > > > > +++ b/hw/core/qdev-properties.c
> > > > > > @@ -716,30 +716,32 @@ const PropertyInfo qdev_prop_pci_devfn = {
> > > > > >/* --- blocksize --- */
> > > > > > +#define MIN_BLOCK_SIZE 512
> > > > > > +#define MAX_BLOCK_SIZE 2147483648
> > > > > 
> > > > > ...but 2G block sizes are going to have tremendous performance 
> > > > > problems.
> > > > > 
> > > > > I'm not necessarily opposed to the widening to a 32-bit type, but 
> > > > > think you
> > > > > need more justification or a smaller number for the max block size,
> > > > 
> > > > I thought any smaller value would just be arbitrary and hard to reason
> > > > about, so I went ahead with the max value that fit in the type and could
> > > > be made visibile to the guest.
> > > 
> > > You've got bigger problems than what is visible to the guest. 
> > > block/qcow2.c
> > > operates on a cluster at a time; if you are stating that it now requires
> > > reading multiple clusters to operate on one, qcow2 will have to do lots of
> > > wasteful read-modify-write cycles.
> > 
> > I'm failing to see how this is supposed to happen.  The guest will issue
> > requests bigger than the cluster size; why would it cause RMW?
> > 
> > Big logical_block_size would cause RMW in the guest if it wants to
> > perform smaller writes, but that's up to the user to take this tradeoff,
> > isn't it?
> > 
> > > You really need a strong reason to
> > > support a maximum larger than 2M other than just "so the guest can
> > > experiment with it".
> > 
> > Do I get you right that your suggestion is to cap the block size
> > property at 2MB?
> > 
> > Thanks,
> > Roman.
> 
> Ping?

Ping?

> > > > 
> > > > Besides this is a property that is set explicitly, so I don't see a
> > > > problem leaving this up to the user.
> > > > 
> > > > > particularly since qcow2 refuses to use cluster sizes larger than 2M 
> > > > > and it
> > > > > makes no sense to allow a block size larger than a cluster size.
> > > > 
> > > > This still doesn't contradict passing a bigger value to the guest, for
> > > > experimenting if nothing else.
> > > > 
> > > > Thanks,
> > > > Roman.
> > > > 
> > > 
> > > -- 
> > > Eric Blake, Principal Software Engineer
> > > Red Hat, Inc.   +1-919-301-3226
> > > Virtualization:  qemu.org | libvirt.org

Re: [PATCH] block: make BlockConf.*_size properties 32-bit

[Kevin Wolf]

Am 24.03.2020 um 09:55 hat Roman Kagan geschrieben:
> On Mon, Mar 02, 2020 at 01:55:02PM +0300, Roman Kagan wrote:
> > On Thu, Feb 13, 2020 at 04:55:44PM +0300, Roman Kagan wrote:
> > > On Thu, Feb 13, 2020 at 06:47:10AM -0600, Eric Blake wrote:
> > > > On 2/13/20 2:01 AM, Roman Kagan wrote:
> > > > > On Wed, Feb 12, 2020 at 03:44:19PM -0600, Eric Blake wrote:
> > > > > > On 2/11/20 5:54 AM, Roman Kagan wrote:
> > > > > > > Devices (virtio-blk, scsi, etc.) and the block layer are happy to 
> > > > > > > use
> > > > > > > 32-bit for logical_block_size, physical_block_size, and 
> > > > > > > min_io_size.
> > > > > > > However, the properties in BlockConf are defined as uint16_t 
> > > > > > > limiting
> > > > > > > the values to 32768.
> > > > > > > 
> > > > > > > This appears unnecessary tight, and we've seen bigger block sizes 
> > > > > > > handy
> > > > > > > at times.
> > > > > > 
> > > > > > What larger sizes?  I could see 64k or maybe even 1M block sizes,...
> > > > > 
> > > > > We played exactly with these two :)
> > > > > 
> > > > > > > 
> > > > > > > Make them 32 bit instead and lift the limitation.
> > > > > > > 
> > > > > > > Signed-off-by: Roman Kagan 
> > > > > > > ---
> > > > > > >hw/core/qdev-properties.c| 21 -
> > > > > > >include/hw/block/block.h |  8 
> > > > > > >include/hw/qdev-properties.h |  2 +-
> > > > > > >3 files changed, 17 insertions(+), 14 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> > > > > > > index 7f93bfeb88..5f84e4a3b8 100644
> > > > > > > --- a/hw/core/qdev-properties.c
> > > > > > > +++ b/hw/core/qdev-properties.c
> > > > > > > @@ -716,30 +716,32 @@ const PropertyInfo qdev_prop_pci_devfn = {
> > > > > > >/* --- blocksize --- */
> > > > > > > +#define MIN_BLOCK_SIZE 512
> > > > > > > +#define MAX_BLOCK_SIZE 2147483648
> > > > > > 
> > > > > > ...but 2G block sizes are going to have tremendous performance 
> > > > > > problems.
> > > > > > 
> > > > > > I'm not necessarily opposed to the widening to a 32-bit type, but 
> > > > > > think you
> > > > > > need more justification or a smaller number for the max block size,
> > > > > 
> > > > > I thought any smaller value would just be arbitrary and hard to reason
> > > > > about, so I went ahead with the max value that fit in the type and 
> > > > > could
> > > > > be made visibile to the guest.
> > > > 
> > > > You've got bigger problems than what is visible to the guest. 
> > > > block/qcow2.c
> > > > operates on a cluster at a time; if you are stating that it now requires
> > > > reading multiple clusters to operate on one, qcow2 will have to do lots 
> > > > of
> > > > wasteful read-modify-write cycles.
> > > 
> > > I'm failing to see how this is supposed to happen.  The guest will issue
> > > requests bigger than the cluster size; why would it cause RMW?
> > > 
> > > Big logical_block_size would cause RMW in the guest if it wants to
> > > perform smaller writes, but that's up to the user to take this tradeoff,
> > > isn't it?
> > > 
> > > > You really need a strong reason to
> > > > support a maximum larger than 2M other than just "so the guest can
> > > > experiment with it".
> > > 
> > > Do I get you right that your suggestion is to cap the block size
> > > property at 2MB?
> > > 
> > > Thanks,
> > > Roman.
> > 
> > Ping?
> 
> Ping?

Eric, I think this was a question for you.

But anyway, capping at 2 MB sounds reasonable enough to me.

Kevin

> > > > > 
> > > > > Besides this is a property that is set explicitly, so I don't see a
> > > > > problem leaving this up to the user.
> > > > > 
> > > > > > particularly since qcow2 refuses to use cluster sizes larger than 
> > > > > > 2M and it
> > > > > > makes no sense to allow a block size larger than a cluster size.
> > > > > 
> > > > > This still doesn't contradict passing a bigger value to the guest, for
> > > > > experimenting if nothing else.
> > > > > 
> > > > > Thanks,
> > > > > Roman.
> > > > > 
> > > > 
> > > > -- 
> > > > Eric Blake, Principal Software Engineer
> > > > Red Hat, Inc.   +1-919-301-3226
> > > > Virtualization:  qemu.org | libvirt.org
> 

Re: [PATCH 4/5] block/io: fix bdrv_co_do_pwrite_zeroes head calculation

[Vladimir Sementsov-Ogievskiy]

14.03.2020 0:47, Eric Blake wrote:

On 3/2/20 4:05 AM, Vladimir Sementsov-Ogievskiy wrote:

It's wrong to update head using num in this place, as num may be
reduced during the iteration, and we'll have wrong head value on next
iteration.

Instead update head at iteration end.

Cc: qemu-sta...@nongnu.org
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/io.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)


Offhand, I don't see how this fixes any bug
/me reads on



diff --git a/block/io.c b/block/io.c
index 75fd5600c2..c64566b4cf 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1785,7 +1785,6 @@ static int coroutine_fn 
bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
   * convenience, limit this request to max_transfer even if
   * we don't need to fall back to writes.  */
  num = MIN(MIN(bytes, max_transfer), alignment - head);
-    head = (head + num) % alignment;
  assert(num < max_write_zeroes);


Here, we've asserted that if head was non-zero, num was already smaller than 
max_write_zeroes.  The rest of the loop does indeed have code that appears like 
it can reduce num, but that code is guarded:

     /* limit request size */
     if (num > max_write_zeroes) {
     num = max_write_zeroes;
     }
...
     if (ret == -ENOTSUP && !(flags & BDRV_REQ_NO_FALLBACK)) {
     /* Fall back to bounce buffer if write zeroes is unsupported */
     BdrvRequestFlags write_flags = flags & ~BDRV_REQ_ZERO_WRITE;

     if ((flags & BDRV_REQ_FUA) &&
     !(bs->supported_write_flags & BDRV_REQ_FUA)) {
     /* No need for bdrv_driver_pwrite() to do a fallback
  * flush on each chunk; use just one at the end */
     write_flags &= ~BDRV_REQ_FUA;
     need_flush = true;
     }
     num = MIN(num, max_transfer);


Now I think that this is impossible. If we updated head above, than num is 
already less or equal to max_transfer, so is not updated by this line. Or do I 
now miss something?

So, the patch may be good as refactoring but not really needed for 5.0.



Oh. Now I see.  If max_write_zeroes is > max_transfer, but we fall back to a 
bounce buffer, it is indeed possible that a misaligned request that forces 
fallbacks to writes may indeed require more than one write to get to the point 
where it is then using a buffer aligned to max_write_zeroes.

Do we have an iotest provoking this, or is it theoretical?  With an iotest, 
this one is material for 5.0 even if the rest of the series misses soft freeze.


  } else if (tail && num > alignment) {
  /* Shorten the request to the last aligned sector.  */
@@ -1844,6 +1843,9 @@ static int coroutine_fn 
bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
  offset += num;
  bytes -= num;
+    if (head) {
+    head = offset % alignment;
+    }


Reviewed-by: Eric Blake 




--
Best regards,
Vladimir

Re: [PATCH] iotests: drop group file

[Kevin Wolf]

Am 24.03.2020 um 08:41 hat Vladimir Sementsov-Ogievskiy geschrieben:
> When sending iotests to upstream or do patch porting from one branch
> to another we very often have to resolve conflicts in group file, as
> many absolutely independent features are intersecting by this file.
> These conflicts are simple, but imagine how much time we all have
> already spent on resolving them? Let's finally get rid of group file.
> 
> This patch transposes group info: instead of collecting it in one file,
> let each test define its groups by itself.
> 
> Several steps are done to achive it:
> 
> 1. Define groups in test files automatically:
> 
> grep '^[0-9]\{3\} ' group | while read line; do
> file=$(awk '{print $1}' <<< "$line");
> groups=$(sed -e 's/^... //' <<< "$line");
> awk "NR==2{print \"# group: $groups\"}1" $file > tmp;
> cat tmp > $file;
> done
> 
> 2. Copy groups documentation into docs/devel/testing.rst, which already
>has a section about iotests.
> 
> 3. Modify check script to work without group file.
> 
>Here is a logic change: before, even if test do not belong to any
>group (only iotest 142 currently) it should be defined in group
>file. Now, test is not forced to define any group. Instead check
>considers all files with names matching [0-9][0-9][0-9] as tests.

This has both a positive and a negative effect: Now you don't have to
modify another file when you add a new test, but it will be picked up
automatically. However, if you want to disable a test, you could
previously just remove it from groups (or comment it out), and now you
have to delete the test instead.

Downstream, I think we still disable a few tests because we're compiling
out features that are required for some tests to pass, and deleting the
test cases completely would probably move conflicts just to a different
place.

So I think we need a method to distuingish an enabled test that is in no
group from a disabled test.

>check script is also refactored to make it simple to do next cool
>thing about iotests: allow meaningful names for test-case files.

This one would also require us to be able to distinguish test case files
from arbitrary other files.

> -#
> -# test-group association ... one line per test
> -#
> -001 rw auto quick
> -002 rw auto quick
> -003 rw auto
> -004 rw auto quick
> -005 img auto quick
> -# 006 was removed, do not reuse

We lose these comments without a replacement. I wonder whether it's
important or if we can think of another way to make sure that numbers
aren't reused. (I'm not completely sure any more why we decided that we
don't want to reuse numbers. Maybe because of backports?)

Kevin

Re: [PULL for-5.0 0/1] Block patches

[Peter Maydell]

On Mon, 23 Mar 2020 at 19:24, Stefan Hajnoczi  wrote:
>
> The following changes since commit 29e0855c5af62bbb0b0b6fed792e004dad92ba95:
>
>   Merge remote-tracking branch 'remotes/elmarco/tags/slirp-pull-request' into 
> staging (2020-03-22 21:00:38 +)
>
> are available in the Git repository at:
>
>   https://github.com/stefanha/qemu.git tags/block-pull-request
>
> for you to fetch changes up to ff807d559205a434fd37d3343f01a0ab128bd190:
>
>   aio-posix: fix io_uring with external events (2020-03-23 11:05:44 +)
>
> 
> Pull request
>
> 
>
> Stefan Hajnoczi (1):
>   aio-posix: fix io_uring with external events
>
>  util/fdmon-io_uring.c | 13 -
>  1 file changed, 12 insertions(+), 1 deletion(-)


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/5.0
for any user-visible changes.

-- PMM

Re: Potential Null dereference

[Kevin Wolf]

Am 24.03.2020 um 08:14 hat Philippe Mathieu-Daudé geschrieben:
> On 3/24/20 4:05 AM, Mansour Ahmadi wrote:
> > Hi,
> > 
> > Nullness of  needs to be checked here:
> > https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/commit.c#L221
> > 
> > pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),...

Do you have a reproducer? It's not obvious to me how bs->backing could
be NULL here.

> > 
> > While it is done at 2 other locations:
> > https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/backup-top.c#L113
> > https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/mirror.c#L1477

Commit 18775ff3269 made the change for mirror, however its commit
message is terse and doesn't say anything about the scenario where it
would happen. We also didn't add a test case for it. I would have
expected that failure to add the backing file would immediately error
out and not try to refresh the filename first.

backup-top.c has the check from the beginning. I assume it just copied
it from mirror.

Vladimir, do you remember the details?

Kevin

Re: [PATCH] iotests: drop group file

[Daniel P . Berrangé]

On Tue, Mar 24, 2020 at 10:36:29AM +0100, Kevin Wolf wrote:
> Am 24.03.2020 um 08:41 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > When sending iotests to upstream or do patch porting from one branch
> > to another we very often have to resolve conflicts in group file, as
> > many absolutely independent features are intersecting by this file.
> > These conflicts are simple, but imagine how much time we all have
> > already spent on resolving them? Let's finally get rid of group file.
> > 
> > This patch transposes group info: instead of collecting it in one file,
> > let each test define its groups by itself.
> > 
> > Several steps are done to achive it:
> > 
> > 1. Define groups in test files automatically:
> > 
> > grep '^[0-9]\{3\} ' group | while read line; do
> > file=$(awk '{print $1}' <<< "$line");
> > groups=$(sed -e 's/^... //' <<< "$line");
> > awk "NR==2{print \"# group: $groups\"}1" $file > tmp;
> > cat tmp > $file;
> > done
> > 
> > 2. Copy groups documentation into docs/devel/testing.rst, which already
> >has a section about iotests.
> > 
> > 3. Modify check script to work without group file.
> > 
> >Here is a logic change: before, even if test do not belong to any
> >group (only iotest 142 currently) it should be defined in group
> >file. Now, test is not forced to define any group. Instead check
> >considers all files with names matching [0-9][0-9][0-9] as tests.
> 
> This has both a positive and a negative effect: Now you don't have to
> modify another file when you add a new test, but it will be picked up
> automatically. However, if you want to disable a test, you could
> previously just remove it from groups (or comment it out), and now you
> have to delete the test instead.
> 
> Downstream, I think we still disable a few tests because we're compiling
> out features that are required for some tests to pass, and deleting the
> test cases completely would probably move conflicts just to a different
> place.
> 
> So I think we need a method to distuingish an enabled test that is in no
> group from a disabled test.


Can we just have a file "blacklist.local" (which doesn't exist by default)
where you list tests to skip locally ?

Or a "group.local" such that any info in this group.local file will replace
the default info extracted from the test file ?

> >check script is also refactored to make it simple to do next cool
> >thing about iotests: allow meaningful names for test-case files.
> 
> This one would also require us to be able to distinguish test case files
> from arbitrary other files.

A test-X.sh or a XX.test  naming convention ?

> > -#
> > -# test-group association ... one line per test
> > -#
> > -001 rw auto quick
> > -002 rw auto quick
> > -003 rw auto
> > -004 rw auto quick
> > -005 img auto quick
> > -# 006 was removed, do not reuse
> 
> We lose these comments without a replacement. I wonder whether it's
> important or if we can think of another way to make sure that numbers
> aren't reused. (I'm not completely sure any more why we decided that we
> don't want to reuse numbers. Maybe because of backports?)

The key goal of the patch is to avoid conflicts from clashing changes
between branches. To full achieve that goal we need to both avoid
touching the shared groups file, but more than than, we must avoid
creating clashing test filenames.

If we keep using 3-digit filenames then the goal of this patch is
not achieved, as the risk of clashes is still higher. IOW, we must
switch to a more verbose naming convention to increase the entropy
and thus eliminate risk of clashes. If this is done, then the idea
of reserving previously used test names ceases to be something to
worry about.

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

Re: [PATCH] iotests: drop group file

[Vladimir Sementsov-Ogievskiy]

24.03.2020 12:36, Kevin Wolf wrote:

Am 24.03.2020 um 08:41 hat Vladimir Sementsov-Ogievskiy geschrieben:

When sending iotests to upstream or do patch porting from one branch
to another we very often have to resolve conflicts in group file, as
many absolutely independent features are intersecting by this file.
These conflicts are simple, but imagine how much time we all have
already spent on resolving them? Let's finally get rid of group file.

This patch transposes group info: instead of collecting it in one file,
let each test define its groups by itself.

Several steps are done to achive it:

1. Define groups in test files automatically:

 grep '^[0-9]\{3\} ' group | while read line; do
 file=$(awk '{print $1}' <<< "$line");
 groups=$(sed -e 's/^... //' <<< "$line");
 awk "NR==2{print \"# group: $groups\"}1" $file > tmp;
 cat tmp > $file;
 done

2. Copy groups documentation into docs/devel/testing.rst, which already
has a section about iotests.

3. Modify check script to work without group file.

Here is a logic change: before, even if test do not belong to any
group (only iotest 142 currently) it should be defined in group
file. Now, test is not forced to define any group. Instead check
considers all files with names matching [0-9][0-9][0-9] as tests.


This has both a positive and a negative effect: Now you don't have to
modify another file when you add a new test, but it will be picked up
automatically. However, if you want to disable a test, you could
previously just remove it from groups (or comment it out), and now you
have to delete the test instead.


Hmm. Probably, you could add it to group "disabled", and run check -x disabled.



Downstream, I think we still disable a few tests because we're compiling
out features that are required for some tests to pass, and deleting the
test cases completely would probably move conflicts just to a different
place.


I came to fixing all test in downstream, correctly skipping test-cases which
needs compiled-out feature. We don't comment out tests in group, all tests
should pass or be correctly skipped by check.



So I think we need a method to distuingish an enabled test that is in no
group from a disabled test.


What about "disabled" group?




check script is also refactored to make it simple to do next cool
thing about iotests: allow meaningful names for test-case files.


This one would also require us to be able to distinguish test case files
from arbitrary other files.


I think, something like "all files started with 'test-' prefix"




-#
-# test-group association ... one line per test
-#
-001 rw auto quick
-002 rw auto quick
-003 rw auto
-004 rw auto quick
-005 img auto quick
-# 006 was removed, do not reuse


We lose these comments without a replacement. I wonder whether it's
important or if we can think of another way to make sure that numbers
aren't reused. (I'm not completely sure any more why we decided that we
don't want to reuse numbers. Maybe because of backports?)



Hmm.. Okay, if we had this test in past, than drop, than reuse number for the
another test, we possibly may break bisecting.

I can add notes about it into documentation (nobody will read it). Anyway, I
don't think it is too bad.

And if we move to textual file-names for tests, we will not reuse old removed
numbers anyway.

--
Best regards,
Vladimir

Re: [PATCH] iotests: drop group file

[Vladimir Sementsov-Ogievskiy]

24.03.2020 12:51, Daniel P. Berrangé wrote:

On Tue, Mar 24, 2020 at 10:36:29AM +0100, Kevin Wolf wrote:

Am 24.03.2020 um 08:41 hat Vladimir Sementsov-Ogievskiy geschrieben:

When sending iotests to upstream or do patch porting from one branch
to another we very often have to resolve conflicts in group file, as
many absolutely independent features are intersecting by this file.
These conflicts are simple, but imagine how much time we all have
already spent on resolving them? Let's finally get rid of group file.

This patch transposes group info: instead of collecting it in one file,
let each test define its groups by itself.

Several steps are done to achive it:

1. Define groups in test files automatically:

 grep '^[0-9]\{3\} ' group | while read line; do
 file=$(awk '{print $1}' <<< "$line");
 groups=$(sed -e 's/^... //' <<< "$line");
 awk "NR==2{print \"# group: $groups\"}1" $file > tmp;
 cat tmp > $file;
 done

2. Copy groups documentation into docs/devel/testing.rst, which already
has a section about iotests.

3. Modify check script to work without group file.

Here is a logic change: before, even if test do not belong to any
group (only iotest 142 currently) it should be defined in group
file. Now, test is not forced to define any group. Instead check
considers all files with names matching [0-9][0-9][0-9] as tests.


This has both a positive and a negative effect: Now you don't have to
modify another file when you add a new test, but it will be picked up
automatically. However, if you want to disable a test, you could
previously just remove it from groups (or comment it out), and now you
have to delete the test instead.

Downstream, I think we still disable a few tests because we're compiling
out features that are required for some tests to pass, and deleting the
test cases completely would probably move conflicts just to a different
place.

So I think we need a method to distuingish an enabled test that is in no
group from a disabled test.



Can we just have a file "blacklist.local" (which doesn't exist by default)
where you list tests to skip locally ?

Or a "group.local" such that any info in this group.local file will replace
the default info extracted from the test file ?


check script is also refactored to make it simple to do next cool
thing about iotests: allow meaningful names for test-case files.


This one would also require us to be able to distinguish test case files
from arbitrary other files.


A test-X.sh or a XX.test  naming convention ?


-#
-# test-group association ... one line per test
-#
-001 rw auto quick
-002 rw auto quick
-003 rw auto
-004 rw auto quick
-005 img auto quick
-# 006 was removed, do not reuse


We lose these comments without a replacement. I wonder whether it's
important or if we can think of another way to make sure that numbers
aren't reused. (I'm not completely sure any more why we decided that we
don't want to reuse numbers. Maybe because of backports?)


The key goal of the patch is to avoid conflicts from clashing changes
between branches. To full achieve that goal we need to both avoid
touching the shared groups file, but more than than, we must avoid
creating clashing test filenames.

If we keep using 3-digit filenames then the goal of this patch is
not achieved, as the risk of clashes is still higher. IOW, we must
switch to a more verbose naming convention to increase the entropy
and thus eliminate risk of clashes. If this is done, then the idea
of reserving previously used test names ceases to be something to
worry about.


Hm. You are right, better to solve all these problems together.


--
Best regards,
Vladimir

Re: [PATCH] iotests: drop group file

[Denis V. Lunev]

On 3/24/20 12:36 PM, Kevin Wolf wrote:
> Am 24.03.2020 um 08:41 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> When sending iotests to upstream or do patch porting from one branch
>> to another we very often have to resolve conflicts in group file, as
>> many absolutely independent features are intersecting by this file.
>> These conflicts are simple, but imagine how much time we all have
>> already spent on resolving them? Let's finally get rid of group file.
>>
>> This patch transposes group info: instead of collecting it in one file,
>> let each test define its groups by itself.
>>
>> Several steps are done to achive it:
>>
>> 1. Define groups in test files automatically:
>>
>> grep '^[0-9]\{3\} ' group | while read line; do
>> file=$(awk '{print $1}' <<< "$line");
>> groups=$(sed -e 's/^... //' <<< "$line");
>> awk "NR==2{print \"# group: $groups\"}1" $file > tmp;
>> cat tmp > $file;
>> done
>>
>> 2. Copy groups documentation into docs/devel/testing.rst, which already
>>has a section about iotests.
>>
>> 3. Modify check script to work without group file.
>>
>>Here is a logic change: before, even if test do not belong to any
>>group (only iotest 142 currently) it should be defined in group
>>file. Now, test is not forced to define any group. Instead check
>>considers all files with names matching [0-9][0-9][0-9] as tests.
> This has both a positive and a negative effect: Now you don't have to
> modify another file when you add a new test, but it will be picked up
> automatically. However, if you want to disable a test, you could
> previously just remove it from groups (or comment it out), and now you
> have to delete the test instead.
>
> Downstream, I think we still disable a few tests because we're compiling
> out features that are required for some tests to pass, and deleting the
> test cases completely would probably move conflicts just to a different
> place.
>
> So I think we need a method to distuingish an enabled test that is in no
> group from a disabled test.
+1 for blacklist.local file

>
>>check script is also refactored to make it simple to do next cool
>>thing about iotests: allow meaningful names for test-case files.
> This one would also require us to be able to distinguish test case files
> from arbitrary other files.
>
>> -#
>> -# test-group association ... one line per test
>> -#
>> -001 rw auto quick
>> -002 rw auto quick
>> -003 rw auto
>> -004 rw auto quick
>> -005 img auto quick
>> -# 006 was removed, do not reuse
> We lose these comments without a replacement. I wonder whether it's
> important or if we can think of another way to make sure that numbers
> aren't reused. (I'm not completely sure any more why we decided that we
> don't want to reuse numbers. Maybe because of backports?)
we could generate this file with a starter script with proper
option.

Den

Re: block stream and bitmaps

[Kevin Wolf]

Am 23.03.2020 um 19:06 hat John Snow geschrieben:
> Hi Kevin,
> 
> I'm hoping to get some preliminary ideas from you (capped at five
> minutes' effort?) on this subject. My ideas here are a bit shaky, I only
> have really rough notions here.
> 
> We want to use bitmaps with 'drive' semantics; i.e. tracking changes to
> the visible guest data. What we have are bitmaps with node semantics,
> tracking changes to the data at a particular level in the graph.
> 
> For commit, this isn't a big deal: we can disable bitmaps in the backing
> file while we commit and then re-enable it on completion. We usually
> have a separate bitmap enabled on the root node that is recording writes
> during this time, and can be moved later.
> 
> For streaming, this is more challenging: new writes will dirty the
> bitmap, but so will writes from the stream job itself.
> 
> Semantically, we want to ignore writes from the stream while recording
> them from the guest -- differentiating based on source.

No, based on source is actually not what you want. What you really want
is that BDRV_REQ_WRITE_UNCHANGED doesn't mark any blocks dirty.

We discussed this specific case of streaming at FOSDEM (with Paolo and
probably Nir). Paolo was even convinced that unchanged writes already
behave like this, but we agreed that dirtying blocks for them would be a
bug. After checking that the code is indeed buggy, I was planning to
send a patch, but never got around to actually do that. Sorry about
that.

> Bitmaps aren't really geared to do that right now. With the changes to
> Bdrv Roles that Max was engineering, do you think it's possible to add
> some kind of write source discrimination to bitmaps, or is that too messy?

I don't think it would work because copy-on-read requests come from the
same parent node as writes (no matter whether the legacy code in
block/io.c or a copy-on-read filter node is used).

> For both commit and stream, it might be nice to say: "This bitmap is
> enabled, but ignores writes from [all? specific types? specific
> instances?] jobs.

Commit is a bit trickier, because it's not WRITE_UNCHANGED. The result
is only unchanged for the top layer, but not for the backing file you're
committing to. Not sure if we can represent this condition somehow.

> Or, I wonder if what we truly want is some kind of bitmap "forwarder"
> object on block-backend objects that represent the semantic drive view,
> and only writes through that *backend* get forwarded to the bitmaps
> attached to whatever node the bitmap is actually associated with.
> 
> (That might wind up causing weird problems too, though... since those
> objects are no longer intended to be user-addressable, managing that
> configuration might get intensely strange.)

Hm... Drive-based does suggest that it's managed at the BlockBackend
level. So having a bitmap that isn't added as a dirty bitmap to the BDS,
but only to the BB does make sense to me. The BB would be addressed
with the qdev ID of the device, as usual (which underlines that it's
really per device).

The part that's unclear to me is how to make such bitmaps persistent.
You can change the root node of a BB and even remove the root node
completely (for removable devices; but even changing is technically
remove followed by insert), so you may need to move the bitmap around
between image files and at least for some time you might not have any
place to store the bitmap.

Or you say that you store it in one specific node, be it the root node
of the BB or not, and it will always stay there no matter how you change
the graph and whether the BB and that node are even in the same subtree.
That node would just get an additonal refcount, so you can't remove it
until the BB goes away.

Unless you already have a better plan (I hope you do, I didn't think
about it for more than a few minutes), maybe the latter would actually
be the most reasonable solution.

Kevin

Re: [PATCH v2 07/12] tests/acceptance: Remove shebang header

[Alex Bennée]

Philippe Mathieu-Daudé  writes:

> Patch created mechanically by running:
>
>   $ chmod 644 $(git grep -lF '#!/usr/bin/env python' \
>   | xargs grep -L 'if __name__.*__main__')
>   $ sed -i "/^#\!\/usr\/bin\/\(env\ \)\?python.\?$/d" \
>   $(git grep -lF '#!/usr/bin/env python' \
>   | xargs grep -L 'if __name__.*__main__')

OK, but my question is why? Aren't shebangs considered good practice for
finding the executable for a script?

If the acceptance scripts are special in this regard we should say why
in the commit message.

>
> Reported-by: Vladimir Sementsov-Ogievskiy 
> Suggested-by: Stefan Hajnoczi 
> Reviewed-by: Wainer dos Santos Moschetta 
> Acked-by: Stefan Hajnoczi 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  tests/acceptance/virtio_seg_max_adjust.py  | 1 -
>  tests/acceptance/x86_cpu_model_versions.py | 1 -
>  2 files changed, 2 deletions(-)
>  mode change 100755 => 100644 tests/acceptance/virtio_seg_max_adjust.py
>
> diff --git a/tests/acceptance/virtio_seg_max_adjust.py 
> b/tests/acceptance/virtio_seg_max_adjust.py
> old mode 100755
> new mode 100644
> index 5458573138..8d4f24da49
> --- a/tests/acceptance/virtio_seg_max_adjust.py
> +++ b/tests/acceptance/virtio_seg_max_adjust.py
> @@ -1,4 +1,3 @@
> -#!/usr/bin/env python
>  #
>  # Test virtio-scsi and virtio-blk queue settings for all machine types
>  #
> diff --git a/tests/acceptance/x86_cpu_model_versions.py 
> b/tests/acceptance/x86_cpu_model_versions.py
> index 90558d9a71..01ff614ec2 100644
> --- a/tests/acceptance/x86_cpu_model_versions.py
> +++ b/tests/acceptance/x86_cpu_model_versions.py
> @@ -1,4 +1,3 @@
> -#!/usr/bin/env python
>  #
>  # Basic validation of x86 versioned CPU models and CPU model aliases
>  #


-- 
Alex Bennée

Re: [PATCH] iotests: drop group file

[Daniel P . Berrangé]

On Tue, Mar 24, 2020 at 01:02:18PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 24.03.2020 12:36, Kevin Wolf wrote:
> > Am 24.03.2020 um 08:41 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > When sending iotests to upstream or do patch porting from one branch
> > > to another we very often have to resolve conflicts in group file, as
> > > many absolutely independent features are intersecting by this file.
> > > These conflicts are simple, but imagine how much time we all have
> > > already spent on resolving them? Let's finally get rid of group file.
> > > 
> > > This patch transposes group info: instead of collecting it in one file,
> > > let each test define its groups by itself.
> > > 
> > > Several steps are done to achive it:
> > > 
> > > 1. Define groups in test files automatically:
> > > 
> > >  grep '^[0-9]\{3\} ' group | while read line; do
> > >  file=$(awk '{print $1}' <<< "$line");
> > >  groups=$(sed -e 's/^... //' <<< "$line");
> > >  awk "NR==2{print \"# group: $groups\"}1" $file > tmp;
> > >  cat tmp > $file;
> > >  done
> > > 
> > > 2. Copy groups documentation into docs/devel/testing.rst, which already
> > > has a section about iotests.
> > > 
> > > 3. Modify check script to work without group file.
> > > 
> > > Here is a logic change: before, even if test do not belong to any
> > > group (only iotest 142 currently) it should be defined in group
> > > file. Now, test is not forced to define any group. Instead check
> > > considers all files with names matching [0-9][0-9][0-9] as tests.
> > 
> > This has both a positive and a negative effect: Now you don't have to
> > modify another file when you add a new test, but it will be picked up
> > automatically. However, if you want to disable a test, you could
> > previously just remove it from groups (or comment it out), and now you
> > have to delete the test instead.
> 
> Hmm. Probably, you could add it to group "disabled", and run check -x 
> disabled.

As a developer you don't really want to be making changes to git tracked
files in order to temporarily skip a test, as then git reports them as
modified & you risk accidentally committing throwaway changes.

Better to have a separate groups.local file to record local overrides
in a non-tracked file.


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

Re: [PATCH] iotests: drop group file

[Kevin Wolf]

Am 24.03.2020 um 11:21 hat Daniel P. Berrangé geschrieben:
> On Tue, Mar 24, 2020 at 01:02:18PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > 24.03.2020 12:36, Kevin Wolf wrote:
> > > Am 24.03.2020 um 08:41 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > > When sending iotests to upstream or do patch porting from one branch
> > > > to another we very often have to resolve conflicts in group file, as
> > > > many absolutely independent features are intersecting by this file.
> > > > These conflicts are simple, but imagine how much time we all have
> > > > already spent on resolving them? Let's finally get rid of group file.
> > > > 
> > > > This patch transposes group info: instead of collecting it in one file,
> > > > let each test define its groups by itself.
> > > > 
> > > > Several steps are done to achive it:
> > > > 
> > > > 1. Define groups in test files automatically:
> > > > 
> > > >  grep '^[0-9]\{3\} ' group | while read line; do
> > > >  file=$(awk '{print $1}' <<< "$line");
> > > >  groups=$(sed -e 's/^... //' <<< "$line");
> > > >  awk "NR==2{print \"# group: $groups\"}1" $file > tmp;
> > > >  cat tmp > $file;
> > > >  done
> > > > 
> > > > 2. Copy groups documentation into docs/devel/testing.rst, which already
> > > > has a section about iotests.
> > > > 
> > > > 3. Modify check script to work without group file.
> > > > 
> > > > Here is a logic change: before, even if test do not belong to any
> > > > group (only iotest 142 currently) it should be defined in group
> > > > file. Now, test is not forced to define any group. Instead check
> > > > considers all files with names matching [0-9][0-9][0-9] as tests.
> > > 
> > > This has both a positive and a negative effect: Now you don't have to
> > > modify another file when you add a new test, but it will be picked up
> > > automatically. However, if you want to disable a test, you could
> > > previously just remove it from groups (or comment it out), and now you
> > > have to delete the test instead.
> > 
> > Hmm. Probably, you could add it to group "disabled", and run check -x 
> > disabled.
> 
> As a developer you don't really want to be making changes to git tracked
> files in order to temporarily skip a test, as then git reports them as
> modified & you risk accidentally committing throwaway changes.
> 
> Better to have a separate groups.local file to record local overrides
> in a non-tracked file.

For locally disabling tests, we have that 'expunged' file that nobody
knows about and that I just found when we were reformatting the ./check
output...

I wouldn't want to use that for disabling the tests in a downstream
repository, though, it should stay a local thing even there.

Kevin

Re: [PATCH] iotests: Fix cleanup path in some tests

[Max Reitz]

On 24.02.20 18:16, Max Reitz wrote:
> Some iotests leave behind some external data file when run for qcow2
> with -o data_file.  Fix that.
> 
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/085 | 1 +
>  tests/qemu-iotests/087 | 6 ++
>  tests/qemu-iotests/279 | 2 +-
>  3 files changed, 8 insertions(+), 1 deletion(-)

Thanks for the review, applied to my block branch.

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] iotests/026: Move v3-exclusive test to new file

[Max Reitz]

On 11.03.20 15:07, Max Reitz wrote:
> data_file does not work with v2, and we probably want 026 to keep
> working for v2 images.  Thus, open a new file for v3-exclusive error
> path test cases.
> 
> Fixes: 81311255f217859413c94f2cd9cebf2684bbda94
>(“iotests/026: Test EIO on allocation in a data-file”)
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/026 | 31 ---
>  tests/qemu-iotests/026.out |  6 --
>  tests/qemu-iotests/026.out.nocache |  6 --
>  tests/qemu-iotests/289 | 89 ++
>  tests/qemu-iotests/289.out |  8 +++
>  tests/qemu-iotests/group   |  1 +
>  6 files changed, 98 insertions(+), 43 deletions(-)
>  create mode 100755 tests/qemu-iotests/289
>  create mode 100644 tests/qemu-iotests/289.out

Thanks for the review and test, applied to my block branch.

Max



signature.asc
Description: OpenPGP digital signature

Re: Potential Null dereference

[Vladimir Sementsov-Ogievskiy]

24.03.2020 12:50, Kevin Wolf wrote:

Am 24.03.2020 um 08:14 hat Philippe Mathieu-Daudé geschrieben:

On 3/24/20 4:05 AM, Mansour Ahmadi wrote:

Hi,

Nullness of  needs to be checked here:
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/commit.c#L221

pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),...


Do you have a reproducer? It's not obvious to me how bs->backing could
be NULL here.



While it is done at 2 other locations:
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/backup-top.c#L113
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/mirror.c#L1477


Commit 18775ff3269 made the change for mirror, however its commit
message is terse and doesn't say anything about the scenario where it
would happen. We also didn't add a test case for it. I would have
expected that failure to add the backing file would immediately error
out and not try to refresh the filename first.

backup-top.c has the check from the beginning. I assume it just copied
it from mirror.

Vladimir, do you remember the details?



No :(
But I believe that "Backing may be zero after failed bdrv_attach_child in 
bdrv_set_backing_hd, which leads to SIGSEGV." means real problem, probably 
reproduced on some experiment branch, not on master.

Let's try most simple check:

apply the following:

=
diff --git a/include/block/block.h b/include/block/block.h
index e569a4d747..dc20b55075 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -298,6 +298,8 @@ BdrvChild *bdrv_open_child(const char *filename,
const BdrvChildRole *child_role,
bool allow_none, Error **errp);
 BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp);
+void bdrv_set_backing_hd_ex(BlockDriverState *bs, BlockDriverState *backing_hd,
+bool bdrv_attach_child_fail, Error **errp);
 void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
  Error **errp);
 int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
diff --git a/block.c b/block.c
index a2542c977b..21b6124d73 100644
--- a/block.c
+++ b/block.c
@@ -2743,8 +2743,8 @@ static bool bdrv_inherits_from_recursive(BlockDriverState 
*child,
  * Sets the backing file link of a BDS. A new reference is created; callers
  * which don't need their own reference any more must call bdrv_unref().
  */
-void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
- Error **errp)
+void bdrv_set_backing_hd_ex(BlockDriverState *bs, BlockDriverState *backing_hd,
+bool bdrv_attach_child_fail, Error **errp)
 {
 bool update_inherits_from = bdrv_chain_contains(bs, backing_hd) &&
 bdrv_inherits_from_recursive(backing_hd, bs);
@@ -2766,8 +2766,14 @@ void bdrv_set_backing_hd(BlockDriverState *bs, 
BlockDriverState *backing_hd,
 goto out;
 }

-bs->backing = bdrv_attach_child(bs, backing_hd, "backing", &child_backing,
-errp);
+if (bdrv_attach_child_fail) {
+bs->backing = NULL;
+error_setg(errp, "Unpredicted failure :)");
+} else {
+bs->backing = bdrv_attach_child(bs, backing_hd, "backing", 
&child_backing,
+errp);
+}
+
 /* If backing_hd was already part of bs's backing chain, and
  * inherits_from pointed recursively to bs then let's update it to
  * point directly to bs (else it will become NULL). */
@@ -2779,6 +2785,12 @@ out:
 bdrv_refresh_limits(bs, NULL);
 }

+void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
+ Error **errp)
+{
+bdrv_set_backing_hd_ex(bs, backing_hd, false, errp);
+}
 /*
  * Opens the backing file for a BlockDriverState if not yet open
  *
diff --git a/block/mirror.c b/block/mirror.c
index 447051dbc6..907bb2b718 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -675,7 +675,7 @@ static int mirror_exit_common(Job *job)
 if (!abort && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
 BlockDriverState *backing = s->is_none_mode ? src : s->base;
 if (backing_bs(target_bs) != backing) {
-bdrv_set_backing_hd(target_bs, backing, &local_err);
+bdrv_set_backing_hd_ex(target_bs, backing, true, &local_err);
 if (local_err) {
 error_report_err(local_err);
 ret = -EPERM;
@@ -1477,6 +1477,7 @@ static void 
bdrv_mirror_top_refresh_filename(BlockDriverState *bs)
 if (bs->backing == NULL) {
 /* we can be here after failed bdrv_attach_child in
  * bdrv_set_backing_hd */
+abort();
 return;
 }
 pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),


==

[root@kvm qemu-iotests]# git grep -il mirror ?

Re: [PATCH] qcow2: Forbid discard in qcow2 v2 images with backing files

[Max Reitz]

On 23.03.20 20:44, Alberto Garcia wrote:
> A discard request deallocates the selected clusters so they read back
> as zeroes. This is done by clearing the cluster offset field and
> setting QCOW_OFLAG_ZERO in the L2 entry.
> 
> This flag is however only supported when qcow_version >= 3. In older
> images the cluster is simply deallocated, exposing any possible stale
> data from the backing file.
> 
> Since discard is an advisory operation it's safer to simply forbid it
> in this scenario.
> 
> Note that we are adding this check to qcow2_co_pdiscard() and not to
> qcow2_cluster_discard() or discard_in_l2_slice() because the last
> two are also used by qcow2_snapshot_create() to discard the clusters
> used by the VM state. In this case there's no risk of exposing stale
> data to the guest and we really want that the clusters are always
> discarded.

Sounds good to me.

> Signed-off-by: Alberto Garcia 
> ---
>  block/qcow2.c  |  6 +++
>  tests/qemu-iotests/060 |  5 ++-
>  tests/qemu-iotests/060.out |  2 -
>  tests/qemu-iotests/289 | 90 ++
>  tests/qemu-iotests/289.out | 52 ++
>  tests/qemu-iotests/group   |  1 +
>  6 files changed, 152 insertions(+), 4 deletions(-)
>  create mode 100755 tests/qemu-iotests/289
>  create mode 100644 tests/qemu-iotests/289.out
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index d44b45633d..7bb7e392e1 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3763,6 +3763,12 @@ static coroutine_fn int 
> qcow2_co_pdiscard(BlockDriverState *bs,
>  int ret;
>  BDRVQcow2State *s = bs->opaque;
>  
> +/* If the image does not support QCOW_OFLAG_ZERO then discarding
> + * clusters could expose stale data from the backing file. */
> +if (s->qcow_version < 3 && bs->backing) {
> +return -ENOTSUP;
> +}
> +
>  if (!QEMU_IS_ALIGNED(offset | bytes, s->cluster_size)) {
>  assert(bytes < s->cluster_size);
>  /* Ignore partial clusters, except for the special case of the
> diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
> index 043f12904a..4a4fdfb1e1 100755
> --- a/tests/qemu-iotests/060
> +++ b/tests/qemu-iotests/060
> @@ -167,9 +167,10 @@ _make_test_img -o 'compat=0.10' -b "$BACKING_IMG" 1G

More context: This image is created with -o 'compat=0.10', just because
a discard on such an image would result in the cluster being freed.  We
can drop that compat=0.10 bit now.

>  # Write two clusters, the second one enforces creation of an L2 table after
>  # the first data cluster.
>  $QEMU_IO -c 'write 0k 64k' -c 'write 512M 64k' "$TEST_IMG" | _filter_qemu_io
> -# Discard the first cluster. This cluster will soon enough be reallocated and
> +# Free the first cluster. This cluster will soon enough be reallocated and
>  # used for COW.
> -$QEMU_IO -c 'discard 0k 64k' "$TEST_IMG" | _filter_qemu_io
> +poke_file "$TEST_IMG" '262144' "\x00\x00\x00\x00\x00\x00\x00\x00" # 0x4 
> - L2 entry
> +poke_file "$TEST_IMG" '131082' "\x00\x00" # 0x2000a - Refcount entry
>  # Now, corrupt the image by marking the second L2 table cluster as free.
>  poke_file "$TEST_IMG" '131084' "\x00\x00" # 0x2000c
>  # Start a write operation requiring COW on the image stopping it right before

[...]

> diff --git a/tests/qemu-iotests/289 b/tests/qemu-iotests/289
> new file mode 100755
> index 00..13b4984721
> --- /dev/null
> +++ b/tests/qemu-iotests/289

[...]

> +_cleanup()
> +{
> +_cleanup_test_img
> +rm -f "$TEST_IMG.backing"

I’d call the image $TEST_IMG.base so _cleanup_test_img picks up on it.
(rm-ing test images is also wrong, because with external data files,
there will be more than one file.  It doesn’t matter here anyway because
this test doesn’t support external data files, but, well.)

> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +
> +_supported_fmt qcow2
> +_supported_proto file
> +_supported_os Linux

I’d mark the compat option unsupported because this test will ignore it.
 Furthermore, the refcount_bits and data_file options are really
unsupported, because they won’t work with compat=0.10.

The test itself looks good.

Max



signature.asc
Description: OpenPGP digital signature

[PATCH v2] qcow2: Forbid discard in qcow2 v2 images with backing files

[Alberto Garcia]

A discard request deallocates the selected clusters so they read back
as zeroes. This is done by clearing the cluster offset field and
setting QCOW_OFLAG_ZERO in the L2 entry.

This flag is however only supported when qcow_version >= 3. In older
images the cluster is simply deallocated, exposing any possible stale
data from the backing file.

Since discard is an advisory operation it's safer to simply forbid it
in this scenario.

Note that we are adding this check to qcow2_co_pdiscard() and not to
qcow2_cluster_discard() or discard_in_l2_slice() because the last
two are also used by qcow2_snapshot_create() to discard the clusters
used by the VM state. In this case there's no risk of exposing stale
data to the guest and we really want that the clusters are always
discarded.

Signed-off-by: Alberto Garcia 
---
v2:

- Don't create the image with compat=0.10 in iotest 060 [Max]
- Use $TEST_IMG.base for the backing image name in iotest 289 [Max]
- Add list of unsupported options to iotest 289 [Max]

 block/qcow2.c  |  6 +++
 tests/qemu-iotests/060 | 10 ++---
 tests/qemu-iotests/060.out |  2 -
 tests/qemu-iotests/289 | 91 ++
 tests/qemu-iotests/289.out | 52 ++
 tests/qemu-iotests/group   |  1 +
 6 files changed, 154 insertions(+), 8 deletions(-)
 create mode 100755 tests/qemu-iotests/289
 create mode 100644 tests/qemu-iotests/289.out

diff --git a/block/qcow2.c b/block/qcow2.c
index d44b45633d..7bb7e392e1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3763,6 +3763,12 @@ static coroutine_fn int 
qcow2_co_pdiscard(BlockDriverState *bs,
 int ret;
 BDRVQcow2State *s = bs->opaque;
 
+/* If the image does not support QCOW_OFLAG_ZERO then discarding
+ * clusters could expose stale data from the backing file. */
+if (s->qcow_version < 3 && bs->backing) {
+return -ENOTSUP;
+}
+
 if (!QEMU_IS_ALIGNED(offset | bytes, s->cluster_size)) {
 assert(bytes < s->cluster_size);
 /* Ignore partial clusters, except for the special case of the
diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index 043f12904a..8820290be6 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -160,16 +160,14 @@ TEST_IMG=$BACKING_IMG _make_test_img 1G
 
 $QEMU_IO -c 'write 0k 64k' "$BACKING_IMG" | _filter_qemu_io
 
-# compat=0.10 is required in order to make the following discard actually
-# unallocate the sector rather than make it a zero sector - we want COW, after
-# all.
-_make_test_img -o 'compat=0.10' -b "$BACKING_IMG" 1G
+_make_test_img -b "$BACKING_IMG" 1G
 # Write two clusters, the second one enforces creation of an L2 table after
 # the first data cluster.
 $QEMU_IO -c 'write 0k 64k' -c 'write 512M 64k' "$TEST_IMG" | _filter_qemu_io
-# Discard the first cluster. This cluster will soon enough be reallocated and
+# Free the first cluster. This cluster will soon enough be reallocated and
 # used for COW.
-$QEMU_IO -c 'discard 0k 64k' "$TEST_IMG" | _filter_qemu_io
+poke_file "$TEST_IMG" '262144' "\x00\x00\x00\x00\x00\x00\x00\x00" # 0x4 - 
L2 entry
+poke_file "$TEST_IMG" '131082' "\x00\x00" # 0x2000a - Refcount entry
 # Now, corrupt the image by marking the second L2 table cluster as free.
 poke_file "$TEST_IMG" '131084' "\x00\x00" # 0x2000c
 # Start a write operation requiring COW on the image stopping it right before
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index d27692a33c..09caaea865 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -105,8 +105,6 @@ wrote 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote 65536/65536 bytes at offset 536870912
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-discard 65536/65536 bytes at offset 0
-64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 qcow2: Marking image as corrupt: Preventing invalid write on metadata 
(overlaps with active L2 table); further corruption events will be suppressed
 blkdebug: Suspended request '0'
 write failed: Input/output error
diff --git a/tests/qemu-iotests/289 b/tests/qemu-iotests/289
new file mode 100755
index 00..629c992d79
--- /dev/null
+++ b/tests/qemu-iotests/289
@@ -0,0 +1,91 @@
+#!/usr/bin/env bash
+#
+# Test how 'qemu-io -c discard' behaves on v2 and v3 qcow2 images
+#
+# Copyright (C) 2020 Igalia, S.L.
+# Author: Alberto Garcia 
+#
+# 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

[PULL 2/6] block: Assert BlockDriver::format_name is not NULL

[Max Reitz]

From: Philippe Mathieu-Daudé 

bdrv_do_find_format() calls strcmp() using BlockDriver::format_name
as argument, which must not be NULL. Assert this field is not null
when we register a block driver in bdrv_register().

Reported-by: Mansour Ahmadi 
Signed-off-by: Philippe Mathieu-Daudé 
Message-Id: <2020031835.23856-1-phi...@redhat.com>
Reviewed-by: Alberto Garcia 
Signed-off-by: Max Reitz 
---
 block.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block.c b/block.c
index a2542c977b..6b984dc883 100644
--- a/block.c
+++ b/block.c
@@ -363,6 +363,7 @@ char *bdrv_get_full_backing_filename(BlockDriverState *bs, 
Error **errp)
 
 void bdrv_register(BlockDriver *bdrv)
 {
+assert(bdrv->format_name);
 QLIST_INSERT_HEAD(&bdrv_drivers, bdrv, list);
 }
 
-- 
2.25.1

[PULL 1/6] block: Avoid memleak on qcow2 image info failure

[Max Reitz]

From: Eric Blake 

If we fail to get bitmap info, we must not leak the encryption info.

Fixes: b8968c875f403
Fixes: Coverity CID 1421894
Signed-off-by: Eric Blake 
Message-Id: <20200320183620.1112123-1-ebl...@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Andrey Shinkevich 
Tested-by: Andrey Shinkevich 
Signed-off-by: Max Reitz 
---
 block/qcow2.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index d44b45633d..e08917ed84 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4811,6 +4811,7 @@ static ImageInfoSpecific 
*qcow2_get_specific_info(BlockDriverState *bs,
 if (local_err) {
 error_propagate(errp, local_err);
 qapi_free_ImageInfoSpecific(spec_info);
+qapi_free_QCryptoBlockInfo(encrypt_info);
 return NULL;
 }
 *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
-- 
2.25.1

[PULL 4/6] block/qcow2: zero data_file child after free

[Max Reitz]

From: Vladimir Sementsov-Ogievskiy 

data_file being NULL doesn't seem to be a correct state, but it's
better than dead pointer and simpler to debug.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Message-Id: <20200316060631.30052-3-vsement...@virtuozzo.com>
Reviewed-by: John Snow 
Signed-off-by: Max Reitz 
---
 block/qcow2.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index e08917ed84..d1da3d91db 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1758,6 +1758,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
*bs, QDict *options,
 g_free(s->image_data_file);
 if (has_data_file(bs)) {
 bdrv_unref_child(bs, s->data_file);
+s->data_file = NULL;
 }
 g_free(s->unknown_header_fields);
 cleanup_unknown_header_ext(bs);
@@ -2621,6 +2622,7 @@ static void qcow2_close(BlockDriverState *bs)
 
 if (has_data_file(bs)) {
 bdrv_unref_child(bs, s->data_file);
+s->data_file = NULL;
 }
 
 qcow2_refcount_close(bs);
-- 
2.25.1

[PULL 3/6] block: bdrv_set_backing_bs: fix use-after-free

[Max Reitz]

From: Vladimir Sementsov-Ogievskiy 

There is a use-after-free possible: bdrv_unref_child() leaves
bs->backing freed but not NULL. bdrv_attach_child may produce nested
polling loop due to drain, than access of freed pointer is possible.

I've produced the following crash on 30 iotest with modified code. It
does not reproduce on master, but still seems possible:

#0  __strcmp_avx2 () at /lib64/libc.so.6
#1  bdrv_backing_overridden (bs=0x55c9d3cc2060) at block.c:6350
#2  bdrv_refresh_filename (bs=0x55c9d3cc2060) at block.c:6404
#3  bdrv_backing_attach (c=0x55c9d48e5520) at block.c:1063
#4  bdrv_replace_child_noperm
(child=child@entry=0x55c9d48e5520,
new_bs=new_bs@entry=0x55c9d3cc2060) at block.c:2290
#5  bdrv_replace_child
(child=child@entry=0x55c9d48e5520,
new_bs=new_bs@entry=0x55c9d3cc2060) at block.c:2320
#6  bdrv_root_attach_child
(child_bs=child_bs@entry=0x55c9d3cc2060,
child_name=child_name@entry=0x55c9d241d478 "backing",
child_role=child_role@entry=0x55c9d26ecee0 ,
ctx=, perm=, shared_perm=21,
opaque=0x55c9d3c5a3d0, errp=0x7ffd117108e0) at block.c:2424
#7  bdrv_attach_child
(parent_bs=parent_bs@entry=0x55c9d3c5a3d0,
child_bs=child_bs@entry=0x55c9d3cc2060,
child_name=child_name@entry=0x55c9d241d478 "backing",
child_role=child_role@entry=0x55c9d26ecee0 ,
errp=errp@entry=0x7ffd117108e0) at block.c:5876
#8  in bdrv_set_backing_hd
(bs=bs@entry=0x55c9d3c5a3d0,
backing_hd=backing_hd@entry=0x55c9d3cc2060,
errp=errp@entry=0x7ffd117108e0)
at block.c:2576
#9  stream_prepare (job=0x55c9d49d84a0) at block/stream.c:150
#10 job_prepare (job=0x55c9d49d84a0) at job.c:761
#11 job_txn_apply (txn=, fn=) at
job.c:145
#12 job_do_finalize (job=0x55c9d49d84a0) at job.c:778
#13 job_completed_txn_success (job=0x55c9d49d84a0) at job.c:832
#14 job_completed (job=0x55c9d49d84a0) at job.c:845
#15 job_completed (job=0x55c9d49d84a0) at job.c:836
#16 job_exit (opaque=0x55c9d49d84a0) at job.c:864
#17 aio_bh_call (bh=0x55c9d471a160) at util/async.c:117
#18 aio_bh_poll (ctx=ctx@entry=0x55c9d3c46720) at util/async.c:117
#19 aio_poll (ctx=ctx@entry=0x55c9d3c46720,
blocking=blocking@entry=true)
at util/aio-posix.c:728
#20 bdrv_parent_drained_begin_single (poll=true, c=0x55c9d3d558f0)
at block/io.c:121
#21 bdrv_parent_drained_begin_single (c=c@entry=0x55c9d3d558f0,
poll=poll@entry=true)
at block/io.c:114
#22 bdrv_replace_child_noperm
(child=child@entry=0x55c9d3d558f0,
new_bs=new_bs@entry=0x55c9d3d27300) at block.c:2258
#23 bdrv_replace_child
(child=child@entry=0x55c9d3d558f0,
new_bs=new_bs@entry=0x55c9d3d27300) at block.c:2320
#24 bdrv_root_attach_child
(child_bs=child_bs@entry=0x55c9d3d27300,
child_name=child_name@entry=0x55c9d241d478 "backing",
child_role=child_role@entry=0x55c9d26ecee0 ,
ctx=, perm=, shared_perm=21,
opaque=0x55c9d3cc2060, errp=0x7ffd11710c60) at block.c:2424
#25 bdrv_attach_child
(parent_bs=parent_bs@entry=0x55c9d3cc2060,
child_bs=child_bs@entry=0x55c9d3d27300,
child_name=child_name@entry=0x55c9d241d478 "backing",
child_role=child_role@entry=0x55c9d26ecee0 ,
errp=errp@entry=0x7ffd11710c60) at block.c:5876
#26 bdrv_set_backing_hd
(bs=bs@entry=0x55c9d3cc2060,
backing_hd=backing_hd@entry=0x55c9d3d27300,
errp=errp@entry=0x7ffd11710c60)
at block.c:2576
#27 stream_prepare (job=0x55c9d495ead0) at block/stream.c:150
...

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Message-Id: <20200316060631.30052-2-vsement...@virtuozzo.com>
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: John Snow 
Signed-off-by: Max Reitz 
---
 block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 6b984dc883..cccae5add9 100644
--- a/block.c
+++ b/block.c
@@ -2760,10 +2760,10 @@ void bdrv_set_backing_hd(BlockDriverState *bs, 
BlockDriverState *backing_hd,
 
 if (bs->backing) {
 bdrv_unref_child(bs, bs->backing);
+bs->backing = NULL;
 }
 
 if (!backing_hd) {
-bs->backing = NULL;
 goto out;
 }
 
-- 
2.25.1

[PULL 5/6] iotests: Fix cleanup path in some tests

[Max Reitz]

Some iotests leave behind some external data file when run for qcow2
with -o data_file.  Fix that.

Signed-off-by: Max Reitz 
Message-Id: <20200224171631.384314-1-mre...@redhat.com>
Reviewed-by: Eric Blake 
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/085 | 1 +
 tests/qemu-iotests/087 | 6 ++
 tests/qemu-iotests/279 | 2 +-
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
index 46981dbb64..dd3c993a2d 100755
--- a/tests/qemu-iotests/085
+++ b/tests/qemu-iotests/085
@@ -39,6 +39,7 @@ SNAPSHOTS=10
 _cleanup()
 {
 _cleanup_qemu
+_cleanup_test_img
 for i in $(seq 1 ${SNAPSHOTS})
 do
 _rm_test_img "${TEST_DIR}/${i}-${snapshot_virt0}"
diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
index d6c8613419..bdfdad3454 100755
--- a/tests/qemu-iotests/087
+++ b/tests/qemu-iotests/087
@@ -26,6 +26,12 @@ echo "QA output created by $seq"
 
 status=1   # failure is the default!
 
+_cleanup()
+{
+_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
 # get standard environment, filters and checks
 . ./common.rc
 . ./common.filter
diff --git a/tests/qemu-iotests/279 b/tests/qemu-iotests/279
index 30d29b1cb2..75a4747e6b 100755
--- a/tests/qemu-iotests/279
+++ b/tests/qemu-iotests/279
@@ -26,7 +26,7 @@ status=1  # failure is the default!
 _cleanup()
 {
 _cleanup_test_img
-rm -f "$TEST_IMG.mid"
+_rm_test_img "$TEST_IMG.mid"
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
-- 
2.25.1

[PULL 0/6] Block patches for 5.0-rc0

[Max Reitz]

The following changes since commit f1e748d27996e0cd8269db837a32e453dd55930a:

  Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' into 
staging (2020-03-23 20:54:24 +)

are available in the Git repository at:

  https://github.com/XanClic/qemu.git tags/pull-block-2020-03-24

for you to fetch changes up to c264e5d2f9f5d73977eac8e5d084f727b3d07ea9:

  iotests/026: Move v3-exclusive test to new file (2020-03-24 12:05:31 +0100)


Block patches for 5.0-rc0:
- Use-after-free fix
- Fix for a memleak in an error path
- Preventative measures against other potential use-after-frees, and
  against NULL deferences at runtime
- iotest fixes


Eric Blake (1):
  block: Avoid memleak on qcow2 image info failure

Max Reitz (2):
  iotests: Fix cleanup path in some tests
  iotests/026: Move v3-exclusive test to new file

Philippe Mathieu-Daudé (1):
  block: Assert BlockDriver::format_name is not NULL

Vladimir Sementsov-Ogievskiy (2):
  block: bdrv_set_backing_bs: fix use-after-free
  block/qcow2: zero data_file child after free

 block.c|  3 +-
 block/qcow2.c  |  3 +
 tests/qemu-iotests/026 | 31 ---
 tests/qemu-iotests/026.out |  6 --
 tests/qemu-iotests/026.out.nocache |  6 --
 tests/qemu-iotests/085 |  1 +
 tests/qemu-iotests/087 |  6 ++
 tests/qemu-iotests/279 |  2 +-
 tests/qemu-iotests/289 | 89 ++
 tests/qemu-iotests/289.out |  8 +++
 tests/qemu-iotests/group   |  1 +
 11 files changed, 111 insertions(+), 45 deletions(-)
 create mode 100755 tests/qemu-iotests/289
 create mode 100644 tests/qemu-iotests/289.out

-- 
2.25.1

[PULL 6/6] iotests/026: Move v3-exclusive test to new file

[Max Reitz]

data_file does not work with v2, and we probably want 026 to keep
working for v2 images.  Thus, open a new file for v3-exclusive error
path test cases.

Fixes: 81311255f217859413c94f2cd9cebf2684bbda94
   (“iotests/026: Test EIO on allocation in a data-file”)
Signed-off-by: Max Reitz 
Message-Id: <20200311140707.1243218-1-mre...@redhat.com>
Reviewed-by: John Snow 
Tested-by: John Snow 
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/026 | 31 ---
 tests/qemu-iotests/026.out |  6 --
 tests/qemu-iotests/026.out.nocache |  6 --
 tests/qemu-iotests/289 | 89 ++
 tests/qemu-iotests/289.out |  8 +++
 tests/qemu-iotests/group   |  1 +
 6 files changed, 98 insertions(+), 43 deletions(-)
 create mode 100755 tests/qemu-iotests/289
 create mode 100644 tests/qemu-iotests/289.out

diff --git a/tests/qemu-iotests/026 b/tests/qemu-iotests/026
index b05a4692cf..b9713eb591 100755
--- a/tests/qemu-iotests/026
+++ b/tests/qemu-iotests/026
@@ -240,37 +240,6 @@ $QEMU_IO -c "write 0 $CLUSTER_SIZE" "$BLKDBG_TEST_IMG" | 
_filter_qemu_io
 
 _check_test_img
 
-echo
-echo === Avoid freeing external data clusters on failure ===
-echo
-
-# Similar test as the last one, except we test what happens when there
-# is an error when writing to an external data file instead of when
-# writing to a preallocated zero cluster
-_make_test_img -o "data_file=$TEST_IMG.data_file" $CLUSTER_SIZE
-
-# Put blkdebug above the data-file, and a raw node on top of that so
-# that blkdebug will see a write_aio event and emit an error
-$QEMU_IO -c "write 0 $CLUSTER_SIZE" \
-"json:{
- 'driver': 'qcow2',
- 'file': { 'driver': 'file', 'filename': '$TEST_IMG' },
- 'data-file': {
- 'driver': 'raw',
- 'file': {
- 'driver': 'blkdebug',
- 'config': '$TEST_DIR/blkdebug.conf',
- 'image': {
- 'driver': 'file',
- 'filename': '$TEST_IMG.data_file'
- }
- }
- }
- }" \
-| _filter_qemu_io
-
-_check_test_img
-
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/026.out b/tests/qemu-iotests/026.out
index c1b3b58482..83989996ff 100644
--- a/tests/qemu-iotests/026.out
+++ b/tests/qemu-iotests/026.out
@@ -653,10 +653,4 @@ wrote 1024/1024 bytes at offset 0
 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 write failed: Input/output error
 No errors were found on the image.
-
-=== Avoid freeing external data clusters on failure ===
-
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1024 
data_file=TEST_DIR/t.IMGFMT.data_file
-write failed: Input/output error
-No errors were found on the image.
 *** done
diff --git a/tests/qemu-iotests/026.out.nocache 
b/tests/qemu-iotests/026.out.nocache
index 8d5001648a..9359d26d7e 100644
--- a/tests/qemu-iotests/026.out.nocache
+++ b/tests/qemu-iotests/026.out.nocache
@@ -661,10 +661,4 @@ wrote 1024/1024 bytes at offset 0
 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 write failed: Input/output error
 No errors were found on the image.
-
-=== Avoid freeing external data clusters on failure ===
-
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1024 
data_file=TEST_DIR/t.IMGFMT.data_file
-write failed: Input/output error
-No errors were found on the image.
 *** done
diff --git a/tests/qemu-iotests/289 b/tests/qemu-iotests/289
new file mode 100755
index 00..1c11d4030e
--- /dev/null
+++ b/tests/qemu-iotests/289
@@ -0,0 +1,89 @@
+#!/usr/bin/env bash
+#
+# qcow2 v3-exclusive error path testing
+# (026 tests paths common to v2 and v3)
+#
+# Copyright (C) 2020 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 .
+#
+
+seq=$(basename $0)
+echo "QA output created by $seq"
+
+status=1   # failure is the default!
+
+_cleanup()
+{
+_cleanup_test_img
+rm "$TEST_DIR/blkdebug.conf"
+rm -f "$TEST_IMG.data_file"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.pattern
+
+_supported_fmt qcow2
+_supported_proto file
+# This is a v3-exclusive test;
+# As for data_file, error paths often very much depend on whether
+# there is an external data file or not; so we create one exactly when
+# we want to test it
+_unsupported_imgopts 'compat=

Re: Potential Null dereference

[Vladimir Sementsov-Ogievskiy]

24.03.2020 14:59, Vladimir Sementsov-Ogievskiy wrote:

24.03.2020 12:50, Kevin Wolf wrote:

Am 24.03.2020 um 08:14 hat Philippe Mathieu-Daudé geschrieben:

On 3/24/20 4:05 AM, Mansour Ahmadi wrote:

Hi,

Nullness of  needs to be checked here:
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/commit.c#L221

pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),...


Do you have a reproducer? It's not obvious to me how bs->backing could
be NULL here.



While it is done at 2 other locations:
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/backup-top.c#L113
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/mirror.c#L1477


Commit 18775ff3269 made the change for mirror, however its commit
message is terse and doesn't say anything about the scenario where it
would happen. We also didn't add a test case for it. I would have
expected that failure to add the backing file would immediately error
out and not try to refresh the filename first.

backup-top.c has the check from the beginning. I assume it just copied
it from mirror.

Vladimir, do you remember the details?



No :(
But I believe that "Backing may be zero after failed bdrv_attach_child in 
bdrv_set_backing_hd, which leads to SIGSEGV." means real problem, probably 
reproduced on some experiment branch, not on master.

Let's try most simple check:

apply the following:

=
diff --git a/include/block/block.h b/include/block/block.h
index e569a4d747..dc20b55075 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -298,6 +298,8 @@ BdrvChild *bdrv_open_child(const char *filename,
     const BdrvChildRole *child_role,
     bool allow_none, Error **errp);
  BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp);
+void bdrv_set_backing_hd_ex(BlockDriverState *bs, BlockDriverState *backing_hd,
+    bool bdrv_attach_child_fail, Error **errp);
  void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
   Error **errp);
  int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
diff --git a/block.c b/block.c
index a2542c977b..21b6124d73 100644
--- a/block.c
+++ b/block.c
@@ -2743,8 +2743,8 @@ static bool bdrv_inherits_from_recursive(BlockDriverState 
*child,
   * Sets the backing file link of a BDS. A new reference is created; callers
   * which don't need their own reference any more must call bdrv_unref().
   */
-void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
- Error **errp)
+void bdrv_set_backing_hd_ex(BlockDriverState *bs, BlockDriverState *backing_hd,
+    bool bdrv_attach_child_fail, Error **errp)
  {
  bool update_inherits_from = bdrv_chain_contains(bs, backing_hd) &&
  bdrv_inherits_from_recursive(backing_hd, bs);
@@ -2766,8 +2766,14 @@ void bdrv_set_backing_hd(BlockDriverState *bs, 
BlockDriverState *backing_hd,
  goto out;
  }

-    bs->backing = bdrv_attach_child(bs, backing_hd, "backing", &child_backing,
-    errp);
+    if (bdrv_attach_child_fail) {
+    bs->backing = NULL;
+    error_setg(errp, "Unpredicted failure :)");
+    } else {
+    bs->backing = bdrv_attach_child(bs, backing_hd, "backing", 
&child_backing,
+    errp);
+    }
+
  /* If backing_hd was already part of bs's backing chain, and
   * inherits_from pointed recursively to bs then let's update it to
   * point directly to bs (else it will become NULL). */
@@ -2779,6 +2785,12 @@ out:
  bdrv_refresh_limits(bs, NULL);
  }

+void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
+ Error **errp)
+{
+    bdrv_set_backing_hd_ex(bs, backing_hd, false, errp);
+}
  /*
   * Opens the backing file for a BlockDriverState if not yet open
   *
diff --git a/block/mirror.c b/block/mirror.c
index 447051dbc6..907bb2b718 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -675,7 +675,7 @@ static int mirror_exit_common(Job *job)
  if (!abort && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
  BlockDriverState *backing = s->is_none_mode ? src : s->base;
  if (backing_bs(target_bs) != backing) {
-    bdrv_set_backing_hd(target_bs, backing, &local_err);
+    bdrv_set_backing_hd_ex(target_bs, backing, true, &local_err);
  if (local_err) {
  error_report_err(local_err);
  ret = -EPERM;
@@ -1477,6 +1477,7 @@ static void 
bdrv_mirror_top_refresh_filename(BlockDriverState *bs)
  if (bs->backing == NULL) {
  /* we can be here after failed bdrv_attach_child in
   * bdrv_set_backing_hd */
+    abort();
  return;
  }
  pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),


=

Re: [PATCH v2] qcow2: Forbid discard in qcow2 v2 images with backing files

[Max Reitz]

On 24.03.20 13:16, Alberto Garcia wrote:
> A discard request deallocates the selected clusters so they read back
> as zeroes. This is done by clearing the cluster offset field and
> setting QCOW_OFLAG_ZERO in the L2 entry.
> 
> This flag is however only supported when qcow_version >= 3. In older
> images the cluster is simply deallocated, exposing any possible stale
> data from the backing file.
> 
> Since discard is an advisory operation it's safer to simply forbid it
> in this scenario.
> 
> Note that we are adding this check to qcow2_co_pdiscard() and not to
> qcow2_cluster_discard() or discard_in_l2_slice() because the last
> two are also used by qcow2_snapshot_create() to discard the clusters
> used by the VM state. In this case there's no risk of exposing stale
> data to the guest and we really want that the clusters are always
> discarded.
> 
> Signed-off-by: Alberto Garcia 
> ---
> v2:
> 
> - Don't create the image with compat=0.10 in iotest 060 [Max]
> - Use $TEST_IMG.base for the backing image name in iotest 289 [Max]
> - Add list of unsupported options to iotest 289 [Max]
> 
>  block/qcow2.c  |  6 +++
>  tests/qemu-iotests/060 | 10 ++---
>  tests/qemu-iotests/060.out |  2 -
>  tests/qemu-iotests/289 | 91 ++
>  tests/qemu-iotests/289.out | 52 ++
>  tests/qemu-iotests/group   |  1 +
>  6 files changed, 154 insertions(+), 8 deletions(-)
>  create mode 100755 tests/qemu-iotests/289
>  create mode 100644 tests/qemu-iotests/289.out

Reviewed-by: Max Reitz 

(Maybe someone else wants to give a comment still, though)



signature.asc
Description: OpenPGP digital signature

Re: Potential Null dereference

[Kevin Wolf]

Am 24.03.2020 um 13:37 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 24.03.2020 14:59, Vladimir Sementsov-Ogievskiy wrote:
> > Aha, new crashes! Let's look at them.
> > 
> > 41 and 155 failed with crash, 141 without but I see "+{"error": {"class": 
> > "GenericError", "desc": "Block device drv0 is in use"}}" in its output.
> > 
> > Hmm, but all crashes are because of assert(QTAILQ_EMPTY(&all_bdrv_states)); 
> > in bdrv_close_all..
> > 
> > So, we have a problem, but another one..
> 
> Investigate it a bit more.
> 
> Accordingly to comment, bdrv_attach_child unrefs child bs even on failure, so 
> let's do
> 
> --- a/block.c
> +++ b/block.c
> @@ -2768,6 +2768,7 @@ void bdrv_set_backing_hd_ex(BlockDriverState *bs, 
> BlockDriverState *backing_hd,
> 
>  if (bdrv_attach_child_fail) {
>  bs->backing = NULL;
> +bdrv_unref(backing_hd);
>  error_setg(errp, "Unpredicted failure :)");
>  } else {
>  bs->backing = bdrv_attach_child(bs, backing_hd, "backing", 
> &child_backing,
> 
> 
> - then, all three tests fails, but without crashes. OK.
> 
> The only interesting thing is that, it seems that bdrv_attach_child doesn't 
> unref child on all failure paths.
> 
> It calls bdrv_root_attach_child..
> 
> which doesn't unref child on the path
> 
> if (bdrv_get_aio_context(child_bs) != ctx) {
>   ...
>if (ret < 0) {
>  error_propagate(errp, local_err);
>  g_free(child);
>  bdrv_abort_perm_update(child_bs);
>  return NULL;
>}
> }
> 
> or am I wrong?

I think you're right, we need a bdrv_unref() there. The function comment
makes it clear that bdrv_root_attach_child() is supposed to consume the
reference both in success and error cases.

Kevin

Re: [PATCH v8 01/11] iotests: do a light delinting

[Max Reitz]

On 17.03.20 01:40, John Snow wrote:
> This doesn't fix everything in here, but it does help clean up the
> pylint report considerably.
> 
> This should be 100% style changes only; the intent is to make pylint
> more useful by working on establishing a baseline for iotests that we
> can gate against in the future.
> 
> Signed-off-by: John Snow 
> Reviewed-by: Philippe Mathieu-Daudé 
> ---
>  tests/qemu-iotests/iotests.py | 83 ++-
>  1 file changed, 43 insertions(+), 40 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v8 03/11] iotests: ignore import warnings from pylint

[Max Reitz]

On 17.03.20 01:40, John Snow wrote:
> The right way to solve this is to come up with a virtual environment
> infrastructure that sets all the paths correctly, and/or to create
> installable python modules that can be imported normally.
> 
> That's hard, so just silence this error for now.
> 
> Signed-off-by: John Snow 
> ---
>  tests/qemu-iotests/iotests.py | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

[PATCH] nvme: Print 'cqid' for nvme_del_cq

[Minwoo Im]

The given argument for this trace should be cqid, not sqid.

Signed-off-by: Minwoo Im 
---
 hw/block/trace-events | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/trace-events b/hw/block/trace-events
index f78939fa9da1..bf6d11b58b85 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -37,7 +37,7 @@ nvme_rw(const char *verb, uint32_t blk_count, uint64_t 
byte_count, uint64_t lba)
 nvme_create_sq(uint64_t addr, uint16_t sqid, uint16_t cqid, uint16_t qsize, 
uint16_t qflags) "create submission queue, addr=0x%"PRIx64", sqid=%"PRIu16", 
cqid=%"PRIu16", qsize=%"PRIu16", qflags=%"PRIu16""
 nvme_create_cq(uint64_t addr, uint16_t cqid, uint16_t vector, uint16_t size, 
uint16_t qflags, int ien) "create completion queue, addr=0x%"PRIx64", 
cqid=%"PRIu16", vector=%"PRIu16", qsize=%"PRIu16", qflags=%"PRIu16", ien=%d"
 nvme_del_sq(uint16_t qid) "deleting submission queue sqid=%"PRIu16""
-nvme_del_cq(uint16_t cqid) "deleted completion queue, sqid=%"PRIu16""
+nvme_del_cq(uint16_t cqid) "deleted completion queue, cqid=%"PRIu16""
 nvme_identify_ctrl(void) "identify controller"
 nvme_identify_ns(uint16_t ns) "identify namespace, nsid=%"PRIu16""
 nvme_identify_nslist(uint16_t ns) "identify namespace list, nsid=%"PRIu16""
-- 
2.17.1

Re: [PATCH] block: make BlockConf.*_size properties 32-bit

[Eric Blake]

On 2/13/20 7:55 AM, Roman Kagan wrote:

On Thu, Feb 13, 2020 at 06:47:10AM -0600, Eric Blake wrote:

On 2/13/20 2:01 AM, Roman Kagan wrote:

On Wed, Feb 12, 2020 at 03:44:19PM -0600, Eric Blake wrote:

On 2/11/20 5:54 AM, Roman Kagan wrote:

Devices (virtio-blk, scsi, etc.) and the block layer are happy to use
32-bit for logical_block_size, physical_block_size, and min_io_size.
However, the properties in BlockConf are defined as uint16_t limiting
the values to 32768.

This appears unnecessary tight, and we've seen bigger block sizes handy
at times.


What larger sizes?  I could see 64k or maybe even 1M block sizes,...


We played exactly with these two :)



Make them 32 bit instead and lift the limitation.



@@ -716,30 +716,32 @@ const PropertyInfo qdev_prop_pci_devfn = {
/* --- blocksize --- */
+#define MIN_BLOCK_SIZE 512
+#define MAX_BLOCK_SIZE 2147483648


...but 2G block sizes are going to have tremendous performance problems.

I'm not necessarily opposed to the widening to a 32-bit type, but think you
need more justification or a smaller number for the max block size,


I thought any smaller value would just be arbitrary and hard to reason
about, so I went ahead with the max value that fit in the type and could
be made visibile to the guest.


You've got bigger problems than what is visible to the guest. block/qcow2.c
operates on a cluster at a time; if you are stating that it now requires
reading multiple clusters to operate on one, qcow2 will have to do lots of
wasteful read-modify-write cycles.


I'm failing to see how this is supposed to happen.  The guest will issue
requests bigger than the cluster size; why would it cause RMW?

Big logical_block_size would cause RMW in the guest if it wants to
perform smaller writes, but that's up to the user to take this tradeoff,
isn't it?


You really need a strong reason to
support a maximum larger than 2M other than just "so the guest can
experiment with it".


Do I get you right that your suggestion is to cap the block size
property at 2MB?


Yes, for now, I think 2M is a better maximum than 2G or 4G unless you 
have benchmark data to prove that a larger maximum does not cause problems.


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

Re: [PATCH v8 04/11] iotests: replace mutable list default args

[Max Reitz]

On 17.03.20 01:40, John Snow wrote:
> It's bad hygiene: if we modify this list, it will be modified across all
> invocations.
> 
> (Remaining bad usages are fixed in a subsequent patch which changes the
> function signature anyway.)
> 
> Signed-off-by: John Snow 
> Reviewed-by: Philippe Mathieu-Daudé 
> ---
>  tests/qemu-iotests/iotests.py | 24 
>  1 file changed, 12 insertions(+), 12 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH 2/3] iotests: Add poke_file_[bl]e functions

[Eric Blake]

On 3/10/20 12:22 PM, Max Reitz wrote:

   +# poke_file_le 'test.img' 512 2 65534
+poke_file_le()
+{


I like the interface.  However, the implementation is a bit bloated (but
then again, that's why you cc'd me for review ;)


+    local img=$1 ofs=$2 len=$3 val=$4 str=''


Noticing that this is not in yet, I have one more suggestion:

The initial doc comment is not helpful without reading the rest of the 
function: Is 512 the offset or the value being written?  Better might be:


# poke_file_le test.img $offset $width $value



+
+# poke_file_be 'test.img' 512 2 65279
+poke_file_be()


and here, too.

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

Re: [PATCH] qcow2: Forbid discard in qcow2 v2 images with backing files

[Eric Blake]

On 3/23/20 2:44 PM, Alberto Garcia wrote:

A discard request deallocates the selected clusters so they read back
as zeroes. This is done by clearing the cluster offset field and
setting QCOW_OFLAG_ZERO in the L2 entry.

This flag is however only supported when qcow_version >= 3. In older
images the cluster is simply deallocated, exposing any possible stale
data from the backing file.

Since discard is an advisory operation it's safer to simply forbid it
in this scenario.

Note that we are adding this check to qcow2_co_pdiscard() and not to
qcow2_cluster_discard() or discard_in_l2_slice() because the last
two are also used by qcow2_snapshot_create() to discard the clusters
used by the VM state. In this case there's no risk of exposing stale
data to the guest and we really want that the clusters are always
discarded.

Signed-off-by: Alberto Garcia 
---
  block/qcow2.c  |  6 +++
  tests/qemu-iotests/060 |  5 ++-
  tests/qemu-iotests/060.out |  2 -
  tests/qemu-iotests/289 | 90 ++
  tests/qemu-iotests/289.out | 52 ++
  tests/qemu-iotests/group   |  1 +
  6 files changed, 152 insertions(+), 4 deletions(-)
  create mode 100755 tests/qemu-iotests/289
  create mode 100644 tests/qemu-iotests/289.out


The actual fix is much smaller than the iotest fallout ;)


+++ b/tests/qemu-iotests/060
@@ -167,9 +167,10 @@ _make_test_img -o 'compat=0.10' -b "$BACKING_IMG" 1G
  # Write two clusters, the second one enforces creation of an L2 table after
  # the first data cluster.
  $QEMU_IO -c 'write 0k 64k' -c 'write 512M 64k' "$TEST_IMG" | _filter_qemu_io
-# Discard the first cluster. This cluster will soon enough be reallocated and
+# Free the first cluster. This cluster will soon enough be reallocated and
  # used for COW.
-$QEMU_IO -c 'discard 0k 64k' "$TEST_IMG" | _filter_qemu_io
+poke_file "$TEST_IMG" '262144' "\x00\x00\x00\x00\x00\x00\x00\x00" # 0x4 - 
L2 entry
+poke_file "$TEST_IMG" '131082' "\x00\x00" # 0x2000a - Refcount entry


Instead of writing '262144' ... # 0x4, you could write $((0x4)) 
in-place.  Similarly for 131082 vs. 0x2000a.


Also, Max has pending patches for adding poke_file_be; if those land 
first, this becomes simpler as:


poke_file_be "$TEST_IMG" $((0x4)) 8 0 # L2 entry
poke_file_be "$TEST_IMG" $((0x2000a)) 2 0 # Refcount entry


+++ b/tests/qemu-iotests/289
@@ -0,0 +1,90 @@
+#!/usr/bin/env bash
+#
+# Test how 'qemu-io -c discard' behaves on v2 and v3 qcow2 images
At any rate, the new test looks reasonable to me. I see you have other 
review comments for improving it, with thos in, you can add


Reviewed-by: Eric Blake 

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

Re: [PATCH] nvme: Print 'cqid' for nvme_del_cq

[Philippe Mathieu-Daudé]

On 3/24/20 3:06 PM, Minwoo Im wrote:

The given argument for this trace should be cqid, not sqid.

Signed-off-by: Minwoo Im 
---
  hw/block/trace-events | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/trace-events b/hw/block/trace-events
index f78939fa9da1..bf6d11b58b85 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -37,7 +37,7 @@ nvme_rw(const char *verb, uint32_t blk_count, uint64_t 
byte_count, uint64_t lba)
  nvme_create_sq(uint64_t addr, uint16_t sqid, uint16_t cqid, uint16_t qsize, uint16_t qflags) "create submission queue, 
addr=0x%"PRIx64", sqid=%"PRIu16", cqid=%"PRIu16", qsize=%"PRIu16", qflags=%"PRIu16""
  nvme_create_cq(uint64_t addr, uint16_t cqid, uint16_t vector, uint16_t size, uint16_t qflags, int ien) "create completion queue, 
addr=0x%"PRIx64", cqid=%"PRIu16", vector=%"PRIu16", qsize=%"PRIu16", qflags=%"PRIu16", 
ien=%d"
  nvme_del_sq(uint16_t qid) "deleting submission queue sqid=%"PRIu16""
-nvme_del_cq(uint16_t cqid) "deleted completion queue, sqid=%"PRIu16""
+nvme_del_cq(uint16_t cqid) "deleted completion queue, cqid=%"PRIu16""
  nvme_identify_ctrl(void) "identify controller"
  nvme_identify_ns(uint16_t ns) "identify namespace, nsid=%"PRIu16""
  nvme_identify_nslist(uint16_t ns) "identify namespace list, nsid=%"PRIu16""



Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v8 05/11] iotests: add pylintrc file

[Max Reitz]

On 17.03.20 01:40, John Snow wrote:
> This allows others to get repeatable results with pylint. If you run
> `pylint iotests.py`, you should see a 100% pass.
> 
> Signed-off-by: John Snow 
> ---
>  tests/qemu-iotests/pylintrc | 22 ++
>  1 file changed, 22 insertions(+)
>  create mode 100644 tests/qemu-iotests/pylintrc

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PULL 0/6] Block patches for 5.0-rc0

[Peter Maydell]

On Tue, 24 Mar 2020 at 12:21, Max Reitz  wrote:
>
> The following changes since commit f1e748d27996e0cd8269db837a32e453dd55930a:
>
>   Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' 
> into staging (2020-03-23 20:54:24 +)
>
> are available in the Git repository at:
>
>   https://github.com/XanClic/qemu.git tags/pull-block-2020-03-24
>
> for you to fetch changes up to c264e5d2f9f5d73977eac8e5d084f727b3d07ea9:
>
>   iotests/026: Move v3-exclusive test to new file (2020-03-24 12:05:31 +0100)
>
> 
> Block patches for 5.0-rc0:
> - Use-after-free fix
> - Fix for a memleak in an error path
> - Preventative measures against other potential use-after-frees, and
>   against NULL deferences at runtime
> - iotest fixes

Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/5.0
for any user-visible changes.

-- PMM

Re: [PATCH v8 06/11] iotests: drop Python 3.4 compatibility code

[Max Reitz]

On 17.03.20 01:41, John Snow wrote:
> We no longer need to accommodate 3.4, drop this code.

Pre-3.4, actually.

> (Also, the line is over 79 characters, so drop it.)
> 
> Touch up the docstring a little bit while we're here.
> 
> Signed-off-by: John Snow 
> ---
>  tests/qemu-iotests/iotests.py | 14 +++---
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 7cd74e7cb1..3d90fb157d 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -22,6 +22,7 @@
>  import unittest
>  import sys
>  import struct
> +from typing import Optional
>  import json
>  import signal
>  import logging
> @@ -350,18 +351,17 @@ def _filter(_key, value):
>  return value
>  return filter_qmp(qmsg, _filter)
>  
> -def log(msg, filters=(), indent=None):
> -'''Logs either a string message or a JSON serializable message (like 
> QMP).
> -If indent is provided, JSON serializable messages are pretty-printed.'''
> +def log(msg, filters=(), indent: Optional[int] = None) -> None:
> +"""
> +Logs either a string message or a JSON serializable message (like QMP).
> +If indent is provided, JSON serializable messages are pretty-printed.
> +"""

I feel like I should complain about this unrelated (I think?) change,
but I won’t.

Reviewed-by: Max Reitz 

>  for flt in filters:
>  msg = flt(msg)
>  if isinstance(msg, (dict, list)):
> -# Python < 3.4 needs to know not to add whitespace when 
> pretty-printing:
> -separators = (', ', ': ') if indent is None else (',', ': ')
>  # Don't sort if it's already sorted
>  do_sort = not isinstance(msg, OrderedDict)
> -print(json.dumps(msg, sort_keys=do_sort,
> - indent=indent, separators=separators))
> +print(json.dumps(msg, sort_keys=do_sort, indent=indent))
>  else:
>  print(msg)
>  
> 




signature.asc
Description: OpenPGP digital signature

Re: [PATCH v8 07/11] iotests: limit line length to 79 chars

[Max Reitz]

On 17.03.20 01:41, John Snow wrote:
> 79 is the PEP8 recommendation. This recommendation works well for
> reading patch diffs in TUI email clients.

Also for my very GUI-y diff program (kompare).

> Signed-off-by: John Snow 
> ---
>  tests/qemu-iotests/iotests.py | 64 +++
>  tests/qemu-iotests/pylintrc   |  6 +++-
>  2 files changed, 47 insertions(+), 23 deletions(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 3d90fb157d..75fd697d77 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py

[...]

> @@ -529,11 +539,13 @@ def pause_drive(self, drive, event=None):
>  self.pause_drive(drive, "write_aio")
>  return
>  self.qmp('human-monitor-command',
> - command_line='qemu-io %s "break %s bp_%s"' % (drive, event, 
> drive))
> + command_line='qemu-io %s "break %s bp_%s"'
> + % (drive, event, drive))

Can we put this value in a variable instead?  I don’t like the %
aligning with the parameter name instead of the string value.  (I also
don’t like how there are no spaces around the assignment =, but around
the %, even though the % binds more strongly.)

>  
>  def resume_drive(self, drive):
>  self.qmp('human-monitor-command',
> - command_line='qemu-io %s "remove_break bp_%s"' % (drive, 
> drive))
> + command_line='qemu-io %s "remove_break bp_%s"'
> + % (drive, drive))
>  
>  def hmp_qemu_io(self, drive, cmd):
>  '''Write to a given drive using an HMP command'''
> @@ -793,16 +805,18 @@ def dictpath(self, d, path):
>  idx = int(idx)
>  
>  if not isinstance(d, dict) or component not in d:
> -self.fail('failed path traversal for "%s" in "%s"' % (path, 
> str(d)))
> +self.fail(f'failed path traversal for "{path}" in "{d}"')

Do we require 3.6 so that f-strings are guaranteed to work?  (I thought
we didn’t.  I’d be happy to use them.)

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v8 07/11] iotests: limit line length to 79 chars

[Max Reitz]

On 24.03.20 16:02, Max Reitz wrote:
> On 17.03.20 01:41, John Snow wrote:
>> 79 is the PEP8 recommendation. This recommendation works well for
>> reading patch diffs in TUI email clients.
> 
> Also for my very GUI-y diff program (kompare).
> 
>> Signed-off-by: John Snow 
>> ---
>>  tests/qemu-iotests/iotests.py | 64 +++
>>  tests/qemu-iotests/pylintrc   |  6 +++-
>>  2 files changed, 47 insertions(+), 23 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 3d90fb157d..75fd697d77 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
> 
> [...]
> 
>> @@ -529,11 +539,13 @@ def pause_drive(self, drive, event=None):
>>  self.pause_drive(drive, "write_aio")
>>  return
>>  self.qmp('human-monitor-command',
>> - command_line='qemu-io %s "break %s bp_%s"' % (drive, 
>> event, drive))
>> + command_line='qemu-io %s "break %s bp_%s"'
>> + % (drive, event, drive))
> 
> Can we put this value in a variable instead?  I don’t like the %
> aligning with the parameter name instead of the string value.  (I also
> don’t like how there are no spaces around the assignment =, but around
> the %, even though the % binds more strongly.)
> 
>>  
>>  def resume_drive(self, drive):
>>  self.qmp('human-monitor-command',
>> - command_line='qemu-io %s "remove_break bp_%s"' % (drive, 
>> drive))
>> + command_line='qemu-io %s "remove_break bp_%s"'
>> + % (drive, drive))
>>  
>>  def hmp_qemu_io(self, drive, cmd):
>>  '''Write to a given drive using an HMP command'''
>> @@ -793,16 +805,18 @@ def dictpath(self, d, path):
>>  idx = int(idx)
>>  
>>  if not isinstance(d, dict) or component not in d:
>> -self.fail('failed path traversal for "%s" in "%s"' % (path, 
>> str(d)))
>> +self.fail(f'failed path traversal for "{path}" in "{d}"')
> 
> Do we require 3.6 so that f-strings are guaranteed to work?  (I thought
> we didn’t.  I’d be happy to use them.)

Oh.  Of course we do.  It says so in this file that this whole series is
about.

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] qcow2: Forbid discard in qcow2 v2 images with backing files

[Alberto Garcia]

On Tue 24 Mar 2020 03:46:07 PM CET, Eric Blake wrote:
>> -$QEMU_IO -c 'discard 0k 64k' "$TEST_IMG" | _filter_qemu_io
>> +poke_file "$TEST_IMG" '262144' "\x00\x00\x00\x00\x00\x00\x00\x00" # 0x4 
>> - L2 entry
>> +poke_file "$TEST_IMG" '131082' "\x00\x00" # 0x2000a - Refcount entry
>
> Instead of writing '262144' ... # 0x4, you could write
> $((0x4)) in-place.  Similarly for 131082 vs. 0x2000a.

The exiting poke_file line in that test was using base 10 so I decided
to use it too for consistency.

I actually realized that $rb_offset and $l2_offset are defined, so I
could use those too.

> Also, Max has pending patches for adding poke_file_be; if those land
> first, this becomes simpler as:
>
> poke_file_be "$TEST_IMG" $((0x4)) 8 0 # L2 entry
> poke_file_be "$TEST_IMG" $((0x2000a)) 2 0 # Refcount entry

I'm fine if those lines are changed when the patch is committed.

Berto

[PATCH 2/6] block/mirror: fix use after free of local_err

[Vladimir Sementsov-Ogievskiy]

local_err is used again in mirror_exit_common() after
bdrv_set_backing_hd(), so we must zero it. Otherwise try to set
non-NULL local_err will crash.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/mirror.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/mirror.c b/block/mirror.c
index 447051dbc6..6203e5946e 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -678,6 +678,7 @@ static int mirror_exit_common(Job *job)
 bdrv_set_backing_hd(target_bs, backing, &local_err);
 if (local_err) {
 error_report_err(local_err);
+local_err = NULL;
 ret = -EPERM;
 }
 }
-- 
2.21.0

[PATCH 1/6] scripts/coccinelle: add error-use-after-free.cocci

[Vladimir Sementsov-Ogievskiy]

Add script to find and fix trivial use-after-free of Error objects.
How to use:
spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
 --macro-file scripts/cocci-macro-file.h --in-place \
 --no-show-diff ( FILES... | --use-gitgrep . )

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 scripts/coccinelle/error-use-after-free.cocci | 52 +++
 MAINTAINERS   |  1 +
 2 files changed, 53 insertions(+)
 create mode 100644 scripts/coccinelle/error-use-after-free.cocci

diff --git a/scripts/coccinelle/error-use-after-free.cocci 
b/scripts/coccinelle/error-use-after-free.cocci
new file mode 100644
index 00..7cfa42355b
--- /dev/null
+++ b/scripts/coccinelle/error-use-after-free.cocci
@@ -0,0 +1,52 @@
+// Find and fix trivial use-after-free of Error objects
+//
+// Copyright (c) 2020 Virtuozzo International GmbH.
+//
+// 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
+// .
+//
+// How to use:
+// spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
+//  --macro-file scripts/cocci-macro-file.h --in-place \
+//  --no-show-diff ( FILES... | --use-gitgrep . )
+
+@ exists@
+identifier fn, fn2;
+expression err;
+@@
+
+ fn(...)
+ {
+ <...
+(
+ error_free(err);
++err = NULL;
+|
+ error_report_err(err);
++err = NULL;
+|
+ error_reportf_err(err, ...);
++err = NULL;
+|
+ warn_report_err(err);
++err = NULL;
+|
+ warn_reportf_err(err, ...);
++err = NULL;
+)
+ ... when != err = NULL
+ when != exit(...)
+ fn2(..., err, ...)
+ ...>
+ }
diff --git a/MAINTAINERS b/MAINTAINERS
index b5c86ec494..ba97cc43fc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2037,6 +2037,7 @@ F: include/qemu/error-report.h
 F: qapi/error.json
 F: util/error.c
 F: util/qemu-error.c
+F: scripts/coccinelle/*err*.cocci
 
 GDB stub
 M: Alex Bennée 
-- 
2.21.0

[PATCH 3/6] dump/win_dump: fix use after free of err

[Vladimir Sementsov-Ogievskiy]

It's possible that we'll try to set err twice (or more). It's bad, it
will crash.

Instead, use warn_report().

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 dump/win_dump.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/dump/win_dump.c b/dump/win_dump.c
index eda2a48974..652c7bad99 100644
--- a/dump/win_dump.c
+++ b/dump/win_dump.c
@@ -304,13 +304,11 @@ static void restore_context(WinDumpHeader64 *h,
 struct saved_context *saved_ctx)
 {
 int i;
-Error *err = NULL;
 
 for (i = 0; i < h->NumberProcessors; i++) {
 if (cpu_memory_rw_debug(first_cpu, saved_ctx[i].addr,
 (uint8_t *)&saved_ctx[i].ctx, sizeof(WinContext), 1)) {
-error_setg(&err, "win-dump: failed to restore CPU #%d context", i);
-warn_report_err(err);
+warn_report("win-dump: failed to restore CPU #%d context", i);
 }
 }
 }
-- 
2.21.0

[PATCH for-5.0 0/6] Several error use-after-free

[Vladimir Sementsov-Ogievskiy]

Hi all!

I accidentally found use-after-free of local_err in mirror, and decided
to search for similar cases with help of small coccinelle script
(patch 01). Happily, there no many cases.

Better to fix zero Error* pointer after each freeing everywhere, but
this is too much for 5.0 and most of these cases will be covered by
error-auto-propagation series.

Note also, that there are still a lot of use-after-free cases possible
when error is not local variable but field of some structure, shared by
several functions.

Vladimir Sementsov-Ogievskiy (6):
  scripts/coccinelle: add error-use-after-free.cocci
  block/mirror: fix use after free of local_err
  dump/win_dump: fix use after free of err
  migration/colo: fix use after free of local_err
  migration/ram: fix use after free of local_err
  qga/commands-posix: fix use after free of local_err

 scripts/coccinelle/error-use-after-free.cocci | 52 +++
 block/mirror.c|  1 +
 dump/win_dump.c   |  4 +-
 migration/colo.c  |  1 +
 migration/ram.c   |  1 +
 qga/commands-posix.c  |  3 ++
 MAINTAINERS   |  1 +
 7 files changed, 60 insertions(+), 3 deletions(-)
 create mode 100644 scripts/coccinelle/error-use-after-free.cocci

-- 
2.21.0

[PATCH 4/6] migration/colo: fix use after free of local_err

[Vladimir Sementsov-Ogievskiy]

local_err is used again in secondary_vm_do_failover() after
replication_stop_all(), so we must zero it. Otherwise try to set
non-NULL local_err will crash.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 migration/colo.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/migration/colo.c b/migration/colo.c
index 44942c4e23..a54ac84f41 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -93,6 +93,7 @@ static void secondary_vm_do_failover(void)
 replication_stop_all(true, &local_err);
 if (local_err) {
 error_report_err(local_err);
+local_err = NULL;
 }
 
 /* Notify all filters of all NIC to do checkpoint */
-- 
2.21.0

[PATCH 5/6] migration/ram: fix use after free of local_err

[Vladimir Sementsov-Ogievskiy]

local_err is used again in migration_bitmap_sync_precopy() after
precopy_notify(), so we must zero it. Otherwise try to set
non-NULL local_err will crash.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 migration/ram.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/migration/ram.c b/migration/ram.c
index c12cfdbe26..04f13feb2e 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -980,6 +980,7 @@ static void migration_bitmap_sync_precopy(RAMState *rs)
  */
 if (precopy_notify(PRECOPY_NOTIFY_BEFORE_BITMAP_SYNC, &local_err)) {
 error_report_err(local_err);
+local_err = NULL;
 }
 
 migration_bitmap_sync(rs);
-- 
2.21.0

[PATCH 6/6] qga/commands-posix: fix use after free of local_err

[Vladimir Sementsov-Ogievskiy]

local_err is used several times in guest_suspend(). Setting non-NULL
local_err will crash, so let's zero it after freeing. Also fix possible
leak of local_err in final if().

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 qga/commands-posix.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 93474ff770..cc69b82704 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1773,6 +1773,7 @@ static void guest_suspend(SuspendMode mode, Error **errp)
 }
 
 error_free(local_err);
+local_err = NULL;
 
 if (pmutils_supports_mode(mode, &local_err)) {
 mode_supported = true;
@@ -1784,6 +1785,7 @@ static void guest_suspend(SuspendMode mode, Error **errp)
 }
 
 error_free(local_err);
+local_err = NULL;
 
 if (linux_sys_state_supports_mode(mode, &local_err)) {
 mode_supported = true;
@@ -1791,6 +1793,7 @@ static void guest_suspend(SuspendMode mode, Error **errp)
 }
 
 if (!mode_supported) {
+error_free(local_err);
 error_setg(errp,
"the requested suspend mode is not supported by the guest");
 } else {
-- 
2.21.0

Re: [PATCH for-5.0 0/6] Several error use-after-free

[Richard Henderson]

On 3/24/20 8:36 AM, Vladimir Sementsov-Ogievskiy wrote:
> Vladimir Sementsov-Ogievskiy (6):
>   scripts/coccinelle: add error-use-after-free.cocci
>   block/mirror: fix use after free of local_err
>   dump/win_dump: fix use after free of err
>   migration/colo: fix use after free of local_err
>   migration/ram: fix use after free of local_err
>   qga/commands-posix: fix use after free of local_err

Whole series:
Reviewed-by: Richard Henderson 


r~

Re: [PATCH 2/6] block/mirror: fix use after free of local_err

[Eric Blake]

On 3/24/20 10:36 AM, Vladimir Sementsov-Ogievskiy wrote:

local_err is used again in mirror_exit_common() after
bdrv_set_backing_hd(), so we must zero it. Otherwise try to set
non-NULL local_err will crash.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/mirror.c | 1 +
  1 file changed, 1 insertion(+)


Reviewed-by: Eric Blake 



diff --git a/block/mirror.c b/block/mirror.c
index 447051dbc6..6203e5946e 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -678,6 +678,7 @@ static int mirror_exit_common(Job *job)
  bdrv_set_backing_hd(target_bs, backing, &local_err);
  if (local_err) {
  error_report_err(local_err);
+local_err = NULL;
  ret = -EPERM;
  }
  }



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

[PATCH for 5.0] block: fix bdrv_root_attach_child forget to unref child_bs

[Vladimir Sementsov-Ogievskiy]

bdrv_root_attach_child promises to drop child_bs reference on failure.
It does it on first handled failure path, but not on the second. Fix
that.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block.c b/block.c
index a2542c977b..6713db773d 100644
--- a/block.c
+++ b/block.c
@@ -2612,6 +2612,7 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState 
*child_bs,
 error_propagate(errp, local_err);
 g_free(child);
 bdrv_abort_perm_update(child_bs);
+bdrv_unref(child_bs);
 return NULL;
 }
 }
-- 
2.21.0

Re: [PATCH for 5.0] block: fix bdrv_root_attach_child forget to unref child_bs

[Kevin Wolf]

Am 24.03.2020 um 16:59 hat Vladimir Sementsov-Ogievskiy geschrieben:
> bdrv_root_attach_child promises to drop child_bs reference on failure.
> It does it on first handled failure path, but not on the second. Fix
> that.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 

Thanks, applied to the block branch.

Kevin

Re: [PATCH v8 11/11] iotests: use python logging for iotests.log()

[Max Reitz]

On 17.03.20 01:41, John Snow wrote:
> We can turn logging on/off globally instead of per-function.
> 
> Remove use_log from run_job, and use python logging to turn on
> diffable output when we run through a script entry point.
> 
> iotest 245 changes output order due to buffering reasons.
> 
> 
> An extended note on python logging:

Thanks!

> A NullHandler is added to `qemu.iotests` to stop output from being
> generated if this code is used as a library without configuring logging.
> A NullHandler is only needed at the root, so a duplicate handler is not
> needed for `qemu.iotests.diff_io`.
> 
> When logging is not configured, messages at the 'WARNING' levels or
> above are printed with default settings. The NullHandler stops this from
> occurring, which is considered good hygiene for code used as a library.
> 
> See https://docs.python.org/3/howto/logging.html#library-config
> 
> When logging is actually enabled (always at the behest of an explicit
> call by a client script), a root logger is implicitly created at the
> root, which allows messages to propagate upwards and be handled/emitted
> from the root logger with default settings.
> 
> When we want iotest logging, we attach a handler to the
> qemu.iotests.diff_io logger and disable propagation to avoid possible
> double-printing.
> 
> For more information on python logging infrastructure, I highly
> recommend downloading the pip package `logging_tree`, which provides
> convenient visualizations of the hierarchical logging configuration
> under different circumstances.
> 
> See https://pypi.org/project/logging_tree/ for more information.
> 
> Signed-off-by: John Snow 
> ---
>  tests/qemu-iotests/030|  4 +--
>  tests/qemu-iotests/155|  2 +-
>  tests/qemu-iotests/245|  1 +
>  tests/qemu-iotests/245.out| 24 
>  tests/qemu-iotests/iotests.py | 53 ---
>  5 files changed, 46 insertions(+), 38 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v8 06/11] iotests: drop Python 3.4 compatibility code

[John Snow]

On 3/24/20 10:54 AM, Max Reitz wrote:
> On 17.03.20 01:41, John Snow wrote:
>> We no longer need to accommodate 3.4, drop this code.
> 
> Pre-3.4, actually.
> 
>> (Also, the line is over 79 characters, so drop it.)
>>
>> Touch up the docstring a little bit while we're here.
>>
>> Signed-off-by: John Snow 
>> ---
>>  tests/qemu-iotests/iotests.py | 14 +++---
>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 7cd74e7cb1..3d90fb157d 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -22,6 +22,7 @@
>>  import unittest
>>  import sys
>>  import struct
>> +from typing import Optional
>>  import json
>>  import signal
>>  import logging
>> @@ -350,18 +351,17 @@ def _filter(_key, value):
>>  return value
>>  return filter_qmp(qmsg, _filter)
>>  
>> -def log(msg, filters=(), indent=None):
>> -'''Logs either a string message or a JSON serializable message (like 
>> QMP).
>> -If indent is provided, JSON serializable messages are pretty-printed.'''
>> +def log(msg, filters=(), indent: Optional[int] = None) -> None:
>> +"""
>> +Logs either a string message or a JSON serializable message (like QMP).
>> +If indent is provided, JSON serializable messages are pretty-printed.
>> +"""
> 
> I feel like I should complain about this unrelated (I think?) change,
> but I won’t.
> 

It just seemed like so little to have in its own little patch, and I
wasn't prepared to fix the docstrings in the whole file ...

... It will be a future patch that tidies up this whole file and drops
the missing-docstring ignore from pylintrc.

> Reviewed-by: Max Reitz 
> 

Thanks.

>>  for flt in filters:
>>  msg = flt(msg)
>>  if isinstance(msg, (dict, list)):
>> -# Python < 3.4 needs to know not to add whitespace when 
>> pretty-printing:
>> -separators = (', ', ': ') if indent is None else (',', ': ')
>>  # Don't sort if it's already sorted
>>  do_sort = not isinstance(msg, OrderedDict)
>> -print(json.dumps(msg, sort_keys=do_sort,
>> - indent=indent, separators=separators))
>> +print(json.dumps(msg, sort_keys=do_sort, indent=indent))
>>  else:
>>  print(msg)
>>  
>>
> 
> 

Re: [PATCH v4] block/nvme: introduce PMR support from NVMe 1.4 spec

[Andrzej Jakowski]

On 3/23/20 6:28 AM, Stefan Hajnoczi wrote:
> Excellent, thank you!
> 
> Reviewed-by: Stefan Hajnoczi 

Awesome, thx! Not sure about process...
Is this patch now staged for inclusion in QEMU?

Re: [PATCH v8 07/11] iotests: limit line length to 79 chars

[John Snow]

On 3/24/20 11:12 AM, Max Reitz wrote:
> On 24.03.20 16:02, Max Reitz wrote:
>> On 17.03.20 01:41, John Snow wrote:
>>> 79 is the PEP8 recommendation. This recommendation works well for
>>> reading patch diffs in TUI email clients.
>>
>> Also for my very GUI-y diff program (kompare).
>>
>>> Signed-off-by: John Snow 
>>> ---
>>>  tests/qemu-iotests/iotests.py | 64 +++
>>>  tests/qemu-iotests/pylintrc   |  6 +++-
>>>  2 files changed, 47 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>>> index 3d90fb157d..75fd697d77 100644
>>> --- a/tests/qemu-iotests/iotests.py
>>> +++ b/tests/qemu-iotests/iotests.py
>>
>> [...]
>>
>>> @@ -529,11 +539,13 @@ def pause_drive(self, drive, event=None):
>>>  self.pause_drive(drive, "write_aio")
>>>  return
>>>  self.qmp('human-monitor-command',
>>> - command_line='qemu-io %s "break %s bp_%s"' % (drive, 
>>> event, drive))
>>> + command_line='qemu-io %s "break %s bp_%s"'
>>> + % (drive, event, drive))
>>
>> Can we put this value in a variable instead?  I don’t like the %
>> aligning with the parameter name instead of the string value.  (I also
>> don’t like how there are no spaces around the assignment =, but around
>> the %, even though the % binds more strongly.)
>>
>>>  

I think I had another patch somewhere that added an HMP helper that
fixes this issue for this particular instance.

I can send that separately as a follow-up. I think otherwise,
unfortunately, "we" "decided" that this indent style is "best".

So I think that this patch is "correct".


(All of the other options for indent styles seemed to be worse visually,
or actively go against PEP8. While PEP8 is not a bible, every conscious
choice to disregard it generally means you will be fighting a CQA tool
at some other point in time. I have therefore adopted a "When in Rome"
policy to reduce friction wherever possible with pylint, flake8, mypy,
pycharm, and so on.)


((I would prefer we begin to deprecate uses of % and begin using
.format() and f-strings wherever possible to help alleviate this kind of
syntax, too -- but I must insist that's for another series.))


>>>  def resume_drive(self, drive):
>>>  self.qmp('human-monitor-command',
>>> - command_line='qemu-io %s "remove_break bp_%s"' % (drive, 
>>> drive))
>>> + command_line='qemu-io %s "remove_break bp_%s"'
>>> + % (drive, drive))
>>>  
>>>  def hmp_qemu_io(self, drive, cmd):
>>>  '''Write to a given drive using an HMP command'''
>>> @@ -793,16 +805,18 @@ def dictpath(self, d, path):
>>>  idx = int(idx)
>>>  
>>>  if not isinstance(d, dict) or component not in d:
>>> -self.fail('failed path traversal for "%s" in "%s"' % 
>>> (path, str(d)))
>>> +self.fail(f'failed path traversal for "{path}" in "{d}"')
>>
>> Do we require 3.6 so that f-strings are guaranteed to work?  (I thought
>> we didn’t.  I’d be happy to use them.)
> 
> Oh.  Of course we do.  It says so in this file that this whole series is
> about.
> 

Yup. I didn't like the idea of iotests using a newer version of python
than our build system, but my concern was not shared, so we get to use
f-strings for non-buildtime tools.

End of support for Python 3.5 will be 2020-09-13; so we'll get to use
f-strings everywhere else soon, too.

--js

Re: [PATCH 2/6] block/mirror: fix use after free of local_err

[John Snow]

On 3/24/20 11:36 AM, Vladimir Sementsov-Ogievskiy wrote:
> local_err is used again in mirror_exit_common() after
> bdrv_set_backing_hd(), so we must zero it. Otherwise try to set
> non-NULL local_err will crash.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/mirror.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 447051dbc6..6203e5946e 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -678,6 +678,7 @@ static int mirror_exit_common(Job *job)
>  bdrv_set_backing_hd(target_bs, backing, &local_err);
>  if (local_err) {
>  error_report_err(local_err);
> +local_err = NULL;
>  ret = -EPERM;
>  }
>  }
> 

Reviewed-by: John Snow 

[PATCH for-5.0 v2 2/3] iotests: Add poke_file_[bl]e functions

[Max Reitz]

Similarly to peek_file_[bl]e, we may want to write binary integers into
a file.  Currently, this often means messing around with poke_file and
raw binary strings.  I hope these functions make it a bit more
comfortable.

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

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 4c246c0450..bf3b9fdea0 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -53,6 +53,30 @@ poke_file()
 printf "$3" | dd "of=$1" bs=1 "seek=$2" conv=notrunc &>/dev/null
 }
 
+# poke_file_le $img_filename $offset $byte_width $value
+# Example: poke_file_le "$TEST_IMG" 512 2 65534
+poke_file_le()
+{
+local img=$1 ofs=$2 len=$3 val=$4 str=''
+
+while ((len--)); do
+str+=$(printf '\\x%02x' $((val & 0xff)))
+val=$((val >> 8))
+done
+
+poke_file "$img" "$ofs" "$str"
+}
+
+# poke_file_be $img_filename $offset $byte_width $value
+# Example: poke_file_be "$TEST_IMG" 512 2 65279
+poke_file_be()
+{
+local img=$1 ofs=$2 len=$3 val=$4
+local str=$(printf "%0$((len * 2))x\n" $val | sed 's/\(..\)/\\x\1/g')
+
+poke_file "$img" "$ofs" "$str"
+}
+
 # peek_file_le 'test.img' 512 2 => 65534
 peek_file_le()
 {
-- 
2.25.1

[PATCH for-5.0 v2 0/3] qemu-img: Fix check's leak/corruption fix report

[Max Reitz]

Branch: https://github.com/XanClic/qemu.git fix-check-result-v2
Branch: https://git.xanclic.moe/XanClic/qemu.git fix-check-result-v2

v1: https://lists.nongnu.org/archive/html/qemu-block/2020-02/msg01418.html


Hi,

While reviewing the “fix two small memleaks” series
(<20200227012950.12256-1-pannengy...@huawei.com>) I noticed that we save
ImageCheck.(leaks|corruptions)_fixed from the first run and overwrite
the values obtained in the second run (where they must be zero because
we do not request any fixes in that second run), but we do not overwrite
ImageCheck.has_(leaks|corruptions)_fixed after the second run.  That
smells fishy.

Furthermore, ImageCheck.has_(leaks|corruptions)_fixed are not set based
on whether (leaks|corruptions)_fixed is non-zero, but actually based on
whether the leaks and corruptions fields (respectively) are non-zero.
qcow2’s check implementation reduces the leaks and corruptions values
when it fixes them, because then there are no leaks and corruptions
after the check anymore.

All in all, after a successful run, you will not see
“qemu-img check --output=json” report corruptions-fixed or leaks-fixed.
Which is a shame.  So this series fixes that and adds a test to ensure
those fields are indeed reported.


v2:
- Patch 2:
  - Rewrote the new functions as per Eric’s suggestions (thanks a lot!)
  - Changed the functions’ “doc strings” to not just show an example,
but perhaps more importantly what each parameter means
- Patch 3:
  - Eric said the pre-existing comment explaining why 138 doesn’t
supported data_file made it sound like qemu-img check had a bug.
I’m sure it does have bugs, but this isn’t one, so I tried to
explain it a bit differently.
  - Use poke_file_be in another place
  - Convert sed | grep | sed to a single sed, as per Eric’s suggestion,
and avoid \< and \>


git-backport-diff against v1:

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

001/3:[] [--] 'qemu-img: Fix check's leak/corruption fix report'
002/3:[0029] [FC] 'iotests: Add poke_file_[bl]e functions'
003/3:[0008] [FC] 'iotests/138: Test leaks/corruptions fixed report'


Max Reitz (3):
  qemu-img: Fix check's leak/corruption fix report
  iotests: Add poke_file_[bl]e functions
  iotests/138: Test leaks/corruptions fixed report

 qemu-img.c   |  9 ++--
 tests/qemu-iotests/138   | 41 ++--
 tests/qemu-iotests/138.out   | 14 
 tests/qemu-iotests/common.rc | 24 +
 4 files changed, 84 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH for-5.0 v2 1/3] qemu-img: Fix check's leak/corruption fix report

[Max Reitz]

There are two problems with qemu-img check's report on how many leaks
and/or corruptions have been fixed:

(1) ImageCheck.has_leaks_fixed and ImageCheck.has_corruptions_fixed are
only true when ImageCheck.leaks or ImageCheck.corruptions (respectively)
are non-zero.  qcow2's check implementation will set the latter to zero
after it has fixed leaks and corruptions, though, so leaks-fixed and
corruptions-fixed are actually never reported after successful repairs.
We should always report them when they are non-zero, just like all the
other fields of ImageCheck.

(2) After something has been fixed and we run the check a second time,
leaks_fixed and corruptions_fixed are taken from the first run; but
has_leaks_fixed and has_corruptions_fixed are not.  The second run
actually cannot fix anything, so with (1) fixed, has_leaks_fixed and
has_corruptions_fixed will always be false here.  (With (1) unfixed,
they will at least be false on successful runs, because then the number
of leaks and corruptions found in the second run should be 0.)
We should save has_leaks_fixed and has_corruptions_fixed just like we
save leaks_fixed and corruptions_fixed.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 qemu-img.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index afddf33f08..b167376bd7 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -647,9 +647,9 @@ static int collect_image_check(BlockDriverState *bs,
 check->leaks= result.leaks;
 check->has_leaks= result.leaks != 0;
 check->corruptions_fixed= result.corruptions_fixed;
-check->has_corruptions_fixed= result.corruptions != 0;
+check->has_corruptions_fixed= result.corruptions_fixed != 0;
 check->leaks_fixed  = result.leaks_fixed;
-check->has_leaks_fixed  = result.leaks != 0;
+check->has_leaks_fixed  = result.leaks_fixed != 0;
 check->image_end_offset = result.image_end_offset;
 check->has_image_end_offset = result.image_end_offset != 0;
 check->total_clusters   = result.bfi.total_clusters;
@@ -803,9 +803,12 @@ static int img_check(int argc, char **argv)
 
 if (check->corruptions_fixed || check->leaks_fixed) {
 int corruptions_fixed, leaks_fixed;
+bool has_leaks_fixed, has_corruptions_fixed;
 
 leaks_fixed = check->leaks_fixed;
+has_leaks_fixed = check->has_leaks_fixed;
 corruptions_fixed   = check->corruptions_fixed;
+has_corruptions_fixed = check->has_corruptions_fixed;
 
 if (output_format == OFORMAT_HUMAN) {
 qprintf(quiet,
@@ -822,7 +825,9 @@ static int img_check(int argc, char **argv)
 ret = collect_image_check(bs, check, filename, fmt, 0);
 
 check->leaks_fixed  = leaks_fixed;
+check->has_leaks_fixed  = has_leaks_fixed;
 check->corruptions_fixed= corruptions_fixed;
+check->has_corruptions_fixed = has_corruptions_fixed;
 }
 
 if (!ret) {
-- 
2.25.1

[PATCH for-5.0 v2 3/3] iotests/138: Test leaks/corruptions fixed report

[Max Reitz]

Test that qemu-img check reports the number of leaks and corruptions
fixed in its JSON report (after a successful run).

While touching the _unsupported_imgopts line, adjust the note on why
data_file does not work with this test: The current comment sounds a bit
like it is a mistake for qemu-img check not to check external data
files' refcounts.  But there are no such refcounts, so it is no mistake.
Just say that qemu-img check does not do much for external data files,
and this is why this test does not work with them.

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/138 | 41 --
 tests/qemu-iotests/138.out | 14 +
 2 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/138 b/tests/qemu-iotests/138
index 54b01046ad..1d5b0bed6d 100755
--- a/tests/qemu-iotests/138
+++ b/tests/qemu-iotests/138
@@ -41,8 +41,10 @@ _supported_fmt qcow2
 _supported_proto file
 _supported_os Linux
 # With an external data file, data clusters are not refcounted
-# (and so qemu-img check does not check their refcount)
-_unsupported_imgopts data_file
+# (so qemu-img check would not do much);
+# we want to modify the refcounts, so we need them to have a specific
+# format (namely u16)
+_unsupported_imgopts data_file 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
 
 echo
 echo '=== Check on an image with a multiple of 2^32 clusters ==='
@@ -65,6 +67,41 @@ poke_file "$TEST_IMG" $((2048 + 8)) 
"\x00\x80\x00\x00\x00\x00\x00\x00"
 # allocate memory", we have an error showing that l2 entry is invalid.
 _check_test_img
 
+echo
+echo '=== Check leaks-fixed/corruptions-fixed report'
+echo
+
+# After leaks and corruptions were fixed, those numbers should be
+# reported by qemu-img check
+_make_test_img 64k
+
+# Allocate data cluster
+$QEMU_IO -c 'write 0 64k' "$TEST_IMG" | _filter_qemu_io
+
+reftable_ofs=$(peek_file_be "$TEST_IMG" 48 8)
+refblock_ofs=$(peek_file_be "$TEST_IMG" $reftable_ofs 8)
+
+# Introduce a leak: Make the image header's refcount 2
+poke_file_be "$TEST_IMG" "$refblock_ofs" 2 2
+
+l1_ofs=$(peek_file_be "$TEST_IMG" 40 8)
+
+# Introduce a corruption: Drop the COPIED flag from the (first) L1 entry
+l1_entry=$(peek_file_be "$TEST_IMG" $l1_ofs 8)
+l1_entry=$((l1_entry & ~(1 << 63)))
+poke_file_be "$TEST_IMG" $l1_ofs 8 $l1_entry
+
+echo
+# Should print the number of corruptions and leaks fixed
+# (Filter out all JSON fields (recognizable by their four-space
+# indentation), but keep the "-fixed" fields (by removing two spaces
+# from their indentation))
+# (Also filter out the L1 entry, because why not)
+_check_test_img -r all --output=json \
+| sed -e 's/^  \(.*\)-fixed"/\1-fixed"/' \
+  -e '/^/d' \
+  -e "s/\\([^0-9a-f]\\)$(printf %x 
$l1_entry)\\([^0-9a-f]\\)/\1L1_ENTRY_VALUE\2/"
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/138.out b/tests/qemu-iotests/138.out
index aca7d47a80..79681e7cc9 100644
--- a/tests/qemu-iotests/138.out
+++ b/tests/qemu-iotests/138.out
@@ -9,4 +9,18 @@ ERROR: counting reference for region exceeding the end of the 
file by one cluste
 
 1 errors were found on the image.
 Data may be corrupted, or further writes to the image may corrupt it.
+
+=== Check leaks-fixed/corruptions-fixed report
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+Leaked cluster 0 refcount=2 reference=1
+Repairing cluster 0 refcount=2 reference=1
+Repairing OFLAG_COPIED L2 cluster: l1_index=0 l1_entry=L1_ENTRY_VALUE 
refcount=1
+{
+  "corruptions-fixed": 1,
+  "leaks-fixed": 1,
+}
 *** done
-- 
2.25.1

Re: [PATCH v8 07/11] iotests: limit line length to 79 chars

[Max Reitz]

On 24.03.20 18:09, John Snow wrote:
> 
> 
> On 3/24/20 11:12 AM, Max Reitz wrote:
>> On 24.03.20 16:02, Max Reitz wrote:
>>> On 17.03.20 01:41, John Snow wrote:
 79 is the PEP8 recommendation. This recommendation works well for
 reading patch diffs in TUI email clients.
>>>
>>> Also for my very GUI-y diff program (kompare).
>>>
 Signed-off-by: John Snow 
 ---
  tests/qemu-iotests/iotests.py | 64 +++
  tests/qemu-iotests/pylintrc   |  6 +++-
  2 files changed, 47 insertions(+), 23 deletions(-)

 diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
 index 3d90fb157d..75fd697d77 100644
 --- a/tests/qemu-iotests/iotests.py
 +++ b/tests/qemu-iotests/iotests.py
>>>
>>> [...]
>>>
 @@ -529,11 +539,13 @@ def pause_drive(self, drive, event=None):
  self.pause_drive(drive, "write_aio")
  return
  self.qmp('human-monitor-command',
 - command_line='qemu-io %s "break %s bp_%s"' % (drive, 
 event, drive))
 + command_line='qemu-io %s "break %s bp_%s"'
 + % (drive, event, drive))
>>>
>>> Can we put this value in a variable instead?  I don’t like the %
>>> aligning with the parameter name instead of the string value.  (I also
>>> don’t like how there are no spaces around the assignment =, but around
>>> the %, even though the % binds more strongly.)
>>>
  
> 
> I think I had another patch somewhere that added an HMP helper that
> fixes this issue for this particular instance.
> 
> I can send that separately as a follow-up. I think otherwise,
> unfortunately, "we" "decided" that this indent style is "best".
> 
> So I think that this patch is "correct".

Perhaps it’s the best if we assume that we had to do it this way, but
can’t we just do a

qemu_io_cmd = f'qemu-io {drive} "break {event} bp_{drive}"'
self.qmp('human-monitor-command', command_line=qemu_io_cmd)

?

(Or maybe an f-string inline.  I was thinking of a separate variable
because I wasn’t aware that f-strings would be an option until I got to
later hunks of this patch...)

> (All of the other options for indent styles seemed to be worse visually,
> or actively go against PEP8. While PEP8 is not a bible, every conscious
> choice to disregard it generally means you will be fighting a CQA tool
> at some other point in time. I have therefore adopted a "When in Rome"
> policy to reduce friction wherever possible with pylint, flake8, mypy,
> pycharm, and so on.)
> 
> 
> ((I would prefer we begin to deprecate uses of % and begin using
> .format() and f-strings wherever possible to help alleviate this kind of
> syntax, too -- but I must insist that's for another series.))

Hm.  But you do change something to f-strings below, why not here?

Max



signature.asc
Description: OpenPGP digital signature

[PATCH for-5.0 v2 0/4] bug fixes extracted from larger qcow2 zero bit work

[Eric Blake]

My proposal [1] to add an autoclear all-zero-content bit to the qcow2
format has now stalled into 5.1 territory, but several patches in my
initial proposal are uncontroversial and obvious bug fixes worth
having in 5.0.

[1] https://lists.gnu.org/archive/html/qemu-devel/2020-01/msg08075.html

Eric Blake (4):
  qcow2: Comment typo fixes
  qcow2: List autoclear bit names in header
  qcow2: Avoid feature name extension on small cluster size
  sheepdog: Consistently set bdrv_has_zero_init_truncate

 docs/interop/qcow2.txt |  3 ++-
 block/qcow2.c  | 29 +++--
 block/sheepdog.c   |  2 ++
 tests/qemu-iotests/031.out |  8 
 tests/qemu-iotests/036 |  6 --
 tests/qemu-iotests/036.out |  4 ++--
 tests/qemu-iotests/061 |  6 --
 tests/qemu-iotests/061.out | 14 +++---
 8 files changed, 48 insertions(+), 24 deletions(-)

-- 
2.26.0.rc2

[PATCH v2 1/4] qcow2: Comment typo fixes

[Eric Blake]

Various trivial typos noticed while working on this file.

Signed-off-by: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Alberto Garcia 
---
 block/qcow2.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index d1da3d91db21..5a7f30e692a2 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -177,7 +177,7 @@ static ssize_t qcow2_crypto_hdr_write_func(QCryptoBlock 
*block, size_t offset,
 }


-/* 
+/*
  * read qcow2 extension and fill bs
  * start reading from start_offset
  * finish reading upon magic of value 0 or when end_offset reached
@@ -3255,7 +3255,7 @@ qcow2_co_create(BlockdevCreateOptions *create_options, 
Error **errp)
  * inconsistency later.
  *
  * We do need a refcount table because growing the refcount table means
- * allocating two new refcount blocks - the seconds of which would be at
+ * allocating two new refcount blocks - the second of which would be at
  * 2 GB for 64k clusters, and we don't want to have a 2 GB initial file
  * size for any qcow2 image.
  */
@@ -3500,7 +3500,7 @@ qcow2_co_create(BlockdevCreateOptions *create_options, 
Error **errp)
 goto out;
 }

-/* Want a backing file? There you go.*/
+/* Want a backing file? There you go. */
 if (qcow2_opts->has_backing_file) {
 const char *backing_format = NULL;

-- 
2.26.0.rc2

[PATCH v2 3/4] qcow2: Avoid feature name extension on small cluster size

[Eric Blake]

As the feature name table can be quite large (over 9k if all 64 bits
of all three feature fields have names; a mere 8 features leaves only
8 bytes for a backing file name in a 512-byte cluster), it is unwise
to emit this optional header in images with small cluster sizes.

Update iotest 036 to skip running on small cluster sizes; meanwhile,
note that iotest 061 never passed on alternative cluster sizes
(however, I limited this patch to tests with output affected by adding
feature names, rather than auditing for other tests that are not
robust to alternative cluster sizes).

Signed-off-by: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Alberto Garcia 
---
 block/qcow2.c  | 11 +--
 tests/qemu-iotests/036 |  6 --
 tests/qemu-iotests/061 |  6 --
 3 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 67b0c214fba4..9475ace57163 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2823,8 +2823,15 @@ int qcow2_update_header(BlockDriverState *bs)
 buflen -= ret;
 }

-/* Feature table */
-if (s->qcow_version >= 3) {
+/*
+ * Feature table.  A mere 8 feature names occupies 392 bytes, and
+ * when coupled with the v3 minimum header of 104 bytes plus the
+ * 8-byte end-of-extension marker, that would leave only 8 bytes
+ * for a backing file name in an image with 512-byte clusters.
+ * Thus, we choose to omit this header for cluster sizes 4k and
+ * smaller.
+ */
+if (s->qcow_version >= 3 && s->cluster_size > 4096) {
 static const Qcow2Feature features[] = {
 {
 .type = QCOW2_FEAT_TYPE_INCOMPATIBLE,
diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036
index 512598421c20..cf522de7a1aa 100755
--- a/tests/qemu-iotests/036
+++ b/tests/qemu-iotests/036
@@ -44,8 +44,10 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2
 _supported_proto file
 # Only qcow2v3 and later supports feature bits;
-# qcow2.py does not support external data files
-_unsupported_imgopts 'compat=0.10' data_file
+# qcow2.py does not support external data files;
+# this test requires a cluster size large enough for the feature table
+_unsupported_imgopts 'compat=0.10' data_file \
+'cluster_size=\(512\|1024\|2048\|4096\)'

 echo
 echo === Image with unknown incompatible feature bit ===
diff --git a/tests/qemu-iotests/061 b/tests/qemu-iotests/061
index 36b040491fef..ce285d308408 100755
--- a/tests/qemu-iotests/061
+++ b/tests/qemu-iotests/061
@@ -44,8 +44,10 @@ _supported_os Linux
 # Conversion between different compat versions can only really work
 # with refcount_bits=16;
 # we have explicit tests for data_file here, but the whole test does
-# not work with it
-_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)' data_file
+# not work with it;
+# we have explicit tests for various cluster sizes, the remaining tests
+# require the default 64k cluster
+_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)' data_file 
cluster_size

 echo
 echo "=== Testing version downgrade with zero expansion ==="
-- 
2.26.0.rc2

[PATCH v2 2/4] qcow2: List autoclear bit names in header

[Eric Blake]

The feature table is supposed to advertise the name of all feature
bits that we support; however, we forgot to update the table for
autoclear bits.  While at it, move the table to read-only memory in
code, and tweak the qcow2 spec to name the second autoclear bit.
Update iotests that are affected by the longer header length.

Fixes: 88ddffae
Fixes: 93c24936
Signed-off-by: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 

---
[v2] tweak name of "bitmaps" autoclear bit [Vladimir]
---
 docs/interop/qcow2.txt |  3 ++-
 block/qcow2.c  | 12 +++-
 tests/qemu-iotests/031.out |  8 
 tests/qemu-iotests/036.out |  4 ++--
 tests/qemu-iotests/061.out | 14 +++---
 5 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 5597e244746e..640e0eca4000 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -143,7 +143,8 @@ the next fields through header_length.
 bit is unset, the bitmaps extension data must 
be
 considered inconsistent.

-Bit 1:  If this bit is set, the external data file can
+Bit 1:  Raw external data bit
+If this bit is set, the external data file can
 be read as a consistent standalone raw image
 without looking at the qcow2 metadata.

diff --git a/block/qcow2.c b/block/qcow2.c
index 5a7f30e692a2..67b0c214fba4 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2825,7 +2825,7 @@ int qcow2_update_header(BlockDriverState *bs)

 /* Feature table */
 if (s->qcow_version >= 3) {
-Qcow2Feature features[] = {
+static const Qcow2Feature features[] = {
 {
 .type = QCOW2_FEAT_TYPE_INCOMPATIBLE,
 .bit  = QCOW2_INCOMPAT_DIRTY_BITNR,
@@ -2846,6 +2846,16 @@ int qcow2_update_header(BlockDriverState *bs)
 .bit  = QCOW2_COMPAT_LAZY_REFCOUNTS_BITNR,
 .name = "lazy refcounts",
 },
+{
+.type = QCOW2_FEAT_TYPE_AUTOCLEAR,
+.bit  = QCOW2_AUTOCLEAR_BITMAPS_BITNR,
+.name = "bitmaps",
+},
+{
+.type = QCOW2_FEAT_TYPE_AUTOCLEAR,
+.bit  = QCOW2_AUTOCLEAR_DATA_FILE_RAW_BITNR,
+.name = "raw external data",
+},
 };

 ret = header_ext_add(buf, QCOW2_EXT_MAGIC_FEATURE_TABLE,
diff --git a/tests/qemu-iotests/031.out b/tests/qemu-iotests/031.out
index d535e407bc30..46f97c5a4ea4 100644
--- a/tests/qemu-iotests/031.out
+++ b/tests/qemu-iotests/031.out
@@ -117,7 +117,7 @@ header_length 104

 Header extension:
 magic 0x6803f857
-length192
+length288
 data  

 Header extension:
@@ -150,7 +150,7 @@ header_length 104

 Header extension:
 magic 0x6803f857
-length192
+length288
 data  

 Header extension:
@@ -164,7 +164,7 @@ No errors were found on the image.

 magic 0x514649fb
 version   3
-backing_file_offset   0x178
+backing_file_offset   0x1d8
 backing_file_size 0x17
 cluster_bits  16
 size  67108864
@@ -188,7 +188,7 @@ data  'host_device'

 Header extension:
 magic 0x6803f857
-length192
+length288
 data  

 Header extension:
diff --git a/tests/qemu-iotests/036.out b/tests/qemu-iotests/036.out
index 0b52b934e115..23b699ce0622 100644
--- a/tests/qemu-iotests/036.out
+++ b/tests/qemu-iotests/036.out
@@ -26,7 +26,7 @@ compatible_features   []
 autoclear_features[63]
 Header extension:
 magic 0x6803f857
-length192
+length288
 data  


@@ -38,7 +38,7 @@ compatible_features   []
 autoclear_features[]
 Header extension:
 magic 0x6803f857
-length192
+length288
 data  

 *** done
diff --git a/tests/qemu-iotests/061.out b/tests/qemu-iotests/061.out
index 8b3091a412bc..413cc4e0f4ab 100644
--- a/tests/qemu-iotests/061.out
+++ b/tests/qemu-iotests/061.out
@@ -26,7 +26,7 @@ header_length 104

 Header extension:
 magic 0x6803f857
-length192
+length288
 data  

 magic 0x514649fb
@@ -84,7 +84,7 @@ header_length 104

 Header extension:
 magic 0x6803f857
-length192
+length288
 data  

 magic 0x514649fb
@@ -140,7 +140

[PATCH v2 4/4] sheepdog: Consistently set bdrv_has_zero_init_truncate

[Eric Blake]

block_int.h claims that .bdrv_has_zero_init must return 0 if
.bdrv_has_zero_init_truncate does likewise; but this is violated if
only the former callback is provided if .bdrv_co_truncate also exists.
When adding the latter callback, it was mistakenly added to only one
of the three possible sheepdog instantiations.

Fixes: 1dcaf527
Signed-off-by: Eric Blake 
---
 block/sheepdog.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index cfa84338a2d6..522c16a93676 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -3269,6 +3269,7 @@ static BlockDriver bdrv_sheepdog_tcp = {
 .bdrv_co_create   = sd_co_create,
 .bdrv_co_create_opts  = sd_co_create_opts,
 .bdrv_has_zero_init   = bdrv_has_zero_init_1,
+.bdrv_has_zero_init_truncate  = bdrv_has_zero_init_1,
 .bdrv_getlength   = sd_getlength,
 .bdrv_get_allocated_file_size = sd_get_allocated_file_size,
 .bdrv_co_truncate = sd_co_truncate,
@@ -3307,6 +3308,7 @@ static BlockDriver bdrv_sheepdog_unix = {
 .bdrv_co_create   = sd_co_create,
 .bdrv_co_create_opts  = sd_co_create_opts,
 .bdrv_has_zero_init   = bdrv_has_zero_init_1,
+.bdrv_has_zero_init_truncate  = bdrv_has_zero_init_1,
 .bdrv_getlength   = sd_getlength,
 .bdrv_get_allocated_file_size = sd_get_allocated_file_size,
 .bdrv_co_truncate = sd_co_truncate,
-- 
2.26.0.rc2

Re: [PATCH for-5.0 v2 2/3] iotests: Add poke_file_[bl]e functions

[Eric Blake]

On 3/24/20 12:27 PM, Max Reitz wrote:

Similarly to peek_file_[bl]e, we may want to write binary integers into
a file.  Currently, this often means messing around with poke_file and
raw binary strings.  I hope these functions make it a bit more
comfortable.

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

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 4c246c0450..bf3b9fdea0 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -53,6 +53,30 @@ poke_file()
  printf "$3" | dd "of=$1" bs=1 "seek=$2" conv=notrunc &>/dev/null
  }
  
+# poke_file_le $img_filename $offset $byte_width $value

+# Example: poke_file_le "$TEST_IMG" 512 2 65534
+poke_file_le()


Yep, that's nicer.


+{
+local img=$1 ofs=$2 len=$3 val=$4 str=''
+
+while ((len--)); do
+str+=$(printf '\\x%02x' $((val & 0xff)))
+val=$((val >> 8))
+done
+
+poke_file "$img" "$ofs" "$str"
+}


and so is that (but I'm biased, here :)

Reviewed-by: Eric Blake 

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

Re: [PATCH for-5.0 v2 3/3] iotests/138: Test leaks/corruptions fixed report

[Eric Blake]

On 3/24/20 12:27 PM, Max Reitz wrote:

Test that qemu-img check reports the number of leaks and corruptions
fixed in its JSON report (after a successful run).

While touching the _unsupported_imgopts line, adjust the note on why
data_file does not work with this test: The current comment sounds a bit
like it is a mistake for qemu-img check not to check external data
files' refcounts.  But there are no such refcounts, so it is no mistake.
Just say that qemu-img check does not do much for external data files,
and this is why this test does not work with them.

Signed-off-by: Max Reitz 
---
  tests/qemu-iotests/138 | 41 --
  tests/qemu-iotests/138.out | 14 +
  2 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/138 b/tests/qemu-iotests/138
index 54b01046ad..1d5b0bed6d 100755
--- a/tests/qemu-iotests/138
+++ b/tests/qemu-iotests/138
@@ -41,8 +41,10 @@ _supported_fmt qcow2
  _supported_proto file
  _supported_os Linux
  # With an external data file, data clusters are not refcounted
-# (and so qemu-img check does not check their refcount)
-_unsupported_imgopts data_file
+# (so qemu-img check would not do much);


Works for me.


+echo
+# Should print the number of corruptions and leaks fixed
+# (Filter out all JSON fields (recognizable by their four-space
+# indentation), but keep the "-fixed" fields (by removing two spaces
+# from their indentation))
+# (Also filter out the L1 entry, because why not)
+_check_test_img -r all --output=json \
+| sed -e 's/^  \(.*\)-fixed"/\1-fixed"/' \
+  -e '/^/d' \
+  -e "s/\\([^0-9a-f]\\)$(printf %x 
$l1_entry)\\([^0-9a-f]\\)/\1L1_ENTRY_VALUE\2/"


It's fun to see the post-review results.  Given the limited times where 
the third -e fires in the output,



+++ b/tests/qemu-iotests/138.out



+Leaked cluster 0 refcount=2 reference=1
+Repairing cluster 0 refcount=2 reference=1
+Repairing OFLAG_COPIED L2 cluster: l1_index=0 l1_entry=L1_ENTRY_VALUE 
refcount=1


it could be written with less typing as:

-e "s/=$(printf %x $l1_entry) /=L1_ENTRY_VALUE /"

but that's not essential.  So either way,

Reviewed-by: Eric Blake 

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

Re: block stream and bitmaps

[John Snow]

On 3/24/20 6:18 AM, Kevin Wolf wrote:
> Am 23.03.2020 um 19:06 hat John Snow geschrieben:
>> Hi Kevin,
>>
>> I'm hoping to get some preliminary ideas from you (capped at five
>> minutes' effort?) on this subject. My ideas here are a bit shaky, I only
>> have really rough notions here.
>>
>> We want to use bitmaps with 'drive' semantics; i.e. tracking changes to
>> the visible guest data. What we have are bitmaps with node semantics,
>> tracking changes to the data at a particular level in the graph.
>>
>> For commit, this isn't a big deal: we can disable bitmaps in the backing
>> file while we commit and then re-enable it on completion. We usually
>> have a separate bitmap enabled on the root node that is recording writes
>> during this time, and can be moved later.
>>
>> For streaming, this is more challenging: new writes will dirty the
>> bitmap, but so will writes from the stream job itself.
>>
>> Semantically, we want to ignore writes from the stream while recording
>> them from the guest -- differentiating based on source.
> 
> No, based on source is actually not what you want. What you really want
> is that BDRV_REQ_WRITE_UNCHANGED doesn't mark any blocks dirty.
> 

This is why I sent the mail, I figured you'd know the better incision
point, and I was right!

> We discussed this specific case of streaming at FOSDEM (with Paolo and
> probably Nir). Paolo was even convinced that unchanged writes already
> behave like this, but we agreed that dirtying blocks for them would be a
> bug. After checking that the code is indeed buggy, I was planning to
> send a patch, but never got around to actually do that. Sorry about
> that.
> 

Glad to hear it has been given consideration, though!

>> Bitmaps aren't really geared to do that right now. With the changes to
>> Bdrv Roles that Max was engineering, do you think it's possible to add
>> some kind of write source discrimination to bitmaps, or is that too messy?
> 
> I don't think it would work because copy-on-read requests come from the
> same parent node as writes (no matter whether the legacy code in
> block/io.c or a copy-on-read filter node is used).
> 

Oh, understood. Rule that approach out, then.

>> For both commit and stream, it might be nice to say: "This bitmap is
>> enabled, but ignores writes from [all? specific types? specific
>> instances?] jobs.
> 
> Commit is a bit trickier, because it's not WRITE_UNCHANGED. The result
> is only unchanged for the top layer, but not for the backing file you're
> committing to. Not sure if we can represent this condition somehow.
> 

Nothing comes to mind apart from a semantic that applies to a graph
subsection instead of an individual node.

i.e. UNCHANGED as applied to [A --> B].

Not saying that's reasonable to develop... or necessarily even possible
to enforce, just nothing else comes to mind.

>> Or, I wonder if what we truly want is some kind of bitmap "forwarder"
>> object on block-backend objects that represent the semantic drive view,
>> and only writes through that *backend* get forwarded to the bitmaps
>> attached to whatever node the bitmap is actually associated with.
>>
>> (That might wind up causing weird problems too, though... since those
>> objects are no longer intended to be user-addressable, managing that
>> configuration might get intensely strange.)
> 
> Hm... Drive-based does suggest that it's managed at the BlockBackend
> level. So having a bitmap that isn't added as a dirty bitmap to the BDS,
> but only to the BB does make sense to me. The BB would be addressed
> with the qdev ID of the device, as usual (which underlines that it's
> really per device).
> 

That's the rough idea, though if it's needed or not is unclear. We might
be able to get by with node semantics if we jazz them up enough...?

Working around all the edge bases of a drive-semantic bitmap seem
difficult to reason about.

In general, it should likely be made persistent against the root-most
node to which writes are routed to a protocol node.
In the common case, that means the top-most qcow2 format node of a chain.

But, many of our job filters also route writes to format nodes too, so
which one is the "canonical" store? Is it even possible to define?

Hm.

I suppose we can always allow bitmaps being attached to the BB and
*disallow them* from being made persistent; but simply exist as an
in-memory tool to help ease the pain of managing data consistency during
critical sections.

We can offer merge-to, merge-from semantics for these in-memory-only
bitmaps. Maybe.

Still mulling it over.

> The part that's unclear to me is how to make such bitmaps persistent.
> You can change the root node of a BB and even remove the root node
> completely (for removable devices; but even changing is technically
> remove followed by insert), so you may need to move the bitmap around
> between image files and at least for some time you might not have any
> place to store the bitmap.
> 

Yeah, we've had discussions about this in the past. 

Re: [PATCH v2 4/4] sheepdog: Consistently set bdrv_has_zero_init_truncate

[John Snow]

On 3/24/20 1:42 PM, Eric Blake wrote:
> block_int.h claims that .bdrv_has_zero_init must return 0 if
> .bdrv_has_zero_init_truncate does likewise; but this is violated if
> only the former callback is provided if .bdrv_co_truncate also exists.
> When adding the latter callback, it was mistakenly added to only one
> of the three possible sheepdog instantiations.
> 
> Fixes: 1dcaf527
> Signed-off-by: Eric Blake 
> ---
>  block/sheepdog.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index cfa84338a2d6..522c16a93676 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -3269,6 +3269,7 @@ static BlockDriver bdrv_sheepdog_tcp = {
>  .bdrv_co_create   = sd_co_create,
>  .bdrv_co_create_opts  = sd_co_create_opts,
>  .bdrv_has_zero_init   = bdrv_has_zero_init_1,
> +.bdrv_has_zero_init_truncate  = bdrv_has_zero_init_1,
>  .bdrv_getlength   = sd_getlength,
>  .bdrv_get_allocated_file_size = sd_get_allocated_file_size,
>  .bdrv_co_truncate = sd_co_truncate,
> @@ -3307,6 +3308,7 @@ static BlockDriver bdrv_sheepdog_unix = {
>  .bdrv_co_create   = sd_co_create,
>  .bdrv_co_create_opts  = sd_co_create_opts,
>  .bdrv_has_zero_init   = bdrv_has_zero_init_1,
> +.bdrv_has_zero_init_truncate  = bdrv_has_zero_init_1,
>  .bdrv_getlength   = sd_getlength,
>  .bdrv_get_allocated_file_size = sd_get_allocated_file_size,
>  .bdrv_co_truncate = sd_co_truncate,
> 

Reviewed-by: John Snow 

Re: [PATCH 4/6] migration/colo: fix use after free of local_err

[Dr. David Alan Gilbert]

* Vladimir Sementsov-Ogievskiy (vsement...@virtuozzo.com) wrote:
> local_err is used again in secondary_vm_do_failover() after
> replication_stop_all(), so we must zero it. Otherwise try to set
> non-NULL local_err will crash.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 

Reviewed-by: Dr. David Alan Gilbert 

I'll queue this

> ---
>  migration/colo.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/migration/colo.c b/migration/colo.c
> index 44942c4e23..a54ac84f41 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -93,6 +93,7 @@ static void secondary_vm_do_failover(void)
>  replication_stop_all(true, &local_err);
>  if (local_err) {
>  error_report_err(local_err);
> +local_err = NULL;
>  }
>  
>  /* Notify all filters of all NIC to do checkpoint */
> -- 
> 2.21.0
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH 5/6] migration/ram: fix use after free of local_err

[Dr. David Alan Gilbert]

* Vladimir Sementsov-Ogievskiy (vsement...@virtuozzo.com) wrote:
> local_err is used again in migration_bitmap_sync_precopy() after
> precopy_notify(), so we must zero it. Otherwise try to set
> non-NULL local_err will crash.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  migration/ram.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index c12cfdbe26..04f13feb2e 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -980,6 +980,7 @@ static void migration_bitmap_sync_precopy(RAMState *rs)
>   */
>  if (precopy_notify(PRECOPY_NOTIFY_BEFORE_BITMAP_SYNC, &local_err)) {
>  error_report_err(local_err);
> +local_err = NULL;

Reviewed-by: Dr. David Alan Gilbert 

and queued.


>  }
>  
>  migration_bitmap_sync(rs);
> -- 
> 2.21.0
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

[PULL 0/2] Ide patches

[John Snow]

The following changes since commit 736cf607e40674776d752acc201f565723e86045:

  Update version for v5.0.0-rc0 release (2020-03-24 17:50:00 +)

are available in the Git repository at:

  https://github.com/jnsnow/qemu.git tags/ide-pull-request

for you to fetch changes up to 51058b3b3bcbe62506cf191fca1c0d679bb80f2b:

  hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs() (2020-03-24 
15:52:16 -0400)


Pull request: IDE

Admittedly the first one is not a crisis fix; but I think it's low-risk to
include for rc1.

The second one is yours, and will shush coverity.



Peter Maydell (1):
  hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs()

Sven Schnelle (1):
  fdc/i8257: implement verify transfer mode

 include/hw/isa/isa.h |  1 -
 hw/block/fdc.c   | 61 +---
 hw/dma/i8257.c   | 20 ++-
 hw/ide/sii3112.c |  8 +++---
 4 files changed, 35 insertions(+), 55 deletions(-)

-- 
2.21.1

[PULL 1/2] fdc/i8257: implement verify transfer mode

[John Snow]

From: Sven Schnelle 

While working on the Tulip driver i tried to write some Teledisk images to
a floppy image which didn't work. Turned out that Teledisk checks the written
data by issuing a READ command to the FDC but running the DMA controller
in VERIFY mode. As we ignored the DMA request in that case, the DMA transfer
never finished, and Teledisk reported an error.

The i8257 spec says about verify transfers:

3) DMA verify, which does not actually involve the transfer of data. When an
8257 channel is in the DMA verify mode, it will respond the same as described
for transfer operations, except that no memory or I/O read/write control signals
will be generated.

Hervé proposed to remove all the dma_mode_ok stuff from fdc to have a more
clear boundary between DMA and FDC, so this patch also does that.

Suggested-by: Hervé Poussineau 
Signed-off-by: Sven Schnelle 
Reviewed-by: Hervé Poussineau 
Signed-off-by: John Snow 
---
 include/hw/isa/isa.h |  1 -
 hw/block/fdc.c   | 61 +---
 hw/dma/i8257.c   | 20 ++-
 3 files changed, 31 insertions(+), 51 deletions(-)

diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index e9ac1f1205..59a4d4b50a 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -56,7 +56,6 @@ typedef int (*IsaDmaTransferHandler)(void *opaque, int nchan, 
int pos,
 typedef struct IsaDmaClass {
 InterfaceClass parent;
 
-IsaDmaTransferMode (*get_transfer_mode)(IsaDma *obj, int nchan);
 bool (*has_autoinitialization)(IsaDma *obj, int nchan);
 int (*read_memory)(IsaDma *obj, int nchan, void *buf, int pos, int len);
 int (*write_memory)(IsaDma *obj, int nchan, void *buf, int pos, int len);
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 22e954e0dc..33bc9e2f92 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -1714,53 +1714,28 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int 
direction)
 }
 fdctrl->eot = fdctrl->fifo[6];
 if (fdctrl->dor & FD_DOR_DMAEN) {
-IsaDmaTransferMode dma_mode;
+/* DMA transfer is enabled. */
 IsaDmaClass *k = ISADMA_GET_CLASS(fdctrl->dma);
-bool dma_mode_ok;
-/* DMA transfer are enabled. Check if DMA channel is well programmed */
-dma_mode = k->get_transfer_mode(fdctrl->dma, fdctrl->dma_chann);
-FLOPPY_DPRINTF("dma_mode=%d direction=%d (%d - %d)\n",
-   dma_mode, direction,
-   (128 << fdctrl->fifo[5]) *
+
+FLOPPY_DPRINTF("direction=%d (%d - %d)\n",
+   direction, (128 << fdctrl->fifo[5]) *
(cur_drv->last_sect - ks + 1), fdctrl->data_len);
-switch (direction) {
-case FD_DIR_SCANE:
-case FD_DIR_SCANL:
-case FD_DIR_SCANH:
-dma_mode_ok = (dma_mode == ISADMA_TRANSFER_VERIFY);
-break;
-case FD_DIR_WRITE:
-dma_mode_ok = (dma_mode == ISADMA_TRANSFER_WRITE);
-break;
-case FD_DIR_READ:
-dma_mode_ok = (dma_mode == ISADMA_TRANSFER_READ);
-break;
-case FD_DIR_VERIFY:
-dma_mode_ok = true;
-break;
-default:
-dma_mode_ok = false;
-break;
-}
-if (dma_mode_ok) {
-/* No access is allowed until DMA transfer has completed */
-fdctrl->msr &= ~FD_MSR_RQM;
-if (direction != FD_DIR_VERIFY) {
-/* Now, we just have to wait for the DMA controller to
- * recall us...
- */
-k->hold_DREQ(fdctrl->dma, fdctrl->dma_chann);
-k->schedule(fdctrl->dma);
-} else {
-/* Start transfer */
-fdctrl_transfer_handler(fdctrl, fdctrl->dma_chann, 0,
-fdctrl->data_len);
-}
-return;
+
+/* No access is allowed until DMA transfer has completed */
+fdctrl->msr &= ~FD_MSR_RQM;
+if (direction != FD_DIR_VERIFY) {
+/*
+ * Now, we just have to wait for the DMA controller to
+ * recall us...
+ */
+k->hold_DREQ(fdctrl->dma, fdctrl->dma_chann);
+k->schedule(fdctrl->dma);
 } else {
-FLOPPY_DPRINTF("bad dma_mode=%d direction=%d\n", dma_mode,
-   direction);
+/* Start transfer */
+fdctrl_transfer_handler(fdctrl, fdctrl->dma_chann, 0,
+fdctrl->data_len);
 }
+return;
 }
 FLOPPY_DPRINTF("start non-DMA transfer\n");
 fdctrl->msr |= FD_MSR_NONDMA | FD_MSR_RQM;
diff --git a/hw/dma/i8257.c b/hw/dma/i8257.c
index ef15c06d77..1b3435ab58 100644
--- a/hw/dma/i8257.c
+++ b/hw/dma/i8257.c
@@ -292,12 +292,6 @@ static uint64_t i8257_read_cont(void *opaque, hwaddr 
nport, unsigned size)
 return val;
 }
 
-static IsaDmaTransferMode i8257_dma_get_transfer_mode(Is

[PULL 2/2] hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs()

[John Snow]

From: Peter Maydell 

Coverity points out (CID 1421984) that we are leaking the
memory returned by qemu_allocate_irqs(). We can avoid this
leak by switching to using qdev_init_gpio_in(); the base
class finalize will free the irqs that this allocates under
the hood.

Signed-off-by: Peter Maydell 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: John Snow 
Tested-by: BALATON Zoltan 
Message-id: 20200323151715.29454-1-peter.mayd...@linaro.org
[Maintainer edit: replace `DEVICE(dev)` by `ds` --js]
Signed-off-by: John Snow 
---
 hw/ide/sii3112.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 06605d7af2..d69079c3d9 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -251,8 +251,8 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
 {
 SiI3112PCIState *d = SII3112_PCI(dev);
 PCIIDEState *s = PCI_IDE(dev);
+DeviceState *ds = DEVICE(dev);
 MemoryRegion *mr;
-qemu_irq *irq;
 int i;
 
 pci_config_set_interrupt_pin(dev->config, 1);
@@ -280,10 +280,10 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
 memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", &d->mmio, 0, 16);
 pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr);
 
-irq = qemu_allocate_irqs(sii3112_set_irq, d, 2);
+qdev_init_gpio_in(ds, sii3112_set_irq, 2);
 for (i = 0; i < 2; i++) {
-ide_bus_new(&s->bus[i], sizeof(s->bus[i]), DEVICE(dev), i, 1);
-ide_init2(&s->bus[i], irq[i]);
+ide_bus_new(&s->bus[i], sizeof(s->bus[i]), ds, i, 1);
+ide_init2(&s->bus[i], qdev_get_gpio_in(ds, i));
 
 bmdma_init(&s->bus[i], &s->bmdma[i], s);
 s->bmdma[i].bus = &s->bus[i];
-- 
2.21.1

Re: block stream and bitmaps

[Vladimir Sementsov-Ogievskiy]

24.03.2020 22:19, John Snow wrote:



On 3/24/20 6:18 AM, Kevin Wolf wrote:

Am 23.03.2020 um 19:06 hat John Snow geschrieben:

Hi Kevin,

I'm hoping to get some preliminary ideas from you (capped at five
minutes' effort?) on this subject. My ideas here are a bit shaky, I only
have really rough notions here.

We want to use bitmaps with 'drive' semantics; i.e. tracking changes to
the visible guest data. What we have are bitmaps with node semantics,
tracking changes to the data at a particular level in the graph.

For commit, this isn't a big deal: we can disable bitmaps in the backing
file while we commit and then re-enable it on completion. We usually
have a separate bitmap enabled on the root node that is recording writes
during this time, and can be moved later.

For streaming, this is more challenging: new writes will dirty the
bitmap, but so will writes from the stream job itself.

Semantically, we want to ignore writes from the stream while recording
them from the guest -- differentiating based on source.


No, based on source is actually not what you want. What you really want
is that BDRV_REQ_WRITE_UNCHANGED doesn't mark any blocks dirty.



This is why I sent the mail, I figured you'd know the better incision
point, and I was right!


We discussed this specific case of streaming at FOSDEM (with Paolo and
probably Nir). Paolo was even convinced that unchanged writes already
behave like this, but we agreed that dirtying blocks for them would be a
bug. After checking that the code is indeed buggy, I was planning to
send a patch, but never got around to actually do that. Sorry about
that.



Glad to hear it has been given consideration, though!


Bitmaps aren't really geared to do that right now. With the changes to
Bdrv Roles that Max was engineering, do you think it's possible to add
some kind of write source discrimination to bitmaps, or is that too messy?


I don't think it would work because copy-on-read requests come from the
same parent node as writes (no matter whether the legacy code in
block/io.c or a copy-on-read filter node is used).



Oh, understood. Rule that approach out, then.


For both commit and stream, it might be nice to say: "This bitmap is
enabled, but ignores writes from [all? specific types? specific
instances?] jobs.


Commit is a bit trickier, because it's not WRITE_UNCHANGED. The result
is only unchanged for the top layer, but not for the backing file you're
committing to. Not sure if we can represent this condition somehow.



Nothing comes to mind apart from a semantic that applies to a graph
subsection instead of an individual node.

i.e. UNCHANGED as applied to [A --> B].

Not saying that's reasonable to develop... or necessarily even possible
to enforce, just nothing else comes to mind.


Or, I wonder if what we truly want is some kind of bitmap "forwarder"
object on block-backend objects that represent the semantic drive view,
and only writes through that *backend* get forwarded to the bitmaps
attached to whatever node the bitmap is actually associated with.

(That might wind up causing weird problems too, though... since those
objects are no longer intended to be user-addressable, managing that
configuration might get intensely strange.)


Hm... Drive-based does suggest that it's managed at the BlockBackend
level. So having a bitmap that isn't added as a dirty bitmap to the BDS,
but only to the BB does make sense to me. The BB would be addressed
with the qdev ID of the device, as usual (which underlines that it's
really per device).



That's the rough idea, though if it's needed or not is unclear. We might
be able to get by with node semantics if we jazz them up enough...?

Working around all the edge bases of a drive-semantic bitmap seem
difficult to reason about.

In general, it should likely be made persistent against the root-most
node to which writes are routed to a protocol node.
In the common case, that means the top-most qcow2 format node of a chain.

But, many of our job filters also route writes to format nodes too, so
which one is the "canonical" store? Is it even possible to define?

Hm.

I suppose we can always allow bitmaps being attached to the BB and
*disallow them* from being made persistent; but simply exist as an
in-memory tool to help ease the pain of managing data consistency during
critical sections.

We can offer merge-to, merge-from semantics for these in-memory-only
bitmaps. Maybe.

Still mulling it over.


We can just add "empty" filter node, and move bitmaps to it and than back
again. If I understand correctly, this gives same semantics like with
moving to BB, but without any additional BB logic. We just need "do-nothing"
filter driver for it.




The part that's unclear to me is how to make such bitmaps persistent.
You can change the root node of a BB and even remove the root node
completely (for removable devices; but even changing is technically
remove followed by insert), so you may need to move the bitmap around
between image files 

Re: [PATCH 6/6] qga/commands-posix: fix use after free of local_err

[Eric Blake]

On 3/24/20 10:36 AM, Vladimir Sementsov-Ogievskiy wrote:

local_err is used several times in guest_suspend(). Setting non-NULL
local_err will crash, so let's zero it after freeing. Also fix possible
leak of local_err in final if().

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  qga/commands-posix.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 93474ff770..cc69b82704 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1773,6 +1773,7 @@ static void guest_suspend(SuspendMode mode, Error **errp)
  }
  
  error_free(local_err);

+local_err = NULL;


Let's show this with more context.


static void guest_suspend(SuspendMode mode, Error **errp)
{
Error *local_err = NULL;
bool mode_supported = false;

if (systemd_supports_mode(mode, &local_err)) {


Hmm - we have an even earlier bug that needs fixing.  Note that 
systemd_supports_mode() returns a bool AND conditionally sets errp.  But 
it is inconsistent: it has the following table of actions based on the 
results of run_process_child() on "systemctl status" coupled with the 
man page on "systemctl status" return values:

-1 (unable to run systemctl) -> errp set, return false
0 (unit is active) -> errp left unchanged, return false
1 (unit not failed) -> errp left unchanged, return true
2 (unused) -> errp left unchanged, return true
3 (unit not active) -> errp left unchanged, return true
4 (no such unit) -> errp left unchanged, return false
5+ (unexpected from systemctl) -> errp left unchanged, return false

But the comments in systemd_supports_mode() claim that ANY status < 4 
(other than -1, which means we did not run systemctl) should count as 
the service existing, even though the most common status is 3.  If our 
comment is to be believed, then we should return true, not false, for 
status 0.


Now, back to _this_ function:


mode_supported = true;
systemd_suspend(mode, &local_err);


Okay - if we get here (whether from status 1-3, or with 
systemd_supports_mode fixed to support status 0-3), local_err is still 
unset prior to calling systemd_suspend(), and we are guaranteed that 
after the call, either we suspended successfully or local_err is now set.



}

if (!local_err) {
return;
}


So if returned, we succeeded at systemd_suspend, and there is nothing 
further to do; but if we get past that point, we don't know if it was 
systemd_supports_mode that failed or systemd_suspend that failed, and we 
don't know if local_err is set.




error_free(local_err);
+local_err = NULL;


Yet, we blindly throw away local_err, without trying to report it.  If 
that's the case, then WHY are we passing in local_err?  Wouldn't it be 
better to pass in NULL (we really don't care about the error message), 
and/or fix systemd_suspend() to return a bool just like 
systemd_supports_mode, and/or fix systemd_supports_mode to guarantee 
that it sets errp when returning false?


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

Re: [PATCH] hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs()

[Mark Cave-Ayland]

On 23/03/2020 15:17, Peter Maydell wrote:

> Coverity points out (CID 1421984) that we are leaking the
> memory returned by qemu_allocate_irqs(). We can avoid this
> leak by switching to using qdev_init_gpio_in(); the base
> class finalize will free the irqs that this allocates under
> the hood.
> 
> Signed-off-by: Peter Maydell 
> ---
> This is how the 'use qdev gpio' approach to fixing the leak looks.
> Disclaimer: I have only tested this with "make check", nothing more.
> 
>  hw/ide/sii3112.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
> index 06605d7af2b..2ae6f5d9df6 100644
> --- a/hw/ide/sii3112.c
> +++ b/hw/ide/sii3112.c
> @@ -251,8 +251,8 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
> **errp)
>  {
>  SiI3112PCIState *d = SII3112_PCI(dev);
>  PCIIDEState *s = PCI_IDE(dev);
> +DeviceState *ds = DEVICE(dev);
>  MemoryRegion *mr;
> -qemu_irq *irq;
>  int i;
>  
>  pci_config_set_interrupt_pin(dev->config, 1);
> @@ -280,10 +280,10 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
> **errp)
>  memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", &d->mmio, 0, 16);
>  pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr);
>  
> -irq = qemu_allocate_irqs(sii3112_set_irq, d, 2);
> +qdev_init_gpio_in(ds, sii3112_set_irq, 2);
>  for (i = 0; i < 2; i++) {
>  ide_bus_new(&s->bus[i], sizeof(s->bus[i]), DEVICE(dev), i, 1);
> -ide_init2(&s->bus[i], irq[i]);
> +ide_init2(&s->bus[i], qdev_get_gpio_in(ds, i));
>  
>  bmdma_init(&s->bus[i], &s->bmdma[i], s);
>  s->bmdma[i].bus = &s->bus[i];

Looks like there is similar use of qemu_allocate_irqs() in via-ide and 
cmd646-ide,
and also reviewing my latest via-ide changes I spotted a silly mistake which was
obviously left in from a previous experimental version.

I'm not sure why Coverity doesn't pick up these other occurrences, however I'll 
send
along a patchset for this shortly.


ATB,

Mark.

[PATCH for-5.0 2/3] via-ide: use qdev gpio rather than qemu_allocate_irqs()

[Mark Cave-Ayland]

This prevents the memory from qemu_allocate_irqs() from being leaked which
can in some cases be spotted by Coverity (CID 1421984).

Signed-off-by: Mark Cave-Ayland 
---
 hw/ide/via.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index 2a55b7fbc6..be09912b33 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -160,6 +160,7 @@ static void via_ide_reset(DeviceState *dev)
 static void via_ide_realize(PCIDevice *dev, Error **errp)
 {
 PCIIDEState *d = PCI_IDE(dev);
+DeviceState *ds = DEVICE(dev);
 uint8_t *pci_conf = dev->config;
 int i;
 
@@ -187,9 +188,10 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
 bmdma_setup_bar(d);
 pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
 
+qdev_init_gpio_in(ds, via_ide_set_irq, 2);
 for (i = 0; i < 2; i++) {
-ide_bus_new(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
-ide_init2(&d->bus[i], qemu_allocate_irq(via_ide_set_irq, d, i));
+ide_bus_new(&d->bus[i], sizeof(d->bus[i]), ds, i, 2);
+ide_init2(&d->bus[i], qdev_get_gpio_in(ds, i));
 
 bmdma_init(&d->bus[i], &d->bmdma[i], d);
 d->bmdma[i].bus = &d->bus[i];
-- 
2.20.1

Re: [PATCH] hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs()

[John Snow]

On 3/24/20 4:43 PM, Mark Cave-Ayland wrote:
> On 23/03/2020 15:17, Peter Maydell wrote:
> 
>> Coverity points out (CID 1421984) that we are leaking the
>> memory returned by qemu_allocate_irqs(). We can avoid this
>> leak by switching to using qdev_init_gpio_in(); the base
>> class finalize will free the irqs that this allocates under
>> the hood.
>>
>> Signed-off-by: Peter Maydell 
>> ---
>> This is how the 'use qdev gpio' approach to fixing the leak looks.
>> Disclaimer: I have only tested this with "make check", nothing more.
>>
>>  hw/ide/sii3112.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
>> index 06605d7af2b..2ae6f5d9df6 100644
>> --- a/hw/ide/sii3112.c
>> +++ b/hw/ide/sii3112.c
>> @@ -251,8 +251,8 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
>> **errp)
>>  {
>>  SiI3112PCIState *d = SII3112_PCI(dev);
>>  PCIIDEState *s = PCI_IDE(dev);
>> +DeviceState *ds = DEVICE(dev);
>>  MemoryRegion *mr;
>> -qemu_irq *irq;
>>  int i;
>>  
>>  pci_config_set_interrupt_pin(dev->config, 1);
>> @@ -280,10 +280,10 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
>> **errp)
>>  memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", &d->mmio, 0, 
>> 16);
>>  pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr);
>>  
>> -irq = qemu_allocate_irqs(sii3112_set_irq, d, 2);
>> +qdev_init_gpio_in(ds, sii3112_set_irq, 2);
>>  for (i = 0; i < 2; i++) {
>>  ide_bus_new(&s->bus[i], sizeof(s->bus[i]), DEVICE(dev), i, 1);
>> -ide_init2(&s->bus[i], irq[i]);
>> +ide_init2(&s->bus[i], qdev_get_gpio_in(ds, i));
>>  
>>  bmdma_init(&s->bus[i], &s->bmdma[i], s);
>>  s->bmdma[i].bus = &s->bus[i];
> 
> Looks like there is similar use of qemu_allocate_irqs() in via-ide and 
> cmd646-ide,
> and also reviewing my latest via-ide changes I spotted a silly mistake which 
> was
> obviously left in from a previous experimental version.
> 
> I'm not sure why Coverity doesn't pick up these other occurrences, however 
> I'll send
> along a patchset for this shortly.
> 

OK;

I will rescind my PR and will re-send it with your patches included.

--js

[PATCH for-5.0 1/3] via-ide: don't use PCI level for legacy IRQs

[Mark Cave-Ayland]

The PCI level calculation was accidentally left in when rebasing from a
previous patchset. Since both IRQs are driven separately, the value
being passed into the IRQ handler should be used directly.

Signed-off-by: Mark Cave-Ayland 
---
 hw/ide/via.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index 8de4945cc1..2a55b7fbc6 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -112,7 +112,6 @@ static void via_ide_set_irq(void *opaque, int n, int level)
 d->config[0x70 + n * 8] &= ~0x80;
 }
 
-level = (d->config[0x70] & 0x80) || (d->config[0x78] & 0x80);
 qemu_set_irq(isa_get_irq(NULL, 14 + n), level);
 }
 
-- 
2.20.1

[PATCH for-5.0 0/3] ide: fix potential memory leaks (plus one via-ide bugfix)

[Mark Cave-Ayland]

This was supposed to be a simple patchset to switch via-ide and cmd646-ide
over to use qdev gpio in the same way as Peter's patch did for sil3112, but
at the same time I spotted a silly mistake in my last set of via-ide
patches which is included as patch 1.

I'm not sure exactly why Coverity CID 1421984 isn't triggered by the
via-ide and cmd646-ide devices, however given the simplicity of the fix it
seems worth doing just to keep everything the same and ensure it won't
suddenly appear in future.

The via-ide changes were tested using the instructions provided by Zoltan
for MIPS fulong2e and PPC pegasos2, whilst the cmd646 change was tested
using one of my SPARC64 Linux test images.

Signed-off-by: Mark Cave-Ayland 


Mark Cave-Ayland (3):
  via-ide: don't use PCI level for legacy IRQs
  via-ide: use qdev gpio rather than qemu_allocate_irqs()
  cmd646-ide: use qdev gpio rather than qemu_allocate_irqs()

 hw/ide/cmd646.c | 9 -
 hw/ide/via.c| 7 ---
 2 files changed, 8 insertions(+), 8 deletions(-)

-- 
2.20.1

[PATCH for-5.0 3/3] cmd646-ide: use qdev gpio rather than qemu_allocate_irqs()

[Mark Cave-Ayland]

This prevents the memory from qemu_allocate_irqs() from being leaked which
can in some cases be spotted by Coverity (CID 1421984).

Signed-off-by: Mark Cave-Ayland 
---
 hw/ide/cmd646.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index 699f25824d..c254631485 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -249,8 +249,8 @@ static void cmd646_pci_config_write(PCIDevice *d, uint32_t 
addr, uint32_t val,
 static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
 {
 PCIIDEState *d = PCI_IDE(dev);
+DeviceState *ds = DEVICE(dev);
 uint8_t *pci_conf = dev->config;
-qemu_irq *irq;
 int i;
 
 pci_conf[PCI_CLASS_PROG] = 0x8f;
@@ -291,16 +291,15 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error 
**errp)
 /* TODO: RST# value should be 0 */
 pci_conf[PCI_INTERRUPT_PIN] = 0x01; // interrupt on pin 1
 
-irq = qemu_allocate_irqs(cmd646_set_irq, d, 2);
+qdev_init_gpio_in(ds, cmd646_set_irq, 2);
 for (i = 0; i < 2; i++) {
-ide_bus_new(&d->bus[i], sizeof(d->bus[i]), DEVICE(dev), i, 2);
-ide_init2(&d->bus[i], irq[i]);
+ide_bus_new(&d->bus[i], sizeof(d->bus[i]), ds, i, 2);
+ide_init2(&d->bus[i], qdev_get_gpio_in(ds, i));
 
 bmdma_init(&d->bus[i], &d->bmdma[i], d);
 d->bmdma[i].bus = &d->bus[i];
 ide_register_restart_cb(&d->bus[i]);
 }
-g_free(irq);
 }
 
 static void pci_cmd646_ide_exitfn(PCIDevice *dev)
-- 
2.20.1

Re: [PULL 0/2] Ide patches

[John Snow]

On 3/24/20 3:55 PM, John Snow wrote:
> The following changes since commit 736cf607e40674776d752acc201f565723e86045:
> 
>   Update version for v5.0.0-rc0 release (2020-03-24 17:50:00 +)
> 
> are available in the Git repository at:
> 
>   https://github.com/jnsnow/qemu.git tags/ide-pull-request
> 
> for you to fetch changes up to 51058b3b3bcbe62506cf191fca1c0d679bb80f2b:
> 
>   hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs() (2020-03-24 
> 15:52:16 -0400)
> 
> 
> Pull request: IDE
> 
> Admittedly the first one is not a crisis fix; but I think it's low-risk to
> include for rc1.
> 
> The second one is yours, and will shush coverity.
> 
> 
> 
> Peter Maydell (1):
>   hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs()
> 
> Sven Schnelle (1):
>   fdc/i8257: implement verify transfer mode
> 
>  include/hw/isa/isa.h |  1 -
>  hw/block/fdc.c   | 61 +---
>  hw/dma/i8257.c   | 20 ++-
>  hw/ide/sii3112.c |  8 +++---
>  4 files changed, 35 insertions(+), 55 deletions(-)
> 

NACK. Mark Cave-Ayland is sending additional fixes.

--js

[PATCH v9 00/14] iotests: use python logging

[John Snow]

This series uses python logging to enable output conditionally on
iotests.log(). We unify an initialization call (which also enables
debugging output for those tests with -d) and then make the switch
inside of iotests.

It will help alleviate the need to create logged/unlogged versions
of all the various helpers we have made.

Also, I got lost and accidentally delinted iotests while I was here.
Sorry about that. By version 9, it's now the overwhelming focus of
this series. No good deed, etc.

V9:

001/14:[] [-C] 'iotests: do a light delinting'
002/14:[] [--] 'iotests: don't use 'format' for drive_add'
003/14:[] [-C] 'iotests: ignore import warnings from pylint'
004/14:[] [--] 'iotests: replace mutable list default args'
005/14:[] [--] 'iotests: add pylintrc file'
006/14:[down]  'iotests: alphabetize standard imports'
007/14:[down]  'iotests: drop pre-Python 3.4 compatibility code'
008/14:[down]  'iotests: touch up log function signature'
009/14:[] [--] 'iotests: limit line length to 79 chars'
010/14:[down]  'iotests: add hmp helper with logging'
011/14:[0004] [FC] 'iotests: add script_initialize'
012/14:[] [--] 'iotest 258: use script_main'
013/14:[] [--] 'iotests: Mark verify functions as private'
014/14:[0001] [FC] 'iotests: use python logging for iotests.log()'

006: New.
007: Split from old patch.
008: Split from old patch; enhanced a little to justify its own patch.
010: New, pulled in from bitmap-populate series. Helps line length.
011: Reflow columns for long `typing` import list. (Kept RB.)
014: New blank line. (Kept RB.)

V8:
- Split out the little drop of Python 3.4 code. (Phil)
- Change line continuation styles (QEMU Memorial Choir)
- Rebase changes; remove use_log from more places, adjust test output.

V7:
- All delinting patches are now entirely front-loaded.
- Redid delinting to avoid "correcting" no-else-return statements.
- Moved more mutable list corrections into patch 4, to make it standalone.
- Moved pylintrc up to patch 5. Disabled no-else-return.
- Added patch 6 to require line length checks.
  (Some python 3.4 compatibility code is removed as a consequence.)
- Patch 7 changes slightly as a result of patch 4 changes.
- Added some logging explainer into patch 10.
  (Patch changes slightly because of patch 6.)

V6:
 - It's been so long since V5, let's just look at it anew.

John Snow (14):
  iotests: do a light delinting
  iotests: don't use 'format' for drive_add
  iotests: ignore import warnings from pylint
  iotests: replace mutable list default args
  iotests: add pylintrc file
  iotests: alphabetize standard imports
  iotests: drop pre-Python 3.4 compatibility code
  iotests: touch up log function signature
  iotests: limit line length to 79 chars
  iotests: add hmp helper with logging
  iotests: add script_initialize
  iotest 258: use script_main
  iotests: Mark verify functions as private
  iotests: use python logging for iotests.log()

 tests/qemu-iotests/030|   4 +-
 tests/qemu-iotests/055|   3 +-
 tests/qemu-iotests/149|   3 +-
 tests/qemu-iotests/155|   2 +-
 tests/qemu-iotests/194|   4 +-
 tests/qemu-iotests/202|   4 +-
 tests/qemu-iotests/203|   4 +-
 tests/qemu-iotests/206|   2 +-
 tests/qemu-iotests/207|   6 +-
 tests/qemu-iotests/208|   2 +-
 tests/qemu-iotests/209|   2 +-
 tests/qemu-iotests/210|   6 +-
 tests/qemu-iotests/211|   6 +-
 tests/qemu-iotests/212|   6 +-
 tests/qemu-iotests/213|   6 +-
 tests/qemu-iotests/216|   4 +-
 tests/qemu-iotests/218|   2 +-
 tests/qemu-iotests/219|   2 +-
 tests/qemu-iotests/222|   7 +-
 tests/qemu-iotests/224|   4 +-
 tests/qemu-iotests/228|   6 +-
 tests/qemu-iotests/234|   4 +-
 tests/qemu-iotests/235|   4 +-
 tests/qemu-iotests/236|   2 +-
 tests/qemu-iotests/237|   2 +-
 tests/qemu-iotests/238|   2 +
 tests/qemu-iotests/242|   2 +-
 tests/qemu-iotests/245|   1 +
 tests/qemu-iotests/245.out|  24 +--
 tests/qemu-iotests/246|   2 +-
 tests/qemu-iotests/248|   2 +-
 tests/qemu-iotests/254|   2 +-
 tests/qemu-iotests/255|   2 +-
 tests/qemu-iotests/256|   2 +-
 tests/qemu-iotests/258|  10 +-
 tests/qemu-iotests/260|   4 +-
 tests/qemu-iotests/262|   4 +-
 tests/qemu-iotests/264|   4 +-
 tests/qemu-iotests/277|   2 +
 tests/qemu-iotests/280|   8 +-
 tests/qemu-iotests/283|   4 +-
 tests/qemu-iotests/iotests.py | 356 --
 tests/qemu-iotests/pylintrc   |  26 +++
 43 files changed, 333 insertions(+), 221 deletions(-)
 create mode 100644 tests/qemu-iotests/pylintrc

-- 
2.21.1

[PATCH v9 01/14] iotests: do a light delinting

[John Snow]

This doesn't fix everything in here, but it does help clean up the
pylint report considerably.

This should be 100% style changes only; the intent is to make pylint
more useful by working on establishing a baseline for iotests that we
can gate against in the future.

Signed-off-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/iotests.py | 83 ++-
 1 file changed, 43 insertions(+), 40 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 7bc4934cd2..886ae962ae 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -16,11 +16,9 @@
 # along with this program.  If not, see .
 #
 
-import errno
 import os
 import re
 import subprocess
-import string
 import unittest
 import sys
 import struct
@@ -35,7 +33,7 @@
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
 from qemu import qtest
 
-assert sys.version_info >= (3,6)
+assert sys.version_info >= (3, 6)
 
 faulthandler.enable()
 
@@ -141,11 +139,11 @@ def qemu_img_log(*args):
 return result
 
 def img_info_log(filename, filter_path=None, imgopts=False, extra_args=[]):
-args = [ 'info' ]
+args = ['info']
 if imgopts:
 args.append('--image-opts')
 else:
-args += [ '-f', imgfmt ]
+args += ['-f', imgfmt]
 args += extra_args
 args.append(filename)
 
@@ -224,7 +222,7 @@ def cmd(self, cmd):
 # quit command is in close(), '\n' is added automatically
 assert '\n' not in cmd
 cmd = cmd.strip()
-assert cmd != 'q' and cmd != 'quit'
+assert cmd not in ('q', 'quit')
 self._p.stdin.write(cmd + '\n')
 self._p.stdin.flush()
 return self._read_output()
@@ -246,10 +244,8 @@ def qemu_nbd_early_pipe(*args):
 sys.stderr.write('qemu-nbd received signal %i: %s\n' %
  (-exitcode,
   ' '.join(qemu_nbd_args + ['--fork'] + list(args
-if exitcode == 0:
-return exitcode, ''
-else:
-return exitcode, subp.communicate()[0]
+
+return exitcode, subp.communicate()[0] if exitcode else ''
 
 def qemu_nbd_popen(*args):
 '''Run qemu-nbd in daemon mode and return the parent's exit code'''
@@ -313,7 +309,7 @@ def filter_qmp(qmsg, filter_fn):
 items = qmsg.items()
 
 for k, v in items:
-if isinstance(v, list) or isinstance(v, dict):
+if isinstance(v, (dict, list)):
 qmsg[k] = filter_qmp(v, filter_fn)
 else:
 qmsg[k] = filter_fn(k, v)
@@ -324,7 +320,7 @@ def filter_testfiles(msg):
 return msg.replace(prefix, 'TEST_DIR/PID-')
 
 def filter_qmp_testfiles(qmsg):
-def _filter(key, value):
+def _filter(_key, value):
 if is_str(value):
 return filter_testfiles(value)
 return value
@@ -350,7 +346,7 @@ def filter_imgfmt(msg):
 return msg.replace(imgfmt, 'IMGFMT')
 
 def filter_qmp_imgfmt(qmsg):
-def _filter(key, value):
+def _filter(_key, value):
 if is_str(value):
 return filter_imgfmt(value)
 return value
@@ -361,7 +357,7 @@ def log(msg, filters=[], indent=None):
 If indent is provided, JSON serializable messages are pretty-printed.'''
 for flt in filters:
 msg = flt(msg)
-if isinstance(msg, dict) or isinstance(msg, list):
+if isinstance(msg, (dict, list)):
 # Python < 3.4 needs to know not to add whitespace when 
pretty-printing:
 separators = (', ', ': ') if indent is None else (',', ': ')
 # Don't sort if it's already sorted
@@ -372,14 +368,14 @@ def log(msg, filters=[], indent=None):
 print(msg)
 
 class Timeout:
-def __init__(self, seconds, errmsg = "Timeout"):
+def __init__(self, seconds, errmsg="Timeout"):
 self.seconds = seconds
 self.errmsg = errmsg
 def __enter__(self):
 signal.signal(signal.SIGALRM, self.timeout)
 signal.setitimer(signal.ITIMER_REAL, self.seconds)
 return self
-def __exit__(self, type, value, traceback):
+def __exit__(self, exc_type, value, traceback):
 signal.setitimer(signal.ITIMER_REAL, 0)
 return False
 def timeout(self, signum, frame):
@@ -388,7 +384,7 @@ def timeout(self, signum, frame):
 def file_pattern(name):
 return "{0}-{1}".format(os.getpid(), name)
 
-class FilePaths(object):
+class FilePaths:
 """
 FilePaths is an auto-generated filename that cleans itself up.
 
@@ -535,11 +531,11 @@ def pause_drive(self, drive, event=None):
 self.pause_drive(drive, "write_aio")
 return
 self.qmp('human-monitor-command',
-command_line='qemu-io %s "break %s bp_%s"' % (drive, 
event, drive))
+ command_line='qemu-io %s "break %s bp_%s"' % (drive, event, 
drive))
 
 def resume_drive(self, drive):
 self.qmp('human-monito

[PATCH v9 03/14] iotests: ignore import warnings from pylint

[John Snow]

The right way to solve this is to come up with a virtual environment
infrastructure that sets all the paths correctly, and/or to create
installable python modules that can be imported normally.

That's hard, so just silence this error for now.

Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Max Reitz 
Signed-off-by: John Snow 
---
 tests/qemu-iotests/iotests.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 7f486e6c4b..0eccca88e0 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -30,6 +30,7 @@
 from collections import OrderedDict
 import faulthandler
 
+# pylint: disable=import-error, wrong-import-position
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
 from qemu import qtest
 
-- 
2.21.1

[PATCH v9 08/14] iotests: touch up log function signature

[John Snow]

Representing nested, recursive data structures in mypy is notoriously
difficult; the best we can reliably do right now is denote the atom
types as "Any" while describing the general shape of the data.

Regardless, this fully annotates the log() function.

Typing notes:

TypeVar is a Type variable that can optionally be constrained by a
sequence of possible types. This variable is bound per-invocation such
that the signature for filter=() requires that its callables take e.g. a
str and return a str.

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

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index c93c6b4557..3a049ece5b 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -28,6 +28,7 @@
 import struct
 import subprocess
 import sys
+from typing import (Any, Callable, Dict, Iterable, List, Optional, TypeVar)
 import unittest
 
 # pylint: disable=import-error, wrong-import-position
@@ -353,9 +354,16 @@ def _filter(_key, value):
 return value
 return filter_qmp(qmsg, _filter)
 
-def log(msg, filters=(), indent=None):
-'''Logs either a string message or a JSON serializable message (like QMP).
-If indent is provided, JSON serializable messages are pretty-printed.'''
+
+Msg = TypeVar('Msg', Dict[str, Any], List[Any], str)
+
+def log(msg: Msg,
+filters: Iterable[Callable[[Msg], Msg]] = (),
+indent: Optional[int] = None) -> None:
+"""
+Logs either a string message or a JSON serializable message (like QMP).
+If indent is provided, JSON serializable messages are pretty-printed.
+"""
 for flt in filters:
 msg = flt(msg)
 if isinstance(msg, (dict, list)):
-- 
2.21.1

[PATCH v9 02/14] iotests: don't use 'format' for drive_add

[John Snow]

It shadows (with a different type) the built-in format.
Use something else.

Signed-off-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/055| 3 ++-
 tests/qemu-iotests/iotests.py | 6 +++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index 82b9f5f47d..4175fff5e4 100755
--- a/tests/qemu-iotests/055
+++ b/tests/qemu-iotests/055
@@ -469,7 +469,8 @@ class TestDriveCompression(iotests.QMPTestCase):
 qemu_img('create', '-f', fmt, blockdev_target_img,
  str(TestDriveCompression.image_len), *args)
 if attach_target:
-self.vm.add_drive(blockdev_target_img, format=fmt, 
interface="none")
+self.vm.add_drive(blockdev_target_img,
+  img_format=fmt, interface="none")
 
 self.vm.launch()
 
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 886ae962ae..7f486e6c4b 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -485,21 +485,21 @@ def add_drive_raw(self, opts):
 self._args.append(opts)
 return self
 
-def add_drive(self, path, opts='', interface='virtio', format=imgfmt):
+def add_drive(self, path, opts='', interface='virtio', img_format=imgfmt):
 '''Add a virtio-blk drive to the VM'''
 options = ['if=%s' % interface,
'id=drive%d' % self._num_drives]
 
 if path is not None:
 options.append('file=%s' % path)
-options.append('format=%s' % format)
+options.append('format=%s' % img_format)
 options.append('cache=%s' % cachemode)
 options.append('aio=%s' % aiomode)
 
 if opts:
 options.append(opts)
 
-if format == 'luks' and 'key-secret' not in opts:
+if img_format == 'luks' and 'key-secret' not in opts:
 # default luks support
 if luks_default_secret_object not in self._args:
 self.add_object(luks_default_secret_object)
-- 
2.21.1

[PATCH v9 05/14] iotests: add pylintrc file

[John Snow]

This allows others to get repeatable results with pylint. If you run
`pylint iotests.py`, you should see a 100% pass.

Signed-off-by: John Snow 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/pylintrc | 22 ++
 1 file changed, 22 insertions(+)
 create mode 100644 tests/qemu-iotests/pylintrc

diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc
new file mode 100644
index 00..8720b6a0de
--- /dev/null
+++ b/tests/qemu-iotests/pylintrc
@@ -0,0 +1,22 @@
+[MESSAGES CONTROL]
+
+# Disable the message, report, category or checker with the given id(s). You
+# can either give multiple identifiers separated by comma (,) or put this
+# option multiple times (only on the command line, not in the configuration
+# file where it should appear only once). You can also use "--disable=all" to
+# disable everything first and then reenable specific checks. For example, if
+# you want to run only the similarities checker, you can use "--disable=all
+# --enable=similarities". If you want to run only the classes checker, but have
+# no Warning level messages displayed, use "--disable=all --enable=classes
+# --disable=W".
+disable=invalid-name,
+no-else-return,
+too-many-lines,
+too-few-public-methods,
+too-many-arguments,
+too-many-locals,
+too-many-branches,
+too-many-public-methods,
+# These are temporary, and should be removed:
+missing-docstring,
+line-too-long,
-- 
2.21.1

[PATCH v9 06/14] iotests: alphabetize standard imports

[John Snow]

I had to fix a merge conflict, so do this tiny harmless thing while I'm
here.

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

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 20da488ad6..2a0e22a3db 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -16,19 +16,19 @@
 # along with this program.  If not, see .
 #
 
+import atexit
+from collections import OrderedDict
+import faulthandler
+import io
+import json
+import logging
 import os
 import re
+import signal
+import struct
 import subprocess
-import unittest
 import sys
-import struct
-import json
-import signal
-import logging
-import atexit
-import io
-from collections import OrderedDict
-import faulthandler
+import unittest
 
 # pylint: disable=import-error, wrong-import-position
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-- 
2.21.1