Re: [PATCH v2] Document qemu-img options data_file and data_file_raw
On 4/30/21 9:45 AM, Max Reitz wrote: >> + ``data_file_raw`` >> +If this option is set to ``on``, QEMU will always keep the external >> +data file consistent as a standalone read-only raw image. It does >> +this by forwarding updates through to the raw image in addition to >> +updating the image metadata. If set to ``off``, QEMU will only >> +update the image metadata without forwarding the changes through >> +to the raw image. The default value is ``off``. > > Hm, what updates and what changes? I mean, the first part makes sense (the > “It does this by...”), but the second part doesn’t. qemu will still forward > most writes to the data file. (Not all, but most.) > > (Also, nit pick: With data_file_raw=off, the data file is not a raw image. > (You still call it that in the penultimate sentence.)) > When you write data to a qcow2 file with data_file, the data also goes to the > data_file, most of the time. The exception is when it can be handled with a > metadata update, i.e. when it's a zero write or discard. > > In addition, such updates (i.e. zero writes, I presume) not happening to the > data file are usually a minor problem. The real problem is that without > data_file_raw, data clusters can be allocated anywhere in the data file, > whereas with data_file_raw, they are allocated at their respective guest > offset (i.e. the host offset always equals the guest offset). > > I personally would have been fine with the first sentence, but if we want > more of an explanation... Perhaps: > > < > If this option is set to ``on``, QEMU will always keep the external data file > consistent as a standalone read-only raw image. > > It does this by effectively forwarding all write accesses that happen to the > qcow2 file to the raw data file, including their offsets. Therefore, data > that is visible on the qcow2 node (i.e., to the guest) at some offset is > visible at the same offset in the raw data file. > > If this option is ``off``, QEMU will use the data file just to store data in > an effectively arbitrary manner. The file’s content will not make sense > without the accompanying qcow2 metadata. Where data is written will have no > relation to its offset as seen by the guest, and some writes (specifically > zero writes) may not be forwarded to the data file at all, but will only be > handled by modifying qcow2 metadata. > > In short: With data_file_raw, the data file reads as a valid raw VM image > file. Without it, its content can only be interpreted by reading the > accompanying qcow2 metadata. > > Note that this option only makes the data file valid as a read-only raw > image. You should not write to it, as this may effectively corrupt the qcow2 > metadata (for example, dirty bitmaps may become out of sync). > > EOF > > This got longer than I wanted it to be. Hm. Anyway, what do you think? I found it very helpful. I'll incorporate your explanation into the next revision. I'm wondering what the most appropriate trailer would be for the next revision? Suggested-by: Max [..] Co-developed-by: Max [..] Let me know if you have a strong preference, otherwise I'll go with Suggested-by: Thank you, Connor
Re: [PATCH 2/2] qemu-img: Require -F with -b backing image
On 5/3/21 4:36 PM, Eric Blake wrote: > Back in commit d9f059aa6c (qemu-img: Deprecate use of -b without -F), > we deprecated the ability to create a file with a backing image that > requires qemu to perform format probing. Qemu can still probe older > files for backwards compatibility, but it is time to finish off the > ability to create such images, due to the potential security risk they > present. Update a couple of iotests affected by the change. > > Signed-off-by: Eric Blake > --- > docs/system/deprecated.rst | 20 - > docs/system/removed-features.rst | 19 > block.c | 37 ++-- > qemu-img.c | 6 -- > tests/qemu-iotests/114 | 18 > tests/qemu-iotests/114.out | 11 -- > tests/qemu-iotests/301 | 4 +--- > tests/qemu-iotests/301.out | 16 ++ > 8 files changed, 50 insertions(+), 81 deletions(-) I'll need a followup to fix iotest failures in 40 and 41 (apparently they weren't passing backing formats, but I did not catch them in my original cleanup of iotests back in commit b66ff2c298) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
[PATCH 1/2] qcow2: Prohibit backing file changes in 'qemu-img amend'
This was deprecated back in bc5ee6da7 (qcow2: Deprecate use of
qemu-img amend to change backing file), and no one in the meantime has
given any reasons why it should be supported. Time to make change
attempts a hard error (but for convenience, specifying the _same_
backing chain is not forbidden). Update a couple of iotests to match.
Signed-off-by: Eric Blake
---
docs/system/deprecated.rst | 12
docs/system/removed-features.rst | 12
block/qcow2.c| 13 -
tests/qemu-iotests/061 | 3 +++
tests/qemu-iotests/061.out | 3 ++-
tests/qemu-iotests/082.out | 6 --
6 files changed, 25 insertions(+), 24 deletions(-)
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 80cae862528a..9ec1a9d0e03e 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -315,18 +315,6 @@ this CPU is also deprecated.
Related binaries
-qemu-img amend to adjust backing file (since 5.1)
-'
-
-The use of ``qemu-img amend`` to modify the name or format of a qcow2
-backing image is deprecated; this functionality was never fully
-documented or tested, and interferes with other amend operations that
-need access to the original backing image (such as deciding whether a
-v3 zero cluster may be left unallocated when converting to a v2
-image). Rather, any changes to the backing chain should be performed
-with ``qemu-img rebase -u`` either before or after the remaining
-changes being performed by amend, as appropriate.
-
qemu-img backing file without format (since 5.1)
diff --git a/docs/system/removed-features.rst b/docs/system/removed-features.rst
index 29e90601a51a..28b5df757d35 100644
--- a/docs/system/removed-features.rst
+++ b/docs/system/removed-features.rst
@@ -454,6 +454,18 @@ topologies described with -smp include all possible cpus,
i.e.
The ``enforce-config-section`` property was replaced by the
``-global migration.send-configuration={on|off}`` option.
+qemu-img amend to adjust backing file (removed in 6.1)
+''
+
+The use of ``qemu-img amend`` to modify the name or format of a qcow2
+backing image was never fully documented or tested, and interferes
+with other amend operations that need access to the original backing
+image (such as deciding whether a v3 zero cluster may be left
+unallocated when converting to a v2 image). Any changes to the
+backing chain should be performed with ``qemu-img rebase -u`` either
+before or after the remaining changes being performed by amend, as
+appropriate.
+
Block devices
-
diff --git a/block/qcow2.c b/block/qcow2.c
index 9727ae8fe341..898c4fc4464c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -5620,15 +5620,10 @@ static int qcow2_amend_options(BlockDriverState *bs,
QemuOpts *opts,
if (backing_file || backing_format) {
if (g_strcmp0(backing_file, s->image_backing_file) ||
g_strcmp0(backing_format, s->image_backing_format)) {
-warn_report("Deprecated use of amend to alter the backing file; "
-"use qemu-img rebase instead");
-}
-ret = qcow2_change_backing_file(bs,
-backing_file ?: s->image_backing_file,
-backing_format ?: s->image_backing_format);
-if (ret < 0) {
-error_setg_errno(errp, -ret, "Failed to change the backing file");
-return ret;
+error_setg(errp, "Cannot amend the backing file");
+error_append_hint(errp,
+ "You can use 'qemu-img rebase' instead.\n");
+return -EINVAL;
}
}
diff --git a/tests/qemu-iotests/061 b/tests/qemu-iotests/061
index e26d94a0df31..9507c223bda4 100755
--- a/tests/qemu-iotests/061
+++ b/tests/qemu-iotests/061
@@ -167,6 +167,9 @@ _make_test_img -o "compat=1.1" 64M
TEST_IMG="$TEST_IMG.base" _make_test_img -o "compat=1.1" 64M
$QEMU_IO -c "write -P 0x2a 0 128k" "$TEST_IMG.base" | _filter_qemu_io
$QEMU_IO -c "read -P 0 0 128k" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IMG amend -o "backing_file=$TEST_IMG.base,backing_fmt=qcow2" \
+ "$TEST_IMG" && echo "unexpected pass"
+$QEMU_IMG rebase -u -b "$TEST_IMG.base" -F qcow2 "$TEST_IMG"
$QEMU_IMG amend -o "backing_file=$TEST_IMG.base,backing_fmt=qcow2" "$TEST_IMG"
$QEMU_IO -c "read -P 0x2a 0 128k" "$TEST_IMG" | _filter_qemu_io
_check_test_img
diff --git a/tests/qemu-iotests/061.out b/tests/qemu-iotests/061.out
index ee30da266514..7ecbd4dea875 100644
--- a/tests/qemu-iotests/061.out
+++ b/tests/qemu-iotests/061.out
@@ -370,7 +370,8 @@ wrote 131072/131072 bytes at offset 0
128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
read 131072/131072 bytes at offset 0
128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-qemu-img: warning: Deprecated use of amend to
[PATCH 2/2] qemu-img: Require -F with -b backing image
Back in commit d9f059aa6c (qemu-img: Deprecate use of -b without -F),
we deprecated the ability to create a file with a backing image that
requires qemu to perform format probing. Qemu can still probe older
files for backwards compatibility, but it is time to finish off the
ability to create such images, due to the potential security risk they
present. Update a couple of iotests affected by the change.
Signed-off-by: Eric Blake
---
docs/system/deprecated.rst | 20 -
docs/system/removed-features.rst | 19
block.c | 37 ++--
qemu-img.c | 6 --
tests/qemu-iotests/114 | 18
tests/qemu-iotests/114.out | 11 --
tests/qemu-iotests/301 | 4 +---
tests/qemu-iotests/301.out | 16 ++
8 files changed, 50 insertions(+), 81 deletions(-)
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 9ec1a9d0e03e..aa6f7d84e583 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -315,26 +315,6 @@ this CPU is also deprecated.
Related binaries
-qemu-img backing file without format (since 5.1)
-
-
-The use of ``qemu-img create``, ``qemu-img rebase``, or ``qemu-img
-convert`` to create or modify an image that depends on a backing file
-now recommends that an explicit backing format be provided. This is
-for safety: if QEMU probes a different format than what you thought,
-the data presented to the guest will be corrupt; similarly, presenting
-a raw image to a guest allows a potential security exploit if a future
-probe sees a non-raw image based on guest writes.
-
-To avoid the warning message, or even future refusal to create an
-unsafe image, you must pass ``-o backing_fmt=`` (or the shorthand
-``-F`` during create) to specify the intended backing format. You may
-use ``qemu-img rebase -u`` to retroactively add a backing format to an
-existing image. However, be aware that there are already potential
-security risks to blindly using ``qemu-img info`` to probe the format
-of an untrusted backing image, when deciding what format to add into
-an existing image.
-
Backwards compatibility
---
diff --git a/docs/system/removed-features.rst b/docs/system/removed-features.rst
index 28b5df757d35..1928d8a483c0 100644
--- a/docs/system/removed-features.rst
+++ b/docs/system/removed-features.rst
@@ -466,6 +466,25 @@ backing chain should be performed with ``qemu-img rebase
-u`` either
before or after the remaining changes being performed by amend, as
appropriate.
+qemu-img backing file without format (removed in 6.1)
+'
+
+The use of ``qemu-img create``, ``qemu-img rebase``, or ``qemu-img
+convert`` to create or modify an image that depends on a backing file
+now requires that an explicit backing format be provided. This is
+for safety: if QEMU probes a different format than what you thought,
+the data presented to the guest will be corrupt; similarly, presenting
+a raw image to a guest allows a potential security exploit if a future
+probe sees a non-raw image based on guest writes.
+
+To avoid creating unsafe backing chains, you must pass ``-o
+backing_fmt=`` (or the shorthand ``-F`` during create) to specify the
+intended backing format. You may use ``qemu-img rebase -u`` to
+retroactively add a backing format to an existing image. However, be
+aware that there are already potential security risks to blindly using
+``qemu-img info`` to probe the format of an untrusted backing image,
+when deciding what format to add into an existing image.
+
Block devices
-
diff --git a/block.c b/block.c
index 874c22c43e3d..931e37a8499b 100644
--- a/block.c
+++ b/block.c
@@ -5033,7 +5033,7 @@ int coroutine_fn bdrv_co_check(BlockDriverState *bs,
* -ENOTSUP - format driver doesn't support changing the backing file
*/
int bdrv_change_backing_file(BlockDriverState *bs, const char *backing_file,
- const char *backing_fmt, bool warn)
+ const char *backing_fmt, bool require)
{
BlockDriver *drv = bs->drv;
int ret;
@@ -5047,10 +5047,8 @@ int bdrv_change_backing_file(BlockDriverState *bs, const
char *backing_file,
return -EINVAL;
}
-if (warn && backing_file && !backing_fmt) {
-warn_report("Deprecated use of backing file without explicit "
-"backing format, use of this image requires "
-"potentially unsafe format probing");
+if (require && backing_file && !backing_fmt) {
+return -EINVAL;
}
if (drv->bdrv_change_backing_file != NULL) {
@@ -6556,24 +6554,11 @@ void bdrv_img_create(const char *filename, const char
*fmt,
goto out;
} else {
if (!backing_fmt) {
-
Re: [PATCH] hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine
Hi Elena, +Mark You asked to use the next KVM-external call slot to talk about the ISA bus issues. I haven't scheduled the call because it seems this thread helped to figure the problems and Markus's analysis resumed them all. From here it should be clearer to see what has to be done to go where we want to be from where we are :) What do you think (in particular, from Markus list)? On 4/29/21 8:08 AM, Markus Armbruster wrote: > Stefan Hajnoczi writes: > >> On Wed, Apr 28, 2021 at 04:18:17PM +0200, Markus Armbruster wrote: >>> Stefan Hajnoczi writes: > > [...] > The approach in this patch is okay but we should keep in mind it only solves piix3-ide. ISA provides a non-qdev backdoor API and there may be more instances of this type of bug. A qdev fix would address the root cause and make it possible to drop the backdoor API, but that's probably too much work for little benefit. >>> >>> What do you mean by backdoor API? Global @isabus? >> >> Yes. It's also strange that isa_get_irq(ISADevice *dev, unsigned isairq) >> accepts dev = NULL as a valid argument. > > @isabus is static in hw/isa/isa-bus.c. Uses: > > * Limit isa_bus_new() to one ISA bus. Arbitrary restriction; multiple > ISA buses could work with suitable memory mapping and IRQ wiring. > "Single ISA bus" assumptions could of course hide elsewhere in the > code. > > * Implied argument to isa_get_irq(), isa_register_ioport(), > isa_register_portio_list(), isa_address_space(), > isa_address_space_io(). > > isa_get_irq() asserts that a non-null @dev is a child of @isabus. > This means we don't actually need @isabus, except when @dev is null. > I suspect two separate functions would be cleaner: one taking an > ISABus * argument, and a wrapper taking an ISADevice * argument. > > isa_address_space() and isa_address_space_io() work the same, less the > assertion. > > isa_register_ioport() and isa_register_portio_list() take a non-null > @dev argument. They don't actually need @isabus. > > To eliminate global @isabus, we need to fix up the callers passing null > @dev. Clean solution: plumb the ISABus returned by isa_bus_new() to the > call sites. Where that's impractical, we can also get it from QOM, like > build_dsdt_microvm() does. >
Re: [PATCH v2 1/6] vhost-user-blk: Make sure to set Error on realize failure
Acked-by: Raphael Norwitz
On Thu, Apr 29, 2021 at 07:13:11PM +0200, Kevin Wolf wrote:
> We have to set errp before jumping to virtio_err, otherwise the caller
> (virtio_device_realize()) will take this as success and crash when it
> later tries to access things that we've already freed in the error path.
>
> Fixes: 77542d431491788d1e8e79d93ce10172ef207775
> Signed-off-by: Kevin Wolf
> ---
> hw/block/vhost-user-blk.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index f5e9682703..7c85248a7b 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -447,7 +447,6 @@ static void vhost_user_blk_device_realize(DeviceState
> *dev, Error **errp)
> {
> VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> VHostUserBlk *s = VHOST_USER_BLK(vdev);
> -Error *err = NULL;
> int i, ret;
>
> if (!s->chardev.chr) {
> @@ -495,8 +494,7 @@ static void vhost_user_blk_device_realize(DeviceState
> *dev, Error **errp)
> NULL, true);
>
> reconnect:
> -if (qemu_chr_fe_wait_connected(>chardev, ) < 0) {
> -error_report_err(err);
> +if (qemu_chr_fe_wait_connected(>chardev, errp) < 0) {
> goto virtio_err;
> }
>
> --
> 2.30.2
>
Re: [PATCH] Add missing coroutine_fn function signature to functions
On Fri, Apr 30, 2021 at 09:34:41PM +, cennedee wrote:
> From 447601c28d5ed0b1208a0560390f760e75ce5613 Mon Sep 17 00:00:00 2001
> From: Cenne Dee
> Date: Fri, 30 Apr 2021 15:52:28 -0400
> Subject: [PATCH] Add missing coroutine_fn function signature to functions
>
> Patch adds the signature for all relevant functions ending with _co
> or those that use them.
>
> Signed-off-by: Cenne Dee
> ---
> block/block-gen.h | 2 +-
> migration/migration.c | 2 +-
> monitor/hmp.c | 2 +-
> scsi/qemu-pr-helper.c | 2 +-
> tests/unit/test-thread-pool.c | 4 ++--
> 5 files changed, 6 insertions(+), 6 deletions(-)
Hi,
Thanks for discussing coroutine_fn on IRC! Here is some feedback on this
patch:
> diff --git a/block/block-gen.h b/block/block-gen.h
> index f80cf48..4963372 100644
> --- a/block/block-gen.h
> +++ b/block/block-gen.h
> @@ -36,7 +36,7 @@ typedef struct BdrvPollCo {
> Coroutine *co; /* Keep pointer here for debugging */
> } BdrvPollCo;
>
> -static inline int bdrv_poll_co(BdrvPollCo *s)
> +static inline int coroutine_fn bdrv_poll_co(BdrvPollCo *s)
> {
> assert(!qemu_in_coroutine());
The assert indicates that this function must not be called from a
coroutine. Adding coroutine_fn is incorrect here.
> diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
> index 7b9389b..7c1ed2a 100644
> --- a/scsi/qemu-pr-helper.c
> +++ b/scsi/qemu-pr-helper.c
> @@ -175,7 +175,7 @@ static int do_sgio_worker(void *opaque)
> return status;
> }
>
> -static int do_sgio(int fd, const uint8_t *cdb, uint8_t *sense,
> +static int coroutine_fn do_sgio(int fd, const uint8_t *cdb, uint8_t *sense,
> uint8_t *buf, int *sz, int dir)
> {
> ThreadPool *pool = aio_get_thread_pool(qemu_get_aio_context());
This is correct but there are several functions that call do_sgio() that
also need coroutine_fn. The eventual parent is prh_co_entry() and it's a
good place to start auditing the code.
> diff --git a/tests/unit/test-thread-pool.c b/tests/unit/test-thread-pool.c
> index 70dc631..21fc118 100644
> --- a/tests/unit/test-thread-pool.c
> +++ b/tests/unit/test-thread-pool.c
> @@ -72,7 +72,7 @@ static void test_submit_aio(void)
> g_assert_cmpint(data.ret, ==, 0);
> }
>
> -static void co_test_cb(void *opaque)
> +static void coroutine_fn co_test_cb(void *opaque)
> {
> WorkerTestData *data = opaque;
>
> @@ -90,7 +90,7 @@ static void co_test_cb(void *opaque)
> /* The test continues in test_submit_co, after aio_poll... */
> }
>
> -static void test_submit_co(void)
> +static void coroutine_fn test_submit_co(void)
This function is not called from a coroutine and should not have
coroutine_fn. It's a regular function called by gtester:
g_test_add_func("/thread-pool/submit-co", test_submit_co);
The above change to co_test_cb() is correct though.
signature.asc
Description: PGP signature
Re: [PATCH v2 1/6] vhost-user-blk: Make sure to set Error on realize failure
On 4/29/21 12:13 PM, Kevin Wolf wrote: > We have to set errp before jumping to virtio_err, otherwise the caller > (virtio_device_realize()) will take this as success and crash when it > later tries to access things that we've already freed in the error path. > > Fixes: 77542d431491788d1e8e79d93ce10172ef207775 > Signed-off-by: Kevin Wolf > --- > hw/block/vhost-user-blk.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 2/6] vhost-user-blk: Don't reconnect during initialisation
So we're not going with the suggestion to retry once or a fixed number
of times? Any reason why not?
On Thu, Apr 29, 2021 at 07:13:12PM +0200, Kevin Wolf wrote:
> This is a partial revert of commits 77542d43149 and bc79c87bcde.
>
> Usually, an error during initialisation means that the configuration was
> wrong. Reconnecting won't make the error go away, but just turn the
> error condition into an endless loop. Avoid this and return errors
> again.
>
> Additionally, calling vhost_user_blk_disconnect() from the chardev event
> handler could result in use-after-free because none of the
> initialisation code expects that the device could just go away in the
> middle. So removing the call fixes crashes in several places.
>
> For example, using a num-queues setting that is incompatible with the
> backend would result in a crash like this (dereferencing dev->opaque,
> which is already NULL):
>
> #0 0x55d0a4bd in vhost_user_read_cb (source=0x568f4690,
> condition=(G_IO_IN | G_IO_HUP), opaque=0x7fffcbf0) at
> ../hw/virtio/vhost-user.c:313
> #1 0x55d950d3 in qio_channel_fd_source_dispatch
> (source=0x57c3f750, callback=0x55d0a478 ,
> user_data=0x7fffcbf0) at ../io/channel-watch.c:84
> #2 0x77b32a9f in g_main_context_dispatch () at
> /lib64/libglib-2.0.so.0
> #3 0x77b84a98 in g_main_context_iterate.constprop () at
> /lib64/libglib-2.0.so.0
> #4 0x77b32163 in g_main_loop_run () at /lib64/libglib-2.0.so.0
> #5 0x55d0a724 in vhost_user_read (dev=0x57bc62f8,
> msg=0x7fffcc50) at ../hw/virtio/vhost-user.c:402
> #6 0x55d0ee6b in vhost_user_get_config (dev=0x57bc62f8,
> config=0x57bc62ac "", config_len=60) at ../hw/virtio/vhost-user.c:2133
> #7 0x55d56d46 in vhost_dev_get_config (hdev=0x57bc62f8,
> config=0x57bc62ac "", config_len=60) at ../hw/virtio/vhost.c:1566
> #8 0x55cdd150 in vhost_user_blk_device_realize (dev=0x57bc60b0,
> errp=0x7fffcf90) at ../hw/block/vhost-user-blk.c:510
> #9 0x55d08f6d in virtio_device_realize (dev=0x57bc60b0,
> errp=0x7fffcff0) at ../hw/virtio/virtio.c:3660
>
> Signed-off-by: Kevin Wolf
> ---
> hw/block/vhost-user-blk.c | 59 +++
> 1 file changed, 17 insertions(+), 42 deletions(-)
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 7c85248a7b..c0b9958da1 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -50,6 +50,8 @@ static const int user_feature_bits[] = {
> VHOST_INVALID_FEATURE_BIT
> };
>
> +static void vhost_user_blk_event(void *opaque, QEMUChrEvent event);
> +
> static void vhost_user_blk_update_config(VirtIODevice *vdev, uint8_t *config)
> {
> VHostUserBlk *s = VHOST_USER_BLK(vdev);
> @@ -362,19 +364,6 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
> vhost_dev_cleanup(>dev);
> }
>
> -static void vhost_user_blk_event(void *opaque, QEMUChrEvent event,
> - bool realized);
> -
> -static void vhost_user_blk_event_realize(void *opaque, QEMUChrEvent event)
> -{
> -vhost_user_blk_event(opaque, event, false);
> -}
> -
> -static void vhost_user_blk_event_oper(void *opaque, QEMUChrEvent event)
> -{
> -vhost_user_blk_event(opaque, event, true);
> -}
> -
> static void vhost_user_blk_chr_closed_bh(void *opaque)
> {
> DeviceState *dev = opaque;
> @@ -382,12 +371,11 @@ static void vhost_user_blk_chr_closed_bh(void *opaque)
> VHostUserBlk *s = VHOST_USER_BLK(vdev);
>
> vhost_user_blk_disconnect(dev);
> -qemu_chr_fe_set_handlers(>chardev, NULL, NULL,
> -vhost_user_blk_event_oper, NULL, opaque, NULL, true);
> +qemu_chr_fe_set_handlers(>chardev, NULL, NULL, vhost_user_blk_event,
> + NULL, opaque, NULL, true);
> }
>
> -static void vhost_user_blk_event(void *opaque, QEMUChrEvent event,
> - bool realized)
> +static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
> {
> DeviceState *dev = opaque;
> VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> @@ -401,17 +389,7 @@ static void vhost_user_blk_event(void *opaque,
> QEMUChrEvent event,
> }
> break;
> case CHR_EVENT_CLOSED:
> -/*
> - * Closing the connection should happen differently on device
> - * initialization and operation stages.
> - * On initalization, we want to re-start vhost_dev initialization
> - * from the very beginning right away when the connection is closed,
> - * so we clean up vhost_dev on each connection closing.
> - * On operation, we want to postpone vhost_dev cleanup to let the
> - * other code perform its own cleanup sequence using vhost_dev data
> - * (e.g. vhost_dev_set_log).
> - */
> -if (realized && !runstate_check(RUN_STATE_SHUTDOWN)) {
> +if
Re: [PATCH 5/6] block: simplify bdrv_child_user_desc()
On Mon 03 May 2021 01:34:01 PM CEST, Vladimir Sementsov-Ogievskiy wrote: > All existing parent types (block nodes, block devices, jobs) has the > realization. So, drop unreachable code. > > Signed-off-by: Vladimir Sementsov-Ogievskiy With the updated description that you propose in your reply to the patch, Reviewed-by: Alberto Garcia Berto
Re: [PATCH 3/6] block-backend: improve blk_root_get_parent_desc()
On Mon 03 May 2021 01:33:59 PM CEST, Vladimir Sementsov-Ogievskiy wrote: > We have different types of parents: block nodes, block backends and > jobs. So, it makes sense to specify type together with name. > > While being here also use g_autofree. > > iotest 307 output is updated. > > Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Alberto Garcia Berto
Re: [PATCH 1/6] block: fix leak of tran in bdrv_root_attach_child
On Mon 03 May 2021 01:33:57 PM CEST, Vladimir Sementsov-Ogievskiy
wrote:
> @@ -2918,12 +2918,18 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState
> *child_bs,
> child_role, perm, shared_perm, opaque,
> , tran, errp);
> if (ret < 0) {
> -bdrv_unref(child_bs);
> -return NULL;
> +goto out;
> }
>
> ret = bdrv_refresh_perms(child_bs, errp);
> +if (ret < 0) {
> +goto out;
> +}
> +
> +out:
I see why you're doing this last error check, but it looks a bit
weird. My first reaction was to think that I was missing something.
I would remove it.
Berto
Re: [PATCH 2/6] block: bdrv_reopen_multiple(): fix leak of tran object
On Mon 03 May 2021 01:33:58 PM CEST, Vladimir Sementsov-Ogievskiy wrote: > We have one path, where tran object is created, but we don't touch and > don't free it in any way: "goto cleanup" in first loop with calls to > bdrv_flush(). > > Fix it simply moving tran_new() call below that loop. > > Reported-by: Coverity (CID 1452772) > Reported-by: Peter Maydell > Suggested-by: Peter Maydell > Fixes: 72373e40fbc7e4218061a8211384db362d3e7348 > Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Alberto Garcia Berto
Re: [PATCH v3 14/15] qemu_iotests: add option to show qemu binary logs on stdout
On 30.04.21 23:04, Emanuele Giuseppe Esposito wrote:
On 30/04/2021 15:50, Max Reitz wrote:
On 14.04.21 19:03, Emanuele Giuseppe Esposito wrote:
Using the flag -p, allow the qemu binary to print to stdout.
This helps especially when doing print-debugging.
I think this shouldn’t refer to prints but to qemu’s stdout/stderr in
general, i.e
Signed-off-by: Emanuele Giuseppe Esposito
---
tests/qemu-iotests/check | 3 ++-
tests/qemu-iotests/iotests.py | 9 +
tests/qemu-iotests/testenv.py | 9 +++--
3 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 489178d9a4..84483922eb 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -33,6 +33,7 @@ def make_argparser() -> argparse.ArgumentParser:
help='pretty print output for make check')
p.add_argument('-d', dest='debug', action='store_true',
help='debug')
+ p.add_argument('-p', dest='print', action='store_true',
help='shows qemu binary prints to stdout')
I’d prefer for the description to be “redirects qemu's stdout and
stderr to the test output”.
Also, this line is too long.
p.add_argument('-gdb', action='store_true',
help="start gdbserver with $GDB_QEMU options. \
Default is localhost:12345")
@@ -117,7 +118,7 @@ if __name__ == '__main__':
aiomode=args.aiomode, cachemode=args.cachemode,
imgopts=args.imgopts, misalign=args.misalign,
debug=args.debug, valgrind=args.valgrind,
- gdb=args.gdb)
+ gdb=args.gdb, qprint=args.print)
testfinder = TestFinder(test_dir=env.source_iotests)
diff --git a/tests/qemu-iotests/iotests.py
b/tests/qemu-iotests/iotests.py
index f9832558a0..52ff7332f8 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -79,6 +79,8 @@
if os.environ.get('GDB_QEMU'):
qemu_gdb = ['gdbserver'] +
os.environ.get('GDB_QEMU').strip().split(' ')
+qemu_print = os.environ.get('PRINT_QEMU', False)
+
imgfmt = os.environ.get('IMGFMT', 'raw')
imgproto = os.environ.get('IMGPROTO', 'file')
output_dir = os.environ.get('OUTPUT_DIR', '.')
@@ -621,6 +623,13 @@ def _post_shutdown(self) -> None:
super()._post_shutdown()
self.subprocess_check_valgrind(qemu_valgrind)
+ def _pre_launch(self) -> None:
+ super()._pre_launch()
+ if qemu_print and self._qemu_log_file != None:
I think "is not None" is mostly used in Python. (I’m saying this in
this weird way because I’m not the one to ask what’s mostly used in
Python...)
(That also silences mypy, which otherwise complains and then fails 297.)
+ # set QEMU binary output to stdout
+ self._qemu_log_file.close()
+ self._qemu_log_file = None
I don’t know enough Python to know how this works. I suppose this
does access the super class’s member? (I could also imagine it
creates a new member, just for this child class, but that then
effectively overwrites the super class’s member.)
Considering _qemu_log_file is apparently meant to be a private
attribute (from the leading underscore), I think it would be better to
have a function provided by the super class for this.
It should access the superclass member, and it's the same that
_post_shutdown does. I can make a function for that.
Understood.
Regarding all other feedback in all patches that I did not answer: agree
on all of them, will adjust everything in v4.
Thanks, looking forward to it! :)
Max
Re: [PATCH v3 05/15] qemu-iotests: delay QMP socket timers
On 30.04.21 23:03, Emanuele Giuseppe Esposito wrote: On 30/04/2021 13:59, Max Reitz wrote: On 14.04.21 19:03, Emanuele Giuseppe Esposito wrote: Attaching a gdbserver implies that the qmp socket should wait indefinitely for an answer from QEMU. Signed-off-by: Emanuele Giuseppe Esposito --- python/qemu/machine.py | 3 +++ tests/qemu-iotests/iotests.py | 10 +- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/python/qemu/machine.py b/python/qemu/machine.py index 12752142c9..d6142271c2 100644 --- a/python/qemu/machine.py +++ b/python/qemu/machine.py @@ -409,6 +409,9 @@ def _launch(self) -> None: stderr=subprocess.STDOUT, shell=False, close_fds=False) + + if 'gdbserver' in self._wrapper: + self._qmp_timer = None Why doesn’t __init__() evaluate this? This here doesn’t feel like the right place for it. If we want to evaluate it here, self._qmp_timer shouldn’t exist, and instead the timeout should be a _post_launch() parameter. (Which I would have nothing against, by the way.) Uhm.. I got another comment in a previous version where for the "event" callbacks it was better a property than passing around a parameter. Which I honestly agree. I think that comment was in the sense of providing a default value, which can be expressed by having a property that is set in __init__. I don’t have anything against making this a property, but I also don’t have anything against making it a _post_launch() parameter. I could even live with both, i.e. set _qmp_timer to 15 in __init__, then have a _post_launch parameter, and pass either self._qmp_timer or None if self._wrapper includes 'gdbserver'. What I do mind is that I don’t understand why the property is modified here. The value of self._qmp_timer is supposed to be 15 by default and None if self._wrapper includes 'gdbserver'. It should thus be changed to None the moment self._wrapper is made to include 'gdbserver'. Because self._wrapper is set only in __init__, this should happen in __init__. What should __init__() do? The check here is to see if the invocation has gdb (and a couple of patches ahead also valgrind), to remove the timer. If I understand what you mean, you want something like def __init__(self, timer): Oh, no. We can optionally do that perhaps later, but what I meant is just to put this in __init__() (without adding any parameters to it): self._qmp_timer = 15.0 if 'gdbserver' not in self._wrapper else None I think self._qmp_timer should always reflect what timeout we are going to use when a VM is launched. So if the conditions influencing the timeout change, it should be updated immediately to reflect this. The only condition we have right now is the content of self._wrapper, which is only set in __init__, so self._qmp_timer should be set once in __init__ and not changed afterwards. That sounds academic, but imagine what would happen if we had a set_qmp_timer() method: The timout could be adjusted, but launch() would just ignore it and update the property, even though the conditions influencing the timout didn’t change between set_qmp_timer() and launch(). Or if we had a get_qmp_timer(); a caller would read a timeout of 15.0 before launch(), even though the timeout is going to be None. Therefore, I think a property should not be updated just before it is read, but instead when any condition that’s supposed to influence its value changes. I suggested making it a parameter because updating a property when reading it sounds like it should be a parameter instead. I.e., one would say def __init__(): self._qmp_timeout_default = 15.0 def post_launch(qmp_timeout): self._qmp.accept(qmp_timeout) def launch(self): ... qmp_timeout = None if 'gdbserver' in self._wrapper \ else self._qmp_timout_default self.post_launch(qmp_timeout) Which is basically the structure your patch has, which gave me the idea. [...] self._post_launch() def _early_cleanup(self) -> None: diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index 05d0dc0751..380527245e 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py [...] @@ -684,6 +687,11 @@ def qmp_to_opts(self, obj): output_list += [key + '=' + obj[key]] return ','.join(output_list) + def get_qmp_events(self, wait: bool = False) -> List[QMPMessage]: + if qemu_gdb: + wait = 0.0 [...] Second, I don’t understand this. If the caller wants to block waiting on an event, then that should have nothing to do with whether we have gdb running or not. As far as I understand, setting wait to 0.0 is the same as wait = False, i.e. we don’t block and just return None immediately if there is no pending event. You're right, this might not be
Re: [PATCH v3 04/15] qemu-iotests: add option to attach gdbserver
On 30.04.21 23:03, Emanuele Giuseppe Esposito wrote: On 30/04/2021 13:38, Max Reitz wrote: On 14.04.21 19:03, Emanuele Giuseppe Esposito wrote: Add -gdb flag and GDB_QEMU environmental variable to python tests to attach a gdbserver to each qemu instance. Well, this patch doesn’t do this, but OK. Maybe "define" rather than "add"? In the sense of defining the "-gdb" option, which is what it actually does. That’s possible, but I think better would be to contrast it with what this patch doesn’t do, but what one could think when reading this description. I.e. to say “Add/define -gdb flag [...] to each qemu instance. This patch only adds and parses this flag, it does not yet add the implementation for it.” Out of interest: Why gdbserver and not “just” gdb? On Fedora, those are separate packages, so I don’t have gdbserver installed, that’s why I’m asking. As far as I have tried, using only gdb with ./check is very hard to use, because the stdout is filtered out by the script. So invoking gdb attaches it to QEMU, but it is not possible to start execution (run command) or interact with it, because of the python script filtering. This leaves the test hanging. gdbserver is just something that a gdb client can attach to (for example, in another console or even in another host) for example with the command # gdb -iex "target remote localhost:12345" . This provides a nice and separate gdb monitor to the client. All right. I thought gdb could be used as a server, too, but... Looks like it can’t. (Like, I thought, you could do something like “gdb -ex 'listen localhost:12345' $cmd”. But seems like there is no server built into gdb proper.) Max
Re: [PATCH 2/2] block: Fix Transaction leak in bdrv_reopen_multiple()
Am 03.05.2021 um 15:09 hat Vladimir Sementsov-Ogievskiy geschrieben: > 03.05.2021 15:41, Kevin Wolf wrote: > > Am 03.05.2021 um 13:40 hat Vladimir Sementsov-Ogievskiy geschrieben: > > > 03.05.2021 14:05, Kevin Wolf wrote: > > > > Like other error paths, this one needs to call tran_finalize() and clean > > > > up the BlockReopenQueue, too. > > > > > > We don't need the "abort" loop on that path. And clean-up of > > > BlockReopenQueue is at "cleanup:" label. > > > > The cleanup of the BlockReopenQueue itself is there, but not of all > > fields in it. Specifically, these lines are needed: > > > > qobject_unref(bs_entry->state.explicit_options); > > qobject_unref(bs_entry->state.options); > > > > The references are taken in bdrv_reopen_queue_child() and either used in > > commit or released on abort. Doing nothing with them leaks them. > > Oops. Somehow I decided they are set in _prepare. > > > > > > So I'd prefer Peter's suggestion (my "[PATCH 2/6] block: > > > bdrv_reopen_multiple(): fix leak of tran object") > > > > I don't like it because I think every call that doesn't end in a commit, > > should jump to the abort label to make sure that everything that remains > > unused because of this is correctly cleaned up. > > > > > agree now.. > > Still, don't we miss these two qobject_unref() calls on success path? No, they are put to use in bdrv_reopen_commit(): qobject_unref(bs->explicit_options); qobject_unref(bs->options); bs->explicit_options = reopen_state->explicit_options; bs->options= reopen_state->options; Kevin
Re: [PATCH 2/4] block/vvfat: Fix leak of BDRVVVFATState::used_clusters
On Fri, Apr 30, 2021 at 06:25:17PM +0200, Philippe Mathieu-Daudé wrote:
used_clusters is allocated in enable_write_target(), called by
vvfat_open(), but never free'd.
Allocate it using GLib API, and free it in vvfat_close().
This fixes (QEMU built with --enable-sanitizers):
Direct leak of 64508 byte(s) in 1 object(s) allocated from:
#0 0x55d7a36378f7 in calloc (qemu-system-x86_64+0x1dab8f7)
#1 0x55d7a5e14246 in enable_write_target block/vvfat.c:3145:24
#2 0x55d7a5e123aa in vvfat_open block/vvfat.c:1236:19
#3 0x55d7a5a5363f in bdrv_open_driver block.c:1526:15
#4 0x55d7a5a9d369 in bdrv_open_common block.c:1802:11
#5 0x55d7a5a609f1 in bdrv_open_inherit block.c:3444:11
#6 0x55d7a5a65411 in bdrv_open_child_bs block.c:3079:10
#7 0x55d7a5a60079 in bdrv_open_inherit block.c:3391:19
#8 0x55d7a5a65da3 in bdrv_open block.c:3537:12
#9 0x55d7a5b33f6a in blk_new_open block/block-backend.c:421:10
#10 0x55d7a5a0a33e in blockdev_init blockdev.c:610:15
#11 0x55d7a5a088e7 in drive_new blockdev.c:994:11
#12 0x55d7a51b10c4 in drive_init_func softmmu/vl.c:636:12
#13 0x55d7a620e148 in qemu_opts_foreach util/qemu-option.c:1167:14
#14 0x55d7a51b0e20 in configure_blockdev softmmu/vl.c:695:9
#15 0x55d7a51a70b5 in qemu_create_early_backends softmmu/vl.c:1895:5
#16 0x55d7a519bf87 in qemu_init softmmu/vl.c:3551:5
#17 0x55d7a366f619 in main softmmu/main.c:49:5
Fixes: a046433a161 ("Major overhaul of the virtual FAT driver for read/write
support")
Signed-off-by: Philippe Mathieu-Daudé
---
block/vvfat.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/block/vvfat.c b/block/vvfat.c
index 5a4a7915220..2cc21787600 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -3142,7 +3142,7 @@ static int enable_write_target(BlockDriverState *bs,
Error **errp)
int size = sector2cluster(s, s->sector_count);
QDict *options;
-s->used_clusters = calloc(size, 1);
+s->used_clusters = g_malloc0(size);
array_init(&(s->commits), sizeof(commit_t));
@@ -3233,6 +3233,7 @@ static void vvfat_close(BlockDriverState *bs)
array_free(&(s->directory));
array_free(&(s->mapping));
g_free(s->cluster_buffer);
+g_free(s->used_clusters);
g_free(s->qcow_filename);
if (s->qcow) {
--
2.26.3
In vvfat_open() we set to NULL the other pointers allocated through GLib
API (e.g. s->qcow_filename), but IIUC the BDRVVVFATState (bs->opaque) is
allocated through g_malloc0() in bdrv_open_driver(), so those
initializations should be superfluous and in this case we can avoid
setting s->used_clusters to NULL.
Reviewed-by: Stefano Garzarella
Re: [PATCH 1/4] block/vvfat: Fix leak of BDRVVVFATState::qcow_filename
On Fri, Apr 30, 2021 at 06:25:16PM +0200, Philippe Mathieu-Daudé wrote:
qcow_filename is allocated in enable_write_target(), called by
vvfat_open(), but never free'd. Free it in vvfat_close().
This fixes (QEMU built with --enable-sanitizers):
Direct leak of 4096 byte(s) in 1 object(s) allocated from:
#0 0x55d7a363773f in malloc
(/mnt/scratch/qemu/sanitizer/qemu-system-x86_64+0x1dab73f)
#1 0x7f55c6447958 in g_malloc (/lib64/libglib-2.0.so.0+0x58958)
#2 0x55d7a5e123aa in vvfat_open block/vvfat.c:1236:19
#3 0x55d7a5a5363f in bdrv_open_driver block.c:1526:15
#4 0x55d7a5a9d369 in bdrv_open_common block.c:1802:11
#5 0x55d7a5a609f1 in bdrv_open_inherit block.c:3444:11
#6 0x55d7a5a65411 in bdrv_open_child_bs block.c:3079:10
#7 0x55d7a5a60079 in bdrv_open_inherit block.c:3391:19
#8 0x55d7a5a65da3 in bdrv_open block.c:3537:12
#9 0x55d7a5b33f6a in blk_new_open block/block-backend.c:421:10
#10 0x55d7a5a0a33e in blockdev_init blockdev.c:610:15
#11 0x55d7a5a088e7 in drive_new blockdev.c:994:11
#12 0x55d7a51b10c4 in drive_init_func softmmu/vl.c:636:12
#13 0x55d7a620e148 in qemu_opts_foreach util/qemu-option.c:1167:14
#14 0x55d7a51b0e20 in configure_blockdev softmmu/vl.c:695:9
#15 0x55d7a51a70b5 in qemu_create_early_backends softmmu/vl.c:1895:5
#16 0x55d7a519bf87 in qemu_init softmmu/vl.c:3551:5
#17 0x55d7a366f619 in main softmmu/main.c:49:5
Fixes: 8475ea48544 ("block/vvfat: Do not unref qcow on closing backing bdrv")
Signed-off-by: Philippe Mathieu-Daudé
---
block/vvfat.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/vvfat.c b/block/vvfat.c
index 54807f82ca1..5a4a7915220 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -3233,6 +3233,7 @@ static void vvfat_close(BlockDriverState *bs)
array_free(&(s->directory));
array_free(&(s->mapping));
g_free(s->cluster_buffer);
+g_free(s->qcow_filename);
if (s->qcow) {
migrate_del_blocker(s->migration_blocker);
--
2.26.3
Reviewed-by: Stefano Garzarella
Re: [PATCH 2/2] block: Fix Transaction leak in bdrv_reopen_multiple()
03.05.2021 15:41, Kevin Wolf wrote: Am 03.05.2021 um 13:40 hat Vladimir Sementsov-Ogievskiy geschrieben: 03.05.2021 14:05, Kevin Wolf wrote: Like other error paths, this one needs to call tran_finalize() and clean up the BlockReopenQueue, too. We don't need the "abort" loop on that path. And clean-up of BlockReopenQueue is at "cleanup:" label. The cleanup of the BlockReopenQueue itself is there, but not of all fields in it. Specifically, these lines are needed: qobject_unref(bs_entry->state.explicit_options); qobject_unref(bs_entry->state.options); The references are taken in bdrv_reopen_queue_child() and either used in commit or released on abort. Doing nothing with them leaks them. Oops. Somehow I decided they are set in _prepare. So I'd prefer Peter's suggestion (my "[PATCH 2/6] block: bdrv_reopen_multiple(): fix leak of tran object") I don't like it because I think every call that doesn't end in a commit, should jump to the abort label to make sure that everything that remains unused because of this is correctly cleaned up. agree now.. Still, don't we miss these two qobject_unref() calls on success path? -- Best regards, Vladimir
Re: [PATCH 2/2] block: Fix Transaction leak in bdrv_reopen_multiple()
Am 03.05.2021 um 13:40 hat Vladimir Sementsov-Ogievskiy geschrieben: > 03.05.2021 14:05, Kevin Wolf wrote: > > Like other error paths, this one needs to call tran_finalize() and clean > > up the BlockReopenQueue, too. > > We don't need the "abort" loop on that path. And clean-up of > BlockReopenQueue is at "cleanup:" label. The cleanup of the BlockReopenQueue itself is there, but not of all fields in it. Specifically, these lines are needed: qobject_unref(bs_entry->state.explicit_options); qobject_unref(bs_entry->state.options); The references are taken in bdrv_reopen_queue_child() and either used in commit or released on abort. Doing nothing with them leaks them. > So I'd prefer Peter's suggestion (my "[PATCH 2/6] block: > bdrv_reopen_multiple(): fix leak of tran object") I don't like it because I think every call that doesn't end in a commit, should jump to the abort label to make sure that everything that remains unused because of this is correctly cleaned up. Kevin
Re: [PATCH 1/2] block: Fix Transaction leak in bdrv_root_attach_child()
03.05.2021 15:14, Vladimir Sementsov-Ogievskiy wrote:
03.05.2021 14:53, Max Reitz wrote:
On 03.05.21 13:51, Vladimir Sementsov-Ogievskiy wrote:
03.05.2021 14:49, Max Reitz wrote:
On 03.05.21 13:05, Kevin Wolf wrote:
The error path needs to call tran_finalize(), too.
Fixes: CID 1452773
Fixes: 548a74c0dbc858edd1a7ee3045b5f2fe710bd8b1
Signed-off-by: Kevin Wolf
---
block.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/block.c b/block.c
index 874c22c43e..5c0ced6238 100644
--- a/block.c
+++ b/block.c
@@ -2918,13 +2918,14 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState
*child_bs,
child_role, perm, shared_perm, opaque,
, tran, errp);
if (ret < 0) {
- bdrv_unref(child_bs);
- return NULL;
+ assert(child == NULL);
+ goto out;
}
ret = bdrv_refresh_perms(child_bs, errp);
- tran_finalize(tran, ret);
+out:
+ tran_finalize(tran, ret);
bdrv_unref(child_bs);
return child;
Looks OK, so:
Reviewed-by: Max Reitz
However, the function’s description says that it will return NULL on error.
But if bdrv_refresh_perms() fails, it will still return a non-NULL child. Is
that right?
No, it's reset to NULL on transaction abort, so code is correct. It's not obvious, and
I've added a comment and assertion in my version of this fix "[PATCH 1/6] block: fix
leak of tran in bdrv_root_attach_child"
The fact that the transaction keeps the pointer to this local variable around
is a bit horrifying, but well.
Yes this looks overcomplicated here. But it's useful for bdrv_set_backing_noperm
where we would have to add a separate entry to @tran to rollback bs->backing
change. Probably, it's better to refactor this thing. Or at least document that
out-pointer argument goes into transaction, and should be valid up to transaction
finalization.
documentation done:
[PATCH 7/6] block: document child argument of bdrv_attach_child_common()
--
Best regards,
Vladimir
[PATCH 7/6] block: document child argument of bdrv_attach_child_common()
The logic around **child is not obvious: this reference is used not
only to return resulting child, but also to rollback NULL value on
transaction abort. Let's document this.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/block.c b/block.c
index 3fc87fbf90..1f89be6f97 100644
--- a/block.c
+++ b/block.c
@@ -2773,6 +2773,12 @@ static TransactionActionDrv bdrv_attach_child_common_drv
= {
/*
* Common part of attaching bdrv child to bs or to blk or to job
+ *
+ * Resulting new child is returned through @child.
+ * At start *@child must be NULL.
+ * @child is saved to a new entry of @tran, so that *@child could be reverted
to
+ * NULL on abort(). So referenced variable must live at least until transaction
+ * end.
*/
static int bdrv_attach_child_common(BlockDriverState *child_bs,
const char *child_name,
@@ -2847,6 +2853,10 @@ static int bdrv_attach_child_common(BlockDriverState
*child_bs,
return 0;
}
+/*
+ * Variable referenced by @child must live at least until transaction end.
+ * (see bdrv_attach_child_common() doc for details)
+ */
static int bdrv_attach_child_noperm(BlockDriverState *parent_bs,
BlockDriverState *child_bs,
const char *child_name,
--
2.29.2
Re: [PATCH 1/2] block: Fix Transaction leak in bdrv_root_attach_child()
03.05.2021 14:53, Max Reitz wrote:
On 03.05.21 13:51, Vladimir Sementsov-Ogievskiy wrote:
03.05.2021 14:49, Max Reitz wrote:
On 03.05.21 13:05, Kevin Wolf wrote:
The error path needs to call tran_finalize(), too.
Fixes: CID 1452773
Fixes: 548a74c0dbc858edd1a7ee3045b5f2fe710bd8b1
Signed-off-by: Kevin Wolf
---
block.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/block.c b/block.c
index 874c22c43e..5c0ced6238 100644
--- a/block.c
+++ b/block.c
@@ -2918,13 +2918,14 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState
*child_bs,
child_role, perm, shared_perm, opaque,
, tran, errp);
if (ret < 0) {
- bdrv_unref(child_bs);
- return NULL;
+ assert(child == NULL);
+ goto out;
}
ret = bdrv_refresh_perms(child_bs, errp);
- tran_finalize(tran, ret);
+out:
+ tran_finalize(tran, ret);
bdrv_unref(child_bs);
return child;
Looks OK, so:
Reviewed-by: Max Reitz
However, the function’s description says that it will return NULL on error.
But if bdrv_refresh_perms() fails, it will still return a non-NULL child. Is
that right?
No, it's reset to NULL on transaction abort, so code is correct. It's not obvious, and
I've added a comment and assertion in my version of this fix "[PATCH 1/6] block: fix
leak of tran in bdrv_root_attach_child"
The fact that the transaction keeps the pointer to this local variable around
is a bit horrifying, but well.
Yes this looks overcomplicated here. But it's useful for bdrv_set_backing_noperm
where we would have to add a separate entry to @tran to rollback bs->backing
change. Probably, it's better to refactor this thing. Or at least document that
out-pointer argument goes into transaction, and should be valid up to transaction
finalization.
--
Best regards,
Vladimir
Re: [PATCH 1/2] block: Fix Transaction leak in bdrv_root_attach_child()
03.05.2021 14:49, Max Reitz wrote:
On 03.05.21 13:05, Kevin Wolf wrote:
The error path needs to call tran_finalize(), too.
Fixes: CID 1452773
Fixes: 548a74c0dbc858edd1a7ee3045b5f2fe710bd8b1
Signed-off-by: Kevin Wolf
---
block.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/block.c b/block.c
index 874c22c43e..5c0ced6238 100644
--- a/block.c
+++ b/block.c
@@ -2918,13 +2918,14 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState
*child_bs,
child_role, perm, shared_perm, opaque,
, tran, errp);
if (ret < 0) {
- bdrv_unref(child_bs);
- return NULL;
+ assert(child == NULL);
+ goto out;
}
ret = bdrv_refresh_perms(child_bs, errp);
- tran_finalize(tran, ret);
+out:
+ tran_finalize(tran, ret);
bdrv_unref(child_bs);
return child;
Looks OK, so:
Reviewed-by: Max Reitz
However, the function’s description says that it will return NULL on error.
But if bdrv_refresh_perms() fails, it will still return a non-NULL child. Is
that right?
No, it's reset to NULL on transaction abort, so code is correct. It's not obvious, and
I've added a comment and assertion in my version of this fix "[PATCH 1/6] block: fix
leak of tran in bdrv_root_attach_child"
--
Best regards,
Vladimir
Re: [PATCH 1/2] block: Fix Transaction leak in bdrv_root_attach_child()
On 03.05.21 13:51, Vladimir Sementsov-Ogievskiy wrote:
03.05.2021 14:49, Max Reitz wrote:
On 03.05.21 13:05, Kevin Wolf wrote:
The error path needs to call tran_finalize(), too.
Fixes: CID 1452773
Fixes: 548a74c0dbc858edd1a7ee3045b5f2fe710bd8b1
Signed-off-by: Kevin Wolf
---
block.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/block.c b/block.c
index 874c22c43e..5c0ced6238 100644
--- a/block.c
+++ b/block.c
@@ -2918,13 +2918,14 @@ BdrvChild
*bdrv_root_attach_child(BlockDriverState *child_bs,
child_role, perm, shared_perm,
opaque,
, tran, errp);
if (ret < 0) {
- bdrv_unref(child_bs);
- return NULL;
+ assert(child == NULL);
+ goto out;
}
ret = bdrv_refresh_perms(child_bs, errp);
- tran_finalize(tran, ret);
+out:
+ tran_finalize(tran, ret);
bdrv_unref(child_bs);
return child;
Looks OK, so:
Reviewed-by: Max Reitz
However, the function’s description says that it will return NULL on
error. But if bdrv_refresh_perms() fails, it will still return a
non-NULL child. Is that right?
No, it's reset to NULL on transaction abort, so code is correct. It's
not obvious, and I've added a comment and assertion in my version of
this fix "[PATCH 1/6] block: fix leak of tran in bdrv_root_attach_child"
The fact that the transaction keeps the pointer to this local variable
around is a bit horrifying, but well.
Max
Re: [PATCH 4/6] block: improve bdrv_child_get_parent_desc()
03.05.2021 14:34, Vladimir Sementsov-Ogievskiy wrote:
We have different types of parents: block nodes, block backends and
jobs. So, it makes sense to specify type together with name.
I forget to note the main thing of the commit:
This handler us used to compose an error message about permission conflict. And
permission conflict occurs in a specific place of block graph. We shouldn't
report name of parent device (as it would define another place in block graph),
but exactly and only the name of the node. So, use bdrv_get_node_name()
directly.
iotest 283 output is updated.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block.c| 2 +-
tests/qemu-iotests/283.out | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/block.c b/block.c
index c4023ab4f4..1de2365843 100644
--- a/block.c
+++ b/block.c
@@ -1160,7 +1160,7 @@ int bdrv_parse_cache_mode(const char *mode, int *flags,
bool *writethrough)
static char *bdrv_child_get_parent_desc(BdrvChild *c)
{
BlockDriverState *parent = c->opaque;
-return g_strdup(bdrv_get_device_or_node_name(parent));
+return g_strdup_printf("node '%s'", bdrv_get_node_name(parent));
}
static void bdrv_child_cb_drained_begin(BdrvChild *child)
diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out
index 97e62a4c94..c9397bfc44 100644
--- a/tests/qemu-iotests/283.out
+++ b/tests/qemu-iotests/283.out
@@ -5,7 +5,7 @@
{"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": "base", "node-name":
"other", "take-child-perms": ["write"]}}
{"return": {}}
{"execute": "blockdev-backup", "arguments": {"device": "source", "sync": "full",
"target": "target"}}
-{"error": {"class": "GenericError", "desc": "Cannot append backup-top filter:
Conflicts with use by source as 'image', which does not allow 'write' on base"}}
+{"error": {"class": "GenericError", "desc": "Cannot append backup-top filter:
Conflicts with use by node 'source' as 'image', which does not allow 'write' on base"}}
=== backup-top should be gone after job-finalize ===
--
Best regards,
Vladimir
Re: [PATCH 1/2] block: Fix Transaction leak in bdrv_root_attach_child()
On 03.05.21 13:05, Kevin Wolf wrote:
The error path needs to call tran_finalize(), too.
Fixes: CID 1452773
Fixes: 548a74c0dbc858edd1a7ee3045b5f2fe710bd8b1
Signed-off-by: Kevin Wolf
---
block.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/block.c b/block.c
index 874c22c43e..5c0ced6238 100644
--- a/block.c
+++ b/block.c
@@ -2918,13 +2918,14 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState
*child_bs,
child_role, perm, shared_perm, opaque,
, tran, errp);
if (ret < 0) {
-bdrv_unref(child_bs);
-return NULL;
+assert(child == NULL);
+goto out;
}
ret = bdrv_refresh_perms(child_bs, errp);
-tran_finalize(tran, ret);
+out:
+tran_finalize(tran, ret);
bdrv_unref(child_bs);
return child;
Looks OK, so:
Reviewed-by: Max Reitz
However, the function’s description says that it will return NULL on
error. But if bdrv_refresh_perms() fails, it will still return a
non-NULL child. Is that right?
Re: [PATCH 2/2] block: Fix Transaction leak in bdrv_reopen_multiple()
03.05.2021 14:05, Kevin Wolf wrote:
Like other error paths, this one needs to call tran_finalize() and clean
up the BlockReopenQueue, too.
We don't need the "abort" loop on that path. And clean-up of BlockReopenQueue is at
"cleanup:" label.
So I'd prefer Peter's suggestion (my "[PATCH 2/6] block: bdrv_reopen_multiple(): fix
leak of tran object")
Fixes: CID 1452772
Fixes: 72373e40fbc7e4218061a8211384db362d3e7348
Signed-off-by: Kevin Wolf
---
block.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 5c0ced6238..69615fabd1 100644
--- a/block.c
+++ b/block.c
@@ -4052,7 +4052,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue,
Error **errp)
ret = bdrv_flush(bs_entry->state.bs);
if (ret < 0) {
error_setg_errno(errp, -ret, "Error flushing drive");
-goto cleanup;
+goto abort;
}
}
--
Best regards,
Vladimir
Re: [PATCH 1/2] block: Fix Transaction leak in bdrv_root_attach_child()
03.05.2021 14:05, Kevin Wolf wrote:
The error path needs to call tran_finalize(), too.
Fixes: CID 1452773
Fixes: 548a74c0dbc858edd1a7ee3045b5f2fe710bd8b1
Signed-off-by: Kevin Wolf
---
block.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/block.c b/block.c
index 874c22c43e..5c0ced6238 100644
--- a/block.c
+++ b/block.c
@@ -2918,13 +2918,14 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState
*child_bs,
child_role, perm, shared_perm, opaque,
, tran, errp);
if (ret < 0) {
-bdrv_unref(child_bs);
-return NULL;
+assert(child == NULL);
+goto out;
}
ret = bdrv_refresh_perms(child_bs, errp);
-tran_finalize(tran, ret);
+out:
+tran_finalize(tran, ret);
bdrv_unref(child_bs);
return child;
}
I like my additional comment and assertion in "[PATCH 1/6] block: fix leak of tran
in bdrv_root_attach_child", still this is OK too:
Reviewed-by: Vladimir Sementsov-Ogievskiy
--
Best regards,
Vladimir
[PATCH 5/6] block: simplify bdrv_child_user_desc()
All existing parent types (block nodes, block devices, jobs) has the
realization. So, drop unreachable code.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block.c | 6 +-
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/block.c b/block.c
index 1de2365843..20efd7a7b0 100644
--- a/block.c
+++ b/block.c
@@ -2037,11 +2037,7 @@ bool bdrv_is_writable(BlockDriverState *bs)
static char *bdrv_child_user_desc(BdrvChild *c)
{
-if (c->klass->get_parent_desc) {
-return c->klass->get_parent_desc(c);
-}
-
-return g_strdup("another user");
+return c->klass->get_parent_desc(c);
}
static bool bdrv_a_allow_b(BdrvChild *a, BdrvChild *b, Error **errp)
--
2.29.2
[PATCH 3/6] block-backend: improve blk_root_get_parent_desc()
We have different types of parents: block nodes, block backends and
jobs. So, it makes sense to specify type together with name.
While being here also use g_autofree.
iotest 307 output is updated.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/block-backend.c | 9 -
tests/qemu-iotests/307.out | 2 +-
2 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index 6fca9853e1..2b7e9b5192 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -142,19 +142,18 @@ static void blk_root_set_aio_ctx(BdrvChild *child,
AioContext *ctx,
static char *blk_root_get_parent_desc(BdrvChild *child)
{
BlockBackend *blk = child->opaque;
-char *dev_id;
+g_autofree char *dev_id = NULL;
if (blk->name) {
-return g_strdup(blk->name);
+return g_strdup_printf("block device '%s'", blk->name);
}
dev_id = blk_get_attached_dev_id(blk);
if (*dev_id) {
-return dev_id;
+return g_strdup_printf("block device '%s'", dev_id);
} else {
/* TODO Callback into the BB owner for something more detailed */
-g_free(dev_id);
-return g_strdup("a block device");
+return g_strdup("unnamed block device");
}
}
diff --git a/tests/qemu-iotests/307.out b/tests/qemu-iotests/307.out
index daa8ad2da0..66bf2ddb74 100644
--- a/tests/qemu-iotests/307.out
+++ b/tests/qemu-iotests/307.out
@@ -53,7 +53,7 @@ exports available: 1
=== Add a writable export ===
{"execute": "block-export-add", "arguments": {"description": "This is the
writable second export", "id": "export1", "name": "export1", "node-name":
"fmt", "type": "nbd", "writable": true, "writethrough": true}}
-{"error": {"class": "GenericError", "desc": "Conflicts with use by sda as
'root', which does not allow 'write' on fmt"}}
+{"error": {"class": "GenericError", "desc": "Conflicts with use by block
device 'sda' as 'root', which does not allow 'write' on fmt"}}
{"execute": "device_del", "arguments": {"id": "sda"}}
{"return": {}}
{"data": {"device": "sda", "path": "/machine/peripheral/sda"}, "event":
"DEVICE_DELETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
--
2.29.2
[PATCH 0/6] block permission updated follow-up
Hi all! Here are two coverity fixes and improvement of error message. Vladimir Sementsov-Ogievskiy (6): block: fix leak of tran in bdrv_root_attach_child block: bdrv_reopen_multiple(): fix leak of tran object block-backend: improve blk_root_get_parent_desc() block: improve bdrv_child_get_parent_desc() block: simplify bdrv_child_user_desc() block: improve permission conflict error message block.c | 51 ++- block/block-backend.c | 9 +++-- tests/qemu-iotests/283.out| 2 +- tests/qemu-iotests/307.out| 2 +- tests/qemu-iotests/tests/qsd-jobs.out | 2 +- 5 files changed, 42 insertions(+), 24 deletions(-) -- 2.29.2
[PATCH 6/6] block: improve permission conflict error message
Now permissions are updated as follows:
1. do graph modifications ignoring permissions
2. do permission update
(of course, we rollback [1] if [2] fails)
So, on stage [2] we can't say which users are "old" and which are
"new" and exist only since [1]. And current error message is a bit
outdated. Let's improve it, to make everything clean.
While being here, add also a comment and some good assertions.
iotests 283, 307, qsd-jobs outputs are updated.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block.c | 29 ---
tests/qemu-iotests/283.out| 2 +-
tests/qemu-iotests/307.out| 2 +-
tests/qemu-iotests/tests/qsd-jobs.out | 2 +-
4 files changed, 25 insertions(+), 10 deletions(-)
diff --git a/block.c b/block.c
index 20efd7a7b0..3fc87fbf90 100644
--- a/block.c
+++ b/block.c
@@ -2040,20 +2040,35 @@ static char *bdrv_child_user_desc(BdrvChild *c)
return c->klass->get_parent_desc(c);
}
+/*
+ * Check that @a allows everything that @b needs. @a and @b must reference same
+ * child node.
+ */
static bool bdrv_a_allow_b(BdrvChild *a, BdrvChild *b, Error **errp)
{
-g_autofree char *user = NULL;
-g_autofree char *perm_names = NULL;
+g_autofree char *a_user = NULL;
+g_autofree char *a_against = NULL;
+g_autofree char *b_user = NULL;
+g_autofree char *b_perm = NULL;
+
+assert(a->bs);
+assert(a->bs == b->bs);
if ((b->perm & a->shared_perm) == b->perm) {
return true;
}
-perm_names = bdrv_perm_names(b->perm & ~a->shared_perm);
-user = bdrv_child_user_desc(a);
-error_setg(errp, "Conflicts with use by %s as '%s', which does not "
- "allow '%s' on %s",
- user, a->name, perm_names, bdrv_get_node_name(b->bs));
+a_user = bdrv_child_user_desc(a);
+a_against = bdrv_perm_names(b->perm & ~a->shared_perm);
+
+b_user = bdrv_child_user_desc(b);
+b_perm = bdrv_perm_names(b->perm);
+
+error_setg(errp, "Permission conflict on node '%s': %s wants to use it as "
+ "'%s', which requires these permissions: %s. On the other hand
%s "
+ "wants to use it as '%s', which doesn't share: %s",
+ bdrv_get_node_name(b->bs),
+ b_user, b->name, b_perm, a_user, a->name, a_against);
return false;
}
diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out
index c9397bfc44..92f3cc1ed5 100644
--- a/tests/qemu-iotests/283.out
+++ b/tests/qemu-iotests/283.out
@@ -5,7 +5,7 @@
{"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image":
"base", "node-name": "other", "take-child-perms": ["write"]}}
{"return": {}}
{"execute": "blockdev-backup", "arguments": {"device": "source", "sync":
"full", "target": "target"}}
-{"error": {"class": "GenericError", "desc": "Cannot append backup-top filter:
Conflicts with use by node 'source' as 'image', which does not allow 'write' on
base"}}
+{"error": {"class": "GenericError", "desc": "Cannot append backup-top filter:
Permission conflict on node 'base': node 'other' wants to use it as 'image',
which requires these permissions: write. On the other hand node 'source' wants
to use it as 'image', which doesn't share: write"}}
=== backup-top should be gone after job-finalize ===
diff --git a/tests/qemu-iotests/307.out b/tests/qemu-iotests/307.out
index 66bf2ddb74..e03932ba4f 100644
--- a/tests/qemu-iotests/307.out
+++ b/tests/qemu-iotests/307.out
@@ -53,7 +53,7 @@ exports available: 1
=== Add a writable export ===
{"execute": "block-export-add", "arguments": {"description": "This is the
writable second export", "id": "export1", "name": "export1", "node-name":
"fmt", "type": "nbd", "writable": true, "writethrough": true}}
-{"error": {"class": "GenericError", "desc": "Conflicts with use by block
device 'sda' as 'root', which does not allow 'write' on fmt"}}
+{"error": {"class": "GenericError", "desc": "Permission conflict on node
'fmt': unnamed block device wants to use it as 'root', which requires these
permissions: consistent read, write. On the other hand block device 'sda' wants
to use it as 'root', which doesn't share: write"}}
{"execute": "device_del", "arguments": {"id": "sda"}}
{"return": {}}
{"data": {"device": "sda", "path": "/machine/peripheral/sda"}, "event":
"DEVICE_DELETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
diff --git a/tests/qemu-iotests/tests/qsd-jobs.out
b/tests/qemu-iotests/tests/qsd-jobs.out
index 9f52255da8..b0596d2c95 100644
--- a/tests/qemu-iotests/tests/qsd-jobs.out
+++ b/tests/qemu-iotests/tests/qsd-jobs.out
@@ -16,7 +16,7 @@ QMP_VERSION
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"JOB_STATUS_CHANGE", "data": {"status": "created", "id": "job0"}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"JOB_STATUS_CHANGE", "data": {"status": "null", "id": "job0"}}
-{"error": {"class":
[PATCH 4/6] block: improve bdrv_child_get_parent_desc()
We have different types of parents: block nodes, block backends and
jobs. So, it makes sense to specify type together with name.
iotest 283 output is updated.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block.c| 2 +-
tests/qemu-iotests/283.out | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/block.c b/block.c
index c4023ab4f4..1de2365843 100644
--- a/block.c
+++ b/block.c
@@ -1160,7 +1160,7 @@ int bdrv_parse_cache_mode(const char *mode, int *flags,
bool *writethrough)
static char *bdrv_child_get_parent_desc(BdrvChild *c)
{
BlockDriverState *parent = c->opaque;
-return g_strdup(bdrv_get_device_or_node_name(parent));
+return g_strdup_printf("node '%s'", bdrv_get_node_name(parent));
}
static void bdrv_child_cb_drained_begin(BdrvChild *child)
diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out
index 97e62a4c94..c9397bfc44 100644
--- a/tests/qemu-iotests/283.out
+++ b/tests/qemu-iotests/283.out
@@ -5,7 +5,7 @@
{"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image":
"base", "node-name": "other", "take-child-perms": ["write"]}}
{"return": {}}
{"execute": "blockdev-backup", "arguments": {"device": "source", "sync":
"full", "target": "target"}}
-{"error": {"class": "GenericError", "desc": "Cannot append backup-top filter:
Conflicts with use by source as 'image', which does not allow 'write' on base"}}
+{"error": {"class": "GenericError", "desc": "Cannot append backup-top filter:
Conflicts with use by node 'source' as 'image', which does not allow 'write' on
base"}}
=== backup-top should be gone after job-finalize ===
--
2.29.2
[PATCH 2/6] block: bdrv_reopen_multiple(): fix leak of tran object
We have one path, where tran object is created, but we don't touch and
don't free it in any way: "goto cleanup" in first loop with calls to
bdrv_flush().
Fix it simply moving tran_new() call below that loop.
Reported-by: Coverity (CID 1452772)
Reported-by: Peter Maydell
Suggested-by: Peter Maydell
Fixes: 72373e40fbc7e4218061a8211384db362d3e7348
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 728aa34b2f..c4023ab4f4 100644
--- a/block.c
+++ b/block.c
@@ -4047,7 +4047,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue,
Error **errp)
{
int ret = -1;
BlockReopenQueueEntry *bs_entry, *next;
-Transaction *tran = tran_new();
+Transaction *tran;
g_autoptr(GHashTable) found = NULL;
g_autoptr(GSList) refresh_list = NULL;
@@ -4061,6 +4061,8 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue,
Error **errp)
}
}
+tran = tran_new();
+
QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
assert(bs_entry->state.bs->quiesce_counter > 0);
ret = bdrv_reopen_prepare(_entry->state, bs_queue, tran, errp);
--
2.29.2
[PATCH 1/6] block: fix leak of tran in bdrv_root_attach_child
bdrv_attach_child_common() doesn't require tran_finalize() on failure
(it does tran_add() only on success path). Still tran_new() must be
paired with tran_finalize() anyway, at least to free empty Transaction
object itself.
So, refactor the function for clean finalization code, same on all
paths.
While being here, also leave a comment on unobvious background zeroing
of child pointer on failure path.
Reported-by: Coverity (CID 1452773)
Reported-by: Peter Maydell
Fixes: 548a74c0dbc858edd1a7ee3045b5f2fe710bd8b1
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/block.c b/block.c
index 874c22c43e..728aa34b2f 100644
--- a/block.c
+++ b/block.c
@@ -2918,12 +2918,18 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState
*child_bs,
child_role, perm, shared_perm, opaque,
, tran, errp);
if (ret < 0) {
-bdrv_unref(child_bs);
-return NULL;
+goto out;
}
ret = bdrv_refresh_perms(child_bs, errp);
+if (ret < 0) {
+goto out;
+}
+
+out:
tran_finalize(tran, ret);
+/* child is unset on failure by bdrv_attach_child_common_abort() */
+assert((ret < 0) == !child);
bdrv_unref(child_bs);
return child;
--
2.29.2
[PATCH 0/2] block: Fix Transaction leaks
These are two follow-up fixes for Vladimir's "block: update graph permissions update". The bugs were reported by Coverity. Kevin Wolf (2): block: Fix Transaction leak in bdrv_root_attach_child() block: Fix Transaction leak in bdrv_reopen_multiple() block.c | 9 + 1 file changed, 5 insertions(+), 4 deletions(-) -- 2.30.2
[PATCH 2/2] block: Fix Transaction leak in bdrv_reopen_multiple()
Like other error paths, this one needs to call tran_finalize() and clean
up the BlockReopenQueue, too.
Fixes: CID 1452772
Fixes: 72373e40fbc7e4218061a8211384db362d3e7348
Signed-off-by: Kevin Wolf
---
block.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 5c0ced6238..69615fabd1 100644
--- a/block.c
+++ b/block.c
@@ -4052,7 +4052,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue,
Error **errp)
ret = bdrv_flush(bs_entry->state.bs);
if (ret < 0) {
error_setg_errno(errp, -ret, "Error flushing drive");
-goto cleanup;
+goto abort;
}
}
--
2.30.2
[PATCH 1/2] block: Fix Transaction leak in bdrv_root_attach_child()
The error path needs to call tran_finalize(), too.
Fixes: CID 1452773
Fixes: 548a74c0dbc858edd1a7ee3045b5f2fe710bd8b1
Signed-off-by: Kevin Wolf
---
block.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/block.c b/block.c
index 874c22c43e..5c0ced6238 100644
--- a/block.c
+++ b/block.c
@@ -2918,13 +2918,14 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState
*child_bs,
child_role, perm, shared_perm, opaque,
, tran, errp);
if (ret < 0) {
-bdrv_unref(child_bs);
-return NULL;
+assert(child == NULL);
+goto out;
}
ret = bdrv_refresh_perms(child_bs, errp);
-tran_finalize(tran, ret);
+out:
+tran_finalize(tran, ret);
bdrv_unref(child_bs);
return child;
}
--
2.30.2
Re: [PULL 37/64] block/snapshot: Fix fallback
Am 03.05.2021 um 11:45 hat Max Reitz geschrieben:
> On 03.05.21 11:40, Kevin Wolf wrote:
> > Am 01.05.2021 um 00:30 hat Peter Maydell geschrieben:
> > > On Mon, 7 Sept 2020 at 12:11, Kevin Wolf wrote:
> > > >
> > > > From: Max Reitz
> > > >
> > > > If the top node's driver does not provide snapshot functionality and we
> > > > want to fall back to a node down the chain, we need to snapshot all
> > > > non-COW children. For simplicity's sake, just do not fall back if there
> > > > is more than one such child. Furthermore, we really only can fall back
> > > > to bs->file and bs->backing, because bdrv_snapshot_goto() has to modify
> > > > the child link (notably, set it to NULL).
> > > >
> > > > Suggested-by: Vladimir Sementsov-Ogievskiy
> > > > Signed-off-by: Max Reitz
> > > > Reviewed-by: Andrey Shinkevich
> > > > Reviewed-by: Kevin Wolf
> > >
> > > Hi; Coverity thinks it's found a problem with this code
> > > (CID 1452774):
> >
> > Cc: Max as the patch author
>
> Yes, I’m writing a patch to add a comment.
>
> > > > @@ -205,39 +258,46 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
> > > > return ret;
> > > > }
> > > >
> > > > -if (bs->file) {
> > > > -BlockDriverState *file;
> > > > -QDict *options = qdict_clone_shallow(bs->options);
> > > > +fallback_ptr = bdrv_snapshot_fallback_ptr(bs);
> > > > +if (fallback_ptr) {
> > > > +QDict *options;
> > > > QDict *file_options;
> > > > Error *local_err = NULL;
> > > > +BlockDriverState *fallback_bs = (*fallback_ptr)->bs;
> > > > +char *subqdict_prefix = g_strdup_printf("%s.",
> > > > (*fallback_ptr)->name);
> > > > +
> > > > +options = qdict_clone_shallow(bs->options);
> > > >
> > > > -file = bs->file->bs;
> > > > /* Prevent it from getting deleted when detached from bs */
> > > > -bdrv_ref(file);
> > > > +bdrv_ref(fallback_bs);
> > > >
> > > > -qdict_extract_subqdict(options, _options, "file.");
> > > > +qdict_extract_subqdict(options, _options,
> > > > subqdict_prefix);
> > > > qobject_unref(file_options);
> > > > -qdict_put_str(options, "file", bdrv_get_node_name(file));
> > > > +g_free(subqdict_prefix);
> > > > +
> > > > +qdict_put_str(options, (*fallback_ptr)->name,
> > > > + bdrv_get_node_name(fallback_bs));
> > > >
> > > > if (drv->bdrv_close) {
> > > > drv->bdrv_close(bs);
> > > > }
> > > > -bdrv_unref_child(bs, bs->file);
> > > > -bs->file = NULL;
> > > >
> > > > -ret = bdrv_snapshot_goto(file, snapshot_id, errp);
> > > > +bdrv_unref_child(bs, *fallback_ptr);
> > > > +*fallback_ptr = NULL;
> > >
> > > Here we set *fallback_ptr to NULL...
> > >
> > > > +
> > > > +ret = bdrv_snapshot_goto(fallback_bs, snapshot_id, errp);
> > > > open_ret = drv->bdrv_open(bs, options, bs->open_flags,
> > > > _err);
> > > > qobject_unref(options);
> > > > if (open_ret < 0) {
> > > > -bdrv_unref(file);
> > > > +bdrv_unref(fallback_bs);
> > > > bs->drv = NULL;
> > > > /* A bdrv_snapshot_goto() error takes precedence */
> > > > error_propagate(errp, local_err);
> > > > return ret < 0 ? ret : open_ret;
> > > > }
> > > >
> > > > -assert(bs->file->bs == file);
> > > > -bdrv_unref(file);
> > > > +assert(fallback_bs == (*fallback_ptr)->bs);
> > >
> > > ...but here we dereference *fallback_ptr, and Coverity doesn't see
> > > anything that it recognizes as being able to change it.
> > >
> > > > +bdrv_unref(fallback_bs);
> > > > return ret;
> > > > }
> > >
> > > False positive, or real issue? (If a false positive, a comment
> > > explaining what's going on wouldn't go amiss -- as a human reader
> > > I'm kind of confused about whether there's some kind of hidden
> > > magic going on here.)
> >
> > I think it's a false positive because drv->bdrv_open() is supposed to
> > give it a non-NULL value again. Not sure if we can make the assumption
> > in every case without checking it, but it feels reasonable to require
> > that drv->bdrv_open() would return failure otherwise. Max?
>
> Yes. I think it’s sensible to add an *fallback_ptr non-NULL check to the
> assert condition (i.e.,
>
> assert(*fallback_ptr && fallback_bs == (*fallback_ptr)->bs);
>
> ), because the intention of the condition is already to verify that
> .bdrv_open() has opened the right node. So we might say what’s missing is
> to also assert that it has opened any node at all, but if we’re fine with
> asserting that it has opened the right node (which we did since
> 7a9e51198c24), we should definitely be fine with asserting that it has
> opened any node at all.
True, that's a good point.
Kevin
Re: [PATCH v2 2/3] hw/pci-host/raven: Manually reset the OR_IRQ device
On 5/2/21 10:45 PM, Peter Maydell wrote:
> On Sun, 2 May 2021 at 21:31, Philippe Mathieu-Daudé wrote:
>>
>> The OR_IRQ device is bus-less, thus isn't reset automatically.
>> Add the raven_pcihost_reset() handler to manually reset the OR IRQ.
>>
>> Fixes: f40b83a4e31 ("40p: use OR gate to wire up raven PCI interrupts")
>> Signed-off-by: Philippe Mathieu-Daudé
>> ---
>> hw/pci-host/prep.c | 11 +++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
>> index 0a9162fba97..7481bbf99d4 100644
>> --- a/hw/pci-host/prep.c
>> +++ b/hw/pci-host/prep.c
>> @@ -230,6 +230,15 @@ static void raven_change_gpio(void *opaque, int n, int
>> level)
>> s->contiguous_map = level;
>> }
>>
>> +static void raven_pcihost_reset_enter(Object *obj, ResetType type)
>> +{
>> +PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(obj);
>> +
>> +if (!s->is_legacy_prep) {
>> +device_cold_reset(DEVICE(>or_irq));
>> +}
>> +}
>> +
>> static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
>> {
>> SysBusDevice *dev = SYS_BUS_DEVICE(d);
>> @@ -419,11 +428,13 @@ static Property raven_pcihost_properties[] = {
>> static void raven_pcihost_class_init(ObjectClass *klass, void *data)
>> {
>> DeviceClass *dc = DEVICE_CLASS(klass);
>> +ResettableClass *rc = RESETTABLE_CLASS(klass);
>>
>> set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>> dc->realize = raven_pcihost_realizefn;
>> device_class_set_props(dc, raven_pcihost_properties);
>> dc->fw_name = "pci";
>> +rc->phases.enter = raven_pcihost_reset_enter;
>> }
>
> Why does this device have an OR gate rather than having its
> map_irq function say "all PCI IRQs go to interrupt 0" ?
> (The PCI core code provides you the "OR" functionality for
> free, because it has to do that anyway for when multiple PCI
> cards share a PCI IRQ -- see pci_change_irq_level() and
> pci_bus_change_irq_level().
>
> Supplementary question: why does the legacy_prep setup create 4
> outbound sysbus IRQs when the map_irq function can only ever
> return 0 or 1 ?
I'll let the maintainers have a look (I'm just trying to pay the
technical debts so we can remove legacy/deprecated API and move
forward).
Thanks for the careful analysis,
Phil.
[PATCH] block/snapshot: Clarify goto fallback behavior
In the bdrv_snapshot_goto() fallback code, we work with a pointer to
either bs->file or bs->backing. We close that child, close the node
(with .bdrv_close()), apply the snapshot on the child node, and then
re-open the node (with .bdrv_open()).
In order for .bdrv_open() to attach the same child node that we had
before, we pass "file={child-node}" or "backing={child-node}" to it.
Therefore, when .bdrv_open() has returned success, we can assume that
bs->file or bs->backing (respectively) points to our original child
again. This is verified by an assertion.
All of this is not immediately clear from a quick glance at the code,
so add a comment to the assertion what it is for, and why it is valid.
It certainly confused Coverity.
Reported-by: Coverity (CID 1452774)
Signed-off-by: Max Reitz
---
block/snapshot.c | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/block/snapshot.c b/block/snapshot.c
index e8ae9a28c1..cce5e35b35 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -275,13 +275,16 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
qobject_unref(file_options);
g_free(subqdict_prefix);
+/* Force .bdrv_open() below to re-attach fallback_bs on *fallback_ptr
*/
qdict_put_str(options, (*fallback_ptr)->name,
bdrv_get_node_name(fallback_bs));
+/* Now close bs, apply the snapshot on fallback_bs, and re-open bs */
if (drv->bdrv_close) {
drv->bdrv_close(bs);
}
+/* .bdrv_open() will re-attach it */
bdrv_unref_child(bs, *fallback_ptr);
*fallback_ptr = NULL;
@@ -296,7 +299,16 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
return ret < 0 ? ret : open_ret;
}
-assert(fallback_bs == (*fallback_ptr)->bs);
+/*
+ * fallback_ptr is >file or >backing. *fallback_ptr
+ * was closed above and set to NULL, but the .bdrv_open() call
+ * has opened it again, because we set the respective option
+ * (with the qdict_put_str() call above).
+ * Assert that .bdrv_open() has attached some child on
+ * *fallback_ptr, and that it has attached the one we wanted
+ * it to (i.e., fallback_bs).
+ */
+assert(*fallback_ptr && fallback_bs == (*fallback_ptr)->bs);
bdrv_unref(fallback_bs);
return ret;
}
--
2.31.1
Re: [PULL 37/64] block/snapshot: Fix fallback
On 03.05.21 11:40, Kevin Wolf wrote:
Am 01.05.2021 um 00:30 hat Peter Maydell geschrieben:
On Mon, 7 Sept 2020 at 12:11, Kevin Wolf wrote:
From: Max Reitz
If the top node's driver does not provide snapshot functionality and we
want to fall back to a node down the chain, we need to snapshot all
non-COW children. For simplicity's sake, just do not fall back if there
is more than one such child. Furthermore, we really only can fall back
to bs->file and bs->backing, because bdrv_snapshot_goto() has to modify
the child link (notably, set it to NULL).
Suggested-by: Vladimir Sementsov-Ogievskiy
Signed-off-by: Max Reitz
Reviewed-by: Andrey Shinkevich
Reviewed-by: Kevin Wolf
Hi; Coverity thinks it's found a problem with this code
(CID 1452774):
Cc: Max as the patch author
Yes, I’m writing a patch to add a comment.
@@ -205,39 +258,46 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
return ret;
}
-if (bs->file) {
-BlockDriverState *file;
-QDict *options = qdict_clone_shallow(bs->options);
+fallback_ptr = bdrv_snapshot_fallback_ptr(bs);
+if (fallback_ptr) {
+QDict *options;
QDict *file_options;
Error *local_err = NULL;
+BlockDriverState *fallback_bs = (*fallback_ptr)->bs;
+char *subqdict_prefix = g_strdup_printf("%s.", (*fallback_ptr)->name);
+
+options = qdict_clone_shallow(bs->options);
-file = bs->file->bs;
/* Prevent it from getting deleted when detached from bs */
-bdrv_ref(file);
+bdrv_ref(fallback_bs);
-qdict_extract_subqdict(options, _options, "file.");
+qdict_extract_subqdict(options, _options, subqdict_prefix);
qobject_unref(file_options);
-qdict_put_str(options, "file", bdrv_get_node_name(file));
+g_free(subqdict_prefix);
+
+qdict_put_str(options, (*fallback_ptr)->name,
+ bdrv_get_node_name(fallback_bs));
if (drv->bdrv_close) {
drv->bdrv_close(bs);
}
-bdrv_unref_child(bs, bs->file);
-bs->file = NULL;
-ret = bdrv_snapshot_goto(file, snapshot_id, errp);
+bdrv_unref_child(bs, *fallback_ptr);
+*fallback_ptr = NULL;
Here we set *fallback_ptr to NULL...
+
+ret = bdrv_snapshot_goto(fallback_bs, snapshot_id, errp);
open_ret = drv->bdrv_open(bs, options, bs->open_flags, _err);
qobject_unref(options);
if (open_ret < 0) {
-bdrv_unref(file);
+bdrv_unref(fallback_bs);
bs->drv = NULL;
/* A bdrv_snapshot_goto() error takes precedence */
error_propagate(errp, local_err);
return ret < 0 ? ret : open_ret;
}
-assert(bs->file->bs == file);
-bdrv_unref(file);
+assert(fallback_bs == (*fallback_ptr)->bs);
...but here we dereference *fallback_ptr, and Coverity doesn't see
anything that it recognizes as being able to change it.
+bdrv_unref(fallback_bs);
return ret;
}
False positive, or real issue? (If a false positive, a comment
explaining what's going on wouldn't go amiss -- as a human reader
I'm kind of confused about whether there's some kind of hidden
magic going on here.)
I think it's a false positive because drv->bdrv_open() is supposed to
give it a non-NULL value again. Not sure if we can make the assumption
in every case without checking it, but it feels reasonable to require
that drv->bdrv_open() would return failure otherwise. Max?
Yes. I think it’s sensible to add an *fallback_ptr non-NULL check to
the assert condition (i.e.,
assert(*fallback_ptr && fallback_bs == (*fallback_ptr)->bs);
), because the intention of the condition is already to verify that
.bdrv_open() has opened the right node. So we might say what’s missing
is to also assert that it has opened any node at all, but if we’re fine
with asserting that it has opened the right node (which we did since
7a9e51198c24), we should definitely be fine with asserting that it has
opened any node at all.
Max
Re: [PULL 37/64] block/snapshot: Fix fallback
Am 01.05.2021 um 00:30 hat Peter Maydell geschrieben:
> On Mon, 7 Sept 2020 at 12:11, Kevin Wolf wrote:
> >
> > From: Max Reitz
> >
> > If the top node's driver does not provide snapshot functionality and we
> > want to fall back to a node down the chain, we need to snapshot all
> > non-COW children. For simplicity's sake, just do not fall back if there
> > is more than one such child. Furthermore, we really only can fall back
> > to bs->file and bs->backing, because bdrv_snapshot_goto() has to modify
> > the child link (notably, set it to NULL).
> >
> > Suggested-by: Vladimir Sementsov-Ogievskiy
> > Signed-off-by: Max Reitz
> > Reviewed-by: Andrey Shinkevich
> > Reviewed-by: Kevin Wolf
>
> Hi; Coverity thinks it's found a problem with this code
> (CID 1452774):
Cc: Max as the patch author
> > @@ -205,39 +258,46 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
> > return ret;
> > }
> >
> > -if (bs->file) {
> > -BlockDriverState *file;
> > -QDict *options = qdict_clone_shallow(bs->options);
> > +fallback_ptr = bdrv_snapshot_fallback_ptr(bs);
> > +if (fallback_ptr) {
> > +QDict *options;
> > QDict *file_options;
> > Error *local_err = NULL;
> > +BlockDriverState *fallback_bs = (*fallback_ptr)->bs;
> > +char *subqdict_prefix = g_strdup_printf("%s.",
> > (*fallback_ptr)->name);
> > +
> > +options = qdict_clone_shallow(bs->options);
> >
> > -file = bs->file->bs;
> > /* Prevent it from getting deleted when detached from bs */
> > -bdrv_ref(file);
> > +bdrv_ref(fallback_bs);
> >
> > -qdict_extract_subqdict(options, _options, "file.");
> > +qdict_extract_subqdict(options, _options, subqdict_prefix);
> > qobject_unref(file_options);
> > -qdict_put_str(options, "file", bdrv_get_node_name(file));
> > +g_free(subqdict_prefix);
> > +
> > +qdict_put_str(options, (*fallback_ptr)->name,
> > + bdrv_get_node_name(fallback_bs));
> >
> > if (drv->bdrv_close) {
> > drv->bdrv_close(bs);
> > }
> > -bdrv_unref_child(bs, bs->file);
> > -bs->file = NULL;
> >
> > -ret = bdrv_snapshot_goto(file, snapshot_id, errp);
> > +bdrv_unref_child(bs, *fallback_ptr);
> > +*fallback_ptr = NULL;
>
> Here we set *fallback_ptr to NULL...
>
> > +
> > +ret = bdrv_snapshot_goto(fallback_bs, snapshot_id, errp);
> > open_ret = drv->bdrv_open(bs, options, bs->open_flags, _err);
> > qobject_unref(options);
> > if (open_ret < 0) {
> > -bdrv_unref(file);
> > +bdrv_unref(fallback_bs);
> > bs->drv = NULL;
> > /* A bdrv_snapshot_goto() error takes precedence */
> > error_propagate(errp, local_err);
> > return ret < 0 ? ret : open_ret;
> > }
> >
> > -assert(bs->file->bs == file);
> > -bdrv_unref(file);
> > +assert(fallback_bs == (*fallback_ptr)->bs);
>
> ...but here we dereference *fallback_ptr, and Coverity doesn't see
> anything that it recognizes as being able to change it.
>
> > +bdrv_unref(fallback_bs);
> > return ret;
> > }
>
> False positive, or real issue? (If a false positive, a comment
> explaining what's going on wouldn't go amiss -- as a human reader
> I'm kind of confused about whether there's some kind of hidden
> magic going on here.)
I think it's a false positive because drv->bdrv_open() is supposed to
give it a non-NULL value again. Not sure if we can make the assumption
in every case without checking it, but it feels reasonable to require
that drv->bdrv_open() would return failure otherwise. Max?
Kevin
Re: [PATCH] block: Drop the sheepdog block driver
On Sat, May 01, 2021 at 09:57:47 +0200, Markus Armbruster wrote: > It was deprecated in commit e1c4269763, v5.2.0. See that commit > message for rationale. > > Signed-off-by: Markus Armbruster > --- > docs/system/deprecated.rst |9 - > docs/system/device-url-syntax.rst.inc | 18 - > docs/system/qemu-block-drivers.rst.inc | 69 - > docs/system/removed-features.rst |7 + > configure | 10 - > meson.build|1 - > qapi/block-core.json | 93 +- > qapi/transaction.json |8 +- > block/sheepdog.c | 3356 > .gitlab-ci.yml |1 - > MAINTAINERS|6 - > block/meson.build |1 - > block/trace-events | 14 - > tests/qemu-iotests/005 |5 - > tests/qemu-iotests/025 |2 +- > tests/qemu-iotests/check |3 +- > tests/qemu-iotests/common.rc |4 - > 17 files changed, 14 insertions(+), 3593 deletions(-) > delete mode 100644 block/sheepdog.c Libvirt will need to adjust one test case (lock it to qemu-6.0 test data), but other than that, this change is okay with us. ACKed-by: Peter Krempa
Re: [PATCH] block: simplify write-threshold and drop write notifiers
30.04.2021 13:04, Max Reitz wrote:
On 22.04.21 00:09, Vladimir Sementsov-Ogievskiy wrote:
write-notifiers are used only for write-threshold. New code for such
purpose should create filters.
Let's handle write-threshold simply in generic code and drop write
notifiers at all.
Also move part of write-threshold API that is used only for testing to
the test.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
I agree that this could be split into 2-3 parts and not combining
everything into one. But I'm tired now.
Er... You should have put it off until the next day then? O:)
Yes, sorry. I wanted to send that day... Don't remember why :) Checked now,
that was not Friday.. I wanted to drop write notifiers long ago, and when I
finally do it I couldn't wait, so cool it seemed to me :)
Thanks for comments, I'll send v2 soon.
It should be multiple patches. At least one to move the write threshold update
to block/io.c, and then another to drop write notifiers.
I can send v2 if needed, so
consider it as RFC. Or take as is if you think it's not too much-in-one.
I also suggest this as a prepartion (and partly substitution) for
"[PATCH v2 0/8] Block layer thread-safety, continued"
include/block/block_int.h | 12 -
include/block/write-threshold.h | 24 -
block.c | 1 -
block/io.c | 21 +---
block/write-threshold.c | 87 ++-
tests/unit/test-write-threshold.c | 38 ++
6 files changed, 54 insertions(+), 129 deletions(-)
[...]
diff --git a/block/io.c b/block/io.c
index ca2dca3007..e0aa775952 100644
--- a/block/io.c
+++ b/block/io.c
@@ -36,6 +36,8 @@
#include "qemu/main-loop.h"
#include "sysemu/replay.h"
+#include "qapi/qapi-events-block-core.h"
+
/* Maximum bounce buffer for copy-on-read and write zeroes, in bytes */
#define MAX_BOUNCE_BUFFER (32768 << BDRV_SECTOR_BITS)
@@ -1974,6 +1976,8 @@ bdrv_co_write_req_prepare(BdrvChild *child, int64_t
offset, int64_t bytes,
child->perm & BLK_PERM_RESIZE);
switch (req->type) {
+ uint64_t write_threshold;
+
Works, but I think this is the first time I see a variable declared in a switch
block. What I usually do for such cases is to put a block after the label.
(i.e. case X: { uint64_t write_threshold; ... })
But it wouldn’t hurt to just declare this at the beginning of
bdrv_co_write_req_prepare(), I think.
case BDRV_TRACKED_WRITE:
case BDRV_TRACKED_DISCARD:
if (flags & BDRV_REQ_WRITE_UNCHANGED) {
@@ -1981,8 +1985,15 @@ bdrv_co_write_req_prepare(BdrvChild *child, int64_t
offset, int64_t bytes,
} else {
assert(child->perm & BLK_PERM_WRITE);
}
- return notifier_with_return_list_notify(>before_write_notifiers,
- req);
+ write_threshold = qatomic_read(>write_threshold_offset);
+ if (write_threshold > 0 && offset + bytes > write_threshold) {
+ qapi_event_send_block_write_threshold(
+ bs->node_name,
+ offset + bytes - write_threshold,
+ write_threshold);
+ qatomic_set(>write_threshold_offset, 0);
+ }
I’d put all of this into a function in block/write-threshold.c that’s called
from here.
Max
+ return 0;
case BDRV_TRACKED_TRUNCATE:
assert(child->perm & BLK_PERM_RESIZE);
return 0;
@@ -3137,12 +3148,6 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs,
QEMUIOVector *qiov)
return true;
}
-void bdrv_add_before_write_notifier(BlockDriverState *bs,
- NotifierWithReturn *notifier)
-{
- notifier_with_return_list_add(>before_write_notifiers, notifier);
-}
-
void bdrv_io_plug(BlockDriverState *bs)
{
BdrvChild *child;
--
Best regards,
Vladimir
