From fa818b2febfc090acb9b2e69c1c2a4e4b38aee83 Mon Sep 17 00:00:00 2001 From: Alberto Garcia Date: Mon, 22 Feb 2021 12:57:37 +0100 Subject: [PATCH 01/30] iotests: Drop deprecated 'props' from object-add Signed-off-by: Alberto Garcia Message-Id: <20210222115737.2993-1-berto@igalia.com> Signed-off-by: Kevin Wolf --- 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 edd43f1a28..d8e0e384cd 100755 --- a/tests/qemu-iotests/087 +++ b/tests/qemu-iotests/087 @@ -143,9 +143,7 @@ run_qemu < Date: Fri, 19 Feb 2021 16:33:46 +0100 Subject: [PATCH 02/30] backup: Remove nodes from job in .clean() 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 Message-Id: <20210219153348.41861-2-mreitz@redhat.com> Signed-off-by: Kevin Wolf --- block/backup.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/backup.c b/block/backup.c index 94e6dcd72e..6cf2f974aa 100644 --- a/block/backup.c +++ b/block/backup.c @@ -103,6 +103,7 @@ 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); } From 705dde27c6c53b73d2aa139b5b2a0ea490153e5b Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 19 Feb 2021 16:33:47 +0100 Subject: [PATCH 03/30] backup-top: Refuse I/O in inactive state 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 Message-Id: <20210219153348.41861-3-mreitz@redhat.com> Signed-off-by: Kevin Wolf --- block/backup-top.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/block/backup-top.c b/block/backup-top.c index d1253e1aa6..589e8b651d 100644 --- a/block/backup-top.c +++ b/block/backup-top.c @@ -45,6 +45,12 @@ 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); } @@ -54,6 +60,10 @@ 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; } From e41799409281eab19c17692d1c52cb4cef7f5494 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 19 Feb 2021 16:33:48 +0100 Subject: [PATCH 04/30] iotests/283: Check that finalize drops backup-top 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 Message-Id: <20210219153348.41861-4-mreitz@redhat.com> Signed-off-by: Kevin Wolf --- 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 79643e375b..010c22f0a2 100755 --- a/tests/qemu-iotests/283 +++ b/tests/qemu-iotests/283 @@ -97,3 +97,56 @@ 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 d8cff22cc1..7e9cd9a7d4 100644 --- a/tests/qemu-iotests/283.out +++ b/tests/qemu-iotests/283.out @@ -6,3 +6,18 @@ {"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": {}} From 4aa6fc69e8d2d64d37af382854ff5b12675248c2 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 15 Feb 2021 16:05:18 -0600 Subject: [PATCH 05/30] iotests: Fix up python style in 300 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 Signed-off-by: Eric Blake Message-Id: <20210215220518.1745469-1-eblake@redhat.com> Reviewed-by: John Snow Reviewed-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Kevin Wolf --- 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 63036f6a6e..adb9276297 100755 --- a/tests/qemu-iotests/300 +++ b/tests/qemu-iotests/300 @@ -22,7 +22,7 @@ import os import random import re -from typing import Dict, List, Optional, Union +from typing import Dict, List, Optional import iotests @@ -30,7 +30,7 @@ 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') @@ -602,7 +602,8 @@ 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' @@ -674,7 +675,8 @@ 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)], From 3b6ad6230e902168f63315e47933025b016f546e Mon Sep 17 00:00:00 2001 From: Stefano Garzarella Date: Thu, 25 Feb 2021 11:36:33 +0100 Subject: [PATCH 06/30] blockjob: report a better error message 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 Signed-off-by: Stefano Garzarella Message-Id: <20210225103633.76746-1-sgarzare@redhat.com> Signed-off-by: Kevin Wolf --- blockjob.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/blockjob.c b/blockjob.c index f2feff051d..ef968017a2 100644 --- a/blockjob.c +++ b/blockjob.c @@ -318,8 +318,12 @@ 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; } @@ -356,7 +360,7 @@ 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), From a5ef35052e66721e9f943b2b9a91176536b4d896 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 1 Mar 2021 10:28:43 -0500 Subject: [PATCH 07/30] storage-daemon: report unexpected arguments on the fly 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 Message-Id: <20210301152844.291799-2-pbonzini@redhat.com> Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- 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 9021a46b3a..b7e1b90fb1 100644 --- a/storage-daemon/qemu-storage-daemon.c +++ b/storage-daemon/qemu-storage-daemon.c @@ -174,7 +174,7 @@ 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); @@ -275,14 +275,13 @@ 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[]) From 501a4b3681c90bbcf610fbbd6335c26af30668d7 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 1 Mar 2021 10:28:44 -0500 Subject: [PATCH 08/30] storage-daemon: include current command line option in the errors 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 Signed-off-by: Paolo Bonzini Message-Id: <20210301152844.291799-3-pbonzini@redhat.com> Signed-off-by: Kevin Wolf --- 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 b7e1b90fb1..78ddf619d4 100644 --- a/storage-daemon/qemu-storage-daemon.c +++ b/storage-daemon/qemu-storage-daemon.c @@ -152,6 +152,20 @@ 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; @@ -174,7 +188,7 @@ 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); @@ -276,12 +290,13 @@ 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[]) From 03d2b412aaf2078425f8472f31c8a9c2340969eb Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Tue, 2 Mar 2021 14:27:46 +0000 Subject: [PATCH 09/30] qemu-storage-daemon: add --pidfile option 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 Signed-off-by: Stefan Hajnoczi Message-Id: <20210302142746.170535-1-stefanha@redhat.com> Reviewed-by: Richard W.M. Jones Signed-off-by: Kevin Wolf --- 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 c05b3d3811..6ce85f2f7d 100644 --- a/docs/tools/qemu-storage-daemon.rst +++ b/docs/tools/qemu-storage-daemon.rst @@ -118,6 +118,20 @@ Standard options: List object properties with ``,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 $( write process ID to a file after startup\n" +"\n" QEMU_HELP_BOTTOM "\n", error_get_progname()); } @@ -126,6 +129,7 @@ enum { OPTION_MONITOR, OPTION_NBD_SERVER, OPTION_OBJECT, + OPTION_PIDFILE, }; extern QemuOptsList qemu_chardev_opts; @@ -178,6 +182,7 @@ 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} @@ -289,6 +294,9 @@ 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); @@ -299,6 +307,27 @@ 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 @@ -326,6 +355,13 @@ 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); } From 3f14b909ebe7296eef6d4b1a1ed5f602ab129602 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Mon, 1 Mar 2021 17:27:27 +0000 Subject: [PATCH 10/30] docs: show how to spawn qemu-storage-daemon with fd passing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 : https://gitlab.com/nbdkit/libnbd/-/commit/89113f484effb0e6c322314ba75c1cbe07a04543 Thanks to Daniel P. Berrangé for suggestions on how to get this working. Now let's document it! Reported-by: Richard W.M. Jones Cc: Kevin Wolf Cc: Daniel P. Berrangé Signed-off-by: Stefan Hajnoczi Message-Id: <20210301172728.135331-2-stefanha@redhat.com> Reviewed-by: Daniel P. Berrangé Reviewed-by: Richard W.M. Jones Signed-off-by: Kevin Wolf --- 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 6ce85f2f7d..5714794775 100644 --- a/docs/tools/qemu-storage-daemon.rst +++ b/docs/tools/qemu-storage-daemon.rst @@ -101,10 +101,12 @@ Standard options: .. option:: --nbd-server addr.type=inet,addr.host=,addr.port=[,tls-creds=][,tls-authz=][,max-connections=] --nbd-server addr.type=unix,addr.path=[,tls-creds=][,tls-authz=][,max-connections=] + --nbd-server addr.type=fd,addr.str=[,tls-creds=][,tls-authz=][,max-connections=] 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``:: @@ -141,6 +143,42 @@ 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=`` and ``--export +type=vhost-user-blk,addr.type=fd,addr.str=`` options. + Export raw image file ``disk.img`` over NBD UNIX domain socket ``nbd.sock``:: $ qemu-storage-daemon \ From e246bf3ddc4d61d03227373fecfdcd4fec3508db Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Mon, 1 Mar 2021 17:27:28 +0000 Subject: [PATCH 11/30] docs: replace insecure /tmp examples in qsd docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reported-by: Daniel P. Berrangé Signed-off-by: Stefan Hajnoczi Message-Id: <20210301172728.135331-3-stefanha@redhat.com> Reviewed-by: Daniel P. Berrangé Reviewed-by: Richard W.M. Jones Signed-off-by: Kevin Wolf --- 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 5714794775..fe3042d609 100644 --- a/docs/tools/qemu-storage-daemon.rst +++ b/docs/tools/qemu-storage-daemon.rst @@ -69,7 +69,7 @@ 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=,node-name=[,name=][,writable=on|off][,bitmap=] --export [type=]vhost-user-blk,id=,node-name=,addr.type=unix,addr.path=[,writable=on|off][,logical-block-size=][,num-queues=] @@ -108,9 +108,10 @@ 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 ,help From 535255b43898d2e96744057eb86f8497d4d7a461 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Tue, 23 Feb 2021 14:46:42 +0000 Subject: [PATCH 12/30] vhost-user-blk: fix blkcfg->num_queues endianness 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 Reviewed-by: Raphael Norwitz Reviewed-by: Michael S. Tsirkin Message-Id: <20210223144653.811468-2-stefanha@redhat.com> Signed-off-by: Kevin Wolf --- 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 da4fbf9084..b870a50e6b 100644 --- a/hw/block/vhost-user-blk.c +++ b/hw/block/vhost-user-blk.c @@ -54,6 +54,9 @@ 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)); } @@ -491,10 +494,6 @@ reconnect: goto reconnect; } - if (s->blkcfg.num_queues != s->num_queues) { - s->blkcfg.num_queues = s->num_queues; - } - return; virtio_err: From 9fb7bb06986741b7fd8427fac9f22177ca38dcff Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Tue, 23 Feb 2021 14:46:43 +0000 Subject: [PATCH 13/30] libqtest: add qtest_socket_server() 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 Reviewed-by: Thomas Huth Reviewed-by: Wainer dos Santos Moschetta Message-Id: <20210223144653.811468-3-stefanha@redhat.com> Signed-off-by: Kevin Wolf --- 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 724f65aa94..e5f1ec590c 100644 --- a/tests/qtest/libqos/libqtest.h +++ b/tests/qtest/libqos/libqtest.h @@ -132,6 +132,14 @@ 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 fd043b0570..b19d2ebda0 100644 --- a/tests/qtest/libqtest.c +++ b/tests/qtest/libqtest.c @@ -81,24 +81,8 @@ 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; } @@ -638,6 +622,28 @@ 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 From 7a23c523762371fd26a7a9ecfa8f16b64618a1ad Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Tue, 23 Feb 2021 14:46:44 +0000 Subject: [PATCH 14/30] libqtest: add qtest_kill_qemu() 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 Reviewed-by: Wainer dos Santos Moschetta Message-Id: <20210223144653.811468-4-stefanha@redhat.com> Signed-off-by: Kevin Wolf --- 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 e5f1ec590c..51287b9276 100644 --- a/tests/qtest/libqos/libqtest.h +++ b/tests/qtest/libqos/libqtest.h @@ -74,6 +74,17 @@ 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 b19d2ebda0..2a98de2907 100644 --- a/tests/qtest/libqtest.c +++ b/tests/qtest/libqtest.c @@ -133,7 +133,7 @@ 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; @@ -143,6 +143,7 @@ static void kill_qemu(QTestState *s) kill(pid, SIGTERM); TFR(pid = waitpid(s->qemu_pid, &s->wstatus, 0)); assert(pid == s->qemu_pid); + s->qemu_pid = -1; } /* @@ -169,7 +170,7 @@ static void kill_qemu(QTestState *s) static void kill_qemu_hook_func(void *s) { - kill_qemu(s); + qtest_kill_qemu(s); } static void sigabrt_handler(int signo) @@ -373,7 +374,7 @@ void qtest_quit(QTestState *s) /* Uninstall SIGABRT handler on last instance */ cleanup_sigabrt_handler(); - kill_qemu(s); + qtest_kill_qemu(s); close(s->fd); close(s->qmp_fd); g_string_free(s->rx, true); From e1fa7f5591c219a94f039754f6fbe58e757e7af6 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Tue, 23 Feb 2021 14:46:45 +0000 Subject: [PATCH 15/30] libqtest: add qtest_remove_abrt_handler() Add a function to remove previously-added abrt handler functions. Now that a symmetric pair of add/remove functions exists we can also balance the SIGABRT handler installation. The signal handler was installed each time qtest_add_abrt_handler() was called. Now it is installed when the abrt handler list becomes non-empty and removed again when the list becomes empty. The qtest_remove_abrt_handler() function will be used by vhost-user-blk-test. Signed-off-by: Stefan Hajnoczi Reviewed-by: Wainer dos Santos Moschetta Message-Id: <20210223144653.811468-5-stefanha@redhat.com> Signed-off-by: Kevin Wolf --- tests/qtest/libqos/libqtest.h | 18 ++++++++++++++++++ tests/qtest/libqtest.c | 35 +++++++++++++++++++++++++++++------ 2 files changed, 47 insertions(+), 6 deletions(-) diff --git a/tests/qtest/libqos/libqtest.h b/tests/qtest/libqos/libqtest.h index 51287b9276..a68dcd79d4 100644 --- a/tests/qtest/libqos/libqtest.h +++ b/tests/qtest/libqos/libqtest.h @@ -649,8 +649,26 @@ void qtest_add_data_func_full(const char *str, void *data, g_free(path); \ } while (0) +/** + * qtest_add_abrt_handler: + * @fn: Handler function + * @data: Argument that is passed to the handler + * + * Add a handler function that is invoked on SIGABRT. This can be used to + * terminate processes and perform other cleanup. The handler can be removed + * with qtest_remove_abrt_handler(). + */ void qtest_add_abrt_handler(GHookFunc fn, const void *data); +/** + * qtest_remove_abrt_handler: + * @data: Argument previously passed to qtest_add_abrt_handler() + * + * Remove an abrt handler that was previously added with + * qtest_add_abrt_handler(). + */ +void qtest_remove_abrt_handler(void *data); + /** * qtest_qmp_assert_success: * @qts: QTestState instance to operate on diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c index 2a98de2907..71e359efcd 100644 --- a/tests/qtest/libqtest.c +++ b/tests/qtest/libqtest.c @@ -196,15 +196,30 @@ static void cleanup_sigabrt_handler(void) sigaction(SIGABRT, &sigact_old, NULL); } +static bool hook_list_is_empty(GHookList *hook_list) +{ + GHook *hook = g_hook_first_valid(hook_list, TRUE); + + if (!hook) { + return false; + } + + g_hook_unref(hook_list, hook); + return true; +} + void qtest_add_abrt_handler(GHookFunc fn, const void *data) { GHook *hook; - /* Only install SIGABRT handler once */ if (!abrt_hooks.is_setup) { g_hook_list_init(&abrt_hooks, sizeof(GHook)); } - setup_sigabrt_handler(); + + /* Only install SIGABRT handler once */ + if (hook_list_is_empty(&abrt_hooks)) { + setup_sigabrt_handler(); + } hook = g_hook_alloc(&abrt_hooks); hook->func = fn; @@ -213,6 +228,17 @@ void qtest_add_abrt_handler(GHookFunc fn, const void *data) g_hook_prepend(&abrt_hooks, hook); } +void qtest_remove_abrt_handler(void *data) +{ + GHook *hook = g_hook_find_data(&abrt_hooks, TRUE, data); + g_hook_destroy_link(&abrt_hooks, hook); + + /* Uninstall SIGABRT handler on last instance */ + if (hook_list_is_empty(&abrt_hooks)) { + cleanup_sigabrt_handler(); + } +} + static const char *qtest_qemu_binary(void) { const char *qemu_bin; @@ -369,10 +395,7 @@ QTestState *qtest_init_with_serial(const char *extra_args, int *sock_fd) void qtest_quit(QTestState *s) { - g_hook_destroy_link(&abrt_hooks, g_hook_find_data(&abrt_hooks, TRUE, s)); - - /* Uninstall SIGABRT handler on last instance */ - cleanup_sigabrt_handler(); + qtest_remove_abrt_handler(s); qtest_kill_qemu(s); close(s->fd); From a4f1542af58fd6ab061e594d4e161f1c8b4a4372 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Tue, 23 Feb 2021 14:46:48 +0000 Subject: [PATCH 16/30] block/export: fix blk_size double byteswap The config->blk_size field is little-endian. Use the native-endian blk_size variable to avoid double byteswapping. Fixes: 11f60f7eaee2630dd6fa0c3a8c49f792e46c4cf1 ("block/export: make vhost-user-blk config space little-endian") Signed-off-by: Stefan Hajnoczi Message-Id: <20210223144653.811468-8-stefanha@redhat.com> Signed-off-by: Kevin Wolf --- block/export/vhost-user-blk-server.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/export/vhost-user-blk-server.c b/block/export/vhost-user-blk-server.c index ab2c4d44c4..7aea132f69 100644 --- a/block/export/vhost-user-blk-server.c +++ b/block/export/vhost-user-blk-server.c @@ -356,7 +356,7 @@ vu_blk_initialize_config(BlockDriverState *bs, config->num_queues = cpu_to_le16(num_queues); config->max_discard_sectors = cpu_to_le32(32768); config->max_discard_seg = cpu_to_le32(1); - config->discard_sector_alignment = cpu_to_le32(config->blk_size >> 9); + config->discard_sector_alignment = cpu_to_le32(blk_size >> 9); config->max_write_zeroes_sectors = cpu_to_le32(32768); config->max_write_zeroes_seg = cpu_to_le32(1); } From 524bac0744e5abf95856fb9e31c01fd2ef102188 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Tue, 23 Feb 2021 14:46:49 +0000 Subject: [PATCH 17/30] block/export: use VIRTIO_BLK_SECTOR_BITS Use VIRTIO_BLK_SECTOR_BITS and VIRTIO_BLK_SECTOR_SIZE when dealing with virtio-blk sector numbers. Although the values happen to be the same as BDRV_SECTOR_BITS and BDRV_SECTOR_SIZE, they are conceptually different. This makes it clearer when we are dealing with virtio-blk sector units. Use VIRTIO_BLK_SECTOR_BITS in vu_blk_initialize_config(). Later patches will use it the new constants the virtqueue request processing code path. Suggested-by: Max Reitz Signed-off-by: Stefan Hajnoczi Message-Id: <20210223144653.811468-9-stefanha@redhat.com> Signed-off-by: Kevin Wolf --- block/export/vhost-user-blk-server.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/block/export/vhost-user-blk-server.c b/block/export/vhost-user-blk-server.c index 7aea132f69..2614a63791 100644 --- a/block/export/vhost-user-blk-server.c +++ b/block/export/vhost-user-blk-server.c @@ -20,6 +20,13 @@ #include "sysemu/block-backend.h" #include "util/block-helpers.h" +/* + * Sector units are 512 bytes regardless of the + * virtio_blk_config->blk_size value. + */ +#define VIRTIO_BLK_SECTOR_BITS 9 +#define VIRTIO_BLK_SECTOR_SIZE (1ull << VIRTIO_BLK_SECTOR_BITS) + enum { VHOST_USER_BLK_NUM_QUEUES_DEFAULT = 1, }; @@ -347,7 +354,8 @@ vu_blk_initialize_config(BlockDriverState *bs, uint32_t blk_size, uint16_t num_queues) { - config->capacity = cpu_to_le64(bdrv_getlength(bs) >> BDRV_SECTOR_BITS); + config->capacity = + cpu_to_le64(bdrv_getlength(bs) >> VIRTIO_BLK_SECTOR_BITS); config->blk_size = cpu_to_le32(blk_size); config->size_max = cpu_to_le32(0); config->seg_max = cpu_to_le32(128 - 2); @@ -356,7 +364,8 @@ vu_blk_initialize_config(BlockDriverState *bs, config->num_queues = cpu_to_le16(num_queues); config->max_discard_sectors = cpu_to_le32(32768); config->max_discard_seg = cpu_to_le32(1); - config->discard_sector_alignment = cpu_to_le32(blk_size >> 9); + config->discard_sector_alignment = + cpu_to_le32(blk_size >> VIRTIO_BLK_SECTOR_BITS); config->max_write_zeroes_sectors = cpu_to_le32(32768); config->max_write_zeroes_seg = cpu_to_le32(1); } @@ -383,7 +392,7 @@ static int vu_blk_exp_create(BlockExport *exp, BlockExportOptions *opts, if (vu_opts->has_logical_block_size) { logical_block_size = vu_opts->logical_block_size; } else { - logical_block_size = BDRV_SECTOR_SIZE; + logical_block_size = VIRTIO_BLK_SECTOR_SIZE; } check_block_size(exp->id, "logical-block-size", logical_block_size, &local_err); From e44362ce317bcc46d409ed6c4a5ed2b46804bcbf Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Tue, 23 Feb 2021 14:46:50 +0000 Subject: [PATCH 18/30] block/export: fix vhost-user-blk export sector number calculation The driver is supposed to honor the blk_size field but the protocol still uses 512-byte sector numbers. It is incorrect to multiply req->sector_num by blk_size. VIRTIO 1.1 5.2.5 Device Initialization says: blk_size can be read to determine the optimal sector size for the driver to use. This does not affect the units used in the protocol (always 512 bytes), but awareness of the correct value can affect performance. Fixes: 3578389bcf76c824a5d82e6586a6f0c71e56f2aa ("block/export: vhost-user block device backend server") Signed-off-by: Stefan Hajnoczi Message-Id: <20210223144653.811468-10-stefanha@redhat.com> Signed-off-by: Kevin Wolf --- block/export/vhost-user-blk-server.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/export/vhost-user-blk-server.c b/block/export/vhost-user-blk-server.c index 2614a63791..f74796241c 100644 --- a/block/export/vhost-user-blk-server.c +++ b/block/export/vhost-user-blk-server.c @@ -144,7 +144,7 @@ static void coroutine_fn vu_blk_virtio_process_req(void *opaque) break; } - int64_t offset = req->sector_num * vexp->blk_size; + int64_t offset = req->sector_num << VIRTIO_BLK_SECTOR_BITS; QEMUIOVector qiov; if (is_write) { qemu_iovec_init_external(&qiov, out_iov, out_num); From db4eadf9f10e19f864d70d1df3a90fbda31b8c06 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Tue, 23 Feb 2021 14:46:51 +0000 Subject: [PATCH 19/30] block/export: port virtio-blk discard/write zeroes input validation Validate discard/write zeroes the same way we do for virtio-blk. Some of these checks are mandated by the VIRTIO specification, others are internal to QEMU. Signed-off-by: Stefan Hajnoczi Message-Id: <20210223144653.811468-11-stefanha@redhat.com> Signed-off-by: Kevin Wolf --- block/export/vhost-user-blk-server.c | 116 +++++++++++++++++++++------ 1 file changed, 93 insertions(+), 23 deletions(-) diff --git a/block/export/vhost-user-blk-server.c b/block/export/vhost-user-blk-server.c index f74796241c..04044228d4 100644 --- a/block/export/vhost-user-blk-server.c +++ b/block/export/vhost-user-blk-server.c @@ -29,6 +29,8 @@ enum { VHOST_USER_BLK_NUM_QUEUES_DEFAULT = 1, + VHOST_USER_BLK_MAX_DISCARD_SECTORS = 32768, + VHOST_USER_BLK_MAX_WRITE_ZEROES_SECTORS = 32768, }; struct virtio_blk_inhdr { unsigned char status; @@ -65,30 +67,102 @@ static void vu_blk_req_complete(VuBlkReq *req) free(req); } +static bool vu_blk_sect_range_ok(VuBlkExport *vexp, uint64_t sector, + size_t size) +{ + uint64_t nb_sectors = size >> BDRV_SECTOR_BITS; + uint64_t total_sectors; + + if (nb_sectors > BDRV_REQUEST_MAX_SECTORS) { + return false; + } + if ((sector << VIRTIO_BLK_SECTOR_BITS) % vexp->blk_size) { + return false; + } + blk_get_geometry(vexp->export.blk, &total_sectors); + if (sector > total_sectors || nb_sectors > total_sectors - sector) { + return false; + } + return true; +} + static int coroutine_fn -vu_blk_discard_write_zeroes(BlockBackend *blk, struct iovec *iov, +vu_blk_discard_write_zeroes(VuBlkExport *vexp, struct iovec *iov, uint32_t iovcnt, uint32_t type) { + BlockBackend *blk = vexp->export.blk; struct virtio_blk_discard_write_zeroes desc; - ssize_t size = iov_to_buf(iov, iovcnt, 0, &desc, sizeof(desc)); + ssize_t size; + uint64_t sector; + uint32_t num_sectors; + uint32_t max_sectors; + uint32_t flags; + int bytes; + + /* Only one desc is currently supported */ + if (unlikely(iov_size(iov, iovcnt) > sizeof(desc))) { + return VIRTIO_BLK_S_UNSUPP; + } + + size = iov_to_buf(iov, iovcnt, 0, &desc, sizeof(desc)); if (unlikely(size != sizeof(desc))) { - error_report("Invalid size %zd, expect %zu", size, sizeof(desc)); - return -EINVAL; + error_report("Invalid size %zd, expected %zu", size, sizeof(desc)); + return VIRTIO_BLK_S_IOERR; } - uint64_t range[2] = { le64_to_cpu(desc.sector) << 9, - le32_to_cpu(desc.num_sectors) << 9 }; - if (type == VIRTIO_BLK_T_DISCARD) { - if (blk_co_pdiscard(blk, range[0], range[1]) == 0) { - return 0; + sector = le64_to_cpu(desc.sector); + num_sectors = le32_to_cpu(desc.num_sectors); + flags = le32_to_cpu(desc.flags); + max_sectors = (type == VIRTIO_BLK_T_WRITE_ZEROES) ? + VHOST_USER_BLK_MAX_WRITE_ZEROES_SECTORS : + VHOST_USER_BLK_MAX_DISCARD_SECTORS; + + /* This check ensures that 'bytes' fits in an int */ + if (unlikely(num_sectors > max_sectors)) { + return VIRTIO_BLK_S_IOERR; + } + + bytes = num_sectors << VIRTIO_BLK_SECTOR_BITS; + + if (unlikely(!vu_blk_sect_range_ok(vexp, sector, bytes))) { + return VIRTIO_BLK_S_IOERR; + } + + /* + * The device MUST set the status byte to VIRTIO_BLK_S_UNSUPP for discard + * and write zeroes commands if any unknown flag is set. + */ + if (unlikely(flags & ~VIRTIO_BLK_WRITE_ZEROES_FLAG_UNMAP)) { + return VIRTIO_BLK_S_UNSUPP; + } + + if (type == VIRTIO_BLK_T_WRITE_ZEROES) { + int blk_flags = 0; + + if (flags & VIRTIO_BLK_WRITE_ZEROES_FLAG_UNMAP) { + blk_flags |= BDRV_REQ_MAY_UNMAP; } - } else if (type == VIRTIO_BLK_T_WRITE_ZEROES) { - if (blk_co_pwrite_zeroes(blk, range[0], range[1], 0) == 0) { - return 0; + + if (blk_co_pwrite_zeroes(blk, sector << VIRTIO_BLK_SECTOR_BITS, + bytes, blk_flags) == 0) { + return VIRTIO_BLK_S_OK; + } + } else if (type == VIRTIO_BLK_T_DISCARD) { + /* + * The device MUST set the status byte to VIRTIO_BLK_S_UNSUPP for + * discard commands if the unmap flag is set. + */ + if (unlikely(flags & VIRTIO_BLK_WRITE_ZEROES_FLAG_UNMAP)) { + return VIRTIO_BLK_S_UNSUPP; + } + + if (blk_co_pdiscard(blk, sector << VIRTIO_BLK_SECTOR_BITS, + bytes) == 0) { + return VIRTIO_BLK_S_OK; } } - return -EINVAL; + return VIRTIO_BLK_S_IOERR; } static void coroutine_fn vu_blk_virtio_process_req(void *opaque) @@ -177,19 +251,13 @@ static void coroutine_fn vu_blk_virtio_process_req(void *opaque) } case VIRTIO_BLK_T_DISCARD: case VIRTIO_BLK_T_WRITE_ZEROES: { - int rc; - if (!vexp->writable) { req->in->status = VIRTIO_BLK_S_IOERR; break; } - rc = vu_blk_discard_write_zeroes(blk, &elem->out_sg[1], out_num, type); - if (rc == 0) { - req->in->status = VIRTIO_BLK_S_OK; - } else { - req->in->status = VIRTIO_BLK_S_IOERR; - } + req->in->status = vu_blk_discard_write_zeroes(vexp, out_iov, out_num, + type); break; } default: @@ -362,11 +430,13 @@ vu_blk_initialize_config(BlockDriverState *bs, config->min_io_size = cpu_to_le16(1); config->opt_io_size = cpu_to_le32(1); config->num_queues = cpu_to_le16(num_queues); - config->max_discard_sectors = cpu_to_le32(32768); + config->max_discard_sectors = + cpu_to_le32(VHOST_USER_BLK_MAX_DISCARD_SECTORS); config->max_discard_seg = cpu_to_le32(1); config->discard_sector_alignment = cpu_to_le32(blk_size >> VIRTIO_BLK_SECTOR_BITS); - config->max_write_zeroes_sectors = cpu_to_le32(32768); + config->max_write_zeroes_sectors + = cpu_to_le32(VHOST_USER_BLK_MAX_WRITE_ZEROES_SECTORS); config->max_write_zeroes_seg = cpu_to_le32(1); } From 05ae4e674e3d47342a7660ae7bc55b393e09f4c7 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Tue, 23 Feb 2021 14:46:53 +0000 Subject: [PATCH 20/30] block/export: port virtio-blk read/write range check Check that the sector number and byte count are valid. Signed-off-by: Stefan Hajnoczi Message-Id: <20210223144653.811468-13-stefanha@redhat.com> Signed-off-by: Kevin Wolf --- block/export/vhost-user-blk-server.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/block/export/vhost-user-blk-server.c b/block/export/vhost-user-blk-server.c index 04044228d4..cb5d896b7b 100644 --- a/block/export/vhost-user-blk-server.c +++ b/block/export/vhost-user-blk-server.c @@ -209,6 +209,8 @@ static void coroutine_fn vu_blk_virtio_process_req(void *opaque) switch (type & ~VIRTIO_BLK_T_BARRIER) { case VIRTIO_BLK_T_IN: case VIRTIO_BLK_T_OUT: { + QEMUIOVector qiov; + int64_t offset; ssize_t ret = 0; bool is_write = type & VIRTIO_BLK_T_OUT; req->sector_num = le64_to_cpu(req->out.sector); @@ -218,13 +220,24 @@ static void coroutine_fn vu_blk_virtio_process_req(void *opaque) break; } - int64_t offset = req->sector_num << VIRTIO_BLK_SECTOR_BITS; - QEMUIOVector qiov; if (is_write) { qemu_iovec_init_external(&qiov, out_iov, out_num); - ret = blk_co_pwritev(blk, offset, qiov.size, &qiov, 0); } else { qemu_iovec_init_external(&qiov, in_iov, in_num); + } + + if (unlikely(!vu_blk_sect_range_ok(vexp, + req->sector_num, + qiov.size))) { + req->in->status = VIRTIO_BLK_S_IOERR; + break; + } + + offset = req->sector_num << VIRTIO_BLK_SECTOR_BITS; + + if (is_write) { + ret = blk_co_pwritev(blk, offset, qiov.size, &qiov, 0); + } else { ret = blk_co_preadv(blk, offset, qiov.size, &qiov, 0); } if (ret >= 0) { From 35f428ba39718711177036ddf112e9299e7f20b2 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Wed, 24 Feb 2021 13:47:02 +0300 Subject: [PATCH 21/30] qcow2-bitmap: make bytes_covered_by_bitmap_cluster() public Rename bytes_covered_by_bitmap_cluster() to bdrv_dirty_bitmap_serialization_coverage() and make it public. It is needed as we are going to share it with bitmap loading in parallels format. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Eric Blake Reviewed-by: Denis V. Lunev Message-Id: <20210224104707.88430-2-vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf --- block/dirty-bitmap.c | 13 +++++++++++++ block/qcow2-bitmap.c | 16 ++-------------- include/block/dirty-bitmap.h | 2 ++ 3 files changed, 17 insertions(+), 14 deletions(-) diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 9b9cd71238..a0eaa28785 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -726,6 +726,19 @@ uint64_t bdrv_dirty_bitmap_serialization_align(const BdrvDirtyBitmap *bitmap) return hbitmap_serialization_align(bitmap->bitmap); } +/* Return the disk size covered by a chunk of serialized bitmap data. */ +uint64_t bdrv_dirty_bitmap_serialization_coverage(int serialized_chunk_size, + const BdrvDirtyBitmap *bitmap) +{ + uint64_t granularity = bdrv_dirty_bitmap_granularity(bitmap); + uint64_t limit = granularity * (serialized_chunk_size << 3); + + assert(QEMU_IS_ALIGNED(limit, + bdrv_dirty_bitmap_serialization_align(bitmap))); + return limit; +} + + void bdrv_dirty_bitmap_serialize_part(const BdrvDirtyBitmap *bitmap, uint8_t *buf, uint64_t offset, uint64_t bytes) diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index 5eef82fa55..42d81c44cd 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -278,18 +278,6 @@ static int free_bitmap_clusters(BlockDriverState *bs, Qcow2BitmapTable *tb) return 0; } -/* Return the disk size covered by a single qcow2 cluster of bitmap data. */ -static uint64_t bytes_covered_by_bitmap_cluster(const BDRVQcow2State *s, - const BdrvDirtyBitmap *bitmap) -{ - uint64_t granularity = bdrv_dirty_bitmap_granularity(bitmap); - uint64_t limit = granularity * (s->cluster_size << 3); - - assert(QEMU_IS_ALIGNED(limit, - bdrv_dirty_bitmap_serialization_align(bitmap))); - return limit; -} - /* load_bitmap_data * @bitmap_table entries must satisfy specification constraints. * @bitmap must be cleared */ @@ -312,7 +300,7 @@ static int load_bitmap_data(BlockDriverState *bs, } buf = g_malloc(s->cluster_size); - limit = bytes_covered_by_bitmap_cluster(s, bitmap); + limit = bdrv_dirty_bitmap_serialization_coverage(s->cluster_size, bitmap); for (i = 0, offset = 0; i < tab_size; ++i, offset += limit) { uint64_t count = MIN(bm_size - offset, limit); uint64_t entry = bitmap_table[i]; @@ -1303,7 +1291,7 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs, } buf = g_malloc(s->cluster_size); - limit = bytes_covered_by_bitmap_cluster(s, bitmap); + limit = bdrv_dirty_bitmap_serialization_coverage(s->cluster_size, bitmap); assert(DIV_ROUND_UP(bm_size, limit) == tb_size); offset = 0; diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h index 36e8da4fc2..f581cf9fd7 100644 --- a/include/block/dirty-bitmap.h +++ b/include/block/dirty-bitmap.h @@ -57,6 +57,8 @@ void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter); uint64_t bdrv_dirty_bitmap_serialization_size(const BdrvDirtyBitmap *bitmap, uint64_t offset, uint64_t bytes); uint64_t bdrv_dirty_bitmap_serialization_align(const BdrvDirtyBitmap *bitmap); +uint64_t bdrv_dirty_bitmap_serialization_coverage(int serialized_chunk_size, + const BdrvDirtyBitmap *bitmap); void bdrv_dirty_bitmap_serialize_part(const BdrvDirtyBitmap *bitmap, uint8_t *buf, uint64_t offset, uint64_t bytes); From 67ae4ace9bce25d37be8dd97630ed336c29d6b72 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Wed, 24 Feb 2021 13:47:03 +0300 Subject: [PATCH 22/30] parallels.txt: fix bitmap L1 table description Actually L1 table entry offset is in 512 bytes sectors. Fix the spec. Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20210224104707.88430-3-vsementsov@virtuozzo.com> Reviewed-by: Denis V. Lunev Signed-off-by: Kevin Wolf --- docs/interop/parallels.txt | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/docs/interop/parallels.txt b/docs/interop/parallels.txt index f15bf35bd1..bb3fadf369 100644 --- a/docs/interop/parallels.txt +++ b/docs/interop/parallels.txt @@ -208,21 +208,25 @@ of its data area are: 28 - 31: l1_size The number of entries in the L1 table of the bitmap. - variable: l1_table (8 * l1_size bytes) - L1 offset table (in bytes) + variable: L1 offset table (l1_table), size: 8 * l1_size bytes -A dirty bitmap is stored using a one-level structure for the mapping to host -clusters - an L1 table. +The dirty bitmap described by this feature extension is stored in a set of +clusters inside the Parallels image file. The offsets of these clusters are +saved in the L1 offset table specified by the feature extension. Each L1 table +entry is a 64 bit integer as described below: -Given an offset in bytes into the bitmap data, the offset in bytes into the -image file can be obtained as follows: +Given an offset in bytes into the bitmap data, corresponding L1 entry is - offset = l1_table[offset / cluster_size] + (offset % cluster_size) + l1_table[offset / cluster_size] -If an L1 table entry is 0, the corresponding cluster of the bitmap is assumed -to be zero. +If an L1 table entry is 0, all bits in the corresponding cluster of the bitmap +are assumed to be 0. -If an L1 table entry is 1, the corresponding cluster of the bitmap is assumed -to have all bits set. +If an L1 table entry is 1, all bits in the corresponding cluster of the bitmap +are assumed to be 1. -If an L1 table entry is not 0 or 1, it allocates a cluster from the data area. +If an L1 table entry is not 0 or 1, it contains the corresponding cluster +offset (in 512b sectors). Given an offset in bytes into the bitmap data the +offset in bytes into the image file can be obtained as follows: + + offset = l1_table[offset / cluster_size] * 512 + (offset % cluster_size) From e0b5207f54e45ccb7c733e736add47f7b06c5867 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Wed, 24 Feb 2021 13:47:04 +0300 Subject: [PATCH 23/30] block/parallels: BDRVParallelsState: add cluster_size field We are going to use it in more places, calculating "s->tracks << BDRV_SECTOR_BITS" doesn't look good. Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20210224104707.88430-4-vsementsov@virtuozzo.com> Reviewed-by: Denis V. Lunev Signed-off-by: Kevin Wolf --- block/parallels.c | 8 ++++---- block/parallels.h | 1 + 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/block/parallels.c b/block/parallels.c index 3c22dfdc9d..9594d84978 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -421,7 +421,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs, int ret; uint32_t i; bool flush_bat = false; - int cluster_size = s->tracks << BDRV_SECTOR_BITS; size = bdrv_getlength(bs->file->bs); if (size < 0) { @@ -472,7 +471,7 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs, high_off = off; } - if (prev_off != 0 && (prev_off + cluster_size) != off) { + if (prev_off != 0 && (prev_off + s->cluster_size) != off) { res->bfi.fragmented_clusters++; } prev_off = off; @@ -487,10 +486,10 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs, } } - res->image_end_offset = high_off + cluster_size; + res->image_end_offset = high_off + s->cluster_size; if (size > res->image_end_offset) { int64_t count; - count = DIV_ROUND_UP(size - res->image_end_offset, cluster_size); + count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size); fprintf(stderr, "%s space leaked at the end of the image %" PRId64 "\n", fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", size - res->image_end_offset); @@ -771,6 +770,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, ret = -EFBIG; goto fail; } + s->cluster_size = s->tracks << BDRV_SECTOR_BITS; s->bat_size = le32_to_cpu(ph.bat_entries); if (s->bat_size > INT_MAX / sizeof(uint32_t)) { diff --git a/block/parallels.h b/block/parallels.h index 5aa101cfc8..9a9209e320 100644 --- a/block/parallels.h +++ b/block/parallels.h @@ -79,6 +79,7 @@ typedef struct BDRVParallelsState { ParallelsPreallocMode prealloc_mode; unsigned int tracks; + unsigned int cluster_size; unsigned int off_multiplier; Error *migration_blocker; From baefd977002e72402f2cc42b11f2cb11b96aae9e Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Wed, 24 Feb 2021 13:47:05 +0300 Subject: [PATCH 24/30] parallels: support bitmap extension for read-only mode Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20210224104707.88430-5-vsementsov@virtuozzo.com> Reviewed-by: Denis V. Lunev Signed-off-by: Kevin Wolf --- block/meson.build | 3 +- block/parallels-ext.c | 300 ++++++++++++++++++++++++++++++++++++++++++ block/parallels.c | 18 +++ block/parallels.h | 6 +- 4 files changed, 325 insertions(+), 2 deletions(-) create mode 100644 block/parallels-ext.c diff --git a/block/meson.build b/block/meson.build index eeaefe5809..d21990ec95 100644 --- a/block/meson.build +++ b/block/meson.build @@ -57,7 +57,8 @@ block_ss.add(when: 'CONFIG_QED', if_true: files( 'qed-table.c', 'qed.c', )) -block_ss.add(when: [libxml2, 'CONFIG_PARALLELS'], if_true: files('parallels.c')) +block_ss.add(when: [libxml2, 'CONFIG_PARALLELS'], + if_true: files('parallels.c', 'parallels-ext.c')) block_ss.add(when: 'CONFIG_WIN32', if_true: files('file-win32.c', 'win32-aio.c')) block_ss.add(when: 'CONFIG_POSIX', if_true: [files('file-posix.c'), coref, iokit]) block_ss.add(when: libiscsi, if_true: files('iscsi-opts.c')) diff --git a/block/parallels-ext.c b/block/parallels-ext.c new file mode 100644 index 0000000000..e0dd0975c6 --- /dev/null +++ b/block/parallels-ext.c @@ -0,0 +1,300 @@ +/* + * Support of Parallels Format Extension. It's a part of Parallels format + * driver. + * + * Copyright (c) 2021 Virtuozzo International GmbH + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +#include "qemu/osdep.h" +#include "qapi/error.h" +#include "block/block_int.h" +#include "parallels.h" +#include "crypto/hash.h" +#include "qemu/uuid.h" + +#define PARALLELS_FORMAT_EXTENSION_MAGIC 0xAB234CEF23DCEA87ULL + +#define PARALLELS_END_OF_FEATURES_MAGIC 0x0ULL +#define PARALLELS_DIRTY_BITMAP_FEATURE_MAGIC 0x20385FAE252CB34AULL + +typedef struct ParallelsFormatExtensionHeader { + uint64_t magic; /* PARALLELS_FORMAT_EXTENSION_MAGIC */ + uint8_t check_sum[16]; +} QEMU_PACKED ParallelsFormatExtensionHeader; + +typedef struct ParallelsFeatureHeader { + uint64_t magic; + uint64_t flags; + uint32_t data_size; + uint32_t _unused; +} QEMU_PACKED ParallelsFeatureHeader; + +typedef struct ParallelsDirtyBitmapFeature { + uint64_t size; + uint8_t id[16]; + uint32_t granularity; + uint32_t l1_size; + /* L1 table follows */ +} QEMU_PACKED ParallelsDirtyBitmapFeature; + +/* Given L1 table read bitmap data from the image and populate @bitmap */ +static int parallels_load_bitmap_data(BlockDriverState *bs, + const uint64_t *l1_table, + uint32_t l1_size, + BdrvDirtyBitmap *bitmap, + Error **errp) +{ + BDRVParallelsState *s = bs->opaque; + int ret = 0; + uint64_t offset, limit; + uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap); + uint8_t *buf = NULL; + uint64_t i, tab_size = + DIV_ROUND_UP(bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size), + s->cluster_size); + + if (tab_size != l1_size) { + error_setg(errp, "Bitmap table size %" PRIu32 " does not correspond " + "to bitmap size and cluster size. Expected %" PRIu64, + l1_size, tab_size); + return -EINVAL; + } + + buf = qemu_blockalign(bs, s->cluster_size); + limit = bdrv_dirty_bitmap_serialization_coverage(s->cluster_size, bitmap); + for (i = 0, offset = 0; i < tab_size; ++i, offset += limit) { + uint64_t count = MIN(bm_size - offset, limit); + uint64_t entry = l1_table[i]; + + if (entry == 0) { + /* No need to deserialize zeros because @bitmap is cleared. */ + continue; + } + + if (entry == 1) { + bdrv_dirty_bitmap_deserialize_ones(bitmap, offset, count, false); + } else { + ret = bdrv_pread(bs->file, entry << BDRV_SECTOR_BITS, buf, + s->cluster_size); + if (ret < 0) { + error_setg_errno(errp, -ret, + "Failed to read bitmap data cluster"); + goto finish; + } + bdrv_dirty_bitmap_deserialize_part(bitmap, buf, offset, count, + false); + } + } + ret = 0; + + bdrv_dirty_bitmap_deserialize_finish(bitmap); + +finish: + qemu_vfree(buf); + + return ret; +} + +/* + * @data buffer (of @data_size size) is the Dirty bitmaps feature which + * consists of ParallelsDirtyBitmapFeature followed by L1 table. + */ +static BdrvDirtyBitmap *parallels_load_bitmap(BlockDriverState *bs, + uint8_t *data, + size_t data_size, + Error **errp) +{ + int ret; + ParallelsDirtyBitmapFeature bf; + g_autofree uint64_t *l1_table = NULL; + BdrvDirtyBitmap *bitmap; + QemuUUID uuid; + char uuidstr[UUID_FMT_LEN + 1]; + int i; + + if (data_size < sizeof(bf)) { + error_setg(errp, "Too small Bitmap Feature area in Parallels Format " + "Extension: %zu bytes, expected at least %zu bytes", + data_size, sizeof(bf)); + return NULL; + } + memcpy(&bf, data, sizeof(bf)); + bf.size = le64_to_cpu(bf.size); + bf.granularity = le32_to_cpu(bf.granularity) << BDRV_SECTOR_BITS; + bf.l1_size = le32_to_cpu(bf.l1_size); + data += sizeof(bf); + data_size -= sizeof(bf); + + if (bf.size != bs->total_sectors) { + error_setg(errp, "Bitmap size (in sectors) %" PRId64 " differs from " + "disk size in sectors %" PRId64, bf.size, bs->total_sectors); + return NULL; + } + + if (bf.l1_size * sizeof(uint64_t) > data_size) { + error_setg(errp, "Bitmaps feature corrupted: l1 table exceeds " + "extension data_size"); + return NULL; + } + + memcpy(&uuid, bf.id, sizeof(uuid)); + qemu_uuid_unparse(&uuid, uuidstr); + bitmap = bdrv_create_dirty_bitmap(bs, bf.granularity, uuidstr, errp); + if (!bitmap) { + return NULL; + } + + l1_table = g_new(uint64_t, bf.l1_size); + for (i = 0; i < bf.l1_size; i++, data += sizeof(uint64_t)) { + l1_table[i] = ldq_le_p(data); + } + + ret = parallels_load_bitmap_data(bs, l1_table, bf.l1_size, bitmap, errp); + if (ret < 0) { + bdrv_release_dirty_bitmap(bitmap); + return NULL; + } + + /* We support format extension only for RO parallels images. */ + assert(!(bs->open_flags & BDRV_O_RDWR)); + bdrv_dirty_bitmap_set_readonly(bitmap, true); + + return bitmap; +} + +static int parallels_parse_format_extension(BlockDriverState *bs, + uint8_t *ext_cluster, Error **errp) +{ + BDRVParallelsState *s = bs->opaque; + int ret; + int remaining = s->cluster_size; + uint8_t *pos = ext_cluster; + ParallelsFormatExtensionHeader eh; + g_autofree uint8_t *hash = NULL; + size_t hash_len = 0; + GSList *bitmaps = NULL, *el; + + memcpy(&eh, pos, sizeof(eh)); + eh.magic = le64_to_cpu(eh.magic); + pos += sizeof(eh); + remaining -= sizeof(eh); + + if (eh.magic != PARALLELS_FORMAT_EXTENSION_MAGIC) { + error_setg(errp, "Wrong parallels Format Extension magic: 0x%" PRIx64 + ", expected: 0x%llx", eh.magic, + PARALLELS_FORMAT_EXTENSION_MAGIC); + goto fail; + } + + ret = qcrypto_hash_bytes(QCRYPTO_HASH_ALG_MD5, (char *)pos, remaining, + &hash, &hash_len, errp); + if (ret < 0) { + goto fail; + } + + if (hash_len != sizeof(eh.check_sum) || + memcmp(hash, eh.check_sum, sizeof(eh.check_sum)) != 0) { + error_setg(errp, "Wrong checksum in Format Extension header. Format " + "extension is corrupted."); + goto fail; + } + + while (true) { + ParallelsFeatureHeader fh; + BdrvDirtyBitmap *bitmap; + + if (remaining < sizeof(fh)) { + error_setg(errp, "Can not read feature header, as remaining bytes " + "(%d) in Format Extension is less than Feature header " + "size (%zu)", remaining, sizeof(fh)); + goto fail; + } + + memcpy(&fh, pos, sizeof(fh)); + pos += sizeof(fh); + remaining -= sizeof(fh); + + fh.magic = le64_to_cpu(fh.magic); + fh.flags = le64_to_cpu(fh.flags); + fh.data_size = le32_to_cpu(fh.data_size); + + if (fh.flags) { + error_setg(errp, "Flags for extension feature are unsupported"); + goto fail; + } + + if (fh.data_size > remaining) { + error_setg(errp, "Feature data_size exceedes Format Extension " + "cluster"); + goto fail; + } + + switch (fh.magic) { + case PARALLELS_END_OF_FEATURES_MAGIC: + return 0; + + case PARALLELS_DIRTY_BITMAP_FEATURE_MAGIC: + bitmap = parallels_load_bitmap(bs, pos, fh.data_size, errp); + if (!bitmap) { + goto fail; + } + bitmaps = g_slist_append(bitmaps, bitmap); + break; + + default: + error_setg(errp, "Unknown feature: 0x%" PRIu64, fh.magic); + goto fail; + } + + pos = ext_cluster + QEMU_ALIGN_UP(pos + fh.data_size - ext_cluster, 8); + } + +fail: + for (el = bitmaps; el; el = el->next) { + bdrv_release_dirty_bitmap(el->data); + } + g_slist_free(bitmaps); + + return -EINVAL; +} + +int parallels_read_format_extension(BlockDriverState *bs, + int64_t ext_off, Error **errp) +{ + BDRVParallelsState *s = bs->opaque; + int ret; + uint8_t *ext_cluster = qemu_blockalign(bs, s->cluster_size); + + assert(ext_off > 0); + + ret = bdrv_pread(bs->file, ext_off, ext_cluster, s->cluster_size); + if (ret < 0) { + error_setg_errno(errp, -ret, "Failed to read Format Extension cluster"); + goto out; + } + + ret = parallels_parse_format_extension(bs, ext_cluster, errp); + +out: + qemu_vfree(ext_cluster); + + return ret; +} diff --git a/block/parallels.c b/block/parallels.c index 9594d84978..6ebad2a2bb 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -29,6 +29,7 @@ */ #include "qemu/osdep.h" +#include "qemu/error-report.h" #include "qapi/error.h" #include "block/block_int.h" #include "block/qdict.h" @@ -843,6 +844,23 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, goto fail_options; } + if (ph.ext_off) { + if (flags & BDRV_O_RDWR) { + /* + * It's unsafe to open image RW if there is an extension (as we + * don't support it). But parallels driver in QEMU historically + * ignores the extension, so print warning and don't care. + */ + warn_report("Format Extension ignored in RW mode"); + } else { + ret = parallels_read_format_extension( + bs, le64_to_cpu(ph.ext_off) << BDRV_SECTOR_BITS, errp); + if (ret < 0) { + goto fail; + } + } + } + if ((flags & BDRV_O_RDWR) && !(flags & BDRV_O_INACTIVE)) { s->header->inuse = cpu_to_le32(HEADER_INUSE_MAGIC); ret = parallels_update_header(bs); diff --git a/block/parallels.h b/block/parallels.h index 9a9209e320..f22f43f988 100644 --- a/block/parallels.h +++ b/block/parallels.h @@ -48,7 +48,8 @@ typedef struct ParallelsHeader { uint64_t nb_sectors; uint32_t inuse; uint32_t data_off; - char padding[12]; + uint32_t flags; + uint64_t ext_off; } QEMU_PACKED ParallelsHeader; typedef enum ParallelsPreallocMode { @@ -85,4 +86,7 @@ typedef struct BDRVParallelsState { Error *migration_blocker; } BDRVParallelsState; +int parallels_read_format_extension(BlockDriverState *bs, + int64_t ext_off, Error **errp); + #endif From 55b116302f26c50772fd8b73f9af13b091461ae5 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Wed, 24 Feb 2021 13:47:06 +0300 Subject: [PATCH 25/30] iotests.py: add unarchive_sample_image() helper Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20210224104707.88430-6-vsementsov@virtuozzo.com> Reviewed-by: Denis V. Lunev Signed-off-by: Kevin Wolf --- tests/qemu-iotests/iotests.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index 4e758308f2..90d0b62523 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -17,6 +17,7 @@ # import atexit +import bz2 from collections import OrderedDict import faulthandler import io @@ -24,6 +25,7 @@ import json import logging import os import re +import shutil import signal import struct import subprocess @@ -96,6 +98,14 @@ luks_default_secret_object = 'secret,id=keysec0,data=' + \ os.environ.get('IMGKEYSECRET', '') luks_default_key_secret_opt = 'key-secret=keysec0' +sample_img_dir = os.environ['SAMPLE_IMG_DIR'] + + +def unarchive_sample_image(sample, fname): + sample_fname = os.path.join(sample_img_dir, sample + '.bz2') + with bz2.open(sample_fname) as f_in, open(fname, 'wb') as f_out: + shutil.copyfileobj(f_in, f_out) + def qemu_tool_pipe_and_status(tool: str, args: Sequence[str], connect_stderr: bool = True) -> Tuple[str, int]: From c203c3b813be4a012b45cc6e33a2c18512071b1c Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Wed, 24 Feb 2021 13:47:07 +0300 Subject: [PATCH 26/30] iotests: add parallels-read-bitmap test Test support for reading bitmap from parallels image format. parallels-with-bitmap.bz2 is generated on Virtuozzo by parallels-with-bitmap.sh Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20210224104707.88430-7-vsementsov@virtuozzo.com> Reviewed-by: Denis V. Lunev Signed-off-by: Kevin Wolf --- .../sample_images/parallels-with-bitmap.bz2 | Bin 0 -> 203 bytes .../sample_images/parallels-with-bitmap.sh | 51 ++++++++++++++++ .../qemu-iotests/tests/parallels-read-bitmap | 55 ++++++++++++++++++ .../tests/parallels-read-bitmap.out | 6 ++ 4 files changed, 112 insertions(+) create mode 100644 tests/qemu-iotests/sample_images/parallels-with-bitmap.bz2 create mode 100755 tests/qemu-iotests/sample_images/parallels-with-bitmap.sh create mode 100755 tests/qemu-iotests/tests/parallels-read-bitmap create mode 100644 tests/qemu-iotests/tests/parallels-read-bitmap.out diff --git a/tests/qemu-iotests/sample_images/parallels-with-bitmap.bz2 b/tests/qemu-iotests/sample_images/parallels-with-bitmap.bz2 new file mode 100644 index 0000000000000000000000000000000000000000..54892fd4d01bf743d395bd4f3d896494146ab5a9 GIT binary patch literal 203 zcmV;+05tzXT4*^jL0KkKS@=;0bpT+Hf7|^?KmNPH-R Fx`3oHQ9u9y literal 0 HcmV?d00001 diff --git a/tests/qemu-iotests/sample_images/parallels-with-bitmap.sh b/tests/qemu-iotests/sample_images/parallels-with-bitmap.sh new file mode 100755 index 0000000000..30615aa6bd --- /dev/null +++ b/tests/qemu-iotests/sample_images/parallels-with-bitmap.sh @@ -0,0 +1,51 @@ +#!/bin/bash +# +# Test parallels load bitmap +# +# Copyright (c) 2021 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 . +# + +CT=parallels-with-bitmap-ct +DIR=$PWD/parallels-with-bitmap-dir +IMG=$DIR/root.hds +XML=$DIR/DiskDescriptor.xml +TARGET=parallels-with-bitmap.bz2 + +rm -rf $DIR + +prlctl create $CT --vmtype ct +prlctl set $CT --device-add hdd --image $DIR --recreate --size 2G + +# cleanup the image +qemu-img create -f parallels $IMG 64G + +# create bitmap +prlctl backup $CT + +prlctl set $CT --device-del hdd1 +prlctl destroy $CT + +dev=$(ploop mount $XML | sed -n 's/^Adding delta dev=\(\/dev\/ploop[0-9]\+\).*/\1/p') +dd if=/dev/zero of=$dev bs=64K seek=5 count=2 oflag=direct +dd if=/dev/zero of=$dev bs=64K seek=30 count=1 oflag=direct +dd if=/dev/zero of=$dev bs=64K seek=10 count=3 oflag=direct +ploop umount $XML # bitmap name will be in the output + +bzip2 -z $IMG + +mv $IMG.bz2 $TARGET + +rm -rf $DIR diff --git a/tests/qemu-iotests/tests/parallels-read-bitmap b/tests/qemu-iotests/tests/parallels-read-bitmap new file mode 100755 index 0000000000..af6b9c5db3 --- /dev/null +++ b/tests/qemu-iotests/tests/parallels-read-bitmap @@ -0,0 +1,55 @@ +#!/usr/bin/env python3 +# +# Test parallels load bitmap +# +# Copyright (c) 2021 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 . +# + +import json +import iotests +from iotests import qemu_nbd_popen, qemu_img_pipe, log, file_path + +iotests.script_initialize(supported_fmts=['parallels']) + +nbd_sock = file_path('nbd-sock', base_dir=iotests.sock_dir) +disk = iotests.file_path('disk') +bitmap = 'e4f2eed0-37fe-4539-b50b-85d2e7fd235f' +nbd_opts = f'driver=nbd,server.type=unix,server.path={nbd_sock}' \ + f',x-dirty-bitmap=qemu:dirty-bitmap:{bitmap}' + + +iotests.unarchive_sample_image('parallels-with-bitmap', disk) + + +with qemu_nbd_popen('--read-only', f'--socket={nbd_sock}', + f'--bitmap={bitmap}', '-f', iotests.imgfmt, disk): + out = qemu_img_pipe('map', '--output=json', '--image-opts', nbd_opts) + chunks = json.loads(out) + cluster = 64 * 1024 + + log('dirty clusters (cluster size is 64K):') + for c in chunks: + assert c['start'] % cluster == 0 + assert c['length'] % cluster == 0 + if c['data']: + continue + + a = c['start'] // cluster + b = (c['start'] + c['length']) // cluster + if b - a > 1: + log(f'{a}-{b-1}') + else: + log(a) diff --git a/tests/qemu-iotests/tests/parallels-read-bitmap.out b/tests/qemu-iotests/tests/parallels-read-bitmap.out new file mode 100644 index 0000000000..e8f6bc9e96 --- /dev/null +++ b/tests/qemu-iotests/tests/parallels-read-bitmap.out @@ -0,0 +1,6 @@ +Start NBD server +dirty clusters (cluster size is 64K): +5-6 +10-12 +30 +Kill NBD server From a960c4b484a3d2cce892f870dba749132402a2f7 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Thu, 4 Mar 2021 12:51:51 +0300 Subject: [PATCH 27/30] MAINTAINERS: update parallels block driver Add new parallels-ext.c and myself as co-maintainer. Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20210304095151.19358-1-vsementsov@virtuozzo.com> Reviewed-by: Denis V. Lunev Signed-off-by: Kevin Wolf --- MAINTAINERS | 3 +++ 1 file changed, 3 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 26c9454823..7da2d01671 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3126,10 +3126,13 @@ F: block/dmg.c parallels M: Stefan Hajnoczi M: Denis V. Lunev +M: Vladimir Sementsov-Ogievskiy L: qemu-block@nongnu.org S: Supported F: block/parallels.c +F: block/parallels-ext.c F: docs/interop/parallels.txt +T: git https://src.openvz.org/scm/~vsementsov/qemu.git parallels qed M: Stefan Hajnoczi From ef809f709de81aef01bbb7403b87cbe2ac7e0c10 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 5 Mar 2021 10:48:56 +0100 Subject: [PATCH 28/30] docs: qsd: Explain --export nbd,name=... default The 'name' option for NBD exports is optional. Add a note that the default for the option is the node name (people could otherwise expect that it's the empty string like for qemu-nbd). Signed-off-by: Kevin Wolf Message-Id: <20210305094856.18964-1-kwolf@redhat.com> Reviewed-by: Max Reitz Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- docs/tools/qemu-storage-daemon.rst | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/tools/qemu-storage-daemon.rst b/docs/tools/qemu-storage-daemon.rst index fe3042d609..086493ebb3 100644 --- a/docs/tools/qemu-storage-daemon.rst +++ b/docs/tools/qemu-storage-daemon.rst @@ -80,8 +80,9 @@ Standard options: requests for modifying data (the default is off). The ``nbd`` export type requires ``--nbd-server`` (see below). ``name`` is - the NBD export name. ``bitmap`` is the name of a dirty bitmap reachable from - the block node, so the NBD client can use NBD_OPT_SET_META_CONTEXT with the + the NBD export name (if not specified, it defaults to the given + ``node-name``). ``bitmap`` is the name of a dirty bitmap reachable from the + block node, so the NBD client can use NBD_OPT_SET_META_CONTEXT with the metadata context name "qemu:dirty-bitmap:BITMAP" to inspect the bitmap. The ``vhost-user-blk`` export type takes a vhost-user socket address on which From 785ec4b1b968906ea1d22f753a3b199be946550b Mon Sep 17 00:00:00 2001 From: Connor Kuehl Date: Fri, 5 Mar 2021 09:19:28 -0600 Subject: [PATCH 29/30] block: Clarify error messages pertaining to 'node-name' Some error messages contain ambiguous representations of the 'node-name' parameter. This can be particularly confusing when exchanging QMP messages (C = client, S = server): C: {"execute": "block_resize", "arguments": { "device": "my_file", "size": 26843545600 }} S: {"error": {"class": "GenericError", "desc": "Cannot find device=my_file nor node_name="}} ^^^^^^^^^ This error message suggests one could send a message with a key called 'node_name': C: {"execute": "block_resize", "arguments": { "node_name": "my_file", "size": 26843545600 }} ^^^^^^^^^ but using the underscore is actually incorrect, the parameter should be 'node-name': S: {"error": {"class": "GenericError", "desc": "Parameter 'node_name' is unexpected"}} This behavior was uncovered in bz1651437, but I ended up going down a rabbit hole looking for other areas where this miscommunication might occur and changing those accordingly as well. Fixes: https://bugzilla.redhat.com/1651437 Signed-off-by: Connor Kuehl Message-Id: <20210305151929.1947331-2-ckuehl@redhat.com> Signed-off-by: Kevin Wolf --- block.c | 8 ++++---- tests/qemu-iotests/030 | 4 ++-- tests/qemu-iotests/040 | 4 ++-- tests/qemu-iotests/051.pc.out | 6 +++--- tests/qemu-iotests/081.out | 2 +- tests/qemu-iotests/085.out | 6 +++--- tests/qemu-iotests/087.out | 2 +- tests/qemu-iotests/206.out | 2 +- tests/qemu-iotests/210.out | 2 +- tests/qemu-iotests/211.out | 2 +- tests/qemu-iotests/212.out | 2 +- tests/qemu-iotests/213.out | 2 +- tests/qemu-iotests/223.out | 4 ++-- tests/qemu-iotests/237.out | 2 +- tests/qemu-iotests/245 | 4 ++-- tests/qemu-iotests/249.out | 2 +- tests/qemu-iotests/283.out | 2 +- tests/qemu-iotests/300 | 4 ++-- 18 files changed, 30 insertions(+), 30 deletions(-) diff --git a/block.c b/block.c index a1f3cecd75..2daff6d29a 100644 --- a/block.c +++ b/block.c @@ -1440,7 +1440,7 @@ static void bdrv_assign_node_name(BlockDriverState *bs, * Check for empty string or invalid characters, but not if it is * generated (generated names use characters not available to the user) */ - error_setg(errp, "Invalid node name"); + error_setg(errp, "Invalid node-name: '%s'", node_name); return; } @@ -1453,7 +1453,7 @@ static void bdrv_assign_node_name(BlockDriverState *bs, /* takes care of avoiding duplicates node names */ if (bdrv_find_node(node_name)) { - error_setg(errp, "Duplicate node name"); + error_setg(errp, "Duplicate nodes with node-name='%s'", node_name); goto out; } @@ -5432,7 +5432,7 @@ BlockDriverState *bdrv_lookup_bs(const char *device, } } - error_setg(errp, "Cannot find device=%s nor node_name=%s", + error_setg(errp, "Cannot find device=\'%s\' nor node-name=\'%s\'", device ? device : "", node_name ? node_name : ""); return NULL; @@ -6752,7 +6752,7 @@ BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs, AioContext *aio_context; if (!to_replace_bs) { - error_setg(errp, "Node name '%s' not found", node_name); + error_setg(errp, "Failed to find node with node-name='%s'", node_name); return NULL; } diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030 index 12aa9ed37e..5fb65b4bef 100755 --- a/tests/qemu-iotests/030 +++ b/tests/qemu-iotests/030 @@ -153,7 +153,7 @@ class TestSingleDrive(iotests.QMPTestCase): def test_device_not_found(self): result = self.vm.qmp('block-stream', device='nonexistent') self.assert_qmp(result, 'error/desc', - 'Cannot find device=nonexistent nor node_name=nonexistent') + 'Cannot find device=\'nonexistent\' nor node-name=\'nonexistent\'') def test_job_id_missing(self): result = self.vm.qmp('block-stream', device='mid') @@ -507,7 +507,7 @@ class TestParallelOps(iotests.QMPTestCase): # Error: the base node does not exist result = self.vm.qmp('block-stream', device='node4', base_node='none', job_id='stream') self.assert_qmp(result, 'error/desc', - 'Cannot find device= nor node_name=none') + 'Cannot find device=\'\' nor node-name=\'none\'') # Error: the base node is not a backing file of the top node result = self.vm.qmp('block-stream', device='node4', base_node='node6', job_id='stream') diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040 index 7ebc9ed825..336ff7c4f2 100755 --- a/tests/qemu-iotests/040 +++ b/tests/qemu-iotests/040 @@ -175,13 +175,13 @@ class TestSingleDrive(ImageCommitTestCase): self.assert_no_active_block_jobs() result = self.vm.qmp('block-commit', device='drive0', top_node='badfile', base_node='base') self.assert_qmp(result, 'error/class', 'GenericError') - self.assert_qmp(result, 'error/desc', "Cannot find device= nor node_name=badfile") + self.assert_qmp(result, 'error/desc', "Cannot find device='' nor node-name='badfile'") def test_base_node_invalid(self): self.assert_no_active_block_jobs() result = self.vm.qmp('block-commit', device='drive0', top_node='mid', base_node='badfile') self.assert_qmp(result, 'error/class', 'GenericError') - self.assert_qmp(result, 'error/desc', "Cannot find device= nor node_name=badfile") + self.assert_qmp(result, 'error/desc', "Cannot find device='' nor node-name='badfile'") def test_top_path_and_node(self): self.assert_no_active_block_jobs() diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out index f707471fb0..f570610f64 100644 --- a/tests/qemu-iotests/051.pc.out +++ b/tests/qemu-iotests/051.pc.out @@ -61,13 +61,13 @@ QEMU X.Y.Z monitor - type 'help' for more information (qemu) quit Testing: -drive file=TEST_DIR/t.qcow2,node-name=123foo -QEMU_PROG: -drive file=TEST_DIR/t.qcow2,node-name=123foo: Invalid node name +QEMU_PROG: -drive file=TEST_DIR/t.qcow2,node-name=123foo: Invalid node-name: '123foo' Testing: -drive file=TEST_DIR/t.qcow2,node-name=_foo -QEMU_PROG: -drive file=TEST_DIR/t.qcow2,node-name=_foo: Invalid node name +QEMU_PROG: -drive file=TEST_DIR/t.qcow2,node-name=_foo: Invalid node-name: '_foo' Testing: -drive file=TEST_DIR/t.qcow2,node-name=foo#12 -QEMU_PROG: -drive file=TEST_DIR/t.qcow2,node-name=foo#12: Invalid node name +QEMU_PROG: -drive file=TEST_DIR/t.qcow2,node-name=foo#12: Invalid node-name: 'foo#12' === Device without drive === diff --git a/tests/qemu-iotests/081.out b/tests/qemu-iotests/081.out index 1974262fac..615c083549 100644 --- a/tests/qemu-iotests/081.out +++ b/tests/qemu-iotests/081.out @@ -140,7 +140,7 @@ Testing: QMP_VERSION {"return": {}} {"error": {"class": "GenericError", "desc": "blkverify=on can only be set if there are exactly two files and vote-threshold is 2"}} -{"error": {"class": "GenericError", "desc": "Cannot find device=drive0-quorum nor node_name=drive0-quorum"}} +{"error": {"class": "GenericError", "desc": "Cannot find device='drive0-quorum' nor node-name='drive0-quorum'"}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}} diff --git a/tests/qemu-iotests/085.out b/tests/qemu-iotests/085.out index 32a193f2c2..1d4c565b6d 100644 --- a/tests/qemu-iotests/085.out +++ b/tests/qemu-iotests/085.out @@ -24,7 +24,7 @@ Formatting 'TEST_DIR/1-snapshot-v0.qcow2', fmt=qcow2 cluster_size=65536 extended { 'execute': 'blockdev-snapshot-sync', 'arguments': { 'snapshot-file':'TEST_DIR/1-snapshot-v0.IMGFMT', 'format': 'IMGFMT' } } -{"error": {"class": "GenericError", "desc": "Cannot find device= nor node_name="}} +{"error": {"class": "GenericError", "desc": "Cannot find device='' nor node-name=''"}} === Invalid command - missing snapshot-file === @@ -222,10 +222,10 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/ { 'execute': 'blockdev-snapshot', 'arguments': { 'node': 'virtio0', 'overlay':'snap_14' } } -{"error": {"class": "GenericError", "desc": "Cannot find device=snap_14 nor node_name=snap_14"}} +{"error": {"class": "GenericError", "desc": "Cannot find device='snap_14' nor node-name='snap_14'"}} { 'execute': 'blockdev-snapshot', 'arguments': { 'node':'nodevice', 'overlay':'snap_13' } } -{"error": {"class": "GenericError", "desc": "Cannot find device=nodevice nor node_name=nodevice"}} +{"error": {"class": "GenericError", "desc": "Cannot find device='nodevice' nor node-name='nodevice'"}} *** done diff --git a/tests/qemu-iotests/087.out b/tests/qemu-iotests/087.out index b61ba638af..e1c23a6983 100644 --- a/tests/qemu-iotests/087.out +++ b/tests/qemu-iotests/087.out @@ -17,7 +17,7 @@ Testing: -drive driver=IMGFMT,id=disk,node-name=test-node,file=TEST_DIR/t.IMGFMT QMP_VERSION {"return": {}} {"error": {"class": "GenericError", "desc": "node-name=disk is conflicting with a device id"}} -{"error": {"class": "GenericError", "desc": "Duplicate node name"}} +{"error": {"class": "GenericError", "desc": "Duplicate nodes with node-name='test-node'"}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}} diff --git a/tests/qemu-iotests/206.out b/tests/qemu-iotests/206.out index 5dd589d14e..b68c443867 100644 --- a/tests/qemu-iotests/206.out +++ b/tests/qemu-iotests/206.out @@ -155,7 +155,7 @@ Format specific information: {"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "qcow2", "file": "this doesn't exist", "size": 33554432}}} {"return": {}} -Job failed: Cannot find device=this doesn't exist nor node_name=this doesn't exist +Job failed: Cannot find device='this doesn't exist' nor node-name='this doesn't exist' {"execute": "job-dismiss", "arguments": {"id": "job0"}} {"return": {}} diff --git a/tests/qemu-iotests/210.out b/tests/qemu-iotests/210.out index 2e9fc596eb..55c0844370 100644 --- a/tests/qemu-iotests/210.out +++ b/tests/qemu-iotests/210.out @@ -108,7 +108,7 @@ Format specific information: {"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "luks", "file": "this doesn't exist", "size": 67108864}}} {"return": {}} -Job failed: Cannot find device=this doesn't exist nor node_name=this doesn't exist +Job failed: Cannot find device='this doesn't exist' nor node-name='this doesn't exist' {"execute": "job-dismiss", "arguments": {"id": "job0"}} {"return": {}} diff --git a/tests/qemu-iotests/211.out b/tests/qemu-iotests/211.out index b83384deea..3bc092a8a8 100644 --- a/tests/qemu-iotests/211.out +++ b/tests/qemu-iotests/211.out @@ -62,7 +62,7 @@ cluster_size: 1048576 {"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "vdi", "file": "this doesn't exist", "size": 33554432}}} {"return": {}} -Job failed: Cannot find device=this doesn't exist nor node_name=this doesn't exist +Job failed: Cannot find device='this doesn't exist' nor node-name='this doesn't exist' {"execute": "job-dismiss", "arguments": {"id": "job0"}} {"return": {}} diff --git a/tests/qemu-iotests/212.out b/tests/qemu-iotests/212.out index 1538d679be..8102033488 100644 --- a/tests/qemu-iotests/212.out +++ b/tests/qemu-iotests/212.out @@ -52,7 +52,7 @@ virtual size: 32 MiB (33554432 bytes) {"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "parallels", "file": "this doesn't exist", "size": 33554432}}} {"return": {}} -Job failed: Cannot find device=this doesn't exist nor node_name=this doesn't exist +Job failed: Cannot find device='this doesn't exist' nor node-name='this doesn't exist' {"execute": "job-dismiss", "arguments": {"id": "job0"}} {"return": {}} diff --git a/tests/qemu-iotests/213.out b/tests/qemu-iotests/213.out index be4ae85180..3cdce4d790 100644 --- a/tests/qemu-iotests/213.out +++ b/tests/qemu-iotests/213.out @@ -55,7 +55,7 @@ cluster_size: 268435456 {"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "vhdx", "file": "this doesn't exist", "size": 33554432}}} {"return": {}} -Job failed: Cannot find device=this doesn't exist nor node_name=this doesn't exist +Job failed: Cannot find device='this doesn't exist' nor node-name='this doesn't exist' {"execute": "job-dismiss", "arguments": {"id": "job0"}} {"return": {}} diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out index bbc85289e3..083b62d053 100644 --- a/tests/qemu-iotests/223.out +++ b/tests/qemu-iotests/223.out @@ -53,7 +53,7 @@ exports available: 0 {"return": {}} {"execute":"nbd-server-add", "arguments":{"device":"nosuch"}} -{"error": {"class": "GenericError", "desc": "Cannot find device=nosuch nor node_name=nosuch"}} +{"error": {"class": "GenericError", "desc": "Cannot find device='nosuch' nor node-name='nosuch'"}} {"execute":"nbd-server-add", "arguments":{"device":"n"}} {"error": {"class": "GenericError", "desc": "Block export id 'n' is already in use"}} @@ -154,7 +154,7 @@ exports available: 0 {"return": {}} {"execute":"nbd-server-add", "arguments":{"device":"nosuch"}} -{"error": {"class": "GenericError", "desc": "Cannot find device=nosuch nor node_name=nosuch"}} +{"error": {"class": "GenericError", "desc": "Cannot find device='nosuch' nor node-name='nosuch'"}} {"execute":"nbd-server-add", "arguments":{"device":"n"}} {"error": {"class": "GenericError", "desc": "Block export id 'n' is already in use"}} diff --git a/tests/qemu-iotests/237.out b/tests/qemu-iotests/237.out index a8c800bfad..aa94986803 100644 --- a/tests/qemu-iotests/237.out +++ b/tests/qemu-iotests/237.out @@ -85,7 +85,7 @@ Format specific information: {"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "vmdk", "file": "this doesn't exist", "size": 33554432}}} {"return": {}} -Job failed: Cannot find device=this doesn't exist nor node_name=this doesn't exist +Job failed: Cannot find device='this doesn't exist' nor node-name='this doesn't exist' {"execute": "job-dismiss", "arguments": {"id": "job0"}} {"return": {}} diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245 index 30b1d7b22d..a6a866c681 100755 --- a/tests/qemu-iotests/245 +++ b/tests/qemu-iotests/245 @@ -187,8 +187,8 @@ class TestBlockdevReopen(iotests.QMPTestCase): self.reopen(opts, {'backing': backing_node_name}) # We can't use a non-existing or empty (non-NULL) node as the backing image - self.reopen(opts, {'backing': 'not-found'}, "Cannot find device= nor node_name=not-found") - self.reopen(opts, {'backing': ''}, "Cannot find device= nor node_name=") + self.reopen(opts, {'backing': 'not-found'}, "Cannot find device=\'\' nor node-name=\'not-found\'") + self.reopen(opts, {'backing': ''}, "Cannot find device=\'\' nor node-name=\'\'") # We can reopen the image just fine if we specify the backing options opts['backing'] = {'driver': iotests.imgfmt, diff --git a/tests/qemu-iotests/249.out b/tests/qemu-iotests/249.out index 92ec81db03..d2bf9be85e 100644 --- a/tests/qemu-iotests/249.out +++ b/tests/qemu-iotests/249.out @@ -18,7 +18,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t. 'filter-node-name': '1234'}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "job0"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "job0"}} -{"error": {"class": "GenericError", "desc": "Invalid node name"}} +{"error": {"class": "GenericError", "desc": "Invalid node-name: '1234'"}} === Send a write command to a drive opened in read-only mode (2) diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out index 7e9cd9a7d4..37c35058ae 100644 --- a/tests/qemu-iotests/283.out +++ b/tests/qemu-iotests/283.out @@ -18,6 +18,6 @@ {"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"} +{"return": "Error: Cannot find device='' nor node-name='backup-filter'\r\n"} {"execute": "job-dismiss", "arguments": {"id": "backup"}} {"return": {}} diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300 index adb9276297..b475a92c47 100755 --- a/tests/qemu-iotests/300 +++ b/tests/qemu-iotests/300 @@ -189,8 +189,8 @@ class TestAliasMigration(TestDirtyBitmapMigration): # Check for error message on the destination if self.src_node_name != self.dst_node_name: self.verify_dest_error(f"Cannot find " - f"device={self.src_node_name} nor " - f"node_name={self.src_node_name}") + f"device='{self.src_node_name}' nor " + f"node-name='{self.src_node_name}'") else: self.verify_dest_error(None) From ef2e38a1a1d2915b148c4a49f61626e62c46fbb6 Mon Sep 17 00:00:00 2001 From: Connor Kuehl Date: Fri, 5 Mar 2021 09:19:29 -0600 Subject: [PATCH 30/30] blockdev: Clarify error messages pertaining to 'node-name' Signed-off-by: Connor Kuehl Message-Id: <20210305151929.1947331-3-ckuehl@redhat.com> Signed-off-by: Kevin Wolf --- blockdev.c | 13 +++++++------ tests/qemu-iotests/245 | 6 +++--- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/blockdev.c b/blockdev.c index cd438e60e3..7c7ab2b386 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1515,13 +1515,13 @@ static void external_snapshot_prepare(BlkActionState *common, s->has_snapshot_node_name ? s->snapshot_node_name : NULL; if (node_name && !snapshot_node_name) { - error_setg(errp, "New overlay node name missing"); + error_setg(errp, "New overlay node-name missing"); goto out; } if (snapshot_node_name && bdrv_lookup_bs(snapshot_node_name, snapshot_node_name, NULL)) { - error_setg(errp, "New overlay node name already in use"); + error_setg(errp, "New overlay node-name already in use"); goto out; } @@ -3598,13 +3598,14 @@ void qmp_x_blockdev_reopen(BlockdevOptions *options, Error **errp) /* Check for the selected node name */ if (!options->has_node_name) { - error_setg(errp, "Node name not specified"); + error_setg(errp, "node-name not specified"); goto fail; } bs = bdrv_find_node(options->node_name); if (!bs) { - error_setg(errp, "Cannot find node named '%s'", options->node_name); + error_setg(errp, "Failed to find node with node-name='%s'", + options->node_name); goto fail; } @@ -3635,7 +3636,7 @@ void qmp_blockdev_del(const char *node_name, Error **errp) bs = bdrv_find_node(node_name); if (!bs) { - error_setg(errp, "Cannot find node %s", node_name); + error_setg(errp, "Failed to find node with node-name='%s'", node_name); return; } if (bdrv_has_blk(bs)) { @@ -3758,7 +3759,7 @@ void qmp_x_blockdev_set_iothread(const char *node_name, StrOrNull *iothread, bs = bdrv_find_node(node_name); if (!bs) { - error_setg(errp, "Cannot find node %s", node_name); + error_setg(errp, "Failed to find node with node-name='%s'", node_name); return; } diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245 index a6a866c681..11104b9208 100755 --- a/tests/qemu-iotests/245 +++ b/tests/qemu-iotests/245 @@ -140,8 +140,8 @@ class TestBlockdevReopen(iotests.QMPTestCase): self.reopen(opts, {'file': 'hd0-file'}) # We cannot change any of these - self.reopen(opts, {'node-name': 'not-found'}, "Cannot find node named 'not-found'") - self.reopen(opts, {'node-name': ''}, "Cannot find node named ''") + self.reopen(opts, {'node-name': 'not-found'}, "Failed to find node with node-name='not-found'") + self.reopen(opts, {'node-name': ''}, "Failed to find node with node-name=''") self.reopen(opts, {'node-name': None}, "Invalid parameter type for 'node-name', expected: string") self.reopen(opts, {'driver': 'raw'}, "Cannot change the option 'driver'") self.reopen(opts, {'driver': ''}, "Invalid parameter ''") @@ -158,7 +158,7 @@ class TestBlockdevReopen(iotests.QMPTestCase): # node-name is optional in BlockdevOptions, but x-blockdev-reopen needs it del opts['node-name'] - self.reopen(opts, {}, "Node name not specified") + self.reopen(opts, {}, "node-name not specified") # Check that nothing has changed self.check_node_graph(original_graph)