The following changes since commit 418fa86dd465b4fd8394373cf83db8fa65d7611c:

Merge remote-tracking branch 'remotes/stsquad/tags/pull-testing-040220-1' into staging (2020-02-04 18:55:06 +0000)

https://github.com/XanClic/qemu.git tags/pull-block-2020-02-06

for you to fetch changes up to a541fcc27c98b96da187c7d4573f3270f3ddd283:

iotests: add test for backup-top failure on permission activation (2020-02-06 13:47:45 +0100)

Block patches:

- Drop BDRV_SECTOR_SIZE from qcow2

- Allow Python iotests to be added to the auto group

(and add some)

- Fix for the backup job

- Fix memleak in bdrv_refresh_filename()

- Use GStrings in two places for greater efficiency (than manually

handling string allocation)

John Snow (1):

iotests: remove 'linux' from default supported platforms

Pan Nengyuan (1):

block: fix memleaks in bdrv_refresh_filename

Thomas Huth (5):

iotests: Test 041 only works on certain systems

iotests: Test 183 does not work on macOS and OpenBSD

iotests: Check for the availability of the required devices in 267 and

127

iotests: Skip Python-based tests if QEMU does not support virtio-blk

iotests: Enable more tests in the 'auto' group to improve test

coverage

Vladimir Sementsov-Ogievskiy (2):

block/backup-top: fix failure path

iotests: add test for backup-top failure on permission activation

From: Alberto Garcia <berto@igalia.com>

This replaces all remaining instances in the qcow2 code.

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

Message-id: b5f74b606c2d9873b12d29acdb7fd498029c4025.1579374329.git.berto@igalia.com

Reviewed-by: Max Reitz <mreitz@redhat.com>

Signed-off-by: Max Reitz <mreitz@redhat.com>

block/qcow2.c | 8 +++++---

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

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

--- a/block/qcow2.c

+++ b/block/qcow2.c

@@ -XXX,XX +XXX,XX @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)

/* Validate options and set default values */

if (!QEMU_IS_ALIGNED(qcow2_opts->size, BDRV_SECTOR_SIZE)) {

- error_setg(errp, "Image size must be a multiple of 512 bytes");

+ error_setg(errp, "Image size must be a multiple of %u bytes",

+ (unsigned) BDRV_SECTOR_SIZE);

ret = -EINVAL;

goto out;

}

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,

return -ENOTSUP;

}

- if (offset & 511) {

- error_setg(errp, "The new size must be a multiple of 512");

+ if (!QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE)) {

+ error_setg(errp, "The new size must be a multiple of %u",

+ (unsigned) BDRV_SECTOR_SIZE);

return -EINVAL;

}

2.24.1

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

This test checks that bug is really fixed by previous commit.

Cc: qemu-stable@nongnu.org # v4.2.0

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

Message-id: 20200121142802.21467-3-vsementsov@virtuozzo.com

Signed-off-by: Max Reitz <mreitz@redhat.com>

tests/qemu-iotests/283 | 92 ++++++++++++++++++++++++++++++++++++++

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

tests/qemu-iotests/group | 1 +

3 files changed, 101 insertions(+)

create mode 100644 tests/qemu-iotests/283

create mode 100644 tests/qemu-iotests/283.out

diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283

new file mode 100644

index XXXXXXX..XXXXXXX

--- /dev/null

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

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

+#!/usr/bin/env python

+#

+# Test for backup-top filter permission activation failure

+#

+# Copyright (c) 2019 Virtuozzo International GmbH.

+#

+# This program is free software; you can redistribute it and/or modify

+# it under the terms of the GNU General Public License as published by

+# the Free Software Foundation; either version 2 of the License, or

+# (at your option) any later version.

+#

+# This program is distributed in the hope that it will be useful,

+# but WITHOUT ANY WARRANTY; without even the implied warranty of

+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the

+# GNU General Public License for more details.

+#

+# You should have received a copy of the GNU General Public License

+# along with this program. If not, see <http://www.gnu.org/licenses/>.

+#

+

+import iotests

+

+# The test is unrelated to formats, restrict it to qcow2 to avoid extra runs

+iotests.verify_image_format(supported_fmts=['qcow2'])

+

+size = 1024 * 1024

+

+""" Test description

+

+When performing a backup, all writes on the source subtree must go through the

+backup-top filter so it can copy all data to the target before it is changed.

+backup-top filter is appended above source node, to achieve this thing, so all

+parents of source node are handled. A configuration with side parents of source

+sub-tree with write permission is unsupported (we'd have append several

+backup-top filter like nodes to handle such parents). The test create an

+example of such configuration and checks that a backup is then not allowed

+(blockdev-backup command should fail).

+

+The configuration:

+

+ ┌────────┐ target ┌─────────────┐

+ │ target │ ◀─────── │ backup_top │

+ └────────┘ └─────────────┘

+ │

+ │ backing

+ ▼

+ ┌─────────────┐

+ │ source │

+ └─────────────┘

+ │

+ │ file

+ ▼

+ ┌─────────────┐ write perm ┌───────┐

+ │ base │ ◀──────────── │ other │

+ └─────────────┘ └───────┘

+

+On activation (see .active field of backup-top state in block/backup-top.c),

+backup-top is going to unshare write permission on its source child. Write

+unsharing will be propagated to the "source->base" link and will conflict with

+other node write permission. So permission update will fail and backup job will

+not be started.

+

+Note, that the only thing which prevents backup of running on such

+configuration is default permission propagation scheme. It may be altered by

+different block drivers, so backup will run in invalid configuration. But

+something is better than nothing. Also, before the previous commit (commit

+preceding this test creation), starting backup on such configuration led to

+crash, so current "something" is a lot better, and this test actual goal is

+to check that crash is fixed :)

+"""

+

+vm = iotests.VM()

+vm.launch()

+

+vm.qmp_log('blockdev-add', **{'node-name': 'target', 'driver': 'null-co'})

+

+vm.qmp_log('blockdev-add', **{

+ 'node-name': 'source',

+ 'driver': 'blkdebug',

+ 'image': {'node-name': 'base', 'driver': 'null-co', 'size': size}

+})

+

+vm.qmp_log('blockdev-add', **{

+ 'node-name': 'other',

+ 'driver': 'blkdebug',

+ 'image': 'base',

+ 'take-child-perms': ['write']

+})

+

+vm.qmp_log('blockdev-backup', sync='full', device='source', target='target')

+

+vm.shutdown()

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

new file mode 100644

index XXXXXXX..XXXXXXX

--- /dev/null

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

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

+{"execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "target"}}

+{"return": {}}

+{"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": {"driver": "null-co", "node-name": "base", "size": 1048576}, "node-name": "source"}}

+{"return": {}}

+{"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 set permissions for backup-top filter: Conflicts with use by other as 'image', which uses 'write' on base"}}

diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group

--- a/tests/qemu-iotests/group

+++ b/tests/qemu-iotests/group

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

279 rw backing quick

280 rw migration quick

281 rw quick

+283 auto quick

2.24.1

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

We can't access top after call bdrv_backup_top_drop, as it is already

freed at this time.

Also, no needs to unref target child by hand, it will be unrefed on

bdrv_close() automatically.

From: Alberto Garcia <berto@igalia.com>

qemu-img's convert_co_copy_range() operates at the sector level and

block_copy() operates at the cluster level so this condition is always

true, but it is not necessary to restrict this here, so let's leave it

to the driver implementation return an error if there is any.

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

Message-id: a4264aaee656910c84161a2965f7a501437379ca.1579374329.git.berto@igalia.com

Reviewed-by: Max Reitz <mreitz@redhat.com>

Signed-off-by: Max Reitz <mreitz@redhat.com>

block/qcow2.c | 4 ----

1 file changed, 4 deletions(-)

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

--- a/block/qcow2.c

+++ b/block/qcow2.c

@@ -XXX,XX +XXX,XX @@ qcow2_co_copy_range_from(BlockDriverState *bs,

case QCOW2_CLUSTER_NORMAL:

child = s->data_file;

copy_offset += offset_into_cluster(s, src_offset);

- if ((copy_offset & 511) != 0) {

- ret = -EIO;

- goto out;

- }

break;

default:

2.24.1

From: Alberto Garcia <berto@igalia.com>

When updating an L1 entry the qcow2 driver writes a (512-byte) sector

worth of data to avoid a read-modify-write cycle. Instead of always

writing 512 bytes we should follow the alignment requirements of the

storage backend.

(the only exception is when the alignment is larger than the cluster

size because then we could be overwriting data after the L1 table)

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

Message-id: 71f34d4ae4b367b32fb36134acbf4f4f7ee681f4.1579374329.git.berto@igalia.com

Reviewed-by: Max Reitz <mreitz@redhat.com>

Signed-off-by: Max Reitz <mreitz@redhat.com>

---

block/qcow2-cluster.c | 25 +++++++++++++++----------

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

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

--- a/block/qcow2-cluster.c

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

@@ -XXX,XX +XXX,XX @@ static int l2_load(BlockDriverState *bs, uint64_t offset,

/*

- * Writes one sector of the L1 table to the disk (can't update single entries

- * and we really don't want bdrv_pread to perform a read-modify-write)

+ * Writes an L1 entry to disk (note that depending on the alignment

+ * requirements this function may write more that just one entry in

+ * order to prevent bdrv_pwrite from performing a read-modify-write)

*/

-#define L1_ENTRIES_PER_SECTOR (512 / 8)

int qcow2_write_l1_entry(BlockDriverState *bs, int l1_index)

{

BDRVQcow2State *s = bs->opaque;

- uint64_t buf[L1_ENTRIES_PER_SECTOR] = { 0 };

int l1_start_index;

int i, ret;

+ int bufsize = MAX(sizeof(uint64_t),

+ MIN(bs->file->bs->bl.request_alignment, s->cluster_size));

+ int nentries = bufsize / sizeof(uint64_t);

+ g_autofree uint64_t *buf = g_try_new0(uint64_t, nentries);

- l1_start_index = l1_index & ~(L1_ENTRIES_PER_SECTOR - 1);

- for (i = 0; i < L1_ENTRIES_PER_SECTOR && l1_start_index + i < s->l1_size;

- i++)

- {

+ if (buf == NULL) {

+ return -ENOMEM;

+ l1_start_index = QEMU_ALIGN_DOWN(l1_index, nentries);

+ for (i = 0; i < MIN(nentries, s->l1_size - l1_start_index); i++) {

buf[i] = cpu_to_be64(s->l1_table[l1_start_index + i]);

}

ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_ACTIVE_L1,

- s->l1_table_offset + 8 * l1_start_index, sizeof(buf), false);

+ s->l1_table_offset + 8 * l1_start_index, bufsize, false);

if (ret < 0) {

return ret;

}

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

BLKDBG_EVENT(bs->file, BLKDBG_L1_UPDATE);

ret = bdrv_pwrite_sync(bs->file,

s->l1_table_offset + 8 * l1_start_index,

- buf, sizeof(buf));

+ buf, bufsize);

if (ret < 0) {

return ret;

}

2.24.1

From: Alberto Garcia <berto@igalia.com>

qcow2_alloc_cluster_offset() and qcow2_get_cluster_offset() always

return offsets that are cluster-aligned so don't just check that they

are sector-aligned.

The check in qcow2_co_preadv_task() is also replaced by an assertion

for the same reason.

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

Reviewed-by: Max Reitz <mreitz@redhat.com>

Message-id: 558ba339965f858bede4c73ce3f50f0c0493597d.1579374329.git.berto@igalia.com

Signed-off-by: Max Reitz <mreitz@redhat.com>

---

block/qcow2.c | 9 +++------

1 file changed, 3 insertions(+), 6 deletions(-)

From: Alberto Garcia <berto@igalia.com>

The L1 table is read from disk using the byte-based bdrv_pread() and

is never accessed beyond its last element, so there's no need to

allocate more memory than that.

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

Reviewed-by: Max Reitz <mreitz@redhat.com>

Message-id: b2e27214ec7b03a585931bcf383ee1ac3a641a10.1579374329.git.berto@igalia.com

Signed-off-by: Max Reitz <mreitz@redhat.com>

block/qcow2-cluster.c | 5 ++---

block/qcow2-refcount.c | 2 +-

block/qcow2-snapshot.c | 3 +--

block/qcow2.c | 2 +-

4 files changed, 5 insertions(+), 7 deletions(-)

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

--- a/block/qcow2-cluster.c

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

@@ -XXX,XX +XXX,XX @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,

#endif

new_l1_size2 = sizeof(uint64_t) * new_l1_size;

- new_l1_table = qemu_try_blockalign(bs->file->bs,

- ROUND_UP(new_l1_size2, 512));

+ new_l1_table = qemu_try_blockalign(bs->file->bs, new_l1_size2);

if (new_l1_table == NULL) {

return -ENOMEM;

}

- memset(new_l1_table, 0, ROUND_UP(new_l1_size2, 512));

+ memset(new_l1_table, 0, new_l1_size2);

if (s->l1_size) {

memcpy(new_l1_table, s->l1_table, s->l1_size * sizeof(uint64_t));

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

--- a/block/qcow2-refcount.c

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

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

* l1_table_offset when it is the current s->l1_table_offset! Be careful

* when changing this! */

if (l1_table_offset != s->l1_table_offset) {

- l1_table = g_try_malloc0(ROUND_UP(l1_size2, 512));

+ l1_table = g_try_malloc0(l1_size2);

if (l1_size2 && l1_table == NULL) {

ret = -ENOMEM;

goto fail;

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,

return ret;

}

new_l1_bytes = sn->l1_size * sizeof(uint64_t);

- new_l1_table = qemu_try_blockalign(bs->file->bs,

- ROUND_UP(new_l1_bytes, 512));

+ new_l1_table = qemu_try_blockalign(bs->file->bs, new_l1_bytes);

if (new_l1_table == NULL) {

return -ENOMEM;

}

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

index XXXXXXX..XXXXXXX 100644

--- a/block/qcow2.c

+++ b/block/qcow2.c

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,

if (s->l1_size > 0) {

s->l1_table = qemu_try_blockalign(bs->file->bs,

- ROUND_UP(s->l1_size * sizeof(uint64_t), 512));

+ s->l1_size * sizeof(uint64_t));

if (s->l1_table == NULL) {

error_setg(errp, "Could not allocate L1 table");

ret = -ENOMEM;

2.24.1

From: Thomas Huth <thuth@redhat.com>

We are going to enable 127 in the "auto" group, but it only works if

virtio-scsi and scsi-hd are available - which is not the case with

QEMU binaries like qemu-system-tricore for example, so we need a

proper check for the availability of these devices here.

A very similar problem exists in iotest 267 - it has been added to

the "auto" group already, but requires virtio-blk and thus currently

fails with qemu-system-tricore for example. Let's also add aproper

check there.

Reviewed-by: Max Reitz <mreitz@redhat.com>

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

Message-id: 20200121095205.26323-5-thuth@redhat.com

Signed-off-by: Max Reitz <mreitz@redhat.com>

---

tests/qemu-iotests/127 | 2 ++

tests/qemu-iotests/267 | 2 ++

tests/qemu-iotests/common.rc | 14 ++++++++++++++

3 files changed, 18 insertions(+)

diff --git a/tests/qemu-iotests/127 b/tests/qemu-iotests/127

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/127

+++ b/tests/qemu-iotests/127

@@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15

_supported_fmt qcow2

_supported_proto file

+_require_devices virtio-scsi scsi-hd

IMG_SIZE=64K

_make_test_img $IMG_SIZE

diff --git a/tests/qemu-iotests/267 b/tests/qemu-iotests/267

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/267

+++ b/tests/qemu-iotests/267

@@ -XXX,XX +XXX,XX @@ _require_drivers copy-on-read

# and generally impossible with external data files

_unsupported_imgopts 'refcount_bits=1[^0-9]' data_file

+_require_devices virtio-blk

+

do_run_qemu()

{

echo Testing: "$@"

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc

--- a/tests/qemu-iotests/common.rc

+++ b/tests/qemu-iotests/common.rc

@@ -XXX,XX +XXX,XX @@ _require_large_file()

rm "$TEST_IMG"

+# Check that a set of devices is available in the QEMU binary

+#

+_require_devices()

+ available=$($QEMU -M none -device help | \

+ grep ^name | sed -e 's/^name "//' -e 's/".*$//')

+ for device

+ do

+ if ! echo "$available" | grep -q "$device" ; then

+ _notrun "$device not available"

+ fi

+ done

# make sure this script returns success

true