Re: [PATCH 0/2] replace sysconf(_SC_PAGESIZE) with qemu_real_host_page_size

[Wei Yang]

On Tue, Oct 15, 2019 at 11:13:48AM +0800, Wei Yang wrote:
>This is a following up patch to cleanup page size, suggested by
>"Dr. David Alan Gilbert" .
>
>Patch 2 does the job, while during the cleanup I found test-mmap.c has quite a
>lot code style problem. To make the code looks good, patch 1 is introduced to
>make checkpatch.pl happy a little.

Does this patch set look good?

>
>Wei Yang (2):
>  tests/tcg/multiarch: fix code style in function main of test-mmap.c
>  core: replace sysconf(_SC_PAGESIZE) with qemu_real_host_page_size
>
> block/file-posix.c  |  2 +-
> net/l2tpv3.c|  2 +-
> tests/tcg/multiarch/test-mmap.c | 67 ++---
> 3 files changed, 38 insertions(+), 33 deletions(-)
>
>-- 
>2.17.1
>

-- 
Wei Yang
Help you, Help me

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

[Eric Blake]

On 11/8/19 3:10 PM, Marc-André Lureau wrote:


+/*
+ * ERRP_AUTO_PROPAGATE
+ *
+ * This macro is created to be the first line of a function with Error **errp
+ * OUT parameter. It's needed only in cases where we want to use error_prepend,
+ * error_append_hint or dereference *errp. It's still safe (but useless) in
+ * other cases.
+ *
+ * If errp is NULL or points to error_fatal, it is rewritten to point to a
+ * local Error object, which will be automatically propagated to the original
+ * errp on function exit (see error_propagator_cleanup).
+ *
+ * After invocation of this macro it is always safe to dereference errp
+ * (as it's not NULL anymore) and to add information (by error_prepend or
+ * error_append_hint)
+ * (as, if it was error_fatal, we swapped it with a local_error to be
+ * propagated on cleanup).


Nice improvements. Minor drawback, the abort()/exit() will now take
place when going out of scope and running the cleanup instead of error
location. Not a big problem I guess.


Your assessment is not quite right:

Any abort() will happen at the leaf node (because we are no longer 
wrapping thing into a local err and skipping error_propagate altogether 
for _abort).


You are correct that any exit() will now happen during cleanup, but that 
is an undetectable change (there is no stack trace present for 
_fatal, so calling error_propagate at a later point in time does 
not affect the observable end behavior).


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

Re: [RFC v5 026/126] python: add commit-per-subsystem.py

[Marc-André Lureau]

Hi

On Fri, Oct 11, 2019 at 9:11 PM Vladimir Sementsov-Ogievskiy
 wrote:
>
> Add script to automatically commit tree-wide changes per-subsystem.

Oh interesting! I guess it could use a --help or a larger commit
message to explain a bit what it does (I imagine from the rest of the
series, but someone looking at the script without context may wonder;)

You could also fix some pep8/pylint/pycodestyle

>
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>
> CC: Gerd Hoffmann 
> CC: "Gonglei (Arei)" 
> CC: Eduardo Habkost 
> CC: Igor Mammedov 
> CC: Laurent Vivier 
> CC: Amit Shah 
> CC: Kevin Wolf 
> CC: Max Reitz 
> CC: John Snow 
> CC: Ari Sundholm 
> CC: Pavel Dovgalyuk 
> CC: Paolo Bonzini 
> CC: Stefan Hajnoczi 
> CC: Fam Zheng 
> CC: Stefan Weil 
> CC: Ronnie Sahlberg 
> CC: Peter Lieven 
> CC: Eric Blake 
> CC: "Denis V. Lunev" 
> CC: Markus Armbruster 
> CC: Alberto Garcia 
> CC: Jason Dillaman 
> CC: Wen Congyang 
> CC: Xie Changlong 
> CC: Liu Yuan 
> CC: "Richard W.M. Jones" 
> CC: Jeff Cody 
> CC: "Marc-André Lureau" 
> CC: "Daniel P. Berrangé" 
> CC: Richard Henderson 
> CC: Greg Kurz 
> CC: "Michael S. Tsirkin" 
> CC: Marcel Apfelbaum 
> CC: Beniamino Galvani 
> CC: Peter Maydell 
> CC: "Cédric Le Goater" 
> CC: Andrew Jeffery 
> CC: Joel Stanley 
> CC: Andrew Baumann 
> CC: "Philippe Mathieu-Daudé" 
> CC: Antony Pavlov 
> CC: Jean-Christophe Dubois 
> CC: Peter Chubb 
> CC: Subbaraya Sundeep 
> CC: Eric Auger 
> CC: Alistair Francis 
> CC: "Edgar E. Iglesias" 
> CC: Stefano Stabellini 
> CC: Anthony Perard 
> CC: Paul Durrant 
> CC: Paul Burton 
> CC: Aleksandar Rikalo 
> CC: Chris Wulff 
> CC: Marek Vasut 
> CC: David Gibson 
> CC: Cornelia Huck 
> CC: Halil Pasic 
> CC: Christian Borntraeger 
> CC: "Hervé Poussineau" 
> CC: Xiao Guangrong 
> CC: Aurelien Jarno 
> CC: Aleksandar Markovic 
> CC: Mark Cave-Ayland 
> CC: Jason Wang 
> CC: Laszlo Ersek 
> CC: Yuval Shaia 
> CC: Palmer Dabbelt 
> CC: Sagar Karandikar 
> CC: Bastian Koppelmann 
> CC: David Hildenbrand 
> CC: Thomas Huth 
> CC: Eric Farman 
> CC: Matthew Rosato 
> CC: Hannes Reinecke 
> CC: Michael Walle 
> CC: Artyom Tarasenko 
> CC: Stefan Berger 
> CC: Samuel Thibault 
> CC: Alex Williamson 
> CC: Tony Krowiak 
> CC: Pierre Morel 
> CC: Michael Roth 
> CC: Hailiang Zhang 
> CC: Juan Quintela 
> CC: "Dr. David Alan Gilbert" 
> CC: Luigi Rizzo 
> CC: Giuseppe Lettieri 
> CC: Vincenzo Maffione 
> CC: Jan Kiszka 
> CC: Anthony Green 
> CC: Stafford Horne 
> CC: Guan Xuetao 
> CC: Max Filippov 
> CC: qemu-block@nongnu.org
> CC: integrat...@gluster.org
> CC: sheep...@lists.wpkg.org
> CC: qemu-...@nongnu.org
> CC: xen-de...@lists.xenproject.org
> CC: qemu-...@nongnu.org
> CC: qemu-s3...@nongnu.org
> CC: qemu-ri...@nongnu.org
>
>  python/commit-per-subsystem.py | 204 +
>  1 file changed, 204 insertions(+)
>  create mode 100755 python/commit-per-subsystem.py
>
> diff --git a/python/commit-per-subsystem.py b/python/commit-per-subsystem.py
> new file mode 100755
> index 00..2ccf84cb15
> --- /dev/null
> +++ b/python/commit-per-subsystem.py
> @@ -0,0 +1,204 @@
> +#!/usr/bin/env python3
> +#
> +# Copyright (c) 2019 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 .
> +#
> +
> +import subprocess
> +import sys
> +import os
> +import glob
> +
> +
> +def git_add(pattern):
> +subprocess.run(['git', 'add', pattern])
> +
> +
> +def git_commit(msg):
> +subprocess.run(['git', 'commit', '-m', msg], capture_output=True)
> +
> +
> +def git_changed_files():
> +ret = subprocess.check_output(['git', 'diff', '--name-only'], 
> encoding='utf-8').split('\n')
> +if ret[-1] == '':
> +del ret[-1]
> +return ret
> +
> +
> +maintainers = sys.argv[1]
> +message = sys.argv[2].strip()
> +
> +subsystem = None
> +
> +remap = {
> +'Block layer core': 'block',
> +'Block Jobs': 'block',
> +'Dirty Bitmaps': 'block',
> +'Block QAPI, monitor, command line': 'block',
> +'Block I/O path': 'block',
> +'Throttling infrastructure': 'block',
> +'Architecture support': 's390x',
> +'Guest CPU Cores (KVM)': 'kvm',
> +'Guest CPU Cores (Xen)': 'xen',
> +'Guest CPU cores (TCG)': 'tcg',
> +'Network Block Device (NBD)': 'nbd',
> +'Parallel NOR Flash devices': 'pflash',
> +'Firmware configuration (fw_cfg)': 'fw_cfg',
> +'Block 

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

[Marc-André Lureau]

On Fri, Oct 11, 2019 at 10:11 PM Vladimir Sementsov-Ogievskiy
 wrote:
>
> Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
> functions with errp OUT parameter.
>
> It has three goals:
>
> 1. Fix issue with error_fatal & error_prepend/error_append_hint: user
> can't see this additional information, because exit() happens in
> error_setg earlier than information is added. [Reported by Greg Kurz]
>
> 2. Fix issue with error_abort & error_propagate: when we wrap
> error_abort by local_err+error_propagate, resulting coredump will
> refer to error_propagate and not to the place where error happened.
> (the macro itself doesn't fix the issue, but it allows to [3.] drop all
> local_err+error_propagate pattern, which will definitely fix the issue)
> [Reported by Kevin Wolf]
>
> 3. Drop local_err+error_propagate pattern, which is used to workaround
> void functions with errp parameter, when caller wants to know resulting
> status. (Note: actually these functions could be merely updated to
> return int error code).
>
> To achieve these goals, we need to add invocation of the macro at start
> of functions, which needs error_prepend/error_append_hint (1.); add
> invocation of the macro at start of functions which do
> local_err+error_propagate scenario the check errors, drop local errors
> from them and just use *errp instead (2., 3.).
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> Reviewed-by: Eric Blake 
> ---
>
> CC: Gerd Hoffmann 
> CC: "Gonglei (Arei)" 
> CC: Eduardo Habkost 
> CC: Igor Mammedov 
> CC: Laurent Vivier 
> CC: Amit Shah 
> CC: Kevin Wolf 
> CC: Max Reitz 
> CC: John Snow 
> CC: Ari Sundholm 
> CC: Pavel Dovgalyuk 
> CC: Paolo Bonzini 
> CC: Stefan Hajnoczi 
> CC: Fam Zheng 
> CC: Stefan Weil 
> CC: Ronnie Sahlberg 
> CC: Peter Lieven 
> CC: Eric Blake 
> CC: "Denis V. Lunev" 
> CC: Markus Armbruster 
> CC: Alberto Garcia 
> CC: Jason Dillaman 
> CC: Wen Congyang 
> CC: Xie Changlong 
> CC: Liu Yuan 
> CC: "Richard W.M. Jones" 
> CC: Jeff Cody 
> CC: "Marc-André Lureau" 
> CC: "Daniel P. Berrangé" 
> CC: Richard Henderson 
> CC: Greg Kurz 
> CC: "Michael S. Tsirkin" 
> CC: Marcel Apfelbaum 
> CC: Beniamino Galvani 
> CC: Peter Maydell 
> CC: "Cédric Le Goater" 
> CC: Andrew Jeffery 
> CC: Joel Stanley 
> CC: Andrew Baumann 
> CC: "Philippe Mathieu-Daudé" 
> CC: Antony Pavlov 
> CC: Jean-Christophe Dubois 
> CC: Peter Chubb 
> CC: Subbaraya Sundeep 
> CC: Eric Auger 
> CC: Alistair Francis 
> CC: "Edgar E. Iglesias" 
> CC: Stefano Stabellini 
> CC: Anthony Perard 
> CC: Paul Durrant 
> CC: Paul Burton 
> CC: Aleksandar Rikalo 
> CC: Chris Wulff 
> CC: Marek Vasut 
> CC: David Gibson 
> CC: Cornelia Huck 
> CC: Halil Pasic 
> CC: Christian Borntraeger 
> CC: "Hervé Poussineau" 
> CC: Xiao Guangrong 
> CC: Aurelien Jarno 
> CC: Aleksandar Markovic 
> CC: Mark Cave-Ayland 
> CC: Jason Wang 
> CC: Laszlo Ersek 
> CC: Yuval Shaia 
> CC: Palmer Dabbelt 
> CC: Sagar Karandikar 
> CC: Bastian Koppelmann 
> CC: David Hildenbrand 
> CC: Thomas Huth 
> CC: Eric Farman 
> CC: Matthew Rosato 
> CC: Hannes Reinecke 
> CC: Michael Walle 
> CC: Artyom Tarasenko 
> CC: Stefan Berger 
> CC: Samuel Thibault 
> CC: Alex Williamson 
> CC: Tony Krowiak 
> CC: Pierre Morel 
> CC: Michael Roth 
> CC: Hailiang Zhang 
> CC: Juan Quintela 
> CC: "Dr. David Alan Gilbert" 
> CC: Luigi Rizzo 
> CC: Giuseppe Lettieri 
> CC: Vincenzo Maffione 
> CC: Jan Kiszka 
> CC: Anthony Green 
> CC: Stafford Horne 
> CC: Guan Xuetao 
> CC: Max Filippov 
> CC: qemu-block@nongnu.org
> CC: integrat...@gluster.org
> CC: sheep...@lists.wpkg.org
> CC: qemu-...@nongnu.org
> CC: xen-de...@lists.xenproject.org
> CC: qemu-...@nongnu.org
> CC: qemu-s3...@nongnu.org
> CC: qemu-ri...@nongnu.org
>
>  include/qapi/error.h | 38 ++
>  1 file changed, 38 insertions(+)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index d6898d833b..47238d9065 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -345,6 +345,44 @@ void error_set_internal(Error **errp,
>  ErrorClass err_class, const char *fmt, ...)
>  GCC_FMT_ATTR(6, 7);
>
> +typedef struct ErrorPropagator {
> +Error *local_err;
> +Error **errp;
> +} ErrorPropagator;
> +
> +static inline void error_propagator_cleanup(ErrorPropagator *prop)
> +{
> +error_propagate(prop->errp, prop->local_err);
> +}
> +
> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
> +
> +/*
> + * ERRP_AUTO_PROPAGATE
> + *
> + * This macro is created to be the first line of a function with Error **errp
> + * OUT parameter. It's needed only in cases where we want to use 
> error_prepend,
> + * error_append_hint or dereference *errp. It's still safe (but useless) in
> + * other cases.
> + *
> + * If errp is NULL or points to error_fatal, it is rewritten to point to a
> + * local Error object, which will be automatically propagated to the original
> + * errp on function exit (see 

Re: [PATCH v2 0/1] virtio: fix IO request length in virtio SCSI/block

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/20191108134249.19004-1-dplotni...@virtuozzo.com/



Hi,

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

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

  TESTcheck-qtest-x86_64: tests/ahci-test
  TESTcheck-unit: tests/test-aio-multithread
test-aio-multithread: /tmp/qemu-test/src/util/async.c:279: aio_ctx_finalize: 
Assertion `((>scheduled_coroutines)->slh_first == ((void *)0))' failed.
ERROR - too few tests run (expected 6, got 2)
make: *** [check-unit] Error 1
make: *** Waiting for unfinished jobs
  TESTiotest-qcow2: 013
  TESTiotest-qcow2: 017
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', 
'--label', 'com.qemu.instance.uuid=66f67685ef3547788ddbfca43e0e9250', '-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-oc5hqlfd/src/docker-src.2019-11-08-14.32.14.21571:/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=66f67685ef3547788ddbfca43e0e9250
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-oc5hqlfd/src'
make: *** [docker-run-test-quick@centos7] Error 2

real11m1.820s
user0m8.507s


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

Re: [RFC v5 000/126] error: auto propagated local_err

[Marc-André Lureau]

Hi

On Fri, Nov 8, 2019 at 7:31 PM Vladimir Sementsov-Ogievskiy
 wrote:
>
> Finally, what is the plan?
>
> Markus what do you think?
>
> Now a lot of patches are reviewed, but a lot of are not.
>
> Is there any hope that all patches will be reviewed? Should I resend the
> whole series, or may be reduce it to reviewed subsystems only?

I don't think we have well established rules for whole-tree cleanups
like this. In the past, several cleanup series got lost.

It will take ages to get every subsystem maintainer to review the
patches. Most likely, since they are quite systematic, there isn't
much to say and it is easy to miss something that has some hidden
ramifications. Perhaps whole-tree cleanups should require at least 2
reviewers to bypass the subsytem maintainer review? But my past
experience with this kind of exercice doesn't encourage me, and
probably I am not the only one.

>
> 11.10.2019 19:03, Vladimir Sementsov-Ogievskiy wrote:
> > Hi all!
> >
> > At the request of Markus: full version of errp propagation. Let's look
> > at it. Cover as much as possible, except inserting macro invocation
> > where it's not necessary.
> >
> > It's huge, and so it's an RFC.
> >
> > In v5 I've added a lot more preparation cleanups:
> > 01-23 are preparation cleanups
> >01: not changed, keep Eric's r-b
> >02: improve commit msg [Markus], keep Eric's r-b
> >03: changed, only error API here, drop r-b
> > 24 is core macro
> >- improve cover letter, wording and macro code style
> >- keep Eric's r-b
> > 25-26: automation scripts
> > - commit-per-subsystem changed a lot. it's a draft, don't bother too
> >   much with it
> > - coccinelle: add support of error_propagate_prepend
> >
> > 27-126: generated patches
> >
> > 
> >
> > Here is a proposal of auto propagation for local_err, to not call
> > error_propagate on every exit point, when we deal with local_err.
> >
> > There are also two issues with errp:
> >
> > 1. error_fatal & error_append_hint/error_prepend: user can't see this
> > additional info, because exit() happens in error_setg earlier than info
> > is added. [Reported by Greg Kurz]
> >
> > 2. error_abort & error_propagate: when we wrap
> > error_abort by local_err+error_propagate, resulting coredump will
> > refer to error_propagate and not to the place where error happened.
> > (the macro itself don't fix the issue, but it allows to [3.] drop all
> > local_err+error_propagate pattern, which will definitely fix the issue)
> > [Reported by Kevin Wolf]
> >
> > 
> >
> > Generated patches split:
> >
> > misc
> > hw/misc/ivshmem.c
> > hw/misc/tmp105.c
> > hw/misc/tmp421.c
> > s390x
> > hw/intc/s390_flic_kvm.c
> > hw/s390x/3270-ccw.c
> > hw/s390x/css-bridge.c
> > hw/s390x/css.c
> > hw/s390x/s390-skeys.c
> > hw/s390x/s390-virtio-ccw.c
> > hw/s390x/sclp.c
> > hw/s390x/tod-kvm.c
> > hw/vfio/ccw.c
> > target/s390x/cpu.c
> > tcg
> > exec.c
> > hw/arm/armv7m.c
> > hw/arm/smmu-common.c
> > hw/arm/smmuv3.c
> > hw/cpu/a15mpcore.c
> > hw/cpu/a9mpcore.c
> > hw/cpu/arm11mpcore.c
> > hw/i386/pc.c
> > hw/intc/nios2_iic.c
> > hw/mips/cps.c
> > hw/riscv/riscv_hart.c
> > hw/riscv/sifive_e.c
> > hw/riscv/sifive_u.c
> > hw/sd/milkymist-memcard.c
> > target/alpha/cpu.c
> > target/arm/cpu.c
> > target/arm/cpu64.c
> > target/cris/cpu.c
> > target/hppa/cpu.c
> > target/i386/cpu.c
> > target/lm32/cpu.c
> > target/m68k/cpu.c
> > target/microblaze/cpu.c
> > target/mips/cpu.c
> > target/moxie/cpu.c
> > target/nios2/cpu.c
> > target/openrisc/cpu.c
> > target/ppc/compat.c
> > target/ppc/translate_init.inc.c
> > target/riscv/cpu.c
> > target/sh4/cpu.c
> > target/sparc/cpu.c
> > target/tricore/cpu.c
> > target/unicore32/cpu.c
> > target/xtensa/cpu.c
> > kvm
> > target/ppc/kvm.c
> > target/s390x/cpu_models.c
> > xen
> > hw/block/dataplane/xen-block.c
> > hw/block/xen-block.c
> > hw/xen/xen-backend.c
> > hw/xen/xen-bus.c
> > hw/xen/xen-host-pci-device.c
> > hw/xen/xen_pt.c
> > hw/xen/xen_pt_config_init.c
> > Hosts
> > qga/commands-win32.c
> > util/oslib-posix.c
> > ARM Machines
> > hw/arm/allwinner-a10.c
> > hw/arm/aspeed_soc.c
> > hw/arm/bcm2835_peripherals.c
> > hw/arm/bcm2836.c
> > hw/arm/digic.c
> > hw/arm/fsl-imx25.c
> > hw/arm/fsl-imx31.c
> > hw/arm/fsl-imx6.c
> > hw/arm/integratorcp.c
> > hw/arm/msf2-soc.c
> > hw/arm/nrf51_soc.c
> > hw/arm/stm32f205_soc.c
> > hw/arm/virt.c
> > hw/arm/xlnx-versal-virt.c
> > hw/arm/xlnx-zynqmp.c
> > hw/cpu/realview_mpcore.c
> > hw/display/bcm2835_fb.c
> > hw/dma/bcm2835_dma.c
> > hw/dma/xilinx_axidma.c
> > hw/gpio/aspeed_gpio.c
> > hw/gpio/bcm2835_gpio.c
> > hw/intc/arm_gic.c
> > hw/intc/arm_gic_kvm.c
> > hw/intc/arm_gicv3.c
> > 

Re: [RFC PATCH 12/18] stubs: Update monitor stubs for qemu-storage-daemon

[Markus Armbruster]

Kevin Wolf  writes:

> Before we can add the monitor to qemu-storage-daemon, we need to add a
> few monitor stubs,

I can see just one: monitor_fdsets_cleanup().

>and we need to make sure that stubs that are actually
> implemented in the monitor core aren't linked so that we don't get
> linker errors because of duplicate symbols.

, by moving them from stubs/monitor.c to new stubs/monitor-core.c

>
> Signed-off-by: Kevin Wolf 
> ---
>  stubs/monitor-core.c | 21 +
>  stubs/monitor.c  | 15 ++-
>  stubs/Makefile.objs  |  1 +
>  3 files changed, 24 insertions(+), 13 deletions(-)
>  create mode 100644 stubs/monitor-core.c
>
> diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c
> new file mode 100644
> index 00..403c00a6d0
> --- /dev/null
> +++ b/stubs/monitor-core.c
> @@ -0,0 +1,21 @@
> +#include "qemu/osdep.h"
> +#include "monitor/monitor.h"
> +#include "qemu-common.h"
> +#include "qapi/qapi-emit-events.h"
> +
> +__thread Monitor *cur_mon;
> +
> +void monitor_init_qmp(Chardev *chr, bool pretty)
> +{
> +}
> +
> +void qapi_event_emit(QAPIEvent event, QDict *qdict)
> +{
> +}
> +
> +int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
> +{
> +abort();
> +}
> +
> +
> diff --git a/stubs/monitor.c b/stubs/monitor.c
> index c3e9a2e4dc..9403f8e72c 100644
> --- a/stubs/monitor.c
> +++ b/stubs/monitor.c
> @@ -1,14 +1,7 @@
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
> -#include "qapi/qapi-emit-events.h"
>  #include "monitor/monitor.h"
> -
> -__thread Monitor *cur_mon;
> -
> -int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
> -{
> -abort();
> -}
> +#include "../monitor/monitor-internal.h"

../ is ugly.  I don't have better ideas.

>  
>  int monitor_get_fd(Monitor *mon, const char *name, Error **errp)
>  {
> @@ -16,14 +9,10 @@ int monitor_get_fd(Monitor *mon, const char *name, Error 
> **errp)
>  return -1;
>  }
>  
> -void monitor_init_qmp(Chardev *chr, bool pretty)
> -{
> -}
> -
>  void monitor_init_hmp(Chardev *chr, bool use_readline)
>  {
>  }
>  
> -void qapi_event_emit(QAPIEvent event, QDict *qdict)
> +void monitor_fdsets_cleanup(void)
>  {
>  }
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index 77fbf72576..ad4515ac70 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -19,6 +19,7 @@ stub-obj-y += machine-init-done.o
>  stub-obj-y += migr-blocker.o
>  stub-obj-y += change-state-handler.o
>  stub-obj-y += monitor.o
> +stub-obj-y += monitor-core.o
>  stub-obj-y += notify-event.o
>  stub-obj-y += qtest.o
>  stub-obj-y += replay.o

Re: [RFC PATCH 10/18] qemu-storage-daemon: Add --chardev option

[Markus Armbruster]

Kevin Wolf  writes:

> This adds a --chardev option to the storage daemon that works the same
> as the -chardev option of the system emulator.
>
> Signed-off-by: Kevin Wolf 
> ---
>  qemu-storage-daemon.c | 19 +++
>  Makefile  |  2 +-
>  2 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/qemu-storage-daemon.c b/qemu-storage-daemon.c
> index 099388f645..46e0a6ea56 100644
> --- a/qemu-storage-daemon.c
> +++ b/qemu-storage-daemon.c
> @@ -26,6 +26,7 @@
>  
>  #include "block/block.h"
>  #include "block/nbd.h"
> +#include "chardev/char.h"
>  #include "crypto/init.h"
>  
>  #include "qapi/error.h"
> @@ -75,6 +76,9 @@ static void help(void)
>  " [,driver specific parameters...]\n"
>  " configure a block backend\n"
>  "\n"
> +"  --chardev configure a character device backend\n"
> +" (see the qemu(1) man page for possible options)\n"

If pointing to the manual page was good enough for --help, we could save
ourselves a ton of trouble :)

> +"\n"
>  "  --export [type=]nbd,device=[,name=]\n"
>  "   [,writable=on|off][,bitmap=]\n"
>  " export the specified block node over NBD\n"
> @@ -96,10 +100,13 @@ QEMU_HELP_BOTTOM "\n",
>  enum {
>  OPTION_OBJECT = 256,
>  OPTION_BLOCKDEV,
> +OPTION_CHARDEV,
>  OPTION_NBD_SERVER,
>  OPTION_EXPORT,
>  };
>  
> +extern QemuOptsList qemu_chardev_opts;
> +
>  static QemuOptsList qemu_object_opts = {
>  .name = "object",
>  .implied_opt_name = "qom-type",
> @@ -130,6 +137,7 @@ static int process_options(int argc, char *argv[], Error 
> **errp)
>  {"help", no_argument, 0, 'h'},
>  {"object", required_argument, 0, OPTION_OBJECT},
>  {"blockdev", required_argument, 0, OPTION_BLOCKDEV},
> +{"chardev", required_argument, 0, OPTION_CHARDEV},
>  {"nbd-server", required_argument, 0, OPTION_NBD_SERVER},
>  {"export", required_argument, 0, OPTION_EXPORT},
>  {"version", no_argument, 0, 'V'},
> @@ -189,6 +197,17 @@ static int process_options(int argc, char *argv[], Error 
> **errp)
>  qapi_free_BlockdevOptions(options);
>  break;
>  }
> +case OPTION_CHARDEV:
> +{
> +QemuOpts *opts = qemu_opts_parse(_chardev_opts,
> + optarg, true, _fatal);
> +if (!qemu_chr_new_from_opts(opts, NULL, _fatal)) {
> +/* No error, but NULL returned means help was printed */
> +exit(EXIT_SUCCESS);
> +}
> +qemu_opts_del(opts);
> +break;
> +}

Differs from vl.c similar to --object [PATCH 02]:

* Options are processed left to right.  Good.

* You use _chardev_opts instead of qemu_opts_find().  Good.

* You use qemu_opts_parse() instead of qemu_opts_parse_noisily().  Only
  here I can actually point to a loss of help.

  For options where the argument is essentially a tagged union, an
  option argument "help" usually lists the tags, an argument "T,help"
  shows help on the parameters that go with tag T.

  --chardev is such an option.  Consider:

$ qemu-system-x86_64 --chardev help
Available chardev backend types: 
  ringbuf
[...]
  tty
$ qemu-storage-daemon --chardev help
Available chardev backend types: 
  pty
[...]
  tty

  Works the same, only the available backend types differ.  Intentional?

  Next:

$ qemu-system-x86_64 --chardev tty,help
chardev options:
  append=
  backend=
  chardev=
  cols=
[...]
  width=
[Exit 1 ]

  The second help isn't very helpful, and the exit code is wrong.  Not
  this patch's problem.

$ qemu-storage-daemon --chardev tty,help
qemu-storage-daemon: Invalid parameter 'help'
[Exit 1 ]

  This patch's problem :)

Also like --object, --chardev is paired with a QMP command, namely
chardev-add, and the two differ for historical reasons.  If we want to
give the storage daemon a QAPIfied command line from the start, we'll
have to decide how to address this issue.


>  case OPTION_NBD_SERVER:
>  {
>  Visitor *v;
> diff --git a/Makefile b/Makefile
> index b913d4d736..0e3e98582d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -561,7 +561,7 @@ qemu-img.o: qemu-img-cmds.h
>  qemu-img$(EXESUF): qemu-img.o $(authz-obj-y) $(block-obj-y) $(crypto-obj-y) 
> $(io-obj-y) $(qom-obj-y) $(COMMON_LDADDS)
>  qemu-nbd$(EXESUF): qemu-nbd.o $(authz-obj-y) $(block-obj-y) $(crypto-obj-y) 
> $(io-obj-y) $(qom-obj-y) $(COMMON_LDADDS)
>  qemu-io$(EXESUF): qemu-io.o $(authz-obj-y) $(block-obj-y) $(crypto-obj-y) 
> $(io-obj-y) $(qom-obj-y) $(COMMON_LDADDS)
> -qemu-storage-daemon$(EXESUF): qemu-storage-daemon.o $(authz-obj-y) 
> $(block-obj-y) $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) 
> $(storage-daemon-obj-y) $(COMMON_LDADDS)
> 

Re: [PATCH v2 3/3] trace: Forbid dynamic field width in event format

[Eric Blake]

On 11/8/19 8:40 AM, Philippe Mathieu-Daudé wrote:

Since not all trace backends support dynamic field width in
format (dtrace via stap does not), forbid them.

Add a check to refuse field width in new formats:




+++ b/scripts/tracetool/__init__.py
@@ -206,6 +206,7 @@ class Event(object):
"\s*"
"(?:(?:(?P\".+),)?\s*(?P\".+))?"
"\s*")
+_DFWRE = re.compile(".*(%0?\*).*")


The use of leading and trailing .* is pointless if the RE itself is used 
unanchored and where you only care if the () matched.


This doesn't catch all valid uses of * in a printf format.  Better might 
be something like:


_DFWRE = re.compile("%[\d\.\- +#']*\*")

which matches only if there is a %, any number of characters that can 
form a printf flag, as well as a numeric field width but dynamic precision.




+if Event._DFWRE.match(fmt):
+raise ValueError("Event format must not contain field width '%*'")
  
  if len(fmt_trans) > 0:

  fmt = [fmt_trans, fmt]



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

Re: [PATCH v2 2/3] hw/mips/gt64xxx: Remove dynamic field width from trace events

[Eric Blake]

On 11/8/19 8:40 AM, Philippe Mathieu-Daudé wrote:

Since not all trace backends support dynamic field width in
format (dtrace via stap does not), replace by a static field
width instead.

Reported-by: Eric Blake 
Buglink: https://bugs.launchpad.net/qemu/+bug/1844817
Signed-off-by: Philippe Mathieu-Daudé 
---
v2: Do not update qemu_log_mask()
---
  hw/mips/gt64xxx_pci.c | 16 
  hw/mips/trace-events  |  4 ++--
  2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
index 5cab9c1ee1..6743e7c929 100644
--- a/hw/mips/gt64xxx_pci.c
+++ b/hw/mips/gt64xxx_pci.c
@@ -642,19 +642,19 @@ static void gt64120_writel(void *opaque, hwaddr addr,
  /* not really implemented */
  s->regs[saddr] = ~(~(s->regs[saddr]) | ~(val & 0xfffe));
  s->regs[saddr] |= !!(s->regs[saddr] & 0xfffe);
-trace_gt64120_write("INTRCAUSE", size << 1, val);
+trace_gt64120_write("INTRCAUSE", size << 3, val);


Again, this isn't mentioned in the commit message.  Why are you changing 
parameter values?




+++ b/hw/mips/trace-events
@@ -1,4 +1,4 @@
  # gt64xxx.c
-gt64120_read(const char *regname, int width, uint64_t value) "gt64120 read %s 
value:0x%0*" PRIx64
-gt64120_write(const char *regname, int width, uint64_t value) "gt64120 write %s 
value:0x%0*" PRIx64
+gt64120_read(const char *regname, int width, uint64_t value) "gt64120 read %s 
width:%d value:0x%08" PRIx64
+gt64120_write(const char *regname, int width, uint64_t value) "gt64120 write %s 
width:%d value:0x%08" PRIx64


Huh, we were really broken - the old code (if passed to printf) would 
try to parse 4 parameters, even though it was only passed 3.  But it 
looks like you still need a v3.


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

Re: [RFC PATCH 09/18] qemu-storage-daemon: Add main loop

[Markus Armbruster]

Kevin Wolf  writes:

> Instead of exiting after processing all command line options, start a
> main loop and keep processing events until exit is requested with a
> signal (e.g. SIGINT).
>
> Now qemu-storage-daemon can be used as an alternative for qemu-nbd that
> provides a few features that were previously only available from QMP,
> such as access to options only available with -blockdev and the socket
> types 'vsock' and 'fd'.
>
> Signed-off-by: Kevin Wolf 
> ---
>  qemu-storage-daemon.c | 13 +
>  Makefile.objs |  2 ++
>  2 files changed, 15 insertions(+)
>
> diff --git a/qemu-storage-daemon.c b/qemu-storage-daemon.c
> index 9e5f474fd0..099388f645 100644
> --- a/qemu-storage-daemon.c
> +++ b/qemu-storage-daemon.c
> @@ -45,10 +45,18 @@
>  #include "qemu/option.h"
>  #include "qom/object_interfaces.h"
>  
> +#include "sysemu/runstate.h"
>  #include "trace/control.h"
>  
>  #include 
>  
> +static bool exit_requested = false;
> +
> +void qemu_system_killed(int signal, pid_t pid)
> +{
> +exit_requested = true;
> +}
> +

This runs within a signal handler, so better make @exit_requested
volatile.  Hmm, vl.c gets it wrong, too.

>  static void help(void)
>  {
>  printf(
> @@ -238,6 +246,7 @@ int main(int argc, char *argv[])
>  
>  error_init(argv[0]);
>  qemu_init_exec_dir(argv[0]);
> +os_setup_signal_handling();
>  
>  module_call_init(MODULE_INIT_QOM);
>  module_call_init(MODULE_INIT_TRACE);
> @@ -256,5 +265,9 @@ int main(int argc, char *argv[])
>  return EXIT_FAILURE;
>  }
>  
> +while (!exit_requested) {
> +main_loop_wait(false);
> +}
> +
>  return EXIT_SUCCESS;
>  }
> diff --git a/Makefile.objs b/Makefile.objs
> index cc262e445f..b667d3f07b 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -43,6 +43,8 @@ io-obj-y = io/
>  
>  storage-daemon-obj-y = block/
>  storage-daemon-obj-y += blockdev.o blockdev-nbd.o iothread.o
> +storage-daemon-obj-$(CONFIG_WIN32) += os-win32.o
> +storage-daemon-obj-$(CONFIG_POSIX) += os-posix.o
>  
>  ##
>  # Target independent part of system emulation. The long term path is to

Re: [RFC PATCH 08/18] qemu-storage-daemon: Add --export option

[Markus Armbruster]

Kevin Wolf  writes:

> Am 06.11.2019 um 14:11 hat Max Reitz geschrieben:
>> On 17.10.19 15:01, Kevin Wolf wrote:
>> > Add a --export option to qemu-storage-daemon to export a block node. For
>> > now, only NBD exports are implemented. Apart from the 'type' option
>> > (which is the implied key), it maps the arguments for nbd-server-add to
>> > the command line. Example:
>> > 
>> > --export nbd,device=disk,name=test-export,writable=on
>> > 
>> > Signed-off-by: Kevin Wolf 
>> > ---
>> >  qapi/block.json   | 27 +++
>> >  qemu-storage-daemon.c | 31 +++
>> >  2 files changed, 58 insertions(+)
>> 
>> Would it be better to collect the BlockExports in a list and work on it
>> after all arguments have been parsed?  As it is, it’s important that
>> users define block devices and things like NBD servers before --export.
>>  Yes, I know, that’s exactly how it works with qemu, but is that really
>> the best way?
>
> It's actually not how QEMU works generally. QEMU collects things in
> QemuOptsLists and then tries to interpret them in the right order. Of
> course, we never get the order actually right, which results in constant
> reshuffling of the order of initialisations in vl.c.
>
> It also means that vl.c (!) has a list of -object types that need to be
> created early so that other backends can make use of them, and of those
> types that actually depend on a backend already being present (see
> object_create_initial() for details).
>
> I think it's much cleaner to simply use the order in the command line
> instead of adding magic that tries to resolve (and fails at actually
> resolving) all the dependencies.

Seconded.

The "process arguments strictly left to right" strategy is also visible
in PATCH 02 and 05.

>  I seem to remember that this was in
> fact one of the things Markus keeps mentioning he would change if he
> were to rewrite the QEMU command line parser from scratch without
> compatibility requirements.

E.g.
Message-ID: <87poomg77m@dusky.pond.sub.org>
https://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg00163.html

Re: [PATCH v2 1/3] hw/block/pflash: Remove dynamic field width from trace events

[Eric Blake]

On 11/8/19 8:40 AM, Philippe Mathieu-Daudé wrote:

Since not all trace backends support dynamic field width in
format (dtrace via stap does not), replace by a static field
width instead.

Reported-by: Eric Blake 
Buglink: https://bugs.launchpad.net/qemu/+bug/1844817
Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/block/pflash_cfi01.c | 8 
  hw/block/pflash_cfi02.c | 8 
  hw/block/trace-events   | 8 
  3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 566c0acb77..787d1196f2 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -276,7 +276,7 @@ static uint32_t pflash_data_read(PFlashCFI01 *pfl, hwaddr 
offset,
  DPRINTF("BUG in %s\n", __func__);
  abort();
  }
-trace_pflash_data_read(offset, width << 1, ret);
+trace_pflash_data_read(offset, width << 3, ret);


Umm, why is width changing?  That's not mentioned in the commit message.


@@ -389,7 +389,7 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset,
  
  break;

  }
-trace_pflash_io_read(offset, width, width << 1, ret, pfl->cmd, 
pfl->wcycle);
+trace_pflash_io_read(offset, width << 3, ret, pfl->cmd, pfl->wcycle);


And even this one is odd.  Matching up to the trace messages:



-pflash_io_read(uint64_t offset, int width, int fmt_width, uint32_t value, uint8_t cmd, uint8_t 
wcycle) "offset:0x%04"PRIx64" width:%d value:0x%0*x cmd:0x%02x wcycle:%u"



+pflash_io_read(uint64_t offset, int width, uint32_t value, uint8_t cmd, uint8_t wcycle) 
"offset:0x%04"PRIx64" width:%d value:0x%04x cmd:0x%02x wcycle:%u"


you are changing from:

"%04"PRIx64" %d %0*x...", offset, width, width << 1, ret,...

(where width<<1, ret matches *x)

into

"%04"PRIx64" %d %04x...", offset, width << 3, ret,...

where you are now printing a different value for width.

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

Re: [RFC PATCH v2 14/26] qcow2: Add subcluster support to qcow2_get_cluster_offset()

[Alberto Garcia]

On Mon 04 Nov 2019 03:58:57 PM CET, Max Reitz wrote:
> OTOH, what I don’t like so far about this series is that the “cluster
> logic” is still everywhere when I think it should just be about
> subclusters now.  (Except in few places where it must be about
> clusters as in something that can have a distinct host offset and/or
> has an own L2 entry.)  So maybe the parameter should really be
> @nb_subclusters.

> But I’m not sure.  For how this function is written right now, it
> makes sense for it to be @nb_clusters, but I think it could be changed
> so it would work with @nb_subclusters, too.

I'm still reviewing your (much appreciated) feedback, but one thing I
can tell you is that my initial versions were doing everything with
subclusters because of the reasons you mention (i.e. there was
@nb_subclusters and all that).

Later when I started to tidy things up I realized that most of those
places only needed the number of clusters after all, and in some cases
the necessary changes were really minimal (like in handle_copied()).

>> +static int count_contiguous_subclusters(BlockDriverState *bs, int 
>> nb_clusters,
>> +unsigned sc_index, uint64_t 
>> *l2_slice,
>> +int l2_index)
>>  {
   /* ... */
>> +if (type == QCOW2_CLUSTER_COMPRESSED) {
>> +return 1; /* Compressed clusters are always counted one by one */
>
> Hm, yes, but cluster by cluster, not subcluster by subcluster, so this
> should be s->subclusters_per_cluster, and perhaps sc_index should be
> asserted to be 0.  (Or it should be s->subclusters_per_cluster -
> sc_index.)

Right, that's a bug, it forces the caller to decompress the cluster 32
times in order to read it completely! Thanks!

(in reality this is not used because this function doesn't get called
for compressed clusters but the same problem happens in the calling
function, as you correctly point out. Maybe I should assert here
instead)

>> @@ -514,8 +499,8 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, 
>> uint64_t offset,
>
> I suppose this is get_subcluster_offset now.

Hmmm no, this returns the actual host cluster offset, then the caller
uses offset_into_cluster() to get the final value (which doesn't need to
be subcluster-aligned anyway).

Berto

Re: [RFC PATCH 06/18] qemu-storage-daemon: Add --nbd-server option

[Markus Armbruster]

Max Reitz  writes:

> On 17.10.19 15:01, Kevin Wolf wrote:
>> Add a --nbd-server option to qemu-storage-daemon to start the built-in
>> NBD server right away. It maps the arguments for nbd-server-start to the
>> command line.
>
> Well, it doesn’t quite, because nbd-server-start takes a
> SocketAddressLegacy, and this takes a SocketAddress.
>
> On one hand I can understand why you would do it differently (especially
> for command-line options), but on the other I find it a bit problematic
> to have --nbd-server be slightly different from nbd-server-start when
> both are intended to be the same.
>
> My biggest problem though lies in the duplication in the QAPI schema.
> If NbdServerOptions.addr were a SocketAddressLegacy, we could let
> nbd-server-start’s options just be of type NbdServerOptions and thus get
> rid of the duplication.
>
> I suspect in practice it’s all not that big of a problem.  I can’t call
> it bad if --nbd-server is just nicer to use.  And the biggest problem
> with duplication in the QAPI schema is that nbd-server-start and
> --nbd-server might get out of sync.  But realistically, I don’t see that
> happen, because if nbd-server-start changes, nbd_server_start() will
> change, too, so we’ll get compile errors in nbd_server_start_options().
>
> *shrug*

Two good reasons for making new --nbd-server differ from existing
nbd-server-start:

1. The nesting sucks.  The CLI's dotted key syntax makes it suck harder.

2. New interfaces should not use SocketAddressLegacy (or any other
   "simple" union) if we can help it.

The duplication is the price we pay for getting it right om the second
try.

> But I do think the commit message should explain why we can’t just use
> NbdServerOptions for nbd-server-start.

Yes, and throw in a comment.

Re: [Qemu-devel] [PATCH v2 08/11] block/crypto: implement blockdev-amend

[Maxim Levitsky]

On Mon, 2019-10-07 at 09:58 +0200, Markus Armbruster wrote:
> Maxim Levitsky  writes:
> 
> > Signed-off-by: Maxim Levitsky 
> > Reviewed-by: Daniel P. Berrangé 
> > ---
> 
> [...]
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 7900914506..4a6db98938 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -4211,8 +4211,11 @@
> >  # Driver specific image creation options for LUKS.
> >  #
> >  # @file Node to create the image format on
> > +#   Mandatory for create
> >  # @size Size of the virtual disk in bytes
> > +#   Mandatory for create
> >  # @preallocationPreallocation mode for the new image
> > +#   Only for create
> >  #   (since: 4.2)
> >  #   (default: off; allowed values: off, metadata, falloc, 
> > full)
> >  #
> > @@ -4220,8 +4223,8 @@
> >  ##
> >  { 'struct': 'BlockdevCreateOptionsLUKS',
> >'base': 'QCryptoBlockCreateOptionsLUKS',
> > -  'data': { 'file': 'BlockdevRef',
> > -'size': 'size',
> > +  'data': { '*file': 'BlockdevRef',
> > +'*size': 'size',
> >  '*preallocation':   'PreallocMode' } }
> >  
> >  ##
> 
> Why is this change needed?
> 
> When the commit message says "implement FOO" and nothing else, then I
> don't expect QAPI schema changes.  Working the answer to my question
> into the commit message might avoid the surprise.

It is because for the amend interface, it doesn't make much sense to pass the 
size,
and the underlying block device.

Thats why I made these optional but I check that these parameters are present 
on creation
and not present on amend.

Still I am more and more inclined to think that I should not use the create 
options for amend,
but rather split into a new structure. This is just not worth it IMHO.


Best regards,
Maxim Levitsky

Re: [PATCH v3 01/22] iotests: s/qocw2/qcow2/

[Maxim Levitsky]

On Thu, 2019-11-07 at 17:36 +0100, Max Reitz wrote:
> Probably due to blind copy-pasting, we have several instances of "qocw2"
> in our iotests.  Fix them.
> 
> Reported-by: Maxim Levitsky 
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/060 | 2 +-
>  tests/qemu-iotests/061 | 2 +-
>  tests/qemu-iotests/062 | 2 +-
>  tests/qemu-iotests/066 | 2 +-
>  tests/qemu-iotests/068 | 2 +-
>  tests/qemu-iotests/108 | 2 +-
>  tests/qemu-iotests/138 | 2 +-
>  tests/qemu-iotests/261 | 2 +-
>  8 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
> index b91d8321bb..725e58a5a5 100755
> --- a/tests/qemu-iotests/060
> +++ b/tests/qemu-iotests/060
> @@ -44,7 +44,7 @@ _filter_io_error()
>  . ./common.rc
>  . ./common.filter
>  
> -# This tests qocw2-specific low-level functionality
> +# This tests qcow2-specific low-level functionality
>  _supported_fmt qcow2
>  _supported_proto file
>  _supported_os Linux
> diff --git a/tests/qemu-iotests/061 b/tests/qemu-iotests/061
> index 4eac5b83bd..e1b8044630 100755
> --- a/tests/qemu-iotests/061
> +++ b/tests/qemu-iotests/061
> @@ -37,7 +37,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  . ./common.rc
>  . ./common.filter
>  
> -# This tests qocw2-specific low-level functionality
> +# This tests qcow2-specific low-level functionality
>  _supported_fmt qcow2
>  _supported_proto file
>  _supported_os Linux
> diff --git a/tests/qemu-iotests/062 b/tests/qemu-iotests/062
> index d5f818fcce..79738b1c26 100755
> --- a/tests/qemu-iotests/062
> +++ b/tests/qemu-iotests/062
> @@ -37,7 +37,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  . ./common.rc
>  . ./common.filter
>  
> -# This tests qocw2-specific low-level functionality
> +# This tests qcow2-specific low-level functionality
>  _supported_fmt qcow2
>  _supported_proto generic
>  
> diff --git a/tests/qemu-iotests/066 b/tests/qemu-iotests/066
> index 28f8c98412..cacbdb6ae0 100755
> --- a/tests/qemu-iotests/066
> +++ b/tests/qemu-iotests/066
> @@ -36,7 +36,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  . ./common.rc
>  . ./common.filter
>  
> -# This tests qocw2-specific low-level functionality
> +# This tests qcow2-specific low-level functionality
>  _supported_fmt qcow2
>  _supported_proto generic
>  
> diff --git a/tests/qemu-iotests/068 b/tests/qemu-iotests/068
> index 22f5ca3ba6..c164ccc64a 100755
> --- a/tests/qemu-iotests/068
> +++ b/tests/qemu-iotests/068
> @@ -36,7 +36,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  . ./common.rc
>  . ./common.filter
>  
> -# This tests qocw2-specific low-level functionality
> +# This tests qcow2-specific low-level functionality
>  _supported_fmt qcow2
>  _supported_proto generic
>  
> diff --git a/tests/qemu-iotests/108 b/tests/qemu-iotests/108
> index 9c08172237..872a9afec9 100755
> --- a/tests/qemu-iotests/108
> +++ b/tests/qemu-iotests/108
> @@ -37,7 +37,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  . ./common.rc
>  . ./common.filter
>  
> -# This tests qocw2-specific low-level functionality
> +# This tests qcow2-specific low-level functionality
>  _supported_fmt qcow2
>  _supported_proto file
>  _supported_os Linux
> diff --git a/tests/qemu-iotests/138 b/tests/qemu-iotests/138
> index 6a731370db..8b2f587af0 100755
> --- a/tests/qemu-iotests/138
> +++ b/tests/qemu-iotests/138
> @@ -36,7 +36,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  . ./common.rc
>  . ./common.filter
>  
> -# This tests qocw2-specific low-level functionality
> +# This tests qcow2-specific low-level functionality
>  _supported_fmt qcow2
>  _supported_proto file
>  _supported_os Linux
> diff --git a/tests/qemu-iotests/261 b/tests/qemu-iotests/261
> index fb96bcfbe2..9f2817251f 100755
> --- a/tests/qemu-iotests/261
> +++ b/tests/qemu-iotests/261
> @@ -40,7 +40,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  . ./common.rc
>  . ./common.filter
>  
> -# This tests qocw2-specific low-level functionality
> +# This tests qcow2-specific low-level functionality
>  _supported_fmt qcow2
>  _supported_proto file
>  _supported_os Linux


Thank you!
Reviewed-by: Maxim Levitsky 

Best regards,
Maxim Levitsky

Re: [Qemu-devel] [PATCH v2 07/11] block: add x-blockdev-amend qmp command

[Maxim Levitsky]

On Mon, 2019-10-07 at 09:53 +0200, Markus Armbruster wrote:
> Maxim Levitsky  writes:
> 
> > Signed-off-by: Maxim Levitsky 
> > ---
> 
> [...]
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index e6edd641f1..7900914506 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -4650,6 +4650,32 @@
> >'data': { 'job-id': 'str',
> >  'options': 'BlockdevCreateOptions' } }
> >  
> > +##
> > +# @x-blockdev-amend:
> > +#
> > +# Starts a job to amend format specific options of an existing open block 
> > device.
> > +# The job is automatically finalized, but a manual job-dismiss is required.
> > +#
> > +# @job-id:  Identifier for the newly created job.
> > +#
> > +# @node-name:   Name of the block node to work on
> > +#
> > +# @options: Options (same as for image creation)
> > +#
> > +# @force:   Allow unsafe operations, format specific
> > +#   For luks that allows erase of the last active keyslot
> > +#   (permanent loss of data),
> > +#   and replacement of an active keyslot
> > +#   (possible loss of data if IO error happens)
> > +#
> > +# Since: 4.2
> > +##
> > +{ 'command': 'x-blockdev-amend',
> > +  'data': { 'job-id': 'str',
> > +'node-name': 'str',
> > +'options': 'BlockdevCreateOptions',
> > +'*force': 'bool' } }
> > +
> >  ##
> >  # @blockdev-open-tray:
> >  #
> > diff --git a/qapi/job.json b/qapi/job.json
> > index a121b615fb..342d29a7aa 100644
> > --- a/qapi/job.json
> > +++ b/qapi/job.json
> > @@ -19,10 +19,12 @@
> >  #
> >  # @create: image creation job type, see "blockdev-create" (since 3.0)
> >  #
> > +# @amend: image options amend job type, see "x-blockdev-amend" (since 4.2)
> > +#
> >  # Since: 1.7
> >  ##
> >  { 'enum': 'JobType',
> > -  'data': ['commit', 'stream', 'mirror', 'backup', 'create'] }
> > +  'data': ['commit', 'stream', 'mirror', 'backup', 'create', 'amend'] }
> >  
> >  ##
> >  # @JobStatus:
> 
> Aha, you introduce "amend" after using the concept in PATCH 02.
> Rather confusing, I'm afraid.
> 
> I guess secret deletion makes sense with amend somehow, and only with
> amend.  If yes, the QAPI documentaion should spell that out, and the
> code should diagnose invalid use (maybe it does already; I haven't
> looked at it).

I think I did mentioned that in QAPI,
and I do check for optional arguments in the code.

I will probably stop using the blockdev-create options for amend after all.

Best regards,
Maxim Levitsky

Re: [RFC PATCH v2 13/26] qcow2: Add subcluster support to calculate_l2_meta()

[Alberto Garcia]

On Mon 04 Nov 2019 03:21:41 PM CET, Max Reitz wrote:
>> If an image has subclusters then there are more copy-on-write
>> scenarios that we need to consider. Let's say we have a write request
>> from the middle of subcluster #3 until the end of the cluster:
>> 
>>- If the cluster is new, then subclusters #0 to #3 from the old
>>  cluster must be copied into the new one.
>
> You mean for snapshots?
>
> (That isn’t quite clear, and I only guess this based on the next
> bullet point which differentiates based on “the old cluster was
> unallocated”.  That’s weird, too, because what does that mean, old
> cluster and new cluster?

Yes, perhaps the terminology is a bit unclear.

When I say "new cluster" is this context I mean that a write request
requires that a new cluster is allocated in the qcow2 file.

Then the "old cluster" would be what was there before the write (i.e. a
cluster with refcount > 1 or an unallocated cluster). Where we are doing
the copy-on-write from.

>>   * If @keep_old is true it means that the clusters were already
>>   * allocated and will be overwritten. If false then the clusters are
>>   * new and we have to decrease the reference count of the old ones.
>> + *
>> + * Returns 1 on success, -errno on failure.
>
> I think there should be a note here on why this doesn’t follow the
> general 0/-errno schema (i.e., “, because that is what callers generally
> expect”).

Good idea.

>> +if (!keep_old) {
>> +switch (type) {
>> +case QCOW2_CLUSTER_NORMAL:
>> +case QCOW2_CLUSTER_COMPRESSED:
>> +case QCOW2_CLUSTER_ZERO_ALLOC:
>> +case QCOW2_CLUSTER_UNALLOCATED_SUBCLUSTER:
>> +cow_start_from = 0;
>
> Somehow (I don’t know why) I find this a bit tough to understand.
>
> Wouldn’t it work to let cow_start start from the first subcluster for
> ZERO_ALLOC and UNALLOCATED_SUBCLUSTER?  We don’t need to COW those, it
> should be sufficient to just make the subclusters before that zero or
> unallocated, respectively.

Here's one good example why I should probably add a QCow2SubclusterType
different from the existing QCow2ClusterType.

In this context, 'type' is the type of the subcluster, and because of
that _ZERO_ALLOC means that the subcluster reads as zeros but the
cluster itself is allocated. Other subcluster may contain data and
that's why we have to copy all of them.

Berto

Re: [PATCH v2 09/11] block/qcow2: implement blockdev-amend

[Maxim Levitsky]

On Fri, 2019-10-04 at 21:03 +0200, Max Reitz wrote:
> On 13.09.19 00:30, Maxim Levitsky wrote:
> > Currently only for changing crypto parameters
> 
> Yep, that elegantly avoids most of the problems we’d have otherwise. :-)
> 
> > Signed-off-by: Maxim Levitsky 
> > ---
> >  block/qcow2.c| 71 
> >  qapi/block-core.json |  6 ++--
> >  2 files changed, 75 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 26f83aeb44..c8847ec6e2 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -3079,6 +3079,18 @@ qcow2_co_create(BlockdevCreateOptions 
> > *create_options, Error **errp)
> >  assert(create_options->driver == BLOCKDEV_DRIVER_QCOW2);
> >  qcow2_opts = _options->u.qcow2;
> >  
> > +if (!qcow2_opts->has_size) {
> > +error_setg(errp, "Size is manadatory for image creation");
> > +return -EINVAL;
> > +
> > +}
> > +
> > +if (!qcow2_opts->has_file) {
> > +error_setg(errp, "'file' is manadatory for image creation");
> > +return -EINVAL;
> > +
> > +}
> > +
> >  bs = bdrv_open_blockdev_ref(qcow2_opts->file, errp);
> >  if (bs == NULL) {
> >  return -EIO;
> > @@ -5111,6 +5123,64 @@ static int qcow2_amend_options(BlockDriverState *bs, 
> > QemuOpts *opts,
> >  return 0;
> >  }
> >  
> > +
> > +static int coroutine_fn qcow2_co_amend(BlockDriverState *bs,
> > +   BlockdevCreateOptions *opts,
> > +   bool force,
> > +   Error **errp)
> > +{
> > +BlockdevCreateOptionsQcow2 *qopts = >u.qcow2;
> > +BDRVQcow2State *s = bs->opaque;
> > +int ret;
> > +
> > +/*
> > + * This is ugly as hell, in later versions of this patch
> > + * something has to be done about this
> 
> Well, at least the language of the comment needs adjustment. :-)
Thats for sure :-)

BTW, if I opt for having a separate amend parameter struct,
this will fix this problem as well, as all unsupported
fields will just not be there.

[..]

Best regards,
Maxim Levitsky

Re: [PATCH for-5.0 v4 3/5] blkdebug: Allow taking/unsharing permissions

[Vladimir Sementsov-Ogievskiy]

08.11.2019 15:34, Max Reitz wrote:
> Sometimes it is useful to be able to add a node to the block graph that
> takes or unshare a certain set of permissions for debugging purposes.
> This patch adds this capability to blkdebug.
> 
> (Note that you cannot make blkdebug release or share permissions that it
> needs to take or cannot share, because this might result in assertion
> failures in the block layer.  But if the blkdebug node has no parents,
> it will not take any permissions and share everything by default, so you
> can then freely choose what permissions to take and share.)
> 
> Signed-off-by: Max Reitz


Reviewed-by: Vladimir Sementsov-Ogievskiy 

-- 
Best regards,
Vladimir

Re: [PATCH v2 09/11] block/qcow2: implement blockdev-amend

[Maxim Levitsky]

On Mon, 2019-10-07 at 10:04 +0200, Markus Armbruster wrote:
> Max Reitz  writes:
> 
> > On 13.09.19 00:30, Maxim Levitsky wrote:
> > > Currently only for changing crypto parameters
> > 
> > Yep, that elegantly avoids most of the problems we’d have otherwise. :-)
> > 
> > > Signed-off-by: Maxim Levitsky 
> 
> [...]
> > > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > > index 4a6db98938..0eb4e45168 100644
> > > --- a/qapi/block-core.json
> > > +++ b/qapi/block-core.json
> > > @@ -4294,6 +4294,7 @@
> > >  # Driver specific image creation options for qcow2.
> > >  #
> > >  # @file Node to create the image format on
> > > +#   Mandatory for create
> > >  # @data-fileNode to use as an external data file in which all 
> > > guest
> > >  #   data is stored so that only metadata remains in the 
> > > qcow2
> > >  #   file (since: 4.0)
> > > @@ -4301,6 +4302,7 @@
> > >  #   standalone (read-only) raw image without looking at 
> > > qcow2
> > >  #   metadata (default: false; since: 4.0)
> > >  # @size Size of the virtual disk in bytes
> > > +#   Mandatory for create
> > >  # @version  Compatibility level (default: v3)
> > >  # @backing-file File name of the backing file if a backing file
> > >  #   should be used
> > > @@ -4315,10 +4317,10 @@
> > >  # Since: 2.12
> > >  ##
> > >  { 'struct': 'BlockdevCreateOptionsQcow2',
> > > -  'data': { 'file': 'BlockdevRef',
> > > +  'data': { '*file':'BlockdevRef',
> > >  '*data-file':   'BlockdevRef',
> > >  '*data-file-raw':   'bool',
> > > -'size': 'size',
> > > +'*size':'size',
> > >  '*version': 'BlockdevQcow2Version',
> > >  '*backing-file':'str',
> > >  '*backing-fmt': 'BlockdevDriver',
> > > 
> 
> My comments to the previous patch apply.
> 
> > Making these fields optional makes me wonder whether it actually make
> > sense to have both create and amend share a single QAPI structure.
> > Wouldn’t it better to have a separate amend structure that then actually
> > reflects what we support?  (This would also solve the problem of how to
> > express in the code what is and what isn’t supported through
> > blockdev-amend.)
> 
> Good point.  As is, the schema is rather confusing, at least to me.  We
> reduce or avoid the confusion in documentation or in code.  I'd prefer
> code unless it leads to too much duplication.  "Too much" is of course
> subjective.  Maxim, would you mind exploring it for us?

Nothing against having a separate amend structure, I actually prefer this,
and I don't think it will complicate the code. 


Best regards,
Maxim Levitsky

Re: [RFC v5 000/126] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

Finally, what is the plan?

Markus what do you think?

Now a lot of patches are reviewed, but a lot of are not.

Is there any hope that all patches will be reviewed? Should I resend the
whole series, or may be reduce it to reviewed subsystems only?

11.10.2019 19:03, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> At the request of Markus: full version of errp propagation. Let's look
> at it. Cover as much as possible, except inserting macro invocation
> where it's not necessary.
> 
> It's huge, and so it's an RFC.
> 
> In v5 I've added a lot more preparation cleanups:
> 01-23 are preparation cleanups
>01: not changed, keep Eric's r-b
>02: improve commit msg [Markus], keep Eric's r-b
>03: changed, only error API here, drop r-b
> 24 is core macro
>- improve cover letter, wording and macro code style
>- keep Eric's r-b
> 25-26: automation scripts
> - commit-per-subsystem changed a lot. it's a draft, don't bother too
>   much with it
> - coccinelle: add support of error_propagate_prepend
> 
> 27-126: generated patches
> 
> 
> 
> Here is a proposal of auto propagation for local_err, to not call
> error_propagate on every exit point, when we deal with local_err.
> 
> There are also two issues with errp:
> 
> 1. error_fatal & error_append_hint/error_prepend: user can't see this
> additional info, because exit() happens in error_setg earlier than info
> is added. [Reported by Greg Kurz]
> 
> 2. error_abort & error_propagate: when we wrap
> error_abort by local_err+error_propagate, resulting coredump will
> refer to error_propagate and not to the place where error happened.
> (the macro itself don't fix the issue, but it allows to [3.] drop all
> local_err+error_propagate pattern, which will definitely fix the issue)
> [Reported by Kevin Wolf]
> 
> 
> 
> Generated patches split:
> 
> misc
> hw/misc/ivshmem.c
> hw/misc/tmp105.c
> hw/misc/tmp421.c
> s390x
> hw/intc/s390_flic_kvm.c
> hw/s390x/3270-ccw.c
> hw/s390x/css-bridge.c
> hw/s390x/css.c
> hw/s390x/s390-skeys.c
> hw/s390x/s390-virtio-ccw.c
> hw/s390x/sclp.c
> hw/s390x/tod-kvm.c
> hw/vfio/ccw.c
> target/s390x/cpu.c
> tcg
> exec.c
> hw/arm/armv7m.c
> hw/arm/smmu-common.c
> hw/arm/smmuv3.c
> hw/cpu/a15mpcore.c
> hw/cpu/a9mpcore.c
> hw/cpu/arm11mpcore.c
> hw/i386/pc.c
> hw/intc/nios2_iic.c
> hw/mips/cps.c
> hw/riscv/riscv_hart.c
> hw/riscv/sifive_e.c
> hw/riscv/sifive_u.c
> hw/sd/milkymist-memcard.c
> target/alpha/cpu.c
> target/arm/cpu.c
> target/arm/cpu64.c
> target/cris/cpu.c
> target/hppa/cpu.c
> target/i386/cpu.c
> target/lm32/cpu.c
> target/m68k/cpu.c
> target/microblaze/cpu.c
> target/mips/cpu.c
> target/moxie/cpu.c
> target/nios2/cpu.c
> target/openrisc/cpu.c
> target/ppc/compat.c
> target/ppc/translate_init.inc.c
> target/riscv/cpu.c
> target/sh4/cpu.c
> target/sparc/cpu.c
> target/tricore/cpu.c
> target/unicore32/cpu.c
> target/xtensa/cpu.c
> kvm
> target/ppc/kvm.c
> target/s390x/cpu_models.c
> xen
> hw/block/dataplane/xen-block.c
> hw/block/xen-block.c
> hw/xen/xen-backend.c
> hw/xen/xen-bus.c
> hw/xen/xen-host-pci-device.c
> hw/xen/xen_pt.c
> hw/xen/xen_pt_config_init.c
> Hosts
> qga/commands-win32.c
> util/oslib-posix.c
> ARM Machines
> hw/arm/allwinner-a10.c
> hw/arm/aspeed_soc.c
> hw/arm/bcm2835_peripherals.c
> hw/arm/bcm2836.c
> hw/arm/digic.c
> hw/arm/fsl-imx25.c
> hw/arm/fsl-imx31.c
> hw/arm/fsl-imx6.c
> hw/arm/integratorcp.c
> hw/arm/msf2-soc.c
> hw/arm/nrf51_soc.c
> hw/arm/stm32f205_soc.c
> hw/arm/virt.c
> hw/arm/xlnx-versal-virt.c
> hw/arm/xlnx-zynqmp.c
> hw/cpu/realview_mpcore.c
> hw/display/bcm2835_fb.c
> hw/dma/bcm2835_dma.c
> hw/dma/xilinx_axidma.c
> hw/gpio/aspeed_gpio.c
> hw/gpio/bcm2835_gpio.c
> hw/intc/arm_gic.c
> hw/intc/arm_gic_kvm.c
> hw/intc/arm_gicv3.c
> hw/intc/arm_gicv3_its_kvm.c
> hw/intc/arm_gicv3_kvm.c
> hw/intc/armv7m_nvic.c
> hw/intc/realview_gic.c
> hw/microblaze/xlnx-zynqmp-pmu.c
> hw/misc/bcm2835_mbox.c
> hw/misc/bcm2835_property.c
> hw/misc/msf2-sysreg.c
> hw/net/xilinx_axienet.c
> hw/nvram/nrf51_nvm.c
> hw/timer/aspeed_timer.c
> hw/watchdog/wdt_aspeed.c
> MIPS Machines
> hw/core/loader-fit.c
> PowerPC Machines
> hw/intc/pnv_xive.c
> hw/intc/xics.c
> hw/intc/xics_kvm.c
> hw/intc/xics_pnv.c
> hw/intc/xics_spapr.c
> hw/isa/pc87312.c
> hw/misc/macio/macio.c
> hw/ppc/e500.c
> hw/ppc/mac_newworld.c
> hw/ppc/pnv.c
> hw/ppc/pnv_core.c
> hw/ppc/pnv_homer.c
> hw/ppc/pnv_lpc.c
> hw/ppc/pnv_occ.c
> hw/ppc/pnv_psi.c
> hw/ppc/spapr.c
> hw/ppc/spapr_caps.c
> hw/ppc/spapr_cpu_core.c
> hw/ppc/spapr_drc.c
> hw/ppc/spapr_irq.c
> 

Re: [PATCH v2 00/11] RFC crypto/luks: encryption key managment using amend interface

[Maxim Levitsky]

On Fri, 2019-10-04 at 21:10 +0200, Max Reitz wrote:
> On 13.09.19 00:30, Maxim Levitsky wrote:
> > This patch series is continuation of my work to add encryption
> > key managment to luks/qcow2 with luks.
> > 
> > This is second version of this patch set.
> > The changes are mostly addressing the review feedback,
> > plus I tested (and fixed sadly) the somewhat ugly code
> > that allows to still write share a raw luks device,
> > while preveting the key managment from happening in this case,
> > as it is unsafe.
> > I added a new iotest dedicated to that as well.
> > 
> > Best regards,
> > Maxim Levitsky
> 
> At least for an RFC looks good from my perspective.  I didn’t look at
> the crypto things very closely (assuming Dan would do so), and I didn’t
> check the iotests in detail.  (But it definitely doesn’t look like they
> lack in breadth.  Maybe I’d like to see a test that you cannot have
> other useful nodes attached to the LUKS or qcow2 node while the
> amendment process is ongoing (because CONSISTENT_READ is unshared).  But
> that’s the only thing I can think of.)
Could you elaborate on this? 

Inside the same process several users can access that luks node at the same
time while one of them changes encryption keys, since this doesn't affect IO of 
the data.

Two users in same process I was *I think* told that can't do the amend in the 
same time
since qmp is protected with a lock. However since I use a block job (to be 
consistent with blockdev-create)
I wonder if several qmp amend commands couldn't race one with another. These 
jobs is running
on the block device AIO context (I changed this recently after a review), but 
stil I am not sure
there can't be a race.

And when there is access to the same image from multiple processes, I do have a 
test that
checks that as long as more that one process has the image open, noone can 
change the encryption keys
(this is only relevant for raw luks format, since for qcow2 this is forbidden 
anyway).

I probably missed something though.

Best regards,
Maxim Levitsky



> 
> Max
> 

[PATCH v2 2/3] hw/mips/gt64xxx: Remove dynamic field width from trace events

[Philippe Mathieu-Daudé]

Since not all trace backends support dynamic field width in
format (dtrace via stap does not), replace by a static field
width instead.

Reported-by: Eric Blake 
Buglink: https://bugs.launchpad.net/qemu/+bug/1844817
Signed-off-by: Philippe Mathieu-Daudé 
---
v2: Do not update qemu_log_mask()
---
 hw/mips/gt64xxx_pci.c | 16 
 hw/mips/trace-events  |  4 ++--
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
index 5cab9c1ee1..6743e7c929 100644
--- a/hw/mips/gt64xxx_pci.c
+++ b/hw/mips/gt64xxx_pci.c
@@ -642,19 +642,19 @@ static void gt64120_writel(void *opaque, hwaddr addr,
 /* not really implemented */
 s->regs[saddr] = ~(~(s->regs[saddr]) | ~(val & 0xfffe));
 s->regs[saddr] |= !!(s->regs[saddr] & 0xfffe);
-trace_gt64120_write("INTRCAUSE", size << 1, val);
+trace_gt64120_write("INTRCAUSE", size << 3, val);
 break;
 case GT_INTRMASK:
 s->regs[saddr] = val & 0x3c3e;
-trace_gt64120_write("INTRMASK", size << 1, val);
+trace_gt64120_write("INTRMASK", size << 3, val);
 break;
 case GT_PCI0_ICMASK:
 s->regs[saddr] = val & 0x03fe;
-trace_gt64120_write("ICMASK", size << 1, val);
+trace_gt64120_write("ICMASK", size << 3, val);
 break;
 case GT_PCI0_SERR0MASK:
 s->regs[saddr] = val & 0x003f;
-trace_gt64120_write("SERR0MASK", size << 1, val);
+trace_gt64120_write("SERR0MASK", size << 3, val);
 break;
 
 /* Reserved when only PCI_0 is configured. */
@@ -930,19 +930,19 @@ static uint64_t gt64120_readl(void *opaque,
 /* Interrupts */
 case GT_INTRCAUSE:
 val = s->regs[saddr];
-trace_gt64120_read("INTRCAUSE", size << 1, val);
+trace_gt64120_read("INTRCAUSE", size << 3, val);
 break;
 case GT_INTRMASK:
 val = s->regs[saddr];
-trace_gt64120_read("INTRMASK", size << 1, val);
+trace_gt64120_read("INTRMASK", size << 3, val);
 break;
 case GT_PCI0_ICMASK:
 val = s->regs[saddr];
-trace_gt64120_read("ICMASK", size << 1, val);
+trace_gt64120_read("ICMASK", size << 3, val);
 break;
 case GT_PCI0_SERR0MASK:
 val = s->regs[saddr];
-trace_gt64120_read("SERR0MASK", size << 1, val);
+trace_gt64120_read("SERR0MASK", size << 3, val);
 break;
 
 /* Reserved when only PCI_0 is configured. */
diff --git a/hw/mips/trace-events b/hw/mips/trace-events
index 75d4c73f2e..86a0213c77 100644
--- a/hw/mips/trace-events
+++ b/hw/mips/trace-events
@@ -1,4 +1,4 @@
 # gt64xxx.c
-gt64120_read(const char *regname, int width, uint64_t value) "gt64120 read %s 
value:0x%0*" PRIx64
-gt64120_write(const char *regname, int width, uint64_t value) "gt64120 write 
%s value:0x%0*" PRIx64
+gt64120_read(const char *regname, int width, uint64_t value) "gt64120 read %s 
width:%d value:0x%08" PRIx64
+gt64120_write(const char *regname, int width, uint64_t value) "gt64120 write 
%s width:%d value:0x%08" PRIx64
 gt64120_isd_remap(uint64_t from_length, uint64_t from_addr, uint64_t 
to_length, uint64_t to_addr) "ISD: 0x%08" PRIx64 "@0x%08" PRIx64 " -> 0x%08" 
PRIx64 "@0x%08" PRIx64
-- 
2.21.0

[PATCH v2 3/3] trace: Forbid dynamic field width in event format

[Philippe Mathieu-Daudé]

Since not all trace backends support dynamic field width in
format (dtrace via stap does not), forbid them.

Add a check to refuse field width in new formats:

  $ make
  [...]
GEN hw/block/trace.h
  Traceback (most recent call last):
File "scripts/tracetool.py", line 152, in 
  main(sys.argv)
File "scripts/tracetool.py", line 143, in main
  events.extend(tracetool.read_events(fh, arg))
File "scripts/tracetool/__init__.py", line 371, in read_events
  event = Event.build(line)
File "scripts/tracetool/__init__.py", line 285, in build
  raise ValueError("Event format must not contain field width '%*'")
  ValueError: Error at hw/block/trace-events:11: Event format must not contain 
field width '%*'

Reported-by: Eric Blake 
Buglink: https://bugs.launchpad.net/qemu/+bug/1844817
Signed-off-by: Philippe Mathieu-Daudé 
---
 scripts/tracetool/__init__.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
index 44c118bc2a..e239be602b 100644
--- a/scripts/tracetool/__init__.py
+++ b/scripts/tracetool/__init__.py
@@ -206,6 +206,7 @@ class Event(object):
   "\s*"
   "(?:(?:(?P\".+),)?\s*(?P\".+))?"
   "\s*")
+_DFWRE = re.compile(".*(%0?\*).*")
 
 _VALID_PROPS = set(["disable", "tcg", "tcg-trans", "tcg-exec", "vcpu"])
 
@@ -280,6 +281,8 @@ class Event(object):
 if fmt.endswith(r'\n"'):
 raise ValueError("Event format must not end with a newline "
  "character")
+if Event._DFWRE.match(fmt):
+raise ValueError("Event format must not contain field width '%*'")
 
 if len(fmt_trans) > 0:
 fmt = [fmt_trans, fmt]
-- 
2.21.0

[PATCH v2 1/3] hw/block/pflash: Remove dynamic field width from trace events

[Philippe Mathieu-Daudé]

Since not all trace backends support dynamic field width in
format (dtrace via stap does not), replace by a static field
width instead.

Reported-by: Eric Blake 
Buglink: https://bugs.launchpad.net/qemu/+bug/1844817
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/block/pflash_cfi01.c | 8 
 hw/block/pflash_cfi02.c | 8 
 hw/block/trace-events   | 8 
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 566c0acb77..787d1196f2 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -276,7 +276,7 @@ static uint32_t pflash_data_read(PFlashCFI01 *pfl, hwaddr 
offset,
 DPRINTF("BUG in %s\n", __func__);
 abort();
 }
-trace_pflash_data_read(offset, width << 1, ret);
+trace_pflash_data_read(offset, width << 3, ret);
 return ret;
 }
 
@@ -389,7 +389,7 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset,
 
 break;
 }
-trace_pflash_io_read(offset, width, width << 1, ret, pfl->cmd, 
pfl->wcycle);
+trace_pflash_io_read(offset, width << 3, ret, pfl->cmd, pfl->wcycle);
 
 return ret;
 }
@@ -414,7 +414,7 @@ static inline void pflash_data_write(PFlashCFI01 *pfl, 
hwaddr offset,
 {
 uint8_t *p = pfl->storage;
 
-trace_pflash_data_write(offset, width << 1, value, pfl->counter);
+trace_pflash_data_write(offset, width << 3, value, pfl->counter);
 switch (width) {
 case 1:
 p[offset] = value;
@@ -453,7 +453,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
 
 cmd = value;
 
-trace_pflash_io_write(offset, width, width << 1, value, pfl->wcycle);
+trace_pflash_io_write(offset, width << 3, value, pfl->wcycle);
 if (!pfl->wcycle) {
 /* Set the device in I/O access mode */
 memory_region_rom_device_set_romd(>mem, false);
diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 4baca701b7..f2993cdfaa 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -260,7 +260,7 @@ static uint64_t pflash_data_read(PFlashCFI02 *pfl, hwaddr 
offset,
 {
 uint8_t *p = (uint8_t *)pfl->storage + offset;
 uint64_t ret = pfl->be ? ldn_be_p(p, width) : ldn_le_p(p, width);
-trace_pflash_data_read(offset, width << 1, ret);
+trace_pflash_data_read(offset, width << 3, ret);
 return ret;
 }
 
@@ -385,7 +385,7 @@ static uint64_t pflash_read(void *opaque, hwaddr offset, 
unsigned int width)
 }
 break;
 }
-trace_pflash_io_read(offset, width, width << 1, ret, pfl->cmd, 
pfl->wcycle);
+trace_pflash_io_read(offset, width << 3, ret, pfl->cmd, pfl->wcycle);
 
 return ret;
 }
@@ -432,7 +432,7 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 uint8_t *p;
 uint8_t cmd;
 
-trace_pflash_io_write(offset, width, width << 1, value, pfl->wcycle);
+trace_pflash_io_write(offset, width << 3, value, pfl->wcycle);
 cmd = value;
 if (pfl->cmd != 0xA0) {
 /* Reset does nothing during chip erase and sector erase. */
@@ -542,7 +542,7 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 }
 goto reset_flash;
 }
-trace_pflash_data_write(offset, width << 1, value, 0);
+trace_pflash_data_write(offset, width << 3, value, 0);
 if (!pfl->ro) {
 p = (uint8_t *)pfl->storage + offset;
 if (pfl->be) {
diff --git a/hw/block/trace-events b/hw/block/trace-events
index 13d1b21dd4..b9e195e172 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -8,10 +8,10 @@ fdc_ioport_write(uint8_t reg, uint8_t value) "write reg 
0x%02x val 0x%02x"
 # pflash_cfi01.c
 pflash_reset(void) "reset"
 pflash_timer_expired(uint8_t cmd) "command 0x%02x done"
-pflash_io_read(uint64_t offset, int width, int fmt_width, uint32_t value, 
uint8_t cmd, uint8_t wcycle) "offset:0x%04"PRIx64" width:%d value:0x%0*x 
cmd:0x%02x wcycle:%u"
-pflash_io_write(uint64_t offset, int width, int fmt_width, uint32_t value, 
uint8_t wcycle) "offset:0x%04"PRIx64" width:%d value:0x%0*x wcycle:%u"
-pflash_data_read(uint64_t offset, int width, uint32_t value) "data 
offset:0x%04"PRIx64" value:0x%0*x"
-pflash_data_write(uint64_t offset, int width, uint32_t value, uint64_t 
counter) "data offset:0x%04"PRIx64" value:0x%0*x counter:0x%016"PRIx64
+pflash_io_read(uint64_t offset, int width, uint32_t value, uint8_t cmd, 
uint8_t wcycle) "offset:0x%04"PRIx64" width:%d value:0x%04x cmd:0x%02x 
wcycle:%u"
+pflash_io_write(uint64_t offset, int width, uint32_t value, uint8_t wcycle) 
"offset:0x%04"PRIx64" width:%d value:0x%04x wcycle:%u"
+pflash_data_read(uint64_t offset, int width, uint32_t value) "data 
offset:0x%04"PRIx64" width:%d value:0x%04x"
+pflash_data_write(uint64_t offset, int width, uint32_t value, uint64_t 
counter) "data offset:0x%04"PRIx64" width:%d value:0x%04x counter:0x%016"PRIx64
 pflash_manufacturer_id(uint16_t id) "Read 

[PATCH v2 0/3] hw: Remove dynamic field width from trace events

[Philippe Mathieu-Daudé]

Eric noted in [1] the dtrace via stap backend can not support
the dynamic '*' width format.
I'd really like to use dynamic width in trace event because the
read/write accesses are easier to read but it is not a priority.
Since next release is close, time to fix LP#1844817 [2].

Since v1:
- Do not update the qemu_log_mask() calls in hw/mips/gt64xxx_pci.c

[1] https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg04720.html
[2] https://bugs.launchpad.net/qemu/+bug/1844817

Philippe Mathieu-Daudé (3):
  hw/block/pflash: Remove dynamic field width from trace events
  hw/mips/gt64xxx: Remove dynamic field width from trace events
  trace: Forbid dynamic field width in event format

 hw/block/pflash_cfi01.c   |  8 
 hw/block/pflash_cfi02.c   |  8 
 hw/mips/gt64xxx_pci.c | 16 
 hw/block/trace-events |  8 
 hw/mips/trace-events  |  4 ++--
 scripts/tracetool/__init__.py |  3 +++
 6 files changed, 25 insertions(+), 22 deletions(-)

-- 
2.21.0

Re: [PATCH 2/3] hw/mips/gt64xxx: Remove dynamic field width from trace event

[Philippe Mathieu-Daudé]

On 11/8/19 3:26 PM, Philippe Mathieu-Daudé wrote:

Since not all trace backends support dynamic field width in
format (dtrace via stap does not), replace by a static field
width instead.

Reported-by: Eric Blake 
Buglink: https://bugs.launchpad.net/qemu/+bug/1844817
Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/mips/gt64xxx_pci.c | 34 +-
  hw/mips/trace-events  |  4 ++--
  2 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
index 5cab9c1ee1..f427793360 100644
--- a/hw/mips/gt64xxx_pci.c
+++ b/hw/mips/gt64xxx_pci.c
@@ -464,7 +464,7 @@ static void gt64120_writel(void *opaque, hwaddr addr,
  qemu_log_mask(LOG_GUEST_ERROR,
"gt64120: Read-only register write "
"reg:0x03%x size:%u value:0x%0*" PRIx64 "\n",
-  saddr << 2, size, size << 1, val);
+  saddr << 2, size, size << 3, val);


Wrong replacement :( I'll respin.


  break;
  
  /* CPU Sync Barrier */

@@ -474,7 +474,7 @@ static void gt64120_writel(void *opaque, hwaddr addr,
  qemu_log_mask(LOG_GUEST_ERROR,
"gt64120: Read-only register write "
"reg:0x03%x size:%u value:0x%0*" PRIx64 "\n",
-  saddr << 2, size, size << 1, val);
+  saddr << 2, size, size << 3, val);
  break;
  
  /* SDRAM and Device Address Decode */

@@ -516,7 +516,7 @@ static void gt64120_writel(void *opaque, hwaddr addr,
  qemu_log_mask(LOG_UNIMP,
"gt64120: Unimplemented device register write "
"reg:0x03%x size:%u value:0x%0*" PRIx64 "\n",
-  saddr << 2, size, size << 1, val);
+  saddr << 2, size, size << 3, val);
  break;
  
  /* ECC */

@@ -529,7 +529,7 @@ static void gt64120_writel(void *opaque, hwaddr addr,
  qemu_log_mask(LOG_GUEST_ERROR,
"gt64120: Read-only register write "
"reg:0x03%x size:%u value:0x%0*" PRIx64 "\n",
-  saddr << 2, size, size << 1, val);
+  saddr << 2, size, size << 3, val);
  break;
  
  /* DMA Record */

@@ -566,7 +566,7 @@ static void gt64120_writel(void *opaque, hwaddr addr,
  qemu_log_mask(LOG_UNIMP,
"gt64120: Unimplemented DMA register write "
"reg:0x03%x size:%u value:0x%0*" PRIx64 "\n",
-  saddr << 2, size, size << 1, val);
+  saddr << 2, size, size << 3, val);
  break;
  
  /* Timer/Counter */

@@ -579,7 +579,7 @@ static void gt64120_writel(void *opaque, hwaddr addr,
  qemu_log_mask(LOG_UNIMP,
"gt64120: Unimplemented timer register write "
"reg:0x03%x size:%u value:0x%0*" PRIx64 "\n",
-  saddr << 2, size, size << 1, val);
+  saddr << 2, size, size << 3, val);
  break;
  
  /* PCI Internal */

@@ -623,7 +623,7 @@ static void gt64120_writel(void *opaque, hwaddr addr,
  qemu_log_mask(LOG_UNIMP,
"gt64120: Unimplemented timer register write "
"reg:0x03%x size:%u value:0x%0*" PRIx64 "\n",
-  saddr << 2, size, size << 1, val);
+  saddr << 2, size, size << 3, val);
  break;
  case GT_PCI0_CFGADDR:
  phb->config_reg = val & 0x80fc;
@@ -642,19 +642,19 @@ static void gt64120_writel(void *opaque, hwaddr addr,
  /* not really implemented */
  s->regs[saddr] = ~(~(s->regs[saddr]) | ~(val & 0xfffe));
  s->regs[saddr] |= !!(s->regs[saddr] & 0xfffe);
-trace_gt64120_write("INTRCAUSE", size << 1, val);
+trace_gt64120_write("INTRCAUSE", size << 3, val);
  break;
  case GT_INTRMASK:
  s->regs[saddr] = val & 0x3c3e;
-trace_gt64120_write("INTRMASK", size << 1, val);
+trace_gt64120_write("INTRMASK", size << 3, val);
  break;
  case GT_PCI0_ICMASK:
  s->regs[saddr] = val & 0x03fe;
-trace_gt64120_write("ICMASK", size << 1, val);
+trace_gt64120_write("ICMASK", size << 3, val);
  break;
  case GT_PCI0_SERR0MASK:
  s->regs[saddr] = val & 0x003f;
-trace_gt64120_write("SERR0MASK", size << 1, val);
+trace_gt64120_write("SERR0MASK", size << 3, val);
  break;
  
  /* Reserved when only PCI_0 is configured. */

@@ -683,7 +683,7 @@ static void gt64120_writel(void *opaque, hwaddr addr,
  qemu_log_mask(LOG_GUEST_ERROR,
"gt64120: Illegal register write "
"reg:0x03%x size:%u value:0x%0*" PRIx64 "\n",
-  saddr << 2, size, size << 1, val);
+  saddr << 2, size, size << 3, val);
   

Re: [PATCH 1/2] block: Remove 'backing': null from bs->{explicit_, }options

[Alberto Garcia]

On Fri 08 Nov 2019 09:53:11 AM CET, Kevin Wolf wrote:
> bs->options and bs->explicit_options shouldn't contain any options for
> child nodes. bdrv_open_inherited() takes care to remove any options that
> match a child name after opening the image and the same is done when
> reopening.
>
> However, we miss the case of 'backing': null, which is a child option,
> but results in no child being created. This means that a 'backing': null
> remains in bs->options and bs->explicit_options.
>
> A typical use for 'backing': null is in live snapshots: blockdev-add for
> the qcow2 overlay makes sure not to open the backing file (because it is
> already opened and blockdev-snapshot will attach it). After doing a
> blockdev-snapshot, bs->options and bs->explicit_options become
> inconsistent with the actual state (bs has a backing file now, but the
> options still say null). On the next occasion that the image is
> reopened, e.g. switching it from read-write to read-only when another
> snapshot is taken, the option will take effect again and the node
> incorrectly loses its backing file.
>
> Fix bdrv_open_inherited() to remove the 'backing' option from
> bs->options and bs->explicit_options even for the case where it
> specifies that no backing file is wanted.
>
> Reported-by: Peter Krempa 
> Signed-off-by: Kevin Wolf 

Reviewed-by: Alberto Garcia 

Berto

Re: [PATCH 2/2] iotests: Test multiple blockdev-snapshot calls

[Alberto Garcia]

On Fri 08 Nov 2019 09:53:12 AM CET, Kevin Wolf wrote:
> +# Test large write to a qcow2 image

This doesn't belong here I guess :)

I wonder if this test could go in 245 instead.

Berto

[PATCH 3/3] trace: Forbid dynamic field width in event format

[Philippe Mathieu-Daudé]

Since not all trace backends support dynamic field width in
format (dtrace via stap does not), forbid them.

Add a check to refuse field width in new formats:

  $ make
  [...]
GEN hw/block/trace.h
  Traceback (most recent call last):
File "scripts/tracetool.py", line 152, in 
  main(sys.argv)
File "scripts/tracetool.py", line 143, in main
  events.extend(tracetool.read_events(fh, arg))
File "scripts/tracetool/__init__.py", line 371, in read_events
  event = Event.build(line)
File "scripts/tracetool/__init__.py", line 285, in build
  raise ValueError("Event format must not contain field width '%*'")
  ValueError: Error at hw/block/trace-events:11: Event format must not contain 
field width '%*'

Reported-by: Eric Blake 
Buglink: https://bugs.launchpad.net/qemu/+bug/1844817
Signed-off-by: Philippe Mathieu-Daudé 
---
 scripts/tracetool/__init__.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
index 44c118bc2a..e239be602b 100644
--- a/scripts/tracetool/__init__.py
+++ b/scripts/tracetool/__init__.py
@@ -206,6 +206,7 @@ class Event(object):
   "\s*"
   "(?:(?:(?P\".+),)?\s*(?P\".+))?"
   "\s*")
+_DFWRE = re.compile(".*(%0?\*).*")
 
 _VALID_PROPS = set(["disable", "tcg", "tcg-trans", "tcg-exec", "vcpu"])
 
@@ -280,6 +281,8 @@ class Event(object):
 if fmt.endswith(r'\n"'):
 raise ValueError("Event format must not end with a newline "
  "character")
+if Event._DFWRE.match(fmt):
+raise ValueError("Event format must not contain field width '%*'")
 
 if len(fmt_trans) > 0:
 fmt = [fmt_trans, fmt]
-- 
2.21.0

[PATCH 1/3] hw/block/pflash: Remove dynamic field width from trace event

[Philippe Mathieu-Daudé]

Since not all trace backends support dynamic field width in
format (dtrace via stap does not), replace by a static field
width instead.

Reported-by: Eric Blake 
Buglink: https://bugs.launchpad.net/qemu/+bug/1844817
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/block/pflash_cfi01.c | 8 
 hw/block/pflash_cfi02.c | 8 
 hw/block/trace-events   | 8 
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 566c0acb77..787d1196f2 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -276,7 +276,7 @@ static uint32_t pflash_data_read(PFlashCFI01 *pfl, hwaddr 
offset,
 DPRINTF("BUG in %s\n", __func__);
 abort();
 }
-trace_pflash_data_read(offset, width << 1, ret);
+trace_pflash_data_read(offset, width << 3, ret);
 return ret;
 }
 
@@ -389,7 +389,7 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset,
 
 break;
 }
-trace_pflash_io_read(offset, width, width << 1, ret, pfl->cmd, 
pfl->wcycle);
+trace_pflash_io_read(offset, width << 3, ret, pfl->cmd, pfl->wcycle);
 
 return ret;
 }
@@ -414,7 +414,7 @@ static inline void pflash_data_write(PFlashCFI01 *pfl, 
hwaddr offset,
 {
 uint8_t *p = pfl->storage;
 
-trace_pflash_data_write(offset, width << 1, value, pfl->counter);
+trace_pflash_data_write(offset, width << 3, value, pfl->counter);
 switch (width) {
 case 1:
 p[offset] = value;
@@ -453,7 +453,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
 
 cmd = value;
 
-trace_pflash_io_write(offset, width, width << 1, value, pfl->wcycle);
+trace_pflash_io_write(offset, width << 3, value, pfl->wcycle);
 if (!pfl->wcycle) {
 /* Set the device in I/O access mode */
 memory_region_rom_device_set_romd(>mem, false);
diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 4baca701b7..f2993cdfaa 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -260,7 +260,7 @@ static uint64_t pflash_data_read(PFlashCFI02 *pfl, hwaddr 
offset,
 {
 uint8_t *p = (uint8_t *)pfl->storage + offset;
 uint64_t ret = pfl->be ? ldn_be_p(p, width) : ldn_le_p(p, width);
-trace_pflash_data_read(offset, width << 1, ret);
+trace_pflash_data_read(offset, width << 3, ret);
 return ret;
 }
 
@@ -385,7 +385,7 @@ static uint64_t pflash_read(void *opaque, hwaddr offset, 
unsigned int width)
 }
 break;
 }
-trace_pflash_io_read(offset, width, width << 1, ret, pfl->cmd, 
pfl->wcycle);
+trace_pflash_io_read(offset, width << 3, ret, pfl->cmd, pfl->wcycle);
 
 return ret;
 }
@@ -432,7 +432,7 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 uint8_t *p;
 uint8_t cmd;
 
-trace_pflash_io_write(offset, width, width << 1, value, pfl->wcycle);
+trace_pflash_io_write(offset, width << 3, value, pfl->wcycle);
 cmd = value;
 if (pfl->cmd != 0xA0) {
 /* Reset does nothing during chip erase and sector erase. */
@@ -542,7 +542,7 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 }
 goto reset_flash;
 }
-trace_pflash_data_write(offset, width << 1, value, 0);
+trace_pflash_data_write(offset, width << 3, value, 0);
 if (!pfl->ro) {
 p = (uint8_t *)pfl->storage + offset;
 if (pfl->be) {
diff --git a/hw/block/trace-events b/hw/block/trace-events
index 13d1b21dd4..b9e195e172 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -8,10 +8,10 @@ fdc_ioport_write(uint8_t reg, uint8_t value) "write reg 
0x%02x val 0x%02x"
 # pflash_cfi01.c
 pflash_reset(void) "reset"
 pflash_timer_expired(uint8_t cmd) "command 0x%02x done"
-pflash_io_read(uint64_t offset, int width, int fmt_width, uint32_t value, 
uint8_t cmd, uint8_t wcycle) "offset:0x%04"PRIx64" width:%d value:0x%0*x 
cmd:0x%02x wcycle:%u"
-pflash_io_write(uint64_t offset, int width, int fmt_width, uint32_t value, 
uint8_t wcycle) "offset:0x%04"PRIx64" width:%d value:0x%0*x wcycle:%u"
-pflash_data_read(uint64_t offset, int width, uint32_t value) "data 
offset:0x%04"PRIx64" value:0x%0*x"
-pflash_data_write(uint64_t offset, int width, uint32_t value, uint64_t 
counter) "data offset:0x%04"PRIx64" value:0x%0*x counter:0x%016"PRIx64
+pflash_io_read(uint64_t offset, int width, uint32_t value, uint8_t cmd, 
uint8_t wcycle) "offset:0x%04"PRIx64" width:%d value:0x%04x cmd:0x%02x 
wcycle:%u"
+pflash_io_write(uint64_t offset, int width, uint32_t value, uint8_t wcycle) 
"offset:0x%04"PRIx64" width:%d value:0x%04x wcycle:%u"
+pflash_data_read(uint64_t offset, int width, uint32_t value) "data 
offset:0x%04"PRIx64" width:%d value:0x%04x"
+pflash_data_write(uint64_t offset, int width, uint32_t value, uint64_t 
counter) "data offset:0x%04"PRIx64" width:%d value:0x%04x counter:0x%016"PRIx64
 pflash_manufacturer_id(uint16_t id) "Read 

[PATCH 2/3] hw/mips/gt64xxx: Remove dynamic field width from trace event

[Philippe Mathieu-Daudé]

Since not all trace backends support dynamic field width in
format (dtrace via stap does not), replace by a static field
width instead.

Reported-by: Eric Blake 
Buglink: https://bugs.launchpad.net/qemu/+bug/1844817
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/mips/gt64xxx_pci.c | 34 +-
 hw/mips/trace-events  |  4 ++--
 2 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
index 5cab9c1ee1..f427793360 100644
--- a/hw/mips/gt64xxx_pci.c
+++ b/hw/mips/gt64xxx_pci.c
@@ -464,7 +464,7 @@ static void gt64120_writel(void *opaque, hwaddr addr,
 qemu_log_mask(LOG_GUEST_ERROR,
   "gt64120: Read-only register write "
   "reg:0x03%x size:%u value:0x%0*" PRIx64 "\n",
-  saddr << 2, size, size << 1, val);
+  saddr << 2, size, size << 3, val);
 break;
 
 /* CPU Sync Barrier */
@@ -474,7 +474,7 @@ static void gt64120_writel(void *opaque, hwaddr addr,
 qemu_log_mask(LOG_GUEST_ERROR,
   "gt64120: Read-only register write "
   "reg:0x03%x size:%u value:0x%0*" PRIx64 "\n",
-  saddr << 2, size, size << 1, val);
+  saddr << 2, size, size << 3, val);
 break;
 
 /* SDRAM and Device Address Decode */
@@ -516,7 +516,7 @@ static void gt64120_writel(void *opaque, hwaddr addr,
 qemu_log_mask(LOG_UNIMP,
   "gt64120: Unimplemented device register write "
   "reg:0x03%x size:%u value:0x%0*" PRIx64 "\n",
-  saddr << 2, size, size << 1, val);
+  saddr << 2, size, size << 3, val);
 break;
 
 /* ECC */
@@ -529,7 +529,7 @@ static void gt64120_writel(void *opaque, hwaddr addr,
 qemu_log_mask(LOG_GUEST_ERROR,
   "gt64120: Read-only register write "
   "reg:0x03%x size:%u value:0x%0*" PRIx64 "\n",
-  saddr << 2, size, size << 1, val);
+  saddr << 2, size, size << 3, val);
 break;
 
 /* DMA Record */
@@ -566,7 +566,7 @@ static void gt64120_writel(void *opaque, hwaddr addr,
 qemu_log_mask(LOG_UNIMP,
   "gt64120: Unimplemented DMA register write "
   "reg:0x03%x size:%u value:0x%0*" PRIx64 "\n",
-  saddr << 2, size, size << 1, val);
+  saddr << 2, size, size << 3, val);
 break;
 
 /* Timer/Counter */
@@ -579,7 +579,7 @@ static void gt64120_writel(void *opaque, hwaddr addr,
 qemu_log_mask(LOG_UNIMP,
   "gt64120: Unimplemented timer register write "
   "reg:0x03%x size:%u value:0x%0*" PRIx64 "\n",
-  saddr << 2, size, size << 1, val);
+  saddr << 2, size, size << 3, val);
 break;
 
 /* PCI Internal */
@@ -623,7 +623,7 @@ static void gt64120_writel(void *opaque, hwaddr addr,
 qemu_log_mask(LOG_UNIMP,
   "gt64120: Unimplemented timer register write "
   "reg:0x03%x size:%u value:0x%0*" PRIx64 "\n",
-  saddr << 2, size, size << 1, val);
+  saddr << 2, size, size << 3, val);
 break;
 case GT_PCI0_CFGADDR:
 phb->config_reg = val & 0x80fc;
@@ -642,19 +642,19 @@ static void gt64120_writel(void *opaque, hwaddr addr,
 /* not really implemented */
 s->regs[saddr] = ~(~(s->regs[saddr]) | ~(val & 0xfffe));
 s->regs[saddr] |= !!(s->regs[saddr] & 0xfffe);
-trace_gt64120_write("INTRCAUSE", size << 1, val);
+trace_gt64120_write("INTRCAUSE", size << 3, val);
 break;
 case GT_INTRMASK:
 s->regs[saddr] = val & 0x3c3e;
-trace_gt64120_write("INTRMASK", size << 1, val);
+trace_gt64120_write("INTRMASK", size << 3, val);
 break;
 case GT_PCI0_ICMASK:
 s->regs[saddr] = val & 0x03fe;
-trace_gt64120_write("ICMASK", size << 1, val);
+trace_gt64120_write("ICMASK", size << 3, val);
 break;
 case GT_PCI0_SERR0MASK:
 s->regs[saddr] = val & 0x003f;
-trace_gt64120_write("SERR0MASK", size << 1, val);
+trace_gt64120_write("SERR0MASK", size << 3, val);
 break;
 
 /* Reserved when only PCI_0 is configured. */
@@ -683,7 +683,7 @@ static void gt64120_writel(void *opaque, hwaddr addr,
 qemu_log_mask(LOG_GUEST_ERROR,
   "gt64120: Illegal register write "
   "reg:0x03%x size:%u value:0x%0*" PRIx64 "\n",
-  saddr << 2, size, size << 1, val);
+  saddr << 2, size, size << 3, val);
 break;
 }
 }
@@ -930,19 +930,19 @@ static uint64_t gt64120_readl(void *opaque,
 /* Interrupts */
 case GT_INTRCAUSE:
 val = 

[PATCH 0/3] hw: Remove dynamic field width from trace event

[Philippe Mathieu-Daudé]

Eric noted in [1] the dtrace via stap backend can not support
the dynamic '*' width format.
I'd really like to use dynamic width in trace event because the
read/write accesses are easier to read but it is not a priority.
Since next release is close, time to fix LP#1844817 [2].

[1] https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg04720.html
[2] https://bugs.launchpad.net/qemu/+bug/1844817

Philippe Mathieu-Daudé (3):
  hw/block/pflash: Remove dynamic field width from trace event
  hw/mips/gt64xxx: Remove dynamic field width from trace event
  trace: Forbid dynamic field width in event format

 hw/block/pflash_cfi01.c   |  8 
 hw/block/pflash_cfi02.c   |  8 
 hw/mips/gt64xxx_pci.c | 34 +-
 hw/block/trace-events |  8 
 hw/mips/trace-events  |  4 ++--
 scripts/tracetool/__init__.py |  3 +++
 6 files changed, 34 insertions(+), 31 deletions(-)

-- 
2.21.0

Re: [PATCH v2 1/1] virtio: make seg_max virtqueue size dependent

[Michael S. Tsirkin]

On Fri, Nov 08, 2019 at 04:42:49PM +0300, Denis Plotnikov wrote:
> seg_max has a restriction to be less or equal to virtqueue size
> according to Virtio 1.0 specification
> 
> Although seg_max can't be set directly, it's worth to express this
> dependancy directly in the code for sanity purpose.
> 
> Signed-off-by: Denis Plotnikov 

Guest visible so please make this depend on machine type.

> ---
>  hw/block/virtio-blk.c | 2 +-
>  hw/scsi/virtio-scsi.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 06e57a4d39..21530304cf 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -903,7 +903,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, 
> uint8_t *config)
>  blk_get_geometry(s->blk, );
>  memset(, 0, sizeof(blkcfg));
>  virtio_stq_p(vdev, , capacity);
> -virtio_stl_p(vdev, _max, 128 - 2);
> +virtio_stl_p(vdev, _max, s->conf.queue_size - 2);
>  virtio_stw_p(vdev, , conf->cyls);
>  virtio_stl_p(vdev, _size, blk_size);
>  virtio_stw_p(vdev, _io_size, conf->min_io_size / blk_size);
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 839f120256..f7e5533cd5 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -650,7 +650,7 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
>  VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
>  
>  virtio_stl_p(vdev, >num_queues, s->conf.num_queues);
> -virtio_stl_p(vdev, >seg_max, 128 - 2);
> +virtio_stl_p(vdev, >seg_max, s->conf.virtqueue_size - 2);
>  virtio_stl_p(vdev, >max_sectors, s->conf.max_sectors);
>  virtio_stl_p(vdev, >cmd_per_lun, s->conf.cmd_per_lun);
>  virtio_stl_p(vdev, >event_info_size, sizeof(VirtIOSCSIEvent));
> -- 
> 2.17.0

Re: [PATCH v2 2/2] qapi: deprecate implicit filters

[Peter Krempa]

On Fri, Nov 08, 2019 at 13:56:03 +, Vladimir Sementsov-Ogievskiy wrote:
> 08.11.2019 16:27, Peter Krempa wrote:
> > On Fri, Nov 08, 2019 at 13:16:55 +0300, Vladimir Sementsov-Ogievskiy wrote:

[...]

> > Note that 'block-commit' and 'drive-mirror' commands are used by libvirt
> > in the pre-blockdev era. In those instances we gather statistics of
> > block devices by nesting in the output of query-blockstats and
> > query-block rather than selecting the appropriate info by any other
> > means (e.g. by node name).
> > 
> > This means that the output MUST stay consistend when block jobs are used
> > and the hack this patch is deprcating will break those.
> > 
> > Note that in libvirt we don't plan to invest time to add workarounds for
> > non-blockdev cases since blockdev by itself is complex enough and I'd
> > strongly prefer not having a third code path to care about.
> > 
> > Given that -blockdev can't be used in all cases (e.g. for sd-cards)
> > which also blocks deprecation of -drive I don't think that hiding of
> > implicit filter nodes can be deprecated until -drive is deprecated.
> > 
> 
> 
> OK, so, we can't deprecate anything around it now.
> 
> What is the problem with sd-cards?

So the problem was that it was impossible to instantiate it via -device,
but looking at the qemu code base this doesn't seem to be true any more.

I'll have a look whether we can rework the instantiation of sd card
frontends in libvirt somehow or whether it actually ever worked.
Unfortunately the documentation seems to be rather sparse.

Re: [PATCH for-5.0 v4 2/5] block: Use bdrv_qapi_perm_to_blk_perm()

[Vladimir Sementsov-Ogievskiy]

08.11.2019 15:34, Max Reitz wrote:
> We can save some LoC in xdbg_graph_add_edge() by using
> bdrv_qapi_perm_to_blk_perm().
> 
> Signed-off-by: Max Reitz 

Reviewed-by: Vladimir Sementsov-Ogievskiy 

> ---
>   block.c | 29 -
>   1 file changed, 8 insertions(+), 21 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 066433f3e2..ae279ff21f 100644
> --- a/block.c
> +++ b/block.c
> @@ -4870,36 +4870,23 @@ static void 
> xdbg_graph_add_node(XDbgBlockGraphConstructor *gr, void *node,
>   static void xdbg_graph_add_edge(XDbgBlockGraphConstructor *gr, void *parent,
>   const BdrvChild *child)
>   {
> -typedef struct {
> -unsigned int flag;
> -BlockPermission num;
> -} PermissionMap;
> -
> -static const PermissionMap permissions[] = {
> -{ BLK_PERM_CONSISTENT_READ, BLOCK_PERMISSION_CONSISTENT_READ },
> -{ BLK_PERM_WRITE,   BLOCK_PERMISSION_WRITE },
> -{ BLK_PERM_WRITE_UNCHANGED, BLOCK_PERMISSION_WRITE_UNCHANGED },
> -{ BLK_PERM_RESIZE,  BLOCK_PERMISSION_RESIZE },
> -{ BLK_PERM_GRAPH_MOD,   BLOCK_PERMISSION_GRAPH_MOD },
> -{ 0, 0 }
> -};
> -const PermissionMap *p;
> +BlockPermission qapi_perm;
>   XDbgBlockGraphEdge *edge;
>   
> -QEMU_BUILD_BUG_ON(1UL << (ARRAY_SIZE(permissions) - 1) != BLK_PERM_ALL + 
> 1);
> -
>   edge = g_new0(XDbgBlockGraphEdge, 1);
>   
>   edge->parent = xdbg_graph_node_num(gr, parent);
>   edge->child = xdbg_graph_node_num(gr, child->bs);
>   edge->name = g_strdup(child->name);
>   
> -for (p = permissions; p->flag; p++) {
> -if (p->flag & child->perm) {
> -QAPI_LIST_ADD(edge->perm, p->num);
> +for (qapi_perm = 0; qapi_perm < BLOCK_PERMISSION__MAX; qapi_perm++) {
> +uint64_t flag = bdrv_qapi_perm_to_blk_perm(qapi_perm);
> +
> +if (flag & child->perm) {
> +QAPI_LIST_ADD(edge->perm, qapi_perm);
>   }
> -if (p->flag & child->shared_perm) {
> -QAPI_LIST_ADD(edge->shared_perm, p->num);
> +if (flag & child->shared_perm) {
> +QAPI_LIST_ADD(edge->shared_perm, qapi_perm);
>   }
>   }
>   
> 


-- 
Best regards,
Vladimir

Re: [PATCH for-5.0 v4 1/5] block: Add bdrv_qapi_perm_to_blk_perm()

[Vladimir Sementsov-Ogievskiy]

08.11.2019 15:34, Max Reitz wrote:
> We need some way to correlate QAPI BlockPermission values with
> BLK_PERM_* flags.  We could:
> 
> (1) have the same order in the QAPI definition as the the BLK_PERM_*
>  flags are in LSb-first order.  However, then there is no guarantee
>  that they actually match (e.g. when someone modifies the QAPI schema
>  without thinking of the BLK_PERM_* definitions).
>  We could add static assertions, but these would break what’s good
>  about this solution, namely its simplicity.
> 
> (2) define the BLK_PERM_* flags based on the BlockPermission values.
>  But this way whenever someone were to modify the QAPI order
>  (perfectly sensible in theory), the BLK_PERM_* values would change.
>  Because these values are used for file locking, this might break
>  file locking between different qemu versions.
> 
> Therefore, go the slightly more cumbersome way: Add a function to
> translate from the QAPI constants to the BLK_PERM_* flags.
> 
> Signed-off-by: Max Reitz 

Reviewed-by: Vladimir Sementsov-Ogievskiy 

> ---
>   block.c   | 18 ++
>   include/block/block.h |  1 +
>   2 files changed, 19 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 4cffc2bc35..066433f3e2 100644
> --- a/block.c
> +++ b/block.c
> @@ -2227,6 +2227,24 @@ void bdrv_format_default_perms(BlockDriverState *bs, 
> BdrvChild *c,
>   *nshared = shared;
>   }
>   
> +uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm)
> +{
> +static const uint64_t permissions[] = {
> +[BLOCK_PERMISSION_CONSISTENT_READ]  = BLK_PERM_CONSISTENT_READ,
> +[BLOCK_PERMISSION_WRITE]= BLK_PERM_WRITE,
> +[BLOCK_PERMISSION_WRITE_UNCHANGED]  = BLK_PERM_WRITE_UNCHANGED,
> +[BLOCK_PERMISSION_RESIZE]   = BLK_PERM_RESIZE,
> +[BLOCK_PERMISSION_GRAPH_MOD]= BLK_PERM_GRAPH_MOD,
> +};
> +
> +QEMU_BUILD_BUG_ON(ARRAY_SIZE(permissions) != BLOCK_PERMISSION__MAX);
> +QEMU_BUILD_BUG_ON(1UL << ARRAY_SIZE(permissions) != BLK_PERM_ALL + 1);

safe enough)

> +
> +assert(qapi_perm < BLOCK_PERMISSION__MAX);
> +
> +return permissions[qapi_perm];
> +}
> +
>   static void bdrv_replace_child_noperm(BdrvChild *child,
> BlockDriverState *new_bs)
>   {
> diff --git a/include/block/block.h b/include/block/block.h
> index 1df9848e74..e9dcfef7fa 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -280,6 +280,7 @@ enum {
>   };
>   
>   char *bdrv_perm_names(uint64_t perm);
> +uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm);
>   
>   /* disk I/O throttling */
>   void bdrv_init(void);
> 


-- 
Best regards,
Vladimir

Re: [PATCH v3] iotests: Test NBD client reconnection

[Roman Kagan]

On Fri, Nov 08, 2019 at 01:49:50PM +, Vladimir Sementsov-Ogievskiy wrote:
> 01.11.2019 19:54, Andrey Shinkevich wrote:
> > +def check_proc_NBD(proc, connector):
> > +try:
> > +exitcode = proc.wait(timeout=10)
> > +
> > +if exitcode < 0:
> > +log('NBD {}: EXIT SIGNAL {}\n'.format(connector, -exitcode))
> > +log(proc.communicate()[0])
> > +else:
> > +line = proc.stdout.readline()
> 
> 
> could we use proc.communicate() for both cases, what is the difference?

In fact if proc produces any non-trivial amount of output you are better
off using .communicate() otherwise your child may block on output and
never exit.  See
https://docs.python.org/3/library/subprocess.html#subprocess.Popen.communicate
for how to express the above logic correctly.  The exit code *after*
.communicate is available in .returncode.

> 
> > +log('NBD {}: {}'.format(connector, line.rstrip()))
> > +
> > +except subprocess.TimeoutExpired:
> > +proc.kill()
> > +log('NBD {}: ERROR timeout expired'.format(connector))
> > +finally:
> > +if connector == 'server':
> > +os.remove(nbd_sock)
> > +os.remove(conf_file)

Roman.

Re: [PATCH v2 2/2] qapi: deprecate implicit filters

[Vladimir Sementsov-Ogievskiy]

08.11.2019 16:27, Peter Krempa wrote:
> On Fri, Nov 08, 2019 at 13:16:55 +0300, Vladimir Sementsov-Ogievskiy wrote:
>> To get rid of implicit filters related workarounds in future let's
>> deprecate them now.
>>
>> Deprecation warning breaks some bash iotests output, so fix it here
>> too: in most of cases just add filter-node-name in test.
>>
>> In 161 add FIXME and deprecation warning into 161.out.
>>
>> In 249, the test case is changed, as we don't need to test "default"
>> filter node name anymore.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>> ---
>>   qemu-deprecated.texi   |  6 ++
>>   qapi/block-core.json   |  9 ++---
>>   include/block/block_int.h  | 10 +-
>>   blockdev.c | 10 ++
>>   tests/qemu-iotests/094 |  1 +
>>   tests/qemu-iotests/095 |  6 --
>>   tests/qemu-iotests/109 |  1 +
>>   tests/qemu-iotests/127 |  1 +
>>   tests/qemu-iotests/141 |  5 -
>>   tests/qemu-iotests/144 |  3 ++-
>>   tests/qemu-iotests/156 |  1 +
>>   tests/qemu-iotests/161 |  7 +++
>>   tests/qemu-iotests/161.out |  1 +
>>   tests/qemu-iotests/185 |  3 +++
>>   tests/qemu-iotests/191 |  2 ++
>>   tests/qemu-iotests/229 |  1 +
>>   tests/qemu-iotests/247 |  8 +---
>>   tests/qemu-iotests/249 |  5 +++--
>>   tests/qemu-iotests/249.out |  2 +-
>>   19 files changed, 68 insertions(+), 14 deletions(-)
>>
>> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
>> index 296bfc93a3..c969faf55a 100644
>> --- a/qemu-deprecated.texi
>> +++ b/qemu-deprecated.texi
>> @@ -204,6 +204,12 @@ and accurate ``query-qmp-schema'' command.
>>   Character devices creating sockets in client mode should not specify
>>   the 'wait' field, which is only applicable to sockets in server mode
>>   
>> +@subsection implicit filters in mirror and commit (since 4.2)
>> +
>> +Mirror and commit jobs insert filters, which becomes implicit if user
>> +omitted @option(filter-node-name) parameter. So omitting it is deprecated
>> +in ``blockdev-mirror'', ``drive-mirror'' and ``block-commit'', set it 
>> always.
>> +
>>   @section Human Monitor Protocol (HMP) commands
>>   
>>   @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove' 
>> (since 3.1)
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 93530d3a13..37caed775f 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -1659,7 +1659,8 @@
>>   # @filter-node-name: the node name that should be assigned to the
>>   #filter driver that the commit job inserts into the 
>> graph
>>   #above @top. If this option is not given, a node name 
>> is
>> -#autogenerated. (Since: 2.9)
>> +#autogenerated. Omitting this option is deprecated, it 
>> will
>> +#be required in future. (Since: 2.9)
>>   #
>>   # @auto-finalize: When false, this job will wait in a PENDING state after 
>> it has
>>   # finished its work, waiting for @block-job-finalize before
> 
> Note that 'block-commit' and 'drive-mirror' commands are used by libvirt
> in the pre-blockdev era. In those instances we gather statistics of
> block devices by nesting in the output of query-blockstats and
> query-block rather than selecting the appropriate info by any other
> means (e.g. by node name).
> 
> This means that the output MUST stay consistend when block jobs are used
> and the hack this patch is deprcating will break those.
> 
> Note that in libvirt we don't plan to invest time to add workarounds for
> non-blockdev cases since blockdev by itself is complex enough and I'd
> strongly prefer not having a third code path to care about.
> 
> Given that -blockdev can't be used in all cases (e.g. for sd-cards)
> which also blocks deprecation of -drive I don't think that hiding of
> implicit filter nodes can be deprecated until -drive is deprecated.
> 


OK, so, we can't deprecate anything around it now.

What is the problem with sd-cards?


-- 
Best regards,
Vladimir

Re: [PATCH v3] iotests: Test NBD client reconnection

[Vladimir Sementsov-Ogievskiy]

01.11.2019 19:54, Andrey Shinkevich wrote:
> The test for an NBD client. The NBD server is disconnected after the
> client write request. The NBD client should reconnect and complete
> the write operation.
> 
> Suggested-by: Denis V. Lunev 
> Suggested-by: Vladimir Sementsov-Ogievskiy 
> Signed-off-by: Andrey Shinkevich 
> ---
>   tests/qemu-iotests/277   | 102 
> +++
>   tests/qemu-iotests/277.out   |   6 ++
>   tests/qemu-iotests/group |   1 +
>   tests/qemu-iotests/iotests.py|   5 ++
>   tests/qemu-iotests/nbd-fault-injector.py |   3 +-
>   5 files changed, 116 insertions(+), 1 deletion(-)
>   create mode 100755 tests/qemu-iotests/277
>   create mode 100644 tests/qemu-iotests/277.out
> 
> diff --git a/tests/qemu-iotests/277 b/tests/qemu-iotests/277
> new file mode 100755
> index 000..e4e6730
> --- /dev/null
> +++ b/tests/qemu-iotests/277
> @@ -0,0 +1,102 @@
> +#!/usr/bin/env python
> +#
> +# Test NBD client reconnection
> +#
> +# Copyright (c) 2019 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 .
> +#
> +
> +import os
> +import subprocess
> +import iotests
> +from iotests import file_path, log
> +
> +
> +def make_conf_file(event):
> +"""
> +Create configuration file for the nbd-fault-injector.py
> +
> +:param event: which event the server should close a connection on
> +"""
> +if os.path.exists(conf_file):
> +os.remove(conf_file)

a bit strange for my eyes to see global variable not defined higher in the file 
than used..

> +
> +with open(conf_file, "w+") as conff:
> +conff.write("[inject-error]\nevent={}\nwhen=after".format(event))

better open with "w" and don't need to remove file before it.

> +
> +
> +def start_server_NBD(event):
> +make_conf_file(event)
> +
> +srv = subprocess.Popen(["nbd-fault-injector.py", "--classic-negotiation",
> +   nbd_sock, conf_file], stdout=subprocess.PIPE,
> +   stderr=subprocess.STDOUT, universal_newlines=True)
> +line = srv.stdout.readline()
> +if "Listening on " in line:
> +log('NBD server: started')
> +else:
> +log('NBD server: ' + line.rstrip())
> +
> +return srv
> +
> +
> +def start_client_NBD():
> +log('NBD client: QEMU-IO write')
> +args = iotests.qemu_io_args_no_fmt + \
> +['-c', 'write -P 0x7 0 3M', '--image-opts',
> + 'driver=nbd,server.type=unix,server.path={},'
> + 'reconnect-delay=7'.format(nbd_sock)]
> +clt = subprocess.Popen(args, stdout=subprocess.PIPE,
> +   stderr=subprocess.STDOUT,
> +   universal_newlines=True)
> +return clt

Could you reuse QemuIoInteractive as a client?

> +
> +
> +def check_proc_NBD(proc, connector):
> +try:
> +exitcode = proc.wait(timeout=10)
> +
> +if exitcode < 0:
> +log('NBD {}: EXIT SIGNAL {}\n'.format(connector, -exitcode))
> +log(proc.communicate()[0])
> +else:
> +line = proc.stdout.readline()


could we use proc.communicate() for both cases, what is the difference?

> +log('NBD {}: {}'.format(connector, line.rstrip()))
> +
> +except subprocess.TimeoutExpired:
> +proc.kill()
> +log('NBD {}: ERROR timeout expired'.format(connector))
> +finally:
> +if connector == 'server':
> +os.remove(nbd_sock)
> +os.remove(conf_file)
> +
> +
> +conf_file = os.path.join(iotests.test_dir, "nbd-fault-injector.conf")

use file_path here too.

> +nbd_sock = file_path('nbd-sock')
> +nbd_uri = 'nbd+unix:///?socket=' + nbd_sock

unused variable

> +if os.path.exists(nbd_sock):
> +os.remove(nbd_sock)

I don't think we need this

> +
> +srv = start_server_NBD('data')
> +clt = start_client_NBD()
> +# The server should close the connection after a client write request
> +check_proc_NBD(srv, 'server')
> +# Start the NBD server again
> +srv = start_server_NBD('reply')
> +# The client should reconnect and complete the write operation
> +check_proc_NBD(clt, 'client')
> +# Make it sure that server terminated
> +check_proc_NBD(srv, 'server')
> diff --git a/tests/qemu-iotests/277.out b/tests/qemu-iotests/277.out
> new file mode 100644
> index 000..45404b3
> --- /dev/null
> +++ 

[PATCH v2 0/1] virtio: fix IO request length in virtio SCSI/block

[Denis Plotnikov]

v2:
  * the standalone patch to make seg_max virtqueue size dependent
  * other patches are postponed
  
v1:
  the initial series

Denis Plotnikov (1):
  virtio: make seg_max virtqueue size dependent

 hw/block/virtio-blk.c | 2 +-
 hw/scsi/virtio-scsi.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
2.17.0

[PATCH v2 1/1] virtio: make seg_max virtqueue size dependent

[Denis Plotnikov]

seg_max has a restriction to be less or equal to virtqueue size
according to Virtio 1.0 specification

Although seg_max can't be set directly, it's worth to express this
dependancy directly in the code for sanity purpose.

Signed-off-by: Denis Plotnikov 
---
 hw/block/virtio-blk.c | 2 +-
 hw/scsi/virtio-scsi.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 06e57a4d39..21530304cf 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -903,7 +903,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, 
uint8_t *config)
 blk_get_geometry(s->blk, );
 memset(, 0, sizeof(blkcfg));
 virtio_stq_p(vdev, , capacity);
-virtio_stl_p(vdev, _max, 128 - 2);
+virtio_stl_p(vdev, _max, s->conf.queue_size - 2);
 virtio_stw_p(vdev, , conf->cyls);
 virtio_stl_p(vdev, _size, blk_size);
 virtio_stw_p(vdev, _io_size, conf->min_io_size / blk_size);
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 839f120256..f7e5533cd5 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -650,7 +650,7 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
 VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
 
 virtio_stl_p(vdev, >num_queues, s->conf.num_queues);
-virtio_stl_p(vdev, >seg_max, 128 - 2);
+virtio_stl_p(vdev, >seg_max, s->conf.virtqueue_size - 2);
 virtio_stl_p(vdev, >max_sectors, s->conf.max_sectors);
 virtio_stl_p(vdev, >cmd_per_lun, s->conf.cmd_per_lun);
 virtio_stl_p(vdev, >event_info_size, sizeof(VirtIOSCSIEvent));
-- 
2.17.0

Re: [PATCH v2 07/11] block: add x-blockdev-amend qmp command

[Maxim Levitsky]

On Fri, 2019-11-08 at 11:36 +0100, Max Reitz wrote:
> On 08.11.19 10:26, Maxim Levitsky wrote:
> > On Fri, 2019-10-04 at 20:53 +0200, Max Reitz wrote:
> > > On 13.09.19 00:30, Maxim Levitsky wrote:
> > > > Signed-off-by: Maxim Levitsky 
> > > > ---
> > > >  block/Makefile.objs   |   2 +-
> > > >  block/amend.c | 116 ++
> > > >  include/block/block_int.h |  23 ++--
> > > >  qapi/block-core.json  |  26 +
> > > >  qapi/job.json |   4 +-
> > > >  5 files changed, 163 insertions(+), 8 deletions(-)
> > > >  create mode 100644 block/amend.c
> 
> [...]
> 
> > > > +static int coroutine_fn blockdev_amend_run(Job *job, Error **errp)
> > > > +{
> > > > +BlockdevAmendJob *s = container_of(job, BlockdevAmendJob, common);
> > > > +int ret;
> > > > +
> > > > +job_progress_set_remaining(>common, 1);
> > > > +ret = s->bs->drv->bdrv_co_amend(s->bs, s->opts, s->force, errp);
> > > > +job_progress_update(>common, 1);
> > > 
> > > It would be nice if the amend job could make use of the progress
> > > reporting that we have in place for amend.
> > 
> > I also thought about it, but is it worth it?
> > 
> > I looked through the status reporting of the qcow2 amend
> > code (which doesn't really allowed to be run through
> > qmp blockdev-amend, due to complexity of changing 
> > the qcow2 format on the fly).
> 
> True, and we could always add it later.
> 
> I suppose I was mostly wondering because bdrv_amend_options already has
> all of that infrastructure and I was assuming that qcow2's bdrv_co_amend
> implementation would make some use of the existing function.  Well, it
> doesn’t, so *shrug*
> 
> [...]
> 
> > > > +/*
> > > > + * Create the block job
> > > > + * TODO Running in the main context. Block drivers need to error 
> > > > out or add
> > > > + * locking when they use a BDS in a different AioContext.
> > > 
> > > Why shouldn’t the job just run in the node’s context?
> > 
> > This is shameless copy from the blockdev-create code
> > (which I did note in the copyright of the file)
> 
> Well, you noted that it’s heavily based on it, not that it’s just C

'heavily based' is a polite way to say that file is copied and then changed
to fit new purpose, isn't it :-)


> 
> So I suppose the comment is just wrong here?
Yes, I absolutely missed this part since I don't know the block layer well 
enough.
Thanks for explaining this on IRC, that blockdev_create job is special
in a sense that block device state doesn't exit yet for it (I also was already 
bitten
by this in luks block driver), thus the job runs in main AIO context.
No need to do so in amend, and thus I'll remove that wrongly copy'ed 
comment.

Thanks a lot!!

Best regards,
Maxim Levitsky


> 
> Max
> 

Re: [PATCH v2 1/2] qapi: add filter-node-name option to drive-mirror

[Peter Krempa]

On Fri, Nov 08, 2019 at 13:16:54 +0300, Vladimir Sementsov-Ogievskiy wrote:
> To correspond to blockdev-mirror command and make it possible to
> deprecate implicit filters in the next commit.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  qapi/block-core.json | 7 +++
>  blockdev.c   | 2 +-
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index aa97ee2641..93530d3a13 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1992,6 +1992,12 @@
>  # @on-target-error: the action to take on an error on the target,
>  #   default 'report' (no limitations, since this applies to
>  #   a different block device than @device).
> +#
> +# @filter-node-name: the node name that should be assigned to the
> +#filter driver that the mirror job inserts into the graph
> +#above @device. If this option is not given, a node name 
> is
> +#autogenerated. (Since: 4.2)
> +#

Speaking for libvirt and based on what I've responded to patch 2 there's
no value in adding this parameter for libvirt's use.

This also kind of means that drive-mirror can be deprecated together
with deprecating -drive.

Re: [RFC PATCH 05/18] qemu-storage-daemon: Add --blockdev option

[Markus Armbruster]

Kevin Wolf  writes:

> This adds a --blockdev option to the storage daemon that works the same
> as the -blockdev option of the system emulator.
>
> In order to be able to link with blockdev.o, we also need to change
> stream.o from common-obj to block-obj, which is where all other block
> jobs already are.
>
> Signed-off-by: Kevin Wolf 
> ---
>  qemu-storage-daemon.c | 29 +
>  Makefile  |  5 -
>  Makefile.objs |  7 +++
>  block/Makefile.objs   |  2 +-
>  4 files changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/qemu-storage-daemon.c b/qemu-storage-daemon.c
> index 48d6af43a6..904e3c3a46 100644
> --- a/qemu-storage-daemon.c
> +++ b/qemu-storage-daemon.c
> @@ -28,6 +28,10 @@
>  #include "crypto/init.h"
>  
>  #include "qapi/error.h"
> +#include "qapi/qapi-visit-block-core.h"
> +#include "qapi/qapi-commands-block-core.h"
> +#include "qapi/qobject-input-visitor.h"
> +
>  #include "qemu-common.h"
>  #include "qemu-version.h"
>  #include "qemu/config-file.h"
> @@ -53,6 +57,13 @@ static void help(void)
>  " specify tracing options\n"
>  "  -V, --version  output version information and exit\n"
>  "\n"
> +"  --blockdev [driver=][,node-name=][,discard=ignore|unmap]\n"
> +" [,cache.direct=on|off][,cache.no-flush=on|off]\n"
> +" [,read-only=on|off][,auto-read-only=on|off]\n"
> +" [,force-share=on|off][,detect-zeroes=on|off|unmap]\n"
> +" [,driver specific parameters...]\n"
> +" configure a block backend\n"
> +"\n"
>  "  --object   define a QOM object such as 'secret' for\n"
>  " passwords and/or encryption keys\n"
>  "\n"
> @@ -62,6 +73,7 @@ QEMU_HELP_BOTTOM "\n",
>  
>  enum {
>  OPTION_OBJECT = 256,
> +OPTION_BLOCKDEV,
>  };
>  
>  static QemuOptsList qemu_object_opts = {
> @@ -82,6 +94,7 @@ static int process_options(int argc, char *argv[], Error 
> **errp)
>  static const struct option long_options[] = {
>  {"help", no_argument, 0, 'h'},
>  {"object", required_argument, 0, OPTION_OBJECT},
> +{"blockdev", required_argument, 0, OPTION_BLOCKDEV},
>  {"version", no_argument, 0, 'V'},
>  {"trace", required_argument, NULL, 'T'},
>  {0, 0, 0, 0}
> @@ -123,6 +136,22 @@ static int process_options(int argc, char *argv[], Error 
> **errp)
>  qemu_opts_del(opts);
>  break;
>  }
> +break;
> +case OPTION_BLOCKDEV:
> +{
> +Visitor *v;
> +BlockdevOptions *options;
> +
> +v = qobject_input_visitor_new_str(optarg, "driver",
> +  _fatal);
> +
> +visit_type_BlockdevOptions(v, NULL, , _fatal);
> +visit_free(v);
> +
> +qmp_blockdev_add(options, _fatal);
> +qapi_free_BlockdevOptions(options);
> +break;
> +}
>  }
>  }
>  if (optind != argc) {

Unlike --object, --blockdev is already QAPIfied, albeit crudely.

Similar difference to vl.c as for --object: vl.c records the option for
later processing, while here you process it right away.  Simpler and
saner.

I figure you intend to do all options this way.

Should you explain the difference in a commit message?  A comment?

[...]

Re: [PATCH v2 2/2] qapi: deprecate implicit filters

[Peter Krempa]

On Fri, Nov 08, 2019 at 13:16:55 +0300, Vladimir Sementsov-Ogievskiy wrote:
> To get rid of implicit filters related workarounds in future let's
> deprecate them now.
> 
> Deprecation warning breaks some bash iotests output, so fix it here
> too: in most of cases just add filter-node-name in test.
> 
> In 161 add FIXME and deprecation warning into 161.out.
> 
> In 249, the test case is changed, as we don't need to test "default"
> filter node name anymore.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  qemu-deprecated.texi   |  6 ++
>  qapi/block-core.json   |  9 ++---
>  include/block/block_int.h  | 10 +-
>  blockdev.c | 10 ++
>  tests/qemu-iotests/094 |  1 +
>  tests/qemu-iotests/095 |  6 --
>  tests/qemu-iotests/109 |  1 +
>  tests/qemu-iotests/127 |  1 +
>  tests/qemu-iotests/141 |  5 -
>  tests/qemu-iotests/144 |  3 ++-
>  tests/qemu-iotests/156 |  1 +
>  tests/qemu-iotests/161 |  7 +++
>  tests/qemu-iotests/161.out |  1 +
>  tests/qemu-iotests/185 |  3 +++
>  tests/qemu-iotests/191 |  2 ++
>  tests/qemu-iotests/229 |  1 +
>  tests/qemu-iotests/247 |  8 +---
>  tests/qemu-iotests/249 |  5 +++--
>  tests/qemu-iotests/249.out |  2 +-
>  19 files changed, 68 insertions(+), 14 deletions(-)
> 
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 296bfc93a3..c969faf55a 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -204,6 +204,12 @@ and accurate ``query-qmp-schema'' command.
>  Character devices creating sockets in client mode should not specify
>  the 'wait' field, which is only applicable to sockets in server mode
>  
> +@subsection implicit filters in mirror and commit (since 4.2)
> +
> +Mirror and commit jobs insert filters, which becomes implicit if user
> +omitted @option(filter-node-name) parameter. So omitting it is deprecated
> +in ``blockdev-mirror'', ``drive-mirror'' and ``block-commit'', set it always.
> +
>  @section Human Monitor Protocol (HMP) commands
>  
>  @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove' (since 
> 3.1)
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 93530d3a13..37caed775f 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1659,7 +1659,8 @@
>  # @filter-node-name: the node name that should be assigned to the
>  #filter driver that the commit job inserts into the graph
>  #above @top. If this option is not given, a node name is
> -#autogenerated. (Since: 2.9)
> +#autogenerated. Omitting this option is deprecated, it 
> will
> +#be required in future. (Since: 2.9)
>  #
>  # @auto-finalize: When false, this job will wait in a PENDING state after it 
> has
>  # finished its work, waiting for @block-job-finalize before

Note that 'block-commit' and 'drive-mirror' commands are used by libvirt
in the pre-blockdev era. In those instances we gather statistics of
block devices by nesting in the output of query-blockstats and
query-block rather than selecting the appropriate info by any other
means (e.g. by node name).

This means that the output MUST stay consistend when block jobs are used
and the hack this patch is deprcating will break those.

Note that in libvirt we don't plan to invest time to add workarounds for
non-blockdev cases since blockdev by itself is complex enough and I'd
strongly prefer not having a third code path to care about.

Given that -blockdev can't be used in all cases (e.g. for sd-cards)
which also blocks deprecation of -drive I don't think that hiding of
implicit filter nodes can be deprecated until -drive is deprecated.

Re: [PATCH v2 05/11] block/crypto: implement the encryption key management

[Maxim Levitsky]

On Fri, 2019-11-08 at 14:12 +0100, Max Reitz wrote:
> On 08.11.19 12:04, Maxim Levitsky wrote:
> > On Fri, 2019-11-08 at 11:49 +0100, Max Reitz wrote:
> > > On 08.11.19 10:30, Maxim Levitsky wrote:
> > > > On Fri, 2019-10-04 at 20:41 +0200, Max Reitz wrote:
> > > > > On 13.09.19 00:30, Maxim Levitsky wrote:
> > > > > > This implements the encryption key management
> > > > > > using the generic code in qcrypto layer
> > > > > > (currently only for qemu-img amend)
> > > > > > 
> > > > > > This code adds another 'write_func' because the initialization
> > > > > > write_func works directly on the underlying file,
> > > > > > because during the creation, there is no open instance
> > > > > > of the luks driver, but during regular use, we have it,
> > > > > > and should use it instead.
> > > > > > 
> > > > > > 
> > > > > > This commit also adds a 'hack/workaround' I and Kevin Wolf 
> > > > > > (thanks)
> > > > > > made to make the driver still support write sharing,
> > > > > > but be safe against concurrent  metadata update (the keys)
> > > > > > Eventually write sharing for luks driver will be deprecated
> > > > > > and removed together with this hack.
> > > > > > 
> > > > > > The hack is that we ask (as a format driver) for
> > > > > > BLK_PERM_CONSISTENT_READ always
> > > > > > (technically always unless opened with BDRV_O_NO_IO)
> > > > > > 
> > > > > > and then when we want to update the keys, we
> > > > > > unshare that permission. So if someone else
> > > > > > has the image open, even readonly, this will fail.
> > > > > > 
> > > > > > Also thanks to Daniel Berrange for the variant of
> > > > > > that hack that involves asking for read,
> > > > > > rather that write permission
> > > > > > 
> > > > > > Signed-off-by: Maxim Levitsky 
> > > > > > ---
> > > > > >  block/crypto.c | 118 
> > > > > > +++--
> > > > > >  1 file changed, 115 insertions(+), 3 deletions(-)
> > > > > > 
> > > > > > diff --git a/block/crypto.c b/block/crypto.c
> > > > > > index a6a3e1f1d8..f42fa057e6 100644
> > > > > > --- a/block/crypto.c
> > > > > > +++ b/block/crypto.c
> > > > > > @@ -36,6 +36,7 @@ typedef struct BlockCrypto BlockCrypto;
> > > > > >  
> > > > > >  struct BlockCrypto {
> > > > > >  QCryptoBlock *block;
> > > > > > +bool updating_keys;
> > > > > >  };
> > > > > >  
> > > > > >  
> > > > > > @@ -70,6 +71,24 @@ static ssize_t 
> > > > > > block_crypto_read_func(QCryptoBlock *block,
> > > > > >  return ret;
> > > > > >  }
> > > > > >  
> > > > > > +static ssize_t block_crypto_write_func(QCryptoBlock *block,
> > > > > > +   size_t offset,
> > > > > > +   const uint8_t *buf,
> > > > > > +   size_t buflen,
> > > > > > +   void *opaque,
> > > > > > +   Error **errp)
> > > > > 
> > > > > There’s already a function of this name for creation.
> > > > 
> > > > There is a long story why two write functions are needed.
> > > > i tried to use only one, but at the end I and Daniel both agreed
> > > > that its just better to have two functions.
> > > > 
> > > > The reason is that during creation, the luks BlockDriverState doesn't 
> > > > exist yet,
> > > > and so the creation routine basically just writes to the underlying 
> > > > protocol driver.
> > > > 
> > > > Thats is why the block_crypto_create_write_func receives a BlockBackend 
> > > > pointer,
> > > > to which the BlockDriverState of the underlying protocol driver is 
> > > > inserted.
> > > > 
> > > > 
> > > > On the other hand, for amend, the luks block device is open, and it 
> > > > only knows
> > > > about its own BlockDriverState, and thus the io should be done on 
> > > > bs->file
> > > > 
> > > > So instead of trying to coerce a single callback to do both of this,
> > > > we decided to just have a little code duplication.
> > > 
> > > I meant: This doesn’t compile.  There’s already another function of this
> > > name.
> > > 
> > 
> > You probably didn't apply the 'block-crypto: misc refactoring' patch, 
> > or I forgot to send it.
> 
> Maybe you forgot to mention anywhere that I should.

Now I remember that this patch was in the original re-factoring patch series,
and for some reason it was dropped.
Its now locally in my git tree, so I forgot that it wasn't in this patch series
already.

Sorry for the mess, I'll soon resend the updated patch series.

Best regards,
Maxim Levitsky

Re: [PATCH v2 05/11] block/crypto: implement the encryption key management

[Max Reitz]

On 08.11.19 12:04, Maxim Levitsky wrote:
> On Fri, 2019-11-08 at 11:49 +0100, Max Reitz wrote:
>> On 08.11.19 10:30, Maxim Levitsky wrote:
>>> On Fri, 2019-10-04 at 20:41 +0200, Max Reitz wrote:
 On 13.09.19 00:30, Maxim Levitsky wrote:
> This implements the encryption key management
> using the generic code in qcrypto layer
> (currently only for qemu-img amend)
>
> This code adds another 'write_func' because the initialization
> write_func works directly on the underlying file,
> because during the creation, there is no open instance
> of the luks driver, but during regular use, we have it,
> and should use it instead.
>
>
> This commit also adds a   'hack/workaround' I and Kevin Wolf (thanks)
> made to   make the driver still support write sharing,
> but be safe against concurrent  metadata update (the keys)
> Eventually write sharing for luks driver will be deprecated
> and removed together with this hack.
>
> The hack is that we ask   (as a format driver) for
> BLK_PERM_CONSISTENT_READ always
> (technically always unless opened with BDRV_O_NO_IO)
>
> and then when we want to update   the keys, we
> unshare   that permission. So if someone else
> has the   image open, even readonly, this will fail.
>
> Also thanks to Daniel Berrange for the variant of
> that hack that involves   asking for read,
> rather that write permission
>
> Signed-off-by: Maxim Levitsky 
> ---
>  block/crypto.c | 118 +++--
>  1 file changed, 115 insertions(+), 3 deletions(-)
>
> diff --git a/block/crypto.c b/block/crypto.c
> index a6a3e1f1d8..f42fa057e6 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -36,6 +36,7 @@ typedef struct BlockCrypto BlockCrypto;
>  
>  struct BlockCrypto {
>  QCryptoBlock *block;
> +bool updating_keys;
>  };
>  
>  
> @@ -70,6 +71,24 @@ static ssize_t block_crypto_read_func(QCryptoBlock 
> *block,
>  return ret;
>  }
>  
> +static ssize_t block_crypto_write_func(QCryptoBlock *block,
> +   size_t offset,
> +   const uint8_t *buf,
> +   size_t buflen,
> +   void *opaque,
> +   Error **errp)

 There’s already a function of this name for creation.
>>>
>>> There is a long story why two write functions are needed.
>>> i tried to use only one, but at the end I and Daniel both agreed
>>> that its just better to have two functions.
>>>
>>> The reason is that during creation, the luks BlockDriverState doesn't exist 
>>> yet,
>>> and so the creation routine basically just writes to the underlying 
>>> protocol driver.
>>>
>>> Thats is why the block_crypto_create_write_func receives a BlockBackend 
>>> pointer,
>>> to which the BlockDriverState of the underlying protocol driver is inserted.
>>>
>>>
>>> On the other hand, for amend, the luks block device is open, and it only 
>>> knows
>>> about its own BlockDriverState, and thus the io should be done on bs->file
>>>
>>> So instead of trying to coerce a single callback to do both of this,
>>> we decided to just have a little code duplication.
>>
>> I meant: This doesn’t compile.  There’s already another function of this
>> name.
>>
> 
> You probably didn't apply the 'block-crypto: misc refactoring' patch, 
> or I forgot to send it.

Maybe you forgot to mention anywhere that I should.

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v2 0/2] Deprecate implicit filters

[Maxim Levitsky]

On Fri, 2019-11-08 at 13:16 +0300, Vladimir Sementsov-Ogievskiy wrote:
> v2:
> Don't deprecate drive-backup, it is unrelated thing and will be resent
> in separate.
> Don't deprecate drive-mirror. Instead add filter-node-name to
> drive-mirror to behave like blockdev-mirror
> Fix all broken iotests.

I did a quick overview of these patches (I don't know the area well
to do a full review) and it looks fine to me, other than that FIXME you added,
which (at least looking at the explanation) I think should be investigated,
as it might point to a deeper problem somewhere.

Also *I think* that I would merge these two patches together,
but this is only my personal taste.

Best regards,
Maxim Levitsky


> 
> Vladimir Sementsov-Ogievskiy (2):
>   qapi: add filter-node-name option to drive-mirror
>   qapi: deprecate implicit filters
> 
>  qemu-deprecated.texi   |  6 ++
>  qapi/block-core.json   | 14 --
>  include/block/block_int.h  | 10 +-
>  blockdev.c | 12 +++-
>  tests/qemu-iotests/094 |  1 +
>  tests/qemu-iotests/095 |  6 --
>  tests/qemu-iotests/109 |  1 +
>  tests/qemu-iotests/127 |  1 +
>  tests/qemu-iotests/141 |  5 -
>  tests/qemu-iotests/144 |  3 ++-
>  tests/qemu-iotests/156 |  1 +
>  tests/qemu-iotests/161 |  7 +++
>  tests/qemu-iotests/161.out |  1 +
>  tests/qemu-iotests/185 |  3 +++
>  tests/qemu-iotests/191 |  2 ++
>  tests/qemu-iotests/229 |  1 +
>  tests/qemu-iotests/247 |  8 +---
>  tests/qemu-iotests/249 |  5 +++--
>  tests/qemu-iotests/249.out |  2 +-
>  19 files changed, 75 insertions(+), 14 deletions(-)
> 

[PATCH for-5.0 v4 5/5] iotests: Add test for failing mirror complete

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Tested-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
---
 tests/qemu-iotests/041 | 44 ++
 tests/qemu-iotests/041.out |  4 ++--
 2 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 8568426311..d7be30b62b 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -1121,6 +1121,50 @@ class TestOrphanedSource(iotests.QMPTestCase):
  target='dest-ro')
 self.assert_qmp(result, 'error/class', 'GenericError')
 
+def test_failing_permission_in_complete(self):
+self.assert_no_active_block_jobs()
+
+# Unshare consistent-read on the target
+# (The mirror job does not care)
+result = self.vm.qmp('blockdev-add',
+ driver='blkdebug',
+ node_name='dest-perm',
+ image='dest',
+ unshare_child_perms=['consistent-read'])
+self.assert_qmp(result, 'return', {})
+
+result = self.vm.qmp('blockdev-mirror', job_id='job', device='src',
+ sync='full', target='dest',
+ filter_node_name='mirror-filter')
+self.assert_qmp(result, 'return', {})
+
+# Require consistent-read on the source
+# (We can only add this node once the job has started, or it
+# will complain that it does not want to run on non-root nodes)
+result = self.vm.qmp('blockdev-add',
+ driver='blkdebug',
+ node_name='src-perm',
+ image='src',
+ take_child_perms=['consistent-read'])
+self.assert_qmp(result, 'return', {})
+
+# While completing, mirror will attempt to replace src by
+# dest, which must fail because src-perm requires
+# consistent-read but dest-perm does not share it; thus
+# aborting the job when it is supposed to complete
+self.complete_and_wait('job',
+   completion_error='Operation not permitted')
+
+# Assert that all of our nodes are still there (except for the
+# mirror filter, which should be gone despite the failure)
+nodes = self.vm.qmp('query-named-block-nodes')['return']
+nodes = [node['node-name'] for node in nodes]
+
+for expect in ('src', 'src-perm', 'dest', 'dest-perm'):
+self.assertTrue(expect in nodes, '%s disappeared' % expect)
+self.assertFalse('mirror-filter' in nodes,
+ 'Mirror filter node did not disappear')
+
 if __name__ == '__main__':
 iotests.main(supported_fmts=['qcow2', 'qed'],
  supported_protocols=['file'])
diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out
index 2c448b4239..f496be9197 100644
--- a/tests/qemu-iotests/041.out
+++ b/tests/qemu-iotests/041.out
@@ -1,5 +1,5 @@
-..
+...
 --
-Ran 90 tests
+Ran 91 tests
 
 OK
-- 
2.23.0

[PATCH for-5.0 v4 1/5] block: Add bdrv_qapi_perm_to_blk_perm()

[Max Reitz]

We need some way to correlate QAPI BlockPermission values with
BLK_PERM_* flags.  We could:

(1) have the same order in the QAPI definition as the the BLK_PERM_*
flags are in LSb-first order.  However, then there is no guarantee
that they actually match (e.g. when someone modifies the QAPI schema
without thinking of the BLK_PERM_* definitions).
We could add static assertions, but these would break what’s good
about this solution, namely its simplicity.

(2) define the BLK_PERM_* flags based on the BlockPermission values.
But this way whenever someone were to modify the QAPI order
(perfectly sensible in theory), the BLK_PERM_* values would change.
Because these values are used for file locking, this might break
file locking between different qemu versions.

Therefore, go the slightly more cumbersome way: Add a function to
translate from the QAPI constants to the BLK_PERM_* flags.

Signed-off-by: Max Reitz 
---
 block.c   | 18 ++
 include/block/block.h |  1 +
 2 files changed, 19 insertions(+)

diff --git a/block.c b/block.c
index 4cffc2bc35..066433f3e2 100644
--- a/block.c
+++ b/block.c
@@ -2227,6 +2227,24 @@ void bdrv_format_default_perms(BlockDriverState *bs, 
BdrvChild *c,
 *nshared = shared;
 }
 
+uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm)
+{
+static const uint64_t permissions[] = {
+[BLOCK_PERMISSION_CONSISTENT_READ]  = BLK_PERM_CONSISTENT_READ,
+[BLOCK_PERMISSION_WRITE]= BLK_PERM_WRITE,
+[BLOCK_PERMISSION_WRITE_UNCHANGED]  = BLK_PERM_WRITE_UNCHANGED,
+[BLOCK_PERMISSION_RESIZE]   = BLK_PERM_RESIZE,
+[BLOCK_PERMISSION_GRAPH_MOD]= BLK_PERM_GRAPH_MOD,
+};
+
+QEMU_BUILD_BUG_ON(ARRAY_SIZE(permissions) != BLOCK_PERMISSION__MAX);
+QEMU_BUILD_BUG_ON(1UL << ARRAY_SIZE(permissions) != BLK_PERM_ALL + 1);
+
+assert(qapi_perm < BLOCK_PERMISSION__MAX);
+
+return permissions[qapi_perm];
+}
+
 static void bdrv_replace_child_noperm(BdrvChild *child,
   BlockDriverState *new_bs)
 {
diff --git a/include/block/block.h b/include/block/block.h
index 1df9848e74..e9dcfef7fa 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -280,6 +280,7 @@ enum {
 };
 
 char *bdrv_perm_names(uint64_t perm);
+uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm);
 
 /* disk I/O throttling */
 void bdrv_init(void);
-- 
2.23.0

[PATCH for-5.0 v4 3/5] blkdebug: Allow taking/unsharing permissions

[Max Reitz]

Sometimes it is useful to be able to add a node to the block graph that
takes or unshare a certain set of permissions for debugging purposes.
This patch adds this capability to blkdebug.

(Note that you cannot make blkdebug release or share permissions that it
needs to take or cannot share, because this might result in assertion
failures in the block layer.  But if the blkdebug node has no parents,
it will not take any permissions and share everything by default, so you
can then freely choose what permissions to take and share.)

Signed-off-by: Max Reitz 
---
 block/blkdebug.c | 93 +++-
 qapi/block-core.json | 14 ++-
 2 files changed, 105 insertions(+), 2 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 5ae96c52b0..af44aa973f 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -28,10 +28,14 @@
 #include "qemu/cutils.h"
 #include "qemu/config-file.h"
 #include "block/block_int.h"
+#include "block/qdict.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
+#include "qapi/qapi-visit-block-core.h"
 #include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qlist.h"
 #include "qapi/qmp/qstring.h"
+#include "qapi/qobject-input-visitor.h"
 #include "sysemu/qtest.h"
 
 typedef struct BDRVBlkdebugState {
@@ -44,6 +48,9 @@ typedef struct BDRVBlkdebugState {
 uint64_t opt_discard;
 uint64_t max_discard;
 
+uint64_t take_child_perms;
+uint64_t unshare_child_perms;
+
 /* For blkdebug_refresh_filename() */
 char *config_file;
 
@@ -344,6 +351,69 @@ static void blkdebug_parse_filename(const char *filename, 
QDict *options,
 qdict_put_str(options, "x-image", filename);
 }
 
+static int blkdebug_parse_perm_list(uint64_t *dest, QDict *options,
+const char *prefix, Error **errp)
+{
+int ret = 0;
+QDict *subqdict = NULL;
+QObject *crumpled_subqdict = NULL;
+Visitor *v = NULL;
+BlockPermissionList *perm_list = NULL, *element;
+Error *local_err = NULL;
+
+*dest = 0;
+
+qdict_extract_subqdict(options, , prefix);
+if (!qdict_size(subqdict)) {
+goto out;
+}
+
+crumpled_subqdict = qdict_crumple(subqdict, errp);
+if (!crumpled_subqdict) {
+ret = -EINVAL;
+goto out;
+}
+
+v = qobject_input_visitor_new(crumpled_subqdict);
+visit_type_BlockPermissionList(v, NULL, _list, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+ret = -EINVAL;
+goto out;
+}
+
+for (element = perm_list; element; element = element->next) {
+*dest |= bdrv_qapi_perm_to_blk_perm(element->value);
+}
+
+out:
+qapi_free_BlockPermissionList(perm_list);
+visit_free(v);
+qobject_unref(subqdict);
+qobject_unref(crumpled_subqdict);
+return ret;
+}
+
+static int blkdebug_parse_perms(BDRVBlkdebugState *s, QDict *options,
+Error **errp)
+{
+int ret;
+
+ret = blkdebug_parse_perm_list(>take_child_perms, options,
+   "take-child-perms.", errp);
+if (ret < 0) {
+return ret;
+}
+
+ret = blkdebug_parse_perm_list(>unshare_child_perms, options,
+   "unshare-child-perms.", errp);
+if (ret < 0) {
+return ret;
+}
+
+return 0;
+}
+
 static QemuOptsList runtime_opts = {
 .name = "blkdebug",
 .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
@@ -419,6 +489,12 @@ static int blkdebug_open(BlockDriverState *bs, QDict 
*options, int flags,
 /* Set initial state */
 s->state = 1;
 
+/* Parse permissions modifiers before opening the image file */
+ret = blkdebug_parse_perms(s, options, errp);
+if (ret < 0) {
+goto out;
+}
+
 /* Open the image file */
 bs->file = bdrv_open_child(qemu_opt_get(opts, "x-image"), options, "image",
bs, _file, false, _err);
@@ -916,6 +992,21 @@ static int blkdebug_reopen_prepare(BDRVReopenState 
*reopen_state,
 return 0;
 }
 
+static void blkdebug_child_perm(BlockDriverState *bs, BdrvChild *c,
+const BdrvChildRole *role,
+BlockReopenQueue *reopen_queue,
+uint64_t perm, uint64_t shared,
+uint64_t *nperm, uint64_t *nshared)
+{
+BDRVBlkdebugState *s = bs->opaque;
+
+bdrv_filter_default_perms(bs, c, role, reopen_queue, perm, shared,
+  nperm, nshared);
+
+*nperm |= s->take_child_perms;
+*nshared &= ~s->unshare_child_perms;
+}
+
 static const char *const blkdebug_strong_runtime_opts[] = {
 "config",
 "inject-error.",
@@ -940,7 +1031,7 @@ static BlockDriver bdrv_blkdebug = {
 .bdrv_file_open = blkdebug_open,
 .bdrv_close = blkdebug_close,
 .bdrv_reopen_prepare= blkdebug_reopen_prepare,
-.bdrv_child_perm= bdrv_filter_default_perms,
+

[PATCH for-5.0 v4 0/5] iotests: Test failing mirror complete

[Max Reitz]

Hi,

v3 of this series was this:

https://lists.nongnu.org/archive/html/qemu-block/2019-10/msg00868.html

In the meantime, I’ve merged the first patch, so the subject of the
series has changed.

In v4, I’ve tried to address Vladimir’s concern of how to map QAPI
BlockPermission values to BLK_PERM_* flags with the new patches 1 and 2,
and a minor change to patch 3:
- Patch 1: Added, this new function will translate QAPI BlockPermission
   values to BLK_PERM_* flags
- Patch 2: Added; we can make use of the new function in
   xdbg_graph_add_edge() to save some LoC
- Patch 3:
  - %s/4.2/5.0/
  - Let blkdebug_parse_perm_list() initialize *dest
  - Use bdrv_qapi_perm_to_blk_perm() to translate QAPI BlockPermission
values instead of blindly assuming an x -> 2^x mapping


git-backport-diff against v3:

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

001/5:[down] 'block: Add bdrv_qapi_perm_to_blk_perm()'
002/5:[down] 'block: Use bdrv_qapi_perm_to_blk_perm()'
003/5:[0008] [FC] 'blkdebug: Allow taking/unsharing permissions'
004/5:[] [--] 'iotests: Add @error to wait_until_completed'
005/5:[] [--] 'iotests: Add test for failing mirror complete'


Max Reitz (5):
  block: Add bdrv_qapi_perm_to_blk_perm()
  block: Use bdrv_qapi_perm_to_blk_perm()
  blkdebug: Allow taking/unsharing permissions
  iotests: Add @error to wait_until_completed
  iotests: Add test for failing mirror complete

 block.c   | 47 ++
 block/blkdebug.c  | 93 ++-
 include/block/block.h |  1 +
 qapi/block-core.json  | 14 +-
 tests/qemu-iotests/041| 44 +
 tests/qemu-iotests/041.out|  4 +-
 tests/qemu-iotests/iotests.py | 18 ---
 7 files changed, 190 insertions(+), 31 deletions(-)

-- 
2.23.0

[PATCH for-5.0 v4 2/5] block: Use bdrv_qapi_perm_to_blk_perm()

[Max Reitz]

We can save some LoC in xdbg_graph_add_edge() by using
bdrv_qapi_perm_to_blk_perm().

Signed-off-by: Max Reitz 
---
 block.c | 29 -
 1 file changed, 8 insertions(+), 21 deletions(-)

diff --git a/block.c b/block.c
index 066433f3e2..ae279ff21f 100644
--- a/block.c
+++ b/block.c
@@ -4870,36 +4870,23 @@ static void 
xdbg_graph_add_node(XDbgBlockGraphConstructor *gr, void *node,
 static void xdbg_graph_add_edge(XDbgBlockGraphConstructor *gr, void *parent,
 const BdrvChild *child)
 {
-typedef struct {
-unsigned int flag;
-BlockPermission num;
-} PermissionMap;
-
-static const PermissionMap permissions[] = {
-{ BLK_PERM_CONSISTENT_READ, BLOCK_PERMISSION_CONSISTENT_READ },
-{ BLK_PERM_WRITE,   BLOCK_PERMISSION_WRITE },
-{ BLK_PERM_WRITE_UNCHANGED, BLOCK_PERMISSION_WRITE_UNCHANGED },
-{ BLK_PERM_RESIZE,  BLOCK_PERMISSION_RESIZE },
-{ BLK_PERM_GRAPH_MOD,   BLOCK_PERMISSION_GRAPH_MOD },
-{ 0, 0 }
-};
-const PermissionMap *p;
+BlockPermission qapi_perm;
 XDbgBlockGraphEdge *edge;
 
-QEMU_BUILD_BUG_ON(1UL << (ARRAY_SIZE(permissions) - 1) != BLK_PERM_ALL + 
1);
-
 edge = g_new0(XDbgBlockGraphEdge, 1);
 
 edge->parent = xdbg_graph_node_num(gr, parent);
 edge->child = xdbg_graph_node_num(gr, child->bs);
 edge->name = g_strdup(child->name);
 
-for (p = permissions; p->flag; p++) {
-if (p->flag & child->perm) {
-QAPI_LIST_ADD(edge->perm, p->num);
+for (qapi_perm = 0; qapi_perm < BLOCK_PERMISSION__MAX; qapi_perm++) {
+uint64_t flag = bdrv_qapi_perm_to_blk_perm(qapi_perm);
+
+if (flag & child->perm) {
+QAPI_LIST_ADD(edge->perm, qapi_perm);
 }
-if (p->flag & child->shared_perm) {
-QAPI_LIST_ADD(edge->shared_perm, p->num);
+if (flag & child->shared_perm) {
+QAPI_LIST_ADD(edge->shared_perm, qapi_perm);
 }
 }
 
-- 
2.23.0

[PATCH for-5.0 v4 4/5] iotests: Add @error to wait_until_completed

[Max Reitz]

Callers can use this new parameter to expect failure during the
completion process.

Signed-off-by: Max Reitz 
Reviewed-by: John Snow 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/iotests.py | 18 --
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 075f4739da..2010b71d4b 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -783,15 +783,20 @@ class QMPTestCase(unittest.TestCase):
 self.assert_no_active_block_jobs()
 return result
 
-def wait_until_completed(self, drive='drive0', check_offset=True, 
wait=60.0):
+def wait_until_completed(self, drive='drive0', check_offset=True, 
wait=60.0,
+ error=None):
 '''Wait for a block job to finish, returning the event'''
 while True:
 for event in self.vm.get_qmp_events(wait=wait):
 if event['event'] == 'BLOCK_JOB_COMPLETED':
 self.assert_qmp(event, 'data/device', drive)
-self.assert_qmp_absent(event, 'data/error')
-if check_offset:
-self.assert_qmp(event, 'data/offset', 
event['data']['len'])
+if error is None:
+self.assert_qmp_absent(event, 'data/error')
+if check_offset:
+self.assert_qmp(event, 'data/offset',
+event['data']['len'])
+else:
+self.assert_qmp(event, 'data/error', error)
 self.assert_no_active_block_jobs()
 return event
 elif event['event'] == 'JOB_STATUS_CHANGE':
@@ -809,7 +814,8 @@ class QMPTestCase(unittest.TestCase):
 self.assert_qmp(event, 'data/type', 'mirror')
 self.assert_qmp(event, 'data/offset', event['data']['len'])
 
-def complete_and_wait(self, drive='drive0', wait_ready=True):
+def complete_and_wait(self, drive='drive0', wait_ready=True,
+  completion_error=None):
 '''Complete a block job and wait for it to finish'''
 if wait_ready:
 self.wait_ready(drive=drive)
@@ -817,7 +823,7 @@ class QMPTestCase(unittest.TestCase):
 result = self.vm.qmp('block-job-complete', device=drive)
 self.assert_qmp(result, 'return', {})
 
-event = self.wait_until_completed(drive=drive)
+event = self.wait_until_completed(drive=drive, error=completion_error)
 self.assert_qmp(event, 'data/type', 'mirror')
 
 def pause_wait(self, job_id='job0'):
-- 
2.23.0

Re: [PATCH v2 0/2] Deprecate implicit filters

[Vladimir Sementsov-Ogievskiy]

Something strange, I don't think it related to patchset.

08.11.2019 15:00, no-re...@patchew.org wrote:
> Patchew URL: 
> https://patchew.org/QEMU/20191108101655.10611-1-vsement...@virtuozzo.com/
> 
> 
> 
> Hi,
> 
> This series failed the docker-mingw@fedora 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-mingw@fedora J=14 NETWORK=1
> === TEST SCRIPT END ===
> 
>SIGNpc-bios/optionrom/pvh.bin
>GEN docs/interop/qemu-ga-ref.7
> /tmp/qemu-test/src/qemu-deprecated.texi:210: @option expected braces
> make: *** [Makefile:994: qemu-doc.txt] Error 1
> make: *** Waiting for unfinished jobs
> /tmp/qemu-test/src/qemu-deprecated.texi:210: @option expected braces
> make: *** [Makefile:987: qemu-doc.html] Error 1
> Traceback (most recent call last):
>File "./tests/docker/docker.py", line 662, in 
>  sys.exit(main())
> ---
>  raise CalledProcessError(retcode, cmd)
> subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', 
> '--label', 'com.qemu.instance.uuid=3d404482279f4fe68fcb2c9971f2480a', '-u', 
> '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', 
> '-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-mlpokwtg/src/docker-src.2019-11-08-06.56.33.23805:/var/tmp/qemu:z,ro',
>  'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit 
> status 2.
> filter=--filter=label=com.qemu.instance.uuid=3d404482279f4fe68fcb2c9971f2480a
> make[1]: *** [docker-run] Error 1
> make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-mlpokwtg/src'
> make: *** [docker-run-test-mingw@fedora] Error 2
> 
> real4m11.170s
> user0m4.404s
> 
> 
> The full log is available at
> http://patchew.org/logs/20191108101655.10611-1-vsement...@virtuozzo.com/testing.docker-mingw@fedora/?type=message.
> ---
> Email generated automatically by Patchew [https://patchew.org/].
> Please send your feedback to patchew-de...@redhat.com
> 


-- 
Best regards,
Vladimir

Re: [PATCH v2 0/2] Deprecate implicit filters

[no-reply]

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



Hi,

This series failed the docker-mingw@fedora 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-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

  SIGNpc-bios/optionrom/pvh.bin
  GEN docs/interop/qemu-ga-ref.7
/tmp/qemu-test/src/qemu-deprecated.texi:210: @option expected braces
make: *** [Makefile:994: qemu-doc.txt] Error 1
make: *** Waiting for unfinished jobs
/tmp/qemu-test/src/qemu-deprecated.texi:210: @option expected braces
make: *** [Makefile:987: qemu-doc.html] Error 1
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 662, in 
sys.exit(main())
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', 
'--label', 'com.qemu.instance.uuid=3d404482279f4fe68fcb2c9971f2480a', '-u', 
'1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', 
'-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-mlpokwtg/src/docker-src.2019-11-08-06.56.33.23805:/var/tmp/qemu:z,ro',
 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit 
status 2.
filter=--filter=label=com.qemu.instance.uuid=3d404482279f4fe68fcb2c9971f2480a
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-mlpokwtg/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real4m11.170s
user0m4.404s


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

Re: [PATCH v2 02/11] qcrypto-luks: extend the create options for upcoming encryption key management

[Maxim Levitsky]

On Fri, 2019-11-08 at 11:48 +0100, Max Reitz wrote:
> On 08.11.19 10:28, Maxim Levitsky wrote:
> > On Fri, 2019-10-04 at 19:42 +0200, Max Reitz wrote:
> > > On 13.09.19 00:30, Maxim Levitsky wrote:
> > > > Now you can specify which slot to put the encryption key to
> > > > Plus add 'active' option which will let  user erase the key secret
> > > > instead of adding it.
> > > > Check that active=true it when creating.
> > > > 
> > > > Signed-off-by: Maxim Levitsky 
> > > > ---
> > > >  block/crypto.c |  2 ++
> > > >  block/crypto.h | 16 +++
> > > >  block/qcow2.c  |  2 ++
> > > >  crypto/block-luks.c| 26 +++---
> > > >  qapi/crypto.json   | 19 ++
> > > >  tests/qemu-iotests/082.out | 54 ++
> > > >  6 files changed, 115 insertions(+), 4 deletions(-)
> > > 
> > > (Just doing a cursory RFC-style review)
> > > 
> > > I think we also want to reject unlock-secret if it’s given for creation;
> > 
> > Agree, I'll do this in the next version.
> > 
> > > and I suppose it’d be more important to print which slots are OK than
> > > the slot the user has given.  (It isn’t like we shouldn’t print that
> > > slot index, but it’s more likely the user knows that than what the
> > > limits are.  I think.)
> > 
> > I don't really understand what you mean here :-( 
> > 
> > Since this is qmp interface,
> > I can't really print anything from it, other that error messages.
> 
> Exactly, I’m referring to the error message.  Right now it’s:
> 
> "Invalid slot %" PRId64 " is specified", luks_opts.slot
> 
> I think it should be something like:
> 
> "Invalid slot %" PRId64 " specified, must be between 0 and %u",
> luks_opt.slot, QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS - 1

This is a very good idea! implemented now and will
post in the next version.

Best regards,
Maxim Levitsky

Re: [PATCH] iotests: Fix "no qualified output" error path

[no-reply]

Patchew URL: https://patchew.org/QEMU/20191108085713.27551-1-kw...@redhat.com/



Hi,

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

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

  TESTiotest-qcow2: 268
Failures: 192
Failed 1 of 108 iotests
make: *** [check-tests/check-block.sh] Error 1
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 662, in 
sys.exit(main())
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', 
'--label', 'com.qemu.instance.uuid=414a771dbd814a1183fd7732b0c1f2c5', '-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-g2muu7fm/src/docker-src.2019-11-08-06.10.38.10992:/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=414a771dbd814a1183fd7732b0c1f2c5
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-g2muu7fm/src'
make: *** [docker-run-test-quick@centos7] Error 2

real13m23.714s
user0m9.033s


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

Re: [PATCH v2 05/11] block/crypto: implement the encryption key management

[Maxim Levitsky]

On Fri, 2019-11-08 at 11:49 +0100, Max Reitz wrote:
> On 08.11.19 10:30, Maxim Levitsky wrote:
> > On Fri, 2019-10-04 at 20:41 +0200, Max Reitz wrote:
> > > On 13.09.19 00:30, Maxim Levitsky wrote:
> > > > This implements the encryption key management
> > > > using the generic code in qcrypto layer
> > > > (currently only for qemu-img amend)
> > > > 
> > > > This code adds another 'write_func' because the initialization
> > > > write_func works directly on the underlying file,
> > > > because during the creation, there is no open instance
> > > > of the luks driver, but during regular use, we have it,
> > > > and should use it instead.
> > > > 
> > > > 
> > > > This commit also adds a 'hack/workaround' I and Kevin Wolf (thanks)
> > > > made to make the driver still support write sharing,
> > > > but be safe against concurrent  metadata update (the keys)
> > > > Eventually write sharing for luks driver will be deprecated
> > > > and removed together with this hack.
> > > > 
> > > > The hack is that we ask (as a format driver) for
> > > > BLK_PERM_CONSISTENT_READ always
> > > > (technically always unless opened with BDRV_O_NO_IO)
> > > > 
> > > > and then when we want to update the keys, we
> > > > unshare that permission. So if someone else
> > > > has the image open, even readonly, this will fail.
> > > > 
> > > > Also thanks to Daniel Berrange for the variant of
> > > > that hack that involves asking for read,
> > > > rather that write permission
> > > > 
> > > > Signed-off-by: Maxim Levitsky 
> > > > ---
> > > >  block/crypto.c | 118 +++--
> > > >  1 file changed, 115 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/block/crypto.c b/block/crypto.c
> > > > index a6a3e1f1d8..f42fa057e6 100644
> > > > --- a/block/crypto.c
> > > > +++ b/block/crypto.c
> > > > @@ -36,6 +36,7 @@ typedef struct BlockCrypto BlockCrypto;
> > > >  
> > > >  struct BlockCrypto {
> > > >  QCryptoBlock *block;
> > > > +bool updating_keys;
> > > >  };
> > > >  
> > > >  
> > > > @@ -70,6 +71,24 @@ static ssize_t block_crypto_read_func(QCryptoBlock 
> > > > *block,
> > > >  return ret;
> > > >  }
> > > >  
> > > > +static ssize_t block_crypto_write_func(QCryptoBlock *block,
> > > > +   size_t offset,
> > > > +   const uint8_t *buf,
> > > > +   size_t buflen,
> > > > +   void *opaque,
> > > > +   Error **errp)
> > > 
> > > There’s already a function of this name for creation.
> > 
> > There is a long story why two write functions are needed.
> > i tried to use only one, but at the end I and Daniel both agreed
> > that its just better to have two functions.
> > 
> > The reason is that during creation, the luks BlockDriverState doesn't exist 
> > yet,
> > and so the creation routine basically just writes to the underlying 
> > protocol driver.
> > 
> > Thats is why the block_crypto_create_write_func receives a BlockBackend 
> > pointer,
> > to which the BlockDriverState of the underlying protocol driver is inserted.
> > 
> > 
> > On the other hand, for amend, the luks block device is open, and it only 
> > knows
> > about its own BlockDriverState, and thus the io should be done on bs->file
> > 
> > So instead of trying to coerce a single callback to do both of this,
> > we decided to just have a little code duplication.
> 
> I meant: This doesn’t compile.  There’s already another function of this
> name.
> 

You probably didn't apply the 'block-crypto: misc refactoring' patch, 
or I forgot to send it.
All that patch does is to rename block_crypto_write_func to 
block_crypto_create_write_func
and same (for consistency) for block_crypto_init_func -> 
block_crypto_create_init_func

And then in this patch I add the block_crypto_write_func, to be used for 
anything
but creation code, together with existing block_crypto_read_func which is 
already
not used for creation.


Best regards,
Maxim Levitsky

Re: [PATCH v2 05/11] block/crypto: implement the encryption key management

[Max Reitz]

On 08.11.19 10:30, Maxim Levitsky wrote:
> On Fri, 2019-10-04 at 20:41 +0200, Max Reitz wrote:
>> On 13.09.19 00:30, Maxim Levitsky wrote:
>>> This implements the encryption key management
>>> using the generic code in qcrypto layer
>>> (currently only for qemu-img amend)
>>>
>>> This code adds another 'write_func' because the initialization
>>> write_func works directly on the underlying file,
>>> because during the creation, there is no open instance
>>> of the luks driver, but during regular use, we have it,
>>> and should use it instead.
>>>
>>>
>>> This commit also adds a 'hack/workaround' I and Kevin Wolf (thanks)
>>> made to make the driver still support write sharing,
>>> but be safe against concurrent  metadata update (the keys)
>>> Eventually write sharing for luks driver will be deprecated
>>> and removed together with this hack.
>>>
>>> The hack is that we ask (as a format driver) for
>>> BLK_PERM_CONSISTENT_READ always
>>> (technically always unless opened with BDRV_O_NO_IO)
>>>
>>> and then when we want to update the keys, we
>>> unshare that permission. So if someone else
>>> has the image open, even readonly, this will fail.
>>>
>>> Also thanks to Daniel Berrange for the variant of
>>> that hack that involves asking for read,
>>> rather that write permission
>>>
>>> Signed-off-by: Maxim Levitsky 
>>> ---
>>>  block/crypto.c | 118 +++--
>>>  1 file changed, 115 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/block/crypto.c b/block/crypto.c
>>> index a6a3e1f1d8..f42fa057e6 100644
>>> --- a/block/crypto.c
>>> +++ b/block/crypto.c
>>> @@ -36,6 +36,7 @@ typedef struct BlockCrypto BlockCrypto;
>>>  
>>>  struct BlockCrypto {
>>>  QCryptoBlock *block;
>>> +bool updating_keys;
>>>  };
>>>  
>>>  
>>> @@ -70,6 +71,24 @@ static ssize_t block_crypto_read_func(QCryptoBlock 
>>> *block,
>>>  return ret;
>>>  }
>>>  
>>> +static ssize_t block_crypto_write_func(QCryptoBlock *block,
>>> +   size_t offset,
>>> +   const uint8_t *buf,
>>> +   size_t buflen,
>>> +   void *opaque,
>>> +   Error **errp)
>>
>> There’s already a function of this name for creation.
> 
> There is a long story why two write functions are needed.
> i tried to use only one, but at the end I and Daniel both agreed
> that its just better to have two functions.
> 
> The reason is that during creation, the luks BlockDriverState doesn't exist 
> yet,
> and so the creation routine basically just writes to the underlying protocol 
> driver.
> 
> Thats is why the block_crypto_create_write_func receives a BlockBackend 
> pointer,
> to which the BlockDriverState of the underlying protocol driver is inserted.
> 
> 
> On the other hand, for amend, the luks block device is open, and it only knows
> about its own BlockDriverState, and thus the io should be done on bs->file
> 
> So instead of trying to coerce a single callback to do both of this,
> we decided to just have a little code duplication.

I meant: This doesn’t compile.  There’s already another function of this
name.

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v2 02/11] qcrypto-luks: extend the create options for upcoming encryption key management

[Max Reitz]

On 08.11.19 10:28, Maxim Levitsky wrote:
> On Fri, 2019-10-04 at 19:42 +0200, Max Reitz wrote:
>> On 13.09.19 00:30, Maxim Levitsky wrote:
>>> Now you can specify which slot to put the encryption key to
>>> Plus add 'active' option which will let  user erase the key secret
>>> instead of adding it.
>>> Check that active=true it when creating.
>>>
>>> Signed-off-by: Maxim Levitsky 
>>> ---
>>>  block/crypto.c |  2 ++
>>>  block/crypto.h | 16 +++
>>>  block/qcow2.c  |  2 ++
>>>  crypto/block-luks.c| 26 +++---
>>>  qapi/crypto.json   | 19 ++
>>>  tests/qemu-iotests/082.out | 54 ++
>>>  6 files changed, 115 insertions(+), 4 deletions(-)
>>
>> (Just doing a cursory RFC-style review)
>>
>> I think we also want to reject unlock-secret if it’s given for creation;
> Agree, I'll do this in the next version.
> 
>> and I suppose it’d be more important to print which slots are OK than
>> the slot the user has given.  (It isn’t like we shouldn’t print that
>> slot index, but it’s more likely the user knows that than what the
>> limits are.  I think.)
> I don't really understand what you mean here :-( 
> 
> Since this is qmp interface,
> I can't really print anything from it, other that error messages.

Exactly, I’m referring to the error message.  Right now it’s:

"Invalid slot %" PRId64 " is specified", luks_opts.slot

I think it should be something like:

"Invalid slot %" PRId64 " specified, must be between 0 and %u",
luks_opt.slot, QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS - 1

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v2 07/11] block: add x-blockdev-amend qmp command

[Max Reitz]

On 08.11.19 10:26, Maxim Levitsky wrote:
> On Fri, 2019-10-04 at 20:53 +0200, Max Reitz wrote:
>> On 13.09.19 00:30, Maxim Levitsky wrote:
>>> Signed-off-by: Maxim Levitsky 
>>> ---
>>>  block/Makefile.objs   |   2 +-
>>>  block/amend.c | 116 ++
>>>  include/block/block_int.h |  23 ++--
>>>  qapi/block-core.json  |  26 +
>>>  qapi/job.json |   4 +-
>>>  5 files changed, 163 insertions(+), 8 deletions(-)
>>>  create mode 100644 block/amend.c

[...]

>>> +static int coroutine_fn blockdev_amend_run(Job *job, Error **errp)
>>> +{
>>> +BlockdevAmendJob *s = container_of(job, BlockdevAmendJob, common);
>>> +int ret;
>>> +
>>> +job_progress_set_remaining(>common, 1);
>>> +ret = s->bs->drv->bdrv_co_amend(s->bs, s->opts, s->force, errp);
>>> +job_progress_update(>common, 1);
>>
>> It would be nice if the amend job could make use of the progress
>> reporting that we have in place for amend.
> 
> I also thought about it, but is it worth it?
> 
> I looked through the status reporting of the qcow2 amend
> code (which doesn't really allowed to be run through
> qmp blockdev-amend, due to complexity of changing 
> the qcow2 format on the fly).

True, and we could always add it later.

I suppose I was mostly wondering because bdrv_amend_options already has
all of that infrastructure and I was assuming that qcow2's bdrv_co_amend
implementation would make some use of the existing function.  Well, it
doesn’t, so *shrug*

[...]

>>> +/*
>>> + * Create the block job
>>> + * TODO Running in the main context. Block drivers need to error out 
>>> or add
>>> + * locking when they use a BDS in a different AioContext.
>>
>> Why shouldn’t the job just run in the node’s context?
> 
> This is shameless copy from the blockdev-create code
> (which I did note in the copyright of the file)
Well, you noted that it’s heavily based on it, not that it’s just C

So I suppose the comment is just wrong here?

Max



signature.asc
Description: OpenPGP digital signature

[PATCH v2 2/2] qapi: deprecate implicit filters

[Vladimir Sementsov-Ogievskiy]

To get rid of implicit filters related workarounds in future let's
deprecate them now.

Deprecation warning breaks some bash iotests output, so fix it here
too: in most of cases just add filter-node-name in test.

In 161 add FIXME and deprecation warning into 161.out.

In 249, the test case is changed, as we don't need to test "default"
filter node name anymore.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 qemu-deprecated.texi   |  6 ++
 qapi/block-core.json   |  9 ++---
 include/block/block_int.h  | 10 +-
 blockdev.c | 10 ++
 tests/qemu-iotests/094 |  1 +
 tests/qemu-iotests/095 |  6 --
 tests/qemu-iotests/109 |  1 +
 tests/qemu-iotests/127 |  1 +
 tests/qemu-iotests/141 |  5 -
 tests/qemu-iotests/144 |  3 ++-
 tests/qemu-iotests/156 |  1 +
 tests/qemu-iotests/161 |  7 +++
 tests/qemu-iotests/161.out |  1 +
 tests/qemu-iotests/185 |  3 +++
 tests/qemu-iotests/191 |  2 ++
 tests/qemu-iotests/229 |  1 +
 tests/qemu-iotests/247 |  8 +---
 tests/qemu-iotests/249 |  5 +++--
 tests/qemu-iotests/249.out |  2 +-
 19 files changed, 68 insertions(+), 14 deletions(-)

diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 296bfc93a3..c969faf55a 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -204,6 +204,12 @@ and accurate ``query-qmp-schema'' command.
 Character devices creating sockets in client mode should not specify
 the 'wait' field, which is only applicable to sockets in server mode
 
+@subsection implicit filters in mirror and commit (since 4.2)
+
+Mirror and commit jobs insert filters, which becomes implicit if user
+omitted @option(filter-node-name) parameter. So omitting it is deprecated
+in ``blockdev-mirror'', ``drive-mirror'' and ``block-commit'', set it always.
+
 @section Human Monitor Protocol (HMP) commands
 
 @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove' (since 
3.1)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 93530d3a13..37caed775f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1659,7 +1659,8 @@
 # @filter-node-name: the node name that should be assigned to the
 #filter driver that the commit job inserts into the graph
 #above @top. If this option is not given, a node name is
-#autogenerated. (Since: 2.9)
+#autogenerated. Omitting this option is deprecated, it will
+#be required in future. (Since: 2.9)
 #
 # @auto-finalize: When false, this job will wait in a PENDING state after it 
has
 # finished its work, waiting for @block-job-finalize before
@@ -1996,7 +1997,8 @@
 # @filter-node-name: the node name that should be assigned to the
 #filter driver that the mirror job inserts into the graph
 #above @device. If this option is not given, a node name is
-#autogenerated. (Since: 4.2)
+#autogenerated. Omitting this option is deprecated, it will
+#be required in future. (Since: 4.2)
 #
 # @unmap: Whether to try to unmap target sectors where source has
 # only zero. If true, and target unallocated sectors will read as zero,
@@ -2311,7 +2313,8 @@
 # @filter-node-name: the node name that should be assigned to the
 #filter driver that the mirror job inserts into the graph
 #above @device. If this option is not given, a node name is
-#autogenerated. (Since: 2.9)
+#autogenerated. Omitting this option is deprecated, it will
+#be required in future. (Since: 2.9)
 #
 # @copy-mode: when to copy data to the destination; defaults to 'background'
 # (Since: 3.0)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index dd033d0b37..48ff3af48d 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -793,7 +793,15 @@ struct BlockDriverState {
 bool sg;/* if true, the device is a /dev/sg* */
 bool probed;/* if true, format was probed rather than specified */
 bool force_share; /* if true, always allow all shared permissions */
-bool implicit;  /* if true, this filter node was automatically inserted */
+
+/*
+ * @implicit field is deprecated, don't set it to true for new filters.
+ * If true, this filter node was automatically inserted and user don't
+ * know about it and unprepared for any effects of it. So, implicit
+ * filters are workarounded and skipped in many places of the block
+ * layer code.
+ */
+bool implicit;
 
 BlockDriver *drv; /* NULL means no media */
 void *opaque;
diff --git a/blockdev.c b/blockdev.c
index 2ca614c77f..8c3a409c94 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -,6 +,11 @@ void qmp_block_commit(bool has_job_id, const char 
*job_id, const char *device,

[PATCH v2 0/2] Deprecate implicit filters

[Vladimir Sementsov-Ogievskiy]

v2:
Don't deprecate drive-backup, it is unrelated thing and will be resent
in separate.
Don't deprecate drive-mirror. Instead add filter-node-name to
drive-mirror to behave like blockdev-mirror
Fix all broken iotests.

Vladimir Sementsov-Ogievskiy (2):
  qapi: add filter-node-name option to drive-mirror
  qapi: deprecate implicit filters

 qemu-deprecated.texi   |  6 ++
 qapi/block-core.json   | 14 --
 include/block/block_int.h  | 10 +-
 blockdev.c | 12 +++-
 tests/qemu-iotests/094 |  1 +
 tests/qemu-iotests/095 |  6 --
 tests/qemu-iotests/109 |  1 +
 tests/qemu-iotests/127 |  1 +
 tests/qemu-iotests/141 |  5 -
 tests/qemu-iotests/144 |  3 ++-
 tests/qemu-iotests/156 |  1 +
 tests/qemu-iotests/161 |  7 +++
 tests/qemu-iotests/161.out |  1 +
 tests/qemu-iotests/185 |  3 +++
 tests/qemu-iotests/191 |  2 ++
 tests/qemu-iotests/229 |  1 +
 tests/qemu-iotests/247 |  8 +---
 tests/qemu-iotests/249 |  5 +++--
 tests/qemu-iotests/249.out |  2 +-
 19 files changed, 75 insertions(+), 14 deletions(-)

-- 
2.21.0

[PATCH v2 1/2] qapi: add filter-node-name option to drive-mirror

[Vladimir Sementsov-Ogievskiy]

To correspond to blockdev-mirror command and make it possible to
deprecate implicit filters in the next commit.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 qapi/block-core.json | 7 +++
 blockdev.c   | 2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index aa97ee2641..93530d3a13 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1992,6 +1992,12 @@
 # @on-target-error: the action to take on an error on the target,
 #   default 'report' (no limitations, since this applies to
 #   a different block device than @device).
+#
+# @filter-node-name: the node name that should be assigned to the
+#filter driver that the mirror job inserts into the graph
+#above @device. If this option is not given, a node name is
+#autogenerated. (Since: 4.2)
+#
 # @unmap: Whether to try to unmap target sectors where source has
 # only zero. If true, and target unallocated sectors will read as zero,
 # target image sectors will be unmapped; otherwise, zeroes will be
@@ -2022,6 +2028,7 @@
 '*speed': 'int', '*granularity': 'uint32',
 '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
 '*on-target-error': 'BlockdevOnError',
+'*filter-node-name': 'str',
 '*unmap': 'bool', '*copy-mode': 'MirrorCopyMode',
 '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
 
diff --git a/blockdev.c b/blockdev.c
index 8e029e9c01..2ca614c77f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -4008,7 +4008,7 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
arg->has_on_source_error, arg->on_source_error,
arg->has_on_target_error, arg->on_target_error,
arg->has_unmap, arg->unmap,
-   false, NULL,
+   arg->has_filter_node_name, arg->filter_node_name,
arg->has_copy_mode, arg->copy_mode,
arg->has_auto_finalize, arg->auto_finalize,
arg->has_auto_dismiss, arg->auto_dismiss,
-- 
2.21.0

Re: [PATCH] iotests: Fix "no qualified output" error path

[Max Reitz]

On 08.11.19 09:57, Kevin Wolf wrote:
> The variable for error messages to be displayed is $results, not
> $reason. Fix 'check' to print the "no qualified output" error message
> again instead of having a failure without any message telling the user
> why it failed.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/check | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v2 02/11] qcrypto-luks: extend the create options for upcoming encryption key management

[Maxim Levitsky]

On Thu, 2019-10-10 at 15:44 +0200, Kevin Wolf wrote:
> Am 13.09.2019 um 00:30 hat Maxim Levitsky geschrieben:
> > Now you can specify which slot to put the encryption key to
> > Plus add 'active' option which will let  user erase the key secret
> > instead of adding it.
> > Check that active=true it when creating.
> > 
> > Signed-off-by: Maxim Levitsky 
> > diff --git a/qapi/crypto.json b/qapi/crypto.json
> > index b2a4cff683..9b83a70634 100644
> > --- a/qapi/crypto.json
> > +++ b/qapi/crypto.json
> > @@ -190,6 +190,20 @@
> >  #  Currently defaults to 'sha256'
> >  # @hash-alg: the master key hash algorithm
> >  #Currently defaults to 'sha256'
> > +#
> > +# @active: Should the new secret be added (true) or erased (false)
> > +#  (amend only, since 4.2)
> > +#
> > +# @slot: The slot in which to put/erase the secret
> > +#if not given, will select first free slot for secret addtion
> > +#and erase all matching keyslots for erase. except last one
> > +#(optional, since 4.2)
> > +#
> > +# @unlock-secret: The secret to use to unlock the image
> > +#If not given, will use the secret that was used
> > +#when opening the image.
> > +#(optional, for amend only, since 4.2)
> > +#
> >  # @iter-time: number of milliseconds to spend in
> >  # PBKDF passphrase processing. Currently defaults
> >  # to 2000. (since 2.8)
> 
> This approach doesn't look right to me. BlockdevCreateOptions should
> describe the state of the image after the operation. You're describing
> an update instead (and in a way that doesn't allow you to change
> everything that you may want to change, so that you need to call the
> operation multiple times).
> 
> I imagined the syntax of a blockdev-amend QMP command similar to
> x-blockdev-reopen: Describe the full set of options that you want to
> have in effect after the operation; if you don't want to change some
> option, you just specify it again with its old value.

This approach is a compromise trying to create more or less usable interface.
In particular we (I and Daniel) wanted the following to work:

1. ability to add a new password to an empty keyslot and then remove the
old password. This is probably the most common operation and it won't
require the caller to know anything about the keyslots.

2. Allow the user to not know the passwords of some keyslots.
For example if I want to add a new keyslot, I might  not know
some of the other keyslots. Specifying all the active keyslots,
on each amend would force the user to know all the passwords
(you can't 'extract' a password by reading a keyslot, since only
hash of it is stored there for the security reasons)


Thus the amend interface either allows you to add a keyslot (either a specific 
one,
or first free one), and to remove a keyslot (again, either a specific one, or
one that matches given password).




> 
> Specifically for luks, this probably means that you have a @slots, which
> is a list that contains at least the secret for each slot, or JSON null
> for a slot that should be left empty.
> 
> With the same approach, you don't have to make 'size' optional in later
> patches, you can just require that the current size is re-specified. And
> later, blockdev-amend could actually allow changing the size of images
> if you provide a different value.

This can be done IMHO.

> 
> Kevin


Best regards,   
Maxim Levitsky

Re: [PATCH v3 00/22] iotests: Allow ./check -o data_file

[Max Reitz]

On 07.11.19 22:10, no-re...@patchew.org wrote:
> Patchew URL: 
> https://patchew.org/QEMU/20191107163708.833192-1-mre...@redhat.com/
> 
> 
> 
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Subject: [PATCH v3 00/22] iotests: Allow ./check -o data_file
> Type: series
> Message-id: 20191107163708.833192-1-mre...@redhat.com
> 
> === 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'
> 01a2839 iotests: Allow check -o data_file
> 31bc07b iotests: Disable data_file where it cannot be used
> 98a2575 iotests: Make 198 work with data_file
> e8f406f iotests: Make 137 work with data_file
> 46cc09d iotests: Make 110 work with data_file
> 1f7b2e5 iotests: Make 091 work with data_file
> 401d3ef iotests: Avoid cp/mv of test images
> a3746a2 iotests: Use _rm_test_img for deleting test images
> 37a01c8 iotests: Avoid qemu-img create
> a05c5ec iotests: Drop IMGOPTS use in 267
> 44aac70 iotests: Replace IMGOPTS='' by --no-opts
> cb9ee70 iotests: Replace IMGOPTS= by -o
> 3c2893f iotests: Inject space into -ocompat=0.10 in 051
> 8b5f9d4 iotests: Add -o and --no-opts to _make_test_img
> 239f933 iotests: Let _make_test_img parse its parameters
> 405ddde iotests: Drop compat=1.1 in 050
> 527ae22 iotests: Replace IMGOPTS by _unsupported_imgopts
> 77f688d iotests: Filter refcount_order in 036
> 3f29d5f iotests: Add _filter_json_filename
> 58975a8 iotests/qcow2.py: Split feature fields into bits
> 7ea641e iotests/qcow2.py: Add dump-header-exts
> 469af5e iotests: s/qocw2/qcow2/
> 
> === OUTPUT BEGIN ===
> 1/22 Checking commit 469af5ede216 (iotests: s/qocw2/qcow2/)
> 2/22 Checking commit 7ea641ec6b0a (iotests/qcow2.py: Add dump-header-exts)
> ERROR: line over 90 characters
> #33: FILE: tests/qemu-iotests/qcow2.py:237:
> +[ 'dump-header-exts', cmd_dump_header_exts, 0, 'Dump image 
> header extensions' ],

As in v1, I deliberately followed the existing style in this file and
believe it’s for the best.

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v1 2/4] virtio: make seg_max virtqueue size dependent

[Michael S. Tsirkin]

On Fri, Nov 08, 2019 at 07:43:22AM +, Denis Plotnikov wrote:
> The 1st patch from the series seems to be useless. The patch extending 
> queue length by adding machine type may break vm-s which use seabios 
> with max queue size = 128.
> 
> Looks like only this patch doesn't break anything and helps to express 
> queue size and seg max dependency (the specification constraint) 
> explicitly. So, I would like to re-send this patch as a standalone one 
> and send other patches including the test later, when we all agree on 
> how exactly to deal with issues posted in the thread.

OK, and I think we should make it machine type dependent.

> Any objections are welcome.
> 
> Denis
> 
> On 06.11.2019 14:54, Michael S. Tsirkin wrote:
> > On Wed, Nov 06, 2019 at 10:07:02AM +, Denis Lunev wrote:
> >> On 11/5/19 9:51 PM, Michael S. Tsirkin wrote:
> >>> On Tue, Nov 05, 2019 at 07:11:03PM +0300, Denis Plotnikov wrote:
>  seg_max has a restriction to be less or equal to virtqueue size
>  according to Virtio 1.0 specification
> 
>  Although seg_max can't be set directly, it's worth to express this
>  dependancy directly in the code for sanity purpose.
> 
>  Signed-off-by: Denis Plotnikov 
> >>> This is guest visible so needs to be machine type dependent, right?
> >> we have discussed this verbally with Stefan and think that
> >> there is no need to add that to the machine type as:
> >>
> >> - original default was 126, which matches 128 as queue
> >>    length in old machine types
> >> - queue length > 128 is not observed in the field as
> >>    SeaBios has quirk that asserts
> > Well that's just the SeaBios virtio driver. Not everyone's using that to
> > drive their devices.
> >
> >> - if queue length will be set to something < 128 - linux
> >>    guest will crash
> > Again that's just one guest driver. Not everyone is using that either.
> >
> >
> >> If we really need to preserve original __buggy__ behavior -
> >> we can add boolean property, pls let us know.
> >>
> >> Den
> > Looks like some drivers are buggy but I'm not sure it's
> > the same as saying the behavior is buggy.
> > So yes, I'd say it's preferable to be compatible.
> >
> >
>  ---
>    hw/block/virtio-blk.c | 2 +-
>    hw/scsi/virtio-scsi.c | 2 +-
>    2 files changed, 2 insertions(+), 2 deletions(-)
> 
>  diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
>  index 06e57a4d39..21530304cf 100644
>  --- a/hw/block/virtio-blk.c
>  +++ b/hw/block/virtio-blk.c
>  @@ -903,7 +903,7 @@ static void virtio_blk_update_config(VirtIODevice 
>  *vdev, uint8_t *config)
>    blk_get_geometry(s->blk, );
>    memset(, 0, sizeof(blkcfg));
>    virtio_stq_p(vdev, , capacity);
>  -virtio_stl_p(vdev, _max, 128 - 2);
>  +virtio_stl_p(vdev, _max, s->conf.queue_size - 2);
>    virtio_stw_p(vdev, , conf->cyls);
>    virtio_stl_p(vdev, _size, blk_size);
>    virtio_stw_p(vdev, _io_size, conf->min_io_size / 
>  blk_size);
>  diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
>  index 839f120256..f7e5533cd5 100644
>  --- a/hw/scsi/virtio-scsi.c
>  +++ b/hw/scsi/virtio-scsi.c
>  @@ -650,7 +650,7 @@ static void virtio_scsi_get_config(VirtIODevice 
>  *vdev,
>    VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
>    
>    virtio_stl_p(vdev, >num_queues, s->conf.num_queues);
>  -virtio_stl_p(vdev, >seg_max, 128 - 2);
>  +virtio_stl_p(vdev, >seg_max, s->conf.virtqueue_size - 2);
>    virtio_stl_p(vdev, >max_sectors, s->conf.max_sectors);
>    virtio_stl_p(vdev, >cmd_per_lun, s->conf.cmd_per_lun);
>    virtio_stl_p(vdev, >event_info_size, 
>  sizeof(VirtIOSCSIEvent));
>  -- 
>  2.17.0

[PATCH 3/4 V2] hw/scsi: add SCSI COMPARE_AND_WRITE support

[Yaowei Bai]

This patch emulates COMPARE_AND_WRITE command with the
BDRV_REQ_COMPARE_AND_WRITE flag introduced by last patch. It matches
the SBC-4 standard except the FUA bit support, it'll be finished in
the next patch.

Note that cmd->xfer is set 2 * the number got by scsi_data_cdb_xfer
so we could touch the least code.

Signed-off-by: Yaowei Bai 
---
 hw/scsi/emulation.c |  1 +
 hw/scsi/scsi-bus.c  |  4 +++
 hw/scsi/scsi-disk.c | 88 +
 hw/scsi/trace-events|  1 +
 include/hw/scsi/emulation.h |  3 ++
 include/scsi/utils.h|  2 ++
 scsi/utils.c|  5 +++
 7 files changed, 104 insertions(+)

diff --git a/hw/scsi/emulation.c b/hw/scsi/emulation.c
index 06d62f3..1f53c4a 100644
--- a/hw/scsi/emulation.c
+++ b/hw/scsi/emulation.c
@@ -9,6 +9,7 @@ int scsi_emulate_block_limits(uint8_t *outbuf, const 
SCSIBlockLimits *bl)
 memset(outbuf, 0, 0x3c);
 
 outbuf[0] = bl->wsnz; /* wsnz */
+outbuf[1] = MAX_COMPARE_AND_WRITE_LENGTH;
 
 if (bl->max_io_sectors) {
 /* optimal transfer length granularity.  This field and the optimal
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 359d50d..a20eb11 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -1003,6 +1003,9 @@ static int scsi_req_xfer(SCSICommand *cmd, SCSIDevice 
*dev, uint8_t *buf)
 case WRITE_VERIFY_16:
 cmd->xfer *= dev->blocksize;
 break;
+case COMPARE_AND_WRITE:
+cmd->xfer *= 2 * dev->blocksize;
+break;
 case READ_6:
 case READ_REVERSE:
 /* length 0 means 256 blocks */
@@ -1222,6 +1225,7 @@ static void scsi_cmd_xfer_mode(SCSICommand *cmd)
 case PERSISTENT_RESERVE_OUT:
 case MAINTENANCE_OUT:
 case SET_WINDOW:
+case COMPARE_AND_WRITE:
 case SCAN:
 /* SCAN conflicts with START_STOP.  START_STOP has cmd->xfer set to 0 
for
  * non-scanner devices, so we only get here for SCAN and not for 
START_STOP.
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 07fb5eb..f9a0267 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -478,6 +478,9 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, 
bool acct_failed)
 case ENOSPC:
 scsi_check_condition(r, SENSE_CODE(SPACE_ALLOC_FAILED));
 break;
+case EILSEQ:
+scsi_check_condition(r, SENSE_CODE(MISCOMPARE_DURING_VERIFY));
+break;
 default:
 scsi_check_condition(r, SENSE_CODE(IO_ERROR));
 break;
@@ -1825,6 +1828,84 @@ static void scsi_disk_emulate_write_same(SCSIDiskReq *r, 
uint8_t *inbuf)
scsi_write_same_complete, data);
 }
 
+typedef struct CompareAndWriteCBData {
+SCSIDiskReq *r;
+int64_t sector;
+int nb_sectors;
+QEMUIOVector qiov;
+struct iovec iov;
+} CompareAndWriteCBData;
+
+static void scsi_compare_and_write_complete(void *opaque, int ret)
+{
+CompareAndWriteCBData *data = opaque;
+SCSIDiskReq *r = data->r;
+SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
+
+assert(r->req.aiocb != NULL);
+r->req.aiocb = NULL;
+aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
+if (scsi_disk_req_check_error(r, ret, true)) {
+goto done;
+}
+
+block_acct_done(blk_get_stats(s->qdev.conf.blk), >acct);
+scsi_req_complete(>req, GOOD);
+
+done:
+scsi_req_unref(>req);
+qemu_vfree(data->iov.iov_base);
+g_free(data);
+aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
+}
+
+static void scsi_disk_emulate_compare_and_write(SCSIDiskReq *r, uint8_t *inbuf)
+{
+SCSIRequest *req = >req;
+SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev);
+uint32_t nb_sectors = scsi_data_cdb_xfer(r->req.cmd.buf);
+CompareAndWriteCBData *data;
+uint8_t *buf;
+int i;
+
+if (nb_sectors > MAX_COMPARE_AND_WRITE_LENGTH) {
+scsi_check_condition(r, SENSE_CODE(INVALID_FIELD));
+return;
+}
+
+if (blk_is_read_only(s->qdev.conf.blk)) {
+scsi_check_condition(r, SENSE_CODE(WRITE_PROTECTED));
+return;
+}
+
+if (r->req.cmd.lba > s->qdev.max_lba ||
+!check_lba_range(s, r->req.cmd.lba, nb_sectors)) {
+scsi_check_condition(r, SENSE_CODE(LBA_OUT_OF_RANGE));
+return;
+}
+
+data = g_new0(CompareAndWriteCBData, 1);
+data->r = r;
+data->sector = r->req.cmd.lba * (s->qdev.blocksize / 512);
+data->nb_sectors = r->req.cmd.xfer * (s->qdev.blocksize / 512);
+data->iov.iov_len = data->nb_sectors;
+data->iov.iov_base = buf = blk_blockalign(s->qdev.conf.blk,
+  data->iov.iov_len);
+qemu_iovec_init_external(>qiov, >iov, 1);
+
+for (i = 0; i < data->iov.iov_len; i += s->qdev.blocksize) {
+memcpy([i], [i], s->qdev.blocksize);
+}
+
+scsi_req_ref(>req);
+block_acct_start(blk_get_stats(s->qdev.conf.blk), 

[PATCH 2/4 V2] block/rbd: implement bdrv_aio_compare_and_write interface

[Yaowei Bai]

This patch adds librbd's SCSI COMPARE_AND_WRITE command interface
support with bdrv_aio_compare_and_write function pointer. Note
currently when a miscompare happens a mismatch offset of 0 is
always reported rather than the actual mismatch offset. This
should not be a big issue contemporarily and will be fixed
accordingly in the future.

Signed-off-by: Yaowei Bai 
---
 block/raw-format.c |  3 ++-
 block/rbd.c| 45 +++--
 2 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/block/raw-format.c b/block/raw-format.c
index 3a76ec7..e87cd44 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -439,7 +439,8 @@ static int raw_open(BlockDriverState *bs, QDict *options, 
int flags,
 
 bs->sg = bs->file->bs->sg;
 bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED |
-(BDRV_REQ_FUA & bs->file->bs->supported_write_flags);
+(BDRV_REQ_FUA & bs->file->bs->supported_write_flags) |
+(BDRV_REQ_COMPARE_AND_WRITE & bs->file->bs->supported_write_flags);
 bs->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED |
 ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
 bs->file->bs->supported_zero_flags);
diff --git a/block/rbd.c b/block/rbd.c
index 027cbcc..0e45bc3 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -73,11 +73,18 @@
 #define LIBRBD_USE_IOVEC 0
 #endif
 
+#ifdef LIBRBD_SUPPORTS_COMPARE_AND_WRITE
+#define LIBRBD_HAVE_COMPARE_AND_WRITE 1
+#else
+#define LIBRBD_HAVE_COMPARE_AND_WRITE 0
+#endif
+
 typedef enum {
 RBD_AIO_READ,
 RBD_AIO_WRITE,
 RBD_AIO_DISCARD,
-RBD_AIO_FLUSH
+RBD_AIO_FLUSH,
+RBD_AIO_COMPARE_AND_WRITE
 } RBDAIOCmd;
 
 typedef struct RBDAIOCB {
@@ -798,6 +805,10 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
*options, int flags,
 }
 }
 
+if (LIBRBD_HAVE_COMPARE_AND_WRITE) {
+bs->supported_write_flags = BDRV_REQ_COMPARE_AND_WRITE;
+}
+
 r = 0;
 goto out;
 
@@ -933,7 +944,15 @@ static BlockAIOCB *rbd_start_aio(BlockDriverState *bs,
 
 rcb = g_new(RADOSCB, 1);
 
-if (!LIBRBD_USE_IOVEC) {
+if (cmd == RBD_AIO_COMPARE_AND_WRITE) {
+acb->bounce = qemu_try_blockalign(bs, qiov->size);
+if (acb->bounce == NULL) {
+goto failed;
+}
+
+qemu_iovec_to_buf(acb->qiov, 0, acb->bounce, qiov->size);
+rcb->buf = acb->bounce;
+} else if (!LIBRBD_USE_IOVEC) {
 if (cmd == RBD_AIO_DISCARD || cmd == RBD_AIO_FLUSH) {
 acb->bounce = NULL;
 } else {
@@ -993,6 +1012,10 @@ static BlockAIOCB *rbd_start_aio(BlockDriverState *bs,
 case RBD_AIO_FLUSH:
 r = rbd_aio_flush_wrapper(s->image, c);
 break;
+case RBD_AIO_COMPARE_AND_WRITE:
+r = rbd_aio_compare_and_write(s->image, off, size / 2, rcb->buf,
+ (rcb->buf + size / 2), c, 0, 0);
+break;
 default:
 r = -EINVAL;
 }
@@ -1056,6 +1079,20 @@ static int qemu_rbd_co_flush(BlockDriverState *bs)
 }
 #endif
 
+#ifdef LIBRBD_SUPPORTS_COMPARE_AND_WRITE
+static BlockAIOCB *qemu_rbd_aio_compare_and_write(BlockDriverState *bs,
+  uint64_t offset,
+  uint64_t bytes,
+  QEMUIOVector *qiov,
+  int flags,
+  BlockCompletionFunc *cb,
+  void *opaque)
+{
+return rbd_start_aio(bs, offset, qiov, bytes, cb, opaque,
+ RBD_AIO_COMPARE_AND_WRITE);
+}
+#endif
+
 static int qemu_rbd_getinfo(BlockDriverState *bs, BlockDriverInfo *bdi)
 {
 BDRVRBDState *s = bs->opaque;
@@ -1310,6 +1347,10 @@ static BlockDriver bdrv_rbd = {
 .bdrv_aio_pdiscard  = qemu_rbd_aio_pdiscard,
 #endif
 
+#ifdef LIBRBD_SUPPORTS_COMPARE_AND_WRITE
+.bdrv_aio_compare_and_write = qemu_rbd_aio_compare_and_write,
+#endif
+
 .bdrv_snapshot_create   = qemu_rbd_snap_create,
 .bdrv_snapshot_delete   = qemu_rbd_snap_remove,
 .bdrv_snapshot_list = qemu_rbd_snap_list,
-- 
1.8.3.1

[PATCH 0/4 V2] SCSI COMPARE_AND_WRITE command support

[Yaowei Bai]

Recently ceph/librbd added several interfaces to handle SCSI commands like
COMPARE_AND_WRITE directly. However they were only be used in special
scenarios, i.e. ISCSI. That involves more software components which makes
the IO path longer and could bring more potential issues. Actually we're
maintaining several ceph clusters with ISCSI protocal being used which,
i have to say, is not an easy job.

So supporting COMPARE_AND_WRITE in scsi-disk would be a reasonable solution
and easy to implement with the help of the SCSI handlers in librbd, which
could leave alone the ISCSI stuff and make the IO path shorter.

This patchset implements it by reusing the blk_aio_pwritev interface and
introducing a new BDRV_REQ_COMPARE_AND_WRITE element into BdrvRequestFlags
to indicate a COMPARE_AND_WRITE request, rather than adding a new interface
into block-backend.c.

The FUA support is implemented in the blk_aio_pwritev's callback function
similar to its emulation in scsi_write_do_fua function, maybe through
BDRV_REQ_FUA is another doable way.

This patchset is tested with the method of sg_compare_and_write.txt from
sg3_utils. Link below:

https://github.com/hreinecke/sg3_utils/blob/master/examples/sg_compare_and_write.txt

v1->v2: fix checkpatch script complaints and cleanup

Yaowei Bai (4):
  block: add SCSI COMPARE_AND_WRITE support
  block/rbd: implement bdrv_aio_compare_and_write interface
  hw/scsi: add SCSI COMPARE_AND_WRITE support
  scsi-disk: add FUA support for COMPARE_AND_WRITE

 block/io.c  | 20 +
 block/raw-format.c  |  3 +-
 block/rbd.c | 45 -
 hw/scsi/emulation.c |  1 +
 hw/scsi/scsi-bus.c  |  4 ++
 hw/scsi/scsi-disk.c | 98 +
 hw/scsi/trace-events|  1 +
 include/block/block.h   |  5 ++-
 include/block/block_int.h   |  3 ++
 include/hw/scsi/emulation.h |  3 ++
 include/scsi/utils.h|  2 +
 scsi/utils.c|  5 +++
 12 files changed, 185 insertions(+), 5 deletions(-)

-- 
1.8.3.1

[PATCH 1/4 V2] block: add SCSI COMPARE_AND_WRITE support

[Yaowei Bai]

Some storages(i.e. librbd) already have interfaces to handle some SCSI
commands directly. This patch adds COMPARE_AND_WRITE command support
through the write path in the block io layer by introducing a new element
BDRV_REQ_COMPARE_AND_WRITE into BdrvRequestFlags which indicates a
COMPARE_AND_WRITE request. In this way we could easily extend to other
SCSI commands support like WRITE_SAME in the future.

Signed-off-by: Yaowei Bai 
---
 block/io.c| 20 
 include/block/block.h |  5 +++--
 include/block/block_int.h |  3 +++
 3 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/block/io.c b/block/io.c
index f75777f..3507d71 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1186,6 +1186,26 @@ static int coroutine_fn 
bdrv_driver_pwritev(BlockDriverState *bs,
 goto emulate_flags;
 }
 
+if (drv->bdrv_aio_compare_and_write &&
+  (flags & BDRV_REQ_COMPARE_AND_WRITE)) {
+BlockAIOCB *acb;
+CoroutineIOCompletion co = {
+.coroutine = qemu_coroutine_self(),
+};
+
+acb = drv->bdrv_aio_compare_and_write(bs, offset, bytes, qiov,
+flags & bs->supported_write_flags,
+bdrv_co_io_em_complete, );
+flags &= ~bs->supported_write_flags;
+if (acb == NULL) {
+ret = -EIO;
+} else {
+qemu_coroutine_yield();
+ret = co.ret;
+}
+goto emulate_flags;
+}
+
 if (drv->bdrv_aio_pwritev) {
 BlockAIOCB *acb;
 CoroutineIOCompletion co = {
diff --git a/include/block/block.h b/include/block/block.h
index 1df9848..f71aa4f 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -92,9 +92,10 @@ typedef enum {
  * on read request and means that caller doesn't really need data to be
  * written to qiov parameter which may be NULL.
  */
-BDRV_REQ_PREFETCH  = 0x200,
+BDRV_REQ_PREFETCH   = 0x200,
+BDRV_REQ_COMPARE_AND_WRITE  = 0x400,
 /* Mask of valid flags */
-BDRV_REQ_MASK   = 0x3ff,
+BDRV_REQ_MASK   = 0x7ff,
 } BdrvRequestFlags;
 
 typedef struct BlockSizes {
diff --git a/include/block/block_int.h b/include/block/block_int.h
index dd033d0..96096e0 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -189,6 +189,9 @@ struct BlockDriver {
 BlockAIOCB *(*bdrv_aio_pdiscard)(BlockDriverState *bs,
 int64_t offset, int bytes,
 BlockCompletionFunc *cb, void *opaque);
+BlockAIOCB *(*bdrv_aio_compare_and_write)(BlockDriverState *bs,
+uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags,
+BlockCompletionFunc *cb, void *opaque);
 
 int coroutine_fn (*bdrv_co_readv)(BlockDriverState *bs,
 int64_t sector_num, int nb_sectors, QEMUIOVector *qiov);
-- 
1.8.3.1

[PATCH 4/4 V2] scsi-disk: add FUA support for COMPARE_AND_WRITE

[Yaowei Bai]

It is implemented in the blk_aio_pwritev's callback function in a way
similar to its emulation in scsi_write_do_fua function

Signed-off-by: Yaowei Bai 
---
 hw/scsi/scsi-disk.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index f9a0267..0731a3b 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -229,6 +229,7 @@ static bool scsi_is_cmd_fua(SCSICommand *cmd)
 case WRITE_10:
 case WRITE_12:
 case WRITE_16:
+case COMPARE_AND_WRITE:
 return (cmd->buf[1] & 8) != 0;
 
 case VERIFY_10:
@@ -1850,10 +1851,17 @@ static void scsi_compare_and_write_complete(void 
*opaque, int ret)
 }
 
 block_acct_done(blk_get_stats(s->qdev.conf.blk), >acct);
+if (r->need_fua_emulation) {
+block_acct_start(blk_get_stats(s->qdev.conf.blk), >acct, 0,
+ BLOCK_ACCT_FLUSH);
+r->req.aiocb = blk_aio_flush(s->qdev.conf.blk, scsi_aio_complete, r);
+goto free;
+}
 scsi_req_complete(>req, GOOD);
 
 done:
 scsi_req_unref(>req);
+free:
 qemu_vfree(data->iov.iov_base);
 g_free(data);
 aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
@@ -1954,6 +1962,7 @@ static int32_t scsi_disk_emulate_command(SCSIRequest 
*req, uint8_t *buf)
 {
 SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
 SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev);
+SCSIDiskClass *sdc = (SCSIDiskClass *) object_get_class(OBJECT(s));
 uint64_t nb_sectors;
 uint8_t *outbuf;
 int buflen;
@@ -2209,6 +2218,7 @@ static int32_t scsi_disk_emulate_command(SCSIRequest 
*req, uint8_t *buf)
 return 0;
 }
 assert(!r->req.aiocb);
+r->need_fua_emulation = sdc->need_fua_emulation(>req.cmd);
 r->iov.iov_len = MIN(r->buflen, req->cmd.xfer);
 if (r->iov.iov_len == 0) {
 scsi_req_complete(>req, GOOD);
-- 
1.8.3.1

Re: Deprecating stuff for 4.2

[Vladimir Sementsov-Ogievskiy]

08.11.2019 9:41, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy  writes:
> 
>> 07.11.2019 21:52, Philippe Mathieu-Daudé wrote:
> [...]
>>> Pre-release period, time to deprecate some stuffs :)
>>>
>>> How should we proceed? Do you have something in mind?
>>>
>>> There are older threads about this. Should we start a new thread? Gather 
>>> the different ideas on the Wiki?
>>>
>>> (Obviously you are not the one responsible of this topic, you just happen 
>>> to be the last one worried about it on the list).
>>>
>>> Regards,
>>>
>>> Phil.
> 
> 4.2.0-rc0 has been tagged, i.e. we're in hard freeze already.  Only bug
> fixes are accepted during hard freeze.  We've occasionally bent this
> rule after -rc0 for borderline cases, e.g. to tweak a new external
> interface before the release calcifies it.  Making a case for bending
> the rules becomes harder with each -rc.
> 
> Ideally, we'd double-check new interfaces for gaffes before a release,
> and whether old interfaces that have been replaced now should be
> deprecated.  There's rarely time for that, and pretty much never for
> releases right after KVM Forum.
> 
> So no, I don't have anything in mind for 4.2.
> 
> We intend to tag -rc1 next Tuesday.  To make that deadline, we'd need
> patches, not just ideas.
> 
>> Hi!
>>
>> I wanted to resend, but faced some problems, and understand that I can't do 
>> it in time before soft-freeze..
>> But you say, that we can deprecate something even after hard-freeze?
> 
> See above.
> 
>> Ok, the problem that I faced, is that deprecation warnings breaks some 
>> iotests.. What can we do?
>>
>> 1. Update iotests...
>> 1.1 Just update iotests outputs to show warnings. Then, in next release 
>> cycle, update iotests, to not use deprecated things
> 
> Sounds workable to me, but I'm not the maintainer.
> 
>> or
>> 1.2 Update iotests to not use deprecated things.. Not appropriate for 
>> hard freeze.
> 
> Unnecessarily risky compared to 1.1.
> 
>> or
>> 2. Commit deprecations without warnings.. But how do people find out about 
>> this?
> 
> Not nice.
> 
> We do it for QMP, but only because we still lack the means to warn
> there.
> 
>> Next, what exactly to deprecate? As I understand, we can't deprecate 
>> drive-mirror now?
>> So I propose to:
>>
>> 1. deprecate drive-backup
>> 2. add optional filter-node-name parameter to drive-mirror, to correspond to 
>> commit and mirror
>> 3. deprecate that filter-node-name is optional for commit and mirror.
> 
> To have a chance there, we need patches a.s.a.p.
> 

OK, I'll send today and we'll see, what to do with it.

-- 
Best regards,
Vladimir

Re: [PATCH v2 05/11] block/crypto: implement the encryption key management

[Maxim Levitsky]

On Fri, 2019-10-04 at 20:41 +0200, Max Reitz wrote:
> On 13.09.19 00:30, Maxim Levitsky wrote:
> > This implements the encryption key management
> > using the generic code in qcrypto layer
> > (currently only for qemu-img amend)
> > 
> > This code adds another 'write_func' because the initialization
> > write_func works directly on the underlying file,
> > because during the creation, there is no open instance
> > of the luks driver, but during regular use, we have it,
> > and should use it instead.
> > 
> > 
> > This commit also adds a 'hack/workaround' I and Kevin Wolf (thanks)
> > made to make the driver still support write sharing,
> > but be safe against concurrent  metadata update (the keys)
> > Eventually write sharing for luks driver will be deprecated
> > and removed together with this hack.
> > 
> > The hack is that we ask (as a format driver) for
> > BLK_PERM_CONSISTENT_READ always
> > (technically always unless opened with BDRV_O_NO_IO)
> > 
> > and then when we want to update the keys, we
> > unshare that permission. So if someone else
> > has the image open, even readonly, this will fail.
> > 
> > Also thanks to Daniel Berrange for the variant of
> > that hack that involves asking for read,
> > rather that write permission
> > 
> > Signed-off-by: Maxim Levitsky 
> > ---
> >  block/crypto.c | 118 +++--
> >  1 file changed, 115 insertions(+), 3 deletions(-)
> > 
> > diff --git a/block/crypto.c b/block/crypto.c
> > index a6a3e1f1d8..f42fa057e6 100644
> > --- a/block/crypto.c
> > +++ b/block/crypto.c
> > @@ -36,6 +36,7 @@ typedef struct BlockCrypto BlockCrypto;
> >  
> >  struct BlockCrypto {
> >  QCryptoBlock *block;
> > +bool updating_keys;
> >  };
> >  
> >  
> > @@ -70,6 +71,24 @@ static ssize_t block_crypto_read_func(QCryptoBlock 
> > *block,
> >  return ret;
> >  }
> >  
> > +static ssize_t block_crypto_write_func(QCryptoBlock *block,
> > +   size_t offset,
> > +   const uint8_t *buf,
> > +   size_t buflen,
> > +   void *opaque,
> > +   Error **errp)
> 
> There’s already a function of this name for creation.

There is a long story why two write functions are needed.
i tried to use only one, but at the end I and Daniel both agreed
that its just better to have two functions.

The reason is that during creation, the luks BlockDriverState doesn't exist yet,
and so the creation routine basically just writes to the underlying protocol 
driver.

Thats is why the block_crypto_create_write_func receives a BlockBackend pointer,
to which the BlockDriverState of the underlying protocol driver is inserted.


On the other hand, for amend, the luks block device is open, and it only knows
about its own BlockDriverState, and thus the io should be done on bs->file

So instead of trying to coerce a single callback to do both of this,
we decided to just have a little code duplication.


> 
> > +{
> > +BlockDriverState *bs = opaque;
> > +ssize_t ret;
> > +
> > +ret = bdrv_pwrite(bs->file, offset, buf, buflen);
> > +if (ret < 0) {
> > +error_setg_errno(errp, -ret, "Could not write encryption header");
> > +return ret;
> > +}
> > +return ret;
> > +}
> > +
> >  
> >  struct BlockCryptoCreateData {
> >  BlockBackend *blk;
> 
> [...]
> 
> > +static void
> > +block_crypto_child_perms(BlockDriverState *bs, BdrvChild *c,
> > + const BdrvChildRole *role,
> > + BlockReopenQueue *reopen_queue,
> > + uint64_t perm, uint64_t shared,
> > + uint64_t *nperm, uint64_t *nshared)
> > +{
> > +
> > +BlockCrypto *crypto = bs->opaque;
> > +
> > +/*
> > + * Ask for consistent read permission so that if
> > + * someone else tries to open this image with this permission
> > + * neither will be able to edit encryption keys
> > + */
> > +if (!(bs->open_flags & BDRV_O_NO_IO)) {
> > +perm |= BLK_PERM_CONSISTENT_READ;
> > +}
> > +
> > +/*
> > + * This driver doesn't modify LUKS metadata except
> > + * when updating the encryption slots.
> > + * Thus unlike a proper format driver we don't ask for
> > + * shared write permission. However we need it
> > + * when we area updating keys, to ensure that only we
> > + * had opened the device r/w
> > + *
> > + * Encryption update will set the crypto->updating_keys
> > + * during that period and refresh permissions
> > + *
> > + */
> > +
> > +if (crypto->updating_keys) {
> > +/*need exclusive write access for header update  */
> > +perm |= BLK_PERM_WRITE;
> > +shared &= ~(BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE);
> > +}
> > +
> > +bdrv_filter_default_perms(bs, c, role, 

Re: [PATCH v2 11/11] iotests : add tests for encryption key management

[Maxim Levitsky]

On Fri, 2019-10-04 at 21:11 +0200, Max Reitz wrote:
> On 13.09.19 00:30, Maxim Levitsky wrote:
> > Note that currently I add tests 300-302, which are
> > placeholders to ease the rebase. In final version
> > of these patches I will update these.
> > 
> > Signed-off-by: Maxim Levitsky 
> > ---
> >  tests/qemu-iotests/300 | 202 +
> >  tests/qemu-iotests/300.out |  98 +++
> >  tests/qemu-iotests/301 |  90 +
> >  tests/qemu-iotests/301.out |  30 +
> >  tests/qemu-iotests/302 | 252 +
> >  tests/qemu-iotests/302.out |  18 +++
> >  tests/qemu-iotests/303 | 228 +
> >  tests/qemu-iotests/303.out |  28 +
> >  tests/qemu-iotests/group   |   9 ++
> >  9 files changed, 955 insertions(+)
> >  create mode 100755 tests/qemu-iotests/300
> >  create mode 100644 tests/qemu-iotests/300.out
> >  create mode 100755 tests/qemu-iotests/301
> >  create mode 100644 tests/qemu-iotests/301.out
> >  create mode 100644 tests/qemu-iotests/302
> >  create mode 100644 tests/qemu-iotests/302.out
> >  create mode 100644 tests/qemu-iotests/303
> >  create mode 100644 tests/qemu-iotests/303.out
> 
> [...]
> 
> > diff --git a/tests/qemu-iotests/303.out b/tests/qemu-iotests/303.out
> > new file mode 100644
> > index 00..1cf3917208
> > --- /dev/null
> > +++ b/tests/qemu-iotests/303.out
> > @@ -0,0 +1,28 @@
> > +qemu-img: Failed to get shared "consistent read" lock
> > +Is another process using the image 
> > [/home/mlevitsk/USERSPACE/qemu/build-luks/tests/qemu-iotests/scratch/test.img]?
> 
> Ah, this should be filtered.

Ooops, missed this one, thanks!
Fixed now.

> 
> Max
> 

Best regards,
Maxim Levitsky

Re: [PATCH v2 02/11] qcrypto-luks: extend the create options for upcoming encryption key management

[Maxim Levitsky]

On Fri, 2019-10-04 at 19:42 +0200, Max Reitz wrote:
> On 13.09.19 00:30, Maxim Levitsky wrote:
> > Now you can specify which slot to put the encryption key to
> > Plus add 'active' option which will let  user erase the key secret
> > instead of adding it.
> > Check that active=true it when creating.
> > 
> > Signed-off-by: Maxim Levitsky 
> > ---
> >  block/crypto.c |  2 ++
> >  block/crypto.h | 16 +++
> >  block/qcow2.c  |  2 ++
> >  crypto/block-luks.c| 26 +++---
> >  qapi/crypto.json   | 19 ++
> >  tests/qemu-iotests/082.out | 54 ++
> >  6 files changed, 115 insertions(+), 4 deletions(-)
> 
> (Just doing a cursory RFC-style review)
> 
> I think we also want to reject unlock-secret if it’s given for creation;
Agree, I'll do this in the next version.

> and I suppose it’d be more important to print which slots are OK than
> the slot the user has given.  (It isn’t like we shouldn’t print that
> slot index, but it’s more likely the user knows that than what the
> limits are.  I think.)
I don't really understand what you mean here :-( 

Since this is qmp interface,
I can't really print anything from it, other that error messages.



> 
> Max
> 

Best regards,
Maxim Levitsky

Re: [Qemu-devel] [PATCH v2 02/11] qcrypto-luks: extend the create options for upcoming encryption key management

[Maxim Levitsky]

On Mon, 2019-10-07 at 09:49 +0200, Markus Armbruster wrote:
> Quick QAPI schema review only.
> 
> Maxim Levitsky  writes:
> 
> > Now you can specify which slot to put the encryption key to
> > Plus add 'active' option which will let  user erase the key secret
> > instead of adding it.
> > Check that active=true it when creating.
> > 
> > Signed-off-by: Maxim Levitsky 
> 
> [...]
> > diff --git a/qapi/crypto.json b/qapi/crypto.json
> > index b2a4cff683..9b83a70634 100644
> > --- a/qapi/crypto.json
> > +++ b/qapi/crypto.json
> > @@ -190,6 +190,20 @@
> 
>##
># @QCryptoBlockCreateOptionsLUKS:
>#
># The options that apply to LUKS encryption format initialization
>#
># @cipher-alg: the cipher algorithm for data encryption
>#  Currently defaults to 'aes-256'.
># @cipher-mode: the cipher mode for data encryption
>#   Currently defaults to 'xts'
># @ivgen-alg: the initialization vector generator
># Currently defaults to 'plain64'
># @ivgen-hash-alg: the initialization vector generator hash
> >  #  Currently defaults to 'sha256'
> >  # @hash-alg: the master key hash algorithm
> >  #Currently defaults to 'sha256'
> > +#
> > +# @active: Should the new secret be added (true) or erased (false)
> > +#  (amend only, since 4.2)
> 
> Is "active" established terminology?  I wouldn't have guessed its
> meaning from its name...

Yea, this is one of the warts of the interface that I wanted to discuss with 
everyone
basically the result of using same API for creation and amend.

blockdev-amend, similiar to qemu-img amend will allow the user to change the 
format driver
settings, and will use the same BlockdevCreateOptions for that, similiar to how 
qemu-img amend works.

For creation of course the 'active' parameter is redundant, and it is forced to 
true.
For amend it allows to add or erase a new key.
We couldn't really think about any better name, so I kind of decided just to 
document
this and leave it like that.

> 
> As far as I can see, QCryptoBlockCreateOptionsLUKS is used just for
> blockdev-create with options.driver \in { luks, qcow, qcow2 }:
> 
>{ 'command': 'blockdev-create',
>  'data': { ...
>'options': 'BlockdevCreateOptions' } }
> 
>{ 'union': 'BlockdevCreateOptions',
>  ...
>  'data': {
>  ...
>  'luks':   'BlockdevCreateOptionsLUKS',
>  ...
>  'qcow':   'BlockdevCreateOptionsQcow',
>  'qcow2':  'BlockdevCreateOptionsQcow2',
>  ... } }
> 
> With luks:
> 
>{ 'struct': 'BlockdevCreateOptionsLUKS',
>  'base': 'QCryptoBlockCreateOptionsLUKS',
>  ... }
> 
> With qcow and qcow2:
> 
> { 'struct': 'BlockdevCreateOptionsQcow',
>   'data': { ...
> '*encrypt': 'QCryptoBlockCreateOptions' } }
> { 'struct': 'BlockdevCreateOptionsQcow2',
>   'data': { ...
> '*encrypt': 'QCryptoBlockCreateOptions',
> ... } }
> 
> { 'union': 'QCryptoBlockCreateOptions',
>   'base': 'QCryptoBlockOptionsBase',
>   'discriminator': 'format',
>   'data': { ...
> 'luks': 'QCryptoBlockCreateOptionsLUKS' } }
> 
> I think I understand why we want blockdev-create to be able to specify a
> new secret.
> 
> Why do we want it to be able to delete an existing secret?  How would
> that even work?  Color me confused...

The BlockdevCreateOptions will now be used in
both creation and amend of a block device. Of course the deletion
of an existing secret doesn't make sense on creation time, and a check
is present to disallow the user to do that.

At the same time, the size and 'file' arguments are made optional,
so that during amend you could change the block device without
specifying those.


> 
> > +#
> > +# @slot: The slot in which to put/erase the secret
> > +#if not given, will select first free slot for secret addtion
> > +#and erase all matching keyslots for erase. except last one
> > +#(optional, since 4.2)
> 
> Excuse my possibly ignorant question: what exactly is a "matching
> keyslot"?
Not ignorant at all, I dropped a word here.
I meant to say that it will erase all the keyslots which match the given
secret, except last one. The 'active' is what decides if to add to to remove
a secret.


> 
> > +#
> > +# @unlock-secret: The secret to use to unlock the image
> > +#If not given, will use the secret that was used
> > +#when opening the image.
> > +#(optional, for amend only, since 4.2)
> 
> More ignorance: what is "amend"?  No mention of it in qapi/*json...
Not ignorant at all again! This interface will be added in the next patch,
and all the changes in this patch (other that specifying the keyslot,
which can be in theory useful anyway) are for it.


> 
> > +#
> >  # @iter-time: number of milliseconds to spend in
> >  # PBKDF passphrase processing. 

Re: [PATCH v2 07/11] block: add x-blockdev-amend qmp command

[Maxim Levitsky]

On Fri, 2019-10-04 at 20:53 +0200, Max Reitz wrote:
> On 13.09.19 00:30, Maxim Levitsky wrote:
> > Signed-off-by: Maxim Levitsky 
> > ---
> >  block/Makefile.objs   |   2 +-
> >  block/amend.c | 116 ++
> >  include/block/block_int.h |  23 ++--
> >  qapi/block-core.json  |  26 +
> >  qapi/job.json |   4 +-
> >  5 files changed, 163 insertions(+), 8 deletions(-)
> >  create mode 100644 block/amend.c
> 
> I think I’d move this (and everything to belongs to it) to a different
> series.
I already kind of do this, patches prior to this one fully implement
the existing amend code path, while this and patches after this
one implement the qmp x-blockdev-amend code path.

i don't mind sending this as two separate patch series, now that
first refactoring patch series is committed upsteam.


> 
> > diff --git a/block/Makefile.objs b/block/Makefile.objs
> > index 35f3bca4d9..10d0308792 100644
> > --- a/block/Makefile.objs
> > +++ b/block/Makefile.objs
> > @@ -18,7 +18,7 @@ block-obj-y += block-backend.o snapshot.o qapi.o
> >  block-obj-$(CONFIG_WIN32) += file-win32.o win32-aio.o
> >  block-obj-$(CONFIG_POSIX) += file-posix.o
> >  block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
> > -block-obj-y += null.o mirror.o commit.o io.o create.o
> > +block-obj-y += null.o mirror.o commit.o io.o create.o amend.o
> >  block-obj-y += throttle-groups.o
> >  block-obj-$(CONFIG_LINUX) += nvme.o
> >  
> > diff --git a/block/amend.c b/block/amend.c
> > new file mode 100644
> > index 00..9bd28e08e7
> > --- /dev/null
> > +++ b/block/amend.c
> > @@ -0,0 +1,116 @@
> > +/*
> > + * Block layer code related to image options amend
> > + *
> > + * Copyright (c) 2018 Kevin Wolf 
> > + * Copyright (c) 2019 Maxim Levitsky 
> > + *
> > + * Heavily based on create.c
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a 
> > copy
> > + * of this software and associated documentation files (the "Software"), 
> > to deal
> > + * in the Software without restriction, including without limitation the 
> > rights
> > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or 
> > sell
> > + * copies of the Software, and to permit persons to whom the Software is
> > + * furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included 
> > in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS 
> > OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR 
> > OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> > FROM,
> > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS 
> > IN
> > + * THE SOFTWARE.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "block/block_int.h"
> > +#include "qemu/job.h"
> > +#include "qemu/main-loop.h"
> > +#include "qapi/qapi-commands-block-core.h"
> > +#include "qapi/qapi-visit-block-core.h"
> > +#include "qapi/clone-visitor.h"
> > +#include "qapi/error.h"
> > +
> > +typedef struct BlockdevAmendJob {
> > +Job common;
> > +BlockdevCreateOptions *opts;
> > +BlockDriverState *bs;
> > +bool force;
> > +} BlockdevAmendJob;
> > +
> > +static int coroutine_fn blockdev_amend_run(Job *job, Error **errp)
> > +{
> > +BlockdevAmendJob *s = container_of(job, BlockdevAmendJob, common);
> > +int ret;
> > +
> > +job_progress_set_remaining(>common, 1);
> > +ret = s->bs->drv->bdrv_co_amend(s->bs, s->opts, s->force, errp);
> > +job_progress_update(>common, 1);
> 
> It would be nice if the amend job could make use of the progress
> reporting that we have in place for amend.

I also thought about it, but is it worth it?

I looked through the status reporting of the qcow2 amend
code (which doesn't really allowed to be run through
qmp blockdev-amend, due to complexity of changing 
the qcow2 format on the fly).



> 
> > +
> > +qapi_free_BlockdevCreateOptions(s->opts);
> > +
> > +return ret;
> > +}
> > +
> > +static const JobDriver blockdev_amend_job_driver = {
> > +.instance_size = sizeof(BlockdevAmendJob),
> > +.job_type  = JOB_TYPE_AMEND,
> > +.run   = blockdev_amend_run,
> > +};
> > +
> > +void qmp_x_blockdev_amend(const char *job_id,
> > +const char *node_name,
> > +BlockdevCreateOptions *options,
> > +bool has_force,
> > +bool force,
> > +Error **errp)
> > +{
> > +BlockdevAmendJob *s;
> > +const char *fmt = BlockdevDriver_str(options->driver);
> > +BlockDriver *drv = bdrv_find_format(fmt);
> > +

Re: [PATCH v2 07/11] block: add x-blockdev-amend qmp command

[Maxim Levitsky]

On Fri, 2019-10-04 at 20:53 +0200, Max Reitz wrote:
> On 13.09.19 00:30, Maxim Levitsky wrote:
> > Signed-off-by: Maxim Levitsky 
> > ---
> >  block/Makefile.objs   |   2 +-
> >  block/amend.c | 116 ++
> >  include/block/block_int.h |  23 ++--
> >  qapi/block-core.json  |  26 +
> >  qapi/job.json |   4 +-
> >  5 files changed, 163 insertions(+), 8 deletions(-)
> >  create mode 100644 block/amend.c
> 
> I think I’d move this (and everything to belongs to it) to a different
> series.
I already kind of do this, patches prior to this one fully implment
the existing amend code path, while this and patches after this
one implement the qmp x-blockdev-amend code path.

i don't mind sending this as two separate patch series, now that
first refactoring patch series is committed upsteam.


> 
> > diff --git a/block/Makefile.objs b/block/Makefile.objs
> > index 35f3bca4d9..10d0308792 100644
> > --- a/block/Makefile.objs
> > +++ b/block/Makefile.objs
> > @@ -18,7 +18,7 @@ block-obj-y += block-backend.o snapshot.o qapi.o
> >  block-obj-$(CONFIG_WIN32) += file-win32.o win32-aio.o
> >  block-obj-$(CONFIG_POSIX) += file-posix.o
> >  block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
> > -block-obj-y += null.o mirror.o commit.o io.o create.o
> > +block-obj-y += null.o mirror.o commit.o io.o create.o amend.o
> >  block-obj-y += throttle-groups.o
> >  block-obj-$(CONFIG_LINUX) += nvme.o
> >  
> > diff --git a/block/amend.c b/block/amend.c
> > new file mode 100644
> > index 00..9bd28e08e7
> > --- /dev/null
> > +++ b/block/amend.c
> > @@ -0,0 +1,116 @@
> > +/*
> > + * Block layer code related to image options amend
> > + *
> > + * Copyright (c) 2018 Kevin Wolf 
> > + * Copyright (c) 2019 Maxim Levitsky 
> > + *
> > + * Heavily based on create.c
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a 
> > copy
> > + * of this software and associated documentation files (the "Software"), 
> > to deal
> > + * in the Software without restriction, including without limitation the 
> > rights
> > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or 
> > sell
> > + * copies of the Software, and to permit persons to whom the Software is
> > + * furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included 
> > in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS 
> > OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR 
> > OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> > FROM,
> > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS 
> > IN
> > + * THE SOFTWARE.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "block/block_int.h"
> > +#include "qemu/job.h"
> > +#include "qemu/main-loop.h"
> > +#include "qapi/qapi-commands-block-core.h"
> > +#include "qapi/qapi-visit-block-core.h"
> > +#include "qapi/clone-visitor.h"
> > +#include "qapi/error.h"
> > +
> > +typedef struct BlockdevAmendJob {
> > +Job common;
> > +BlockdevCreateOptions *opts;
> > +BlockDriverState *bs;
> > +bool force;
> > +} BlockdevAmendJob;
> > +
> > +static int coroutine_fn blockdev_amend_run(Job *job, Error **errp)
> > +{
> > +BlockdevAmendJob *s = container_of(job, BlockdevAmendJob, common);
> > +int ret;
> > +
> > +job_progress_set_remaining(>common, 1);
> > +ret = s->bs->drv->bdrv_co_amend(s->bs, s->opts, s->force, errp);
> > +job_progress_update(>common, 1);
> 
> It would be nice if the amend job could make use of the progress
> reporting that we have in place for amend.

I also thought about it, but is it worth it?

I looked through the status reporting of the qcow2 amend
code (which doesn't really allowed to be run through
qmp blockdev-amend, due to complexity of changing 
the qcow2 format on the fly).


> 
> > +
> > +qapi_free_BlockdevCreateOptions(s->opts);
> > +
> > +return ret;
> > +}
> > +
> > +static const JobDriver blockdev_amend_job_driver = {
> > +.instance_size = sizeof(BlockdevAmendJob),
> > +.job_type  = JOB_TYPE_AMEND,
> > +.run   = blockdev_amend_run,
> > +};
> > +
> > +void qmp_x_blockdev_amend(const char *job_id,
> > +const char *node_name,
> > +BlockdevCreateOptions *options,
> > +bool has_force,
> > +bool force,
> > +Error **errp)
> > +{
> > +BlockdevAmendJob *s;
> > +const char *fmt = BlockdevDriver_str(options->driver);
> > +BlockDriver *drv = bdrv_find_format(fmt);
> > +

Re: [RFC PATCH 18/18] qemu-storage-daemon: Add --monitor option

[Markus Armbruster]

Quick observation: --help fails to mention --monitor.

[PATCH] iotests: Fix "no qualified output" error path

[Kevin Wolf]

The variable for error messages to be displayed is $results, not
$reason. Fix 'check' to print the "no qualified output" error message
again instead of having a failure without any message telling the user
why it failed.

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/check | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 588c453a94..5218728470 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -876,7 +876,7 @@ do
 if [ ! -f "$reference" ]
 then
 status="fail"
-reason="no qualified output"
+results="no qualified output"
 err=true
 else
 if diff -w "$reference" $tmp.out >/dev/null 2>&1
-- 
2.20.1

[PATCH 2/2] iotests: Test multiple blockdev-snapshot calls

[Kevin Wolf]

Test that doing a second blockdev-snapshot doesn't make the first
overlay's backing file go away.

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/273 |  76 +
 tests/qemu-iotests/273.out | 337 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 414 insertions(+)
 create mode 100755 tests/qemu-iotests/273
 create mode 100644 tests/qemu-iotests/273.out

diff --git a/tests/qemu-iotests/273 b/tests/qemu-iotests/273
new file mode 100755
index 00..60076de7ce
--- /dev/null
+++ b/tests/qemu-iotests/273
@@ -0,0 +1,76 @@
+#!/usr/bin/env bash
+#
+# Test large write to a qcow2 image
+#
+# Copyright (C) 2019 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+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
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+# This is a qcow2 regression test
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+do_run_qemu()
+{
+echo Testing: "$@"
+$QEMU -nographic -qmp-pretty stdio -nodefaults "$@"
+echo
+}
+
+run_qemu()
+{
+do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qemu | _filter_qmp |
+_filter_generated_node_ids | _filter_imgfmt | _filter_actual_image_size
+}
+
+TEST_IMG="$TEST_IMG.base" _make_test_img 64M
+TEST_IMG="$TEST_IMG.mid" _make_test_img -b "$TEST_IMG.base"
+_make_test_img -b "$TEST_IMG.mid"
+
+run_qemu \
+-blockdev file,node-name=base,filename="$TEST_IMG.base" \
+ -blockdev file,node-name=midf,filename="$TEST_IMG.mid" \
+ -blockdev 
'{"driver":"qcow2","node-name":"mid","file":"midf","backing":null}' \
+ -blockdev file,node-name=topf,filename="$TEST_IMG" \
+ -blockdev 
'{"driver":"qcow2","file":"topf","node-name":"top","backing":null}' \
+<

[PATCH 0/2] block: Fix multiple blockdev-snapshot calls

[Kevin Wolf]

Kevin Wolf (2):
  block: Remove 'backing': null from bs->{explicit_,}options
  iotests: Test multiple blockdev-snapshot calls

 block.c|   2 +
 tests/qemu-iotests/273 |  76 +
 tests/qemu-iotests/273.out | 337 +
 tests/qemu-iotests/group   |   1 +
 4 files changed, 416 insertions(+)
 create mode 100755 tests/qemu-iotests/273
 create mode 100644 tests/qemu-iotests/273.out

-- 
2.20.1

[PATCH 1/2] block: Remove 'backing': null from bs->{explicit_, }options

[Kevin Wolf]

bs->options and bs->explicit_options shouldn't contain any options for
child nodes. bdrv_open_inherited() takes care to remove any options that
match a child name after opening the image and the same is done when
reopening.

However, we miss the case of 'backing': null, which is a child option,
but results in no child being created. This means that a 'backing': null
remains in bs->options and bs->explicit_options.

A typical use for 'backing': null is in live snapshots: blockdev-add for
the qcow2 overlay makes sure not to open the backing file (because it is
already opened and blockdev-snapshot will attach it). After doing a
blockdev-snapshot, bs->options and bs->explicit_options become
inconsistent with the actual state (bs has a backing file now, but the
options still say null). On the next occasion that the image is
reopened, e.g. switching it from read-write to read-only when another
snapshot is taken, the option will take effect again and the node
incorrectly loses its backing file.

Fix bdrv_open_inherited() to remove the 'backing' option from
bs->options and bs->explicit_options even for the case where it
specifies that no backing file is wanted.

Reported-by: Peter Krempa 
Signed-off-by: Kevin Wolf 
---
 block.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block.c b/block.c
index dad5a3d8e0..74ba9acb08 100644
--- a/block.c
+++ b/block.c
@@ -3019,6 +3019,8 @@ static BlockDriverState *bdrv_open_inherit(const char 
*filename,
 "use \"backing\": null instead");
 }
 flags |= BDRV_O_NO_BACKING;
+qdict_del(bs->explicit_options, "backing");
+qdict_del(bs->options, "backing");
 qdict_del(options, "backing");
 }
 
-- 
2.20.1

Re: [Qemu-devel] Exposing feature deprecation to machine clients

[Max Reitz]

On 07.11.19 20:13, Vladimir Sementsov-Ogievskiy wrote:
> 07.11.2019 21:52, Philippe Mathieu-Daudé wrote:
>> Hi Markus,
>>
>> On 8/15/19 7:40 PM, John Snow wrote:
>>> On 8/15/19 10:16 AM, Markus Armbruster wrote:
 John Snow  writes:
>> [...]
> I asked Markus this not too long ago; do we want to amend the QAPI
> schema specification to allow commands to return with "Warning" strings,
> or "Deprecated" stings to allow in-band deprecation notices for cases
> like these?
>
> example:
>
> { "return": {},
>    "deprecated": True,
>    "warning": "Omitting filter-node-name parameter is deprecated, it will
> be required in the future"
> }
>
> There's no "error" key, so this should be recognized as success by
> compatible clients, but they'll definitely see the extra information.

 This is a compatible evolution of the QMP protocol.

> Part of my motivation is to facilitate a more aggressive deprecation of
> legacy features by ensuring that we are able to rigorously notify users
> through any means that they need to adjust their scripts.

 Yes, we should help libvirt etc. with detecting use of deprecated
 features.  We discussed this at the KVM Forum 2018 BoF on deprecating
 stuff.  Minutes:

  Message-ID: <87mur0ls8o@dusky.pond.sub.org>
  https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg05828.html

 Last item is relevant here.

 Adding deprecation information to QMP's success response belongs to "We
 can also pass the buck to the next layer up", next to "emit a QMP
 event".

 Let's compare the two, i.e. "deprecation info in success response"
 vs. "deprecation event".

 1. Possible triggers

 Anything we put in the success response should only ever apply to the
 (successful) command.  So this one's limited to QMP commands.

 A QMP event is not limited to QMP commands.  For instance, it could be
 emitted for deprecated CLI features (long after the fact, in addition to
 human-readable warnings on stderr), or when we detect use of a
 deprecated feature only after we sent the success response, say in a
 job.  Neither use case is particularly convincing.  Reporting use of
 deprecated CLI in QMP feels like a work-around for the CLI's
 machine-unfriendliness.  Job-like commands should really check their
 arguments upfront.

 2. Connection to trigger

 Connecting responses to commands is the QMP protocol's responsibility.
 Transmitting deprecation information in the response trivially ties it
 to the offending command.

 The QMP protocol doesn't tie events to anything.  Thus, a deprecation
 event needs an event-specific tie to its trigger.

 The obvious way to tie it to a command mirrors how the QMP protocol ties
 responses to commands: by command ID.  The event either has to be sent
 just to the offending monitor (currently, all events are broadcast to
 all monitors), or include a suitable monitor ID.

 For non-command triggers, we'd have to invent some other tie.

 3. Interface complexity

 Tying the event to some arbitrary trigger adds complexity.

 Do we need non-command triggers, and badly enough to justify the
 additional complexity?

 4. Implementation complexity

 Emitting an event could be as simple as

  qapi_event_send_deprecated(qmp_command_id(),
     "Omitting 'filter-node-name'");

 where qmp_command_id() returns the ID of the currently executing
 command.  Making qmp_command_id() work is up to the QMP core.  Simple
 enough as long as each QMP command runs to completion before its monitor
 starts the next one.

 The event is "fire and forget".  There is no warning object propagated
 up the call chain into the QMP core like errors objects are.

 "Fire and forget" is ideal for letting arbitrary code decide "this is
 deprecated".

 Note the QAPI schema remains untouched.

 Unlike an event, which can be emitted anywhere, the success response
 gets built in the QMP core.  To have the core add deprecation info to
 it, we need to get the info to the core.

 If deprecation info originates in command code, like errors do, we need
 to propagate it up the call chain into the QMP core like errors.

 Propagating errors is painful.  It has caused massive churn all over the
 place.

 I don't think we can hitch deprecation info to the existing error
 propagation, since we need to take the success path back to the QMP
 core, not an error path.

 Propagating a second object for warnings... thanks, but no thanks.

>>>
>>> Probably the best argument against it. Fire-and-forget avoids the
>>> problem. Events might work just fine, but