mbox series

[v5,0/5] block: widen and check consistency of size-related BlockConf properties

Message ID 20200526081740.256236-1-rvkagan@yandex-team.ru
Headers show
Series block: widen and check consistency of size-related BlockConf properties | expand

Message

Roman Kagan May 26, 2020, 8:17 a.m. UTC
BlockConf includes several properties counted in bytes.

Enhance their handling in a some aspects, specifically

- accept common size suffixes (k, m)
- perform consistency checks on the values, to prevent their silent
  truncation and rounding
- lift the upper limit on physical_block_size and logical_block_size

Also fix the accessor for opt_io_size in virtio-blk to make it match the size
of the field.

History:
v4 -> v5:
- re-split the patches [Philippe]
- fix/reword error messages [Philippe, Kevin]
- do early return on failed consistency check [Philippe]
- use QEMU_IS_ALIGNED instead of open coding [Philippe]
- make all BlockConf size props support suffixes
- expand the log for virtio-blk opt_io_size [Michael]

v3 -> v4:
- add patch to fix opt_io_size width in virtio-blk
- add patch to perform consistency checks [Kevin]
- check min_io_size against truncation [Kevin]

v2 -> v3:
- mention qcow2 cluster size limit in the log and comment [Eric]

v1 -> v2:
- cap the property at 2 MiB [Eric]
- accept size suffixes

Roman Kagan (5):
  virtio-blk: store opt_io_size with correct size
  block: consolidate blocksize properties consistency checks
  qdev-properties: blocksize: use same limits in code and description
  block: make size-related BlockConf properties accept size suffixes
  block: lift blocksize property limit to 2 MiB

 include/hw/block/block.h     |  18 +-
 include/hw/qdev-properties.h |   2 +-
 hw/block/block.c             |  64 ++++-
 hw/block/fdc.c               |   5 +-
 hw/block/nvme.c              |   5 +-
 hw/block/virtio-blk.c        |   9 +-
 hw/core/qdev-properties.c    | 109 +++----
 hw/ide/qdev.c                |   5 +-
 hw/scsi/scsi-disk.c          |  12 +-
 hw/usb/dev-storage.c         |   5 +-
 tests/qemu-iotests/172.out   | 532 +++++++++++++++++------------------
 11 files changed, 423 insertions(+), 343 deletions(-)

Comments

no-reply@patchew.org May 26, 2020, 10:42 a.m. UTC | #1
Patchew URL: https://patchew.org/QEMU/20200526081740.256236-1-rvkagan@yandex-team.ru/



Hi,

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

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

  CC      hw/core/hotplug.o
  CC      hw/core/vmstate-if.o
/tmp/qemu-test/src/hw/block/xen-block.c: In function 'xen_block_realize':
/tmp/qemu-test/src/hw/block/xen-block.c:242:5: error: too few arguments to function 'blkconf_blocksizes'
     blkconf_blocksizes(conf);
     ^
In file included from /tmp/qemu-test/src/include/hw/xen/xen-block.h:12:0,
                 from /tmp/qemu-test/src/hw/block/xen-block.c:25:
/tmp/qemu-test/src/include/hw/block/block.h:90:6: note: declared here
 bool blkconf_blocksizes(BlockConf *conf, Error **errp);
      ^
/tmp/qemu-test/src/hw/block/xen-block.c:260:35: error: format '%u' expects argument of type 'unsigned int', but argument 4 has type 'uint64_t' [-Werror=format=]
                                   conf->discard_granularity);
                                   ^
/tmp/qemu-test/src/hw/block/xen-block.c:274:31: error: format '%u' expects argument of type 'unsigned int', but argument 4 has type 'uint64_t' [-Werror=format=]
                               conf->logical_block_size);
                               ^
cc1: all warnings being treated as errors
  CC      hw/core/irq.o
make: *** [hw/block/xen-block.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in <module>
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=241d08d624884dde8ed7b9b977600893', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-31zomjr6/src/docker-src.2020-05-26-06.40.14.18987:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=241d08d624884dde8ed7b9b977600893
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-31zomjr6/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    2m12.190s
user    0m8.441s


The full log is available at
http://patchew.org/logs/20200526081740.256236-1-rvkagan@yandex-team.ru/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
no-reply@patchew.org May 26, 2020, 10:46 a.m. UTC | #2
Patchew URL: https://patchew.org/QEMU/20200526081740.256236-1-rvkagan@yandex-team.ru/



Hi,

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

=== TEST SCRIPT BEGIN ===
#!/bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      hw/display/ramfb-standalone.o
  CC      hw/display/cirrus_vga.o
  CC      hw/display/cirrus_vga_isa.o
/tmp/qemu-test/src/hw/block/xen-block.c:242:28: error: too few arguments to function call, expected 2, have 1
    blkconf_blocksizes(conf);
    ~~~~~~~~~~~~~~~~~~     ^
/tmp/qemu-test/src/include/hw/block/block.h:90:1: note: 'blkconf_blocksizes' declared here
bool blkconf_blocksizes(BlockConf *conf, Error **errp);
^
/usr/lib64/clang/8.0.0/include/stdbool.h:31:14: note: expanded from macro 'bool'
#define bool _Bool
             ^
/tmp/qemu-test/src/hw/block/xen-block.c:260:35: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat]
                                  conf->discard_granularity);
                                  ^~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/qemu-test/src/hw/block/xen-block.c:274:31: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat]
                              conf->logical_block_size);
                              ^~~~~~~~~~~~~~~~~~~~~~~~
3 errors generated.
  CC      hw/display/xenfb.o
make: *** [/tmp/qemu-test/src/rules.mak:69: hw/block/xen-block.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in <module>
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=51c33fdd1b004bd7aea32bcbcf627ced', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=x86_64-softmmu', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-dii29f5p/src/docker-src.2020-05-26-06.43.06.26195:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-debug']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=51c33fdd1b004bd7aea32bcbcf627ced
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-dii29f5p/src'
make: *** [docker-run-test-debug@fedora] Error 2

real    3m36.906s
user    0m9.145s


The full log is available at
http://patchew.org/logs/20200526081740.256236-1-rvkagan@yandex-team.ru/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
no-reply@patchew.org May 26, 2020, 10:47 a.m. UTC | #3
Patchew URL: https://patchew.org/QEMU/20200526081740.256236-1-rvkagan@yandex-team.ru/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20200526081740.256236-1-rvkagan@yandex-team.ru
Subject: [PATCH v5 0/5] block: widen and check consistency of size-related BlockConf properties
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
6611c53 block: lift blocksize property limit to 2 MiB
1efe094 block: make size-related BlockConf properties accept size suffixes
f9d88df qdev-properties: blocksize: use same limits in code and description
bee66cf block: consolidate blocksize properties consistency checks
8b2fdc1 virtio-blk: store opt_io_size with correct size

=== OUTPUT BEGIN ===
1/5 Checking commit 8b2fdc1b18b2 (virtio-blk: store opt_io_size with correct size)
2/5 Checking commit bee66cfde76f (block: consolidate blocksize properties consistency checks)
3/5 Checking commit f9d88df4c03e (qdev-properties: blocksize: use same limits in code and description)
4/5 Checking commit 1efe094b7637 (block: make size-related BlockConf properties accept size suffixes)
ERROR: line over 90 characters
#200: FILE: hw/core/qdev-properties.c:1244:
+                  "Property %s.%s doesn't take value '%" PRId64 "', it's not a power of 2",

total: 1 errors, 0 warnings, 1070 lines checked

Patch 4/5 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

5/5 Checking commit 6611c538915a (block: lift blocksize property limit to 2 MiB)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200526081740.256236-1-rvkagan@yandex-team.ru/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Roman Kagan May 26, 2020, 11 a.m. UTC | #4
Please ignore the series, I forgot xen-block device.

Roman.

On Tue, May 26, 2020 at 11:17:35AM +0300, Roman Kagan wrote:
> BlockConf includes several properties counted in bytes.
> 
> Enhance their handling in a some aspects, specifically
> 
> - accept common size suffixes (k, m)
> - perform consistency checks on the values, to prevent their silent
>   truncation and rounding
> - lift the upper limit on physical_block_size and logical_block_size
> 
> Also fix the accessor for opt_io_size in virtio-blk to make it match the size
> of the field.
> 
> History:
> v4 -> v5:
> - re-split the patches [Philippe]
> - fix/reword error messages [Philippe, Kevin]
> - do early return on failed consistency check [Philippe]
> - use QEMU_IS_ALIGNED instead of open coding [Philippe]
> - make all BlockConf size props support suffixes
> - expand the log for virtio-blk opt_io_size [Michael]
> 
> v3 -> v4:
> - add patch to fix opt_io_size width in virtio-blk
> - add patch to perform consistency checks [Kevin]
> - check min_io_size against truncation [Kevin]
> 
> v2 -> v3:
> - mention qcow2 cluster size limit in the log and comment [Eric]
> 
> v1 -> v2:
> - cap the property at 2 MiB [Eric]
> - accept size suffixes
> 
> Roman Kagan (5):
>   virtio-blk: store opt_io_size with correct size
>   block: consolidate blocksize properties consistency checks
>   qdev-properties: blocksize: use same limits in code and description
>   block: make size-related BlockConf properties accept size suffixes
>   block: lift blocksize property limit to 2 MiB
> 
>  include/hw/block/block.h     |  18 +-
>  include/hw/qdev-properties.h |   2 +-
>  hw/block/block.c             |  64 ++++-
>  hw/block/fdc.c               |   5 +-
>  hw/block/nvme.c              |   5 +-
>  hw/block/virtio-blk.c        |   9 +-
>  hw/core/qdev-properties.c    | 109 +++----
>  hw/ide/qdev.c                |   5 +-
>  hw/scsi/scsi-disk.c          |  12 +-
>  hw/usb/dev-storage.c         |   5 +-
>  tests/qemu-iotests/172.out   | 532 +++++++++++++++++------------------
>  11 files changed, 423 insertions(+), 343 deletions(-)
> 
> -- 
> 2.26.2
> 
>