The following changes since commit 7bc8f9734213b76e76631a483be13d6737c2adbc:

Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20191025' into staging (2019-10-25 13:12:16 +0100)

for you to fetch changes up to 5e9785505210e2477e590e61b1ab100d0ec22b01:

qcow2: Fix corruption bug in qcow2_detect_metadata_preallocation() (2019-10-25 15:18:55 +0200)

Block layer patches:

- qcow2: Fix data corruption bug that is triggered in partial cluster

allocation with default options

- qapi: add support for blkreplay driver

- doc: Describe missing generic -blockdev options

- iotests: Fix 118 when run as root

- Minor code cleanups

Kevin Wolf (5):

iotests: Skip read-only cases in 118 when run as root

blockdev: Use error_report() in hmp_commit()

doc: Describe missing generic -blockdev options

coroutine: Add qemu_co_mutex_assert_locked()

qcow2: Fix corruption bug in qcow2_detect_metadata_preallocation()

Pavel Dovgaluk (1):

qapi: add support for blkreplay driver

Vladimir Sementsov-Ogievskiy (1):

block/backup: drop dead code from backup_job_create

qapi/block-core.json | 18 ++++++++++++++++--

include/qemu/coroutine.h | 15 +++++++++++++++

block/backup.c | 5 +----

block/qcow2-refcount.c | 2 ++

block/qcow2.c | 3 ++-

blockdev.c | 7 +++----

qemu-options.hx | 22 +++++++++++++++++++++-

tests/qemu-iotests/118 | 3 +++

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

9 files changed, 73 insertions(+), 12 deletions(-)

After commit 00e30f05de1d195, there is no more "goto error" points

after job creation, so after "error:" @job is always NULL and we don't

need roll-back job creation.

Reported-by: Coverity (CID 1406402)

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

block/backup.c | 5 +----

1 file changed, 1 insertion(+), 4 deletions(-)

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

--- a/block/backup.c

+++ b/block/backup.c

@@ -XXX,XX +XXX,XX @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,

if (sync_bitmap) {

bdrv_reclaim_dirty_bitmap(sync_bitmap, NULL);

- if (job) {

- backup_clean(&job->common.job);

- job_early_fail(&job->common.job);

- } else if (backup_top) {

+ if (backup_top) {

bdrv_backup_top_drop(backup_top);

2.20.1

qcow2_detect_metadata_preallocation() calls qcow2_get_refcount() which

requires s->lock to be taken to protect its accesses to the refcount

table and refcount blocks. However, nothing in this code path actually

took the lock. This could cause the same cache entry to be used by two

requests at the same time, for different tables at different offsets,

resulting in image corruption.

As it would be preferable to base the detection on consistent data (even

though it's just heuristics), let's take the lock not only around the

qcow2_get_refcount() calls, but around the whole function.

This patch takes the lock in qcow2_co_block_status() earlier and asserts

in qcow2_detect_metadata_preallocation() that we hold the lock.

Fixes: 69f47505ee66afaa513305de0c1895a224e52c45

Cc: qemu-stable@nongnu.org

Reported-by: Michael Weiser <michael.weiser@gmx.de>

block/qcow2-refcount.c | 2 ++

block/qcow2.c | 3 ++-

2 files changed, 4 insertions(+), 1 deletion(-)

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_detect_metadata_preallocation(BlockDriverState *bs)

int64_t i, end_cluster, cluster_count = 0, threshold;

int64_t file_length, real_allocation, real_clusters;

+ qemu_co_mutex_assert_locked(&s->lock);

file_length = bdrv_getlength(bs->file->bs);

if (file_length < 0) {

return file_length;

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_co_block_status(BlockDriverState *bs,

unsigned int bytes;

int status = 0;

+ qemu_co_mutex_lock(&s->lock);

if (!s->metadata_preallocation_checked) {

ret = qcow2_detect_metadata_preallocation(bs);

s->metadata_preallocation = (ret == 1);

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

bytes = MIN(INT_MAX, count);

- qemu_co_mutex_lock(&s->lock);

ret = qcow2_get_cluster_offset(bs, offset, &bytes, &cluster_offset);

qemu_co_mutex_unlock(&s->lock);

2.20.1

Some tests in 118 use chmod to remove write permissions from the file

and assume that the image can indeed not be opened read-write

afterwards. This doesn't work when the test is run as root, because root

can still open the file as writable even when the permission bit isn't

set.

Introduce a @skip_if_root decorator and use it in 118 to skip the tests

in question when the script is run as root.

tests/qemu-iotests/118 | 3 +++

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

2 files changed, 13 insertions(+)

@@ -XXX,XX +XXX,XX @@ def skip_if_unsupported(required_formats=[], read_only=False):

return func_wrapper

return skip_test_decorator

+def skip_if_user_is_root(func):

+ '''Skip Test Decorator

+ Runs the test only without root permissions'''

+ def func_wrapper(*args, **kwargs):

+ if os.getuid() == 0:

+ case_notrun('{}: cannot be run as root'.format(args[0]))

+ else:

+ return func(*args, **kwargs)

+ return func_wrapper

def execute_unittest(output, verbosity, debug):

runner = unittest.TextTestRunner(stream=output, descriptions=True,

verbosity=verbosity)

2.20.1

Some functions require that the caller holds a certain CoMutex for them

to operate correctly. Add a function so that they can assert the lock is

really held.

Cc: qemu-stable@nongnu.org

include/qemu/coroutine.h | 15 +++++++++++++++

1 file changed, 15 insertions(+)

diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h

index XXXXXXX..XXXXXXX 100644

--- a/include/qemu/coroutine.h

+++ b/include/qemu/coroutine.h

@@ -XXX,XX +XXX,XX @@ void coroutine_fn qemu_co_mutex_lock(CoMutex *mutex);

*/

void coroutine_fn qemu_co_mutex_unlock(CoMutex *mutex);

+/**

+ * Assert that the current coroutine holds @mutex.

+ */

+static inline coroutine_fn void qemu_co_mutex_assert_locked(CoMutex *mutex)

+{

+ /*

+ * mutex->holder doesn't need any synchronisation if the assertion holds

+ * true because the mutex protects it. If it doesn't hold true, we still

+ * don't mind if another thread takes or releases mutex behind our back,

+ * because the condition will be false no matter whether we read NULL or

+ * the pointer for any other coroutine.

+ */

+ assert(atomic_read(&mutex->locked) &&

+ mutex->holder == qemu_coroutine_self());

+}

/**

* CoQueues are a mechanism to queue coroutines in order to continue executing

2.20.1

We added more generic options after introducing -blockdev and forgot to

update the documentation (man page and --help output) accordingly. Do

that now.

qemu-options.hx | 22 +++++++++++++++++++++-

1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/qemu-options.hx b/qemu-options.hx

index XXXXXXX..XXXXXXX 100644

--- a/qemu-options.hx

+++ b/qemu-options.hx

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

DEF("blockdev", HAS_ARG, QEMU_OPTION_blockdev,

"-blockdev [driver=]driver[,node-name=N][,discard=ignore|unmap]\n"

" [,cache.direct=on|off][,cache.no-flush=on|off]\n"

- " [,read-only=on|off][,detect-zeroes=on|off|unmap]\n"

+ " [,read-only=on|off][,auto-read-only=on|off]\n"

+ " [,force-share=on|off][,detect-zeroes=on|off|unmap]\n"

" [,driver specific parameters...]\n"

" configure a block backend\n", QEMU_ARCH_ALL)

STEXI

@@ -XXX,XX +XXX,XX @@ name is not intended to be predictable and changes between QEMU invocations.

For the top level, an explicit node name must be specified.

@item read-only

Open the node read-only. Guest write attempts will fail.

+

+Note that some block drivers support only read-only access, either generally or

+in certain configurations. In this case, the default value

+@option{read-only=off} does not work and the option must be specified

+explicitly.

+@item auto-read-only

+If @option{auto-read-only=on} is set, QEMU may fall back to read-only usage

+even when @option{read-only=off} is requested, or even switch between modes as

+needed, e.g. depending on whether the image file is writable or whether a

+writing user is attached to the node.

+@item force-share

+Override the image locking system of QEMU by forcing the node to utilize

+weaker shared access for permissions where it would normally request exclusive

+access. When there is the potential for multiple instances to have the same

+file open (whether this invocation of QEMU is the first or the second

+instance), both instances must permit shared access for the second instance to

+succeed at opening the file.

+

+Enabling @option{force-share=on} requires @option{read-only=on}.

@item cache.direct

The host page cache can be avoided with @option{cache.direct=on}. This will

attempt to do disk IO directly to the guest's memory. QEMU may still perform an

2.20.1

From: Pavel Dovgalyuk <pavel.dovgaluk@gmail.com>

This patch adds support for blkreplay driver to the blockdev options.

Now blkreplay can be used with -blockdev command line option

in the following format:

-blockdev driver=blkreplay,image=file-node-name,node-name=replay-node-name

This option makes possible implementation of the better command

line support for record/replay invocations.

Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>

qapi/block-core.json | 18 ++++++++++++++++--

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

diff --git a/qapi/block-core.json b/qapi/block-core.json

index XXXXXXX..XXXXXXX 100644

--- a/qapi/block-core.json

+++ b/qapi/block-core.json

# @nvme: Since 2.12

# @copy-on-read: Since 3.0

# @blklogwrites: Since 3.0

+# @blkreplay: Since 4.2

# Since: 2.9

##

{ 'enum': 'BlockdevDriver',

- 'data': [ 'blkdebug', 'blklogwrites', 'blkverify', 'bochs', 'cloop',

- 'copy-on-read', 'dmg', 'file', 'ftp', 'ftps', 'gluster',

+ 'data': [ 'blkdebug', 'blklogwrites', 'blkreplay', 'blkverify', 'bochs',

+ 'cloop', 'copy-on-read', 'dmg', 'file', 'ftp', 'ftps', 'gluster',

'host_cdrom', 'host_device', 'http', 'https', 'iscsi', 'luks',

'nbd', 'nfs', 'null-aio', 'null-co', 'nvme', 'parallels', 'qcow',

'qcow2', 'qed', 'quorum', 'raw', 'rbd',

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

'data': { 'test': 'BlockdevRef',

'raw': 'BlockdevRef' } }

+##

+# @BlockdevOptionsBlkreplay:

+#

+# Driver specific block device options for blkreplay.

+#

+# @image: disk image which should be controlled with blkreplay

+#

+# Since: 4.2

+##

+{ 'struct': 'BlockdevOptionsBlkreplay',

+ 'data': { 'image': 'BlockdevRef' } }

##

# @QuorumReadPattern:

#

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

'blkdebug': 'BlockdevOptionsBlkdebug',

'blklogwrites':'BlockdevOptionsBlklogwrites',

'blkverify': 'BlockdevOptionsBlkverify',

+ 'blkreplay': 'BlockdevOptionsBlkreplay',

'bochs': 'BlockdevOptionsGenericFormat',

'cloop': 'BlockdevOptionsGenericFormat',

'copy-on-read':'BlockdevOptionsGenericFormat',

2.20.1

Instead of using monitor_printf() to report errors, hmp_commit() should

use error_report() like other places do.

blockdev.c | 7 +++----

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

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

--- a/blockdev.c

+++ b/blockdev.c

@@ -XXX,XX +XXX,XX @@ void hmp_commit(Monitor *mon, const QDict *qdict)

blk = blk_by_name(device);

if (!blk) {

- monitor_printf(mon, "Device '%s' not found\n", device);

+ error_report("Device '%s' not found", device);

return;

}

if (!blk_is_available(blk)) {

- monitor_printf(mon, "Device '%s' has no medium\n", device);

+ error_report("Device '%s' has no medium", device);

return;

}

@@ -XXX,XX +XXX,XX @@ void hmp_commit(Monitor *mon, const QDict *qdict)

aio_context_release(aio_context);

}

if (ret < 0) {

- monitor_printf(mon, "'commit' error for '%s': %s\n", device,

- strerror(-ret));

+ error_report("'commit' error for '%s': %s", device, strerror(-ret));

}

}

2.20.1