From: Alberto Garcia <berto@igalia.com>

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

Message-Id: <20210222115737.2993-1-berto@igalia.com>

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

---

tests/qemu-iotests/087 | 8 ++------

tests/qemu-iotests/184 | 18 ++++++------------

tests/qemu-iotests/218 | 2 +-

tests/qemu-iotests/235 | 2 +-

tests/qemu-iotests/245 | 4 ++--

tests/qemu-iotests/258 | 6 +++---

tests/qemu-iotests/258.out | 4 ++--

tests/qemu-iotests/295 | 2 +-

tests/qemu-iotests/296 | 2 +-

9 files changed, 19 insertions(+), 29 deletions(-)

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

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/087

+++ b/tests/qemu-iotests/087

@@ -XXX,XX +XXX,XX @@ run_qemu <<EOF

"arguments": {

"qom-type": "secret",

"id": "sec0",

- "props": {

- "data": "123456"

- }

+ "data": "123456"

}

}

{ "execute": "blockdev-add",

@@ -XXX,XX +XXX,XX @@ run_qemu <<EOF

"arguments": {

"qom-type": "secret",

"id": "sec0",

- "props": {

- "data": "123456"

- }

+ "data": "123456"

}

}

{ "execute": "blockdev-add",

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

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/184

+++ b/tests/qemu-iotests/184

@@ -XXX,XX +XXX,XX @@ run_qemu <<EOF

"arguments": {

"qom-type": "throttle-group",

"id": "group0",

- "props": {

- "limits" : {

- "iops-total": 1000

- }

+ "limits" : {

+ "iops-total": 1000

}

}

}

@@ -XXX,XX +XXX,XX @@ run_qemu <<EOF

"arguments": {

"qom-type": "throttle-group",

"id": "group0",

- "props" : {

- "limits": {

- "iops-total": 1000

- }

+ "limits": {

+ "iops-total": 1000

}

}

}

@@ -XXX,XX +XXX,XX @@ run_qemu <<EOF

"arguments": {

"qom-type": "throttle-group",

"id": "group0",

- "props" : {

- "limits": {

- "iops-total": 1000

- }

+ "limits": {

+ "iops-total": 1000

}

}

}

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

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/218

+++ b/tests/qemu-iotests/218

@@ -XXX,XX +XXX,XX @@ with iotests.VM() as vm, \

vm.launch()

ret = vm.qmp('object-add', qom_type='throttle-group', id='tg',

- props={'x-bps-read': 4096})

+ limits={'bps-read': 4096})

assert ret['return'] == {}

ret = vm.qmp('blockdev-add',

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

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/235

+++ b/tests/qemu-iotests/235

@@ -XXX,XX +XXX,XX @@ vm.add_args('-drive', 'id=src,file=' + disk)

vm.launch()

log(vm.qmp('object-add', qom_type='throttle-group', id='tg0',

- props={ 'x-bps-total': size }))

+ limits={'bps-total': size}))

log(vm.qmp('blockdev-add',

**{ 'node-name': 'target',

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

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/245

+++ b/tests/qemu-iotests/245

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

###### throttle ######

######################

opts = { 'qom-type': 'throttle-group', 'id': 'group0',

- 'props': { 'limits': { 'iops-total': 1000 } } }

+ 'limits': { 'iops-total': 1000 } }

result = self.vm.qmp('object-add', conv_keys = False, **opts)

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

opts = { 'qom-type': 'throttle-group', 'id': 'group1',

- 'props': { 'limits': { 'iops-total': 2000 } } }

+ 'limits': { 'iops-total': 2000 } }

result = self.vm.qmp('object-add', conv_keys = False, **opts)

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

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

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/258

+++ b/tests/qemu-iotests/258

@@ -XXX,XX +XXX,XX @@ def test_concurrent_finish(write_to_stream_node):

vm.qmp_log('object-add',

qom_type='throttle-group',

id='tg',

- props={

- 'x-iops-write': 1,

- 'x-iops-write-max': 1

+ limits={

+ 'iops-write': 1,

+ 'iops-write-max': 1

})

vm.qmp_log('blockdev-add',

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

index XXXXXXX..XXXXXXX 100644

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

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

@@ -XXX,XX +XXX,XX @@ Running tests:

=== Commit and stream finish concurrently (letting stream write) ===

-{"execute": "object-add", "arguments": {"id": "tg", "props": {"x-iops-write": 1, "x-iops-write-max": 1}, "qom-type": "throttle-group"}}

+{"execute": "object-add", "arguments": {"id": "tg", "limits": {"iops-write": 1, "iops-write-max": 1}, "qom-type": "throttle-group"}}

{"return": {}}

{"execute": "blockdev-add", "arguments": {"backing": {"backing": {"backing": {"backing": {"driver": "raw", "file": {"driver": "file", "filename": "TEST_DIR/PID-node0.img"}, "node-name": "node0"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node1.img"}, "node-name": "node1"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node2.img"}, "node-name": "node2"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node3.img"}, "node-name": "node3"}, "driver": "IMGFMT", "file": {"driver": "throttle", "file": {"driver": "file", "filename": "TEST_DIR/PID-node4.img"}, "throttle-group": "tg"}, "node-name": "node4"}}

{"return": {}}

@@ -XXX,XX +XXX,XX @@ Running tests:

=== Commit and stream finish concurrently (letting commit write) ===

-{"execute": "object-add", "arguments": {"id": "tg", "props": {"x-iops-write": 1, "x-iops-write-max": 1}, "qom-type": "throttle-group"}}

+{"execute": "object-add", "arguments": {"id": "tg", "limits": {"iops-write": 1, "iops-write-max": 1}, "qom-type": "throttle-group"}}

{"return": {}}

{"execute": "blockdev-add", "arguments": {"backing": {"backing": {"backing": {"backing": {"driver": "raw", "file": {"driver": "throttle", "file": {"driver": "file", "filename": "TEST_DIR/PID-node0.img"}, "throttle-group": "tg"}, "node-name": "node0"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node1.img"}, "node-name": "node1"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node2.img"}, "node-name": "node2"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node3.img"}, "node-name": "node3"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node4.img"}, "node-name": "node4"}}

{"return": {}}

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

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/295

+++ b/tests/qemu-iotests/295

@@ -XXX,XX +XXX,XX @@ class Secret:

def to_qmp_object(self):

return { "qom_type" : "secret", "id": self.id(),

- "props": { "data": self.secret() } }

+ "data": self.secret() }

################################################################################

class EncryptionSetupTestCase(iotests.QMPTestCase):

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

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/296

+++ b/tests/qemu-iotests/296

@@ -XXX,XX +XXX,XX @@ class Secret:

def to_qmp_object(self):

return { "qom_type" : "secret", "id": self.id(),

- "props": { "data": self.secret() } }

+ "data": self.secret() }

################################################################################

--

2.29.2

From: Max Reitz <mreitz@redhat.com>

The block job holds a reference to the backup-top node (because it is

passed as the main job BDS to block_job_create()). Therefore,

bdrv_backup_top_drop() cannot delete the backup-top node (replacing it

by its child does not affect the job parent, because that has

.stay_at_node set). That is a problem, because all of its I/O functions

assume the BlockCopyState (s->bcs) to be valid and that it has a

filtered child; but after bdrv_backup_top_drop(), neither of those

things are true.

It does not make sense to add new parents to backup-top after

backup_clean(), so we should detach it from the job before

bdrv_backup_top_drop(). Because there is no function to do that for a

single node, just detach all of the job's nodes -- the job does not do

anything past backup_clean() anyway.

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

Message-Id: <20210219153348.41861-2-mreitz@redhat.com>

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

---

block/backup.c | 1 +

1 file changed, 1 insertion(+)

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

index XXXXXXX..XXXXXXX 100644

--- a/block/backup.c

+++ b/block/backup.c

@@ -XXX,XX +XXX,XX @@ static void backup_abort(Job *job)

static void backup_clean(Job *job)

{

BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);

+ block_job_remove_all_bdrv(&s->common);

bdrv_backup_top_drop(s->backup_top);

}

--

2.29.2

From: Max Reitz <mreitz@redhat.com>

When the backup-top node transitions from active to inactive in

bdrv_backup_top_drop(), the BlockCopyState is freed and the filtered

child is removed, so the node effectively becomes unusable.

However, noone told its I/O functions this, so they will happily

continue accessing bs->backing and s->bcs. Prevent that by aborting

early when s->active is false.

(After the preceding patch, the node should be gone after

bdrv_backup_top_drop(), so this should largely be a theoretical problem.

But still, better to be safe than sorry, and also I think it just makes

sense to check s->active in the I/O functions.)

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

Message-Id: <20210219153348.41861-3-mreitz@redhat.com>

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

---

block/backup-top.c | 10 ++++++++++

1 file changed, 10 insertions(+)

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

index XXXXXXX..XXXXXXX 100644

--- a/block/backup-top.c

+++ b/block/backup-top.c

@@ -XXX,XX +XXX,XX @@ static coroutine_fn int backup_top_co_preadv(

BlockDriverState *bs, uint64_t offset, uint64_t bytes,

QEMUIOVector *qiov, int flags)

{

+ BDRVBackupTopState *s = bs->opaque;

+

+ if (!s->active) {

+ return -EIO;

+ }

+

return bdrv_co_preadv(bs->backing, offset, bytes, qiov, flags);

}

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

BDRVBackupTopState *s = bs->opaque;

uint64_t off, end;

+ if (!s->active) {

+ return -EIO;

+ }

+

if (flags & BDRV_REQ_WRITE_UNCHANGED) {

return 0;

}

--

2.29.2

From: Max Reitz <mreitz@redhat.com>

Without any of HEAD^ or HEAD^^ applied, qemu will most likely crash on

the qemu-io invocation, for a variety of immediate reasons. The

underlying problem is generally a use-after-free access into

backup-top's BlockCopyState.

With only HEAD^ applied, qemu-io will run into an EIO (which is not

capture by the output, but you can see that the qemu-io invocation will

be accepted (i.e., qemu-io will run) in contrast to the reference

output, where the node name cannot be found), and qemu will then crash

in query-named-block-nodes: bdrv_get_allocated_file_size() detects

backup-top to be a filter and passes the request through to its child.

However, after bdrv_backup_top_drop(), that child is NULL, so the

recursive call crashes.

With HEAD^^ applied, this test should pass.

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

Message-Id: <20210219153348.41861-4-mreitz@redhat.com>

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

---

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

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

2 files changed, 68 insertions(+)

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

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/283

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

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

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

vm.shutdown()

+

+

+print('\n=== backup-top should be gone after job-finalize ===\n')

+

+# Check that the backup-top node is gone after job-finalize.

+#

+# During finalization, the node becomes inactive and can no longer

+# function. If it is still present, new parents might be attached, and

+# there would be no meaningful way to handle their I/O requests.

+

+vm = iotests.VM()

+vm.launch()

+

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

+ 'node-name': 'source',

+ 'driver': 'null-co',

+})

+

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

+ 'node-name': 'target',

+ 'driver': 'null-co',

+})

+

+vm.qmp_log('blockdev-backup',

+ job_id='backup',

+ device='source',

+ target='target',

+ sync='full',

+ filter_node_name='backup-filter',

+ auto_finalize=False,

+ auto_dismiss=False)

+

+vm.event_wait('BLOCK_JOB_PENDING', 5.0)

+

+# The backup-top filter should still be present prior to finalization

+assert vm.node_info('backup-filter') is not None

+

+vm.qmp_log('job-finalize', id='backup')

+vm.event_wait('BLOCK_JOB_COMPLETED', 5.0)

+

+# The filter should be gone now. Check that by trying to access it

+# with qemu-io (which will most likely crash qemu if it is still

+# there.).

+vm.qmp_log('human-monitor-command',

+ command_line='qemu-io backup-filter "write 0 1M"')

+

+# (Also, do an explicit check.)

+assert vm.node_info('backup-filter') is None

+

+vm.qmp_log('job-dismiss', id='backup')

+vm.event_wait('JOB_STATUS_CHANGE', 5.0, {'data': {'status': 'null'}})

+

+vm.shutdown()

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 @@

{"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"}}

+

+=== backup-top should be gone after job-finalize ===

+

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

+{"return": {}}

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

+{"return": {}}

+{"execute": "blockdev-backup", "arguments": {"auto-dismiss": false, "auto-finalize": false, "device": "source", "filter-node-name": "backup-filter", "job-id": "backup", "sync": "full", "target": "target"}}

+{"return": {}}

+{"execute": "job-finalize", "arguments": {"id": "backup"}}

+{"return": {}}

+{"execute": "human-monitor-command", "arguments": {"command-line": "qemu-io backup-filter \"write 0 1M\""}}

+{"return": "Error: Cannot find device= nor node_name=backup-filter\r\n"}

+{"execute": "job-dismiss", "arguments": {"id": "backup"}}

+{"return": {}}

--

2.29.2

From: Eric Blake <eblake@redhat.com>

Break some long lines, and relax our type hints to be more generic to

any JSON, in order to more easily permit the additional JSON depth now

possible in migration parameters. Detected by iotest 297.

Fixes: ca4bfec41d56

(qemu-iotests: 300: Add test case for modifying persistence of bitmap)

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

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

Message-Id: <20210215220518.1745469-1-eblake@redhat.com>

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

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

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

---

tests/qemu-iotests/300 | 10 ++++++----

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

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

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/300

+++ b/tests/qemu-iotests/300

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

import os

import random

import re

-from typing import Dict, List, Optional, Union

+from typing import Dict, List, Optional

import iotests

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

# pylint: disable=wrong-import-order

import qemu

-BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]]

+BlockBitmapMapping = List[Dict[str, object]]

mig_sock = os.path.join(iotests.sock_dir, 'mig_sock')

@@ -XXX,XX +XXX,XX @@ class TestCrossAliasMigration(TestDirtyBitmapMigration):

class TestAliasTransformMigration(TestDirtyBitmapMigration):

"""

- Tests the 'transform' option which modifies bitmap persistence on migration.

+ Tests the 'transform' option which modifies bitmap persistence on

+ migration.

"""

src_node_name = 'node-a'

@@ -XXX,XX +XXX,XX @@ class TestAliasTransformMigration(TestDirtyBitmapMigration):

bitmaps = self.vm_b.query_bitmaps()

for node in bitmaps:

- bitmaps[node] = sorted(((bmap['name'], bmap['persistent']) for bmap in bitmaps[node]))

+ bitmaps[node] = sorted(((bmap['name'], bmap['persistent'])

+ for bmap in bitmaps[node]))

self.assertEqual(bitmaps,

{'node-a': [('bmap-a', True), ('bmap-b', False)],

--

2.29.2

From: Stefano Garzarella <sgarzare@redhat.com>

When a block job fails, we report strerror(-job->job.ret) error

message, also if the job set an error object.

Let's report a better error message using error_get_pretty(job->job.err).

If an error object was not set, strerror(-job->ret) is used as fallback,

as explained in include/qemu/job.h:

typedef struct Job {

...

/**

* Error object for a failed job.

* If job->ret is nonzero and an error object was not set, it will be set

* to strerror(-job->ret) during job_completed.

*/

Error *err;

}

In block_job_query() there can be a transient where 'job.err' is not set

by a scheduled bottom half. In that case we use strerror(-job->ret) as it

was before.

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

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

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

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

---

blockjob.c | 10 +++++++---

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

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

index XXXXXXX..XXXXXXX 100644

--- a/blockjob.c

+++ b/blockjob.c

@@ -XXX,XX +XXX,XX @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp)

info->status = job->job.status;

info->auto_finalize = job->job.auto_finalize;

info->auto_dismiss = job->job.auto_dismiss;

- info->has_error = job->job.ret != 0;

- info->error = job->job.ret ? g_strdup(strerror(-job->job.ret)) : NULL;

+ if (job->job.ret) {

+ info->has_error = true;

+ info->error = job->job.err ?

+ g_strdup(error_get_pretty(job->job.err)) :

+ g_strdup(strerror(-job->job.ret));

+ }

return info;

}

@@ -XXX,XX +XXX,XX @@ static void block_job_event_completed(Notifier *n, void *opaque)

}

if (job->job.ret < 0) {

- msg = strerror(-job->job.ret);

+ msg = error_get_pretty(job->job.err);

}

qapi_event_send_block_job_completed(job_type(&job->job),

--

2.29.2

From: Paolo Bonzini <pbonzini@redhat.com>

If the first character of optstring is '-', then each nonoption argv

element is handled as if it were the argument of an option with character

code 1. This removes the reordering of the argv array, and enables usage

of loc_set_cmdline to provide better error messages.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Message-Id: <20210301152844.291799-2-pbonzini@redhat.com>

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

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

---

storage-daemon/qemu-storage-daemon.c | 9 ++++-----

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

diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c

index XXXXXXX..XXXXXXX 100644

--- a/storage-daemon/qemu-storage-daemon.c

+++ b/storage-daemon/qemu-storage-daemon.c

@@ -XXX,XX +XXX,XX @@ static void process_options(int argc, char *argv[])

* they are given on the command lines. This means that things must be

* defined first before they can be referenced in another option.

*/

- while ((c = getopt_long(argc, argv, "hT:V", long_options, NULL)) != -1) {

+ while ((c = getopt_long(argc, argv, "-hT:V", long_options, NULL)) != -1) {

switch (c) {

case '?':

exit(EXIT_FAILURE);

@@ -XXX,XX +XXX,XX @@ static void process_options(int argc, char *argv[])

qobject_unref(args);

break;

}

+ case 1:

+ error_report("Unexpected argument: %s", optarg);

+ exit(EXIT_FAILURE);

default:

g_assert_not_reached();

}

}

- if (optind != argc) {

- error_report("Unexpected argument: %s", argv[optind]);

- exit(EXIT_FAILURE);

- }

}

int main(int argc, char *argv[])

--

2.29.2

From: Paolo Bonzini <pbonzini@redhat.com>

Use the location management facilities that the emulator uses, so that

the current command line option appears in the error message.

Before:

$ storage-daemon/qemu-storage-daemon --nbd key..=

qemu-storage-daemon: Invalid parameter 'key..'

After:

$ storage-daemon/qemu-storage-daemon --nbd key..=

qemu-storage-daemon: --nbd key..=: Invalid parameter 'key..'

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

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Message-Id: <20210301152844.291799-3-pbonzini@redhat.com>

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

---

storage-daemon/qemu-storage-daemon.c | 19 +++++++++++++++++--

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

diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c

index XXXXXXX..XXXXXXX 100644

--- a/storage-daemon/qemu-storage-daemon.c

+++ b/storage-daemon/qemu-storage-daemon.c

@@ -XXX,XX +XXX,XX @@ static void init_qmp_commands(void)

qmp_marshal_qmp_capabilities, QCO_ALLOW_PRECONFIG);

}

+static int getopt_set_loc(int argc, char **argv, const char *optstring,

+ const struct option *longopts)

+{

+ int c, save_index;

+

+ optarg = NULL;

+ save_index = optind;

+ c = getopt_long(argc, argv, optstring, longopts, NULL);

+ if (optarg) {

+ loc_set_cmdline(argv, save_index, MAX(1, optind - save_index));

+ }

+ return c;

+}

+

static void process_options(int argc, char *argv[])

{

int c;

@@ -XXX,XX +XXX,XX @@ static void process_options(int argc, char *argv[])

* they are given on the command lines. This means that things must be

* defined first before they can be referenced in another option.

*/

- while ((c = getopt_long(argc, argv, "-hT:V", long_options, NULL)) != -1) {

+ while ((c = getopt_set_loc(argc, argv, "-hT:V", long_options)) != -1) {

switch (c) {

case '?':

exit(EXIT_FAILURE);

@@ -XXX,XX +XXX,XX @@ static void process_options(int argc, char *argv[])

break;

}

case 1:

- error_report("Unexpected argument: %s", optarg);

+ error_report("Unexpected argument");

exit(EXIT_FAILURE);

default:

g_assert_not_reached();

}

}

+ loc_set_none();

}

int main(int argc, char *argv[])

--

2.29.2

From: Stefan Hajnoczi <stefanha@redhat.com>

Daemons often have a --pidfile option where the pid is written to a file

so that scripts can stop the daemon by sending a signal.

The pid file also acts as a lock to prevent multiple instances of the

daemon from launching for a given pid file.

QEMU, qemu-nbd, qemu-ga, virtiofsd, and qemu-pr-helper all support the

--pidfile option. Add it to qemu-storage-daemon too.

Reported-by: Richard W.M. Jones <rjones@redhat.com>

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Message-Id: <20210302142746.170535-1-stefanha@redhat.com>

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>

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

---

docs/tools/qemu-storage-daemon.rst | 14 +++++++++++

storage-daemon/qemu-storage-daemon.c | 36 ++++++++++++++++++++++++++++

2 files changed, 50 insertions(+)

diff --git a/docs/tools/qemu-storage-daemon.rst b/docs/tools/qemu-storage-daemon.rst

index XXXXXXX..XXXXXXX 100644

--- a/docs/tools/qemu-storage-daemon.rst

+++ b/docs/tools/qemu-storage-daemon.rst

@@ -XXX,XX +XXX,XX @@ Standard options:

List object properties with ``<type>,help``. See the :manpage:`qemu(1)`

manual page for a description of the object properties.

+.. option:: --pidfile PATH

+

+ is the path to a file where the daemon writes its pid. This allows scripts to

+ stop the daemon by sending a signal::

+

+ $ kill -SIGTERM $(<path/to/qsd.pid)

+

+ A file lock is applied to the file so only one instance of the daemon can run

+ with a given pid file path. The daemon unlinks its pid file when terminating.

+

+ The pid file is written after chardevs, exports, and NBD servers have been

+ created but before accepting connections. The daemon has started successfully

+ when the pid file is written and clients may begin connecting.

+

Examples

--------

Launch the daemon with QMP monitor socket ``qmp.sock`` so clients can execute

diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c

index XXXXXXX..XXXXXXX 100644

--- a/storage-daemon/qemu-storage-daemon.c

+++ b/storage-daemon/qemu-storage-daemon.c

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

#include "sysemu/runstate.h"

#include "trace/control.h"

+static const char *pid_file;

static volatile bool exit_requested = false;

void qemu_system_killed(int signal, pid_t pid)

@@ -XXX,XX +XXX,XX @@ static void help(void)

" See the qemu(1) man page for documentation of the\n"

" objects that can be added.\n"

"\n"

+" --pidfile <path> write process ID to a file after startup\n"

+"\n"

QEMU_HELP_BOTTOM "\n",

error_get_progname());

}

@@ -XXX,XX +XXX,XX @@ enum {

OPTION_MONITOR,

OPTION_NBD_SERVER,

OPTION_OBJECT,

+ OPTION_PIDFILE,

};

extern QemuOptsList qemu_chardev_opts;

@@ -XXX,XX +XXX,XX @@ static void process_options(int argc, char *argv[])

{"monitor", required_argument, NULL, OPTION_MONITOR},

{"nbd-server", required_argument, NULL, OPTION_NBD_SERVER},

{"object", required_argument, NULL, OPTION_OBJECT},

+ {"pidfile", required_argument, NULL, OPTION_PIDFILE},

{"trace", required_argument, NULL, 'T'},

{"version", no_argument, NULL, 'V'},

{0, 0, 0, 0}

@@ -XXX,XX +XXX,XX @@ static void process_options(int argc, char *argv[])

qobject_unref(args);

break;

}

+ case OPTION_PIDFILE:

+ pid_file = optarg;

+ break;

case 1:

error_report("Unexpected argument");

exit(EXIT_FAILURE);

@@ -XXX,XX +XXX,XX @@ static void process_options(int argc, char *argv[])

loc_set_none();

}

+static void pid_file_cleanup(void)

+{

+ unlink(pid_file);

+}

+

+static void pid_file_init(void)

+{

+ Error *err = NULL;

+

+ if (!pid_file) {

+ return;

+ }

+

+ if (!qemu_write_pidfile(pid_file, &err)) {

+ error_reportf_err(err, "cannot create PID file: ");

+ exit(EXIT_FAILURE);

+ }

+

+ atexit(pid_file_cleanup);

+}

+

int main(int argc, char *argv[])

{

#ifdef CONFIG_POSIX

@@ -XXX,XX +XXX,XX @@ int main(int argc, char *argv[])

qemu_init_main_loop(&error_fatal);

process_options(argc, argv);

+ /*

+ * Write the pid file after creating chardevs, exports, and NBD servers but

+ * before accepting connections. This ordering is documented. Do not change

+ * it.

+ */

+ pid_file_init();

+

while (!exit_requested) {

main_loop_wait(false);

}

--

2.29.2

From: Stefan Hajnoczi <stefanha@redhat.com>

The QMP monitor, NBD server, and vhost-user-blk export all support file

descriptor passing. This is a useful technique because it allows the

parent process to spawn and wait for qemu-storage-daemon without busy

waiting, which may delay startup due to arbitrary sleep() calls.

This Python example is inspired by the test case written for libnbd by

Richard W.M. Jones <rjones@redhat.com>:

https://gitlab.com/nbdkit/libnbd/-/commit/89113f484effb0e6c322314ba75c1cbe07a04543

Thanks to Daniel P. Berrangé <berrange@redhat.com> for suggestions on

how to get this working. Now let's document it!

Reported-by: Richard W.M. Jones <rjones@redhat.com>

Cc: Kevin Wolf <kwolf@redhat.com>

Cc: Daniel P. Berrangé <berrange@redhat.com>

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Message-Id: <20210301172728.135331-2-stefanha@redhat.com>

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>

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

---

docs/tools/qemu-storage-daemon.rst | 42 ++++++++++++++++++++++++++++--

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

diff --git a/docs/tools/qemu-storage-daemon.rst b/docs/tools/qemu-storage-daemon.rst

index XXXXXXX..XXXXXXX 100644

--- a/docs/tools/qemu-storage-daemon.rst

+++ b/docs/tools/qemu-storage-daemon.rst

@@ -XXX,XX +XXX,XX @@ Standard options:

.. option:: --nbd-server addr.type=inet,addr.host=<host>,addr.port=<port>[,tls-creds=<id>][,tls-authz=<id>][,max-connections=<n>]

--nbd-server addr.type=unix,addr.path=<path>[,tls-creds=<id>][,tls-authz=<id>][,max-connections=<n>]

+ --nbd-server addr.type=fd,addr.str=<fd>[,tls-creds=<id>][,tls-authz=<id>][,max-connections=<n>]

is a server for NBD exports. Both TCP and UNIX domain sockets are supported.

- TLS encryption can be configured using ``--object`` tls-creds-* and authz-*

- secrets (see below).

+ A listen socket can be provided via file descriptor passing (see Examples

+ below). TLS encryption can be configured using ``--object`` tls-creds-* and

+ authz-* secrets (see below).

To configure an NBD server on UNIX domain socket path ``/tmp/nbd.sock``::

@@ -XXX,XX +XXX,XX @@ QMP commands::

--chardev socket,path=qmp.sock,server=on,wait=off,id=char1 \

--monitor chardev=char1

+Launch the daemon from Python with a QMP monitor socket using file descriptor

+passing so there is no need to busy wait for the QMP monitor to become

+available::

+

+ #!/usr/bin/env python3

+ import subprocess

+ import socket

+

+ sock_path = '/var/run/qmp.sock'

+

+ with socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) as listen_sock:

+ listen_sock.bind(sock_path)

+ listen_sock.listen()

+

+ fd = listen_sock.fileno()

+

+ subprocess.Popen(

+ ['qemu-storage-daemon',

+ '--chardev', f'socket,fd={fd},server=on,id=char1',

+ '--monitor', 'chardev=char1'],

+ pass_fds=[fd],

+ )

+

+ # listen_sock was automatically closed when leaving the 'with' statement

+ # body. If the daemon process terminated early then the following connect()

+ # will fail with "Connection refused" because no process has the listen

+ # socket open anymore. Launch errors can be detected this way.

+

+ qmp_sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)

+ qmp_sock.connect(sock_path)

+ ...QMP interaction...

+

+The same socket spawning approach also works with the ``--nbd-server

+addr.type=fd,addr.str=<fd>`` and ``--export

+type=vhost-user-blk,addr.type=fd,addr.str=<fd>`` options.

+

Export raw image file ``disk.img`` over NBD UNIX domain socket ``nbd.sock``::

$ qemu-storage-daemon \

--

2.29.2

From: Stefan Hajnoczi <stefanha@redhat.com>

World-writeable directories have security issues. Avoid showing them in

the documentation since someone might accidentally use them in

situations where they are insecure.

There tend to be 3 security problems:

1. Denial of service. An adversary may be able to create the file

beforehand, consume all space/inodes, etc to sabotage us.

2. Impersonation. An adversary may be able to create a listen socket and

accept incoming connections that were meant for us.

3. Unauthenticated client access. An adversary may be able to connect to

us if we did not set the uid/gid and permissions correctly.

These can be prevented or mitigated with private /tmp, carefully setting

the umask, etc but that requires special action and does not apply to

all situations. Just avoid using /tmp in examples.

Reported-by: Richard W.M. Jones <rjones@redhat.com>

Reported-by: Daniel P. Berrangé <berrange@redhat.com>

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Message-Id: <20210301172728.135331-3-stefanha@redhat.com>

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>

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

---

docs/tools/qemu-storage-daemon.rst | 7 ++++---

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

diff --git a/docs/tools/qemu-storage-daemon.rst b/docs/tools/qemu-storage-daemon.rst

index XXXXXXX..XXXXXXX 100644

--- a/docs/tools/qemu-storage-daemon.rst

+++ b/docs/tools/qemu-storage-daemon.rst

@@ -XXX,XX +XXX,XX @@ Standard options:

a description of character device properties. A common character device

definition configures a UNIX domain socket::

- --chardev socket,id=char1,path=/tmp/qmp.sock,server=on,wait=off

+ --chardev socket,id=char1,path=/var/run/qsd-qmp.sock,server=on,wait=off

.. option:: --export [type=]nbd,id=<id>,node-name=<node-name>[,name=<export-name>][,writable=on|off][,bitmap=<name>]

--export [type=]vhost-user-blk,id=<id>,node-name=<node-name>,addr.type=unix,addr.path=<socket-path>[,writable=on|off][,logical-block-size=<block-size>][,num-queues=<num-queues>]

@@ -XXX,XX +XXX,XX @@ Standard options:

below). TLS encryption can be configured using ``--object`` tls-creds-* and

authz-* secrets (see below).

- To configure an NBD server on UNIX domain socket path ``/tmp/nbd.sock``::

+ To configure an NBD server on UNIX domain socket path

+ ``/var/run/qsd-nbd.sock``::

- --nbd-server addr.type=unix,addr.path=/tmp/nbd.sock

+ --nbd-server addr.type=unix,addr.path=/var/run/qsd-nbd.sock

.. option:: --object help

--object <type>,help

--

2.29.2

From: Stefan Hajnoczi <stefanha@redhat.com>

Treat the num_queues field as virtio-endian. On big-endian hosts the

vhost-user-blk num_queues field was in the wrong endianness.

Move the blkcfg.num_queues store operation from realize to

vhost_user_blk_update_config() so feature negotiation has finished and

we know the endianness of the device. VIRTIO 1.0 devices are

little-endian, but in case someone wants to use legacy VIRTIO we support

all endianness cases.

Cc: qemu-stable@nongnu.org

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Message-Id: <20210223144653.811468-2-stefanha@redhat.com>

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

---

hw/block/vhost-user-blk.c | 7 +++----

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

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c

index XXXXXXX..XXXXXXX 100644

--- a/hw/block/vhost-user-blk.c

+++ b/hw/block/vhost-user-blk.c

@@ -XXX,XX +XXX,XX @@ static void vhost_user_blk_update_config(VirtIODevice *vdev, uint8_t *config)

{

VHostUserBlk *s = VHOST_USER_BLK(vdev);

+ /* Our num_queues overrides the device backend */

+ virtio_stw_p(vdev, &s->blkcfg.num_queues, s->num_queues);

+

memcpy(config, &s->blkcfg, sizeof(struct virtio_blk_config));

}

@@ -XXX,XX +XXX,XX @@ reconnect:

goto reconnect;

}

- if (s->blkcfg.num_queues != s->num_queues) {

- s->blkcfg.num_queues = s->num_queues;

- }

-

return;

virtio_err:

--

2.29.2

From: Stefan Hajnoczi <stefanha@redhat.com>

Add an API that returns a new UNIX domain socket in the listen state.

The code for this was already there but only used internally in

init_socket().

This new API will be used by vhost-user-blk-test.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

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

Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>

Message-Id: <20210223144653.811468-3-stefanha@redhat.com>

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

---

tests/qtest/libqos/libqtest.h | 8 +++++++

tests/qtest/libqtest.c | 40 ++++++++++++++++++++---------------

2 files changed, 31 insertions(+), 17 deletions(-)

diff --git a/tests/qtest/libqos/libqtest.h b/tests/qtest/libqos/libqtest.h

index XXXXXXX..XXXXXXX 100644

--- a/tests/qtest/libqos/libqtest.h

+++ b/tests/qtest/libqos/libqtest.h

@@ -XXX,XX +XXX,XX @@ void qtest_qmp_send(QTestState *s, const char *fmt, ...)

void qtest_qmp_send_raw(QTestState *s, const char *fmt, ...)

GCC_FMT_ATTR(2, 3);

+/**

+ * qtest_socket_server:

+ * @socket_path: the UNIX domain socket path

+ *

+ * Create and return a listen socket file descriptor, or abort on failure.

+ */

+int qtest_socket_server(const char *socket_path);

+

/**

* qtest_vqmp_fds:

* @s: #QTestState instance to operate on.

diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c

index XXXXXXX..XXXXXXX 100644

--- a/tests/qtest/libqtest.c

+++ b/tests/qtest/libqtest.c

@@ -XXX,XX +XXX,XX @@ static void qtest_client_set_rx_handler(QTestState *s, QTestRecvFn recv);

static int init_socket(const char *socket_path)

{

- struct sockaddr_un addr;

- int sock;

- int ret;

-

- sock = socket(PF_UNIX, SOCK_STREAM, 0);

- g_assert_cmpint(sock, !=, -1);

-

- addr.sun_family = AF_UNIX;

- snprintf(addr.sun_path, sizeof(addr.sun_path), "%s", socket_path);

+ int sock = qtest_socket_server(socket_path);

qemu_set_cloexec(sock);

-

- do {

- ret = bind(sock, (struct sockaddr *)&addr, sizeof(addr));

- } while (ret == -1 && errno == EINTR);

- g_assert_cmpint(ret, !=, -1);

- ret = listen(sock, 1);

- g_assert_cmpint(ret, !=, -1);

-

return sock;

}

@@ -XXX,XX +XXX,XX @@ QDict *qtest_qmp_receive_dict(QTestState *s)

return qmp_fd_receive(s->qmp_fd);

}

+int qtest_socket_server(const char *socket_path)

+{

+ struct sockaddr_un addr;

+ int sock;

+ int ret;

+

+ sock = socket(PF_UNIX, SOCK_STREAM, 0);

+ g_assert_cmpint(sock, !=, -1);

+

+ addr.sun_family = AF_UNIX;

+ snprintf(addr.sun_path, sizeof(addr.sun_path), "%s", socket_path);

+

+ do {

+ ret = bind(sock, (struct sockaddr *)&addr, sizeof(addr));

+ } while (ret == -1 && errno == EINTR);

+ g_assert_cmpint(ret, !=, -1);

+ ret = listen(sock, 1);

+ g_assert_cmpint(ret, !=, -1);

+

+ return sock;

+}

+

/**

* Allow users to send a message without waiting for the reply,

* in the case that they choose to discard all replies up until

--

2.29.2

From: Stefan Hajnoczi <stefanha@redhat.com>

Tests that manage multiple processes may wish to kill QEMU before

destroying the QTestState. Expose a function to do that.

The vhost-user-blk-test testcase will need this.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>

Message-Id: <20210223144653.811468-4-stefanha@redhat.com>

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

---

tests/qtest/libqos/libqtest.h | 11 +++++++++++

tests/qtest/libqtest.c | 7 ++++---

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

diff --git a/tests/qtest/libqos/libqtest.h b/tests/qtest/libqos/libqtest.h

index XXXXXXX..XXXXXXX 100644

--- a/tests/qtest/libqos/libqtest.h

+++ b/tests/qtest/libqos/libqtest.h

@@ -XXX,XX +XXX,XX @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args);

*/

QTestState *qtest_init_with_serial(const char *extra_args, int *sock_fd);

+/**

+ * qtest_kill_qemu:

+ * @s: #QTestState instance to operate on.

+ *

+ * Kill the QEMU process and wait for it to terminate. It is safe to call this

+ * function multiple times. Normally qtest_quit() is used instead because it

+ * also frees QTestState. Use qtest_kill_qemu() when you just want to kill QEMU

+ * and qtest_quit() will be called later.

+ */

+void qtest_kill_qemu(QTestState *s);

+

/**

* qtest_quit:

* @s: #QTestState instance to operate on.

diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c

index XXXXXXX..XXXXXXX 100644

--- a/tests/qtest/libqtest.c

+++ b/tests/qtest/libqtest.c

@@ -XXX,XX +XXX,XX @@ void qtest_set_expected_status(QTestState *s, int status)

s->expected_status = status;

}

-static void kill_qemu(QTestState *s)

+void qtest_kill_qemu(QTestState *s)

{

pid_t pid = s->qemu_pid;

int wstatus;

@@ -XXX,XX +XXX,XX @@ static void kill_qemu(QTestState *s)