Re: [PATCH v4 02/11] monitor/hmp: uninline add_init_drive

[Maxim Levitsky]

On Tue, 2020-03-03 at 18:10 +0100, Kevin Wolf wrote:
> Am 30.01.2020 um 13:34 hat Maxim Levitsky geschrieben:
> > This is only used by hmp_drive_add.
> > The code is just a bit shorter this way.
> > 
> > No functional changes
> > 
> > Signed-off-by: Maxim Levitsky 
> > Reviewed-by: Markus Armbruster 
> 
> Shouldn't the subject say "inline" rather than "uninline"?
> 
> Kevin

Oh, you are absolutely correct. I don't know why I even now thought
about this that way.

Best regards,
Maxim Levitsky

Re: [PATCH v4 07/11] monitor/hmp: move hmp_snapshot_* to block-hmp-cmds.c hmp_snapshot_blkdev is from GPLv2 version of the hmp-cmds.c thus have to change the licence to GPLv2

[Maxim Levitsky]

On Tue, 2020-03-03 at 18:15 +0100, Kevin Wolf wrote:
> Am 30.01.2020 um 13:34 hat Maxim Levitsky geschrieben:
> > Signed-off-by: Maxim Levitsky 
> > Reviewed-by: Dr. David Alan Gilbert 
> 
> Very long subject line. I suppose the license notice should be in the
> body instead.
> 
> >  block/monitor/block-hmp-cmds.c | 56 --
> >  include/block/block-hmp-cmds.h |  4 +++
> >  include/monitor/hmp.h  |  3 --
> >  monitor/hmp-cmds.c | 47 
> >  4 files changed, 58 insertions(+), 52 deletions(-)
> > 
> > diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
> > index 8e8288c2f1..b83687196f 100644
> > --- a/block/monitor/block-hmp-cmds.c
> > +++ b/block/monitor/block-hmp-cmds.c
> > @@ -1,10 +1,13 @@
> >  /*
> >   * Blockdev HMP commands
> >   *
> > + *  Authors:
> > + *  Anthony Liguori   
> > + *
> >   * Copyright (c) 2003-2008 Fabrice Bellard
> >   *
> > - * This work is licensed under the terms of the GNU GPL, version 2 or
> > - * later.  See the COPYING file in the top-level directory.
> > + * This work is licensed under the terms of the GNU GPL, version 2.
> > + * See the COPYING file in the top-level directory.
> 
> Please also copy the next paragraph of the license header:
> 
>  * Contributions after 2012-01-13 are licensed under the terms of the
>  * GNU GPL, version 2 or (at your option) any later version.
> 
> Kevin
Will do,
Best regards,
Maxim Levitsky

Re: [PATCH 1/2] misc: Replace zero-length arrays with flexible array member (automatic)

[David Hildenbrand]

On 04.03.20 01:51, Philippe Mathieu-Daudé wrote:
> Description copied from Linux kernel commit from Gustavo A. R. Silva
> (see [3]):
> 
> --v-- description start --v--
> 
>   The current codebase makes use of the zero-length array language
>   extension to the C90 standard, but the preferred mechanism to
>   declare variable-length types such as these ones is a flexible
>   array member [1], introduced in C99:
> 
>   struct foo {
>   int stuff;
>   struct boo array[];
>   };
> 
>   By making use of the mechanism above, we will get a compiler
>   warning in case the flexible array does not occur last in the
>   structure, which will help us prevent some kind of undefined
>   behavior bugs from being unadvertenly introduced [2] to the
>   Linux codebase from now on.
> 
> --^-- description end --^--
> 
> Do the similar housekeeping in the QEMU codebase (which uses
> C99 since commit 7be41675f7cb).
> 
> All these instances of code were found with the help of the
> following Coccinelle script:
> 
>   @@
>   identifier s, a;
>   type T;
>   @@
>struct s {
>   ...
>   -   T a[0];
>   +   T a[];
>   };
>   @@
>   identifier s, a;
>   type T;
>   @@
>struct s {
>   ...
>   -   T a[0];
>   +   T a[];
>} QEMU_PACKED;
> 
> [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> [2] 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=76497732932f
> [3] 
> https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?id=17642a2fbd2c1
> 
> Inspired-by: Gustavo A. R. Silva 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  bsd-user/qemu.h   |  2 +-
>  contrib/libvhost-user/libvhost-user.h |  2 +-
>  hw/m68k/bootinfo.h|  2 +-
>  hw/scsi/srp.h |  6 +++---
>  hw/xen/xen_pt.h   |  2 +-
>  include/hw/acpi/acpi-defs.h   | 12 ++--
>  include/hw/arm/smmu-common.h  |  2 +-
>  include/hw/i386/intel_iommu.h |  3 ++-
>  include/hw/virtio/virtio-iommu.h  |  2 +-
>  include/sysemu/cryptodev.h|  2 +-
>  include/tcg/tcg.h |  2 +-
>  pc-bios/s390-ccw/bootmap.h|  2 +-
>  pc-bios/s390-ccw/sclp.h   |  2 +-
>  tests/qtest/libqos/ahci.h |  2 +-
>  block/linux-aio.c |  2 +-
>  hw/acpi/nvdimm.c  |  6 +++---
>  hw/dma/soc_dma.c  |  2 +-
>  hw/i386/x86.c |  2 +-
>  hw/misc/omap_l4.c |  2 +-
>  hw/nvram/eeprom93xx.c |  2 +-
>  hw/rdma/vmw/pvrdma_qp_ops.c   |  4 ++--
>  hw/usb/dev-network.c  |  2 +-
>  hw/usb/dev-smartcard-reader.c |  4 ++--
>  hw/virtio/virtio.c|  4 ++--
>  net/queue.c   |  2 +-
>  25 files changed, 38 insertions(+), 37 deletions(-)
> 
> diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
> index 09e8aed9c7..f8bb1e5459 100644
> --- a/bsd-user/qemu.h
> +++ b/bsd-user/qemu.h
> @@ -95,7 +95,7 @@ typedef struct TaskState {
>  struct sigqueue *first_free; /* first free siginfo queue entry */
>  int signal_pending; /* non zero if a signal may be pending */
>  
> -uint8_t stack[0];
> +uint8_t stack[];
>  } __attribute__((aligned(16))) TaskState;
>  
>  void init_task_state(TaskState *ts);
> diff --git a/contrib/libvhost-user/libvhost-user.h 
> b/contrib/libvhost-user/libvhost-user.h
> index 6fc8000e99..f30394fab6 100644
> --- a/contrib/libvhost-user/libvhost-user.h
> +++ b/contrib/libvhost-user/libvhost-user.h
> @@ -286,7 +286,7 @@ typedef struct VuVirtqInflight {
>  uint16_t used_idx;
>  
>  /* Used to track the state of each descriptor in descriptor table */
> -VuDescStateSplit desc[0];
> +VuDescStateSplit desc[];
>  } VuVirtqInflight;
>  
>  typedef struct VuVirtqInflightDesc {
> diff --git a/hw/m68k/bootinfo.h b/hw/m68k/bootinfo.h
> index 5f8ded2686..c954270aad 100644
> --- a/hw/m68k/bootinfo.h
> +++ b/hw/m68k/bootinfo.h
> @@ -14,7 +14,7 @@
>  struct bi_record {
>  uint16_t tag;/* tag ID */
>  uint16_t size;   /* size of record */
> -uint32_t data[0];/* data */
> +uint32_t data[]; /* data */
>  };
>  
>  /* machine independent tags */
> diff --git a/hw/scsi/srp.h b/hw/scsi/srp.h
> index d27f31d2d5..54c954badd 100644
> --- a/hw/scsi/srp.h
> +++ b/hw/scsi/srp.h
> @@ -112,7 +112,7 @@ struct srp_direct_buf {
>  struct srp_indirect_buf {
>  struct srp_direct_buftable_desc;
>  uint32_t len;
> -struct srp_direct_bufdesc_list[0];
> +struct srp_direct_bufdesc_list[];
>  } QEMU_PACKED;
>  
>  enum {
> @@ -211,7 +211,7 @@ struct srp_cmd {
>  uint8_treserved4;
>  uint8_tadd_cdb_len;
>  uint8_tcdb[16];
> -uint8_tadd_data[0];
> +uint8_tadd_data[];
>  } QEMU_PACKED;
>  
>  enum {
> @@ -241,7 +241,7 @@ struct srp_rsp {
>  uint32_t   data_in_res_cnt;
>  

Re: [PATCH 2/2] misc: Replace zero-length arrays with flexible array member (manual)

[David Hildenbrand]

On 04.03.20 01:58, Philippe Mathieu-Daudé wrote:
> Description copied from Linux kernel commit from Gustavo A. R. Silva
> (see [3]):
> 
> --v-- description start --v--
> 
>   The current codebase makes use of the zero-length array language
>   extension to the C90 standard, but the preferred mechanism to
>   declare variable-length types such as these ones is a flexible
>   array member [1], introduced in C99:
> 
>   struct foo {
>   int stuff;
>   struct boo array[];
>   };
> 
>   By making use of the mechanism above, we will get a compiler
>   warning in case the flexible array does not occur last in the
>   structure, which will help us prevent some kind of undefined
>   behavior bugs from being unadvertenly introduced [2] to the
>   Linux codebase from now on.
> 
> --^-- description end --^--
> 
> Do the similar housekeeping in the QEMU codebase (which uses
> C99 since commit 7be41675f7cb).
> 
> All these instances of code were found with the help of the
> following command (then manual analysis):
> 
>   git grep -F '[0];'
> 
> [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> [2] 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=76497732932f
> [3] 
> https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?id=17642a2fbd2c1
> 
> Inspired-by: Gustavo A. R. Silva 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  docs/interop/vhost-user.rst   | 4 ++--
>  block/qed.h   | 2 +-
>  include/hw/acpi/acpi-defs.h   | 4 ++--
>  include/hw/boards.h   | 2 +-
>  include/hw/s390x/event-facility.h | 2 +-
>  include/hw/s390x/sclp.h   | 8 
>  block/vmdk.c  | 2 +-
>  hw/char/sclpconsole-lm.c  | 2 +-
>  hw/char/sclpconsole.c | 2 +-
>  hw/s390x/virtio-ccw.c | 2 +-
>  target/s390x/ioinst.c | 2 +-
>  11 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
> index 401652397c..3b1b6602c7 100644
> --- a/docs/interop/vhost-user.rst
> +++ b/docs/interop/vhost-user.rst
> @@ -568,7 +568,7 @@ For split virtqueue, queue region can be implemented as:
>uint16_t used_idx;
>  
>/* Used to track the state of each descriptor in descriptor table */
> -  DescStateSplit desc[0];
> +  DescStateSplit desc[];
>} QueueRegionSplit;
>  
>  To track inflight I/O, the queue region should be processed as follows:
> @@ -690,7 +690,7 @@ For packed virtqueue, queue region can be implemented as:
>uint8_t padding[7];
>  
>/* Used to track the state of each descriptor fetched from descriptor 
> ring */
> -  DescStatePacked desc[0];
> +  DescStatePacked desc[];
>} QueueRegionPacked;
>  
>  To track inflight I/O, the queue region should be processed as follows:
> diff --git a/block/qed.h b/block/qed.h
> index 42c115d822..87428ba00e 100644
> --- a/block/qed.h
> +++ b/block/qed.h
> @@ -103,7 +103,7 @@ typedef struct {
>  } QEMU_PACKED QEDHeader;
>  
>  typedef struct {
> -uint64_t offsets[0];/* in bytes */
> +uint64_t offsets[]; /* in bytes */
>  } QEDTable;
>  
>  /* The L2 cache is a simple write-through cache for L2 structures */
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index 19f7ba7b70..c13327fa78 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -152,7 +152,7 @@ typedef struct AcpiSerialPortConsoleRedirection
>   */
>  struct AcpiRsdtDescriptorRev1 {
>  ACPI_TABLE_HEADER_DEF   /* ACPI common table header */
> -uint32_t table_offset_entry[0];  /* Array of pointers to other */
> +uint32_t table_offset_entry[];  /* Array of pointers to other */
>  /* ACPI tables */
>  } QEMU_PACKED;
>  typedef struct AcpiRsdtDescriptorRev1 AcpiRsdtDescriptorRev1;
> @@ -162,7 +162,7 @@ typedef struct AcpiRsdtDescriptorRev1 
> AcpiRsdtDescriptorRev1;
>   */
>  struct AcpiXsdtDescriptorRev2 {
>  ACPI_TABLE_HEADER_DEF   /* ACPI common table header */
> -uint64_t table_offset_entry[0];  /* Array of pointers to other */
> +uint64_t table_offset_entry[];  /* Array of pointers to other */
>  /* ACPI tables */
>  } QEMU_PACKED;
>  typedef struct AcpiXsdtDescriptorRev2 AcpiXsdtDescriptorRev2;
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 9bc42dfb22..c96120d15f 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -71,7 +71,7 @@ typedef struct CPUArchId {
>   */
>  typedef struct {
>  int len;
> -CPUArchId cpus[0];
> +CPUArchId cpus[];
>  } CPUArchIdList;
>  
>  /**
> diff --git a/include/hw/s390x/event-facility.h 
> b/include/hw/s390x/event-facility.h
> index bdc32a3c09..700a610f33 100644
> --- a/include/hw/s390x/event-facility.h
> +++ b/include/hw/s390x/event-facility.h
> @@ -122,7 +122,7 @@ typedef struct MDBO {
>  
>  typedef struct MDB {
>  MdbHeader header;
> -MDBO mdbo[0];
> +M

Re: [PATCH 2/2] misc: Replace zero-length arrays with flexible array member (manual)

[Philippe Mathieu-Daudé]

On 3/4/20 1:58 AM, Philippe Mathieu-Daudé wrote:

Description copied from Linux kernel commit from Gustavo A. R. Silva
(see [3]):

--v-- description start --v--

   The current codebase makes use of the zero-length array language
   extension to the C90 standard, but the preferred mechanism to
   declare variable-length types such as these ones is a flexible
   array member [1], introduced in C99:

   struct foo {
   int stuff;
   struct boo array[];
   };

   By making use of the mechanism above, we will get a compiler
   warning in case the flexible array does not occur last in the
   structure, which will help us prevent some kind of undefined
   behavior bugs from being unadvertenly introduced [2] to the
   Linux codebase from now on.

--^-- description end --^--

Do the similar housekeeping in the QEMU codebase (which uses
C99 since commit 7be41675f7cb).

All these instances of code were found with the help of the
following command (then manual analysis):

   git grep -F '[0];'

[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=76497732932f
[3] 
https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?id=17642a2fbd2c1

Inspired-by: Gustavo A. R. Silva 
Signed-off-by: Philippe Mathieu-Daudé 
---
  docs/interop/vhost-user.rst   | 4 ++--
  block/qed.h   | 2 +-
  include/hw/acpi/acpi-defs.h   | 4 ++--
  include/hw/boards.h   | 2 +-
  include/hw/s390x/event-facility.h | 2 +-
  include/hw/s390x/sclp.h   | 8 
  block/vmdk.c  | 2 +-
  hw/char/sclpconsole-lm.c  | 2 +-
  hw/char/sclpconsole.c | 2 +-
  hw/s390x/virtio-ccw.c | 2 +-
  target/s390x/ioinst.c | 2 +-
  11 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
index 401652397c..3b1b6602c7 100644
--- a/docs/interop/vhost-user.rst
+++ b/docs/interop/vhost-user.rst
@@ -568,7 +568,7 @@ For split virtqueue, queue region can be implemented as:
uint16_t used_idx;
  
/* Used to track the state of each descriptor in descriptor table */

-  DescStateSplit desc[0];
+  DescStateSplit desc[];
} QueueRegionSplit;
  
  To track inflight I/O, the queue region should be processed as follows:

@@ -690,7 +690,7 @@ For packed virtqueue, queue region can be implemented as:
uint8_t padding[7];
  
/* Used to track the state of each descriptor fetched from descriptor ring */

-  DescStatePacked desc[0];
+  DescStatePacked desc[];
} QueueRegionPacked;
  
  To track inflight I/O, the queue region should be processed as follows:

diff --git a/block/qed.h b/block/qed.h
index 42c115d822..87428ba00e 100644
--- a/block/qed.h
+++ b/block/qed.h
@@ -103,7 +103,7 @@ typedef struct {
  } QEMU_PACKED QEDHeader;
  
  typedef struct {

-uint64_t offsets[0];/* in bytes */
+uint64_t offsets[]; /* in bytes */


Apparently this one is incorrect, it triggers:

GCC:
block/qed.h:106:14: error: flexible array member in otherwise empty struct

Clang:
block/qed.h:106:14: error: flexible array member 'offsets' not allowed 
in otherwise empty struct



  } QEDTable;
  
  /* The L2 cache is a simple write-through cache for L2 structures */

diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index 19f7ba7b70..c13327fa78 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -152,7 +152,7 @@ typedef struct AcpiSerialPortConsoleRedirection
   */
  struct AcpiRsdtDescriptorRev1 {
  ACPI_TABLE_HEADER_DEF   /* ACPI common table header */
-uint32_t table_offset_entry[0];  /* Array of pointers to other */
+uint32_t table_offset_entry[];  /* Array of pointers to other */
  /* ACPI tables */
  } QEMU_PACKED;
  typedef struct AcpiRsdtDescriptorRev1 AcpiRsdtDescriptorRev1;
@@ -162,7 +162,7 @@ typedef struct AcpiRsdtDescriptorRev1 
AcpiRsdtDescriptorRev1;
   */
  struct AcpiXsdtDescriptorRev2 {
  ACPI_TABLE_HEADER_DEF   /* ACPI common table header */
-uint64_t table_offset_entry[0];  /* Array of pointers to other */
+uint64_t table_offset_entry[];  /* Array of pointers to other */
  /* ACPI tables */
  } QEMU_PACKED;
  typedef struct AcpiXsdtDescriptorRev2 AcpiXsdtDescriptorRev2;
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 9bc42dfb22..c96120d15f 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -71,7 +71,7 @@ typedef struct CPUArchId {
   */
  typedef struct {
  int len;
-CPUArchId cpus[0];
+CPUArchId cpus[];
  } CPUArchIdList;
  
  /**

diff --git a/include/hw/s390x/event-facility.h 
b/include/hw/s390x/event-facility.h
index bdc32a3c09..700a610f33 100644
--- a/include/hw/s390x/event-facility.h
+++ b/include/hw/s390x/event-facility.h
@@ -122,7 +122,7 @@ typedef struct MDBO {
  
  typedef struct MDB {

  Md

Re: [PATCH 0/2] block: bdrv_reopen() with backing file in different AioContext

[Peter Krempa]

On Thu, Feb 27, 2020 at 19:18:02 +0100, Kevin Wolf wrote:
> Kevin Wolf (2):
>   iotests: Refactor blockdev-reopen test for iothreads
>   block: bdrv_reopen() with backing file in different AioContext

Thanks for the patches. It fixes the problem we've talked about:

Tested-by: Peter Krempa 

Re: [PATCH 0/2] block: bdrv_reopen() with backing file in different AioContext

[Kevin Wolf]

Am 04.03.2020 um 10:29 hat Peter Krempa geschrieben:
> On Thu, Feb 27, 2020 at 19:18:02 +0100, Kevin Wolf wrote:
> > Kevin Wolf (2):
> >   iotests: Refactor blockdev-reopen test for iothreads
> >   block: bdrv_reopen() with backing file in different AioContext
> 
> Thanks for the patches. It fixes the problem we've talked about:
> 
> Tested-by: Peter Krempa 

Thanks for testing, applied to the block branch.

Kevin

Re: [PATCH v6 1/9] iotests: do a light delinting

[Kevin Wolf]

Am 03.03.2020 um 22:25 hat John Snow geschrieben:
> 
> 
> On 2/27/20 7:59 AM, Max Reitz wrote:
> > On 27.02.20 01:06, 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. This will be important if (when?) we
> >> begin adding type hints to our code base.

I'm not sure I understand this connection. mypy doesn't care about
style.

> >> Signed-off-by: John Snow 
> >> ---
> >>  tests/qemu-iotests/iotests.py | 88 ++-
> >>  1 file changed, 45 insertions(+), 43 deletions(-)
> > 
> > I feel like I’m the wrongest person there is for reviewing a Python
> > style-fixing patch, but here I am and so here I go:
> 
> No, it's good.

Max can't be the wrongest person for it because that's already me.

> >> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> >> index 8815052eb5..e8a0ea14fc 100644
> >> --- a/tests/qemu-iotests/iotests.py
> >> +++ b/tests/qemu-iotests/iotests.py
> > 
> > [...]
> > 
> >> @@ -245,8 +243,7 @@ def qemu_nbd_early_pipe(*args):
> >>' '.join(qemu_nbd_args + ['--fork'] + 
> >> list(args
> >>  if exitcode == 0:
> >>  return exitcode, ''
> >> -else:
> >> -return exitcode, subp.communicate()[0]
> >> +return exitcode, subp.communicate()[0]
> > 
> > If we want to make such a change (which I don’t doubt we want), I think
> > it should be the other way around: Make the condition “exitcode != 0”,
> > so the final return is the return for the successful case.  (Just
> > because I think that’s how we usually do it, at least in the qemu code?)
> > 
> > [...]
> > 
> 
> Yes, makes sense. I was behaving a little more mechanically.

Here and...

> >> @@ -756,12 +750,13 @@ def assert_block_path(self, root, path, 
> >> expected_node, graph=None):
> >>  assert node is not None, 'Cannot follow path %s%s' % (root, 
> >> path)
> >>  
> >>  try:
> >> -node_id = next(edge['child'] for edge in graph['edges'] \
> >> - if edge['parent'] == 
> >> node['id'] and
> >> -edge['name'] == 
> >> child_name)
> >> +node_id = next(edge['child'] for edge in graph['edges']
> >> +   if edge['parent'] == node['id'] and
> >> +   edge['name'] == child_name)
> > 
> > I don’t mind the if alignment, but I do mind not aligning the third line
> > to the “edge” above it (i.e. the third line is part of the condition, so
> > I’d align it to the “if” condition).
> > 
> > But then again it’s nothing new that I like to disagree with commonly
> > agreed-upon Python coding styles, so.
> > 
> > [...]
> > 
> 
> OK, that can be addressed by highlighting the sub-expression with
> parentheses:
> 
> node_id = next(edge['child'] for edge in graph['edges']
>if (edge['parent'] == node['id'] and
>edge['name'] == child_name))

...here I must say that while I think Max's suggestions feel like an
improvement to me over the patch, I actually like the current code best
in both cases.

In fact, after scanning your patch, I feel it's actually the majority of
changes that pylint wants that aren't an improvement... Maybe this just
underlines the fact that I am the wrongest person to review such patches
and not Max. Though I'm surprised that I'm generally not the person who
introduces the code violating the rules, and I don't have the impression
in this thread that anyone is eager to defend pylint's opinion.

Now I ran pylint myself and it prints some even more ridiculous warnings
like variable names being too short for its liking. I guess this means
that if we want to run it without warnings or errors, we need to use a
config file anyway to disable the worst parts.

And if we have a config file anyway, maybe we can more selectively
enable the checks that we actually want?

Kevin

Re: [PATCH 0/2] misc: Replace zero-length arrays with flexible array member

[Paolo Bonzini]

On 04/03/20 01:51, Philippe Mathieu-Daudé wrote:
> This is a tree-wide cleanup inspired by a Linux kernel commit
> (from Gustavo A. R. Silva).
> 
> --v-- description start --v--
> 
>   The current codebase makes use of the zero-length array language
>   extension to the C90 standard, but the preferred mechanism to
>   declare variable-length types such as these ones is a flexible
>   array member [1], introduced in C99:
> 
>   struct foo {
>   int stuff;
>   struct boo array[];
>   };
> 
>   By making use of the mechanism above, we will get a compiler
>   warning in case the flexible array does not occur last in the
>   structure, which will help us prevent some kind of undefined
>   behavior bugs from being unadvertenly introduced [2] to the
>   Linux codebase from now on.
> 
> --^-- description end --^--
> 
> Do the similar housekeeping in the QEMU codebase (which uses
> C99 since commit 7be41675f7cb).
> 
> The first patch is done with the help of a coccinelle semantic
> patch. However Coccinelle does not recognize:
> 
>   struct foo {
>   int stuff;
>   struct boo array[];
>   } QEMU_PACKED;
> 
> but does recognize:
> 
>   struct QEMU_PACKED foo {
>   int stuff;
>   struct boo array[];
>   };
> 
> I'm not sure why, neither it is worth refactoring all QEMU
> structures to use the attributes before the structure name,
> so I did the 2nd patch manually.
> 
> Anyway this is annoying, because many structures are not handled
> by coccinelle. Maybe this needs to be reported to upstream
> coccinelle?
> 
> I used spatch 1.0.8 with:
> 
>   -I include --include-headers \
>   --macro-file scripts/cocci-macro-file.h \
>   --keep-comments --indent 4
> 
> Regards,
> 
> Phil.
> 
> Philippe Mathieu-Daudé (2):
>   misc: Replace zero-length arrays with flexible array member
> (automatic)
>   misc: Replace zero-length arrays with flexible array member (manual)
> 
>  docs/interop/vhost-user.rst   |  4 ++--
>  block/qed.h   |  2 +-
>  bsd-user/qemu.h   |  2 +-
>  contrib/libvhost-user/libvhost-user.h |  2 +-
>  hw/m68k/bootinfo.h|  2 +-
>  hw/scsi/srp.h |  6 +++---
>  hw/xen/xen_pt.h   |  2 +-
>  include/hw/acpi/acpi-defs.h   | 16 
>  include/hw/arm/smmu-common.h  |  2 +-
>  include/hw/boards.h   |  2 +-
>  include/hw/i386/intel_iommu.h |  3 ++-
>  include/hw/s390x/event-facility.h |  2 +-
>  include/hw/s390x/sclp.h   |  8 
>  include/hw/virtio/virtio-iommu.h  |  2 +-
>  include/sysemu/cryptodev.h|  2 +-
>  include/tcg/tcg.h |  2 +-
>  pc-bios/s390-ccw/bootmap.h|  2 +-
>  pc-bios/s390-ccw/sclp.h   |  2 +-
>  tests/qtest/libqos/ahci.h |  2 +-
>  block/linux-aio.c |  2 +-
>  block/vmdk.c  |  2 +-
>  hw/acpi/nvdimm.c  |  6 +++---
>  hw/char/sclpconsole-lm.c  |  2 +-
>  hw/char/sclpconsole.c |  2 +-
>  hw/dma/soc_dma.c  |  2 +-
>  hw/i386/x86.c |  2 +-
>  hw/misc/omap_l4.c |  2 +-
>  hw/nvram/eeprom93xx.c |  2 +-
>  hw/rdma/vmw/pvrdma_qp_ops.c   |  4 ++--
>  hw/s390x/virtio-ccw.c |  2 +-
>  hw/usb/dev-network.c  |  2 +-
>  hw/usb/dev-smartcard-reader.c |  4 ++--
>  hw/virtio/virtio.c|  4 ++--
>  net/queue.c   |  2 +-
>  target/s390x/ioinst.c |  2 +-
>  35 files changed, 54 insertions(+), 53 deletions(-)
> 

Queued (minus the qed part).

Paolo

Re: [PATCH v4 5/5] iotests: 287: add qcow2 compression type test

[Vladimir Sementsov-Ogievskiy]

03.03.2020 16:34, Denis Plotnikov wrote:

The test checks fulfilling qcow2 requiriements for the compression
type feature and zstd compression type operability.

Signed-off-by: Denis Plotnikov 
---
  tests/qemu-iotests/287 | 127 +
  tests/qemu-iotests/287.out |  43 +
  tests/qemu-iotests/group   |   1 +
  3 files changed, 171 insertions(+)
  create mode 100755 tests/qemu-iotests/287
  create mode 100644 tests/qemu-iotests/287.out

diff --git a/tests/qemu-iotests/287 b/tests/qemu-iotests/287
new file mode 100755
index 00..39cb665c85
--- /dev/null
+++ b/tests/qemu-iotests/287


[..]


+# Test: using zstd compression, write to and read from an image
+echo
+echo "=== Testing reading and writing with zstd ==="
+echo
+
+CLUSTER_SIZE=65536
+IMGOPTS='compression_type=zstd' _make_test_img 64M


As I understand, you should define env variable assignments on the same line
with _make_test_img so that they be passed to it, like
CLUSTER_SIZE=65536 IMGOPTS='compression_type=zstd' _make_test_img 64M

with this:
Reviewed-by: Vladimir Sementsov-Ogievskiy 


+$QEMU_IO -c "write -c -P 0xAC 65536 64k " "$TEST_IMG" | _filter_qemu_io


you may s/65536/64k/


+$QEMU_IO -c "read -P 0xAC 65536 65536 " "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -v 131070 8 " "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -v 65534 8" "$TEST_IMG" | _filter_qemu_io
+



[..]


--
Best regards,
Vladimir

Re: [PATCH v4 4/5] qcow2: add zstd cluster compression

[Denis Plotnikov]

On 04.03.2020 10:49, Vladimir Sementsov-Ogievskiy wrote:

03.03.2020 16:34, Denis Plotnikov wrote:

zstd significantly reduces cluster compression time.
It provides better compression performance maintaining
the same level of the compression ratio in comparison with
zlib, which, at the moment, is the only compression
method available.

The performance test results:
Test compresses and decompresses qemu qcow2 image with just
installed rhel-7.6 guest.
Image cluster size: 64K. Image on disk size: 2.2G

The test was conducted with brd disk to reduce the influence
of disk subsystem to the test results.
The results is given in seconds.

compress cmd:
   time ./qemu-img convert -O qcow2 -c -o compression_type=[zlib|zstd]
   src.img [zlib|zstd]_compressed.img
decompress cmd
   time ./qemu-img convert -O qcow2
   [zlib|zstd]_compressed.img uncompressed.img

    compression   decompression
  zlib   zstd   zlib zstd

real 65.5   16.3 (-75 %)    1.9  1.6 (-16 %)
user 65.0   15.8    5.3  2.5
sys   3.3    0.2    2.0  2.0

Both ZLIB and ZSTD gave the same compression ratio: 1.57
compressed image size in both cases: 1.4G

Signed-off-by: Denis Plotnikov 
---


[..]


+static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
+   const void *src, size_t src_size)
+{
+    size_t ret;
+
+    /*
+ * steal ZSTD_LEN_BUF bytes in the very beginning of the buffer
+ * to store compressed chunk size
+ */
+    char *d_buf = ((char *) dest) + ZSTD_LEN_BUF;
+
+    /*
+ * sanity check that we can store the compressed data length,
+ * and there is some space left for the compressor buffer
+ */
+    if (dest_size <= ZSTD_LEN_BUF) {
+    return -ENOMEM;
+    }
+
+    dest_size -= ZSTD_LEN_BUF;
+
+    ret = ZSTD_compress(d_buf, dest_size, src, src_size, 5);


You may want to define ZSTD_COMPRESSION_LEVEL constant instead of raw 
number.
I didn't introduce it intentionally. zlib compression has the 
compression level hardcoded as well.
I think it's better to introduce the compression level for both of them 
in the future but not in the scope of this series.

anyway,
Reviewed-by: Vladimir Sementsov-Ogievskiy 

Re: [PATCH v4 5/5] iotests: 287: add qcow2 compression type test

[Denis Plotnikov]

On 04.03.2020 14:27, Vladimir Sementsov-Ogievskiy wrote:

03.03.2020 16:34, Denis Plotnikov wrote:

The test checks fulfilling qcow2 requiriements for the compression
type feature and zstd compression type operability.

Signed-off-by: Denis Plotnikov 
---
  tests/qemu-iotests/287 | 127 +
  tests/qemu-iotests/287.out |  43 +
  tests/qemu-iotests/group   |   1 +
  3 files changed, 171 insertions(+)
  create mode 100755 tests/qemu-iotests/287
  create mode 100644 tests/qemu-iotests/287.out

diff --git a/tests/qemu-iotests/287 b/tests/qemu-iotests/287
new file mode 100755
index 00..39cb665c85
--- /dev/null
+++ b/tests/qemu-iotests/287


[..]


+# Test: using zstd compression, write to and read from an image
+echo
+echo "=== Testing reading and writing with zstd ==="
+echo
+
+CLUSTER_SIZE=65536
+IMGOPTS='compression_type=zstd' _make_test_img 64M


As I understand, you should define env variable assignments on the 
same line

with _make_test_img so that they be passed to it, like
CLUSTER_SIZE=65536 IMGOPTS='compression_type=zstd' _make_test_img 64M
It works like a regular env variable and can be defined on another line 
above.
Anyway, I'll move "CLUSTER_SIZE=65536" to the beginning of the test to 
avoid any confusions.


with this:
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Thanks for reviewing the series! l'll send v5 with all modifications 
shortly.


Denis



+$QEMU_IO -c "write -c -P 0xAC 65536 64k " "$TEST_IMG" | _filter_qemu_io


you may s/65536/64k/


+$QEMU_IO -c "read -P 0xAC 65536 65536 " "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -v 131070 8 " "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -v 65534 8" "$TEST_IMG" | _filter_qemu_io
+



[..]

Re: [PATCH 1/2] misc: Replace zero-length arrays with flexible array member (automatic)

[Philippe Mathieu-Daudé]

On 3/4/20 1:51 AM, Philippe Mathieu-Daudé wrote:

Description copied from Linux kernel commit from Gustavo A. R. Silva
(see [3]):

--v-- description start --v--

   The current codebase makes use of the zero-length array language
   extension to the C90 standard, but the preferred mechanism to
   declare variable-length types such as these ones is a flexible
   array member [1], introduced in C99:

   struct foo {
   int stuff;
   struct boo array[];
   };

   By making use of the mechanism above, we will get a compiler
   warning in case the flexible array does not occur last in the
   structure, which will help us prevent some kind of undefined
   behavior bugs from being unadvertenly introduced [2] to the
   Linux codebase from now on.

--^-- description end --^--

Do the similar housekeeping in the QEMU codebase (which uses
C99 since commit 7be41675f7cb).

All these instances of code were found with the help of the
following Coccinelle script:

   @@
   identifier s, a;
   type T;
   @@
struct s {
   ...
   -   T a[0];
   +   T a[];
   };
   @@
   identifier s, a;
   type T;
   @@
struct s {
   ...
   -   T a[0];
   +   T a[];
} QEMU_PACKED;

[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=76497732932f
[3] 
https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?id=17642a2fbd2c1

Inspired-by: Gustavo A. R. Silva 
Signed-off-by: Philippe Mathieu-Daudé 
---
  bsd-user/qemu.h   |  2 +-
  contrib/libvhost-user/libvhost-user.h |  2 +-
  hw/m68k/bootinfo.h|  2 +-
  hw/scsi/srp.h |  6 +++---
  hw/xen/xen_pt.h   |  2 +-
  include/hw/acpi/acpi-defs.h   | 12 ++--
  include/hw/arm/smmu-common.h  |  2 +-
  include/hw/i386/intel_iommu.h |  3 ++-
  include/hw/virtio/virtio-iommu.h  |  2 +-
  include/sysemu/cryptodev.h|  2 +-
  include/tcg/tcg.h |  2 +-
  pc-bios/s390-ccw/bootmap.h|  2 +-
  pc-bios/s390-ccw/sclp.h   |  2 +-
  tests/qtest/libqos/ahci.h |  2 +-
  block/linux-aio.c |  2 +-
  hw/acpi/nvdimm.c  |  6 +++---
  hw/dma/soc_dma.c  |  2 +-
  hw/i386/x86.c |  2 +-
  hw/misc/omap_l4.c |  2 +-
  hw/nvram/eeprom93xx.c |  2 +-
  hw/rdma/vmw/pvrdma_qp_ops.c   |  4 ++--
  hw/usb/dev-network.c  |  2 +-
  hw/usb/dev-smartcard-reader.c |  4 ++--
  hw/virtio/virtio.c|  4 ++--
  net/queue.c   |  2 +-
  25 files changed, 38 insertions(+), 37 deletions(-)


[...]

diff --git a/hw/scsi/srp.h b/hw/scsi/srp.h
index d27f31d2d5..54c954badd 100644
--- a/hw/scsi/srp.h
+++ b/hw/scsi/srp.h
@@ -112,7 +112,7 @@ struct srp_direct_buf {
  struct srp_indirect_buf {
  struct srp_direct_buftable_desc;
  uint32_t len;
-struct srp_direct_bufdesc_list[0];
+struct srp_direct_bufdesc_list[];
  } QEMU_PACKED;
  
  enum {

@@ -211,7 +211,7 @@ struct srp_cmd {
  uint8_treserved4;
  uint8_tadd_cdb_len;
  uint8_tcdb[16];
-uint8_tadd_data[0];
+uint8_tadd_data[];
  } QEMU_PACKED;
  
  enum {

@@ -241,7 +241,7 @@ struct srp_rsp {
  uint32_t   data_in_res_cnt;
  uint32_t   sense_data_len;
  uint32_t   resp_data_len;
-uint8_tdata[0];
+uint8_tdata[];
  } QEMU_PACKED;


hw/scsi/spapr_vscsi.c:69:29: error: field 'iu' with variable sized type 
'union viosrp_iu' not at the end of a struct or class is a GNU extension 
[-Werror,-Wgnu-variable-sized-type-not-at-end]

union viosrp_iu iu;
^

Yay we found a bug! Thanks Gustavo :)

union srp_iu {
struct srp_login_req login_req;
struct srp_login_rsp login_rsp;
struct srp_login_rej login_rej;
struct srp_i_logout i_logout;
struct srp_t_logout t_logout;
struct srp_tsk_mgmt tsk_mgmt;
struct srp_cmd cmd;
struct srp_rsp rsp;
uint8_t reserved[SRP_MAX_IU_LEN];
};

union viosrp_iu {
union srp_iu srp;
union mad_iu mad;
};

typedef struct vscsi_req {
vscsi_crq   crq;
union viosrp_iu iu;

/* SCSI request tracking */
SCSIRequest *sreq;
uint32_tqtag; /* qemu tag != srp tag */
boolactive;
boolwriting;
booldma_error;
uint32_tdata_len;
uint32_tsenselen;
uint8_t sense[SCSI_SENSE_BUF_SIZE];

/* RDMA related bits */
uint8_t dma_fmt;
uint16_tlocal_desc;
uint16_ttotal_desc;
uint16_tcdb_offset;
uint16_tcur_desc_num;
uint16_t   

[PATCH v5 1/5] block/qcow2-threads: fix qcow2_decompress

[Denis Plotnikov]

From: Vladimir Sementsov-Ogievskiy 

On success path we return what inflate() returns instead of 0. And it
most probably works for Z_STREAM_END as it is positive, but is
definitely broken for Z_BUF_ERROR.

While being here, switch to errno return code, to be closer to
qcow2_compress API (and usual expectations).

Revert condition in if to be more positive. Drop dead initialization of
ret.

Cc: qemu-sta...@nongnu.org # v4.0
Fixes: 341926ab83e2b
Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Alberto Garcia 
---
 block/qcow2-threads.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 77bb578cdf..a68126f291 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -128,12 +128,12 @@ static ssize_t qcow2_compress(void *dest, size_t 
dest_size,
  * @src - source buffer, @src_size bytes
  *
  * Returns: 0 on success
- *  -1 on fail
+ *  -EIO on fail
  */
 static ssize_t qcow2_decompress(void *dest, size_t dest_size,
 const void *src, size_t src_size)
 {
-int ret = 0;
+int ret;
 z_stream strm;
 
 memset(&strm, 0, sizeof(strm));
@@ -144,17 +144,19 @@ static ssize_t qcow2_decompress(void *dest, size_t 
dest_size,
 
 ret = inflateInit2(&strm, -12);
 if (ret != Z_OK) {
-return -1;
+return -EIO;
 }
 
 ret = inflate(&strm, Z_FINISH);
-if ((ret != Z_STREAM_END && ret != Z_BUF_ERROR) || strm.avail_out != 0) {
+if ((ret == Z_STREAM_END || ret == Z_BUF_ERROR) && strm.avail_out == 0) {
 /*
  * We approve Z_BUF_ERROR because we need @dest buffer to be filled, 
but
  * @src buffer may be processed partly (because in qcow2 we know size 
of
  * compressed data with precision of one sector)
  */
-ret = -1;
+ret = 0;
+} else {
+ret = -EIO;
 }
 
 inflateEnd(&strm);
-- 
2.17.0

[PATCH v5 3/5] qcow2: rework the cluster compression routine

[Denis Plotnikov]

The patch enables processing the image compression type defined
for the image and chooses an appropriate method for image clusters
(de)compression.

Signed-off-by: Denis Plotnikov 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2-threads.c | 71 ---
 1 file changed, 60 insertions(+), 11 deletions(-)

diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index a68126f291..7dbaf53489 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -74,7 +74,9 @@ typedef struct Qcow2CompressData {
 } Qcow2CompressData;
 
 /*
- * qcow2_compress()
+ * qcow2_zlib_compress()
+ *
+ * Compress @src_size bytes of data using zlib compression method
  *
  * @dest - destination buffer, @dest_size bytes
  * @src - source buffer, @src_size bytes
@@ -83,8 +85,8 @@ typedef struct Qcow2CompressData {
  *  -ENOMEM destination buffer is not enough to store compressed data
  *  -EIOon any other error
  */
-static ssize_t qcow2_compress(void *dest, size_t dest_size,
-  const void *src, size_t src_size)
+static ssize_t qcow2_zlib_compress(void *dest, size_t dest_size,
+   const void *src, size_t src_size)
 {
 ssize_t ret;
 z_stream strm;
@@ -119,10 +121,10 @@ static ssize_t qcow2_compress(void *dest, size_t 
dest_size,
 }
 
 /*
- * qcow2_decompress()
+ * qcow2_zlib_decompress()
  *
  * Decompress some data (not more than @src_size bytes) to produce exactly
- * @dest_size bytes.
+ * @dest_size bytes using zlib compression method
  *
  * @dest - destination buffer, @dest_size bytes
  * @src - source buffer, @src_size bytes
@@ -130,8 +132,8 @@ static ssize_t qcow2_compress(void *dest, size_t dest_size,
  * Returns: 0 on success
  *  -EIO on fail
  */
-static ssize_t qcow2_decompress(void *dest, size_t dest_size,
-const void *src, size_t src_size)
+static ssize_t qcow2_zlib_decompress(void *dest, size_t dest_size,
+ const void *src, size_t src_size)
 {
 int ret;
 z_stream strm;
@@ -191,20 +193,67 @@ qcow2_co_do_compress(BlockDriverState *bs, void *dest, 
size_t dest_size,
 return arg.ret;
 }
 
+/*
+ * qcow2_co_compress()
+ *
+ * Compress @src_size bytes of data using the compression
+ * method defined by the image compression type
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: compressed size on success
+ *  a negative error code on failure
+ */
 ssize_t coroutine_fn
 qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
   const void *src, size_t src_size)
 {
-return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
-qcow2_compress);
+BDRVQcow2State *s = bs->opaque;
+Qcow2CompressFunc fn;
+
+switch (s->compression_type) {
+case QCOW2_COMPRESSION_TYPE_ZLIB:
+fn = qcow2_zlib_compress;
+break;
+
+default:
+abort();
+}
+
+return qcow2_co_do_compress(bs, dest, dest_size, src, src_size, fn);
 }
 
+/*
+ * qcow2_co_decompress()
+ *
+ * Decompress some data (not more than @src_size bytes) to produce exactly
+ * @dest_size bytes using the compression method defined by the image
+ * compression type
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: 0 on success
+ *  a negative error code on failure
+ */
 ssize_t coroutine_fn
 qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
 const void *src, size_t src_size)
 {
-return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
-qcow2_decompress);
+BDRVQcow2State *s = bs->opaque;
+Qcow2CompressFunc fn;
+
+switch (s->compression_type) {
+case QCOW2_COMPRESSION_TYPE_ZLIB:
+fn = qcow2_zlib_decompress;
+break;
+
+default:
+abort();
+}
+
+return qcow2_co_do_compress(bs, dest, dest_size, src, src_size, fn);
 }
 
 
-- 
2.17.0

[PATCH v5 0/5] qcow2: Implement zstd cluster compression method

[Denis Plotnikov]

v5:
   * replace -ENOTSUP with abort in qcow2_co_decompress [Vladimir]
   * set cluster size for all test cases in the beginning of the 287 test

v4:
   * the series is rebased on top of 01 "block/qcow2-threads: fix 
qcow2_decompress"
   * 01 is just a no-change resend to avoid extra dependencies. Still, it may 
be merged in separate

v3:
   * remove redundant max compression type value check [Vladimir, Eric]
 (the switch below checks everything)
   * prevent compression type changing on "qemu-img amend" [Vladimir]
   * remove zstd config setting, since it has been added already by
 "migration" patches [Vladimir]
   * change the compression type error message [Vladimir] 
   * fix alignment and 80-chars exceeding [Vladimir]

v2:
   * rework compression type setting [Vladimir]
   * squash iotest changes to the compression type introduction patch 
[Vladimir, Eric]
   * fix zstd availability checking in zstd iotest [Vladimir]
   * remove unnecessry casting [Eric]
   * remove rudundant checks [Eric]
   * fix compressed cluster layout in qcow2 spec [Vladimir]
   * fix wording [Eric, Vladimir]
   * fix compression type filtering in iotests [Eric]

v1:
   the initial series

Denis Plotnikov (4):
  qcow2: introduce compression type feature
  qcow2: rework the cluster compression routine
  qcow2: add zstd cluster compression
  iotests: 287: add qcow2 compression type test

Vladimir Sementsov-Ogievskiy (1):
  block/qcow2-threads: fix qcow2_decompress

 docs/interop/qcow2.txt   |  20 +++
 configure|   2 +-
 qapi/block-core.json |  23 +++-
 block/qcow2.h|  18 ++-
 include/block/block_int.h|   1 +
 block/qcow2-threads.c| 206 ---
 block/qcow2.c| 108 
 tests/qemu-iotests/031.out   |  14 +--
 tests/qemu-iotests/036.out   |   4 +-
 tests/qemu-iotests/049.out   | 102 +++
 tests/qemu-iotests/060.out   |   1 +
 tests/qemu-iotests/061.out   |  34 ++---
 tests/qemu-iotests/065   |  28 +++--
 tests/qemu-iotests/080   |   2 +-
 tests/qemu-iotests/144.out   |   4 +-
 tests/qemu-iotests/182.out   |   2 +-
 tests/qemu-iotests/242.out   |   5 +
 tests/qemu-iotests/255.out   |   8 +-
 tests/qemu-iotests/287   | 128 +++
 tests/qemu-iotests/287.out   |  43 +++
 tests/qemu-iotests/common.filter |   3 +-
 tests/qemu-iotests/group |   1 +
 22 files changed, 644 insertions(+), 113 deletions(-)
 create mode 100755 tests/qemu-iotests/287
 create mode 100644 tests/qemu-iotests/287.out

-- 
2.17.0

[PATCH v5 2/5] qcow2: introduce compression type feature

[Denis Plotnikov]

The patch adds some preparation parts for incompatible compression type
feature to qcow2 allowing the use different compression methods for
image clusters (de)compressing.

It is implied that the compression type is set on the image creation and
can be changed only later by image conversion, thus compression type
defines the only compression algorithm used for the image, and thus,
for all image clusters.

The goal of the feature is to add support of other compression methods
to qcow2. For example, ZSTD which is more effective on compression than ZLIB.

The default compression is ZLIB. Images created with ZLIB compression type
are backward compatible with older qemu versions.

Adding of the compression type breaks a number of tests because now the
compression type is reported on image creation and there are some changes
in the qcow2 header in size and offsets.

The tests are fixed in the following ways:
* filter out compression_type for all the tests
* fix header size, feature table size and backing file offset
  affected tests: 031, 036, 061, 080
  header_size +=8: 1 byte compression type
   7 bytes padding
  feature_table += 48: incompatible feture compression type
  backing_file_offset += 56 (8 + 48 -> header_change + fature_table_change)
* add "compression type" for test output matching when it isn't filtered
  affected tests: 049, 060, 061, 065, 144, 182, 242, 255

Signed-off-by: Denis Plotnikov 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 qapi/block-core.json |  22 ++-
 block/qcow2.h|  18 +-
 include/block/block_int.h|   1 +
 block/qcow2.c| 101 ++
 tests/qemu-iotests/031.out   |  14 ++---
 tests/qemu-iotests/036.out   |   4 +-
 tests/qemu-iotests/049.out   | 102 +++
 tests/qemu-iotests/060.out   |   1 +
 tests/qemu-iotests/061.out   |  34 ++-
 tests/qemu-iotests/065   |  28 ++---
 tests/qemu-iotests/080   |   2 +-
 tests/qemu-iotests/144.out   |   4 +-
 tests/qemu-iotests/182.out   |   2 +-
 tests/qemu-iotests/242.out   |   5 ++
 tests/qemu-iotests/255.out   |   8 +--
 tests/qemu-iotests/common.filter |   3 +-
 16 files changed, 253 insertions(+), 96 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 85e27bb61f..a67eb8bff4 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -78,6 +78,8 @@
 #
 # @bitmaps: A list of qcow2 bitmap details (since 4.0)
 #
+# @compression-type: the image cluster compression method (since 5.0)
+#
 # Since: 1.7
 ##
 { 'struct': 'ImageInfoSpecificQCow2',
@@ -89,7 +91,8 @@
   '*corrupt': 'bool',
   'refcount-bits': 'int',
   '*encrypt': 'ImageInfoSpecificQCow2Encryption',
-  '*bitmaps': ['Qcow2BitmapInfo']
+  '*bitmaps': ['Qcow2BitmapInfo'],
+  'compression-type': 'Qcow2CompressionType'
   } }
 
 ##
@@ -4392,6 +4395,18 @@
   'data': [ 'v2', 'v3' ] }
 
 
+##
+# @Qcow2CompressionType:
+#
+# Compression type used in qcow2 image file
+#
+# @zlib:  zlib compression, see 
+#
+# Since: 5.0
+##
+{ 'enum': 'Qcow2CompressionType',
+  'data': [ 'zlib' ] }
+
 ##
 # @BlockdevCreateOptionsQcow2:
 #
@@ -4415,6 +4430,8 @@
 # allowed values: off, falloc, full, metadata)
 # @lazy-refcounts: True if refcounts may be updated lazily (default: off)
 # @refcount-bits: Width of reference counts in bits (default: 16)
+# @compression-type: The image cluster compression method
+#(default: zlib, since 5.0)
 #
 # Since: 2.12
 ##
@@ -4430,7 +4447,8 @@
 '*cluster-size':'size',
 '*preallocation':   'PreallocMode',
 '*lazy-refcounts':  'bool',
-'*refcount-bits':   'int' } }
+'*refcount-bits':   'int',
+'*compression-type':'Qcow2CompressionType' } }
 
 ##
 # @BlockdevCreateOptionsQed:
diff --git a/block/qcow2.h b/block/qcow2.h
index 0942126232..485effcb70 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -146,6 +146,12 @@ typedef struct QCowHeader {
 
 uint32_t refcount_order;
 uint32_t header_length;
+
+/* Additional fields */
+uint8_t  compression_type;
+
+/* header must be a multiple of 8 */
+uint8_t  padding[7];
 } QEMU_PACKED QCowHeader;
 
 typedef struct QEMU_PACKED QCowSnapshotHeader {
@@ -216,13 +222,16 @@ enum {
 QCOW2_INCOMPAT_DIRTY_BITNR  = 0,
 QCOW2_INCOMPAT_CORRUPT_BITNR= 1,
 QCOW2_INCOMPAT_DATA_FILE_BITNR  = 2,
+QCOW2_INCOMPAT_COMPRESSION_BITNR = 3,
 QCOW2_INCOMPAT_DIRTY= 1 << QCOW2_INCOMPAT_DIRTY_BITNR,
 QCOW2_INCOMPAT_CORRUPT  = 1 << QCOW2_INCOMPAT_CORRUPT_BITNR,
 QCOW2_INCOMPAT_DATA_FILE= 1 << QCOW2_INCOMPAT_DATA_FILE_BITNR,
+QCOW2_INCOMPAT_COMPRESSION  = 1 << QCOW2_INCOMPAT_COMPRESSION_BITNR,
 
 QCOW2_INCOMPAT_MASK = QCOW2_INCOMPAT_DIRTY
   

[PATCH v5 5/5] iotests: 287: add qcow2 compression type test

[Denis Plotnikov]

The test checks fulfilling qcow2 requiriements for the compression
type feature and zstd compression type operability.

Signed-off-by: Denis Plotnikov 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/287 | 128 +
 tests/qemu-iotests/287.out |  43 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 172 insertions(+)
 create mode 100755 tests/qemu-iotests/287
 create mode 100644 tests/qemu-iotests/287.out

diff --git a/tests/qemu-iotests/287 b/tests/qemu-iotests/287
new file mode 100755
index 00..49d15b3d43
--- /dev/null
+++ b/tests/qemu-iotests/287
@@ -0,0 +1,128 @@
+#!/usr/bin/env bash
+#
+# Test case for an image using zstd compression
+#
+# 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 .
+#
+
+# creator
+owner=dplotni...@virtuozzo.com
+
+seq="$(basename $0)"
+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
+
+# standard environment
+. ./common.rc
+. ./common.filter
+
+# This tests qocw2-specific low-level functionality
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+# for all the cases
+CLUSTER_SIZE=65536
+
+# Check if we can run this test.
+
+IMGOPTS='compression_type=zstd' _make_test_img 64M | grep "Invalid parameter 
'zstd'" 2>&1 1>/dev/null
+
+ZSTD_SUPPORTED=$?
+
+if (($ZSTD_SUPPORTED==0)); then
+_notrun "ZSTD is disabled"
+fi
+
+# Test: when compression is zlib the incompatible bit is unset
+echo
+echo "=== Testing compression type incompatible bit setting for zlib ==="
+echo
+
+IMGOPTS='compression_type=zlib' _make_test_img 64M
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+
+# Test: when compression differs from zlib the incompatible bit is set
+echo
+echo "=== Testing compression type incompatible bit setting for zstd ==="
+echo
+
+IMGOPTS='compression_type=zstd' _make_test_img 64M
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+
+# Test: an image can't be openned if compression type is zlib and
+#   incompatible feature compression type is set
+echo
+echo "=== Testing zlib with incompatible bit set  ==="
+echo
+
+IMGOPTS='compression_type=zlib' _make_test_img 64M
+$PYTHON qcow2.py "$TEST_IMG" set-feature-bit incompatible 3
+# to make sure the bit was actually set
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+$QEMU_IMG info "$TEST_IMG" 2>1 1>/dev/null
+if (($?==0)); then
+echo "Error: The image openned successfully. The image must not be openned"
+fi
+
+# Test: an image can't be openned if compression type is NOT zlib and
+#   incompatible feature compression type is UNSET
+echo
+echo "=== Testing zstd with incompatible bit unset  ==="
+echo
+
+IMGOPTS='compression_type=zstd' _make_test_img 64M
+$PYTHON qcow2.py "$TEST_IMG" set-header incompatible_features 0
+# to make sure the bit was actually unset
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+$QEMU_IMG info "$TEST_IMG" 2>1 1>/dev/null
+if (($?==0)); then
+echo "Error: The image openned successfully. The image must not be openned"
+fi
+# Test: check compression type values
+echo
+echo "=== Testing compression type values  ==="
+echo
+# zlib=0
+IMGOPTS='compression_type=zlib' _make_test_img 64M
+od -j104 -N1 -An -vtu1 "$TEST_IMG"
+
+# zstd=1
+IMGOPTS='compression_type=zstd' _make_test_img 64M
+od -j104 -N1 -An -vtu1 "$TEST_IMG"
+
+# Test: using zstd compression, write to and read from an image
+echo
+echo "=== Testing reading and writing with zstd ==="
+echo
+
+IMGOPTS='compression_type=zstd' _make_test_img 64M
+$QEMU_IO -c "write -c -P 0xAC 65536 64k " "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -P 0xAC 65536 65536 " "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -v 131070 8 " "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -v 65534 8" "$TEST_IMG" | _filter_qemu_io
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/287.out b/tests/qemu-iotests/287.out
new file mode 100644
index 00..8e51c3078d
--- /dev/null
+++ b/tests/qemu-iotests/287.out
@@ -0,0 +1,43 @@
+QA output created by 287
+
+=== Testing compression type incompatible bit setting for zlib ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+

[PATCH v5 4/5] qcow2: add zstd cluster compression

[Denis Plotnikov]

zstd significantly reduces cluster compression time.
It provides better compression performance maintaining
the same level of the compression ratio in comparison with
zlib, which, at the moment, is the only compression
method available.

The performance test results:
Test compresses and decompresses qemu qcow2 image with just
installed rhel-7.6 guest.
Image cluster size: 64K. Image on disk size: 2.2G

The test was conducted with brd disk to reduce the influence
of disk subsystem to the test results.
The results is given in seconds.

compress cmd:
  time ./qemu-img convert -O qcow2 -c -o compression_type=[zlib|zstd]
  src.img [zlib|zstd]_compressed.img
decompress cmd
  time ./qemu-img convert -O qcow2
  [zlib|zstd]_compressed.img uncompressed.img

   compression   decompression
 zlib   zstd   zlib zstd

real 65.5   16.3 (-75 %)1.9  1.6 (-16 %)
user 65.0   15.85.3  2.5
sys   3.30.22.0  2.0

Both ZLIB and ZSTD gave the same compression ratio: 1.57
compressed image size in both cases: 1.4G

Signed-off-by: Denis Plotnikov 
QAPI part:
Acked-by: Markus Armbruster 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 docs/interop/qcow2.txt |  20 +++
 configure  |   2 +-
 qapi/block-core.json   |   3 +-
 block/qcow2-threads.c  | 123 +
 block/qcow2.c  |   7 +++
 5 files changed, 153 insertions(+), 2 deletions(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 5597e24474..9048114445 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -208,6 +208,7 @@ version 2.
 
 Available compression type values:
 0: zlib 
+1: zstd 
 
 
 === Header padding ===
@@ -575,11 +576,30 @@ Compressed Clusters Descriptor (x = 62 - (cluster_bits - 
8)):
 Another compressed cluster may map to the tail of the final
 sector used by this compressed cluster.
 
+The layout of the compressed data depends on the 
compression
+type used for the image (see compressed cluster layout).
+
 If a cluster is unallocated, read requests shall read the data from the backing
 file (except if bit 0 in the Standard Cluster Descriptor is set). If there is
 no backing file or the backing file is smaller than the image, they shall read
 zeros for all parts that are not covered by the backing file.
 
+=== Compressed Cluster Layout ===
+
+The compressed cluster data has a layout depending on the compression
+type used for the image, as follows:
+
+Compressed data layout for the available compression types:
+data_space_lenght - data chunk length available to store a compressed cluster.
+(for more details see "Compressed Clusters Descriptor")
+x = data_space_length - 1
+
+0:  (default)  zlib :
+Byte  0 -  x: the compressed data content
+  all the space provided used for compressed data
+1:  zstd :
+Byte  0 -  3: the length of compressed data in bytes
+  4 -  x: the compressed data content
 
 == Snapshots ==
 
diff --git a/configure b/configure
index caa65f5883..b2a0aa241a 100755
--- a/configure
+++ b/configure
@@ -1835,7 +1835,7 @@ disabled with --disable-FEATURE, default is enabled if 
available:
   lzfse   support of lzfse compression library
   (for reading lzfse-compressed dmg images)
   zstdsupport for zstd compression library
-  (for migration compression)
+  (for migration compression and qcow2 cluster compression)
   seccomp seccomp support
   coroutine-pool  coroutine freelist (better performance)
   glusterfs   GlusterFS backend
diff --git a/qapi/block-core.json b/qapi/block-core.json
index a67eb8bff4..84889fb741 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4401,11 +4401,12 @@
 # Compression type used in qcow2 image file
 #
 # @zlib:  zlib compression, see 
+# @zstd:  zstd compression, see 
 #
 # Since: 5.0
 ##
 { 'enum': 'Qcow2CompressionType',
-  'data': [ 'zlib' ] }
+  'data': [ 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
 
 ##
 # @BlockdevCreateOptionsQcow2:
diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 7dbaf53489..eeae68e88e 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -28,6 +28,11 @@
 #define ZLIB_CONST
 #include 
 
+#ifdef CONFIG_ZSTD
+#include 
+#include 
+#endif
+
 #include "qcow2.h"
 #include "block/thread-pool.h"
 #include "crypto.h"
@@ -166,6 +171,114 @@ static ssize_t qcow2_zlib_decompress(void *de

Re: [PATCH v7 03/11] scripts: add coccinelle script to use auto propagated errp

[Vladimir Sementsov-Ogievskiy]

23.02.2020 11:55, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  writes:


Script adds ERRP_AUTO_PROPAGATE macro invocation where appropriate and
does corresponding changes in code (look for details in
include/qapi/error.h)

Usage example:
spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
  --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
  blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]

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

CC: Eric Blake 
CC: Kevin Wolf 
CC: Max Reitz 
CC: Greg Kurz 
CC: Stefano Stabellini 
CC: Anthony Perard 
CC: Paul Durrant 
CC: Stefan Hajnoczi 
CC: "Philippe Mathieu-Daudé" 
CC: Laszlo Ersek 
CC: Gerd Hoffmann 
CC: Stefan Berger 
CC: Markus Armbruster 
CC: Michael Roth 
CC: qemu-block@nongnu.org
CC: xen-de...@lists.xenproject.org

  include/qapi/error.h  |   3 +
  scripts/coccinelle/auto-propagated-errp.cocci | 158 ++
  2 files changed, 161 insertions(+)
  create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci

diff --git a/include/qapi/error.h b/include/qapi/error.h
index b9452d4806..79f8e95214 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -141,6 +141,9 @@
   * ...
   * }
   *
+ * For mass conversion use script
+ *   scripts/coccinelle/auto-propagated-errp.cocci
+ *
   *
   * Receive and accumulate multiple errors (first one wins):
   * Error *err = NULL, *local_err = NULL;


Extra blank line.


diff --git a/scripts/coccinelle/auto-propagated-errp.cocci 
b/scripts/coccinelle/auto-propagated-errp.cocci
new file mode 100644
index 00..fb03c871cb
--- /dev/null
+++ b/scripts/coccinelle/auto-propagated-errp.cocci
@@ -0,0 +1,158 @@
+// Use ERRP_AUTO_PROPAGATE (see include/qapi/error.h)
+//
+// 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 .
+//
+// Usage example:
+// spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
+//  --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
+//  blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
+
+@rule0@
+// Add invocation to errp-functions where necessary
+// We should skip functions with "Error *const *errp"
+// parameter, but how to do it with coccinelle?
+// I don't know, so, I skip them by function name regex.
+// It's safe: if we did not skip some functions with
+// "Error *const *errp", ERRP_AUTO_PROPAGATE invocation
+// will fail to compile, because of const violation.


Not skipping a function we should skip fails to compile.

What about skipping a function we should not skip?


+identifier fn !~ "error_append_.*_hint";
+identifier local_err, ERRP;


A few of our coccinelle scripts use ALL_CAPS for meta-variables.  Most
don't.  Either is fine with me.  Mixing the two styles feels a bit
confusing, though.


+@@
+
+ fn(..., Error **ERRP, ...)
+ {
++   ERRP_AUTO_PROPAGATE();
+<+...
+when != ERRP_AUTO_PROPAGATE();
+(
+error_append_hint(ERRP, ...);
+|
+error_prepend(ERRP, ...);
+|
+Error *local_err = NULL;
+)
+...+>
+ }


Misses error_vprepend().  Currently harmless, but as long as we commit
the script, we better make it as robust as we reasonably can.

The previous patch explains this Coccinelle script's intent:

   To achieve these goals, later patches will add invocations
   of this macro at the start of functions with either use
   error_prepend/error_append_hint (solving 1) or which use
   local_err+error_propagate to check errors, switching those
   functions to use *errp instead (solving 2 and 3).

This rule matches "use error_prepend/error_append_hint" directly.  It
appears to use presence of a local Error * variable as proxy for "use
local_err+error_propagate to check errors".  Hmm.

We obviously have such a variable when we use "local_err+error_propagate
to check errors".  But we could also have such variables without use of
error_propagate().  In fact, error.h documents such use:

  * Call a function and receive an error from it:
  * Error *err = NULL;
  * foo(arg, &err);
  * if (err) {
  * handle the error...
  * }

where "handle the error" frees it.

I figure such uses typically occur in functions without an Error **errp
parameter.  This rule doesn't apply then.  But they could occur even in
functions with such a parameter. 

Re: [PATCH 1/2] misc: Replace zero-length arrays with flexible array member (automatic)

[Paolo Bonzini]

On 04/03/20 14:12, Philippe Mathieu-Daudé wrote:
> 
> hw/scsi/spapr_vscsi.c:69:29: error: field 'iu' with variable sized type
> 'union viosrp_iu' not at the end of a struct or class is a GNU extension
> [-Werror,-Wgnu-variable-sized-type-not-at-end]
>     union viosrp_iu iu;
>     ^
> 
> Yay we found a bug! Thanks Gustavo :)
> 
> union srp_iu {
>     struct srp_login_req login_req;
>     struct srp_login_rsp login_rsp;
>     struct srp_login_rej login_rej;
>     struct srp_i_logout i_logout;
>     struct srp_t_logout t_logout;
>     struct srp_tsk_mgmt tsk_mgmt;
>     struct srp_cmd cmd;
>     struct srp_rsp rsp;
>     uint8_t reserved[SRP_MAX_IU_LEN];
> };

It's variable-sized but it's okay as long as the total size doesn't
exceed SRP_MAX_IU_LEN.  So it's not a bug, but I agree it's a time bomb.
 Moving the field last should work, but it would still be quite
dangerous code.

Paolo

Re: [PATCH 1/2] misc: Replace zero-length arrays with flexible array member (automatic)

[Philippe Mathieu-Daudé]

On 3/4/20 2:44 PM, Paolo Bonzini wrote:

On 04/03/20 14:12, Philippe Mathieu-Daudé wrote:


hw/scsi/spapr_vscsi.c:69:29: error: field 'iu' with variable sized type
'union viosrp_iu' not at the end of a struct or class is a GNU extension
[-Werror,-Wgnu-variable-sized-type-not-at-end]
     union viosrp_iu iu;
     ^

Yay we found a bug! Thanks Gustavo :)

union srp_iu {
     struct srp_login_req login_req;
     struct srp_login_rsp login_rsp;
     struct srp_login_rej login_rej;
     struct srp_i_logout i_logout;
     struct srp_t_logout t_logout;
     struct srp_tsk_mgmt tsk_mgmt;
     struct srp_cmd cmd;
     struct srp_rsp rsp;
     uint8_t reserved[SRP_MAX_IU_LEN];
};


It's variable-sized but it's okay as long as the total size doesn't
exceed SRP_MAX_IU_LEN.  So it's not a bug, but I agree it's a time bomb.
  Moving the field last should work, but it would still be quite
dangerous code.


Yeah I reached the same conclusion.

I'll send a fix for the dangerous code.
Do you want to drop this series, or only the change in 'struct srp_rsp' 
(or in all hw/scsi/srp.h). Actually I guess it makes sense I move the 
'hw/scsi/srp.h' changes with the series cleaning dangerous code.

Re: [PATCH 1/2] misc: Replace zero-length arrays with flexible array member (automatic)

[Paolo Bonzini]

On 04/03/20 15:12, Philippe Mathieu-Daudé wrote:
> I'll send a fix for the dangerous code.
> Do you want to drop this series, or only the change in 'struct srp_rsp'
> (or in all hw/scsi/srp.h). Actually I guess it makes sense I move the
> 'hw/scsi/srp.h' changes with the series cleaning dangerous code.

As you prefer, it's not urgent to merge it.

Paolo

Re: [PATCH] block/qcow2-threads: fix qcow2_decompress

[Ján Tomko]

On a Monday in 2020, Vladimir Sementsov-Ogievskiy wrote:

On success path we return what inflate() returns instead of 0. And it
most probably works for Z_STREAM_END as it is positive, but is
definitely broken for Z_BUF_ERROR.

While being here, switch to errno return code, to be closer to
qcow2_compress API (and usual expectations).

Revert condition in if to be more positive. Drop dead initialization of
ret.

Cc: qemu-sta...@nongnu.org # v4.0
Fixes: 341926ab83e2b
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---

Hi!

Reviewing Den's series about zstd in qcow2 support, I found an existing
bug. Let's fix it. This is to be a new base of zstd series.

block/qcow2-threads.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH v9 0/4] delete created files when block_crypto_co_create_opts_luks fails

[Daniel Henrique Barboza]

Ping

On 1/30/20 6:39 PM, Daniel Henrique Barboza wrote:

The version 8 of this patch series got buried and it's now
conflicting with master. Rebase and re-sending it.

Also, I contemplated the idea of moving/copying the password
verification in qcrypto_block_luks_create() all the way back to the
start of block_crypto_co_create_opts_luks(), failing early before the
bdrv_create_file(), avoiding the problem altogether without the
need of a delete_file API I'm trying to push here (see patch 03
commit message for detailed info about the bug).

This idea was dropped after I saw that:

- We would need to store the resulting password, now being retrieved
early in block_crypto_co_create_opts_luks(), in a new
QCryptoBlockCreateOptions string to be used inside
qcrypto_block_luks_create() as intended. An alternative would be to
call qcrypto_secret_lookup_as_utf8() twice, discarding the first
string;

- There are a lot of ways to fail in qcrypto_block_luks_create()
other than a non-UTF8 password that would trigger the same problem.
A more appropiate way of doing what I intended, instead of
copying/hacking code around to fail before bdrv_create(), is some sort
of bdrv_validate() API that would encapsulate everything that is
related to user input validation for the security drivers. This
API could then be called before the file creation (maybe inside
bdrv_create itself) and fail early if needed. This is too overkill
for what I'm trying to fix here, and I'm not sure if it would be
a net gain compared to the delete_file API.


All that said, I believe that this patch series presents a sane
solution with the code we have ATM.


changes in this version:
- rebase with current master at 204aa60b37
- previous version:
https://lists.gnu.org/archive/html/qemu-devel/2019-11/msg01551.html


Daniel Henrique Barboza (4):
   block: introducing 'bdrv_co_delete_file' interface
   block.c: adding bdrv_co_delete_file
   crypto.c: cleanup created file when block_crypto_co_create_opts_luks
 fails
   qemu-iotests: adding LUKS cleanup for non-UTF8 secret error

  block.c| 26 +++
  block/crypto.c | 18 ++
  block/file-posix.c | 23 +
  include/block/block.h  |  1 +
  include/block/block_int.h  |  4 +++
  tests/qemu-iotests/282 | 67 ++
  tests/qemu-iotests/282.out | 11 +++
  tests/qemu-iotests/group   |  1 +
  8 files changed, 151 insertions(+)
  create mode 100755 tests/qemu-iotests/282
  create mode 100644 tests/qemu-iotests/282.out

Re: [PATCH v5 2/5] qcow2: introduce compression type feature

[Markus Armbruster]

Denis Plotnikov  writes:

> The patch adds some preparation parts for incompatible compression type
> feature to qcow2 allowing the use different compression methods for
> image clusters (de)compressing.
>
> It is implied that the compression type is set on the image creation and
> can be changed only later by image conversion, thus compression type
> defines the only compression algorithm used for the image, and thus,
> for all image clusters.
>
> The goal of the feature is to add support of other compression methods
> to qcow2. For example, ZSTD which is more effective on compression than ZLIB.
>
> The default compression is ZLIB. Images created with ZLIB compression type
> are backward compatible with older qemu versions.
>
> Adding of the compression type breaks a number of tests because now the
> compression type is reported on image creation and there are some changes
> in the qcow2 header in size and offsets.
>
> The tests are fixed in the following ways:
> * filter out compression_type for all the tests
> * fix header size, feature table size and backing file offset
>   affected tests: 031, 036, 061, 080
>   header_size +=8: 1 byte compression type
>7 bytes padding
>   feature_table += 48: incompatible feture compression type
>   backing_file_offset += 56 (8 + 48 -> header_change + 
> fature_table_change)
> * add "compression type" for test output matching when it isn't filtered
>   affected tests: 049, 060, 061, 065, 144, 182, 242, 255
>
> Signed-off-by: Denis Plotnikov 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 
> ---
>  qapi/block-core.json |  22 ++-
>  block/qcow2.h|  18 +-
>  include/block/block_int.h|   1 +
>  block/qcow2.c| 101 ++
>  tests/qemu-iotests/031.out   |  14 ++---
>  tests/qemu-iotests/036.out   |   4 +-
>  tests/qemu-iotests/049.out   | 102 +++
>  tests/qemu-iotests/060.out   |   1 +
>  tests/qemu-iotests/061.out   |  34 ++-
>  tests/qemu-iotests/065   |  28 ++---
>  tests/qemu-iotests/080   |   2 +-
>  tests/qemu-iotests/144.out   |   4 +-
>  tests/qemu-iotests/182.out   |   2 +-
>  tests/qemu-iotests/242.out   |   5 ++
>  tests/qemu-iotests/255.out   |   8 +--
>  tests/qemu-iotests/common.filter |   3 +-
>  16 files changed, 253 insertions(+), 96 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 85e27bb61f..a67eb8bff4 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -78,6 +78,8 @@
>  #
>  # @bitmaps: A list of qcow2 bitmap details (since 4.0)
>  #
> +# @compression-type: the image cluster compression method (since 5.0)
> +#
>  # Since: 1.7
>  ##
>  { 'struct': 'ImageInfoSpecificQCow2',
> @@ -89,7 +91,8 @@
>'*corrupt': 'bool',
>'refcount-bits': 'int',
>'*encrypt': 'ImageInfoSpecificQCow2Encryption',
> -  '*bitmaps': ['Qcow2BitmapInfo']
> +  '*bitmaps': ['Qcow2BitmapInfo'],
> +  'compression-type': 'Qcow2CompressionType'
>} }
>  
>  ##
> @@ -4392,6 +4395,18 @@
>'data': [ 'v2', 'v3' ] }
>  
>  
> +##
> +# @Qcow2CompressionType:
> +#
> +# Compression type used in qcow2 image file
> +#
> +# @zlib:  zlib compression, see 
> +#
> +# Since: 5.0
> +##
> +{ 'enum': 'Qcow2CompressionType',
> +  'data': [ 'zlib' ] }
> +
>  ##
>  # @BlockdevCreateOptionsQcow2:
>  #
> @@ -4415,6 +4430,8 @@
>  # allowed values: off, falloc, full, metadata)
>  # @lazy-refcounts: True if refcounts may be updated lazily (default: off)
>  # @refcount-bits: Width of reference counts in bits (default: 16)
> +# @compression-type: The image cluster compression method
> +#(default: zlib, since 5.0)
>  #
>  # Since: 2.12
>  ##
> @@ -4430,7 +4447,8 @@
>  '*cluster-size':'size',
>  '*preallocation':   'PreallocMode',
>  '*lazy-refcounts':  'bool',
> -'*refcount-bits':   'int' } }
> +'*refcount-bits':   'int',
> +'*compression-type':'Qcow2CompressionType' } }
>  
>  ##
>  # @BlockdevCreateOptionsQed:
[...]

QAPI part:
Acked-by: Markus Armbruster 

Re: [PATCH v7 03/11] scripts: add coccinelle script to use auto propagated errp

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy  writes:

> 23.02.2020 11:55, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy  writes:
>>
>>> Script adds ERRP_AUTO_PROPAGATE macro invocation where appropriate and
>>> does corresponding changes in code (look for details in
>>> include/qapi/error.h)
>>>
>>> Usage example:
>>> spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>>>   --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>>>   blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>> ---
>>>
>>> CC: Eric Blake 
>>> CC: Kevin Wolf 
>>> CC: Max Reitz 
>>> CC: Greg Kurz 
>>> CC: Stefano Stabellini 
>>> CC: Anthony Perard 
>>> CC: Paul Durrant 
>>> CC: Stefan Hajnoczi 
>>> CC: "Philippe Mathieu-Daudé" 
>>> CC: Laszlo Ersek 
>>> CC: Gerd Hoffmann 
>>> CC: Stefan Berger 
>>> CC: Markus Armbruster 
>>> CC: Michael Roth 
>>> CC: qemu-block@nongnu.org
>>> CC: xen-de...@lists.xenproject.org
>>>
>>>   include/qapi/error.h  |   3 +
>>>   scripts/coccinelle/auto-propagated-errp.cocci | 158 ++
>>>   2 files changed, 161 insertions(+)
>>>   create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
>>>
>>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>>> index b9452d4806..79f8e95214 100644
>>> --- a/include/qapi/error.h
>>> +++ b/include/qapi/error.h
>>> @@ -141,6 +141,9 @@
>>>* ...
>>>* }
>>>*
>>> + * For mass conversion use script
>>> + *   scripts/coccinelle/auto-propagated-errp.cocci
>>> + *
>>>*
>>>* Receive and accumulate multiple errors (first one wins):
>>>* Error *err = NULL, *local_err = NULL;
>>
>> Extra blank line.
>>
>>> diff --git a/scripts/coccinelle/auto-propagated-errp.cocci 
>>> b/scripts/coccinelle/auto-propagated-errp.cocci
>>> new file mode 100644
>>> index 00..fb03c871cb
>>> --- /dev/null
>>> +++ b/scripts/coccinelle/auto-propagated-errp.cocci
>>> @@ -0,0 +1,158 @@
>>> +// Use ERRP_AUTO_PROPAGATE (see include/qapi/error.h)
>>> +//
>>> +// 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 .
>>> +//
>>> +// Usage example:
>>> +// spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>>> +//  --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>>> +//  blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>>> +
>>> +@rule0@
>>> +// Add invocation to errp-functions where necessary
>>> +// We should skip functions with "Error *const *errp"
>>> +// parameter, but how to do it with coccinelle?
>>> +// I don't know, so, I skip them by function name regex.
>>> +// It's safe: if we did not skip some functions with
>>> +// "Error *const *errp", ERRP_AUTO_PROPAGATE invocation
>>> +// will fail to compile, because of const violation.
>>
>> Not skipping a function we should skip fails to compile.
>>
>> What about skipping a function we should not skip?
>>
>>> +identifier fn !~ "error_append_.*_hint";
>>> +identifier local_err, ERRP;
>>
>> A few of our coccinelle scripts use ALL_CAPS for meta-variables.  Most
>> don't.  Either is fine with me.  Mixing the two styles feels a bit
>> confusing, though.
>>
>>> +@@
>>> +
>>> + fn(..., Error **ERRP, ...)
>>> + {
>>> ++   ERRP_AUTO_PROPAGATE();
>>> +<+...
>>> +when != ERRP_AUTO_PROPAGATE();
>>> +(
>>> +error_append_hint(ERRP, ...);
>>> +|
>>> +error_prepend(ERRP, ...);
>>> +|
>>> +Error *local_err = NULL;
>>> +)
>>> +...+>
>>> + }
>>
>> Misses error_vprepend().  Currently harmless, but as long as we commit
>> the script, we better make it as robust as we reasonably can.
>>
>> The previous patch explains this Coccinelle script's intent:
>>
>>To achieve these goals, later patches will add invocations
>>of this macro at the start of functions with either use
>>error_prepend/error_append_hint (solving 1) or which use
>>local_err+error_propagate to check errors, switching those
>>functions to use *errp instead (solving 2 and 3).
>>
>> This rule matches "use error_prepend/error_append_hint" directly.  It
>> appears to use presence of a local Error * variable as proxy for "use
>> local_err+error_propagate to check errors".  Hmm.
>>
>> We obviously have such a variable when we use "local_err+e

[PATCH 0/2] misc: Replace zero-length arrays with flexible array member

[Philippe Mathieu-Daudé]

v2:
- do not modify qed.h (structure with single member)
- based on hw/scsi/spapr_vscsi fix series

This is a tree-wide cleanup inspired by a Linux kernel commit
(from Gustavo A. R. Silva).

--v-- description start --v--

  The current codebase makes use of the zero-length array language
  extension to the C90 standard, but the preferred mechanism to
  declare variable-length types such as these ones is a flexible
  array member [1], introduced in C99:

  struct foo {
  int stuff;
  struct boo array[];
  };

  By making use of the mechanism above, we will get a compiler
  warning in case the flexible array does not occur last in the
  structure, which will help us prevent some kind of undefined
  behavior bugs from being unadvertenly introduced [2] to the
  Linux codebase from now on.

--^-- description end --^--

Do the similar housekeeping in the QEMU codebase (which uses
C99 since commit 7be41675f7cb).

The first patch is done with the help of a coccinelle semantic
patch. However Coccinelle does not recognize:

  struct foo {
  int stuff;
  struct boo array[];
  } QEMU_PACKED;

but does recognize:

  struct QEMU_PACKED foo {
  int stuff;
  struct boo array[];
  };

I'm not sure why, neither it is worth refactoring all QEMU
structures to use the attributes before the structure name,
so I did the 2nd patch manually.

Anyway this is annoying, because many structures are not handled
by coccinelle. Maybe this needs to be reported to upstream
coccinelle?

I used spatch 1.0.8 with:

  -I include --include-headers \
  --macro-file scripts/cocci-macro-file.h \
  --keep-comments --indent 4

Regards,

Phil.

Based-on: <20200304153311.22959-1-phi...@redhat.com>
Supersedes: <20200304005105.27454-1-phi...@redhat.com>

Philippe Mathieu-Daudé (2):
  misc: Replace zero-length arrays with flexible array member
(automatic)
  misc: Replace zero-length arrays with flexible array member (manual)

 docs/interop/vhost-user.rst   |  4 ++--
 bsd-user/qemu.h   |  2 +-
 contrib/libvhost-user/libvhost-user.h |  2 +-
 hw/m68k/bootinfo.h|  2 +-
 hw/scsi/srp.h |  6 +++---
 hw/xen/xen_pt.h   |  2 +-
 include/hw/acpi/acpi-defs.h   | 16 
 include/hw/arm/smmu-common.h  |  2 +-
 include/hw/boards.h   |  2 +-
 include/hw/i386/intel_iommu.h |  3 ++-
 include/hw/s390x/event-facility.h |  2 +-
 include/hw/s390x/sclp.h   |  8 
 include/hw/virtio/virtio-iommu.h  |  2 +-
 include/sysemu/cryptodev.h|  2 +-
 include/tcg/tcg.h |  2 +-
 pc-bios/s390-ccw/bootmap.h|  2 +-
 pc-bios/s390-ccw/sclp.h   |  2 +-
 tests/qtest/libqos/ahci.h |  2 +-
 block/linux-aio.c |  2 +-
 block/vmdk.c  |  2 +-
 hw/acpi/nvdimm.c  |  6 +++---
 hw/char/sclpconsole-lm.c  |  2 +-
 hw/char/sclpconsole.c |  2 +-
 hw/dma/soc_dma.c  |  2 +-
 hw/i386/x86.c |  2 +-
 hw/misc/omap_l4.c |  2 +-
 hw/nvram/eeprom93xx.c |  2 +-
 hw/rdma/vmw/pvrdma_qp_ops.c   |  4 ++--
 hw/s390x/virtio-ccw.c |  2 +-
 hw/usb/dev-network.c  |  2 +-
 hw/usb/dev-smartcard-reader.c |  4 ++--
 hw/virtio/virtio.c|  4 ++--
 net/queue.c   |  2 +-
 target/s390x/ioinst.c |  2 +-
 34 files changed, 53 insertions(+), 52 deletions(-)

-- 
2.21.1

Re: [PATCH 0/2] misc: Replace zero-length arrays with flexible array member

[Philippe Mathieu-Daudé]

On 3/4/20 4:35 PM, Philippe Mathieu-Daudé wrote:

v2:
- do not modify qed.h (structure with single member)
- based on hw/scsi/spapr_vscsi fix series

This is a tree-wide cleanup inspired by a Linux kernel commit
(from Gustavo A. R. Silva).


Please ignore, for some reason the 'v2' tag is missing.

[PATCH 1/2] misc: Replace zero-length arrays with flexible array member (automatic)

[Philippe Mathieu-Daudé]

Description copied from Linux kernel commit from Gustavo A. R. Silva
(see [3]):

--v-- description start --v--

  The current codebase makes use of the zero-length array language
  extension to the C90 standard, but the preferred mechanism to
  declare variable-length types such as these ones is a flexible
  array member [1], introduced in C99:

  struct foo {
  int stuff;
  struct boo array[];
  };

  By making use of the mechanism above, we will get a compiler
  warning in case the flexible array does not occur last in the
  structure, which will help us prevent some kind of undefined
  behavior bugs from being unadvertenly introduced [2] to the
  Linux codebase from now on.

--^-- description end --^--

Do the similar housekeeping in the QEMU codebase (which uses
C99 since commit 7be41675f7cb).

All these instances of code were found with the help of the
following Coccinelle script:

  @@
  identifier s, m, a;
  type t, T;
  @@
   struct s {
  ...
  t m;
  -   T a[0];
  +   T a[];
  };
  @@
  identifier s, m, a;
  type t, T;
  @@
   struct s {
  ...
  t m;
  -   T a[0];
  +   T a[];
   } QEMU_PACKED;

[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=76497732932f
[3] 
https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?id=17642a2fbd2c1

Inspired-by: Gustavo A. R. Silva 
Reviewed-by: David Hildenbrand 
Signed-off-by: Philippe Mathieu-Daudé 
---
v2: cocci script updated to not match structures of onlyi
a single flexible array member:

  block/qed.h:106:14: error: flexible array member 'offsets' not allowed in 
otherwise empty struct
  uint64_t offsets[]; /* in bytes */
   ^
---
 bsd-user/qemu.h   |  2 +-
 contrib/libvhost-user/libvhost-user.h |  2 +-
 hw/m68k/bootinfo.h|  2 +-
 hw/scsi/srp.h |  6 +++---
 hw/xen/xen_pt.h   |  2 +-
 include/hw/acpi/acpi-defs.h   | 12 ++--
 include/hw/arm/smmu-common.h  |  2 +-
 include/hw/i386/intel_iommu.h |  3 ++-
 include/hw/virtio/virtio-iommu.h  |  2 +-
 include/sysemu/cryptodev.h|  2 +-
 include/tcg/tcg.h |  2 +-
 pc-bios/s390-ccw/bootmap.h|  2 +-
 pc-bios/s390-ccw/sclp.h   |  2 +-
 tests/qtest/libqos/ahci.h |  2 +-
 block/linux-aio.c |  2 +-
 hw/acpi/nvdimm.c  |  6 +++---
 hw/dma/soc_dma.c  |  2 +-
 hw/i386/x86.c |  2 +-
 hw/misc/omap_l4.c |  2 +-
 hw/nvram/eeprom93xx.c |  2 +-
 hw/rdma/vmw/pvrdma_qp_ops.c   |  4 ++--
 hw/usb/dev-network.c  |  2 +-
 hw/usb/dev-smartcard-reader.c |  4 ++--
 hw/virtio/virtio.c|  4 ++--
 net/queue.c   |  2 +-
 25 files changed, 38 insertions(+), 37 deletions(-)

diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
index 09e8aed9c7..f8bb1e5459 100644
--- a/bsd-user/qemu.h
+++ b/bsd-user/qemu.h
@@ -95,7 +95,7 @@ typedef struct TaskState {
 struct sigqueue *first_free; /* first free siginfo queue entry */
 int signal_pending; /* non zero if a signal may be pending */
 
-uint8_t stack[0];
+uint8_t stack[];
 } __attribute__((aligned(16))) TaskState;
 
 void init_task_state(TaskState *ts);
diff --git a/contrib/libvhost-user/libvhost-user.h 
b/contrib/libvhost-user/libvhost-user.h
index 6fc8000e99..f30394fab6 100644
--- a/contrib/libvhost-user/libvhost-user.h
+++ b/contrib/libvhost-user/libvhost-user.h
@@ -286,7 +286,7 @@ typedef struct VuVirtqInflight {
 uint16_t used_idx;
 
 /* Used to track the state of each descriptor in descriptor table */
-VuDescStateSplit desc[0];
+VuDescStateSplit desc[];
 } VuVirtqInflight;
 
 typedef struct VuVirtqInflightDesc {
diff --git a/hw/m68k/bootinfo.h b/hw/m68k/bootinfo.h
index 5f8ded2686..c954270aad 100644
--- a/hw/m68k/bootinfo.h
+++ b/hw/m68k/bootinfo.h
@@ -14,7 +14,7 @@
 struct bi_record {
 uint16_t tag;/* tag ID */
 uint16_t size;   /* size of record */
-uint32_t data[0];/* data */
+uint32_t data[]; /* data */
 };
 
 /* machine independent tags */
diff --git a/hw/scsi/srp.h b/hw/scsi/srp.h
index d27f31d2d5..54c954badd 100644
--- a/hw/scsi/srp.h
+++ b/hw/scsi/srp.h
@@ -112,7 +112,7 @@ struct srp_direct_buf {
 struct srp_indirect_buf {
 struct srp_direct_buftable_desc;
 uint32_t len;
-struct srp_direct_bufdesc_list[0];
+struct srp_direct_bufdesc_list[];
 } QEMU_PACKED;
 
 enum {
@@ -211,7 +211,7 @@ struct srp_cmd {
 uint8_treserved4;
 uint8_tadd_cdb_len;
 uint8_tcdb[16];
-uint8_tadd_data[0];
+uint8_tadd_data[];
 } QEMU_PACKED;
 
 enum {
@@ -241,7 +241,7 @@ struct srp_rsp {
 uint32_t   data_in_res_cnt;
 uint32_t   s

[PATCH v2 2/2] misc: Replace zero-length arrays with flexible array member (manual)

[Philippe Mathieu-Daudé]

Description copied from Linux kernel commit from Gustavo A. R. Silva
(see [3]):

--v-- description start --v--

  The current codebase makes use of the zero-length array language
  extension to the C90 standard, but the preferred mechanism to
  declare variable-length types such as these ones is a flexible
  array member [1], introduced in C99:

  struct foo {
  int stuff;
  struct boo array[];
  };

  By making use of the mechanism above, we will get a compiler
  warning in case the flexible array does not occur last in the
  structure, which will help us prevent some kind of undefined
  behavior bugs from being unadvertenly introduced [2] to the
  Linux codebase from now on.

--^-- description end --^--

Do the similar housekeeping in the QEMU codebase (which uses
C99 since commit 7be41675f7cb).

All these instances of code were found with the help of the
following command (then manual analysis, without modifying
structures only having a single flexible array member, such
QEDTable in block/qed.h):

  git grep -F '[0];'

[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=76497732932f
[3] 
https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?id=17642a2fbd2c1

Inspired-by: Gustavo A. R. Silva 
Reviewed-by: David Hildenbrand 
Signed-off-by: Philippe Mathieu-Daudé 
---
v2: Do not modify block/qed.h:

  block/qed.h:106:14: error: flexible array member 'offsets' not allowed in 
otherwise empty struct
  uint64_t offsets[]; /* in bytes */
   ^
---
 docs/interop/vhost-user.rst   | 4 ++--
 include/hw/acpi/acpi-defs.h   | 4 ++--
 include/hw/boards.h   | 2 +-
 include/hw/s390x/event-facility.h | 2 +-
 include/hw/s390x/sclp.h   | 8 
 block/vmdk.c  | 2 +-
 hw/char/sclpconsole-lm.c  | 2 +-
 hw/char/sclpconsole.c | 2 +-
 hw/s390x/virtio-ccw.c | 2 +-
 target/s390x/ioinst.c | 2 +-
 10 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
index 401652397c..3b1b6602c7 100644
--- a/docs/interop/vhost-user.rst
+++ b/docs/interop/vhost-user.rst
@@ -568,7 +568,7 @@ For split virtqueue, queue region can be implemented as:
   uint16_t used_idx;
 
   /* Used to track the state of each descriptor in descriptor table */
-  DescStateSplit desc[0];
+  DescStateSplit desc[];
   } QueueRegionSplit;
 
 To track inflight I/O, the queue region should be processed as follows:
@@ -690,7 +690,7 @@ For packed virtqueue, queue region can be implemented as:
   uint8_t padding[7];
 
   /* Used to track the state of each descriptor fetched from descriptor 
ring */
-  DescStatePacked desc[0];
+  DescStatePacked desc[];
   } QueueRegionPacked;
 
 To track inflight I/O, the queue region should be processed as follows:
diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index 19f7ba7b70..c13327fa78 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -152,7 +152,7 @@ typedef struct AcpiSerialPortConsoleRedirection
  */
 struct AcpiRsdtDescriptorRev1 {
 ACPI_TABLE_HEADER_DEF   /* ACPI common table header */
-uint32_t table_offset_entry[0];  /* Array of pointers to other */
+uint32_t table_offset_entry[];  /* Array of pointers to other */
 /* ACPI tables */
 } QEMU_PACKED;
 typedef struct AcpiRsdtDescriptorRev1 AcpiRsdtDescriptorRev1;
@@ -162,7 +162,7 @@ typedef struct AcpiRsdtDescriptorRev1 
AcpiRsdtDescriptorRev1;
  */
 struct AcpiXsdtDescriptorRev2 {
 ACPI_TABLE_HEADER_DEF   /* ACPI common table header */
-uint64_t table_offset_entry[0];  /* Array of pointers to other */
+uint64_t table_offset_entry[];  /* Array of pointers to other */
 /* ACPI tables */
 } QEMU_PACKED;
 typedef struct AcpiXsdtDescriptorRev2 AcpiXsdtDescriptorRev2;
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 9bc42dfb22..c96120d15f 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -71,7 +71,7 @@ typedef struct CPUArchId {
  */
 typedef struct {
 int len;
-CPUArchId cpus[0];
+CPUArchId cpus[];
 } CPUArchIdList;
 
 /**
diff --git a/include/hw/s390x/event-facility.h 
b/include/hw/s390x/event-facility.h
index bdc32a3c09..700a610f33 100644
--- a/include/hw/s390x/event-facility.h
+++ b/include/hw/s390x/event-facility.h
@@ -122,7 +122,7 @@ typedef struct MDBO {
 
 typedef struct MDB {
 MdbHeader header;
-MDBO mdbo[0];
+MDBO mdbo[];
 } QEMU_PACKED MDB;
 
 typedef struct SclpMsg {
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index c54413b78c..cd7b24359f 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -132,7 +132,7 @@ typedef struct ReadInfo {
 uint16_t highest_cpu;
 uint8_t  _reserved5[124 - 122]; /* 122-123 */
 uint32_t hmfai;
-struct CPUEntry entries[0];
+   

[PATCH v2 1/2] misc: Replace zero-length arrays with flexible array member (automatic)

[Philippe Mathieu-Daudé]

Description copied from Linux kernel commit from Gustavo A. R. Silva
(see [3]):

--v-- description start --v--

  The current codebase makes use of the zero-length array language
  extension to the C90 standard, but the preferred mechanism to
  declare variable-length types such as these ones is a flexible
  array member [1], introduced in C99:

  struct foo {
  int stuff;
  struct boo array[];
  };

  By making use of the mechanism above, we will get a compiler
  warning in case the flexible array does not occur last in the
  structure, which will help us prevent some kind of undefined
  behavior bugs from being unadvertenly introduced [2] to the
  Linux codebase from now on.

--^-- description end --^--

Do the similar housekeeping in the QEMU codebase (which uses
C99 since commit 7be41675f7cb).

All these instances of code were found with the help of the
following Coccinelle script:

  @@
  identifier s, m, a;
  type t, T;
  @@
   struct s {
  ...
  t m;
  -   T a[0];
  +   T a[];
  };
  @@
  identifier s, m, a;
  type t, T;
  @@
   struct s {
  ...
  t m;
  -   T a[0];
  +   T a[];
   } QEMU_PACKED;

[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=76497732932f
[3] 
https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?id=17642a2fbd2c1

Inspired-by: Gustavo A. R. Silva 
Reviewed-by: David Hildenbrand 
Signed-off-by: Philippe Mathieu-Daudé 
---
v2: cocci script updated to not match structures of onlyi
a single flexible array member:

  block/qed.h:106:14: error: flexible array member 'offsets' not allowed in 
otherwise empty struct
  uint64_t offsets[]; /* in bytes */
   ^
---
 bsd-user/qemu.h   |  2 +-
 contrib/libvhost-user/libvhost-user.h |  2 +-
 hw/m68k/bootinfo.h|  2 +-
 hw/scsi/srp.h |  6 +++---
 hw/xen/xen_pt.h   |  2 +-
 include/hw/acpi/acpi-defs.h   | 12 ++--
 include/hw/arm/smmu-common.h  |  2 +-
 include/hw/i386/intel_iommu.h |  3 ++-
 include/hw/virtio/virtio-iommu.h  |  2 +-
 include/sysemu/cryptodev.h|  2 +-
 include/tcg/tcg.h |  2 +-
 pc-bios/s390-ccw/bootmap.h|  2 +-
 pc-bios/s390-ccw/sclp.h   |  2 +-
 tests/qtest/libqos/ahci.h |  2 +-
 block/linux-aio.c |  2 +-
 hw/acpi/nvdimm.c  |  6 +++---
 hw/dma/soc_dma.c  |  2 +-
 hw/i386/x86.c |  2 +-
 hw/misc/omap_l4.c |  2 +-
 hw/nvram/eeprom93xx.c |  2 +-
 hw/rdma/vmw/pvrdma_qp_ops.c   |  4 ++--
 hw/usb/dev-network.c  |  2 +-
 hw/usb/dev-smartcard-reader.c |  4 ++--
 hw/virtio/virtio.c|  4 ++--
 net/queue.c   |  2 +-
 25 files changed, 38 insertions(+), 37 deletions(-)

diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
index 09e8aed9c7..f8bb1e5459 100644
--- a/bsd-user/qemu.h
+++ b/bsd-user/qemu.h
@@ -95,7 +95,7 @@ typedef struct TaskState {
 struct sigqueue *first_free; /* first free siginfo queue entry */
 int signal_pending; /* non zero if a signal may be pending */
 
-uint8_t stack[0];
+uint8_t stack[];
 } __attribute__((aligned(16))) TaskState;
 
 void init_task_state(TaskState *ts);
diff --git a/contrib/libvhost-user/libvhost-user.h 
b/contrib/libvhost-user/libvhost-user.h
index 6fc8000e99..f30394fab6 100644
--- a/contrib/libvhost-user/libvhost-user.h
+++ b/contrib/libvhost-user/libvhost-user.h
@@ -286,7 +286,7 @@ typedef struct VuVirtqInflight {
 uint16_t used_idx;
 
 /* Used to track the state of each descriptor in descriptor table */
-VuDescStateSplit desc[0];
+VuDescStateSplit desc[];
 } VuVirtqInflight;
 
 typedef struct VuVirtqInflightDesc {
diff --git a/hw/m68k/bootinfo.h b/hw/m68k/bootinfo.h
index 5f8ded2686..c954270aad 100644
--- a/hw/m68k/bootinfo.h
+++ b/hw/m68k/bootinfo.h
@@ -14,7 +14,7 @@
 struct bi_record {
 uint16_t tag;/* tag ID */
 uint16_t size;   /* size of record */
-uint32_t data[0];/* data */
+uint32_t data[]; /* data */
 };
 
 /* machine independent tags */
diff --git a/hw/scsi/srp.h b/hw/scsi/srp.h
index d27f31d2d5..54c954badd 100644
--- a/hw/scsi/srp.h
+++ b/hw/scsi/srp.h
@@ -112,7 +112,7 @@ struct srp_direct_buf {
 struct srp_indirect_buf {
 struct srp_direct_buftable_desc;
 uint32_t len;
-struct srp_direct_bufdesc_list[0];
+struct srp_direct_bufdesc_list[];
 } QEMU_PACKED;
 
 enum {
@@ -211,7 +211,7 @@ struct srp_cmd {
 uint8_treserved4;
 uint8_tadd_cdb_len;
 uint8_tcdb[16];
-uint8_tadd_data[0];
+uint8_tadd_data[];
 } QEMU_PACKED;
 
 enum {
@@ -241,7 +241,7 @@ struct srp_rsp {
 uint32_t   data_in_res_cnt;
 uint32_t   s

[PATCH v2 0/2] misc: Replace zero-length arrays with flexible array member

[Philippe Mathieu-Daudé]

v2:
- do not modify qed.h (structure with single member)
- based on hw/scsi/spapr_vscsi fix series:
  https://mid.mail-archive.com/20200304153311.22959-1-philmd@redhat.com

This is a tree-wide cleanup inspired by a Linux kernel commit
(from Gustavo A. R. Silva).

--v-- description start --v--

  The current codebase makes use of the zero-length array language
  extension to the C90 standard, but the preferred mechanism to
  declare variable-length types such as these ones is a flexible
  array member [1], introduced in C99:

  struct foo {
  int stuff;
  struct boo array[];
  };

  By making use of the mechanism above, we will get a compiler
  warning in case the flexible array does not occur last in the
  structure, which will help us prevent some kind of undefined
  behavior bugs from being unadvertenly introduced [2] to the
  Linux codebase from now on.

--^-- description end --^--

Do the similar housekeeping in the QEMU codebase (which uses
C99 since commit 7be41675f7cb).

The first patch is done with the help of a coccinelle semantic
patch. However Coccinelle does not recognize:

  struct foo {
  int stuff;
  struct boo array[];
  } QEMU_PACKED;

but does recognize:

  struct QEMU_PACKED foo {
  int stuff;
  struct boo array[];
  };

I'm not sure why, neither it is worth refactoring all QEMU
structures to use the attributes before the structure name,
so I did the 2nd patch manually.

Anyway this is annoying, because many structures are not handled
by coccinelle. Maybe this needs to be reported to upstream
coccinelle?

I used spatch 1.0.8 with:

  -I include --include-headers \
  --macro-file scripts/cocci-macro-file.h \
  --keep-comments --indent 4

Regards,

Phil.

Based-on: <20200304153311.22959-1-phi...@redhat.com>
Supersedes: <20200304005105.27454-1-phi...@redhat.com>

Philippe Mathieu-Daudé (2):
  misc: Replace zero-length arrays with flexible array member
(automatic)
  misc: Replace zero-length arrays with flexible array member (manual)

 docs/interop/vhost-user.rst   |  4 ++--
 bsd-user/qemu.h   |  2 +-
 contrib/libvhost-user/libvhost-user.h |  2 +-
 hw/m68k/bootinfo.h|  2 +-
 hw/scsi/srp.h |  6 +++---
 hw/xen/xen_pt.h   |  2 +-
 include/hw/acpi/acpi-defs.h   | 16 
 include/hw/arm/smmu-common.h  |  2 +-
 include/hw/boards.h   |  2 +-
 include/hw/i386/intel_iommu.h |  3 ++-
 include/hw/s390x/event-facility.h |  2 +-
 include/hw/s390x/sclp.h   |  8 
 include/hw/virtio/virtio-iommu.h  |  2 +-
 include/sysemu/cryptodev.h|  2 +-
 include/tcg/tcg.h |  2 +-
 pc-bios/s390-ccw/bootmap.h|  2 +-
 pc-bios/s390-ccw/sclp.h   |  2 +-
 tests/qtest/libqos/ahci.h |  2 +-
 block/linux-aio.c |  2 +-
 block/vmdk.c  |  2 +-
 hw/acpi/nvdimm.c  |  6 +++---
 hw/char/sclpconsole-lm.c  |  2 +-
 hw/char/sclpconsole.c |  2 +-
 hw/dma/soc_dma.c  |  2 +-
 hw/i386/x86.c |  2 +-
 hw/misc/omap_l4.c |  2 +-
 hw/nvram/eeprom93xx.c |  2 +-
 hw/rdma/vmw/pvrdma_qp_ops.c   |  4 ++--
 hw/s390x/virtio-ccw.c |  2 +-
 hw/usb/dev-network.c  |  2 +-
 hw/usb/dev-smartcard-reader.c |  4 ++--
 hw/virtio/virtio.c|  4 ++--
 net/queue.c   |  2 +-
 target/s390x/ioinst.c |  2 +-
 34 files changed, 53 insertions(+), 52 deletions(-)

-- 
2.21.1

Re: [PATCH v6 1/9] iotests: do a light delinting

[John Snow]

On 3/4/20 6:12 AM, Kevin Wolf wrote:
> Am 03.03.2020 um 22:25 hat John Snow geschrieben:
>>
>>
>> On 2/27/20 7:59 AM, Max Reitz wrote:
>>> On 27.02.20 01:06, 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. This will be important if (when?) we
 begin adding type hints to our code base.
> 
> I'm not sure I understand this connection. mypy doesn't care about
> style.
> 

Each catches things the other doesn't -- and often getting mypy passing
involves new errors in non-type-checked contexts. I have personally
found it useful to *always use both tools*.

So having a pylint baseline will help ensure that -- while we transition
to mypy, and expect to see many errors during the transition -- we have
a solid baseline from the other tool to avoid regressions with.

 Signed-off-by: John Snow 
 ---
  tests/qemu-iotests/iotests.py | 88 ++-
  1 file changed, 45 insertions(+), 43 deletions(-)
>>>
>>> I feel like I’m the wrongest person there is for reviewing a Python
>>> style-fixing patch, but here I am and so here I go:
>>
>> No, it's good.
> 
> Max can't be the wrongest person for it because that's already me.
> 
 diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
 index 8815052eb5..e8a0ea14fc 100644
 --- a/tests/qemu-iotests/iotests.py
 +++ b/tests/qemu-iotests/iotests.py
>>>
>>> [...]
>>>
 @@ -245,8 +243,7 @@ def qemu_nbd_early_pipe(*args):
' '.join(qemu_nbd_args + ['--fork'] + 
 list(args
  if exitcode == 0:
  return exitcode, ''
 -else:
 -return exitcode, subp.communicate()[0]
 +return exitcode, subp.communicate()[0]
>>>
>>> If we want to make such a change (which I don’t doubt we want), I think
>>> it should be the other way around: Make the condition “exitcode != 0”,
>>> so the final return is the return for the successful case.  (Just
>>> because I think that’s how we usually do it, at least in the qemu code?)
>>>
>>> [...]
>>>
>>
>> Yes, makes sense. I was behaving a little more mechanically.
> 
> Here and...
> 

This check can be disabled I think legitimately.

 @@ -756,12 +750,13 @@ def assert_block_path(self, root, path, 
 expected_node, graph=None):
  assert node is not None, 'Cannot follow path %s%s' % (root, 
 path)
  
  try:
 -node_id = next(edge['child'] for edge in graph['edges'] \
 - if edge['parent'] == 
 node['id'] and
 -edge['name'] == 
 child_name)
 +node_id = next(edge['child'] for edge in graph['edges']
 +   if edge['parent'] == node['id'] and
 +   edge['name'] == child_name)
>>>
>>> I don’t mind the if alignment, but I do mind not aligning the third line
>>> to the “edge” above it (i.e. the third line is part of the condition, so
>>> I’d align it to the “if” condition).
>>>
>>> But then again it’s nothing new that I like to disagree with commonly
>>> agreed-upon Python coding styles, so.
>>>
>>> [...]
>>>
>>
>> OK, that can be addressed by highlighting the sub-expression with
>> parentheses:
>>
>> node_id = next(edge['child'] for edge in graph['edges']
>>if (edge['parent'] == node['id'] and
>>edge['name'] == child_name))
> 
> ...here I must say that while I think Max's suggestions feel like an
> improvement to me over the patch, I actually like the current code best
> in both cases.
> 

This check I think *should* stay enabled to catch bad indenting, even if
it comes out looking subpar (subjective) in this one instance.

We might be able to fine-tune the indent checks, but I think for one
debated instance ... it's not important.

> In fact, after scanning your patch, I feel it's actually the majority of
> changes that pylint wants that aren't an improvement... Maybe this just
> underlines the fact that I am the wrongest person to review such patches
> and not Max. Though I'm surprised that I'm generally not the person who
> introduces the code violating the rules, and I don't have the impression
> in this thread that anyone is eager to defend pylint's opinion.
> 
> Now I ran pylint myself and it prints some even more ridiculous warnings
> like variable names being too short for its liking. I guess this means
> that if we want to run it without warnings or errors, we need to use a
> config file anyway to disable the worst parts.
> 
> And if we have a config file anyway, maybe we can more selectively
> enable the checks that we actually w

Re: [PATCH v6 7/9] iotests: ignore import warnings from pylint

[John Snow]

On 3/3/20 7:02 PM, Philippe Mathieu-Daudé wrote:
> On 3/3/20 8:57 PM, John Snow wrote:
>> On 2/27/20 9:14 AM, Philippe Mathieu-Daudé wrote:
>>> On 2/27/20 1:06 AM, 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.
>>>
>>> I'm tempted to NAck this and require an "installable python module"...
>>>
>>> Let's discuss why it is that hard!
>>>
>>
>> I've been tricked into this before. It's not work I am interested in
>> doing right now; it's WAY beyond the scope of what I am doing here.
>>
>> It involves properly factoring all of our python code, deciding which
>> portions are meant to be installed separately from QEMU itself, coming
>> up with a versioning scheme, packaging code, and moving code around in
>> many places.
>>
>> Then it involves coming up with tooling and infrastructure for creating
>> virtual environments, installing the right packages to it, and using it
>> to run our python tests.
>>
>> No, that's way too invasive. I'm not doing it and I will scream loudly
>> if you make me.
>>
>> A less invasive hack involves setting the python import path in a
>> consolidated spot so that python knows where it can import from. This
>> works, but might break the ability to run such tests as one-offs without
>> executing the environment setup.
>>
>> Again, not work I care to do right now and so I won't. The benefit of
>> these patches is to provide some minimum viable CI CQA for Python where
>> we had none before, NOT fix every possible Python problem in one shot.
> 
> OK I guess we misunderstood each other :)
> 
> I didn't understood your comment as personal to you for this patch, but
> generic. It makes sense it is not your priority and it is obvious this
> task will take a single developer a lot of time resources. I am
> certainly NOT asking you to do it.
> 
> My question was rather community-oriented.
> (Cc'ing Eduardo because we talked about this after the last KVM forum).
> 

OK, *that* is a fair question -- but you did threaten to reject the
patch, implying it was in-scope for this series. In no uncertain terms,
it is not.


HOWEVER ... since we're here, let's discuss python packaging.

This is the iotests code base. It is not intended to be packaged as
such, it's intended to be run in-tree or in the build folder. It will
rely on files being in specific paths and so on.

(I don't remember if iotests is designed to detect features at runtime
or if it still uses the build options to skip tests. Cleber would know
better, as he's battled this distinction in the past.)

Regardless, I think iotests depends on the qemu version AND the build
configuration, so iotests shouldn't be independently packaged or
packagable. It should remain "a collection of scripts" instead.

In this paradigm, python can only find descendant files. Importing from
subfolders using relative paths.

In this case, we're trying to climb *up* the path tree, which causes
grief. One often quoted solution is to use syspath hacking to
dynamically, at runtime, add parent folders to the PYTHONPATH.

Most static analysis tooling I have used to date is unable to cope with
this workaround -- hence pylint's failure to find the package being
imported.

(If we progress to using mypy, mypy will also be unable to cope with
this statement. It becomes clear we need to create a well defined
environment so tools know where they are allowed to look for sources.)

However, the thing we are trying to import is something that arguably
can be turned into an installable package as a light Python SDK for
interfacing with QEMU. That's worth doing. This code does not (AFAIK)
depend on any build configuration. That's good!

So there are a few approaches, and they're not mutually exclusive.
Option 1 *can* be a stopgap to option 2.

1. Use the iotest runners to set the PYTHONPATH to include the
python/qemu source tree directory. The imports will fail outside of the
test runner now, but we won't have to do any syspath hacking. This is
likely not important because the tests already require a good number of
environment variables set to function properly anyway.

This is a good workaround, but still outside of the scope for this series.

Notably, any pylint gating will need to occur under this specialized
environment (with PYTHONPATH set.) This might be more of a burden at times.

2. Convert the "python/qemu" folder into an installable "qemu" module.
The version of this package can track the `git describe` version well.

Once it is installable, there are several ways to use it:

A: Install it using pip/setuptools to the system. (pip3 install .)

B: Install it to the user's environment (pip3 install --user .)

C: Create a virtual environment (using whichever virtual environment
tool of your choice) and then having entered the venv, installin

Re: [PATCH v6 9/9] iotests: add pylintrc file

[John Snow]

On 3/4/20 2:22 AM, Markus Armbruster wrote:
> John Snow  writes:
> 
>> Repeatable results. run `pylint iotests.py` and you should get a pass.
> 
> Start your sentences with a capital letter, please.
> 

The German complains about the capitalization, but not the sentence
fragment.

>>
>> Signed-off-by: John Snow 
>> ---
>>  tests/qemu-iotests/pylintrc | 20 
>>  1 file changed, 20 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..feed506f75
>> --- /dev/null
>> +++ b/tests/qemu-iotests/pylintrc
>> @@ -0,0 +1,20 @@
>> +[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,
>> +missing-docstring,
>> +line-too-long,
>> +too-many-lines,
>> +too-few-public-methods,
>> +too-many-arguments,
>> +too-many-locals,
>> +too-many-branches,
>> +too-many-public-methods,
>> \ No newline at end of file
> 
> Add the newline, please.
> 
> German pejorative for the too-many- and too-few- warnings: "Müsli".
> Implies it's for muesli-knitters / granola-crunchers indulging their
> orthorexia.
> 

They are useful at times as they can suggest when you are going a bit
overboard on "organically grown" design. For cleaning an existing
codebase, it's more of a hindrance to the immediate goal of establishing
a baseline.

(*cough* I try to adhere to them in my own freshly written code, and
disable per-line when I've decided to veto the suggestion. Not
appropriate for a codebase like ours. As Max reminds me, it's just tests
-- don't make them too clever or pretty.)

Regardless. It's not appropriate here and now.

> missing-docstring is not advisable for libraries.  Feels okay here.
> 

Ideally we do start using them, but it's out of scope here. Since I did
some cleanup, I wanted to establish the baseline of what I adhered to.

*not* suggest that it's the destination state.

Adding proper docstrings should be done during mypy conversion once the
types are determined, understood, and enforced. Not before then.

> line-too-long might be worth cleaning up.  How many of them do we have
> now?
> 

Five in iotests.py using the default length of 100. 15 if I limit to 80.

If we agree that 100 is okay, I can tackle this in an amendment patch.
If 80 is okay, I'm going to put it off as one coat of paint too many.

(I will try to clean up the 100+ lines for my next version. I am
hesitant to make a deeper cut because I have the feeling it's the type
of series that will incur a lot of nitpicks on indent style.)

--js

Re: [PATCH v2 0/2] misc: Replace zero-length arrays with flexible array member

[John Snow]

On 3/4/20 10:38 AM, Philippe Mathieu-Daudé wrote:
> v2:
> - do not modify qed.h (structure with single member)
> - based on hw/scsi/spapr_vscsi fix series:
>   https://mid.mail-archive.com/20200304153311.22959-1-philmd@redhat.com
> 
> This is a tree-wide cleanup inspired by a Linux kernel commit
> (from Gustavo A. R. Silva).
> 
> --v-- description start --v--
> 
>   The current codebase makes use of the zero-length array language
>   extension to the C90 standard, but the preferred mechanism to
>   declare variable-length types such as these ones is a flexible
>   array member [1], introduced in C99:
> 
>   struct foo {
>   int stuff;
>   struct boo array[];
>   };
> 
>   By making use of the mechanism above, we will get a compiler
>   warning in case the flexible array does not occur last in the
>   structure, which will help us prevent some kind of undefined
>   behavior bugs from being unadvertenly introduced [2] to the
>   Linux codebase from now on.
> 
> --^-- description end --^--
> 
> Do the similar housekeeping in the QEMU codebase (which uses
> C99 since commit 7be41675f7cb).
> 
> The first patch is done with the help of a coccinelle semantic
> patch. However Coccinelle does not recognize:
> 
>   struct foo {
>   int stuff;
>   struct boo array[];
>   } QEMU_PACKED;
> 
> but does recognize:
> 
>   struct QEMU_PACKED foo {
>   int stuff;
>   struct boo array[];
>   };
> 
> I'm not sure why, neither it is worth refactoring all QEMU
> structures to use the attributes before the structure name,
> so I did the 2nd patch manually.
> 
> Anyway this is annoying, because many structures are not handled
> by coccinelle. Maybe this needs to be reported to upstream
> coccinelle?
> 
> I used spatch 1.0.8 with:
> 
>   -I include --include-headers \
>   --macro-file scripts/cocci-macro-file.h \
>   --keep-comments --indent 4
> 
> Regards,
> 
> Phil.
> 
> Based-on: <20200304153311.22959-1-phi...@redhat.com>
> Supersedes: <20200304005105.27454-1-phi...@redhat.com>
> 
> Philippe Mathieu-Daudé (2):
>   misc: Replace zero-length arrays with flexible array member
> (automatic)
>   misc: Replace zero-length arrays with flexible array member (manual)
> 
>  docs/interop/vhost-user.rst   |  4 ++--
>  bsd-user/qemu.h   |  2 +-
>  contrib/libvhost-user/libvhost-user.h |  2 +-
>  hw/m68k/bootinfo.h|  2 +-
>  hw/scsi/srp.h |  6 +++---
>  hw/xen/xen_pt.h   |  2 +-
>  include/hw/acpi/acpi-defs.h   | 16 
>  include/hw/arm/smmu-common.h  |  2 +-
>  include/hw/boards.h   |  2 +-
>  include/hw/i386/intel_iommu.h |  3 ++-
>  include/hw/s390x/event-facility.h |  2 +-
>  include/hw/s390x/sclp.h   |  8 
>  include/hw/virtio/virtio-iommu.h  |  2 +-
>  include/sysemu/cryptodev.h|  2 +-
>  include/tcg/tcg.h |  2 +-
>  pc-bios/s390-ccw/bootmap.h|  2 +-
>  pc-bios/s390-ccw/sclp.h   |  2 +-
>  tests/qtest/libqos/ahci.h |  2 +-
>  block/linux-aio.c |  2 +-
>  block/vmdk.c  |  2 +-
>  hw/acpi/nvdimm.c  |  6 +++---
>  hw/char/sclpconsole-lm.c  |  2 +-
>  hw/char/sclpconsole.c |  2 +-
>  hw/dma/soc_dma.c  |  2 +-
>  hw/i386/x86.c |  2 +-
>  hw/misc/omap_l4.c |  2 +-
>  hw/nvram/eeprom93xx.c |  2 +-
>  hw/rdma/vmw/pvrdma_qp_ops.c   |  4 ++--
>  hw/s390x/virtio-ccw.c |  2 +-
>  hw/usb/dev-network.c  |  2 +-
>  hw/usb/dev-smartcard-reader.c |  4 ++--
>  hw/virtio/virtio.c|  4 ++--
>  net/queue.c   |  2 +-
>  target/s390x/ioinst.c |  2 +-
>  34 files changed, 53 insertions(+), 52 deletions(-)
> 

I'll admit I did not manually verify ALL of this, but instead trust that:

1. The conversion is correct, and this is a desirable change to make.
2. Sample conversions I looked at appear correct.
3. It builds.
4. It passes tests.

So:

Acked-by: John Snow 

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[Mark Cave-Ayland]

On 04/03/2020 00:22, BALATON Zoltan wrote:

 So on that basis the best explanation as to what is happening is the
 comment in the link you provided here:
 https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/powerpc/platforms/chrp/pci.c?h=v4.14.172#n353



 /* Pegasos2 firmware version 20040810 configures the built-in IDE 
 controller
 * in legacy mode, but sets the PCI registers to PCI native mode.
 * The chip can only operate in legacy mode, so force the PCI class into 
 legacy
 * mode as well. The same fixup must be done to the class-code property in
 * the IDE node /pci@8000/ide@C,1
 */
>>>
>>> I'm not sure that it makes much sense that the firmware configures the chip 
>>> one way
>>> then puts info about a different way in the device tree. There could be 
>>> bugs but this
>>> is not likely. Especially because I see in traces that the firmware does 
>>> try to
>>> configure the device in native mode. These are the first few accesses of 
>>> the firmware
>>> to via-ide:
>>
>> But that is exactly what is happening! The comment above clearly indicates 
>> the
>> firmware incorrectly sets the IDE controller in native mode which is in exact
>> agreement with the trace you provide below. And in fact if you look at
> 
> I may be reading the comment wrong but to me that says that "firmware 
> configures IDE
> in _legacy_ mode" whereas the trace shows it actually configures it in 
> _native_ mode
> which is complying to the CHRP doc above. But since it cannot comply to the 
> "native
> devices using OpenPIC" part it probably tries to apply the "ISA devices 
> embedded in
> PCI" part and locks IRQ to 14 and 15. Or it just wants to avoid sharing IRQs 
> as much
> as possible and the designers decided they will use IRQ14 and 15 for IDE.

Interesting. My interpretation of the comment was that the hardware can only 
operate
in legacy mode, even though the firmware configures the PCI registers to enable
native mode (which is why the class-code and IRQ routing are wrong).

>> https://www.powerdeveloper.org/platforms/pegasos/devicetree you can see the 
>> nvramrc
>> hack that was released in order to fix the device tree to boot Linux which 
>> alters the
>> class-code and sets interrupts to 14 (which I believe is invalid, but 
>> seemingly good
>> enough here).
> 
> Isn't this the same fixup that newer Linux kernels already include? Maybe 
> this was
> needed before Linux properly supported Pegasos2 but later kernels will do this
> anyway. This does not give us any new info we did not have before I think 
> maybe just
> easier to see all fixups in one place.
> 
>>> pci_cfg_write via-ide 12:1 @0x9 <- 0xf
>>> pci_cfg_write via-ide 12:1 @0x40 <- 0xb
>>> pci_cfg_write via-ide 12:1 @0x41 <- 0xf2
>>> pci_cfg_write via-ide 12:1 @0x43 <- 0x35
>>> pci_cfg_write via-ide 12:1 @0x44 <- 0x18
>>> pci_cfg_write via-ide 12:1 @0x45 <- 0x1c
>>> pci_cfg_write via-ide 12:1 @0x46 <- 0xc0
>>> pci_cfg_write via-ide 12:1 @0x50 <- 0x17171717
>>> pci_cfg_write via-ide 12:1 @0x54 <- 0x14
>>> pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106
>>> pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106
>>> pci_cfg_read via-ide 12:1 @0x8 -> 0x1018f06
>>> pci_cfg_read via-ide 12:1 @0xc -> 0x0
>>> pci_cfg_read via-ide 12:1 @0x2c -> 0x11001af4
>>> pci_cfg_read via-ide 12:1 @0x3c -> 0x10e
>>> pci_cfg_read via-ide 12:1 @0x4 -> 0x2800080
>>> pci_cfg_read via-ide 12:1 @0x3c -> 0x10e
>>> pci_cfg_write via-ide 12:1 @0x3c <- 0x109
>>>
>>> The very first write is to turn on native mode, so I think it's not about 
>>> what the
>>> firmware does but something about how hardware is wired on Pegasos II or 
>>> the VT8231
>>> chip itself that only allows legacy interrupts instead of 100% native mode 
>>> for IDE.
>>>
 Given that the DT is wrong, then we should assume that all OSs would have 
 to
 compensate for this in the same way as Linux, and therefore this should be 
 handled
 automatically.

 AFAICT this then only leaves the question: why does the firmware set
 PCI_INTERRUPT_LINE to 9, which is presumably why you are seeing problems 
 running
 MorphOS under QEMU.
>>>
>>> Linux does try to handle both true native mode and half-native mode. It 
>>> only uses
>>> half-native mode if finds IRQ14 on Pegasos, otherwise skips Pegasos 
>>> specific fixup
>>> and uses true native mode setup. I don't know what MorphOS does but I think 
>>> it justs
>>> knows that Pegasos2 has this quirk and does not look at the device tree at 
>>> all.

I think this is the other way around? From the code above:

if (viaide->irq != 14)
return;

Doesn't this suggest that chrp_pci_fixup_vt8231_ata() exits without applying 
the fix
if it finds PCI_INTERRUPT_LINE set to 9 from the firmware above?

Do you have a copy of the full DT and the firmware revision number that was 
used to
generate your Linux boot output on real hardware?

>> Again to summarise: this is a 

[PATCH v7 01/10] 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. This will be important if (when?) we
begin adding type hints to our code base.

Signed-off-by: John Snow 
---
 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 8815052eb5..c3aa857140 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
@@ -34,7 +32,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)
 
 # This will not work if arguments contain spaces but is necessary if we
 # want to support the override options that ./check supports.
@@ -138,11 +136,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)
 
@@ -221,7 +219,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()
@@ -243,10 +241,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'''
@@ -310,7 +306,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)
@@ -321,7 +317,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
@@ -347,7 +343,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
@@ -358,7 +354,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
@@ -369,14 +365,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):
@@ -385,7 +381,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.
 
@@ -532,11 +528,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_l

[PATCH v7 08/10] iotest 258: use script_main

[John Snow]

Since this one is nicely factored to use a single entry point,
use script_main to run the tests.

Signed-off-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/258 | 11 ---
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/tests/qemu-iotests/258 b/tests/qemu-iotests/258
index a65151dda6..e305a1502f 100755
--- a/tests/qemu-iotests/258
+++ b/tests/qemu-iotests/258
@@ -23,12 +23,6 @@ import iotests
 from iotests import log, qemu_img, qemu_io_silent, \
 filter_qmp_testfiles, filter_qmp_imgfmt
 
-# Need backing file and change-backing-file support
-iotests.script_initialize(
-supported_fmts=['qcow2', 'qed'],
-supported_platforms=['linux'],
-)
-
 # Returns a node for blockdev-add
 def node(node_name, path, backing=None, fmt=None, throttle=None):
 if fmt is None:
@@ -161,4 +155,7 @@ def main():
 test_concurrent_finish(False)
 
 if __name__ == '__main__':
-main()
+# Need backing file and change-backing-file support
+iotests.script_main(main,
+supported_fmts=['qcow2', 'qed'],
+supported_platforms=['linux'])
-- 
2.21.1

[PATCH v7 02/10] iotests: don't use 'format' for drive_add

[John Snow]

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

Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Max Reitz 
Signed-off-by: John Snow 
---
 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 c3aa857140..cd0f185d30 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -482,21 +482,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 v7 00/10] 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.

V7:

[] : 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/10:[0025] [FC] 'iotests: do a light delinting'
002/10:[] [--] 'iotests: don't use 'format' for drive_add'
003/10:[] [-C] 'iotests: ignore import warnings from pylint'
004/10:[0008] [FC] 'iotests: replace mutable list default args'
005/10:[0006] [FC] 'iotests: add pylintrc file'
006/10:[down] 'iotests: limit line length to 79 chars'
007/10:[0008] [FC] 'iotests: add script_initialize'
008/10:[] [--] 'iotest 258: use script_main'
009/10:[] [--] 'iotests: Mark verify functions as private'
010/10:[0006] [FC] 'iotests: use python logging for iotests.log()'

- 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.
 - Dropped patch 1, rebased, added more delinting.
 - I'm not touching the supported_platforms thing.
   Not interested in rehashing that debate.

V5:
 - Rebased again
 - Allow Python tests to run on any platform

V4:
 - Rebased on top of kwolf/block at the behest of mreitz

V3:
 - Rebased for 4.1+; now based on main branch.

V2:
 - Added all of the other python tests I missed to use script_initialize
 - Refactored the common setup as per Ehabkost's suggestion
 - Added protocol arguments to common initialization,
   but this isn't strictly required.

John Snow (10):
  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: limit line length to 79 chars
  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/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 | 300 --
 tests/qemu-iotests/pylintrc   |  26 +++
 42 files changed, 300 insertions(+), 196 deletions(-)
 create mode 100644 tests/qemu-iotests/pylintrc

-- 
2.21.1

[PATCH v7 03/10] 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.

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 cd0f185d30..fd65b90691 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -29,6 +29,7 @@
 import io
 from collections import OrderedDict
 
+# 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 v7 04/10] iotests: replace mutable list default args

[John Snow]

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 
---
 tests/qemu-iotests/iotests.py | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index fd65b90691..54d68774e1 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -136,7 +136,7 @@ def qemu_img_log(*args):
 log(result, filters=[filter_testfiles])
 return result
 
-def img_info_log(filename, filter_path=None, imgopts=False, extra_args=[]):
+def img_info_log(filename, filter_path=None, imgopts=False, extra_args=()):
 args = ['info']
 if imgopts:
 args.append('--image-opts')
@@ -350,7 +350,7 @@ def _filter(_key, value):
 return value
 return filter_qmp(qmsg, _filter)
 
-def log(msg, filters=[], indent=None):
+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.'''
 for flt in filters:
@@ -566,7 +566,7 @@ def get_qmp_events_filtered(self, wait=60.0):
 result.append(filter_qmp_event(ev))
 return result
 
-def qmp_log(self, cmd, filters=[], indent=None, **kwargs):
+def qmp_log(self, cmd, filters=(), indent=None, **kwargs):
 full_cmd = OrderedDict((
 ("execute", cmd),
 ("arguments", ordered_qmp(kwargs))
@@ -967,7 +967,7 @@ def case_notrun(reason):
 open('%s/%s.casenotrun' % (output_dir, seq), 'a').write(
 '[case not run] ' + reason + '\n')
 
-def verify_image_format(supported_fmts=[], unsupported_fmts=[]):
+def verify_image_format(supported_fmts=(), unsupported_fmts=()):
 assert not (supported_fmts and unsupported_fmts)
 
 if 'generic' in supported_fmts and \
@@ -981,7 +981,7 @@ def verify_image_format(supported_fmts=[], 
unsupported_fmts=[]):
 if not_sup or (imgfmt in unsupported_fmts):
 notrun('not suitable for this image format: %s' % imgfmt)
 
-def verify_protocol(supported=[], unsupported=[]):
+def verify_protocol(supported=(), unsupported=()):
 assert not (supported and unsupported)
 
 if 'generic' in supported:
@@ -1000,11 +1000,11 @@ def verify_platform(supported=None, unsupported=None):
 if not any((sys.platform.startswith(x) for x in supported)):
 notrun('not suitable for this OS: %s' % sys.platform)
 
-def verify_cache_mode(supported_cache_modes=[]):
+def verify_cache_mode(supported_cache_modes=()):
 if supported_cache_modes and (cachemode not in supported_cache_modes):
 notrun('not suitable for this cache mode: %s' % cachemode)
 
-def verify_aio_mode(supported_aio_modes=[]):
+def verify_aio_mode(supported_aio_modes=()):
 if supported_aio_modes and (aiomode not in supported_aio_modes):
 notrun('not suitable for this aio mode: %s' % aiomode)
 
@@ -1044,7 +1044,7 @@ def supported_formats(read_only=False):
 
 return supported_formats.formats[read_only]
 
-def skip_if_unsupported(required_formats=[], read_only=False):
+def skip_if_unsupported(required_formats=(), read_only=False):
 '''Skip Test Decorator
Runs the test if all the required formats are whitelisted'''
 def skip_test_decorator(func):
@@ -1095,11 +1095,11 @@ def execute_unittest(output, verbosity, debug):
 sys.stderr.write(out)
 
 def execute_test(test_function=None,
- supported_fmts=[],
+ supported_fmts=(),
  supported_platforms=None,
- supported_cache_modes=[], supported_aio_modes={},
- unsupported_fmts=[], supported_protocols=[],
- unsupported_protocols=[]):
+ supported_cache_modes=(), supported_aio_modes=(),
+ unsupported_fmts=(), supported_protocols=(),
+ unsupported_protocols=()):
 """Run either unittest or script-style tests."""
 
 # We are using TEST_DIR and QEMU_DEFAULT_MACHINE as proxies to
-- 
2.21.1

[PATCH v7 10/10] iotests: use python logging for iotests.log()

[John Snow]

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:

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/245|  1 +
 tests/qemu-iotests/245.out| 24 +-
 tests/qemu-iotests/iotests.py | 48 +--
 4 files changed, 44 insertions(+), 33 deletions(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index aa911d266a..104e3cee1b 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -411,8 +411,8 @@ class TestParallelOps(iotests.QMPTestCase):
 result = self.vm.qmp('block-job-set-speed', device='drive0', speed=0)
 self.assert_qmp(result, 'return', {})
 
-self.vm.run_job(job='drive0', auto_dismiss=True, use_log=False)
-self.vm.run_job(job='node4', auto_dismiss=True, use_log=False)
+self.vm.run_job(job='drive0', auto_dismiss=True)
+self.vm.run_job(job='node4', auto_dismiss=True)
 self.assert_no_active_block_jobs()
 
 # Test a block-stream and a block-commit job in parallel
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index 489bf78bd0..edb40a4ae8 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -1002,5 +1002,6 @@ class TestBlockdevReopen(iotests.QMPTestCase):
 self.reopen(opts, {'backing': 'hd2'})
 
 if __name__ == '__main__':
+iotests.activate_logging()
 iotests.main(supported_fmts=["qcow2"],
  supported_protocols=["file"])
diff --git a/tests/qemu-iotests/245.out b/tests/qemu-iotests/245.out
index a19de5214d..15c3630e92 100644
--- a/tests/qemu-iotests/245.out
+++ b/tests/qemu-iotests/245.out
@@ -1,17 +1,17 @@
+{"execute": "job-finalize", "arguments": {"id": "commit0"}}
+{"return": {}}
+{"data": {"id": "commit0", "type": "commit"}, "event": "BLOCK_JOB_PENDING", 
"timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "commit0", "len": 3145728, "offset": 3145728, "speed": 0, 
"type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": 
{"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "job-finalize", "arguments": {"id": "stream0"}}
+{"return": {}}
+{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", 
"timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, 
"type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": 
{"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "job-finalize", "arguments": {"id": "stream0"}}
+{"return": {}}
+{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", 
"timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, 
"type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": 
{"microseconds": "USECS", "seconds": "SECS"}}
 ..
 --
 Ran 18 tests
 
 OK
-{"execute": "job-finalize", "arguments": {"id": "commit0"}}
-{"return": {}}
-{"data": {"id": "commit0", "type": "commit"}, "event": "BLOCK_JOB_PENDING", 
"timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
-{"data": {"device": "commit0", "len": 3145728, "offset": 3145728, "speed": 0, 
"type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": 
{"microseconds": "USECS", "seconds": "SECS"}}
-{"execute": "job-finalize", "arguments": {"id": "stream0"}}
-{"return": {}}
-{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB

[PATCH v7 09/10] iotests: Mark verify functions as private

[John Snow]

Discourage their use.

(Also, make pending patches not yet using the new entry points fail in a
very obvious way.)

Reviewed-by: Max Reitz 
Signed-off-by: John Snow 
---
 tests/qemu-iotests/iotests.py | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 1f88d2fa2a..23678f2daa 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -982,7 +982,7 @@ def case_notrun(reason):
 open('%s/%s.casenotrun' % (output_dir, seq), 'a').write(
 '[case not run] ' + reason + '\n')
 
-def verify_image_format(supported_fmts=(), unsupported_fmts=()):
+def _verify_image_format(supported_fmts=(), unsupported_fmts=()):
 assert not (supported_fmts and unsupported_fmts)
 
 if 'generic' in supported_fmts and \
@@ -996,7 +996,7 @@ def verify_image_format(supported_fmts=(), 
unsupported_fmts=()):
 if not_sup or (imgfmt in unsupported_fmts):
 notrun('not suitable for this image format: %s' % imgfmt)
 
-def verify_protocol(supported=(), unsupported=()):
+def _verify_protocol(supported=(), unsupported=()):
 assert not (supported and unsupported)
 
 if 'generic' in supported:
@@ -1006,7 +1006,7 @@ def verify_protocol(supported=(), unsupported=()):
 if not_sup or (imgproto in unsupported):
 notrun('not suitable for this protocol: %s' % imgproto)
 
-def verify_platform(supported=(), unsupported=()):
+def _verify_platform(supported=(), unsupported=()):
 if any((sys.platform.startswith(x) for x in unsupported)):
 notrun('not suitable for this OS: %s' % sys.platform)
 
@@ -1014,11 +1014,11 @@ def verify_platform(supported=(), unsupported=()):
 if not any((sys.platform.startswith(x) for x in supported)):
 notrun('not suitable for this OS: %s' % sys.platform)
 
-def verify_cache_mode(supported_cache_modes=()):
+def _verify_cache_mode(supported_cache_modes=()):
 if supported_cache_modes and (cachemode not in supported_cache_modes):
 notrun('not suitable for this cache mode: %s' % cachemode)
 
-def verify_aio_mode(supported_aio_modes=()):
+def _verify_aio_mode(supported_aio_modes=()):
 if supported_aio_modes and (aiomode not in supported_aio_modes):
 notrun('not suitable for this aio mode: %s' % aiomode)
 
@@ -1145,11 +1145,11 @@ def execute_setup_common(supported_fmts: 
Collection[str] = (),
 sys.stderr.write('Please run this test via the "check" script\n')
 sys.exit(os.EX_USAGE)
 
-verify_image_format(supported_fmts, unsupported_fmts)
-verify_protocol(supported_protocols, unsupported_protocols)
-verify_platform(supported=supported_platforms)
-verify_cache_mode(supported_cache_modes)
-verify_aio_mode(supported_aio_modes)
+_verify_image_format(supported_fmts, unsupported_fmts)
+_verify_protocol(supported_protocols, unsupported_protocols)
+_verify_platform(supported=supported_platforms)
+_verify_cache_mode(supported_cache_modes)
+_verify_aio_mode(supported_aio_modes)
 
 debug = '-d' in sys.argv
 if debug:
-- 
2.21.1

[PATCH v7 06/10] iotests: limit line length to 79 chars

[John Snow]

79 is the PEP8 recommendation. This recommendation works well for
reading patch diffs in TUI email clients.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/iotests.py | 69 ++-
 tests/qemu-iotests/pylintrc   |  6 ++-
 2 files changed, 48 insertions(+), 27 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 54d68774e1..1be11f491f 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -76,9 +76,11 @@
 def qemu_img(*args):
 '''Run qemu-img and return the exit code'''
 devnull = open('/dev/null', 'r+')
-exitcode = subprocess.call(qemu_img_args + list(args), stdin=devnull, 
stdout=devnull)
+exitcode = subprocess.call(qemu_img_args + list(args),
+   stdin=devnull, stdout=devnull)
 if exitcode < 0:
-sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' 
'.join(qemu_img_args + list(args
+sys.stderr.write('qemu-img received signal %i: %s\n' % (
+-exitcode, ' '.join(qemu_img_args + list(args
 return exitcode
 
 def ordered_qmp(qmsg, conv_keys=True):
@@ -117,7 +119,8 @@ def qemu_img_verbose(*args):
 '''Run qemu-img without suppressing its output and return the exit code'''
 exitcode = subprocess.call(qemu_img_args + list(args))
 if exitcode < 0:
-sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' 
'.join(qemu_img_args + list(args
+sys.stderr.write('qemu-img received signal %i: %s\n' % (
+-exitcode, ' '.join(qemu_img_args + list(args
 return exitcode
 
 def qemu_img_pipe(*args):
@@ -128,7 +131,8 @@ def qemu_img_pipe(*args):
 universal_newlines=True)
 exitcode = subp.wait()
 if exitcode < 0:
-sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' 
'.join(qemu_img_args + list(args
+sys.stderr.write('qemu-img received signal %i: %s\n' % (
+-exitcode, ' '.join(qemu_img_args + list(args
 return subp.communicate()[0]
 
 def qemu_img_log(*args):
@@ -158,7 +162,8 @@ def qemu_io(*args):
 universal_newlines=True)
 exitcode = subp.wait()
 if exitcode < 0:
-sys.stderr.write('qemu-io received signal %i: %s\n' % (-exitcode, ' 
'.join(args)))
+sys.stderr.write('qemu-io received signal %i: %s\n' % (
+-exitcode, ' '.join(args)))
 return subp.communicate()[0]
 
 def qemu_io_log(*args):
@@ -280,10 +285,13 @@ def filter_test_dir(msg):
 def filter_win32(msg):
 return win32_re.sub("", msg)
 
-qemu_io_re = re.compile(r"[0-9]* ops; [0-9\/:. sec]* \([0-9\/.inf]* 
[EPTGMKiBbytes]*\/sec and [0-9\/.inf]* ops\/sec\)")
+qemu_io_re = re.compile(r"[0-9]* ops; [0-9\/:. sec]* "
+r"\([0-9\/.inf]* [EPTGMKiBbytes]*\/sec "
+r"and [0-9\/.inf]* ops\/sec\)")
 def filter_qemu_io(msg):
 msg = filter_win32(msg)
-return qemu_io_re.sub("X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)", 
msg)
+return qemu_io_re.sub("X ops; XX:XX:XX.X "
+  "(XXX YYY/sec and XXX ops/sec)", msg)
 
 chown_re = re.compile(r"chown [0-9]+:[0-9]+")
 def filter_chown(msg):
@@ -335,7 +343,9 @@ def filter_img_info(output, filename):
 line = line.replace(filename, 'TEST_IMG') \
.replace(imgfmt, 'IMGFMT')
 line = re.sub('iters: [0-9]+', 'iters: XXX', line)
-line = re.sub('uuid: [-a-f0-9]+', 'uuid: 
----', line)
+line = re.sub('uuid: [-a-f0-9]+',
+  'uuid: ----',
+  line)
 line = re.sub('cid: [0-9]+', 'cid: XX', line)
 lines.append(line)
 return '\n'.join(lines)
@@ -356,12 +366,9 @@ def log(msg, filters=(), indent=None):
 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)
 
@@ -529,11 +536,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))
 
 def resume_drive(self, drive):
 self.qmp('human-monitor-command',
- command_line='qemu-io %s "remove_break bp_%s"' % (drive, 
drive))
+  

[PATCH v7 05/10] 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 
---
 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 v7 07/10] iotests: add script_initialize

[John Snow]

Like script_main, but doesn't require a single point of entry.
Replace all existing initialization sections with this drop-in replacement.

This brings debug support to all existing script-style iotests.

Reviewed-by: Max Reitz 
Signed-off-by: John Snow 
---
 tests/qemu-iotests/149|  3 +-
 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/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|  7 ++--
 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 | 73 +++
 37 files changed, 128 insertions(+), 80 deletions(-)

diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
index b4a21bf7b7..852768f80a 100755
--- a/tests/qemu-iotests/149
+++ b/tests/qemu-iotests/149
@@ -382,8 +382,7 @@ def test_once(config, qemu_img=False):
 
 
 # Obviously we only work with the luks image format
-iotests.verify_image_format(supported_fmts=['luks'])
-iotests.verify_platform()
+iotests.script_initialize(supported_fmts=['luks'])
 
 # We need sudo in order to run cryptsetup to create
 # dm-crypt devices. This is safe to use on any
diff --git a/tests/qemu-iotests/194 b/tests/qemu-iotests/194
index 9dc1bd3510..8b1f720af4 100755
--- a/tests/qemu-iotests/194
+++ b/tests/qemu-iotests/194
@@ -21,8 +21,8 @@
 
 import iotests
 
-iotests.verify_image_format(supported_fmts=['qcow2', 'qed', 'raw'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2', 'qed', 'raw'],
+  supported_platforms=['linux'])
 
 with iotests.FilePath('source.img') as source_img_path, \
  iotests.FilePath('dest.img') as dest_img_path, \
diff --git a/tests/qemu-iotests/202 b/tests/qemu-iotests/202
index 920a8683ef..e3900a44d1 100755
--- a/tests/qemu-iotests/202
+++ b/tests/qemu-iotests/202
@@ -24,8 +24,8 @@
 
 import iotests
 
-iotests.verify_image_format(supported_fmts=['qcow2'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2'],
+  supported_platforms=['linux'])
 
 with iotests.FilePath('disk0.img') as disk0_img_path, \
  iotests.FilePath('disk1.img') as disk1_img_path, \
diff --git a/tests/qemu-iotests/203 b/tests/qemu-iotests/203
index 49eff5d405..4b4bd3307d 100755
--- a/tests/qemu-iotests/203
+++ b/tests/qemu-iotests/203
@@ -24,8 +24,8 @@
 
 import iotests
 
-iotests.verify_image_format(supported_fmts=['qcow2'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2'],
+  supported_platforms=['linux'])
 
 with iotests.FilePath('disk0.img') as disk0_img_path, \
  iotests.FilePath('disk1.img') as disk1_img_path, \
diff --git a/tests/qemu-iotests/206 b/tests/qemu-iotests/206
index e2b50ae24d..f42432a838 100755
--- a/tests/qemu-iotests/206
+++ b/tests/qemu-iotests/206
@@ -23,7 +23,7 @@
 import iotests
 from iotests import imgfmt
 
-iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.script_initialize(supported_fmts=['qcow2'])
 
 with iotests.FilePath('t.qcow2') as disk_path, \
  iotests.FilePath('t.qcow2.base') as backing_path, \
diff --git a/tests/qemu-iotests/207 b/tests/qemu-iotests/207
index 3d9c1208ca..a6621410da 100755
--- a/tests/qemu-iotests/207
+++ b/tests/qemu-iotests/207
@@ -24,8 +24,10 @@ import iotests
 import subprocess
 import re
 
-iotests.verify_image_format(supported_fmts=['raw'])
-iotests.verify_protocol(supported=['ssh'])
+iotests.script_initialize(
+supported_fmts=['raw'],
+supported_protocols=['ssh'],
+)
 
 def filter_hash(qmsg):
 def _filter(key, value):
diff --git a/tests/qemu-iotests/208 b/tests/qemu-iotests/208
index 1c3fc8c7fd..6cb642f821 100755
--- a/tests/qemu-iotests/208
+++ b/tests/qemu-iotests/208
@@ -22,7 +22,7 @@
 
 import iotests
 
-iotests.verify_image_format(supp

Re: [PATCH v7 09/10] iotests: Mark verify functions as private

[Philippe Mathieu-Daudé]

On 3/4/20 10:38 PM, John Snow wrote:

Discourage their use.


I recommend you to repeat the subject, else it is harder to review 
looking only at patch description.




(Also, make pending patches not yet using the new entry points fail in a
very obvious way.)

Reviewed-by: Max Reitz 
Signed-off-by: John Snow 
---
  tests/qemu-iotests/iotests.py | 20 ++--
  1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 1f88d2fa2a..23678f2daa 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -982,7 +982,7 @@ def case_notrun(reason):
  open('%s/%s.casenotrun' % (output_dir, seq), 'a').write(
  '[case not run] ' + reason + '\n')
  
-def verify_image_format(supported_fmts=(), unsupported_fmts=()):

+def _verify_image_format(supported_fmts=(), unsupported_fmts=()):
  assert not (supported_fmts and unsupported_fmts)
  
  if 'generic' in supported_fmts and \

@@ -996,7 +996,7 @@ def verify_image_format(supported_fmts=(), 
unsupported_fmts=()):
  if not_sup or (imgfmt in unsupported_fmts):
  notrun('not suitable for this image format: %s' % imgfmt)
  
-def verify_protocol(supported=(), unsupported=()):

+def _verify_protocol(supported=(), unsupported=()):
  assert not (supported and unsupported)
  
  if 'generic' in supported:

@@ -1006,7 +1006,7 @@ def verify_protocol(supported=(), unsupported=()):
  if not_sup or (imgproto in unsupported):
  notrun('not suitable for this protocol: %s' % imgproto)
  
-def verify_platform(supported=(), unsupported=()):

+def _verify_platform(supported=(), unsupported=()):
  if any((sys.platform.startswith(x) for x in unsupported)):
  notrun('not suitable for this OS: %s' % sys.platform)
  
@@ -1014,11 +1014,11 @@ def verify_platform(supported=(), unsupported=()):

  if not any((sys.platform.startswith(x) for x in supported)):
  notrun('not suitable for this OS: %s' % sys.platform)
  
-def verify_cache_mode(supported_cache_modes=()):

+def _verify_cache_mode(supported_cache_modes=()):
  if supported_cache_modes and (cachemode not in supported_cache_modes):
  notrun('not suitable for this cache mode: %s' % cachemode)
  
-def verify_aio_mode(supported_aio_modes=()):

+def _verify_aio_mode(supported_aio_modes=()):
  if supported_aio_modes and (aiomode not in supported_aio_modes):
  notrun('not suitable for this aio mode: %s' % aiomode)
  
@@ -1145,11 +1145,11 @@ def execute_setup_common(supported_fmts: Collection[str] = (),

  sys.stderr.write('Please run this test via the "check" script\n')
  sys.exit(os.EX_USAGE)
  
-verify_image_format(supported_fmts, unsupported_fmts)

-verify_protocol(supported_protocols, unsupported_protocols)
-verify_platform(supported=supported_platforms)
-verify_cache_mode(supported_cache_modes)
-verify_aio_mode(supported_aio_modes)
+_verify_image_format(supported_fmts, unsupported_fmts)
+_verify_protocol(supported_protocols, unsupported_protocols)
+_verify_platform(supported=supported_platforms)
+_verify_cache_mode(supported_cache_modes)
+_verify_aio_mode(supported_aio_modes)
  
  debug = '-d' in sys.argv

  if debug:

Re: [PATCH v7 01/10] iotests: do a light delinting

[Philippe Mathieu-Daudé]

On 3/4/20 10:38 PM, 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. This will be important if (when?) we
begin adding type hints to our code base.

Signed-off-by: John Snow 
---
  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 8815052eb5..c3aa857140 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
@@ -34,7 +32,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)
  
  # This will not work if arguments contain spaces but is necessary if we

  # want to support the override options that ./check supports.
@@ -138,11 +136,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)
  
@@ -221,7 +219,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()
@@ -243,10 +241,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'''
@@ -310,7 +306,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)
@@ -321,7 +317,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
@@ -347,7 +343,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
@@ -358,7 +354,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
@@ -369,14 +365,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):
@@ -385,7 +381,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.
  
@@ -532,11 +528,11 @@ def pause_drive(self, drive, event=None):

  self.pause_drive(drive, "write_aio")
  return
  self.qmp('human-m

Re: [PATCH v7 04/10] iotests: replace mutable list default args

[Philippe Mathieu-Daudé]

On 3/4/20 10:38 PM, 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 
---
  tests/qemu-iotests/iotests.py | 24 
  1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index fd65b90691..54d68774e1 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -136,7 +136,7 @@ def qemu_img_log(*args):
  log(result, filters=[filter_testfiles])
  return result
  
-def img_info_log(filename, filter_path=None, imgopts=False, extra_args=[]):

+def img_info_log(filename, filter_path=None, imgopts=False, extra_args=()):
  args = ['info']
  if imgopts:
  args.append('--image-opts')
@@ -350,7 +350,7 @@ def _filter(_key, value):
  return value
  return filter_qmp(qmsg, _filter)
  
-def log(msg, filters=[], indent=None):

+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.'''
  for flt in filters:
@@ -566,7 +566,7 @@ def get_qmp_events_filtered(self, wait=60.0):
  result.append(filter_qmp_event(ev))
  return result
  
-def qmp_log(self, cmd, filters=[], indent=None, **kwargs):

+def qmp_log(self, cmd, filters=(), indent=None, **kwargs):
  full_cmd = OrderedDict((
  ("execute", cmd),
  ("arguments", ordered_qmp(kwargs))
@@ -967,7 +967,7 @@ def case_notrun(reason):
  open('%s/%s.casenotrun' % (output_dir, seq), 'a').write(
  '[case not run] ' + reason + '\n')
  
-def verify_image_format(supported_fmts=[], unsupported_fmts=[]):

+def verify_image_format(supported_fmts=(), unsupported_fmts=()):
  assert not (supported_fmts and unsupported_fmts)
  
  if 'generic' in supported_fmts and \

@@ -981,7 +981,7 @@ def verify_image_format(supported_fmts=[], 
unsupported_fmts=[]):
  if not_sup or (imgfmt in unsupported_fmts):
  notrun('not suitable for this image format: %s' % imgfmt)
  
-def verify_protocol(supported=[], unsupported=[]):

+def verify_protocol(supported=(), unsupported=()):
  assert not (supported and unsupported)
  
  if 'generic' in supported:

@@ -1000,11 +1000,11 @@ def verify_platform(supported=None, unsupported=None):
  if not any((sys.platform.startswith(x) for x in supported)):
  notrun('not suitable for this OS: %s' % sys.platform)
  
-def verify_cache_mode(supported_cache_modes=[]):

+def verify_cache_mode(supported_cache_modes=()):
  if supported_cache_modes and (cachemode not in supported_cache_modes):
  notrun('not suitable for this cache mode: %s' % cachemode)
  
-def verify_aio_mode(supported_aio_modes=[]):

+def verify_aio_mode(supported_aio_modes=()):
  if supported_aio_modes and (aiomode not in supported_aio_modes):
  notrun('not suitable for this aio mode: %s' % aiomode)
  
@@ -1044,7 +1044,7 @@ def supported_formats(read_only=False):
  
  return supported_formats.formats[read_only]
  
-def skip_if_unsupported(required_formats=[], read_only=False):

+def skip_if_unsupported(required_formats=(), read_only=False):
  '''Skip Test Decorator
 Runs the test if all the required formats are whitelisted'''
  def skip_test_decorator(func):
@@ -1095,11 +1095,11 @@ def execute_unittest(output, verbosity, debug):
  sys.stderr.write(out)
  
  def execute_test(test_function=None,

- supported_fmts=[],
+ supported_fmts=(),
   supported_platforms=None,
- supported_cache_modes=[], supported_aio_modes={},
- unsupported_fmts=[], supported_protocols=[],
- unsupported_protocols=[]):
+ supported_cache_modes=(), supported_aio_modes=(),
+ unsupported_fmts=(), supported_protocols=(),
+ unsupported_protocols=()):
  """Run either unittest or script-style tests."""
  
  # We are using TEST_DIR and QEMU_DEFAULT_MACHINE as proxies to




Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v7 06/10] iotests: limit line length to 79 chars

[Philippe Mathieu-Daudé]

On 3/4/20 10:38 PM, John Snow wrote:

79 is the PEP8 recommendation. This recommendation works well for
reading patch diffs in TUI email clients.

Signed-off-by: John Snow 
---
  tests/qemu-iotests/iotests.py | 69 ++-
  tests/qemu-iotests/pylintrc   |  6 ++-
  2 files changed, 48 insertions(+), 27 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 54d68774e1..1be11f491f 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -76,9 +76,11 @@
  def qemu_img(*args):
  '''Run qemu-img and return the exit code'''
  devnull = open('/dev/null', 'r+')
-exitcode = subprocess.call(qemu_img_args + list(args), stdin=devnull, 
stdout=devnull)
+exitcode = subprocess.call(qemu_img_args + list(args),
+   stdin=devnull, stdout=devnull)
  if exitcode < 0:
-sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' 
'.join(qemu_img_args + list(args
+sys.stderr.write('qemu-img received signal %i: %s\n' % (
+-exitcode, ' '.join(qemu_img_args + list(args


Do we want to indent Python like C and align argument below opening 
parenthesis? Except when using sys.stderr.write() you seem to do it.



  return exitcode
  
  def ordered_qmp(qmsg, conv_keys=True):

@@ -117,7 +119,8 @@ def qemu_img_verbose(*args):
  '''Run qemu-img without suppressing its output and return the exit code'''
  exitcode = subprocess.call(qemu_img_args + list(args))
  if exitcode < 0:
-sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' 
'.join(qemu_img_args + list(args
+sys.stderr.write('qemu-img received signal %i: %s\n' % (
+-exitcode, ' '.join(qemu_img_args + list(args
  return exitcode
  
  def qemu_img_pipe(*args):

@@ -128,7 +131,8 @@ def qemu_img_pipe(*args):
  universal_newlines=True)
  exitcode = subp.wait()
  if exitcode < 0:
-sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' 
'.join(qemu_img_args + list(args
+sys.stderr.write('qemu-img received signal %i: %s\n' % (
+-exitcode, ' '.join(qemu_img_args + list(args
  return subp.communicate()[0]
  
  def qemu_img_log(*args):

@@ -158,7 +162,8 @@ def qemu_io(*args):
  universal_newlines=True)
  exitcode = subp.wait()
  if exitcode < 0:
-sys.stderr.write('qemu-io received signal %i: %s\n' % (-exitcode, ' 
'.join(args)))
+sys.stderr.write('qemu-io received signal %i: %s\n' % (
+-exitcode, ' '.join(args)))
  return subp.communicate()[0]
  
  def qemu_io_log(*args):

@@ -280,10 +285,13 @@ def filter_test_dir(msg):
  def filter_win32(msg):
  return win32_re.sub("", msg)
  
-qemu_io_re = re.compile(r"[0-9]* ops; [0-9\/:. sec]* \([0-9\/.inf]* [EPTGMKiBbytes]*\/sec and [0-9\/.inf]* ops\/sec\)")

+qemu_io_re = re.compile(r"[0-9]* ops; [0-9\/:. sec]* "
+r"\([0-9\/.inf]* [EPTGMKiBbytes]*\/sec "
+r"and [0-9\/.inf]* ops\/sec\)")
  def filter_qemu_io(msg):
  msg = filter_win32(msg)
-return qemu_io_re.sub("X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)", 
msg)
+return qemu_io_re.sub("X ops; XX:XX:XX.X "
+  "(XXX YYY/sec and XXX ops/sec)", msg)
  
  chown_re = re.compile(r"chown [0-9]+:[0-9]+")

  def filter_chown(msg):
@@ -335,7 +343,9 @@ def filter_img_info(output, filename):
  line = line.replace(filename, 'TEST_IMG') \
 .replace(imgfmt, 'IMGFMT')
  line = re.sub('iters: [0-9]+', 'iters: XXX', line)
-line = re.sub('uuid: [-a-f0-9]+', 'uuid: 
----', line)
+line = re.sub('uuid: [-a-f0-9]+',
+  'uuid: ----',
+  line)
  line = re.sub('cid: [0-9]+', 'cid: XX', line)
  lines.append(line)
  return '\n'.join(lines)
@@ -356,12 +366,9 @@ def log(msg, filters=(), indent=None):
  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))


Unrelated change. Maybe worth a separate patch?


  else:
  print(msg)
  
@@ -529,11 +536,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, 

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[BALATON Zoltan]

On Wed, 4 Mar 2020, Mark Cave-Ayland wrote:

On 04/03/2020 00:22, BALATON Zoltan wrote:

So on that basis the best explanation as to what is happening is the
comment in the link you provided here:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/powerpc/platforms/chrp/pci.c?h=v4.14.172#n353

/* Pegasos2 firmware version 20040810 configures the built-in IDE controller
* in legacy mode, but sets the PCI registers to PCI native mode.
* The chip can only operate in legacy mode, so force the PCI class into legacy
* mode as well. The same fixup must be done to the class-code property in
* the IDE node /pci@8000/ide@C,1
*/


I'm not sure that it makes much sense that the firmware configures the chip one 
way
then puts info about a different way in the device tree. There could be bugs 
but this
is not likely. Especially because I see in traces that the firmware does try to
configure the device in native mode. These are the first few accesses of the 
firmware
to via-ide:


But that is exactly what is happening! The comment above clearly indicates the
firmware incorrectly sets the IDE controller in native mode which is in exact
agreement with the trace you provide below. And in fact if you look at


I may be reading the comment wrong but to me that says that "firmware 
configures IDE
in _legacy_ mode" whereas the trace shows it actually configures it in _native_ 
mode
which is complying to the CHRP doc above. But since it cannot comply to the 
"native
devices using OpenPIC" part it probably tries to apply the "ISA devices 
embedded in
PCI" part and locks IRQ to 14 and 15. Or it just wants to avoid sharing IRQs as 
much
as possible and the designers decided they will use IRQ14 and 15 for IDE.


Interesting. My interpretation of the comment was that the hardware can only 
operate
in legacy mode, even though the firmware configures the PCI registers to enable
native mode (which is why the class-code and IRQ routing are wrong).


I think you may give more significance to this comment than it really has. 
I don't know who put it there but maybe was also guessing what really 
happens and it's not really an authorative answer why it behaves like 
that. It seems to come from this commit and not from Genesi/bPlan but 
sounds more like a bugfix from some user:


https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/arch/powerpc/platforms/chrp/pci.c?h=v3.16.82&id=556ecf9be66f4d493e19bc71a7ce84366d512b71

I give more credibility to what MorphOS expects because the developers of 
that were closely cooperating with the board designers:


https://en.wikipedia.org/wiki/Bplan#Community_support
https://en.wikipedia.org/wiki/Pegasos#Operating_system_support


https://www.powerdeveloper.org/platforms/pegasos/devicetree you can see the 
nvramrc
hack that was released in order to fix the device tree to boot Linux which 
alters the
class-code and sets interrupts to 14 (which I believe is invalid, but seemingly 
good
enough here).


Isn't this the same fixup that newer Linux kernels already include? Maybe this 
was
needed before Linux properly supported Pegasos2 but later kernels will do this
anyway. This does not give us any new info we did not have before I think maybe 
just
easier to see all fixups in one place.


pci_cfg_write via-ide 12:1 @0x9 <- 0xf
pci_cfg_write via-ide 12:1 @0x40 <- 0xb
pci_cfg_write via-ide 12:1 @0x41 <- 0xf2
pci_cfg_write via-ide 12:1 @0x43 <- 0x35
pci_cfg_write via-ide 12:1 @0x44 <- 0x18
pci_cfg_write via-ide 12:1 @0x45 <- 0x1c
pci_cfg_write via-ide 12:1 @0x46 <- 0xc0
pci_cfg_write via-ide 12:1 @0x50 <- 0x17171717
pci_cfg_write via-ide 12:1 @0x54 <- 0x14
pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106
pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106
pci_cfg_read via-ide 12:1 @0x8 -> 0x1018f06
pci_cfg_read via-ide 12:1 @0xc -> 0x0
pci_cfg_read via-ide 12:1 @0x2c -> 0x11001af4
pci_cfg_read via-ide 12:1 @0x3c -> 0x10e
pci_cfg_read via-ide 12:1 @0x4 -> 0x2800080
pci_cfg_read via-ide 12:1 @0x3c -> 0x10e
pci_cfg_write via-ide 12:1 @0x3c <- 0x109

The very first write is to turn on native mode, so I think it's not about what 
the
firmware does but something about how hardware is wired on Pegasos II or the 
VT8231
chip itself that only allows legacy interrupts instead of 100% native mode for 
IDE.


Given that the DT is wrong, then we should assume that all OSs would have to
compensate for this in the same way as Linux, and therefore this should be 
handled
automatically.

AFAICT this then only leaves the question: why does the firmware set
PCI_INTERRUPT_LINE to 9, which is presumably why you are seeing problems running
MorphOS under QEMU.


Linux does try to handle both true native mode and half-native mode. It only 
uses
half-native mode if finds IRQ14 on Pegasos, otherwise skips Pegasos specific 
fixup
and uses true native mode setup. I don't know what MorphOS does but I think it 
justs
knows that Pegasos2 has this quirk and does not look at the device tree at all.


I think th

Re: [PATCH v7 01/10] iotests: do a light delinting

[John Snow]

On 3/4/20 4:45 PM, Philippe Mathieu-Daudé wrote:
> I don't understand the rational for this change, but the result is
> correct anyway, so thanks for this nice cleanup!

Leftover from when 'no-else-return' was enabled.

It's mostly harmless.

Re: [PATCH v7 06/10] iotests: limit line length to 79 chars

[John Snow]

On 3/4/20 4:58 PM, Philippe Mathieu-Daudé wrote:
> Do we want to indent Python like C and align argument below opening
> parenthesis? Except when using sys.stderr.write() you seem to do it.

This isn't an argument to write, it's an argument to the format string,
so I will say "no."

For *where* I've placed the line break, this is the correct indentation.
emacs's python mode will settle on this indent, too.

https://python.org/dev/peps/pep-0008/#indentation

(If anyone quotes Guido's belittling comment in this email, I will
become cross.)


But there are other places to put the line break. This is also
technically valid:

sys.stderr.write('qemu-img received signal %i: %s\n'
 % (-exitcode, ' '.join(qemu_img_args + list(args

And so is this:

sys.stderr.write('qemu-img received signal %i: %s\n' %
 (-exitcode, ' '.join(qemu_img_args + list(args

(And so would be any other number of rewrites to use format codes,
f-strings, or temporary variables to otherwise reduce the length of the
line.)

I will reluctantly admit that wrapping to 79 columns is useful in some
contexts. The beauty of line continuations is something I have little
desire to litigate, though.

If there's some consensus on the true and beautiful way to do it, I will
oblige -- but the thought of spinning more iterations until we find a
color swatch we like is an unpleasant one.

--js

[PATCH v2 2/3] iotests: add JobRunner class

[John Snow]

The idea is that instead of increasing the arguments to job_run all the
time, create a more general-purpose job runner that can be subclassed to
do interesting things with.

pylint note: the 'callbacks' option guards against unused warning
arguments in functions designated as callbacks. It does not currently
guard against "no-self-use" though; hence a once-off ignore.

mypy note: QapiEvent is only a weak alias; it's fully interchangable
with the type it's declared as. In the future, we may wish to tighten
these types. For now, this communicates the rough shape of the type and
(more importantly) the intent.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/255|   9 +-
 tests/qemu-iotests/257|  54 +
 tests/qemu-iotests/iotests.py | 201 +-
 tests/qemu-iotests/pylintrc   |  11 ++
 4 files changed, 200 insertions(+), 75 deletions(-)

diff --git a/tests/qemu-iotests/255 b/tests/qemu-iotests/255
index 8f08f741da..e66cdfd672 100755
--- a/tests/qemu-iotests/255
+++ b/tests/qemu-iotests/255
@@ -71,8 +71,13 @@ with iotests.FilePath('t.qcow2') as disk_path, \
 result = vm.qmp_log('block-commit', job_id='job0', auto_finalize=False,
 device='overlay', top_node='mid')
 
-vm.run_job('job0', auto_finalize=False, pre_finalize=start_requests,
-auto_dismiss=True)
+class TestJobRunner(iotests.JobRunner):
+def on_pending(self, event):
+start_requests()
+super().on_pending(event)
+
+runner = TestJobRunner(vm, 'job0', auto_finalize=False, auto_dismiss=True)
+runner.run()
 
 vm.shutdown()
 
diff --git a/tests/qemu-iotests/257 b/tests/qemu-iotests/257
index 004a433b8b..95341c330f 100755
--- a/tests/qemu-iotests/257
+++ b/tests/qemu-iotests/257
@@ -352,30 +352,40 @@ def test_bitmap_sync(bsync_mode, msync_mode='bitmap', 
failure=None):
 job = backup(drive0, 1, bsync1, msync_mode,
  bitmap="bitmap0", bitmap_mode=bsync_mode)
 
-def _callback():
-"""Issue writes while the job is open to test bitmap divergence."""
-# Note: when `failure` is 'intermediate', this isn't called.
-log('')
-bitmaps = perform_writes(drive0, 2, filter_node_name='backup-top')
-# Named bitmap (static, should be unchanged)
-ebitmap.compare(vm.get_bitmap(drive0.node, 'bitmap0',
-  bitmaps=bitmaps))
-# Anonymous bitmap (dynamic, shows new writes)
-anonymous = EmulatedBitmap()
-anonymous.dirty_group(2)
-anonymous.compare(vm.get_bitmap(drive0.node, '', recording=True,
-bitmaps=bitmaps))
 
-# Simulate the order in which this will happen:
-# group 1 gets cleared first, then group two gets written.
-if ((bsync_mode == 'on-success' and not failure) or
-(bsync_mode == 'always')):
-ebitmap.clear()
-ebitmap.dirty_group(2)
+class TestJobRunner(iotests.JobRunner):
+def on_pending(self, event):
+"""
+Issue writes while the job is open to test bitmap divergence.
+"""
+
+# Note: when `failure` is 'intermediate', this isn't called.
+log('')
+bitmaps = perform_writes(drive0, 2,
+ filter_node_name='backup-top')
+# Named bitmap (static, should be unchanged)
+ebitmap.compare(vm.get_bitmap(drive0.node, 'bitmap0',
+  bitmaps=bitmaps))
+# Anonymous bitmap (dynamic, shows new writes)
+anonymous = EmulatedBitmap()
+anonymous.dirty_group(2)
+anonymous.compare(vm.get_bitmap(drive0.node, '', 
recording=True,
+bitmaps=bitmaps))
+
+# Simulate the order in which this will happen:
+# group 1 gets cleared first, then group two gets written.
+if ((bsync_mode == 'on-success' and not failure) or
+(bsync_mode == 'always')):
+ebitmap.clear()
+ebitmap.dirty_group(2)
+
+super().on_pending(event)
+
+
+runner = TestJobRunner(vm, job, cancel=(failure == 'simulated'),
+   auto_finalize=False, auto_dismiss=True)
+runner.run()
 
-vm.run_job(job, auto_dismiss=True, auto_finalize=False,
-   pre_finalize=_callback,
-   cancel=(failure == 'simulated'))
 bitmaps = vm.query_bitmaps()
 log({'bitmaps': bitmaps}, indent=2)
 log('')
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 2625001978..90d42cdff1 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-io

[PATCH v2 1/3] qmp.py: change event_wait to use a dict

[John Snow]

It's easier to work with than a list of tuples, because we can check the
keys for membership.

Signed-off-by: John Snow 
---
 python/qemu/machine.py| 10 +-
 tests/qemu-iotests/040| 12 ++--
 tests/qemu-iotests/260|  5 +++--
 tests/qemu-iotests/iotests.py | 16 
 4 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 183d8f3d38..748de5f322 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -476,21 +476,21 @@ def event_wait(self, name, timeout=60.0, match=None):
 timeout: QEMUMonitorProtocol.pull_event timeout parameter.
 match: Optional match criteria. See event_match for details.
 """
-return self.events_wait([(name, match)], timeout)
+return self.events_wait({name: match}, timeout)
 
 def events_wait(self, events, timeout=60.0):
 """
 events_wait waits for and returns a named event from QMP with a 
timeout.
 
-events: a sequence of (name, match_criteria) tuples.
+events: a mapping containing {name: match_criteria}.
 The match criteria are optional and may be None.
 See event_match for details.
 timeout: QEMUMonitorProtocol.pull_event timeout parameter.
 """
 def _match(event):
-for name, match in events:
-if event['event'] == name and self.event_match(event, match):
-return True
+name = event['event']
+if name in events:
+return self.event_match(event, events[name])
 return False
 
 # Search cached events
diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 32c82b4ec6..90b59081ff 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -485,12 +485,12 @@ class TestErrorHandling(iotests.QMPTestCase):
 
 def run_job(self, expected_events, error_pauses_job=False):
 match_device = {'data': {'device': 'job0'}}
-events = [
-('BLOCK_JOB_COMPLETED', match_device),
-('BLOCK_JOB_CANCELLED', match_device),
-('BLOCK_JOB_ERROR', match_device),
-('BLOCK_JOB_READY', match_device),
-]
+events = {
+'BLOCK_JOB_COMPLETED': match_device,
+'BLOCK_JOB_CANCELLED': match_device,
+'BLOCK_JOB_ERROR': match_device,
+'BLOCK_JOB_READY': match_device,
+}
 
 completed = False
 log = []
diff --git a/tests/qemu-iotests/260 b/tests/qemu-iotests/260
index 804a7addb9..729f031122 100755
--- a/tests/qemu-iotests/260
+++ b/tests/qemu-iotests/260
@@ -67,8 +67,9 @@ def test(persistent, restart):
 
 vm.qmp_log('block-commit', device='drive0', top=top,
filters=[iotests.filter_qmp_testfiles])
-ev = vm.events_wait((('BLOCK_JOB_READY', None),
- ('BLOCK_JOB_COMPLETED', None)))
+ev = vm.events_wait({
+'BLOCK_JOB_READY': None,
+'BLOCK_JOB_COMPLETED': None })
 log(filter_qmp_event(ev))
 if (ev['event'] == 'BLOCK_JOB_COMPLETED'):
 vm.shutdown()
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 3e606c35ef..2625001978 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -614,14 +614,14 @@ def run_job(self, job, auto_finalize=True, 
auto_dismiss=False,
 """
 match_device = {'data': {'device': job}}
 match_id = {'data': {'id': job}}
-events = [
-('BLOCK_JOB_COMPLETED', match_device),
-('BLOCK_JOB_CANCELLED', match_device),
-('BLOCK_JOB_ERROR', match_device),
-('BLOCK_JOB_READY', match_device),
-('BLOCK_JOB_PENDING', match_id),
-('JOB_STATUS_CHANGE', match_id)
-]
+events = {
+'BLOCK_JOB_COMPLETED': match_device,
+'BLOCK_JOB_CANCELLED': match_device,
+'BLOCK_JOB_ERROR': match_device,
+'BLOCK_JOB_READY': match_device,
+'BLOCK_JOB_PENDING': match_id,
+'JOB_STATUS_CHANGE': match_id,
+}
 error = None
 while True:
 ev = filter_qmp_event(self.events_wait(events, timeout=wait))
-- 
2.21.1

[PATCH v2 3/3] iotests: modify test 040 to use JobRunner

[John Snow]

Instead of having somewhat reproduced it for itself.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/040 | 51 +-
 1 file changed, 25 insertions(+), 26 deletions(-)

diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 90b59081ff..e2ef3bb812 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -483,34 +483,33 @@ class TestErrorHandling(iotests.QMPTestCase):
   file=('top-dbg' if top_debug else 'top-file'),
   backing='mid-fmt')
 
+
+class TestJobRunner(iotests.JobRunner):
+expected_events = ('BLOCK_JOB_COMPLETED',
+   'BLOCK_JOB_ERROR',
+   'BLOCK_JOB_READY')
+
+def __init__(self, *args, test, **kwargs):
+super().__init__(*args, **kwargs)
+self.log = []
+self.test = test
+
+def on_pause(self, event):
+super().on_pause(event)
+result = self._vm.qmp('block-job-resume', device=self._id)
+self.test.assert_qmp(result, 'return', {})
+
+def on_block_job_event(self, event):
+if event['event'] not in self.expected_events:
+self.test.fail("Unexpected event: %s" % event)
+super().on_block_job_event(event)
+self.log.append(event)
+
 def run_job(self, expected_events, error_pauses_job=False):
-match_device = {'data': {'device': 'job0'}}
-events = {
-'BLOCK_JOB_COMPLETED': match_device,
-'BLOCK_JOB_CANCELLED': match_device,
-'BLOCK_JOB_ERROR': match_device,
-'BLOCK_JOB_READY': match_device,
-}
-
-completed = False
-log = []
-while not completed:
-ev = self.vm.events_wait(events, timeout=5.0)
-if ev['event'] == 'BLOCK_JOB_COMPLETED':
-completed = True
-elif ev['event'] == 'BLOCK_JOB_ERROR':
-if error_pauses_job:
-result = self.vm.qmp('block-job-resume', device='job0')
-self.assert_qmp(result, 'return', {})
-elif ev['event'] == 'BLOCK_JOB_READY':
-result = self.vm.qmp('block-job-complete', device='job0')
-self.assert_qmp(result, 'return', {})
-else:
-self.fail("Unexpected event: %s" % ev)
-log.append(iotests.filter_qmp_event(ev))
-
+job = self.TestJobRunner(self.vm, 'job0', test=self)
+job.run()
 self.maxDiff = None
-self.assertEqual(expected_events, log)
+self.assertEqual(expected_events, job.log)
 
 def event_error(self, op, action):
 return {
-- 
2.21.1

[PATCH v2 0/3] iotests: add JobRunner framework

[John Snow]

Requires: 20200304213818.15341-1-js...@redhat.com

(This requires the iotests pylint & logging series.)

The basic idea is to make a generic job runtime manager and allow
callers to subclass the manager. Then, instead of adding callback
arguments to the function all the time, we have à la carte customization
of the loop.

To showcase this a little bit, I removed the pre_finalization argument
and made existing callers use a custom JobRunner; and then converted
test 040 to use this style of job runner.

Is it a simplification? No. Is it cool? Maybe. Did it remove the
duplicated job-running code in 040? yes.

V2:
 - Rebased on logging series; logging conditionals are pretty now.
 - Inlined callback login in 257
 - No longer based on bitmap-populate job (no test 287)
 - Moved super() call to the beginning of test 040's callback
 - Added docstrings and type annotations

John Snow (3):
  qmp.py: change event_wait to use a dict
  iotests: add JobRunner class
  iotests: modify test 040 to use JobRunner

 python/qemu/machine.py|  10 +-
 tests/qemu-iotests/040|  51 +
 tests/qemu-iotests/255|   9 +-
 tests/qemu-iotests/257|  54 +
 tests/qemu-iotests/260|   5 +-
 tests/qemu-iotests/iotests.py | 201 +-
 tests/qemu-iotests/pylintrc   |  11 ++
 7 files changed, 233 insertions(+), 108 deletions(-)

-- 
2.21.1

Re: [PATCH v3 11/20] hw/ide/internal: Remove unused DMARestartFunc typedef

[John Snow]

On 2/20/20 8:05 AM, Philippe Mathieu-Daudé wrote:
> The IDE DMA restart callback has been removed in commit fe09c7c9f0.
> 
> Fixes: fe09c7c9f0
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  include/hw/ide/internal.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
> index 52ec197da0..ce766ac485 100644
> --- a/include/hw/ide/internal.h
> +++ b/include/hw/ide/internal.h
> @@ -326,7 +326,6 @@ typedef int DMAIntFunc(IDEDMA *, int);
>  typedef int32_t DMAInt32Func(IDEDMA *, int32_t len);
>  typedef void DMAu32Func(IDEDMA *, uint32_t);
>  typedef void DMAStopFunc(IDEDMA *, bool);
> -typedef void DMARestartFunc(void *, int, RunState);
>  
>  struct unreported_events {
>  bool eject_request;
> 

Acked-by: John Snow 

Re: [PATCH v3 12/20] hw/ide: Let the DMAIntFunc prototype use a boolean 'is_write' argument

[John Snow]

On 2/20/20 8:05 AM, Philippe Mathieu-Daudé wrote:
> The 'is_write' argument is either 0 or 1.
> Convert it to a boolean type.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  include/hw/ide/internal.h | 2 +-
>  hw/dma/rc4030.c   | 6 +++---
>  hw/ide/ahci.c | 2 +-
>  hw/ide/core.c | 2 +-
>  hw/ide/macio.c| 2 +-
>  hw/ide/pci.c  | 2 +-
>  6 files changed, 8 insertions(+), 8 deletions(-)

Acked-by: John Snow 

Re: [PATCH v6 9/9] iotests: add pylintrc file

[Markus Armbruster]

John Snow  writes:

> On 3/4/20 2:22 AM, Markus Armbruster wrote:
>> John Snow  writes:
>> 
>>> Repeatable results. run `pylint iotests.py` and you should get a pass.
>> 
>> Start your sentences with a capital letter, please.
>> 
>
> The German complains about the capitalization, but not the sentence
> fragment.

Heh!

>>>
>>> Signed-off-by: John Snow 
>>> ---
>>>  tests/qemu-iotests/pylintrc | 20 
>>>  1 file changed, 20 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..feed506f75
>>> --- /dev/null
>>> +++ b/tests/qemu-iotests/pylintrc
>>> @@ -0,0 +1,20 @@
>>> +[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,
>>> +missing-docstring,
>>> +line-too-long,
>>> +too-many-lines,
>>> +too-few-public-methods,
>>> +too-many-arguments,
>>> +too-many-locals,
>>> +too-many-branches,
>>> +too-many-public-methods,
>>> \ No newline at end of file
>> 
>> Add the newline, please.
>> 
>> German pejorative for the too-many- and too-few- warnings: "Müsli".
>> Implies it's for muesli-knitters / granola-crunchers indulging their
>> orthorexia.
>> 
>
> They are useful at times as they can suggest when you are going a bit
> overboard on "organically grown" design. For cleaning an existing
> codebase, it's more of a hindrance to the immediate goal of establishing
> a baseline.

Yes, gentle nudges to reconsider your code organization can be useful.
But when we run pylint with the goal of getting no output, even warnings
are much more than gentle nudges.

> (*cough* I try to adhere to them in my own freshly written code, and
> disable per-line when I've decided to veto the suggestion. Not
> appropriate for a codebase like ours. As Max reminds me, it's just tests
> -- don't make them too clever or pretty.)
>
> Regardless. It's not appropriate here and now.
>
>> missing-docstring is not advisable for libraries.  Feels okay here.
>> 
>
> Ideally we do start using them, but it's out of scope here. Since I did
> some cleanup, I wanted to establish the baseline of what I adhered to.
>
> *not* suggest that it's the destination state.
>
> Adding proper docstrings should be done during mypy conversion once the
> types are determined, understood, and enforced. Not before then.
>
>> line-too-long might be worth cleaning up.  How many of them do we have
>> now?
>> 
>
> Five in iotests.py using the default length of 100. 15 if I limit to 80.
>
> If we agree that 100 is okay, I can tackle this in an amendment patch.
> If 80 is okay, I'm going to put it off as one coat of paint too many.
>
> (I will try to clean up the 100+ lines for my next version. I am
> hesitant to make a deeper cut because I have the feeling it's the type
> of series that will incur a lot of nitpicks on indent style.)

One step at a time.