Eric Blake (1):

qemu-img: Add -F shorthand to convert

From: Max Reitz <mreitz@redhat.com>

gluster's block-status implementation is basically a copy of that in

block/file-posix.c, there is only one thing missing, and that is

aligning trailing data extents to the request alignment (as added by

commit 9c3db310ff0).

Note that 9c3db310ff0 mentions that "there seems to be no other block

driver that sets request_alignment and [...]", but while block/gluster.c

does indeed not set request_alignment, block/io.c's

bdrv_refresh_limits() will still default to an alignment of 512 because

block/gluster.c does not provide a byte-aligned read function.

Therefore, unaligned tails can conceivably occur, and so we should apply

the change from 9c3db310ff0 to gluster's block-status implementation.

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

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

Message-Id: <20210805143603.59503-1-mreitz@redhat.com>

Reviewed-by: Eric Blake <eblake@redhat.com>

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

---

block/gluster.c | 16 ++++++++++++++++

1 file changed, 16 insertions(+)

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

index XXXXXXX..XXXXXXX 100644

--- a/block/gluster.c

+++ b/block/gluster.c

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn qemu_gluster_co_block_status(BlockDriverState *bs,

off_t data = 0, hole = 0;

int ret = -EINVAL;

+ assert(QEMU_IS_ALIGNED(offset | bytes, bs->bl.request_alignment));

+

if (!s->fd) {

return ret;

}

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn qemu_gluster_co_block_status(BlockDriverState *bs,

/* On a data extent, compute bytes to the end of the extent,

* possibly including a partial sector at EOF. */

*pnum = MIN(bytes, hole - offset);

+

+ /*

+ * We are not allowed to return partial sectors, though, so

+ * round up if necessary.

+ */

+ if (!QEMU_IS_ALIGNED(*pnum, bs->bl.request_alignment)) {

+ int64_t file_length = qemu_gluster_getlength(bs);

+ if (file_length > 0) {

+ /* Ignore errors, this is just a safeguard */

+ assert(hole == file_length);

+ }

+ *pnum = ROUND_UP(*pnum, bs->bl.request_alignment);

+ }

+

ret = BDRV_BLOCK_DATA;

} else {

/* On a hole, compute bytes to the beginning of the next extent. */

--

2.31.1

There is a comment above the BDS definition stating care must be taken

to consider handling newly added fields in bdrv_append().

Actually, this comment should have said "bdrv_swap()" as of 4ddc07cac

(nine years ago), and in any case, bdrv_swap() was dropped in

8e419aefa (six years ago). So no such care is necessary anymore.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Reviewed-by: Eric Blake <eblake@redhat.com>

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

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

Message-Id: <20210812084148.14458-2-hreitz@redhat.com>

---

include/block/block_int.h | 6 ------

1 file changed, 6 deletions(-)

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 BdrvChild {

QLIST_ENTRY(BdrvChild) next_parent;

};

-/*

- * Note: the function bdrv_append() copies and swaps contents of

- * BlockDriverStates, so if you add new fields to this struct, please

- * inspect bdrv_append() to determine if the new fields need to be

- * copied as well.

- */

struct BlockDriverState {

/* Protected by big QEMU lock or read-only after opening. No special

* locking needed during I/O...

--

2.31.1

@@ -XXX,XX +XXX,XX @@ static void bdrv_close(BlockDriverState *bs)

bs->explicit_options = NULL;

qobject_unref(bs->full_open_options);

bs->full_open_options = NULL;

+ g_free(bs->block_status_cache);

+ bs->block_status_cache = NULL;

bdrv_release_named_dirty_bitmaps(bs);

assert(QLIST_EMPTY(&bs->dirty_bitmaps));

@@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_backing_chain_next(BlockDriverState *bs)

{

return bdrv_skip_filters(bdrv_cow_bs(bdrv_skip_filters(bs)));

.bdrv_co_block_status() implementations are free to return a *pnum that

exceeds @bytes, because bdrv_co_block_status() in block/io.c will clamp

*pnum as necessary.

On the other hand, if drivers' implementations return values for *pnum

that are as large as possible, our recently introduced block-status

cache will become more effective.

So, make a note in block_int.h that @bytes is no upper limit for *pnum.

Suggested-by: Eric Blake <eblake@redhat.com>

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

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

Message-Id: <20210812084148.14458-4-hreitz@redhat.com>

Reviewed-by: Eric Blake <eblake@redhat.com>

---

include/block/block_int.h | 9 +++++++++

1 file changed, 9 insertions(+)

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 BlockDriver {

* clamped to bdrv_getlength() and aligned to request_alignment,

* as well as non-NULL pnum, map, and file; in turn, the driver

* must return an error or set pnum to an aligned non-zero value.

+ *

+ * Note that @bytes is just a hint on how big of a region the

+ * caller wants to inspect. It is not a limit on *pnum.

+ * Implementations are free to return larger values of *pnum if

+ * doing so does not incur a performance penalty.

+ *

+ * block/io.c's bdrv_co_block_status() will utilize an unclamped

+ * *pnum value for the block-status cache on protocol nodes, prior

+ * to clamping *pnum for return to its caller.

*/

int coroutine_fn (*bdrv_co_block_status)(BlockDriverState *bs,

bool want_zero, int64_t offset, int64_t bytes, int64_t *pnum,

--

2.31.1

bdrv_co_block_status() does it for us, we do not need to do it here.

The advantage of not capping *pnum is that bdrv_co_block_status() can

cache larger data regions than requested by its caller.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Reviewed-by: Eric Blake <eblake@redhat.com>

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

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

Message-Id: <20210812084148.14458-5-hreitz@redhat.com>

---

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

1 file changed, 4 insertions(+), 3 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 find_allocation(BlockDriverState *bs, off_t start,

* the specified offset) that are known to be in the same

* allocated/unallocated state.

*

- * 'bytes' is the max value 'pnum' should be set to.

+ * 'bytes' is a soft cap for 'pnum'. If the information is free, 'pnum' may

+ * well exceed it.

*/

static int coroutine_fn raw_co_block_status(BlockDriverState *bs,

bool want_zero,

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn raw_co_block_status(BlockDriverState *bs,

} else if (data == offset) {

/* On a data extent, compute bytes to the end of the extent,

* possibly including a partial sector at EOF. */

- *pnum = MIN(bytes, hole - offset);

+ *pnum = hole - offset;

/*

* We are not allowed to return partial sectors, though, so

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn raw_co_block_status(BlockDriverState *bs,

} else {

/* On a hole, compute bytes to the beginning of the next extent. */

assert(hole == offset);

- *pnum = MIN(bytes, data - offset);

+ *pnum = data - offset;

ret = BDRV_BLOCK_ZERO;

}

*map = offset;

--

2.31.1

bdrv_co_block_status() does it for us, we do not need to do it here.

The advantage of not capping *pnum is that bdrv_co_block_status() can

cache larger data regions than requested by its caller.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Reviewed-by: Eric Blake <eblake@redhat.com>

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

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

Message-Id: <20210812084148.14458-6-hreitz@redhat.com>

---

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

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

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

index XXXXXXX..XXXXXXX 100644

--- a/block/gluster.c

+++ b/block/gluster.c

@@ -XXX,XX +XXX,XX @@ exit:

* the specified offset) that are known to be in the same

* allocated/unallocated state.

*

- * 'bytes' is the max value 'pnum' should be set to.

+ * 'bytes' is a soft cap for 'pnum'. If the information is free, 'pnum' may

+ * well exceed it.

*

* (Based on raw_co_block_status() from file-posix.c.)

*/

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn qemu_gluster_co_block_status(BlockDriverState *bs,

} else if (data == offset) {

/* On a data extent, compute bytes to the end of the extent,

* possibly including a partial sector at EOF. */

- *pnum = MIN(bytes, hole - offset);

+ *pnum = hole - offset;

/*

* We are not allowed to return partial sectors, though, so

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn qemu_gluster_co_block_status(BlockDriverState *bs,

} else {

/* On a hole, compute bytes to the beginning of the next extent. */

assert(hole == offset);

- *pnum = MIN(bytes, data - offset);

+ *pnum = data - offset;

ret = BDRV_BLOCK_ZERO;

}

--

2.31.1

bdrv_co_block_status() does it for us, we do not need to do it here.

The advantage of not capping *pnum is that bdrv_co_block_status() can

cache larger data regions than requested by its caller.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Reviewed-by: Eric Blake <eblake@redhat.com>

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

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

Message-Id: <20210812084148.14458-7-hreitz@redhat.com>

---

block/iscsi.c | 3 ---

1 file changed, 3 deletions(-)

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

index XXXXXXX..XXXXXXX 100644

--- a/block/iscsi.c

+++ b/block/iscsi.c

@@ -XXX,XX +XXX,XX @@ retry:

iscsi_allocmap_set_allocated(iscsilun, offset, *pnum);

}

- if (*pnum > bytes) {

- *pnum = bytes;

- }

out_unlock:

qemu_mutex_unlock(&iscsilun->mutex);

g_free(iTask.err_str);

--

2.31.1

As of recently, pylint complains when `open()` calls are missing an

`encoding=` specified. Everything we have should be UTF-8 (and in fact,

everything should be UTF-8, period (exceptions apply)), so use that.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Message-Id: <20210824153540.177128-2-hreitz@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: John Snow <jsnow@redhat.com>

---

tests/qemu-iotests/297 | 2 +-

tests/qemu-iotests/iotests.py | 8 +++++---

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

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

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/297

+++ b/tests/qemu-iotests/297

@@ -XXX,XX +XXX,XX @@ def is_python_file(filename):

if filename.endswith('.py'):

return True

- with open(filename) as f:

+ with open(filename, encoding='utf-8') as f:

try:

first_line = f.readline()

return re.match('^#!.*python', first_line) is not None

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py

index XXXXXXX..XXXXXXX 100644

--- a/tests/qemu-iotests/iotests.py

+++ b/tests/qemu-iotests/iotests.py

@@ -XXX,XX +XXX,XX @@ def _post_shutdown(self) -> None:

return

valgrind_filename = f"{test_dir}/{self._popen.pid}.valgrind"

if self.exitcode() == 99:

- with open(valgrind_filename) as f:

+ with open(valgrind_filename, encoding='utf-8') as f:

print(f.read())

else:

os.remove(valgrind_filename)

@@ -XXX,XX +XXX,XX @@ def notrun(reason):

# Each test in qemu-iotests has a number ("seq")

seq = os.path.basename(sys.argv[0])

- with open('%s/%s.notrun' % (output_dir, seq), 'w') as outfile:

+ with open('%s/%s.notrun' % (output_dir, seq), 'w', encoding='utf-8') \

+ as outfile:

outfile.write(reason + '\n')

logger.warning("%s not run: %s", seq, reason)

sys.exit(0)

@@ -XXX,XX +XXX,XX @@ def case_notrun(reason):

# Each test in qemu-iotests has a number ("seq")

seq = os.path.basename(sys.argv[0])

- with open('%s/%s.casenotrun' % (output_dir, seq), 'a') as outfile:

+ with open('%s/%s.casenotrun' % (output_dir, seq), 'a', encoding='utf-8') \

+ as outfile:

outfile.write(' [case not run] ' + reason + '\n')

def _verify_image_format(supported_fmts: Sequence[str] = (),

--

2.31.1

pylint proposes using `[]` instead of `list()` and `{}` instead of

`dict()`, because it is faster. That seems simple enough, so heed its

advice.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Message-Id: <20210824153540.177128-3-hreitz@redhat.com>

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

---

tests/qemu-iotests/iotests.py | 4 ++--

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

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py

index XXXXXXX..XXXXXXX 100644

--- a/tests/qemu-iotests/iotests.py

+++ b/tests/qemu-iotests/iotests.py

@@ -XXX,XX +XXX,XX @@ def hmp_qemu_io(self, drive: str, cmd: str,

def flatten_qmp_object(self, obj, output=None, basestr=''):

if output is None:

- output = dict()

+ output = {}

if isinstance(obj, list):

for i, item in enumerate(obj):

self.flatten_qmp_object(item, output, basestr + str(i) + '.')

@@ -XXX,XX +XXX,XX @@ def flatten_qmp_object(self, obj, output=None, basestr=''):

def qmp_to_opts(self, obj):

obj = self.flatten_qmp_object(obj)

- output_list = list()

+ output_list = []

for key in obj:

output_list += [key + '=' + obj[key]]

return ','.join(output_list)

--

2.31.1

169 and 199 have been renamed and moved to tests/ (commit a44be0334be:

"iotests: rename and move 169 and 199 tests"), so we can drop them from

the skip list.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Reviewed-by: Willian Rampazzo <willianr@redhat.com>

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

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

Message-Id: <20210902094017.32902-2-hreitz@redhat.com>

---

tests/qemu-iotests/297 | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

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

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/297

+++ b/tests/qemu-iotests/297

@@ -XXX,XX +XXX,XX @@ import iotests

SKIP_FILES = (

'030', '040', '041', '044', '045', '055', '056', '057', '065', '093',

'096', '118', '124', '132', '136', '139', '147', '148', '149',

- '151', '152', '155', '163', '165', '169', '194', '196', '199', '202',

+ '151', '152', '155', '163', '165', '194', '196', '202',

'203', '205', '206', '207', '208', '210', '211', '212', '213', '216',

'218', '219', '224', '228', '234', '235', '236', '237', '238',

'240', '242', '245', '246', '248', '255', '256', '257', '258', '260',

--

2.31.1

pylint complains that discards1_sha256 and all_discards_sha256 are first

set in non-__init__ methods.

These variables are not really class-variables anyway, so let them

instead be returned by start_postcopy(), thus silencing pylint.

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

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

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

Message-Id: <20210902094017.32902-3-hreitz@redhat.com>

---

.../tests/migrate-bitmaps-postcopy-test | 13 +++++++------

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

diff --git a/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test b/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test

+++ b/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test

@@ -XXX,XX +XXX,XX @@ class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase):

result = self.vm_a.qmp('x-debug-block-dirty-bitmap-sha256',

node='drive0', name='bitmap0')

- self.discards1_sha256 = result['return']['sha256']

+ discards1_sha256 = result['return']['sha256']

# Check, that updating the bitmap by discards works

- assert self.discards1_sha256 != empty_sha256

+ assert discards1_sha256 != empty_sha256

# We want to calculate resulting sha256. Do it in bitmap0, so, disable

# other bitmaps

@@ -XXX,XX +XXX,XX @@ class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase):

result = self.vm_a.qmp('x-debug-block-dirty-bitmap-sha256',

node='drive0', name='bitmap0')

- self.all_discards_sha256 = result['return']['sha256']

+ all_discards_sha256 = result['return']['sha256']

# Now, enable some bitmaps, to be updated during migration

for i in range(2, nb_bitmaps, 2):

@@ -XXX,XX +XXX,XX @@ class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase):

event_resume = self.vm_b.event_wait('RESUME')

self.vm_b_events.append(event_resume)

- return event_resume

+ return (event_resume, discards1_sha256, all_discards_sha256)

def test_postcopy_success(self):

- event_resume = self.start_postcopy()

+ event_resume, discards1_sha256, all_discards_sha256 = \

+ self.start_postcopy()

# enabled bitmaps should be updated

apply_discards(self.vm_b, discards2)

@@ -XXX,XX +XXX,XX @@ class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase):

for i in range(0, nb_bitmaps, 5):

result = self.vm_b.qmp('x-debug-block-dirty-bitmap-sha256',

node='drive0', name='bitmap{}'.format(i))

- sha = self.discards1_sha256 if i % 2 else self.all_discards_sha256

+ sha = discards1_sha256 if i % 2 else all_discards_sha256

self.assert_qmp(result, 'return/sha256', sha)

def test_early_shutdown_destination(self):

--

2.31.1

There are a couple of things pylint takes issue with:

- The "time" import is unused

- The import order (iotests should come last)

- get_bitmap_hash() doesn't use @self and so should be a function

- Semicolons at the end of some lines

- Parentheses after "if"

- Some lines are too long (80 characters instead of 79)

- inject_test_case()'s @name parameter shadows a top-level @name

variable

- "lambda self: mc(self)" were equivalent to just "mc", but in

inject_test_case(), it is not equivalent, so add a comment and disable

the warning locally

- Always put two empty lines after a function

- f'exec: cat > /dev/null' does not need to be an f-string

Fix them.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Message-Id: <20210902094017.32902-4-hreitz@redhat.com>

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

---

tests/qemu-iotests/tests/migrate-bitmaps-test | 43 +++++++++++--------

1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/tests/qemu-iotests/tests/migrate-bitmaps-test b/tests/qemu-iotests/tests/migrate-bitmaps-test

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/tests/migrate-bitmaps-test

+++ b/tests/qemu-iotests/tests/migrate-bitmaps-test

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

#

import os

-import iotests

-import time

import itertools

import operator

import re

+import iotests

from iotests import qemu_img, qemu_img_create, Timeout

@@ -XXX,XX +XXX,XX @@ mig_cmd = 'exec: cat > ' + mig_file

incoming_cmd = 'exec: cat ' + mig_file

+def get_bitmap_hash(vm):

+ result = vm.qmp('x-debug-block-dirty-bitmap-sha256',

+ node='drive0', name='bitmap0')

+ return result['return']['sha256']

+

+

class TestDirtyBitmapMigration(iotests.QMPTestCase):

def tearDown(self):

self.vm_a.shutdown()

@@ -XXX,XX +XXX,XX @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):

params['persistent'] = True

result = vm.qmp('block-dirty-bitmap-add', **params)

- self.assert_qmp(result, 'return', {});

-

- def get_bitmap_hash(self, vm):

- result = vm.qmp('x-debug-block-dirty-bitmap-sha256',

- node='drive0', name='bitmap0')

- return result['return']['sha256']

+ self.assert_qmp(result, 'return', {})

def check_bitmap(self, vm, sha256):

result = vm.qmp('x-debug-block-dirty-bitmap-sha256',

node='drive0', name='bitmap0')

if sha256:

- self.assert_qmp(result, 'return/sha256', sha256);

+ self.assert_qmp(result, 'return/sha256', sha256)

else:

self.assert_qmp(result, 'error/desc',

- "Dirty bitmap 'bitmap0' not found");

+ "Dirty bitmap 'bitmap0' not found")

def do_test_migration_resume_source(self, persistent, migrate_bitmaps):

granularity = 512

@@ -XXX,XX +XXX,XX @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):

self.add_bitmap(self.vm_a, granularity, persistent)

for r in regions:

self.vm_a.hmp_qemu_io('drive0', 'write %d %d' % r)

- sha256 = self.get_bitmap_hash(self.vm_a)

+ sha256 = get_bitmap_hash(self.vm_a)

result = self.vm_a.qmp('migrate', uri=mig_cmd)

while True:

@@ -XXX,XX +XXX,XX @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):

break

while True:

result = self.vm_a.qmp('query-status')

- if (result['return']['status'] == 'postmigrate'):

+ if result['return']['status'] == 'postmigrate':

break

# test that bitmap is still here

@@ -XXX,XX +XXX,XX @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):

self.add_bitmap(self.vm_a, granularity, persistent)

for r in regions:

self.vm_a.hmp_qemu_io('drive0', 'write %d %d' % r)

- sha256 = self.get_bitmap_hash(self.vm_a)

+ sha256 = get_bitmap_hash(self.vm_a)

if pre_shutdown:

self.vm_a.shutdown()

@@ -XXX,XX +XXX,XX @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):

self.check_bitmap(self.vm_b, sha256 if persistent else False)

-def inject_test_case(klass, name, method, *args, **kwargs):

+def inject_test_case(klass, suffix, method, *args, **kwargs):

mc = operator.methodcaller(method, *args, **kwargs)

- setattr(klass, 'test_' + method + name, lambda self: mc(self))

+ # We want to add a function attribute to `klass`, so that it is

+ # correctly converted to a method on instantiation. The

+ # methodcaller object `mc` is a callable, not a function, so we

+ # need the lambda to turn it into a function.

+ # pylint: disable=unnecessary-lambda

+ setattr(klass, 'test_' + method + suffix, lambda self: mc(self))

+

for cmb in list(itertools.product((True, False), repeat=5)):

name = ('_' if cmb[0] else '_not_') + 'persistent_'

name += ('_' if cmb[1] else '_not_') + 'migbitmap_'

name += '_online' if cmb[2] else '_offline'

name += '_shared' if cmb[3] else '_nonshared'

- if (cmb[4]):

+ if cmb[4]:

name += '__pre_shutdown'

inject_test_case(TestDirtyBitmapMigration, name, 'do_test_migration',

@@ -XXX,XX +XXX,XX @@ class TestDirtyBitmapBackingMigration(iotests.QMPTestCase):

self.assert_qmp(result, 'return', {})

# Check that the bitmaps are there

- for node in self.vm.qmp('query-named-block-nodes', flat=True)['return']:

+ nodes = self.vm.qmp('query-named-block-nodes', flat=True)['return']

+ for node in nodes:

if 'node0' in node['node-name']:

self.assert_qmp(node, 'dirty-bitmaps[0]/name', 'bmap0')

@@ -XXX,XX +XXX,XX @@ class TestDirtyBitmapBackingMigration(iotests.QMPTestCase):

"""

Continue the source after migration.

"""

- result = self.vm.qmp('migrate', uri=f'exec: cat > /dev/null')

+ result = self.vm.qmp('migrate', uri='exec: cat > /dev/null')

self.assert_qmp(result, 'return', {})

with Timeout(10, 'Migration timeout'):

--

2.31.1

The AbnormalShutdown exception class is not in qemu.machine, but in

qemu.machine.machine. (qemu.machine.AbnormalShutdown was enough for

Python to find it in order to run this test, but pylint complains about

it.)

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Message-Id: <20210902094017.32902-5-hreitz@redhat.com>

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

---

tests/qemu-iotests/tests/mirror-top-perms | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/tests/mirror-top-perms b/tests/qemu-iotests/tests/mirror-top-perms

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/tests/mirror-top-perms

+++ b/tests/qemu-iotests/tests/mirror-top-perms

@@ -XXX,XX +XXX,XX @@ class TestMirrorTopPerms(iotests.QMPTestCase):

def tearDown(self):

try:

self.vm.shutdown()

- except qemu.machine.AbnormalShutdown:

+ except qemu.machine.machine.AbnormalShutdown:

pass

if self.vm_b is not None:

--

2.31.1

297 so far does not check the named tests, which reside in the tests/

directory (i.e. full path tests/qemu-iotests/tests). Fix it.

Thanks to the previous two commits, all named tests pass its scrutiny,

so we do not have to add anything to SKIP_FILES.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Reviewed-by: Willian Rampazzo <willianr@redhat.com>

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

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

Message-Id: <20210902094017.32902-6-hreitz@redhat.com>

---

tests/qemu-iotests/297 | 5 +++--

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

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

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/297

+++ b/tests/qemu-iotests/297

@@ -XXX,XX +XXX,XX @@ def is_python_file(filename):

def run_linters():

- files = [filename for filename in (set(os.listdir('.')) - set(SKIP_FILES))

- if is_python_file(filename)]

+ named_tests = [f'tests/{entry}' for entry in os.listdir('tests')]

+ check_tests = set(os.listdir('.') + named_tests) - set(SKIP_FILES)

+ files = [filename for filename in check_tests if is_python_file(filename)]

iotests.logger.debug('Files to be checked:')

iotests.logger.debug(', '.join(sorted(files)))

--

2.31.1

From: Stefano Garzarella <sgarzare@redhat.com>

In mirror_iteration() we call mirror_wait_on_conflicts() with

`self` parameter set to NULL.

Starting from commit d44dae1a7c we dereference `self` pointer in

mirror_wait_on_conflicts() without checks if it is not NULL.

Backtrace:

Program terminated with signal SIGSEGV, Segmentation fault.

#0 mirror_wait_on_conflicts (self=0x0, s=<optimized out>, offset=<optimized out>, bytes=<optimized out>)

at ../block/mirror.c:172

172     self->waiting_for_op = op;

[Current thread is 1 (Thread 0x7f0908931ec0 (LWP 380249))]

(gdb) bt

#0 mirror_wait_on_conflicts (self=0x0, s=<optimized out>, offset=<optimized out>, bytes=<optimized out>)

at ../block/mirror.c:172

#1 0x00005610c5d9d631 in mirror_run (job=0x5610c76a2c00, errp=<optimized out>) at ../block/mirror.c:491

#2 0x00005610c5d58726 in job_co_entry (opaque=0x5610c76a2c00) at ../job.c:917

#3 0x00005610c5f046c6 in coroutine_trampoline (i0=<optimized out>, i1=<optimized out>)

at ../util/coroutine-ucontext.c:173

#4 0x00007f0909975820 in ?? () at ../sysdeps/unix/sysv/linux/x86_64/__start_context.S:91

from /usr/lib64/libc.so.6

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2001404

Fixes: d44dae1a7c ("block/mirror: fix active mirror dead-lock in mirror_wait_on_conflicts")

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

Message-Id: <20210910124533.288318-1-sgarzare@redhat.com>

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

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

---

block/mirror.c | 25 ++++++++++++++++---------

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

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

index XXXXXXX..XXXXXXX 100644

--- a/block/mirror.c

+++ b/block/mirror.c

@@ -XXX,XX +XXX,XX @@ static void coroutine_fn mirror_wait_on_conflicts(MirrorOp *self,

if (ranges_overlap(self_start_chunk, self_nb_chunks,

op_start_chunk, op_nb_chunks))

{

- /*

- * If the operation is already (indirectly) waiting for us, or

- * will wait for us as soon as it wakes up, then just go on

- * (instead of producing a deadlock in the former case).

- */

- if (op->waiting_for_op) {

- continue;

+ if (self) {

+ /*

+ * If the operation is already (indirectly) waiting for us,

+ * or will wait for us as soon as it wakes up, then just go

+ * on (instead of producing a deadlock in the former case).

+ */

+ if (op->waiting_for_op) {

+ continue;

+ }

+

+ self->waiting_for_op = op;

}

- self->waiting_for_op = op;

qemu_co_queue_wait(&op->waiting_requests, NULL);

- self->waiting_for_op = NULL;

+

+ if (self) {

+ self->waiting_for_op = NULL;

+ }

+

break;