Re: [PATCH 2/4] migration: Add block-bitmap-mapping parameter

2020-07-02 Thread Dr. David Alan Gilbert
* Max Reitz (mre...@redhat.com) wrote: > On 30.06.20 12:51, Dr. David Alan Gilbert wrote: > > * Max Reitz (mre...@redhat.com) wrote: > >> This migration parameter allows mapping block node names and bitmap > >> names to aliases for the purpose of block dirty bitmap migration. > >> > >> This way,

Re: [PATCH v9 30/34] qcow2: Add prealloc field to QCowL2Meta

2020-07-02 Thread Max Reitz
On 02.07.20 16:58, Eric Blake wrote: > On 7/2/20 9:50 AM, Max Reitz wrote: >> On 28.06.20 13:02, Alberto Garcia wrote: >>> This field allows us to indicate that the L2 metadata update does not >>> come from a write request with actual data but from a preallocation >>> request. >>> >>> For

Re: [PATCH 07/46] error: Avoid more error_propagate() when error is not used here

2020-07-02 Thread Markus Armbruster
Markus Armbruster writes: > Vladimir Sementsov-Ogievskiy writes: > >> 24.06.2020 19:43, Markus Armbruster wrote: >>> When all we do with an Error we receive into a local variable is >>> propagating to somewhere else, we can just as well receive it there >>> right away. The previous commit did

Re: [PATCH 0/4] migration: Add block-bitmap-mapping parameter

2020-07-02 Thread Vladimir Sementsov-Ogievskiy
Hmm, seems, you didn't use scripts/get_maintainer.pl, as neither Eric nor John are in Cc. Add them. 30.06.2020 11:45, Max Reitz wrote: RFC v1: https://lists.nongnu.org/archive/html/qemu-block/2020-05/msg00912.html RFC v2: https://lists.nongnu.org/archive/html/qemu-block/2020-05/msg00915.html

Re: [PATCH v9 23/34] qcow2: Add subcluster support to discard_in_l2_slice()

2020-07-02 Thread Max Reitz
On 28.06.20 13:02, Alberto Garcia wrote: > Two things need to be taken into account here: > > 1) With full_discard == true the L2 entry must be cleared completely. >This also includes the L2 bitmap if the image has extended L2 >entries. > > 2) With full_discard == false we have to make

Re: [PATCH v9 24/34] qcow2: Add subcluster support to check_refcounts_l2()

2020-07-02 Thread Max Reitz
On 28.06.20 13:02, Alberto Garcia wrote: > The offset field of an uncompressed cluster's L2 entry must be aligned > to the cluster size, otherwise it is invalid. If the cluster has no > data then it means that the offset points to a preallocation, so we > can clear the offset field without

Re: [PATCH 03/46] qdev: Smooth error checking of qdev_realize() & friends

2020-07-02 Thread Markus Armbruster
Markus Armbruster writes: > Vladimir Sementsov-Ogievskiy writes: > >> 24.06.2020 19:43, Markus Armbruster wrote: >>> Convert >>> >>> foo(..., ); >>> if (err) { >>> ... >>> } >>> >>> to >>> >>> if (!foo(..., )) { >>> ... >>> } >>> >>> for

Re: [PATCH v9 21/34] qcow2: Add subcluster support to qcow2_get_host_offset()

2020-07-02 Thread Max Reitz
On 28.06.20 13:02, Alberto Garcia wrote: > The logic of this function remains pretty much the same, except that > it uses count_contiguous_subclusters(), which combines the logic of > count_contiguous_clusters() / count_contiguous_clusters_unallocated() > and checks individual subclusters. > >

Re: [PATCH v9 22/34] qcow2: Add subcluster support to zero_in_l2_slice()

2020-07-02 Thread Max Reitz
On 28.06.20 13:02, Alberto Garcia wrote: > The QCOW_OFLAG_ZERO bit that indicates that a cluster reads as > zeroes is only used in standard L2 entries. Extended L2 entries use > individual 'all zeroes' bits for each subcluster. > > This must be taken into account when updating the L2 entry and

[PATCH v2 1/3] util: validate whether O_DIRECT is supported after failure

2020-07-02 Thread Daniel P . Berrangé
Currently we suggest that a filesystem may not support O_DIRECT after seeing an EINVAL. Other things can cause EINVAL though, so it is better to do an explicit check, and then report a definitive error message. Signed-off-by: Daniel P. Berrangé --- util/osdep.c | 15 +++ 1 file

[PATCH v2 2/3] util: support detailed error reporting for qemu_open

2020-07-02 Thread Daniel P . Berrangé
Create a "qemu_open_err" method which does the same as "qemu_open", but with a "Error **errp" for error reporting. There should be no behavioural difference for existing callers at this stage. Signed-off-by: Daniel P. Berrangé --- include/qemu/osdep.h | 2 ++ util/osdep.c | 66

[PATCH v2 0/3] block: improve error reporting for unsupported O_DIRECT

2020-07-02 Thread Daniel P . Berrangé
v1: https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg00269.html See patch commit messages for rationale Ideally we would convert other callers of qemu_open to use qemu_open_err, and eventually remove qemu_open, renaming qemu_open_err back to qemu_open. Given soft freeze is just days

Re: [PATCH v9 30/34] qcow2: Add prealloc field to QCowL2Meta

2020-07-02 Thread Eric Blake
On 7/2/20 9:50 AM, Max Reitz wrote: On 28.06.20 13:02, Alberto Garcia wrote: This field allows us to indicate that the L2 metadata update does not come from a write request with actual data but from a preallocation request. For traditional images this does not make any difference, but for

Re: [PATCH v9 20/34] qcow2: Add subcluster support to calculate_l2_meta()

2020-07-02 Thread Max Reitz
On 28.06.20 13:02, Alberto Garcia 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: > > 1) If we are writing to a newly allocated cluster

[PATCH v2 3/3] block: switch to use qemu_open_err for improved errors

2020-07-02 Thread Daniel P . Berrangé
Currently at startup if using cache=none on a filesystem lacking O_DIRECT such as tmpfs, at startup QEMU prints qemu-system-x86_64: -drive file=/tmp/foo.img,cache=none: file system may not support O_DIRECT qemu-system-x86_64: -drive file=/tmp/foo.img,cache=none: Could not open '/tmp/foo.img':

Re: [PATCH v9 28/34] qcow2: Add subcluster support to qcow2_co_pwrite_zeroes()

2020-07-02 Thread Max Reitz
On 28.06.20 13:02, Alberto Garcia wrote: > This works now at the subcluster level and pwrite_zeroes_alignment is > updated accordingly. > > qcow2_cluster_zeroize() is turned into qcow2_subcluster_zeroize() with > the following changes: > >- The request can now be subcluster-aligned. > >

Re: [PATCH 33/46] qom: Crash more nicely on object_property_get_link() failure

2020-07-02 Thread Markus Armbruster
Markus Armbruster writes: > Eric Blake writes: > >> On 6/24/20 11:43 AM, Markus Armbruster wrote: >>> Pass _abort instead of NULL where the returned value is >>> dereferenced or asserted to be non-null. >>> >>> Signed-off-by: Markus Armbruster >>> --- >> >>> @@ -63,8 +64,8 @@ hwaddr

Re: [PATCH v9 31/34] qcow2: Add the 'extended_l2' option and the QCOW2_INCOMPAT_EXTL2 bit

2020-07-02 Thread Max Reitz
On 28.06.20 13:02, Alberto Garcia wrote: > Now that the implementation of subclusters is complete we can finally > add the necessary options to create and read images with this feature, > which we call "extended L2 entries". > > Signed-off-by: Alberto Garcia > Reviewed-by: Eric Blake > --- >

Re: [PATCH 2/4] migration: Add block-bitmap-mapping parameter

2020-07-02 Thread Vladimir Sementsov-Ogievskiy
02.07.2020 12:41, Max Reitz wrote: As I've said before, it may be reasonable to ignore bitmaps not referenced in the hash-table. No problem with that.  We just decided on this behavior when we discussed the RFC. Sorry for that. The reason for my changed opinion is a recent bug from customers

Re: [PATCH v9 25/34] qcow2: Update L2 bitmap in qcow2_alloc_cluster_link_l2()

2020-07-02 Thread Max Reitz
On 28.06.20 13:02, Alberto Garcia wrote: > The L2 bitmap needs to be updated after each write to indicate what > new subclusters are now allocated. This needs to happen even if the > cluster was already allocated and the L2 entry was otherwise valid. > > In some cases however a write operation

Re: [PATCH v9 30/34] qcow2: Add prealloc field to QCowL2Meta

2020-07-02 Thread Max Reitz
On 28.06.20 13:02, Alberto Garcia wrote: > This field allows us to indicate that the L2 metadata update does not > come from a write request with actual data but from a preallocation > request. > > For traditional images this does not make any difference, but for > images with extended L2 entries

Re: [PATCH v4 2/2] nvme: allow cmb and pmr to be enabled on same device

2020-07-02 Thread Andrzej Jakowski
On 7/2/20 3:31 AM, Klaus Jensen wrote: > Aight, an update here. This only happens when QEMU is run with a virtual > IOMMU. Otherwise, the kernel is happy. > > With the vIOMMU, qemu also craps out a bit: > > qemu-system-x86_64: vtd_iova_to_slpte: detected slpte permission error >

[PATCH v2 09/44] qemu-option: Simplify around find_default_by_name()

2020-07-02 Thread Markus Armbruster
Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy --- util/qemu-option.c | 18 +- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/util/qemu-option.c b/util/qemu-option.c index 14e211ddd8..e7b540a21b 100644 ---

[PATCH v2 01/44] error: Improve examples in error.h's big comment

2020-07-02 Thread Markus Armbruster
Show errp instead of where is actually unusual. Add a missing declaration. Add a second error pileup example. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Greg Kurz --- include/qapi/error.h | 19 +++ 1

[PATCH v2 24/44] qom: Don't handle impossible object_property_get_link() failure

2020-07-02 Thread Markus Armbruster
Don't handle object_property_get_link() failure that can't happen unless the programmer screwed up, pass _abort. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Philippe Mathieu-Daudé --- hw/arm/bcm2835_peripherals.c | 7 +-- hw/arm/bcm2836.c | 7

[PATCH v2 31/44] qdev: Make functions taking Error ** return bool, not void

2020-07-02 Thread Markus Armbruster
See recent commit "error: Document Error API usage rules" for rationale. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- include/hw/qdev-properties.h | 4 ++-- hw/core/qdev-properties-system.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git

[PATCH v2 16/44] qapi: Make visitor functions taking Error ** return bool, not void

2020-07-02 Thread Markus Armbruster
See recent commit "error: Document Error API usage rules" for rationale. Signed-off-by: Markus Armbruster --- docs/devel/qapi-code-gen.txt | 51 +-- include/qapi/clone-visitor.h | 8 +- include/qapi/visitor-impl.h | 26 +++--- include/qapi/visitor.h| 102

[PATCH 2/6] migration: introduce savevm, loadvm, delvm QMP commands

2020-07-02 Thread Daniel P . Berrangé
savevm, loadvm and delvm are some of the few commands that have never been converted to use QMP. The primary reason for this lack of conversion is that they block execution of the thread for as long as they run. Despite this downside, however, libvirt and applications using libvirt has used these

[PATCH 5/6] migration: support excluding block devs in QMP snapshot commands

2020-07-02 Thread Daniel P . Berrangé
This wires up support for a new "exclude" parameter to the QMP commands for snapshots (savevm, loadvm, delvm). This parameter accepts a list of block driver state node names. One use case for this would be a VM using OVMF firmware where the variables store is a raw disk image. Ideally the

Re: [PATCH 2/6] migration: introduce savevm, loadvm, delvm QMP commands

2020-07-02 Thread Daniel P . Berrangé
On Thu, Jul 02, 2020 at 01:12:52PM -0500, Eric Blake wrote: > On 7/2/20 12:57 PM, Daniel P. Berrangé wrote: > > savevm, loadvm and delvm are some of the few commands that have never > > been converted to use QMP. The primary reason for this lack of > > conversion is that they block execution of

Re: [PATCH v2 37/44] error: Reduce unnecessary error propagation

2020-07-02 Thread Eric Blake
On 7/2/20 10:49 AM, Markus Armbruster wrote: When all we do with an Error we receive into a local variable is propagating to somewhere else, we can just as well receive it there right away, even when we need to keep error_propagate() for other error paths. Signed-off-by: Markus Armbruster ---

Re: [PATCH 1/6] migration: improve error reporting of block driver state name

2020-07-02 Thread Eric Blake
On 7/2/20 12:57 PM, Daniel P. Berrangé wrote: With blockdev, a BlockDriverState may not have an device name, s/an/a/ so using a node name is required as an alternative. Signed-off-by: Daniel P. Berrangé --- migration/savevm.c | 12 ++-- 1 file changed, 6 insertions(+), 6

[PATCH v2 15/44] hmp: Eliminate a variable in hmp_migrate_set_parameter()

2020-07-02 Thread Markus Armbruster
Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- monitor/hmp-cmds.c | 8 ++-- 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c index 2b0b58a336..d7810cb564 100644 --- a/monitor/hmp-cmds.c +++ b/monitor/hmp-cmds.c @@ -1247,7

[PATCH v2 35/44] error: Eliminate error_propagate() with Coccinelle, part 2

2020-07-02 Thread Markus Armbruster
When all we do with an Error we receive into a local variable is propagating to somewhere else, we can just as well receive it there right away. The previous commit did that with a Coccinelle script I consider fairly trustworthy. This commit uses the same script with the matching of return taken

[PATCH v2 42/44] qemu-img: Ignore Error objects where the return value suffices

2020-07-02 Thread Markus Armbruster
Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- qemu-img.c | 8 ++-- 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/qemu-img.c b/qemu-img.c index 27bf94e7ae..c11bfe0268 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -464,22 +464,18 @@ static int

Re: [PATCH v2 23/44] qom: Crash more nicely on object_property_get_link() failure

2020-07-02 Thread Eric Blake
On 7/2/20 10:49 AM, Markus Armbruster wrote: Pass _abort instead of NULL where the returned value is dereferenced or asserted to be non-null. Drop a now redundant assertion. Signed-off-by: Markus Armbruster --- hw/core/platform-bus.c | 6 +++--- Reviewed-by: Eric Blake -- Eric Blake,

Re: [PATCH v2 36/44] error: Eliminate error_propagate() manually

2020-07-02 Thread Eric Blake
On 7/2/20 10:49 AM, Markus Armbruster wrote: When all we do with an Error we receive into a local variable is propagating to somewhere else, we can just as well receive it there right away. The previous two commits did that for sufficiently simple cases with Coccinelle. Do it for several more

Re: [PATCH 0/6] migration: bring savevm/loadvm/delvm over to QMP

2020-07-02 Thread no-reply
Patchew URL: https://patchew.org/QEMU/20200702175754.2211821-1-berra...@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

Re: [PATCH 0/6] migration: bring savevm/loadvm/delvm over to QMP

2020-07-02 Thread no-reply
Patchew URL: https://patchew.org/QEMU/20200702175754.2211821-1-berra...@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

[PATCH v2 00/44] Less clumsy error checking

2020-07-02 Thread Markus Armbruster
When the Error API was created, we adopted the (unwritten) rule to return void when the function returns no useful value on success, unlike GError, which recommends to return true on success and false on error then. When a function returns a distinct error value, say false, a checked call that

[PATCH v2 44/44] hmp: Ignore Error objects where the return value suffices

2020-07-02 Thread Markus Armbruster
qdev_print_props() receives and throws away Error objects just to check for object_property_get_str() and object_property_print() failure. Unnecessary, both return suitable values, so use those instead. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- qdev-monitor.c | 12

[PATCH v2 22/44] qom: Rename qdev_get_type() to object_get_type()

2020-07-02 Thread Markus Armbruster
Commit 2f262e06f0 lifted qdev_get_type() from qdev to object without renaming it accordingly. Do that now. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Philippe Mathieu-Daudé --- qom/object.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git

[PATCH v2 27/44] qom: Make functions taking Error ** return bool, not void

2020-07-02 Thread Markus Armbruster
See recent commit "error: Document Error API usage rules" for rationale. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- include/qom/object.h| 42 ++ include/qom/object_interfaces.h | 12 +++- include/qom/qom-qobject.h | 4 +- qom/object.c

[PATCH v2 30/44] qom: Make functions taking Error ** return bool, not 0/-1

2020-07-02 Thread Markus Armbruster
Just for consistency. Also fix the example in object_set_props()'s documentation. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- include/qom/object.h | 28 +++- qom/object.c | 14 +++--- 2 files changed, 18 insertions(+), 24 deletions(-)

Re: [PATCH v2 14/44] block: Avoid error accumulation in bdrv_img_create()

2020-07-02 Thread Eric Blake
On 7/2/20 10:49 AM, Markus Armbruster wrote: When creating an image fails because the format doesn't support option "backing_file" or "backing_fmt", bdrv_img_create() first has qemu_opt_set() put a generic error into @local_err, then puts the real error into @errp with error_setg(), and then

Re: [PATCH v2 16/44] qapi: Make visitor functions taking Error ** return bool, not void

2020-07-02 Thread Eric Blake
On 7/2/20 10:49 AM, Markus Armbruster wrote: See recent commit "error: Document Error API usage rules" for rationale. Signed-off-by: Markus Armbruster --- Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org |

Re: [PATCH v2 1/3] util: validate whether O_DIRECT is supported after failure

2020-07-02 Thread Philippe Mathieu-Daudé
On 7/2/20 2:56 PM, Daniel P. Berrangé wrote: > Currently we suggest that a filesystem may not support O_DIRECT after > seeing an EINVAL. Other things can cause EINVAL though, so it is better > to do an explicit check, and then report a definitive error message. > > Signed-off-by: Daniel P.

[PATCH v2 03/44] qdev: Use returned bool to check for qdev_realize() etc. failure

2020-07-02 Thread Markus Armbruster
Convert foo(..., ); if (err) { ... } to if (!foo(..., )) { ... } for qdev_realize(), qdev_realize_and_unref(), qbus_realize() and their wrappers isa_realize_and_unref(), pci_realize_and_unref(), sysbus_realize(), sysbus_realize_and_unref(),

[PATCH v2 13/44] qemu-option: Use returned bool to check for failure

2020-07-02 Thread Markus Armbruster
The previous commit enables conversion of foo(..., ); if (err) { ... } to if (!foo(..., )) { ... } for QemuOpts functions that now return true / false on success / error. Coccinelle script: @@ identifier fun = {opts_do_parse, parse_option_bool,

[PATCH v2 06/44] qemu-option: Check return value instead of @err where convenient

2020-07-02 Thread Markus Armbruster
Convert uses like opts = qemu_opts_create(..., ); if (err) { ... } to opts = qemu_opts_create(..., ); if (!opts) { ... } Eliminate error_propagate() that are now unnecessary. Delete @err that are now unused. Signed-off-by: Markus Armbruster

[PATCH v2 40/44] qapi: Purge error_propagate() from QAPI core

2020-07-02 Thread Markus Armbruster
Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- qapi/qapi-visit-core.c | 40 +++- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c index 5a9c47aabf..7e5f40e7f0 100644 ---

Re: [PATCH v2 28/44] qom: Use returned bool to check for failure, Coccinelle part

2020-07-02 Thread Eric Blake
On 7/2/20 10:49 AM, Markus Armbruster wrote: The previous commit enables conversion of foo(..., ); if (err) { ... } to if (!foo(..., errp)) { ... } for QOM functions that now return true / false on success / error. Coccinelle script: @@

Re: [PATCH v2 29/44] qom: Use returned bool to check for failure, manual part

2020-07-02 Thread Eric Blake
On 7/2/20 10:49 AM, Markus Armbruster wrote: The previous commit used Coccinelle to convert from checking the Error object to checking the return value. Convert a few more manually. Signed-off-by: Markus Armbruster --- hw/core/bus.c | 6 +- hw/core/qdev.c | 7

Re: [PATCH v2 32/44] qdev: Use returned bool to check for failure, Coccinelle part

2020-07-02 Thread Eric Blake
On 7/2/20 10:49 AM, Markus Armbruster wrote: The previous commit enables conversion of qdev_prop_set_drive_err(..., ); if (err) { ... } to if (!qdev_prop_set_drive_err(..., errp)) { ... } Coccinelle script: Reviewed-by: Eric Blake -- Eric Blake,

Re: [PATCH 6/6] migration: support picking vmstate disk in QMP snapshot commands

2020-07-02 Thread Eric Blake
On 7/2/20 12:57 PM, Daniel P. Berrangé wrote: This wires up support for a new "vmstate" parameter to the QMP commands for snapshots (savevm, loadvm). This parameter accepts block driver state node name. One use case for this would be a VM using OVMF firmware where the variables store is the

Re: [PATCH v2 03/44] qdev: Use returned bool to check for qdev_realize() etc. failure

2020-07-02 Thread Vladimir Sementsov-Ogievskiy
02.07.2020 18:49, Markus Armbruster wrote: Convert foo(..., ); if (err) { ... } to if (!foo(..., )) { ... } for qdev_realize(), qdev_realize_and_unref(), qbus_realize() and their wrappers isa_realize_and_unref(), pci_realize_and_unref(),

[PATCH v2 20/44] s390x/pci: Fix harmless mistake in zpci's property fid's setter

2020-07-02 Thread Markus Armbruster
s390_pci_set_fid() sets zpci->fid_defined to true even when visit_type_uint32() failed. Reproducer: "-device zpci,fid=junk". Harmless in practice, because qdev_device_add() then fails, throwing away @zpci. Fix it anyway. Cc: Matthew Rosato Cc: Cornelia Huck Signed-off-by: Markus Armbruster

[PATCH v2 12/44] qemu-option: Make functions taking Error ** return bool, not void

2020-07-02 Thread Markus Armbruster
See recent commit "error: Document Error API usage rules" for rationale. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy --- include/qemu/option.h | 16 blockdev.c| 5 ++- util/qemu-option.c| 92

[PATCH v2 38/44] qapi: Smooth another visitor error checking pattern

2020-07-02 Thread Markus Armbruster
Convert visit_type_FOO(v, ..., , ); ... if (err) { ... } to visit_type_FOO(v, ..., , errp); ... if (!ptr) { ... } for functions that set @ptr to non-null / null on success / error. Eliminate error_propagate() that are now unnecessary. Delete

Re: [PATCH v2 03/44] qdev: Use returned bool to check for qdev_realize() etc. failure

2020-07-02 Thread Eric Blake
On 7/2/20 10:49 AM, Markus Armbruster wrote: Convert foo(..., ); if (err) { ... } to if (!foo(..., )) { ... } for qdev_realize(), qdev_realize_and_unref(), qbus_realize() and their wrappers isa_realize_and_unref(), pci_realize_and_unref(),

Re: [PATCH v2 00/44] Less clumsy error checking

2020-07-02 Thread no-reply
Patchew URL: https://patchew.org/QEMU/20200702155000.3455325-1-arm...@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

Re: [PATCH v2 17/44] qapi: Use returned bool to check for failure, Coccinelle part

2020-07-02 Thread Eric Blake
On 7/2/20 10:49 AM, Markus Armbruster wrote: The previous commit enables conversion of visit_foo(..., ); if (err) { ... } to if (!visit_foo(..., errp)) { ... } for visitor functions that now return true / false on success / error. Coccinelle script:

Re: [PATCH v2 33/44] error: Avoid unnecessary error_propagate() after error_setg()

2020-07-02 Thread Eric Blake
On 7/2/20 10:49 AM, Markus Armbruster wrote: Replace error_setg(, ...); error_propagate(errp, err); by error_setg(errp, ...); Candidates for conversion tracked down with this Coccinelle script: @@ identifier err, errp; expression list args; @@ -

Re: [PATCH v2 35/44] error: Eliminate error_propagate() with Coccinelle, part 2

2020-07-02 Thread Eric Blake
On 7/2/20 10:49 AM, Markus Armbruster wrote: When all we do with an Error we receive into a local variable is propagating to somewhere else, we can just as well receive it there right away. The previous commit did that with a Coccinelle script I consider fairly trustworthy. This commit uses

[PATCH v2 08/44] qemu-option: Factor out helper find_default_by_name()

2020-07-02 Thread Markus Armbruster
Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy --- util/qemu-option.c | 47 ++ 1 file changed, 27 insertions(+), 20 deletions(-) diff --git a/util/qemu-option.c b/util/qemu-option.c index

[PATCH v2 21/44] qom: Use error_reportf_err() instead of g_printerr() in examples

2020-07-02 Thread Markus Armbruster
Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- include/qom/object.h | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/include/qom/object.h b/include/qom/object.h index 94a61ccc3f..b70edd8cd9 100644 --- a/include/qom/object.h +++ b/include/qom/object.h @@

[PATCH v2 02/44] error: Document Error API usage rules

2020-07-02 Thread Markus Armbruster
This merely codifies existing practice, with one exception: the rule advising against returning void, where existing practice is mixed. When the Error API was created, we adopted the (unwritten) rule to return void when the function returns no useful value on success, unlike GError, which

[PATCH v2 17/44] qapi: Use returned bool to check for failure, Coccinelle part

2020-07-02 Thread Markus Armbruster
The previous commit enables conversion of visit_foo(..., ); if (err) { ... } to if (!visit_foo(..., errp)) { ... } for visitor functions that now return true / false on success / error. Coccinelle script: @@ identifier fun =~

[PATCH v2 36/44] error: Eliminate error_propagate() manually

2020-07-02 Thread Markus Armbruster
When all we do with an Error we receive into a local variable is propagating to somewhere else, we can just as well receive it there right away. The previous two commits did that for sufficiently simple cases with Coccinelle. Do it for several more manually. Signed-off-by: Markus Armbruster

[PATCH v2 43/44] qdev: Ignore Error objects where the return value suffices

2020-07-02 Thread Markus Armbruster
Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- hw/core/qdev-properties.c | 5 + 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c index e1ad147339..8eb4283a56 100644 --- a/hw/core/qdev-properties.c +++

[PATCH v2 23/44] qom: Crash more nicely on object_property_get_link() failure

2020-07-02 Thread Markus Armbruster
Pass _abort instead of NULL where the returned value is dereferenced or asserted to be non-null. Drop a now redundant assertion. Signed-off-by: Markus Armbruster --- hw/core/platform-bus.c | 6 +++--- hw/ppc/spapr_drc.c | 3 ++- hw/ppc/spapr_hcall.c | 3 ++-

[PATCH v2 39/44] qapi: Smooth visitor error checking in generated code

2020-07-02 Thread Markus Armbruster
Use visitor functions' return values to check for failure. Eliminate error_propagate() that are now unnecessary. Delete @err that are now unused. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- docs/devel/qapi-code-gen.txt | 60 ++--

Re: [PATCH 2/6] migration: introduce savevm, loadvm, delvm QMP commands

2020-07-02 Thread Eric Blake
On 7/2/20 12:57 PM, Daniel P. Berrangé wrote: savevm, loadvm and delvm are some of the few commands that have never been converted to use QMP. The primary reason for this lack of conversion is that they block execution of the thread for as long as they run. Despite this downside, however,

Re: [PATCH 1/6] migration: improve error reporting of block driver state name

2020-07-02 Thread Dr. David Alan Gilbert
* Eric Blake (ebl...@redhat.com) wrote: > On 7/2/20 12:57 PM, Daniel P. Berrangé wrote: > > With blockdev, a BlockDriverState may not have an device name, > > s/an/a/ > > > so using a node name is required as an alternative. > > > > Signed-off-by: Daniel P. Berrangé > > --- > >

[PATCH v2 04/44] macio: Tidy up error handling in macio_newworld_realize()

2020-07-02 Thread Markus Armbruster
macio_newworld_realize() effectively ignores ns->gpio realization errors, leaking the Error object. Fortunately, macio_gpio_realize() can't actually fail. Tidy up. Cc: Mark Cave-Ayland Cc: David Gibson Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Acked-by: David Gibson

[PATCH v2 11/44] qemu-option: Replace opt_set() by cleaner opt_validate()

2020-07-02 Thread Markus Armbruster
opt_set() frees its argument @value on failure. Slightly unclean; functions ideally do nothing on failure. To tidy this up, move opt_create() from opt_set() into its callers, along with the cleanup. Rename opt_set() to opt_validate(), noting its similarity to qemu_opts_validate(). Drop

[PATCH v2 05/44] virtio-crypto-pci: Tidy up virtio_crypto_pci_realize()

2020-07-02 Thread Markus Armbruster
virtio_crypto_pci_realize() continues after realization of its "virtio-crypto-device" fails. Only an object_property_set_link() follows; looks harmless to me. Tidy up anyway: return after failure, just like virtio_rng_pci_realize() does. Cc: "Gonglei (Arei)" Cc: Michael S. Tsirkin

[PATCH v2 41/44] error: Avoid error_propagate() after migrate_add_blocker()

2020-07-02 Thread Markus Armbruster
When migrate_add_blocker(blocker, ) is followed by error_propagate(errp, err), we can often just as well do migrate_add_blocker(..., errp). Do that with this Coccinelle script: @@ expression blocker, err, errp; expression ret; @@ -ret = migrate_add_blocker(blocker, );

[PATCH v2 25/44] qom: Use return values to check for error where that's simpler

2020-07-02 Thread Markus Armbruster
When using the Error object to check for error, we need to receive it into a local variable, then propagate() it to @errp. Using the return value permits allows receiving it straight to @errp. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- qom/object.c | 16 +--- 1

[PATCH v2 29/44] qom: Use returned bool to check for failure, manual part

2020-07-02 Thread Markus Armbruster
The previous commit used Coccinelle to convert from checking the Error object to checking the return value. Convert a few more manually. Signed-off-by: Markus Armbruster --- hw/core/bus.c | 6 +- hw/core/qdev.c | 7 +-- hw/s390x/s390-virtio-ccw.c | 13

[PATCH v2 14/44] block: Avoid error accumulation in bdrv_img_create()

2020-07-02 Thread Markus Armbruster
When creating an image fails because the format doesn't support option "backing_file" or "backing_fmt", bdrv_img_create() first has qemu_opt_set() put a generic error into @local_err, then puts the real error into @errp with error_setg(), and then propagates the former to the latter, which throws

[PATCH v2 19/44] block/parallels: Simplify parallels_open() after previous commit

2020-07-02 Thread Markus Armbruster
Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- block/parallels.c | 7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/block/parallels.c b/block/parallels.c index 32d0ecd398..e0ec819550 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -843,6 +843,7 @@

[PATCH v2 33/44] error: Avoid unnecessary error_propagate() after error_setg()

2020-07-02 Thread Markus Armbruster
Replace error_setg(, ...); error_propagate(errp, err); by error_setg(errp, ...); Related pattern: if (...) { error_setg(, ...); goto out; } ... out: error_propagate(errp, err); return; When all paths to label out are that way, replace by

Re: [PATCH v2 00/44] Less clumsy error checking

2020-07-02 Thread Markus Armbruster
diff -w between v1 rebased and v2, with: diff --git a/include/qapi/error.h b/include/qapi/error.h index c3d84d610a..5ceb3ace06 100644 --- a/include/qapi/error.h +++ b/include/qapi/error.h @@ -145,10 +145,10 @@ * Likewise, do *not* * Error *err = NULL; * if (cond1) { - *

[PATCH v2 07/44] qemu-option: Make uses of find_desc_by_name() more similar

2020-07-02 Thread Markus Armbruster
This is to make the next commit easier to review. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy --- util/qemu-option.c | 32 ++-- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/util/qemu-option.c

[PATCH v2 18/44] qapi: Use returned bool to check for failure, manual part

2020-07-02 Thread Markus Armbruster
The previous commit used Coccinelle to convert from checking the Error object to checking the return value. Convert a few more manually. Also tweak control flow in places to conform to the conventional "if error bail out" pattern. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake ---

[PATCH v2 37/44] error: Reduce unnecessary error propagation

2020-07-02 Thread Markus Armbruster
When all we do with an Error we receive into a local variable is propagating to somewhere else, we can just as well receive it there right away, even when we need to keep error_propagate() for other error paths. Signed-off-by: Markus Armbruster --- block.c | 2 +- block/gluster.c

Re: [PATCH v2 13/44] qemu-option: Use returned bool to check for failure

2020-07-02 Thread Eric Blake
On 7/2/20 10:49 AM, Markus Armbruster wrote: The previous commit enables conversion of foo(..., ); if (err) { ... } to if (!foo(..., )) { ... } for QemuOpts functions that now return true / false on success / error. Coccinelle script: @@

Re: [PATCH v4 2/2] nvme: allow cmb and pmr to be enabled on same device

2020-07-02 Thread Klaus Jensen
On Jul 2 08:07, Andrzej Jakowski wrote: > On 7/2/20 3:31 AM, Klaus Jensen wrote: > > Aight, an update here. This only happens when QEMU is run with a virtual > > IOMMU. Otherwise, the kernel is happy. > > > > With the vIOMMU, qemu also craps out a bit: > > > > qemu-system-x86_64:

[PATCH 3/6] block: add ability to filter out blockdevs during snapshot

2020-07-02 Thread Daniel P . Berrangé
When executing snapshot logic, currently blockdevs are filtered out if they are read-only or if there is no media present. In order to support snapshotting when UEFI firmware is in use, we need the ability to filter out the blockdev used for the firmware vars store. This can be achieved by having

[PATCH 4/6] block: allow specifying name of block device for vmstate storage

2020-07-02 Thread Daniel P . Berrangé
Currently the vmstate will be stored in the first block device that supports snapshots. Historically this would have usually been the root device, but with UEFI it might be the variable store. There needs to be a way to override the choice of block device to store the state in. Signed-off-by:

Re: [PATCH v2 34/44] error: Eliminate error_propagate() with Coccinelle, part 1

2020-07-02 Thread Eric Blake
On 7/2/20 10:49 AM, Markus Armbruster wrote: When all we do with an Error we receive into a local variable is propagating to somewhere else, we can just as well receive it there right away. Convert if (!foo(..., )) { ... error_propagate(errp, err); ...

Re: nvme emulation merge process

2020-07-02 Thread Andrzej Jakowski
On 7/1/20 6:57 AM, Philippe Mathieu-Daudé wrote: > On 7/1/20 3:18 PM, Klaus Jensen wrote: >> On Jul 1 12:34, Kevin Wolf wrote: >>> Am 30.06.2020 um 22:36 hat Klaus Jensen geschrieben: On Jun 30 08:42, Keith Busch wrote: > On Tue, Jun 30, 2020 at 04:09:46PM +0200, Philippe Mathieu-Daudé

Re: nvme emulation merge process

2020-07-02 Thread Keith Busch
On Thu, Jul 02, 2020 at 01:29:26PM -0700, Andrzej Jakowski wrote: > > Thx! Of course I am interested in helping and I think it is actually great > idea to have couple of designated maintainers/reviewers as it would be easier > for folks to receive feedback vs requesting it in polling manner :) >

Re: [PATCH v9 21/34] qcow2: Add subcluster support to qcow2_get_host_offset()

2020-07-02 Thread Alberto Garcia
On Thu 02 Jul 2020 02:46:27 PM CEST, Max Reitz wrote: >> -/* must be allocated */ >> -assert(first_cluster_type == QCOW2_CLUSTER_NORMAL || >> - first_cluster_type == QCOW2_CLUSTER_ZERO_ALLOC); >> +assert(*l2_index + nb_clusters <= s->l2_size); > > Not l2_slice_size? Oh,

Re: [PATCH v9 14/34] qcow2: Add QCow2SubclusterType and qcow2_get_subcluster_type()

2020-07-02 Thread Alberto Garcia
On Thu 02 Jul 2020 11:57:46 AM CEST, Max Reitz wrote: >> The reason why we would want to check it is, of course, because that >> bit does have a meaning in regular L2 entries. >> >> But that bit is ignored in images with subclusters so the only reason >> why we would check it is to report

Re: [PATCH v9 30/34] qcow2: Add prealloc field to QCowL2Meta

2020-07-02 Thread Alberto Garcia
On Thu 02 Jul 2020 05:09:47 PM CEST, Max Reitz wrote: >> Without a backing file, there is no read required - writing to an >> unallocated subcluster within a preallocated cluster merely has to >> provide zeros to the rest of the write.  And depending on whether we >> can intelligently guarantee

Re: [PATCH v2 11/44] qemu-option: Replace opt_set() by cleaner opt_validate()

2020-07-02 Thread Vladimir Sementsov-Ogievskiy
02.07.2020 18:49, Markus Armbruster wrote: opt_set() frees its argument @value on failure. Slightly unclean; functions ideally do nothing on failure. To tidy this up, move opt_create() from opt_set() into its callers, along with the cleanup. Rename opt_set() to opt_validate(), noting its

Re: [PATCH v9 28/34] qcow2: Add subcluster support to qcow2_co_pwrite_zeroes()

2020-07-02 Thread Alberto Garcia
On Thu 02 Jul 2020 04:28:57 PM CEST, Max Reitz wrote: >> +/* For full clusters use zero_in_l2_slice() instead */ >> +assert(nb_subclusters > 0 && nb_subclusters < >> s->subclusters_per_cluster); >> +assert(sc + nb_subclusters <= s->subclusters_per_cluster); > > Maybe we should also

Re: [PATCH 07/17] hw/block/nvme: add support for the asynchronous event request command

2020-07-02 Thread Dmitry Fomichev
Looks good, Reviewed-by: Dmitry Fomichev On Mon, 2020-06-29 at 20:26 +0200, Klaus Jensen wrote: > From: Klaus Jensen > > Add support for the Asynchronous Event Request command. Required for > compliance with NVMe revision 1.3d. See NVM Express 1.3d, Section 5.2 > ("Asynchronous Event Request

  1   2   >