docs/devel/secure-coding-practices.rst | 9 ++++

include/block/block.h | 1 +

include/block/block_int.h | 2 -

include/sysemu/block-backend.h | 4 ++

block.c | 82 ++++++++++++++++++++--------------

block/block-backend.c | 26 +++++------

block/block-copy.c | 80 ++++++++++++++++++++++-----------

block/commit.c | 2 +-

block/file-posix.c | 29 ++++++++----

block/io.c | 4 +-

block/qapi.c | 2 +-

block/qcow2-snapshot.c | 2 +-

block/qcow2.c | 5 +--

block/quorum.c | 2 +-

block/snapshot.c | 2 +-

block/vhdx-log.c | 2 +-

block/vvfat.c | 14 +++---

blockdev.c | 3 +-

nbd/server.c | 82 +++++++++++++++++++++++++---------

qemu-io-cmds.c | 14 +++++-

tests/unit/test-block-iothread.c | 6 ---

tests/qemu-iotests/283.out | 2 +-

tests/qemu-iotests/307.out | 2 +-

tests/qemu-iotests/tests/qsd-jobs.out | 2 +-

24 files changed, 241 insertions(+), 138 deletions(-)

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Coverity thinks blk may be NULL. It's a false-positive, as described in

a new comment.

Fixes: Coverity CID 1453194

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Message-Id: <20210519090532.3753-1-vsementsov@virtuozzo.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

---

qemu-io-cmds.c | 14 ++++++++++++--

1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c

index XXXXXXX..XXXXXXX 100644

--- a/qemu-io-cmds.c

+++ b/qemu-io-cmds.c

@@ -XXX,XX +XXX,XX @@ static int command(BlockBackend *blk, const cmdinfo_t *ct, int argc,

return -EINVAL;

}

- /* Request additional permissions if necessary for this command. The caller

+ /*

+ * Request additional permissions if necessary for this command. The caller

* is responsible for restoring the original permissions afterwards if this

- * is what it wants. */

+ * is what it wants.

+ *

+ * Coverity thinks that blk may be NULL in the following if condition. It's

+ * not so: in init_check_command() we fail if blk is NULL for command with

+ * both CMD_FLAG_GLOBAL and CMD_NOFILE_OK flags unset. And in

+ * qemuio_add_command() we assert that command with non-zero .perm field

+ * doesn't set this flags. So, the following assertion is to silence

+ * Coverity:

+ */

+ assert(blk || !ct->perm);

if (ct->perm && blk_is_available(blk)) {

uint64_t orig_perm, orig_shared_perm;

blk_get_perm(blk, &orig_perm, &orig_shared_perm);

--

2.30.2

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Commit 3ca1f3225727419ba573673b744edac10904276f

"block: BdrvChildClass: add .get_parent_aio_context handler" introduced

new handler and commit 228ca37e12f97788e05bd0c92f89b3e5e4019607

"block: drop ctx argument from bdrv_root_attach_child" made a generic

use of it. But 3ca1f3225727419ba573673b744edac10904276f didn't update

child_vvfat_qcow. Fix that.

Before that fix the command

./build/qemu-system-x86_64 -usb -device usb-storage,drive=fat16 \

-drive file=fat:rw:fat-type=16:"<path of a host folder>",id=fat16,format=raw,if=none

crashes:

1 bdrv_child_get_parent_aio_context (c=0x559d62426d20)

at ../block.c:1440

2 bdrv_attach_child_common

(child_bs=0x559d62468190, child_name=0x559d606f9e3d "write-target",

child_class=0x559d60c58d20 <child_vvfat_qcow>, child_role=3,

perm=3, shared_perm=4, opaque=0x559d62445690,

child=0x7ffc74c2acc8, tran=0x559d6246ddd0, errp=0x7ffc74c2ae60)

at ../block.c:2795

3 bdrv_attach_child_noperm

(parent_bs=0x559d62445690, child_bs=0x559d62468190,

child_name=0x559d606f9e3d "write-target",

child_class=0x559d60c58d20 <child_vvfat_qcow>, child_role=3,

child=0x7ffc74c2acc8, tran=0x559d6246ddd0, errp=0x7ffc74c2ae60) at

../block.c:2855

4 bdrv_attach_child

(parent_bs=0x559d62445690, child_bs=0x559d62468190,

child_name=0x559d606f9e3d "write-target",

child_class=0x559d60c58d20 <child_vvfat_qcow>, child_role=3,

errp=0x7ffc74c2ae60) at ../block.c:2953

5 bdrv_open_child

(filename=0x559d62464b80 "/var/tmp/vl.h3TIS4",

options=0x559d6246ec20, bdref_key=0x559d606f9e3d "write-target",

parent=0x559d62445690, child_class=0x559d60c58d20

<child_vvfat_qcow>, child_role=3, allow_none=false,

errp=0x7ffc74c2ae60) at ../block.c:3351

6 enable_write_target (bs=0x559d62445690, errp=0x7ffc74c2ae60) at

../block/vvfat.c:3176

7 vvfat_open (bs=0x559d62445690, options=0x559d6244adb0, flags=155650,

errp=0x7ffc74c2ae60) at ../block/vvfat.c:1236

8 bdrv_open_driver (bs=0x559d62445690, drv=0x559d60d4f7e0

<bdrv_vvfat>, node_name=0x0,

options=0x559d6244adb0, open_flags=155650,

errp=0x7ffc74c2af70) at ../block.c:1557

9 bdrv_open_common (bs=0x559d62445690, file=0x0,

options=0x559d6244adb0, errp=0x7ffc74c2af70) at

...

(gdb) fr 1

#1 0x0000559d603ea3bf in bdrv_child_get_parent_aio_context

(c=0x559d62426d20) at ../block.c:1440

1440 return c->klass->get_parent_aio_context(c);

(gdb) p c->klass

$1 = (const BdrvChildClass *) 0x559d60c58d20 <child_vvfat_qcow>

(gdb) p c->klass->get_parent_aio_context

$2 = (AioContext *(*)(BdrvChild *)) 0x0

Fixes: 3ca1f3225727419ba573673b744edac10904276f

Fixes: 228ca37e12f97788e05bd0c92f89b3e5e4019607

Reported-by: John Arbuckle <programmingkidx@gmail.com>

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Message-Id: <20210524101257.119377-2-vsementsov@virtuozzo.com>

Tested-by: John Arbuckle <programmingkidx@gmail.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

---

include/block/block.h | 1 +

block.c | 4 ++--

block/vvfat.c | 1 +

3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h

index XXXXXXX..XXXXXXX 100644

--- a/include/block/block.h

+++ b/include/block/block.h

@@ -XXX,XX +XXX,XX @@ bool bdrv_child_can_set_aio_context(BdrvChild *c, AioContext *ctx,

bool bdrv_can_set_aio_context(BlockDriverState *bs, AioContext *ctx,

GSList **ignore, Error **errp);

AioContext *bdrv_child_get_parent_aio_context(BdrvChild *c);

+AioContext *child_of_bds_get_parent_aio_context(BdrvChild *c);

int bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz);

int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry *geo);

diff --git a/block.c b/block.c

index XXXXXXX..XXXXXXX 100644

--- a/block.c

+++ b/block.c

@@ -XXX,XX +XXX,XX @@ static int bdrv_child_cb_update_filename(BdrvChild *c, BlockDriverState *base,

return 0;

}

-static AioContext *bdrv_child_cb_get_parent_aio_context(BdrvChild *c)

+AioContext *child_of_bds_get_parent_aio_context(BdrvChild *c)

{

BlockDriverState *bs = c->opaque;

@@ -XXX,XX +XXX,XX @@ const BdrvChildClass child_of_bds = {

.can_set_aio_ctx = bdrv_child_cb_can_set_aio_ctx,

.set_aio_ctx = bdrv_child_cb_set_aio_ctx,

.update_filename = bdrv_child_cb_update_filename,

- .get_parent_aio_context = bdrv_child_cb_get_parent_aio_context,

+ .get_parent_aio_context = child_of_bds_get_parent_aio_context,

};

AioContext *bdrv_child_get_parent_aio_context(BdrvChild *c)

diff --git a/block/vvfat.c b/block/vvfat.c

index XXXXXXX..XXXXXXX 100644

--- a/block/vvfat.c

+++ b/block/vvfat.c

@@ -XXX,XX +XXX,XX @@ static void vvfat_qcow_options(BdrvChildRole role, bool parent_is_format,

static const BdrvChildClass child_vvfat_qcow = {

.parent_is_bds = true,

.inherit_options = vvfat_qcow_options,

+ .get_parent_aio_context = child_of_bds_get_parent_aio_context,

};

static int enable_write_target(BlockDriverState *bs, Error **errp)

--

2.30.2

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

It's wrong to rely on s->qcow in vvfat_child_perm, as on permission

update during bdrv_open_child() call this field is not set yet.

Still prior to aa5a04c7db27eea6b36de32f241b155f0d9ce34d, it didn't

crash, as bdrv_open_child passed NULL as child to bdrv_child_perm(),

and NULL was equal to NULL in assertion (still, it was bad guarantee

for child being s->qcow, not backing :).

Since aa5a04c7db27eea6b36de32f241b155f0d9ce34d

"add bdrv_attach_child_noperm" bdrv_refresh_perms called on parent node

when attaching child, and new correct child pointer is passed to

.bdrv_child_perm. Still, s->qcow is NULL at the moment. Let's rely only

on role instead.

Without that fix,

./build/qemu-system-x86_64 -usb -device usb-storage,drive=fat16 \

-drive \

file=fat:rw:fat-type=16:"<path of a host folder>",id=fat16,format=raw,if=none

crashes:

(gdb) bt

0 raise () at /lib64/libc.so.6

1 abort () at /lib64/libc.so.6

2 _nl_load_domain.cold () at /lib64/libc.so.6

3 annobin_assert.c_end () at /lib64/libc.so.6

4 vvfat_child_perm (bs=0x559186f3d690, c=0x559186f1ed20, role=3,

reopen_queue=0x0, perm=0, shared=31,

nperm=0x7ffe56f28298, nshared=0x7ffe56f282a0) at

../block/vvfat.c:3214

5 bdrv_child_perm (bs=0x559186f3d690, child_bs=0x559186f60190,

c=0x559186f1ed20, role=3, reopen_queue=0x0,

parent_perm=0, parent_shared=31,

nperm=0x7ffe56f28298, nshared=0x7ffe56f282a0)

at ../block.c:2094

6 bdrv_node_refresh_perm (bs=0x559186f3d690, q=0x0,

tran=0x559186f65850, errp=0x7ffe56f28530) at

../block.c:2336

7 bdrv_list_refresh_perms (list=0x559186db5b90 = {...}, q=0x0,

tran=0x559186f65850, errp=0x7ffe56f28530)

at ../block.c:2358

8 bdrv_refresh_perms (bs=0x559186f3d690, errp=0x7ffe56f28530) at

../block.c:2419

9 bdrv_attach_child

(parent_bs=0x559186f3d690, child_bs=0x559186f60190,

child_name=0x559184d83e3d "write-target",

child_class=0x5591852f3b00 <child_vvfat_qcow>, child_role=3,

errp=0x7ffe56f28530) at ../block.c:2959

10 bdrv_open_child

(filename=0x559186f5cb80 "/var/tmp/vl.7WYmFU",

options=0x559186f66c20, bdref_key=0x559184d83e3d "write-target",

parent=0x559186f3d690, child_class=0x5591852f3b00

<child_vvfat_qcow>, child_role=3, allow_none=false,

errp=0x7ffe56f28530) at ../block.c:3351

11 enable_write_target (bs=0x559186f3d690, errp=0x7ffe56f28530) at

../block/vvfat.c:3177

12 vvfat_open (bs=0x559186f3d690, options=0x559186f42db0, flags=155650,

errp=0x7ffe56f28530) at ../block/vvfat.c:1236

13 bdrv_open_driver (bs=0x559186f3d690, drv=0x5591853d97e0

<bdrv_vvfat>, node_name=0x0,

options=0x559186f42db0, open_flags=155650,

errp=0x7ffe56f28640) at ../block.c:1557

14 bdrv_open_common (bs=0x559186f3d690, file=0x0,

options=0x559186f42db0, errp=0x7ffe56f28640) at

../block.c:1833

...

(gdb) fr 4

#4 vvfat_child_perm (bs=0x559186f3d690, c=0x559186f1ed20, role=3,

reopen_queue=0x0, perm=0, shared=31,

nperm=0x7ffe56f28298, nshared=0x7ffe56f282a0) at

../block/vvfat.c:3214

3214 assert(c == s->qcow || (role & BDRV_CHILD_COW));

(gdb) p role

$1 = 3 # BDRV_CHILD_DATA | BDRV_CHILD_METADATA

(gdb) p *c

$2 = {bs = 0x559186f60190, name = 0x559186f669d0 "write-target", klass

= 0x5591852f3b00 <child_vvfat_qcow>, role = 3, opaque =

0x559186f3d690, perm = 3, shared_perm = 4, frozen = false,

parent_quiesce_counter = 0, next = {le_next = 0x0, le_prev =

0x559186f41818}, next_parent = {le_next = 0x0, le_prev =

0x559186f64320}}

(gdb) p s->qcow

$3 = (BdrvChild *) 0x0

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Message-Id: <20210524101257.119377-3-vsementsov@virtuozzo.com>

Tested-by: John Arbuckle <programmingkidx@gmail.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

---

block/vvfat.c | 7 ++-----

1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c

index XXXXXXX..XXXXXXX 100644

--- a/block/vvfat.c

+++ b/block/vvfat.c

@@ -XXX,XX +XXX,XX @@ static void vvfat_child_perm(BlockDriverState *bs, BdrvChild *c,

uint64_t perm, uint64_t shared,

uint64_t *nperm, uint64_t *nshared)

{

- BDRVVVFATState *s = bs->opaque;

-

- assert(c == s->qcow || (role & BDRV_CHILD_COW));

-

- if (c == s->qcow) {

+ if (role & BDRV_CHILD_DATA) {

/* This is a private node, nobody should try to attach to it */

*nperm = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE;

*nshared = BLK_PERM_WRITE_UNCHANGED;

} else {

+ assert(role & BDRV_CHILD_COW);

/* The backing file is there so 'commit' can use it. vvfat doesn't

* access it in any way. */

*nperm = 0;

--

2.30.2

It's better to use accessor function instead of bs->read_only directly.

In some places use bdrv_is_writable() instead of

checking both BDRV_O_RDWR set and BDRV_O_INACTIVE not set.

In bdrv_open_common() it's a bit strange to add one more variable, but

we are going to drop bs->read_only in the next patch, so new ro local

variable substitutes it here.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Message-Id: <20210527154056.70294-2-vsementsov@virtuozzo.com>

diff --git a/block.c b/block.c

index XXXXXXX..XXXXXXX 100644

--- a/block.c

+++ b/block.c

@@ -XXX,XX +XXX,XX @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,

QemuOpts *opts;

BlockDriver *drv;

Error *local_err = NULL;

+ bool ro;

assert(bs->file == NULL);

assert(options != NULL && bs->options != options);

@@ -XXX,XX +XXX,XX @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,

bs->read_only = !(bs->open_flags & BDRV_O_RDWR);

- if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv, bs->read_only)) {

- if (!bs->read_only && bdrv_is_whitelisted(drv, true)) {

+ ro = bdrv_is_read_only(bs);

+

+ if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv, ro)) {

+ if (!ro && bdrv_is_whitelisted(drv, true)) {

ret = bdrv_apply_auto_read_only(bs, NULL, NULL);

} else {

ret = -ENOTSUP;

}

if (ret < 0) {

error_setg(errp,

- !bs->read_only && bdrv_is_whitelisted(drv, true)

+ !ro && bdrv_is_whitelisted(drv, true)

? "Driver '%s' can only be used for read-only devices"

: "Driver '%s' is not whitelisted",

drv->format_name);

@@ -XXX,XX +XXX,XX @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,

assert(qatomic_read(&bs->copy_on_read) == 0);

if (bs->open_flags & BDRV_O_COPY_ON_READ) {

- if (!bs->read_only) {

+ if (!ro) {

bdrv_enable_copy_on_read(bs);

} else {

error_setg(errp, "Can't use copy-on-read on read-only device");

diff --git a/block/block-backend.c b/block/block-backend.c

index XXXXXXX..XXXXXXX 100644

--- a/block/block-backend.c

+++ b/block/block-backend.c

@@ -XXX,XX +XXX,XX @@ void blk_update_root_state(BlockBackend *blk)

assert(blk->root);

blk->root_state.open_flags = blk->root->bs->open_flags;

- blk->root_state.read_only = blk->root->bs->read_only;

+ blk->root_state.read_only = bdrv_is_read_only(blk->root->bs);

blk->root_state.detect_zeroes = blk->root->bs->detect_zeroes;

}

diff --git a/block/commit.c b/block/commit.c

index XXXXXXX..XXXXXXX 100644

--- a/block/commit.c

+++ b/block/commit.c

@@ -XXX,XX +XXX,XX @@ int bdrv_commit(BlockDriverState *bs)

return -EBUSY;

}

- ro = backing_file_bs->read_only;

+ ro = bdrv_is_read_only(backing_file_bs);

if (ro) {

if (bdrv_reopen_set_read_only(backing_file_bs, false, NULL)) {

diff --git a/block/io.c b/block/io.c

index XXXXXXX..XXXXXXX 100644

--- a/block/io.c

+++ b/block/io.c

@@ -XXX,XX +XXX,XX @@ bdrv_co_write_req_prepare(BdrvChild *child, int64_t offset, int64_t bytes,

bdrv_check_request(offset, bytes, &error_abort);

- if (bs->read_only) {

+ if (bdrv_is_read_only(bs)) {

return -EPERM;

}

@@ -XXX,XX +XXX,XX @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact,

if (new_bytes) {

bdrv_make_request_serialising(&req, 1);

}

- if (bs->read_only) {

+ if (bdrv_is_read_only(bs)) {

error_setg(errp, "Image is read-only");

ret = -EACCES;

goto out;

diff --git a/block/qapi.c b/block/qapi.c

index XXXXXXX..XXXXXXX 100644

--- a/block/qapi.c

+++ b/block/qapi.c

@@ -XXX,XX +XXX,XX @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,

info = g_malloc0(sizeof(*info));

info->file = g_strdup(bs->filename);

- info->ro = bs->read_only;

+ info->ro = bdrv_is_read_only(bs);

info->drv = g_strdup(bs->drv->format_name);

info->encrypted = bs->encrypted;

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c

index XXXXXXX..XXXXXXX 100644

--- a/block/qcow2-snapshot.c

+++ b/block/qcow2-snapshot.c

@@ -XXX,XX +XXX,XX @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs,

int new_l1_bytes;

int ret;

- assert(bs->read_only);

+ assert(bdrv_is_read_only(bs));

/* Search the snapshot */

snapshot_index = find_snapshot_by_id_and_name(bs, snapshot_id, name);

diff --git a/include/block/block_int.h b/include/block/block_int.h

index XXXXXXX..XXXXXXX 100644

--- a/include/block/block_int.h

+++ b/include/block/block_int.h

@@ -XXX,XX +XXX,XX @@ struct BlockDriverState {

struct BlockBackendRootState {

int open_flags;

- bool read_only;

BlockdevDetectZeroesOptions detect_zeroes;

};

diff --git a/block/block-backend.c b/block/block-backend.c

index XXXXXXX..XXXXXXX 100644

--- a/block/block-backend.c

+++ b/block/block-backend.c

@@ -XXX,XX +XXX,XX @@ bool blk_supports_write_perm(BlockBackend *blk)

if (bs) {

return !bdrv_is_read_only(bs);

} else {

- return !blk->root_state.read_only;

+ return blk->root_state.open_flags & BDRV_O_RDWR;

}

}

@@ -XXX,XX +XXX,XX @@ void blk_update_root_state(BlockBackend *blk)

assert(blk->root);

blk->root_state.open_flags = blk->root->bs->open_flags;

- blk->root_state.read_only = bdrv_is_read_only(blk->root->bs);

blk->root_state.detect_zeroes = blk->root->bs->detect_zeroes;

}

@@ -XXX,XX +XXX,XX @@ bool blk_get_detect_zeroes_from_root_state(BlockBackend *blk)

*/

int blk_get_open_flags_from_root_state(BlockBackend *blk)

{

- int bs_flags;

-

- bs_flags = blk->root_state.read_only ? 0 : BDRV_O_RDWR;

- bs_flags |= blk->root_state.open_flags & ~BDRV_O_RDWR;

-

- return bs_flags;

+ return blk->root_state.open_flags;

}

BlockBackendRootState *blk_get_root_state(BlockBackend *blk)

From: Thomas Huth <thuth@redhat.com>

A customer reported that running

qemu-img convert -t none -O qcow2 -f qcow2 input.qcow2 output.qcow2

fails for them with the following error message when the images are

stored on a GPFS file system :

qemu-img: error while writing sector 0: Invalid argument

After analyzing the strace output, it seems like the problem is in

handle_aiocb_write_zeroes(): The call to fallocate(FALLOC_FL_PUNCH_HOLE)

returns EINVAL, which can apparently happen if the file system has

a different idea of the granularity of the operation. It's arguably

a bug in GPFS, since the PUNCH_HOLE mode should not result in EINVAL

according to the man-page of fallocate(), but the file system is out

there in production and so we have to deal with it. In commit 294682cc3a

("block: workaround for unaligned byte range in fallocate()") we also

already applied the a work-around for the same problem to the earlier

fallocate(FALLOC_FL_ZERO_RANGE) call, so do it now similar with the

PUNCH_HOLE call. But instead of silently catching and returning

-ENOTSUP (which causes the caller to fall back to writing zeroes),

let's rather inform the user once about the buggy file system and

try the other fallback instead.

Signed-off-by: Thomas Huth <thuth@redhat.com>

Message-Id: <20210527172020.847617-2-thuth@redhat.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

---

block/file-posix.c | 11 +++++++++++

1 file changed, 11 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c

index XXXXXXX..XXXXXXX 100644

--- a/block/file-posix.c

+++ b/block/file-posix.c

@@ -XXX,XX +XXX,XX @@ static int handle_aiocb_write_zeroes(void *opaque)

return ret;

}

s->has_fallocate = false;

+ } else if (ret == -EINVAL) {

+ /*

+ * Some file systems like older versions of GPFS do not like un-

+ * aligned byte ranges, and return EINVAL in such a case, though

+ * they should not do it according to the man-page of fallocate().

+ * Warn about the bad filesystem and try the final fallback instead.

+ */

+ warn_report_once("Your file system is misbehaving: "

+ "fallocate(FALLOC_FL_PUNCH_HOLE) returned EINVAL. "

+ "Please report this bug to your file sytem "

+ "vendor.");

} else if (ret != -ENOTSUP) {

return ret;

} else {

--

2.30.2

From: Thomas Huth <thuth@redhat.com>

If fallocate(... FALLOC_FL_ZERO_RANGE ...) returns EINVAL, it's likely

an indication that the file system is buggy and does not implement

unaligned accesses right. We still might be lucky with the other

fallback fallocate() calls later in this function, though, so we should

not return immediately and try the others first.

Since FALLOC_FL_ZERO_RANGE could also return EINVAL if the file descriptor

is not a regular file, we ignore this filesystem bug silently, without

printing an error message for the user.

Signed-off-by: Thomas Huth <thuth@redhat.com>

Message-Id: <20210527172020.847617-3-thuth@redhat.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

---

block/file-posix.c | 18 +++++++++---------

1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c

index XXXXXXX..XXXXXXX 100644

--- a/block/file-posix.c

+++ b/block/file-posix.c

@@ -XXX,XX +XXX,XX @@ static int handle_aiocb_write_zeroes(void *opaque)

if (s->has_write_zeroes) {

int ret = do_fallocate(s->fd, FALLOC_FL_ZERO_RANGE,

aiocb->aio_offset, aiocb->aio_nbytes);

- if (ret == -EINVAL) {

- /*

- * Allow falling back to pwrite for file systems that

- * do not support fallocate() for an unaligned byte range.

- */

- return -ENOTSUP;

- }

- if (ret == 0 || ret != -ENOTSUP) {

+ if (ret == -ENOTSUP) {

+ s->has_write_zeroes = false;

+ } else if (ret == 0 || ret != -EINVAL) {

return ret;

}

- s->has_write_zeroes = false;

+ /*

+ * Note: Some file systems do not like unaligned byte ranges, and

+ * return EINVAL in such a case, though they should not do it according

+ * to the man-page of fallocate(). Thus we simply ignore this return

+ * value and try the other fallbacks instead.

+ */

}

#endif

--

2.30.2

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

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.

So, let's add documentation and some assertions.

While being here, drop extra declaration of bdrv_attach_child_noperm().

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Message-Id: <20210601075218.79249-2-vsementsov@virtuozzo.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

---

block.c | 24 +++++++++++++++---------

1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c

index XXXXXXX..XXXXXXX 100644

--- a/block.c

+++ b/block.c

@@ -XXX,XX +XXX,XX @@ static BlockDriverState *bdrv_open_inherit(const char *filename,

static void bdrv_replace_child_noperm(BdrvChild *child,

BlockDriverState *new_bs);

-static int bdrv_attach_child_noperm(BlockDriverState *parent_bs,

- BlockDriverState *child_bs,

- const char *child_name,

- const BdrvChildClass *child_class,

- BdrvChildRole child_role,

- BdrvChild **child,

- Transaction *tran,

- Error **errp);

static void bdrv_remove_filter_or_cow_child(BlockDriverState *bs,

Transaction *tran);

@@ -XXX,XX +XXX,XX @@ 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,

@@ -XXX,XX +XXX,XX @@ 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,

@@ -XXX,XX +XXX,XX @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,

child_role, perm, shared_perm, opaque,

&child, tran, errp);

if (ret < 0) {

- assert(child == NULL);

goto out;

}

@@ -XXX,XX +XXX,XX @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,

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;

}

@@ -XXX,XX +XXX,XX @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,

out:

tran_finalize(tran, ret);

+ /* child is unset on failure by bdrv_attach_child_common_abort() */

+ assert((ret < 0) == !child);

bdrv_unref(child_bs);

--

2.30.2

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

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 <vsementsov@virtuozzo.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Message-Id: <20210601075218.79249-3-vsementsov@virtuozzo.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

---

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 XXXXXXX..XXXXXXX 100644

--- a/block/block-backend.c

+++ b/block/block-backend.c

@@ -XXX,XX +XXX,XX @@ 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("an unnamed block device");

}

}

diff --git a/tests/qemu-iotests/307.out b/tests/qemu-iotests/307.out

index XXXXXXX..XXXXXXX 100644

--- a/tests/qemu-iotests/307.out

+++ b/tests/qemu-iotests/307.out

@@ -XXX,XX +XXX,XX @@ 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.30.2

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

We have different types of parents: block nodes, block backends and

jobs. So, it makes sense to specify type together with name.

Next, 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 refers 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 <vsementsov@virtuozzo.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Message-Id: <20210601075218.79249-4-vsementsov@virtuozzo.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

---

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 XXXXXXX..XXXXXXX 100644

--- a/block.c

+++ b/block.c

@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644

--- a/tests/qemu-iotests/283.out

+++ b/tests/qemu-iotests/283.out

@@ -XXX,XX +XXX,XX @@

{"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.30.2