Re: [PATCH v6 14/14] python: use vm.cmd() instead of vm.qmp() where appropriate
On Thu, Oct 05, 2023 at 04:55:50PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> In many cases we just want an effect of qmp command and want to raise on
> failure. Use vm.cmd() method which does exactly this.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> tests/avocado/vnc.py | 16 +-
> tests/qemu-iotests/030| 168 +++---
> tests/qemu-iotests/040| 172 +++
> tests/qemu-iotests/041| 483 --
> tests/qemu-iotests/045| 15 +-
> tests/qemu-iotests/055| 62 +--
> tests/qemu-iotests/056| 77 ++-
> tests/qemu-iotests/093| 42 +-
> tests/qemu-iotests/118| 225
> tests/qemu-iotests/124| 102 ++--
> tests/qemu-iotests/129| 14 +-
> tests/qemu-iotests/132| 5 +-
> tests/qemu-iotests/139| 45 +-
> tests/qemu-iotests/147| 31 +-
> tests/qemu-iotests/151| 57 +--
> tests/qemu-iotests/152| 10 +-
> tests/qemu-iotests/155| 55 +-
> tests/qemu-iotests/165| 8 +-
> tests/qemu-iotests/196| 3 +-
> tests/qemu-iotests/205| 6 +-
> tests/qemu-iotests/218| 105 ++--
> tests/qemu-iotests/245| 245 -
> tests/qemu-iotests/264| 31 +-
> tests/qemu-iotests/281| 21 +-
> tests/qemu-iotests/295| 15 +-
> tests/qemu-iotests/296| 15 +-
> tests/qemu-iotests/298| 13 +-
> tests/qemu-iotests/300| 54 +-
> tests/qemu-iotests/iotests.py | 9 +-
> .../tests/export-incoming-iothread| 6 +-
> .../qemu-iotests/tests/graph-changes-while-io | 6 +-
> tests/qemu-iotests/tests/image-fleecing | 3 +-
> .../tests/migrate-bitmaps-postcopy-test | 31 +-
> tests/qemu-iotests/tests/migrate-bitmaps-test | 47 +-
> .../qemu-iotests/tests/migrate-during-backup | 41 +-
> .../qemu-iotests/tests/migration-permissions | 9 +-
> .../tests/mirror-ready-cancel-error | 74 ++-
> tests/qemu-iotests/tests/mirror-top-perms | 16 +-
> tests/qemu-iotests/tests/nbd-multiconn| 12 +-
> tests/qemu-iotests/tests/reopen-file | 3 +-
> .../qemu-iotests/tests/stream-error-on-reset | 6 +-
> 41 files changed, 951 insertions(+), 1407 deletions(-)
Big but mechanical. It would be worth amending the commit message to
describe how you found all these spots (in case someone backporting
this patch has to redo the work over a different subset of files based
on what has changed since the two trees diverged).
>
> diff --git a/tests/avocado/vnc.py b/tests/avocado/vnc.py
> index aeeefc70be..862c8996a8 100644
> --- a/tests/avocado/vnc.py
> +++ b/tests/avocado/vnc.py
> @@ -88,9 +88,8 @@ def test_change_password(self):
> self.vm.add_args('-nodefaults', '-S', '-vnc', ':0,password=on')
> self.vm.launch()
> self.assertTrue(self.vm.qmp('query-vnc')['return']['enabled'])
> -set_password_response = self.vm.qmp('change-vnc-password',
> -password='new_password')
> -self.assertEqual(set_password_response['return'], {})
> +self.vm.cmd('change-vnc-password',
> +password='new_password')
Indeed a nicer idiom, where you are able to use it (whether by
self.assertEqual or by self.assert_qmp).
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
Re: [PATCH v6 13/14] tests/vm/basevm.py: use cmd() instead of qmp()
On Thu, Oct 05, 2023 at 04:55:49PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> We don't expect failure here and need 'result' object. cmd() is better
> in this case.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> tests/vm/basevm.py | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
> index a97e23b0ce..8aef4cff96 100644
> --- a/tests/vm/basevm.py
> +++ b/tests/vm/basevm.py
> @@ -312,8 +312,8 @@ def boot(self, img, extra_args=[]):
> self._guest = guest
> # Init console so we can start consuming the chars.
> self.console_init()
> -usernet_info = guest.qmp("human-monitor-command",
> - command_line="info usernet").get("return")
> +usernet_info = guest.cmd("human-monitor-command",
> + command_line="info usernet")
> self.ssh_port = get_info_usernet_hostfwd_port(usernet_info)
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
Re: [PATCH v6 12/14] iotests.py: pause_job(): drop return value
On Thu, Oct 05, 2023 at 04:55:48PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> The returned value is unused. It's simple to check by command
>
> git grep -B 3 '\.pause_job('
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> tests/qemu-iotests/iotests.py | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
Re: [PATCH v6 11/14] iotests: drop some extra ** in qmp() call
On Thu, Oct 05, 2023 at 04:55:47PM +0300, Vladimir Sementsov-Ogievskiy wrote: > qmp() method supports passing dict (if it doesn't need substituting > '_' to '-' in keys). So, drop some extra '**' operators. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > tests/qemu-iotests/040| 4 +- > tests/qemu-iotests/041| 14 +++--- > tests/qemu-iotests/129| 2 +- > tests/qemu-iotests/147| 2 +- > tests/qemu-iotests/155| 2 +- > tests/qemu-iotests/264| 12 ++--- > tests/qemu-iotests/295| 5 +- > tests/qemu-iotests/296| 15 +++--- > tests/qemu-iotests/tests/migrate-bitmaps-test | 4 +- > .../tests/mirror-ready-cancel-error | 50 +-- > 10 files changed, 54 insertions(+), 56 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
Re: [PATCH v6 10/14] iotests: drop some occasional semicolons
On Thu, Oct 05, 2023 at 04:55:46PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> tests/qemu-iotests/041 | 2 +-
> tests/qemu-iotests/196 | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
Maybe in the subject s/occasional/extra/
Reviewed-by: Eric Blake
>
> diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
> index 4d7a829b65..550e4dc391 100755
> --- a/tests/qemu-iotests/041
> +++ b/tests/qemu-iotests/041
> @@ -1086,7 +1086,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
> def test_after_a_quorum_snapshot(self):
> result = self.vm.qmp('blockdev-snapshot-sync', node_name='img1',
> snapshot_file=quorum_snapshot_file,
> - snapshot_node_name="snap1");
> + snapshot_node_name="snap1")
> self.assert_qmp(result, 'return', {})
I'm a bit surprised the linters don't pick up on this.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
Re: [PATCH v6 09/14] iotests: refactor some common qmp result checks into generic pattern
On Thu, Oct 05, 2023 at 04:55:45PM +0300, Vladimir Sementsov-Ogievskiy wrote: > To simplify further conversion. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > tests/qemu-iotests/040 | 3 ++- > tests/qemu-iotests/147 | 3 ++- > tests/qemu-iotests/155 | 4 ++-- > tests/qemu-iotests/218 | 4 ++-- > tests/qemu-iotests/296 | 3 ++- > 5 files changed, 10 insertions(+), 7 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
Re: [PATCH v6 08/14] iotests: add some missed checks of qmp result
On Thu, Oct 05, 2023 at 04:55:44PM +0300, Vladimir Sementsov-Ogievskiy wrote: > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > tests/qemu-iotests/041| 1 + > tests/qemu-iotests/151| 1 + > tests/qemu-iotests/152| 2 ++ > tests/qemu-iotests/tests/migrate-bitmaps-test | 2 ++ > 4 files changed, 6 insertions(+) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
Re: [PATCH v2 03/21] preallocate: Don't poll during permission updates
On 11.09.23 12:46, Kevin Wolf wrote: When the permission related BlockDriver callbacks are called, we are in the middle of an operation traversing the block graph. Polling in such a place is a very bad idea because the graph could change in unexpected ways. In the future, callers will also hold the graph lock, which is likely to turn polling into a deadlock. So we need to get rid of calls to functions like bdrv_getlength() or bdrv_truncate() there as these functions poll internally. They are currently used so that when no parent has write/resize permissions on the image any more, the preallocate filter drops the extra preallocated area in the image file and gives up write/resize permissions itself. In order to achieve this without polling in .bdrv_check_perm, don't immediately truncate the image, but only schedule a BH to do so. The filter keeps the write/resize permissions a bit longer now until the BH has executed. There is one case in which delaying doesn't work: Reopening the image read-only. In this case, bs->file will likely be reopened read-only, too, so keeping write permissions a bit longer on it doesn't work. But we can already cover this case in preallocate_reopen_prepare() and not rely on the permission updates for it. Hmm, now I found one more "future" case. I now try to rebase my "[PATCH v7 0/7] blockdev-replace" https://patchew.org/QEMU/20230421114102.884457-1-vsement...@yandex-team.ru/ And it breaks after this commit. By accident, blockdev-replace series uses exactly "preallocate" filter to test insertion/removing of filters. And removing is broken now. Removing is done as follows: 1. We have filter inserted: disk0 -file-> filter -file-> file0 2. blockdev-replace, replaces file child of disk0, so we should get the picture*: disk0 -file-> file0 <-file- filter 3. blockdev-del filter But step [2] fails, as now preallocate filter doesn't drop permissions during the operation (postponing this for a while) and the picture* is impossible. Permission check fails. Hmmm... Any idea how blockdev-replace and preallocate filter should work :) ? Maybe, doing truncation in .drain_begin() will help? Will try Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake Reviewed-by: Stefan Hajnoczi --- -- Best regards, Vladimir
Re: [PATCH v6 07/14] iotests: QemuStorageDaemon: add cmd() method like in QEMUMachine.
On Thu, Oct 05, 2023 at 04:55:43PM +0300, Vladimir Sementsov-Ogievskiy wrote: > Add similar method for consistency. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > tests/qemu-iotests/iotests.py | 7 ++- > 1 file changed, 6 insertions(+), 1 deletion(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
Re: [PATCH v6 06/14] python/machine.py: upgrade vm.cmd() method
On Thu, Oct 05, 2023 at 04:55:42PM +0300, Vladimir Sementsov-Ogievskiy wrote: > The method is not popular in iotests, we prefer use vm.qmp() and then > check success by hand.. But that's not optimal. To simplify movement to extra '.' > vm.cmd() let's support same interface improvements like in vm.qmp(). > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > python/qemu/machine/machine.py | 12 +++- > 1 file changed, 11 insertions(+), 1 deletion(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
Re: [PATCH v6 05/14] python/qemu: rename command() to cmd()
On Thu, Oct 05, 2023 at 04:55:41PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Use a shorter name. We are going to move in iotests from qmp() to
> command() where possible. But command() is longer than qmp() and don't
> look better. Let's rename.
>
> You can simply grep for '\.command(' and for 'def command(' to check
> that everything is updated (command() in tests/docker/docker.py is
> unrelated).
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> Reviewed-by: Daniel P. Berrangé
> ---
> docs/devel/testing.rst| 10 +-
There's a risk that this will need rebasing on top of any other test
changes being made in parallel; hopefully we can get this series in
sooner rather than later.
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
Re: [PATCH v6 04/14] python: rename QEMUMonitorProtocol.cmd() to cmd_raw()
On Thu, Oct 05, 2023 at 04:55:40PM +0300, Vladimir Sementsov-Ogievskiy wrote: > Having cmd() and command() methods in one class doesn't look good. > Rename cmd() to cmd_raw(), to show its meaning better. > > We also want to rename command() to cmd() in future, so this commit is > a necessary step. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > python/qemu/machine/machine.py | 2 +- > python/qemu/qmp/legacy.py | 4 ++-- > tests/qemu-iotests/iotests.py | 2 +- > 3 files changed, 4 insertions(+), 4 deletions(-) Reviewed-by: Eric Blake I was surprised how few users there are, but agree that this opens up the door to let our common usage be the least typing. -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
Re: [PATCH v6 03/14] scripts/cpu-x86-uarch-abi.py: use .command() instead of .cmd()
On Thu, Oct 05, 2023 at 04:55:39PM +0300, Vladimir Sementsov-Ogievskiy wrote: > Here we don't expect a failure. In case on failure we'll crash on s/case on/case of/ > trying to access ['return']. Let's better use .command() that clearly > raise on failure. Maybe: s/Let's better /Better is to/; s/raise/raises/ > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > scripts/cpu-x86-uarch-abi.py | 12 ++-- > 1 file changed, 6 insertions(+), 6 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
[PULL 15/15] nbd/server: Add FLAG_PAYLOAD support to CMD_BLOCK_STATUS
Allow a client to request a subset of negotiated meta contexts. For
example, a client may ask to use a single connection to learn about
both block status and dirty bitmaps, but where the dirty bitmap
queries only need to be performed on a subset of the disk; forcing the
server to compute that information on block status queries in the rest
of the disk is wasted effort (both at the server, and on the amount of
traffic sent over the wire to be parsed and ignored by the client).
Qemu as an NBD client never requests to use more than one meta
context, so it has no need to use block status payloads. Testing this
instead requires support from libnbd, which CAN access multiple meta
contexts in parallel from a single NBD connection; an interop test
submitted to the libnbd project at the same time as this patch
demonstrates the feature working, as well as testing some corner cases
(for example, when the payload length is longer than the export
length), although other corner cases (like passing the same id
duplicated) requires a protocol fuzzer because libnbd is not wired up
to break the protocol that badly.
This also includes tweaks to 'qemu-nbd --list' to show when a server
is advertising the capability, and to the testsuite to reflect the
addition to that output.
Of note: qemu will always advertise the new feature bit during
NBD_OPT_INFO if extended headers have alreay been negotiated
(regardless of whether any NBD_OPT_SET_META_CONTEXT negotiation has
occurred); but for NBD_OPT_GO, qemu only advertises the feature if
block status is also enabled (that is, if the client does not
negotiate any contexts, then NBD_CMD_BLOCK_STATUS cannot be used, so
the feature is not advertised).
Signed-off-by: Eric Blake
Message-ID: <20230925192229.3186470-26-ebl...@redhat.com>
[eblake: fix logic to reject unnegotiated contexts]
Signed-off-by: Eric Blake
---
docs/interop/nbd.txt | 2 +-
nbd/server.c | 117 --
qemu-nbd.c| 1 +
nbd/trace-events | 1 +
tests/qemu-iotests/223.out| 12 +-
tests/qemu-iotests/307.out| 10 +-
.../tests/nbd-qemu-allocation.out | 2 +-
7 files changed, 125 insertions(+), 20 deletions(-)
diff --git a/docs/interop/nbd.txt b/docs/interop/nbd.txt
index 9aae5e1f294..18efb251de9 100644
--- a/docs/interop/nbd.txt
+++ b/docs/interop/nbd.txt
@@ -69,4 +69,4 @@ NBD_CMD_BLOCK_STATUS for "qemu:dirty-bitmap:", NBD_CMD_CACHE
NBD_CMD_FLAG_FAST_ZERO
* 5.2: NBD_CMD_BLOCK_STATUS for "qemu:allocation-depth"
* 7.1: NBD_FLAG_CAN_MULTI_CONN for shareable writable exports
-* 8.2: NBD_OPT_EXTENDED_HEADERS
+* 8.2: NBD_OPT_EXTENDED_HEADERS, NBD_FLAG_BLOCK_STATUS_PAYLOAD
diff --git a/nbd/server.c b/nbd/server.c
index 2dce9c3ad6a..859c163d19f 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -512,6 +512,9 @@ static int nbd_negotiate_handle_export_name(NBDClient
*client, bool no_zeroes,
if (client->mode >= NBD_MODE_STRUCTURED) {
myflags |= NBD_FLAG_SEND_DF;
}
+if (client->mode >= NBD_MODE_EXTENDED && client->contexts.count) {
+myflags |= NBD_FLAG_BLOCK_STAT_PAYLOAD;
+}
trace_nbd_negotiate_new_style_size_flags(client->exp->size, myflags);
stq_be_p(buf, client->exp->size);
stw_be_p(buf + 8, myflags);
@@ -699,6 +702,10 @@ static int nbd_negotiate_handle_info(NBDClient *client,
Error **errp)
if (client->mode >= NBD_MODE_STRUCTURED) {
myflags |= NBD_FLAG_SEND_DF;
}
+if (client->mode >= NBD_MODE_EXTENDED &&
+(client->contexts.count || client->opt == NBD_OPT_INFO)) {
+myflags |= NBD_FLAG_BLOCK_STAT_PAYLOAD;
+}
trace_nbd_negotiate_new_style_size_flags(exp->size, myflags);
stq_be_p(buf, exp->size);
stw_be_p(buf + 8, myflags);
@@ -2420,6 +2427,90 @@ static int coroutine_fn nbd_co_send_bitmap(NBDClient
*client,
return nbd_co_send_extents(client, request, ea, last, context_id, errp);
}
+/*
+ * nbd_co_block_status_payload_read
+ * Called when a client wants a subset of negotiated contexts via a
+ * BLOCK_STATUS payload. Check the payload for valid length and
+ * contents. On success, return 0 with request updated to effective
+ * length. If request was invalid but all payload consumed, return 0
+ * with request->len and request->contexts->count set to 0 (which will
+ * trigger an appropriate NBD_EINVAL response later on). Return
+ * negative errno if the payload was not fully consumed.
+ */
+static int
+nbd_co_block_status_payload_read(NBDClient *client, NBDRequest *request,
+ Error **errp)
+{
+uint64_t payload_len = request->len;
+g_autofree char *buf = NULL;
+size_t count, i, nr_bitmaps;
+uint32_t id;
+
+if (payload_len > NBD_MAX_BUFFER_SIZE) {
+error_setg(errp, "len (%" PRIu64 ") is larger than max len (%u)",
+ request->len,
[PULL 06/15] nbd/server: Prepare to send extended header replies
Although extended mode is not yet enabled, once we do turn it on, we
need to reply with extended headers to all messages. Update the low
level entry points necessary so that all other callers automatically
get the right header based on the current mode.
Signed-off-by: Eric Blake
Reviewed-by: Vladimir Sementsov-Ogievskiy
Message-ID: <20230925192229.3186470-17-ebl...@redhat.com>
---
nbd/server.c | 30 ++
1 file changed, 22 insertions(+), 8 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index 501749d62b5..910c48c6467 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1938,8 +1938,6 @@ static inline void set_be_chunk(NBDClient *client, struct
iovec *iov,
size_t niov, uint16_t flags, uint16_t type,
NBDRequest *request)
{
-/* TODO - handle structured vs. extended replies */
-NBDStructuredReplyChunk *chunk = iov->iov_base;
size_t i, length = 0;
for (i = 1; i < niov; i++) {
@@ -1947,12 +1945,26 @@ static inline void set_be_chunk(NBDClient *client,
struct iovec *iov,
}
assert(length <= NBD_MAX_BUFFER_SIZE + sizeof(NBDStructuredReadData));
-iov[0].iov_len = sizeof(*chunk);
-stl_be_p(>magic, NBD_STRUCTURED_REPLY_MAGIC);
-stw_be_p(>flags, flags);
-stw_be_p(>type, type);
-stq_be_p(>cookie, request->cookie);
-stl_be_p(>length, length);
+if (client->mode >= NBD_MODE_EXTENDED) {
+NBDExtendedReplyChunk *chunk = iov->iov_base;
+
+iov[0].iov_len = sizeof(*chunk);
+stl_be_p(>magic, NBD_EXTENDED_REPLY_MAGIC);
+stw_be_p(>flags, flags);
+stw_be_p(>type, type);
+stq_be_p(>cookie, request->cookie);
+stq_be_p(>offset, request->from);
+stq_be_p(>length, length);
+} else {
+NBDStructuredReplyChunk *chunk = iov->iov_base;
+
+iov[0].iov_len = sizeof(*chunk);
+stl_be_p(>magic, NBD_STRUCTURED_REPLY_MAGIC);
+stw_be_p(>flags, flags);
+stw_be_p(>type, type);
+stq_be_p(>cookie, request->cookie);
+stl_be_p(>length, length);
+}
}
static int coroutine_fn nbd_co_send_chunk_done(NBDClient *client,
@@ -2512,6 +2524,8 @@ static coroutine_fn int nbd_send_generic_reply(NBDClient
*client,
{
if (client->mode >= NBD_MODE_STRUCTURED && ret < 0) {
return nbd_co_send_chunk_error(client, request, -ret, error_msg, errp);
+} else if (client->mode >= NBD_MODE_EXTENDED) {
+return nbd_co_send_chunk_done(client, request, errp);
} else {
return nbd_co_send_simple_reply(client, request, ret < 0 ? -ret : 0,
NULL, 0, errp);
--
2.41.0
[PULL 05/15] nbd/server: Prepare to receive extended header requests
Although extended mode is not yet enabled, once we do turn it on, we
need to accept extended requests for all messages. Previous patches
have already taken care of supporting 64-bit lengths, now we just need
to read it off the wire.
Note that this implementation will block indefinitely on a buggy
client that sends a non-extended payload (that is, we try to read a
full packet before we ever check the magic number, but a client that
mistakenly sends a simple request after negotiating extended headers
doesn't send us enough bytes), but it's no different from any other
client that stops talking to us partway through a packet and thus not
worth coding around.
Signed-off-by: Eric Blake
Reviewed-by: Vladimir Sementsov-Ogievskiy
Message-ID: <20230925192229.3186470-16-ebl...@redhat.com>
---
nbd/nbd-internal.h | 5 -
nbd/server.c | 43 ++-
2 files changed, 34 insertions(+), 14 deletions(-)
diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index 133b1d94b50..dfa02f77ee4 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -34,8 +34,11 @@
* https://github.com/yoe/nbd/blob/master/doc/proto.md
*/
-/* Size of all NBD_OPT_*, without payload */
+/* Size of all compact NBD_CMD_*, without payload */
#define NBD_REQUEST_SIZE(4 + 2 + 2 + 8 + 8 + 4)
+/* Size of all extended NBD_CMD_*, without payload */
+#define NBD_EXTENDED_REQUEST_SIZE (4 + 2 + 2 + 8 + 8 + 8)
+
/* Size of all NBD_REP_* sent in answer to most NBD_OPT_*, without payload */
#define NBD_REPLY_SIZE (4 + 4 + 8)
/* Size of reply to NBD_OPT_EXPORT_NAME */
diff --git a/nbd/server.c b/nbd/server.c
index 56e9b41828e..501749d62b5 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1411,11 +1411,13 @@ nbd_read_eof(NBDClient *client, void *buffer, size_t
size, Error **errp)
static int coroutine_fn nbd_receive_request(NBDClient *client, NBDRequest
*request,
Error **errp)
{
-uint8_t buf[NBD_REQUEST_SIZE];
-uint32_t magic;
+uint8_t buf[NBD_EXTENDED_REQUEST_SIZE];
+uint32_t magic, expect;
int ret;
+size_t size = client->mode >= NBD_MODE_EXTENDED ?
+NBD_EXTENDED_REQUEST_SIZE : NBD_REQUEST_SIZE;
-ret = nbd_read_eof(client, buf, sizeof(buf), errp);
+ret = nbd_read_eof(client, buf, size, errp);
if (ret < 0) {
return ret;
}
@@ -1423,13 +1425,21 @@ static int coroutine_fn nbd_receive_request(NBDClient
*client, NBDRequest *reque
return -EIO;
}
-/* Request
- [ 0 .. 3] magic (NBD_REQUEST_MAGIC)
- [ 4 .. 5] flags (NBD_CMD_FLAG_FUA, ...)
- [ 6 .. 7] type(NBD_CMD_READ, ...)
- [ 8 .. 15] cookie
- [16 .. 23] from
- [24 .. 27] len
+/*
+ * Compact request
+ * [ 0 .. 3] magic (NBD_REQUEST_MAGIC)
+ * [ 4 .. 5] flags (NBD_CMD_FLAG_FUA, ...)
+ * [ 6 .. 7] type(NBD_CMD_READ, ...)
+ * [ 8 .. 15] cookie
+ * [16 .. 23] from
+ * [24 .. 27] len
+ * Extended request
+ * [ 0 .. 3] magic (NBD_EXTENDED_REQUEST_MAGIC)
+ * [ 4 .. 5] flags (NBD_CMD_FLAG_FUA, NBD_CMD_FLAG_PAYLOAD_LEN, ...)
+ * [ 6 .. 7] type(NBD_CMD_READ, ...)
+ * [ 8 .. 15] cookie
+ * [16 .. 23] from
+ * [24 .. 31] len
*/
magic = ldl_be_p(buf);
@@ -1437,13 +1447,20 @@ static int coroutine_fn nbd_receive_request(NBDClient
*client, NBDRequest *reque
request->type = lduw_be_p(buf + 6);
request->cookie = ldq_be_p(buf + 8);
request->from = ldq_be_p(buf + 16);
-request->len= (uint32_t)ldl_be_p(buf + 24); /* widen 32 to 64 bits */
+if (client->mode >= NBD_MODE_EXTENDED) {
+request->len = ldq_be_p(buf + 24);
+expect = NBD_EXTENDED_REQUEST_MAGIC;
+} else {
+request->len = (uint32_t)ldl_be_p(buf + 24); /* widen 32 to 64 bits */
+expect = NBD_REQUEST_MAGIC;
+}
trace_nbd_receive_request(magic, request->flags, request->type,
request->from, request->len);
-if (magic != NBD_REQUEST_MAGIC) {
-error_setg(errp, "invalid magic (got 0x%" PRIx32 ")", magic);
+if (magic != expect) {
+error_setg(errp, "invalid magic (got 0x%" PRIx32 ", expected 0x%"
+ PRIx32 ")", magic, expect);
return -EINVAL;
}
return 0;
--
2.41.0
[PULL 11/15] nbd/client: Accept 64-bit block status chunks
Once extended mode is enabled, we need to accept 64-bit status replies
(even for replies that don't exceed a 32-bit length). It is easier to
normalize narrow replies into wide format so that the rest of our code
only has to handle one width. Although a server is non-compliant if
it sends a 64-bit reply in compact mode, or a 32-bit reply in extended
mode, it is still easy enough to tolerate these mismatches.
In normal execution, we are only requesting "base:allocation" which
never exceeds 32 bits for flag values. But during testing with
x-dirty-bitmap, we can force qemu to connect to some other context
that might have 64-bit status bit; however, we ignore those upper bits
(other than mapping qemu:allocation-depth into something that
'qemu-img map --output=json' can expose), and since that only affects
testing, we really don't bother with checking whether more than the
two least-significant bits are set.
Signed-off-by: Eric Blake
Reviewed-by: Vladimir Sementsov-Ogievskiy
Message-ID: <20230925192229.3186470-22-ebl...@redhat.com>
---
block/nbd.c| 49 --
block/trace-events | 1 +
2 files changed, 35 insertions(+), 15 deletions(-)
diff --git a/block/nbd.c b/block/nbd.c
index 76461430411..52ebc8b2f53 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -615,13 +615,17 @@ static int nbd_parse_offset_hole_payload(BDRVNBDState *s,
*/
static int nbd_parse_blockstatus_payload(BDRVNBDState *s,
NBDStructuredReplyChunk *chunk,
- uint8_t *payload, uint64_t
orig_length,
- NBDExtent32 *extent, Error **errp)
+ uint8_t *payload, bool wide,
+ uint64_t orig_length,
+ NBDExtent64 *extent, Error **errp)
{
uint32_t context_id;
+uint32_t count;
+size_t ext_len = wide ? sizeof(*extent) : sizeof(NBDExtent32);
+size_t pay_len = sizeof(context_id) + wide * sizeof(count) + ext_len;
/* The server succeeded, so it must have sent [at least] one extent */
-if (chunk->length < sizeof(context_id) + sizeof(*extent)) {
+if (chunk->length < pay_len) {
error_setg(errp, "Protocol error: invalid payload for "
"NBD_REPLY_TYPE_BLOCK_STATUS");
return -EINVAL;
@@ -636,8 +640,15 @@ static int nbd_parse_blockstatus_payload(BDRVNBDState *s,
return -EINVAL;
}
-extent->length = payload_advance32();
-extent->flags = payload_advance32();
+if (wide) {
+count = payload_advance32();
+extent->length = payload_advance64();
+extent->flags = payload_advance64();
+} else {
+count = 0;
+extent->length = payload_advance32();
+extent->flags = payload_advance32();
+}
if (extent->length == 0) {
error_setg(errp, "Protocol error: server sent status chunk with "
@@ -658,7 +669,7 @@ static int nbd_parse_blockstatus_payload(BDRVNBDState *s,
* (always a safe status, even if it loses information).
*/
if (s->info.min_block && !QEMU_IS_ALIGNED(extent->length,
- s->info.min_block)) {
+ s->info.min_block)) {
trace_nbd_parse_blockstatus_compliance("extent length is unaligned");
if (extent->length > s->info.min_block) {
extent->length = QEMU_ALIGN_DOWN(extent->length,
@@ -672,13 +683,15 @@ static int nbd_parse_blockstatus_payload(BDRVNBDState *s,
/*
* We used NBD_CMD_FLAG_REQ_ONE, so the server should not have
* sent us any more than one extent, nor should it have included
- * status beyond our request in that extent. However, it's easy
- * enough to ignore the server's noncompliance without killing the
+ * status beyond our request in that extent. Furthermore, a wide
+ * server should have replied with an accurate count (we left
+ * count at 0 for a narrow server). However, it's easy enough to
+ * ignore the server's noncompliance without killing the
* connection; just ignore trailing extents, and clamp things to
* the length of our request.
*/
-if (chunk->length > sizeof(context_id) + sizeof(*extent)) {
-trace_nbd_parse_blockstatus_compliance("more than one extent");
+if (count != wide || chunk->length > pay_len) {
+trace_nbd_parse_blockstatus_compliance("unexpected extent count");
}
if (extent->length > orig_length) {
extent->length = orig_length;
@@ -1124,7 +1137,7 @@ nbd_co_receive_cmdread_reply(BDRVNBDState *s, uint64_t
cookie,
static int coroutine_fn
nbd_co_receive_blockstatus_reply(BDRVNBDState *s, uint64_t cookie,
- uint64_t length, NBDExtent32 *extent,
+ uint64_t length, NBDExtent64 *extent,
[PULL 09/15] nbd/client: Plumb errp through nbd_receive_replies
Instead of ignoring the low-level error just to refabricate our own
message to pass to the caller, we can just plumb the caller's errp
down to the low level.
Signed-off-by: Eric Blake
Message-ID: <20230925192229.3186470-20-ebl...@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
block/nbd.c | 18 --
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/block/nbd.c b/block/nbd.c
index 4a7f37da1c6..22d3cb11ac8 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -416,7 +416,8 @@ static void coroutine_fn GRAPH_RDLOCK
nbd_reconnect_attempt(BDRVNBDState *s)
reconnect_delay_timer_del(s);
}
-static coroutine_fn int nbd_receive_replies(BDRVNBDState *s, uint64_t cookie)
+static coroutine_fn int nbd_receive_replies(BDRVNBDState *s, uint64_t cookie,
+Error **errp)
{
int ret;
uint64_t ind = COOKIE_TO_INDEX(cookie), ind2;
@@ -457,20 +458,25 @@ static coroutine_fn int nbd_receive_replies(BDRVNBDState
*s, uint64_t cookie)
/* We are under mutex and cookie is 0. We have to do the dirty work. */
assert(s->reply.cookie == 0);
-ret = nbd_receive_reply(s->bs, s->ioc, >reply, NULL);
-if (ret <= 0) {
-ret = ret ? ret : -EIO;
+ret = nbd_receive_reply(s->bs, s->ioc, >reply, errp);
+if (ret == 0) {
+ret = -EIO;
+error_setg(errp, "server dropped connection");
+}
+if (ret < 0) {
nbd_channel_error(s, ret);
return ret;
}
if (nbd_reply_is_structured(>reply) &&
s->info.mode < NBD_MODE_STRUCTURED) {
nbd_channel_error(s, -EINVAL);
+error_setg(errp, "unexpected structured reply");
return -EINVAL;
}
ind2 = COOKIE_TO_INDEX(s->reply.cookie);
if (ind2 >= MAX_NBD_REQUESTS || !s->requests[ind2].coroutine) {
nbd_channel_error(s, -EINVAL);
+error_setg(errp, "unexpected cookie value");
return -EINVAL;
}
if (s->reply.cookie == cookie) {
@@ -842,9 +848,9 @@ static coroutine_fn int nbd_co_do_receive_one_chunk(
}
*request_ret = 0;
-ret = nbd_receive_replies(s, cookie);
+ret = nbd_receive_replies(s, cookie, errp);
if (ret < 0) {
-error_setg(errp, "Connection closed");
+error_prepend(errp, "Connection closed: ");
return -EIO;
}
assert(s->ioc);
--
2.41.0
[PULL 12/15] nbd/client: Request extended headers during negotiation
All the pieces are in place for a client to finally request extended
headers. Note that we must not request extended headers when qemu-nbd
is used to connect to the kernel module (as nbd.ko does not expect
them, but expects us to do the negotiation in userspace before handing
the socket over to the kernel), but there is no harm in all other
clients requesting them.
Extended headers are not essential to the information collected during
'qemu-nbd --list', but probing for it gives us one more piece of
information in that output. Update the iotests affected by the new
line of output.
Signed-off-by: Eric Blake
Reviewed-by: Vladimir Sementsov-Ogievskiy
Message-ID: <20230925192229.3186470-23-ebl...@redhat.com>
---
nbd/client-connection.c | 2 +-
nbd/client.c | 20 ++-
qemu-nbd.c| 3 +++
tests/qemu-iotests/223.out| 6 ++
tests/qemu-iotests/233.out| 4
tests/qemu-iotests/241.out| 3 +++
tests/qemu-iotests/307.out| 5 +
.../tests/nbd-qemu-allocation.out | 1 +
8 files changed, 38 insertions(+), 6 deletions(-)
diff --git a/nbd/client-connection.c b/nbd/client-connection.c
index aa0201b7107..f9da67c87e3 100644
--- a/nbd/client-connection.c
+++ b/nbd/client-connection.c
@@ -93,7 +93,7 @@ NBDClientConnection *nbd_client_connection_new(const
SocketAddress *saddr,
.do_negotiation = do_negotiation,
.initial_info.request_sizes = true,
-.initial_info.mode = NBD_MODE_STRUCTURED,
+.initial_info.mode = NBD_MODE_EXTENDED,
.initial_info.base_allocation = true,
.initial_info.x_dirty_bitmap = g_strdup(x_dirty_bitmap),
.initial_info.name = g_strdup(export_name ?: "")
diff --git a/nbd/client.c b/nbd/client.c
index a2f253062aa..29ffc609a4b 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -953,15 +953,23 @@ static int nbd_start_negotiate(QIOChannel *ioc,
QCryptoTLSCreds *tlscreds,
if (fixedNewStyle) {
int result = 0;
+if (max_mode >= NBD_MODE_EXTENDED) {
+result = nbd_request_simple_option(ioc,
+ NBD_OPT_EXTENDED_HEADERS,
+ false, errp);
+if (result) {
+return result < 0 ? -EINVAL : NBD_MODE_EXTENDED;
+}
+}
if (max_mode >= NBD_MODE_STRUCTURED) {
result = nbd_request_simple_option(ioc,
NBD_OPT_STRUCTURED_REPLY,
false, errp);
-if (result < 0) {
-return -EINVAL;
+if (result) {
+return result < 0 ? -EINVAL : NBD_MODE_STRUCTURED;
}
}
-return result ? NBD_MODE_STRUCTURED : NBD_MODE_SIMPLE;
+return NBD_MODE_SIMPLE;
} else {
return NBD_MODE_EXPORT_NAME;
}
@@ -1034,6 +1042,7 @@ int nbd_receive_negotiate(QIOChannel *ioc,
QCryptoTLSCreds *tlscreds,
}
switch (info->mode) {
+case NBD_MODE_EXTENDED:
case NBD_MODE_STRUCTURED:
if (base_allocation) {
result = nbd_negotiate_simple_meta_context(ioc, info, errp);
@@ -1144,7 +1153,7 @@ int nbd_receive_export_list(QIOChannel *ioc,
QCryptoTLSCreds *tlscreds,
*info = NULL;
result = nbd_start_negotiate(ioc, tlscreds, hostname, ,
- NBD_MODE_STRUCTURED, NULL, errp);
+ NBD_MODE_EXTENDED, NULL, errp);
if (tlscreds && sioc) {
ioc = sioc;
}
@@ -1155,6 +1164,7 @@ int nbd_receive_export_list(QIOChannel *ioc,
QCryptoTLSCreds *tlscreds,
switch ((NBDMode)result) {
case NBD_MODE_SIMPLE:
case NBD_MODE_STRUCTURED:
+case NBD_MODE_EXTENDED:
/* newstyle - use NBD_OPT_LIST to populate array, then try
* NBD_OPT_INFO on each array member. If structured replies
* are enabled, also try NBD_OPT_LIST_META_CONTEXT. */
@@ -1191,7 +1201,7 @@ int nbd_receive_export_list(QIOChannel *ioc,
QCryptoTLSCreds *tlscreds,
break;
}
-if (result == NBD_MODE_STRUCTURED &&
+if (result >= NBD_MODE_STRUCTURED &&
nbd_list_meta_contexts(ioc, [i], errp) < 0) {
goto out;
}
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 54faa87a0c0..1a39bb8fac0 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -235,6 +235,9 @@ static int qemu_nbd_client_list(SocketAddress *saddr,
QCryptoTLSCreds *tls,
printf(" opt block: %u\n", list[i].opt_block);
printf(" max block: %u\n", list[i].max_block);
}
+printf(" transaction size: %s\n",
+ list[i].mode >=
[PULL 14/15] nbd/server: Prepare for per-request filtering of BLOCK_STATUS
The next commit will add support for the optional extension
NBD_CMD_FLAG_PAYLOAD during NBD_CMD_BLOCK_STATUS, where the client can
request that the server only return a subset of negotiated contexts,
rather than all contexts. To make that task easier, this patch
populates the list of contexts to return on a per-command basis (for
now, identical to the full set of negotiated contexts).
Signed-off-by: Eric Blake
Message-ID: <20230925192229.3186470-25-ebl...@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
include/block/nbd.h | 1 +
nbd/server.c| 22 +-
2 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 2006497f987..4e7bd6342f9 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -77,6 +77,7 @@ typedef struct NBDRequest {
uint16_t flags; /* NBD_CMD_FLAG_* */
uint16_t type; /* NBD_CMD_* */
NBDMode mode; /* Determines which network representation to use */
+NBDMetaContexts *contexts; /* Used by NBD_CMD_BLOCK_STATUS */
} NBDRequest;
typedef struct NBDSimpleReply {
diff --git a/nbd/server.c b/nbd/server.c
index 2719992db7b..2dce9c3ad6a 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2505,6 +2505,7 @@ static int coroutine_fn
nbd_co_receive_request(NBDRequestData *req,
break;
case NBD_CMD_BLOCK_STATUS:
+request->contexts = >contexts;
valid_flags |= NBD_CMD_FLAG_REQ_ONE;
break;
@@ -2748,17 +2749,18 @@ static coroutine_fn int nbd_handle_request(NBDClient
*client,
"discard failed", errp);
case NBD_CMD_BLOCK_STATUS:
+assert(request->contexts);
if (!request->len) {
return nbd_send_generic_reply(client, request, -EINVAL,
"need non-zero length", errp);
}
assert(client->mode >= NBD_MODE_EXTENDED ||
request->len <= UINT32_MAX);
-if (client->contexts.count) {
+if (request->contexts->count) {
bool dont_fragment = request->flags & NBD_CMD_FLAG_REQ_ONE;
-int contexts_remaining = client->contexts.count;
+int contexts_remaining = request->contexts->count;
-if (client->contexts.base_allocation) {
+if (request->contexts->base_allocation) {
ret = nbd_co_send_block_status(client, request,
exp->common.blk,
request->from,
@@ -2771,7 +2773,7 @@ static coroutine_fn int nbd_handle_request(NBDClient
*client,
}
}
-if (client->contexts.allocation_depth) {
+if (request->contexts->allocation_depth) {
ret = nbd_co_send_block_status(client, request,
exp->common.blk,
request->from, request->len,
@@ -2784,8 +2786,9 @@ static coroutine_fn int nbd_handle_request(NBDClient
*client,
}
}
+assert(request->contexts->exp == client->exp);
for (i = 0; i < client->exp->nr_export_bitmaps; i++) {
-if (!client->contexts.bitmaps[i]) {
+if (!request->contexts->bitmaps[i]) {
continue;
}
ret = nbd_co_send_bitmap(client, request,
@@ -2801,6 +2804,10 @@ static coroutine_fn int nbd_handle_request(NBDClient
*client,
assert(!contexts_remaining);
return 0;
+} else if (client->contexts.count) {
+return nbd_send_generic_reply(client, request, -EINVAL,
+ "CMD_BLOCK_STATUS payload not valid",
+ errp);
} else {
return nbd_send_generic_reply(client, request, -EINVAL,
"CMD_BLOCK_STATUS not negotiated",
@@ -2879,6 +2886,11 @@ static coroutine_fn void nbd_trip(void *opaque)
} else {
ret = nbd_handle_request(client, , req->data, _err);
}
+if (request.contexts && request.contexts != >contexts) {
+assert(request.type == NBD_CMD_BLOCK_STATUS);
+g_free(request.contexts->bitmaps);
+g_free(request.contexts);
+}
if (ret < 0) {
error_prepend(_err, "Failed to send reply: ");
goto disconnect;
--
2.41.0
[PULL 07/15] nbd/server: Support 64-bit block status
The NBD spec states that if the client negotiates extended headers,
the server must avoid NBD_REPLY_TYPE_BLOCK_STATUS and instead use
NBD_REPLY_TYPE_BLOCK_STATUS_EXT which supports 64-bit lengths, even if
the reply does not need more than 32 bits. As of this patch,
client->mode is still never NBD_MODE_EXTENDED, so the code added here
does not take effect until the next patch enables negotiation.
For now, all metacontexts that we know how to export never populate
more than 32 bits of information, so we don't have to worry about
NBD_REP_ERR_EXT_HEADER_REQD or filtering during handshake, and we
always send all zeroes for the upper 32 bits of status during
NBD_CMD_BLOCK_STATUS.
Note that we previously had some interesting size-juggling on call
chains, such as:
nbd_co_send_block_status(uint32_t length)
-> blockstatus_to_extents(uint32_t bytes)
-> bdrv_block_status_above(bytes, _t num)
-> nbd_extent_array_add(uint64_t num)
-> store num in 32-bit length
But we were lucky that it never overflowed: bdrv_block_status_above
never sets num larger than bytes, and we had previously been capping
'bytes' at 32 bits (since the protocol does not allow sending a larger
request without extended headers). This patch adds some assertions
that ensure we continue to avoid overflowing 32 bits for a narrow
client, while fully utilizing 64-bits all the way through when the
client understands that. Even in 64-bit math, overflow is not an
issue, because all lengths are coming from the block layer, and we
know that the block layer does not support images larger than off_t
(if lengths were coming from the network, the story would be
different).
Signed-off-by: Eric Blake
Reviewed-by: Vladimir Sementsov-Ogievskiy
Message-ID: <20230925192229.3186470-18-ebl...@redhat.com>
---
nbd/server.c | 108 ++-
1 file changed, 82 insertions(+), 26 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index 910c48c6467..183efe27bf8 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2103,20 +2103,24 @@ static int coroutine_fn
nbd_co_send_sparse_read(NBDClient *client,
}
typedef struct NBDExtentArray {
-NBDExtent32 *extents;
+NBDExtent64 *extents;
unsigned int nb_alloc;
unsigned int count;
uint64_t total_length;
+bool extended;
bool can_add;
bool converted_to_be;
} NBDExtentArray;
-static NBDExtentArray *nbd_extent_array_new(unsigned int nb_alloc)
+static NBDExtentArray *nbd_extent_array_new(unsigned int nb_alloc,
+NBDMode mode)
{
NBDExtentArray *ea = g_new0(NBDExtentArray, 1);
+assert(mode >= NBD_MODE_STRUCTURED);
ea->nb_alloc = nb_alloc;
-ea->extents = g_new(NBDExtent32, nb_alloc);
+ea->extents = g_new(NBDExtent64, nb_alloc);
+ea->extended = mode >= NBD_MODE_EXTENDED;
ea->can_add = true;
return ea;
@@ -2135,15 +2139,36 @@ static void
nbd_extent_array_convert_to_be(NBDExtentArray *ea)
int i;
assert(!ea->converted_to_be);
+assert(ea->extended);
ea->can_add = false;
ea->converted_to_be = true;
for (i = 0; i < ea->count; i++) {
-ea->extents[i].flags = cpu_to_be32(ea->extents[i].flags);
-ea->extents[i].length = cpu_to_be32(ea->extents[i].length);
+ea->extents[i].length = cpu_to_be64(ea->extents[i].length);
+ea->extents[i].flags = cpu_to_be64(ea->extents[i].flags);
}
}
+/* Further modifications of the array after conversion are abandoned */
+static NBDExtent32 *nbd_extent_array_convert_to_narrow(NBDExtentArray *ea)
+{
+int i;
+NBDExtent32 *extents = g_new(NBDExtent32, ea->count);
+
+assert(!ea->converted_to_be);
+assert(!ea->extended);
+ea->can_add = false;
+ea->converted_to_be = true;
+
+for (i = 0; i < ea->count; i++) {
+assert((ea->extents[i].length | ea->extents[i].flags) <= UINT32_MAX);
+extents[i].length = cpu_to_be32(ea->extents[i].length);
+extents[i].flags = cpu_to_be32(ea->extents[i].flags);
+}
+
+return extents;
+}
+
/*
* Add extent to NBDExtentArray. If extent can't be added (no available space),
* return -1.
@@ -2154,19 +2179,27 @@ static void
nbd_extent_array_convert_to_be(NBDExtentArray *ea)
* would result in an incorrect range reported to the client)
*/
static int nbd_extent_array_add(NBDExtentArray *ea,
-uint32_t length, uint32_t flags)
+uint64_t length, uint32_t flags)
{
assert(ea->can_add);
if (!length) {
return 0;
}
+if (!ea->extended) {
+assert(length <= UINT32_MAX);
+}
/* Extend previous extent if flags are the same */
if (ea->count > 0 && flags == ea->extents[ea->count - 1].flags) {
-uint64_t sum = (uint64_t)length + ea->extents[ea->count - 1].length;
+uint64_t sum = length + ea->extents[ea->count - 1].length;
-if (sum <= UINT32_MAX) {
+/*
+ * sum
[PULL 13/15] nbd/server: Refactor list of negotiated meta contexts
Peform several minor refactorings of how the list of negotiated meta
contexts is managed, to make upcoming patches easier: Promote the
internal type NBDExportMetaContexts to the public opaque type
NBDMetaContexts, and mark exp const. Use a shorter member name in
NBDClient. Hoist calls to nbd_check_meta_context() earlier in their
callers, as the number of negotiated contexts may impact the flags
exposed in regards to an export, which in turn requires a new
parameter. Drop a redundant parameter to nbd_negotiate_meta_queries.
No semantic change intended on the success path; on the failure path,
dropping context in nbd_check_meta_export even when reporting an error
is safer.
Signed-off-by: Eric Blake
Reviewed-by: Vladimir Sementsov-Ogievskiy
Message-ID: <20230925192229.3186470-24-ebl...@redhat.com>
---
include/block/nbd.h | 1 +
nbd/server.c| 55 -
2 files changed, 31 insertions(+), 25 deletions(-)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index ba8724f5336..2006497f987 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -29,6 +29,7 @@
typedef struct NBDExport NBDExport;
typedef struct NBDClient NBDClient;
typedef struct NBDClientConnection NBDClientConnection;
+typedef struct NBDMetaContexts NBDMetaContexts;
extern const BlockExportDriver blk_exp_nbd;
diff --git a/nbd/server.c b/nbd/server.c
index d3eed6535bf..2719992db7b 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -105,11 +105,13 @@ struct NBDExport {
static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
-/* NBDExportMetaContexts represents a list of contexts to be exported,
+/*
+ * NBDMetaContexts represents a list of meta contexts in use,
* as selected by NBD_OPT_SET_META_CONTEXT. Also used for
- * NBD_OPT_LIST_META_CONTEXT. */
-typedef struct NBDExportMetaContexts {
-NBDExport *exp;
+ * NBD_OPT_LIST_META_CONTEXT.
+ */
+struct NBDMetaContexts {
+const NBDExport *exp; /* associated export */
size_t count; /* number of negotiated contexts */
bool base_allocation; /* export base:allocation context (block status) */
bool allocation_depth; /* export qemu:allocation-depth */
@@ -117,7 +119,7 @@ typedef struct NBDExportMetaContexts {
* export qemu:dirty-bitmap:,
* sized by exp->nr_export_bitmaps
*/
-} NBDExportMetaContexts;
+};
struct NBDClient {
int refcount;
@@ -144,7 +146,7 @@ struct NBDClient {
uint32_t check_align; /* If non-zero, check for aligned client requests */
NBDMode mode;
-NBDExportMetaContexts export_meta;
+NBDMetaContexts contexts; /* Negotiated meta contexts */
uint32_t opt; /* Current option being negotiated */
uint32_t optlen; /* remaining length of data in ioc for the option being
@@ -455,10 +457,10 @@ static int nbd_negotiate_handle_list(NBDClient *client,
Error **errp)
return nbd_negotiate_send_rep(client, NBD_REP_ACK, errp);
}
-static void nbd_check_meta_export(NBDClient *client)
+static void nbd_check_meta_export(NBDClient *client, NBDExport *exp)
{
-if (client->exp != client->export_meta.exp) {
-client->export_meta.count = 0;
+if (exp != client->contexts.exp) {
+client->contexts.count = 0;
}
}
@@ -504,6 +506,7 @@ static int nbd_negotiate_handle_export_name(NBDClient
*client, bool no_zeroes,
error_setg(errp, "export not found");
return -EINVAL;
}
+nbd_check_meta_export(client, client->exp);
myflags = client->exp->nbdflags;
if (client->mode >= NBD_MODE_STRUCTURED) {
@@ -521,7 +524,6 @@ static int nbd_negotiate_handle_export_name(NBDClient
*client, bool no_zeroes,
QTAILQ_INSERT_TAIL(>exp->clients, client, next);
blk_exp_ref(>exp->common);
-nbd_check_meta_export(client);
return 0;
}
@@ -641,6 +643,9 @@ static int nbd_negotiate_handle_info(NBDClient *client,
Error **errp)
errp, "export '%s' not present",
sane_name);
}
+if (client->opt == NBD_OPT_GO) {
+nbd_check_meta_export(client, exp);
+}
/* Don't bother sending NBD_INFO_NAME unless client requested it */
if (sendname) {
@@ -729,7 +734,6 @@ static int nbd_negotiate_handle_info(NBDClient *client,
Error **errp)
client->check_align = check_align;
QTAILQ_INSERT_TAIL(>exp->clients, client, next);
blk_exp_ref(>exp->common);
-nbd_check_meta_export(client);
rc = 1;
}
return rc;
@@ -852,7 +856,7 @@ static bool nbd_strshift(const char **str, const char
*prefix)
* Handle queries to 'base' namespace. For now, only the base:allocation
* context is available. Return true if @query has been handled.
*/
-static bool nbd_meta_base_query(NBDClient *client, NBDExportMetaContexts *meta,
+static bool nbd_meta_base_query(NBDClient *client, NBDMetaContexts *meta,
[PULL 04/15] nbd/server: Support a request payload
Upcoming additions to support NBD 64-bit effect lengths allow for the
possibility to distinguish between payload length (capped at 32M) and
effect length (64 bits, although we generally assume 63 bits because
of off_t limitations). Without that extension, only the NBD_CMD_WRITE
request has a payload; but with the extension, it makes sense to allow
at least NBD_CMD_BLOCK_STATUS to have both a payload and effect length
in a future patch (where the payload is a limited-size struct that in
turn gives the real effect length as well as a subset of known ids for
which status is requested). Other future NBD commands may also have a
request payload, so the 64-bit extension introduces a new
NBD_CMD_FLAG_PAYLOAD_LEN that distinguishes between whether the header
length is a payload length or an effect length, rather than
hard-coding the decision based on the command.
According to the spec, a client should never send a command with a
payload without the negotiation phase proving such extension is
available. So in the unlikely event the bit is set or cleared
incorrectly, the client is already at fault; if the client then
provides the payload, we can gracefully consume it off the wire and
fail the command with NBD_EINVAL (subsequent checks for magic numbers
ensure we are still in sync), while if the client fails to send
payload we block waiting for it (basically deadlocking our connection
to the bad client, but not negatively impacting our ability to service
other clients, so not a security risk). Note that we do not support
the payload version of BLOCK_STATUS yet.
This patch also fixes a latent bug introduced in b2578459: once
request->len can be 64 bits, assigning it to a 32-bit payload_len can
cause wraparound to 0 which then sets req->complete prematurely;
thankfully, the bug was not possible back then (it takes this and
later patches to even allow request->len larger than 32 bits; and
since previously the only 'payload_len = request->len' assignment was
in NBD_CMD_WRITE which also sets check_length, which in turn rejects
lengths larger than 32M before relying on any possibly-truncated value
stored in payload_len).
Signed-off-by: Eric Blake
Message-ID: <20230925192229.3186470-15-ebl...@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy
[eblake: enhance comment on handling client error, fix type bug]
Signed-off-by: Eric Blake
---
nbd/server.c | 42 +-
nbd/trace-events | 1 +
2 files changed, 38 insertions(+), 5 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index 7a6f95071f8..56e9b41828e 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2322,10 +2322,12 @@ static int coroutine_fn
nbd_co_receive_request(NBDRequestData *req,
Error **errp)
{
NBDClient *client = req->client;
+bool extended_with_payload;
bool check_length = false;
bool check_rofs = false;
bool allocate_buffer = false;
-unsigned payload_len = 0;
+bool payload_okay = false;
+uint64_t payload_len = 0;
int valid_flags = NBD_CMD_FLAG_FUA;
int ret;
@@ -2338,6 +2340,13 @@ static int coroutine_fn
nbd_co_receive_request(NBDRequestData *req,
trace_nbd_co_receive_request_decode_type(request->cookie, request->type,
nbd_cmd_lookup(request->type));
+extended_with_payload = client->mode >= NBD_MODE_EXTENDED &&
+request->flags & NBD_CMD_FLAG_PAYLOAD_LEN;
+if (extended_with_payload) {
+payload_len = request->len;
+check_length = true;
+}
+
switch (request->type) {
case NBD_CMD_DISC:
/* Special case: we're going to disconnect without a reply,
@@ -2354,6 +2363,15 @@ static int coroutine_fn
nbd_co_receive_request(NBDRequestData *req,
break;
case NBD_CMD_WRITE:
+if (client->mode >= NBD_MODE_EXTENDED) {
+if (!extended_with_payload) {
+/* The client is noncompliant. Trace it, but proceed. */
+trace_nbd_co_receive_ext_payload_compliance(request->from,
+request->len);
+}
+valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN;
+}
+payload_okay = true;
payload_len = request->len;
check_length = true;
allocate_buffer = true;
@@ -2395,6 +2413,16 @@ static int coroutine_fn
nbd_co_receive_request(NBDRequestData *req,
request->len, NBD_MAX_BUFFER_SIZE);
return -EINVAL;
}
+if (payload_len && !payload_okay) {
+/*
+ * For now, we don't support payloads on other commands; but
+ * we can keep the connection alive by ignoring the payload.
+ * We will fail the command later with NBD_EINVAL for the use
+ * of an unsupported flag (and not for access beyond bounds).
+ */
+assert(request->type != NBD_CMD_WRITE);
+request->len = 0;
+}
if
[PULL 10/15] nbd/client: Initial support for extended headers
Update the client code to be able to send an extended request, and
parse an extended header from the server. Note that since we reject
any structured reply with a too-large payload, we can always normalize
a valid header back into the compact form, so that the caller need not
deal with two branches of a union. Still, until a later patch lets
the client negotiate extended headers, the code added here should not
be reached. Note that because of the different magic numbers, it is
just as easy to trace and then tolerate a non-compliant server sending
the wrong header reply as it would be to insist that the server is
compliant.
Signed-off-by: Eric Blake
Message-ID: <20230925192229.3186470-21-ebl...@redhat.com>
[eblake: fix trace format]
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
include/block/nbd.h | 3 +-
block/nbd.c | 2 +-
nbd/client.c| 104 +---
nbd/trace-events| 3 +-
4 files changed, 74 insertions(+), 38 deletions(-)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 8a765e78dfb..ba8724f5336 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -389,7 +389,8 @@ int nbd_init(int fd, QIOChannelSocket *sioc, NBDExportInfo
*info,
Error **errp);
int nbd_send_request(QIOChannel *ioc, NBDRequest *request);
int coroutine_fn nbd_receive_reply(BlockDriverState *bs, QIOChannel *ioc,
- NBDReply *reply, Error **errp);
+ NBDReply *reply, NBDMode mode,
+ Error **errp);
int nbd_client(int fd);
int nbd_disconnect(int fd);
int nbd_errno_to_system_errno(int err);
diff --git a/block/nbd.c b/block/nbd.c
index 22d3cb11ac8..76461430411 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -458,7 +458,7 @@ static coroutine_fn int nbd_receive_replies(BDRVNBDState
*s, uint64_t cookie,
/* We are under mutex and cookie is 0. We have to do the dirty work. */
assert(s->reply.cookie == 0);
-ret = nbd_receive_reply(s->bs, s->ioc, >reply, errp);
+ret = nbd_receive_reply(s->bs, s->ioc, >reply, s->info.mode, errp);
if (ret == 0) {
ret = -EIO;
error_setg(errp, "server dropped connection");
diff --git a/nbd/client.c b/nbd/client.c
index cecb0f04377..a2f253062aa 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -1346,22 +1346,29 @@ int nbd_disconnect(int fd)
int nbd_send_request(QIOChannel *ioc, NBDRequest *request)
{
-uint8_t buf[NBD_REQUEST_SIZE];
+uint8_t buf[NBD_EXTENDED_REQUEST_SIZE];
+size_t len;
-assert(request->mode <= NBD_MODE_STRUCTURED); /* TODO handle extended */
-assert(request->len <= UINT32_MAX);
trace_nbd_send_request(request->from, request->len, request->cookie,
request->flags, request->type,
nbd_cmd_lookup(request->type));
-stl_be_p(buf, NBD_REQUEST_MAGIC);
stw_be_p(buf + 4, request->flags);
stw_be_p(buf + 6, request->type);
stq_be_p(buf + 8, request->cookie);
stq_be_p(buf + 16, request->from);
-stl_be_p(buf + 24, request->len);
+if (request->mode >= NBD_MODE_EXTENDED) {
+stl_be_p(buf, NBD_EXTENDED_REQUEST_MAGIC);
+stq_be_p(buf + 24, request->len);
+len = NBD_EXTENDED_REQUEST_SIZE;
+} else {
+assert(request->len <= UINT32_MAX);
+stl_be_p(buf, NBD_REQUEST_MAGIC);
+stl_be_p(buf + 24, request->len);
+len = NBD_REQUEST_SIZE;
+}
-return nbd_write(ioc, buf, sizeof(buf), NULL);
+return nbd_write(ioc, buf, len, NULL);
}
/* nbd_receive_simple_reply
@@ -1388,30 +1395,36 @@ static int nbd_receive_simple_reply(QIOChannel *ioc,
NBDSimpleReply *reply,
return 0;
}
-/* nbd_receive_structured_reply_chunk
+/* nbd_receive_reply_chunk_header
* Read structured reply chunk except magic field (which should be already
- * read).
+ * read). Normalize into the compact form.
* Payload is not read.
*/
-static int nbd_receive_structured_reply_chunk(QIOChannel *ioc,
- NBDStructuredReplyChunk *chunk,
- Error **errp)
+static int nbd_receive_reply_chunk_header(QIOChannel *ioc, NBDReply *chunk,
+ Error **errp)
{
int ret;
+size_t len;
+uint64_t payload_len;
-assert(chunk->magic == NBD_STRUCTURED_REPLY_MAGIC);
+if (chunk->magic == NBD_STRUCTURED_REPLY_MAGIC) {
+len = sizeof(chunk->structured);
+} else {
+assert(chunk->magic == NBD_EXTENDED_REPLY_MAGIC);
+len = sizeof(chunk->extended);
+}
ret = nbd_read(ioc, (uint8_t *)chunk + sizeof(chunk->magic),
- sizeof(*chunk) - sizeof(chunk->magic), "structured chunk",
+ len - sizeof(chunk->magic), "structured chunk",
errp);
if (ret < 0) {
return ret;
}
-chunk->flags =
[PULL 08/15] nbd/server: Enable initial support for extended headers
Time to start supporting clients that request extended headers. Now
we can finally reach the code added across several previous patches.
Even though the NBD spec has been altered to allow us to accept
NBD_CMD_READ larger than the max payload size (provided our response
is a hole or broken up over more than one data chunk), we are not
planning to take advantage of that, and continue to cap NBD_CMD_READ
to 32M regardless of header size.
For NBD_CMD_WRITE_ZEROES and NBD_CMD_TRIM, the block layer already
supports 64-bit operations without any effort on our part. For
NBD_CMD_BLOCK_STATUS, the client's length is a hint, and the previous
patch took care of implementing the required
NBD_REPLY_TYPE_BLOCK_STATUS_EXT.
We do not yet support clients that want to do request payload
filtering of NBD_CMD_BLOCK_STATUS; that will be added in later
patches, but is not essential for qemu as a client since qemu only
requests the single context base:allocation.
Signed-off-by: Eric Blake
Reviewed-by: Vladimir Sementsov-Ogievskiy
Message-ID: <20230925192229.3186470-19-ebl...@redhat.com>
---
docs/interop/nbd.txt | 1 +
nbd/server.c | 21 +
2 files changed, 22 insertions(+)
diff --git a/docs/interop/nbd.txt b/docs/interop/nbd.txt
index f5ca25174a6..9aae5e1f294 100644
--- a/docs/interop/nbd.txt
+++ b/docs/interop/nbd.txt
@@ -69,3 +69,4 @@ NBD_CMD_BLOCK_STATUS for "qemu:dirty-bitmap:", NBD_CMD_CACHE
NBD_CMD_FLAG_FAST_ZERO
* 5.2: NBD_CMD_BLOCK_STATUS for "qemu:allocation-depth"
* 7.1: NBD_FLAG_CAN_MULTI_CONN for shareable writable exports
+* 8.2: NBD_OPT_EXTENDED_HEADERS
diff --git a/nbd/server.c b/nbd/server.c
index 183efe27bf8..d3eed6535bf 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -482,6 +482,10 @@ static int nbd_negotiate_handle_export_name(NBDClient
*client, bool no_zeroes,
[10 .. 133] reserved (0) [unless no_zeroes]
*/
trace_nbd_negotiate_handle_export_name();
+if (client->mode >= NBD_MODE_EXTENDED) {
+error_setg(errp, "Extended headers already negotiated");
+return -EINVAL;
+}
if (client->optlen > NBD_MAX_STRING_SIZE) {
error_setg(errp, "Bad length received");
return -EINVAL;
@@ -1264,6 +1268,10 @@ static int nbd_negotiate_options(NBDClient *client,
Error **errp)
case NBD_OPT_STRUCTURED_REPLY:
if (length) {
ret = nbd_reject_length(client, false, errp);
+} else if (client->mode >= NBD_MODE_EXTENDED) {
+ret = nbd_negotiate_send_rep_err(
+client, NBD_REP_ERR_EXT_HEADER_REQD, errp,
+"extended headers already negotiated");
} else if (client->mode >= NBD_MODE_STRUCTURED) {
ret = nbd_negotiate_send_rep_err(
client, NBD_REP_ERR_INVALID, errp,
@@ -1280,6 +1288,19 @@ static int nbd_negotiate_options(NBDClient *client,
Error **errp)
errp);
break;
+case NBD_OPT_EXTENDED_HEADERS:
+if (length) {
+ret = nbd_reject_length(client, false, errp);
+} else if (client->mode >= NBD_MODE_EXTENDED) {
+ret = nbd_negotiate_send_rep_err(
+client, NBD_REP_ERR_INVALID, errp,
+"extended headers already negotiated");
+} else {
+ret = nbd_negotiate_send_rep(client, NBD_REP_ACK, errp);
+client->mode = NBD_MODE_EXTENDED;
+}
+break;
+
default:
ret = nbd_opt_drop(client, NBD_REP_ERR_UNSUP, errp,
"Unsupported option %" PRIu32 " (%s)",
--
2.41.0
Re: [Libguestfs] [PATCH v7 01/12] nbd/server: Support a request payload
On 05.10.23 18:38, Eric Blake wrote:
On Mon, Sep 25, 2023 at 02:22:31PM -0500, Eric Blake wrote:
Upcoming additions to support NBD 64-bit effect lengths allow for the
possibility to distinguish between payload length (capped at 32M) and
effect length (64 bits, although we generally assume 63 bits because
of off_t limitations).
[...]
+++ b/nbd/server.c
@@ -2322,9 +2322,11 @@ static int coroutine_fn
nbd_co_receive_request(NBDRequestData *req,
Error **errp)
{
NBDClient *client = req->client;
+bool extended_with_payload;
bool check_length = false;
bool check_rofs = false;
bool allocate_buffer = false;
+bool payload_okay = false;
unsigned payload_len = 0;
Pre-existing type mismatch caught as a result of Vladimir's review of
12/12, but:
int valid_flags = NBD_CMD_FLAG_FUA;
int ret;
@@ -2338,6 +2340,13 @@ static int coroutine_fn
nbd_co_receive_request(NBDRequestData *req,
trace_nbd_co_receive_request_decode_type(request->cookie, request->type,
nbd_cmd_lookup(request->type));
+extended_with_payload = client->mode >= NBD_MODE_EXTENDED &&
+request->flags & NBD_CMD_FLAG_PAYLOAD_LEN;
+if (extended_with_payload) {
+payload_len = request->len;
this can assign a 64-bit number into a 32-bit variable, which can
truncate to 0,...
+check_length = true;
+}
+
switch (request->type) {
case NBD_CMD_DISC:
/* Special case: we're going to disconnect without a reply,
@@ -2354,6 +2363,15 @@ static int coroutine_fn
nbd_co_receive_request(NBDRequestData *req,
break;
case NBD_CMD_WRITE:
+if (client->mode >= NBD_MODE_EXTENDED) {
+if (!extended_with_payload) {
+/* The client is noncompliant. Trace it, but proceed. */
+trace_nbd_co_receive_ext_payload_compliance(request->from,
+request->len);
+}
+valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN;
+}
+payload_okay = true;
payload_len = request->len;
...the pre-existing code is safe only as long as request->len cannot
exceed 32 bytes (which it can't do until later in this series actually
enables extended requests). Switching the type now is prudent...
check_length = true;
allocate_buffer = true;
@@ -2395,6 +2413,14 @@ static int coroutine_fn
nbd_co_receive_request(NBDRequestData *req,
request->len, NBD_MAX_BUFFER_SIZE);
return -EINVAL;
}
+if (payload_len && !payload_okay) {
+/*
+ * For now, we don't support payloads on other commands; but
+ * we can keep the connection alive by ignoring the payload.
+ */
+assert(request->type != NBD_CMD_WRITE);
+request->len = 0;
...otherwise, this check is bypassed for a request size of exactly 4G
if check_length is false and thus the previous conditional for
request->len vs. NBD_MAX_BUFFER_SIZE didn't trigger (prior to this
patch, payload_len was only set for CND_WRITE which also set
check_length). Thus, I'm squashing in:
diff --git i/nbd/server.c w/nbd/server.c
index 5258064e5ac..1cb66e86a89 100644
--- i/nbd/server.c
+++ w/nbd/server.c
@@ -2327,7 +2327,7 @@ static int coroutine_fn
nbd_co_receive_request(NBDRequestData *req,
bool check_rofs = false;
bool allocate_buffer = false;
bool payload_okay = false;
-unsigned payload_len = 0;
+uint64_t payload_len = 0;
int valid_flags = NBD_CMD_FLAG_FUA;
int ret;
OK, agree
--
Best regards,
Vladimir
Re: [PATCH v3 02/16] hw/ide/ahci: Clean up local variable shadowing
On Wed, Oct 4, 2023 at 8:00 AM Philippe Mathieu-Daudé wrote:
>
> Fix:
>
> hw/ide/ahci.c:1577:23: error: declaration shadows a local variable
> [-Werror,-Wshadow]
> IDEState *s = >port.ifs[j];
> ^
> hw/ide/ahci.c:1569:29: note: previous declaration is here
> void ahci_uninit(AHCIState *s)
> ^
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: John Snow
> ---
> hw/ide/ahci.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index d0a774bc17..fcc5476e9e 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1622,9 +1622,7 @@ void ahci_uninit(AHCIState *s)
> AHCIDevice *ad = >dev[i];
>
> for (j = 0; j < 2; j++) {
> -IDEState *s = >port.ifs[j];
> -
> -ide_exit(s);
> +ide_exit(>port.ifs[j]);
> }
> object_unparent(OBJECT(>port));
> }
> --
> 2.41.0
>
Re: [Libguestfs] [PATCH v7 01/12] nbd/server: Support a request payload
On Mon, Sep 25, 2023 at 02:22:31PM -0500, Eric Blake wrote:
> Upcoming additions to support NBD 64-bit effect lengths allow for the
> possibility to distinguish between payload length (capped at 32M) and
> effect length (64 bits, although we generally assume 63 bits because
> of off_t limitations).
[...]
> +++ b/nbd/server.c
> @@ -2322,9 +2322,11 @@ static int coroutine_fn
> nbd_co_receive_request(NBDRequestData *req,
> Error **errp)
> {
> NBDClient *client = req->client;
> +bool extended_with_payload;
> bool check_length = false;
> bool check_rofs = false;
> bool allocate_buffer = false;
> +bool payload_okay = false;
> unsigned payload_len = 0;
Pre-existing type mismatch caught as a result of Vladimir's review of
12/12, but:
> int valid_flags = NBD_CMD_FLAG_FUA;
> int ret;
> @@ -2338,6 +2340,13 @@ static int coroutine_fn
> nbd_co_receive_request(NBDRequestData *req,
>
> trace_nbd_co_receive_request_decode_type(request->cookie, request->type,
> nbd_cmd_lookup(request->type));
> +extended_with_payload = client->mode >= NBD_MODE_EXTENDED &&
> +request->flags & NBD_CMD_FLAG_PAYLOAD_LEN;
> +if (extended_with_payload) {
> +payload_len = request->len;
this can assign a 64-bit number into a 32-bit variable, which can
truncate to 0,...
> +check_length = true;
> +}
> +
> switch (request->type) {
> case NBD_CMD_DISC:
> /* Special case: we're going to disconnect without a reply,
> @@ -2354,6 +2363,15 @@ static int coroutine_fn
> nbd_co_receive_request(NBDRequestData *req,
> break;
>
> case NBD_CMD_WRITE:
> +if (client->mode >= NBD_MODE_EXTENDED) {
> +if (!extended_with_payload) {
> +/* The client is noncompliant. Trace it, but proceed. */
> +trace_nbd_co_receive_ext_payload_compliance(request->from,
> +request->len);
> +}
> +valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN;
> +}
> +payload_okay = true;
> payload_len = request->len;
...the pre-existing code is safe only as long as request->len cannot
exceed 32 bytes (which it can't do until later in this series actually
enables extended requests). Switching the type now is prudent...
> check_length = true;
> allocate_buffer = true;
> @@ -2395,6 +2413,14 @@ static int coroutine_fn
> nbd_co_receive_request(NBDRequestData *req,
> request->len, NBD_MAX_BUFFER_SIZE);
> return -EINVAL;
> }
> +if (payload_len && !payload_okay) {
> +/*
> + * For now, we don't support payloads on other commands; but
> + * we can keep the connection alive by ignoring the payload.
> + */
> +assert(request->type != NBD_CMD_WRITE);
> +request->len = 0;
...otherwise, this check is bypassed for a request size of exactly 4G
if check_length is false and thus the previous conditional for
request->len vs. NBD_MAX_BUFFER_SIZE didn't trigger (prior to this
patch, payload_len was only set for CND_WRITE which also set
check_length). Thus, I'm squashing in:
diff --git i/nbd/server.c w/nbd/server.c
index 5258064e5ac..1cb66e86a89 100644
--- i/nbd/server.c
+++ w/nbd/server.c
@@ -2327,7 +2327,7 @@ static int coroutine_fn
nbd_co_receive_request(NBDRequestData *req,
bool check_rofs = false;
bool allocate_buffer = false;
bool payload_okay = false;
-unsigned payload_len = 0;
+uint64_t payload_len = 0;
int valid_flags = NBD_CMD_FLAG_FUA;
int ret;
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
Re: [Libguestfs] [PATCH v7 12/12] nbd/server: Add FLAG_PAYLOAD support to CMD_BLOCK_STATUS
On Thu, Oct 05, 2023 at 05:33:26PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > +static int
> > +nbd_co_block_status_payload_read(NBDClient *client, NBDRequest *request,
> > + Error **errp)
> > +{
> > +int payload_len = request->len;
>
> payload_len should be uint64_t
>
> > +g_autofree char *buf = NULL;
> > +size_t count, i, nr_bitmaps;
> > +uint32_t id;
> > +
>
> otherwise, we may do something unexpected here, when reqeuest->len is too big
> for int:
>
> > +if (payload_len > NBD_MAX_BUFFER_SIZE) {
> > +error_setg(errp, "len (%" PRIu64 ") is larger than max len (%u)",
> > + request->len, NBD_MAX_BUFFER_SIZE);
> > +return -EINVAL;
> > +}
Oh, it looks like I introduced that same type mismatch in commit
8db7e2d6 as well, although it appears to have a latent effect until
this series enables the ability for request->length to actually exceed
32 bits. I'll reply on 1/12 with another squash I'm making there.
> > +
> > +assert(client->contexts.exp == client->exp);
> > +nr_bitmaps = client->exp->nr_export_bitmaps;
> > +request->contexts = g_new0(NBDMetaContexts, 1);
> > +request->contexts->exp = client->exp;
> > +
> > +if (payload_len % sizeof(uint32_t) ||
> > +payload_len < sizeof(NBDBlockStatusPayload) ||
> > +payload_len > (sizeof(NBDBlockStatusPayload) +
> > + sizeof(id) * client->contexts.count)) {
> > +goto skip;
> > +}
>
> [..]
>
> >* connection right away, -EAGAIN to indicate we were interrupted and the
> > @@ -2505,7 +2593,18 @@ static int coroutine_fn
> > nbd_co_receive_request(NBDRequestData *req,
> > break;
> >
> > case NBD_CMD_BLOCK_STATUS:
> > -request->contexts = >contexts;
> > +if (extended_with_payload) {
> > +ret = nbd_co_block_status_payload_read(client, request, errp);
> > +if (ret < 0) {
> > +return ret;
> > +}
> > +/* payload now consumed */
> > +check_length = extended_with_payload = false;
>
> why set extended_with_payload to false? it's a bit misleading. And you don't
> do this for WRITE request.
Indeed; it doesn't make any different to later in the function. Will drop.
>
> > +payload_len = 0;
> > +valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN;
> > +} else {
> > +request->contexts = >contexts;
> > +}
> > valid_flags |= NBD_CMD_FLAG_REQ_ONE;
> > break;
> >
>
> [..]
>
>
> with payload_len changed to uint64_t, your squash-in applied and s/>/>=/
> fixed:
> Reviewed-by: Vladimir Sementsov-Ogievskiy
Thanks for the careful review.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
Re: [PULL 0/9] Python patches
On Thu, Oct 5, 2023, 9:00 AM Stefan Hajnoczi wrote: > On Thu, 5 Oct 2023 at 00:49, Philippe Mathieu-Daudé > wrote: > > > > Hi John, > > > > On 4/10/23 21:46, John Snow wrote: > > > The following changes since commit > da1034094d375afe9e3d8ec8980550ea0f06f7e0: > > > > > >Merge tag 'for-upstream' of https://gitlab.com/bonzini/qemu into > staging (2023-10-03 07:43:44 -0400) > > > > > > are available in the Git repository at: > > > > > >https://gitlab.com/jsnow/qemu.git tags/python-pull-request > > > > > > for you to fetch changes up to > 4d7a663cbe8343e884b88e44bd88d37dd0a470e5: > > > > > >Python: test Python 3.12 (2023-10-04 15:19:00 -0400) > > > > > > > > > Python pullreq > > > > > > Buffering improvements for qemu machine, minor changes to support the > > > newly released Python 3.12 > > > > > > > > > > > > John Snow (9): > > >Python/iotests: Add type hint for nbd module > > >python/machine: move socket setup out of _base_args property > > >python/machine: close sock_pair in cleanup path > > >python/console_socket: accept existing FD in initializer > > >python/machine: use socketpair() for console connections > > >python/machine: use socketpair() for qtest connection > > >python/machine: remove unused sock_dir argument > > >python/qmp: remove Server.wait_closed() call for Python 3.12 > > >Python: test Python 3.12 > > > > Is that a pull request or a patch series to be reviewed? > > Strange, some of the patches have Reviewed-by tags but others do not. > I could not find a "Add type hint for nbd module" patch on the mailing > list before this pull request, so I guess it hasn't been reviewed. > > I'll hold off from merging this series for now. > > John: Please make sure all patches have been on the mailing list for > review before sending a pull request. > > Stefan > Okey Dokey. --js
Re: [PULL 1/9] Python/iotests: Add type hint for nbd module
On Thu, Oct 5, 2023, 10:05 AM Eric Blake wrote:
> On Wed, Oct 04, 2023 at 03:46:05PM -0400, John Snow wrote:
> > The test bails gracefully if this module isn't installed, but linters
> > need a little help understanding that. It's enough to just declare the
> > type in this case.
> >
> > (Fixes pylint complaining about use of an uninitialized variable because
> > it isn't wise enough to understand the notrun call is noreturn.)
> >
> > Signed-off-by: John Snow
> > ---
> > tests/qemu-iotests/tests/nbd-multiconn | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
>
> Since there's questions about this pull request seeming to be the
> first time this patch has appeared on list, I'll go ahead and review
> it here on the assumption that a v2 pull request is warranted.
>
Sorry... Tried to "sneak" in some real trivial stuff, but didn't mean to
try and pull a fast one. I figured it'd get reviewed and then we'd merge. I
can see this sentiment is not a well expected one
> >
> > diff --git a/tests/qemu-iotests/tests/nbd-multiconn
> b/tests/qemu-iotests/tests/nbd-multiconn
> > index 478a1eaba27..7e686a786ea 100755
> > --- a/tests/qemu-iotests/tests/nbd-multiconn
> > +++ b/tests/qemu-iotests/tests/nbd-multiconn
> > @@ -20,6 +20,8 @@
> >
> > import os
> > from contextlib import contextmanager
> > +from types import ModuleType
> > +
> > import iotests
> > from iotests import qemu_img_create, qemu_io
> >
> > @@ -28,7 +30,7 @@ disk = os.path.join(iotests.test_dir, 'disk')
> > size = '4M'
> > nbd_sock = os.path.join(iotests.sock_dir, 'nbd_sock')
> > nbd_uri = 'nbd+unix:///{}?socket=' + nbd_sock
> > -
> > +nbd: ModuleType
>
> Is it possible to put this closer to the code actually using 'nbd', as
> in this region?
>
> | if __name__ == '__main__':
> | try:
> | # Easier to use libnbd than to try and set up parallel
> | # 'qemu-nbd --list' or 'qemu-io' processes, but not all systems
> | # have libnbd installed.
> | import nbd # type: ignore
>
> but then again, open_nbd() right after your current location utilizes
> the variable, so I guess not. I trust your judgment on silencing the
> linters, so whether or not you move it (if moving is even possible),
>
It might be possible to move things around to be more localized, but this
was the smallest possible diffstat.
It's not really about the type, the declaration also helps pylint not
complain the "potentially" illegal use. (type: ignore isn't sufficient.)
The alternative is, I think, using some try...except around the import up
at the top, and using a HAVE_NBDLIB variable that we use to exit early
instead. I think there's no real benefit to that much churn, and this gets
the job done with less.
It might be possible to teach pylint that notrun is a NORETURN function,
but I didn't explore that avenue.
> Reviewed-by: Eric Blake
>
Thanks, and apologies for the fastball.
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.
> Virtualization: qemu.org | libguestfs.org
>
>
Re: How to tame CI?
Vladimir Sementsov-Ogievskiy wrote: > On 26.07.23 16:32, Thomas Huth wrote: >> On 26/07/2023 15.00, Peter Maydell wrote: >>> On Wed, 26 Jul 2023 at 13:06, Juan Quintela wrote: To make things easier, this is the part that show how it breaks (this is the gcov test): 357/423 qemu:block / io-qcow2-copy-before-write ERROR 6.38s exit status 1 >>> PYTHON=/builds/juan.quintela/qemu/build/pyvenv/bin/python3 MALLOC_PERTURB_=44 /builds/juan.quintela/qemu/build/pyvenv/bin/python3 /builds/juan.quintela/qemu/build/../tests/qemu-iotests/check -tap -qcow2 copy-before-write --source-dir /builds/juan.quintela/qemu/tests/qemu-iotests --build-dir /builds/juan.quintela/qemu/build/tests/qemu-iotests ― ✀ ― stderr: --- /builds/juan.quintela/qemu/tests/qemu-iotests/tests/copy-before-write.out +++ /builds/juan.quintela/qemu/build/scratch/qcow2-file-copy-before-write/copy-before-write.out.bad @@ -1,5 +1,21 @@ - +...F +== +FAIL: test_timeout_break_snapshot (__main__.TestCbwError) +-- +Traceback (most recent call last): + File "/builds/juan.quintela/qemu/tests/qemu-iotests/tests/copy-before-write", line 210, in test_timeout_break_snapshot + self.assertEqual(log, """\ +AssertionError: 'wrot[195 chars]read 1048576/1048576 bytes at offset 0\n1 MiB,[46 chars]c)\n' != 'wrot[195 chars]read failed: Permission denied\n' + wrote 524288/524288 bytes at offset 0 + 512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + wrote 524288/524288 bytes at offset 524288 + 512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) ++ read failed: Permission denied +- read 1048576/1048576 bytes at offset 0 +- 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + >>> >>> This iotest failing is an intermittent that I've seen running >>> pullreqs on master. I tend to see it on the s390 host. I >>> suspect a race condition somewhere where it fails if the host >>> is heavily loaded. >> It's obviously a failure in an iotest, so let's CC: the >> corresponding people (done now). >> > > Sorry for long delay. > > Does it still fail? > > In the test we expect that copy-before-write operation fails (because > of throttling and timeout), and therefore snapshot is broken and next > read from snapshot should fail. > > But most probably the copy-before-write operation succeeded in this > case for some reason.. I don't think that throttling and timeouts in > block layer can guarantee some determinism.. But usually it works. > > we use throttling with bps-write = 300 * 1024, i.e. 300KB per second. and > cbw-timeout is set to 1 second. > > Then we do write 512K, > > then the comment say: > # We need second write to trigger throttling > > and we write another 512K. > > first 512K are written, and we should wait 512/300 = 1.7 seconds since > _start_ of that write before issuing the second one.. But if write was > slow we may have to wait less than a second from finish of the first > write start the second one. Then timeout will not fire. > > > > I see two possible ways to fix that: > > 1. decrease bps-write a bit. For example to 200 BPS. > > 2. rework the test to use null-co instead of real images. This way we will > not suffer from unstable IO duration. > > > So, is the problem still fire sometimes? For me it is random. When it happens, it do it forever. And then it stops, and don't happens for a while. It is not happening for me now. Later, Juan.
Re: [PATCH v7 12/12] nbd/server: Add FLAG_PAYLOAD support to CMD_BLOCK_STATUS
On 25.09.23 22:22, Eric Blake wrote:
Allow a client to request a subset of negotiated meta contexts. For
example, a client may ask to use a single connection to learn about
both block status and dirty bitmaps, but where the dirty bitmap
queries only need to be performed on a subset of the disk; forcing the
server to compute that information on block status queries in the rest
of the disk is wasted effort (both at the server, and on the amount of
traffic sent over the wire to be parsed and ignored by the client).
Qemu as an NBD client never requests to use more than one meta
context, so it has no need to use block status payloads. Testing this
instead requires support from libnbd, which CAN access multiple meta
contexts in parallel from a single NBD connection; an interop test
submitted to the libnbd project at the same time as this patch
demonstrates the feature working, as well as testing some corner cases
(for example, when the payload length is longer than the export
length), although other corner cases (like passing the same id
duplicated) requires a protocol fuzzer because libnbd is not wired up
to break the protocol that badly.
This also includes tweaks to 'qemu-nbd --list' to show when a server
is advertising the capability, and to the testsuite to reflect the
addition to that output.
Of note: qemu will always advertise the new feature bit during
NBD_OPT_INFO if extended headers have alreay been negotiated
(regardless of whether any NBD_OPT_SET_META_CONTEXT negotiation has
occurred); but for NBD_OPT_GO, qemu only advertises the feature if
block status is also enabled (that is, if the client does not
negotiate any contexts, then NBD_CMD_BLOCK_STATUS cannot be used, so
the feature is not advertised).
Signed-off-by: Eric Blake
---
v5: factor out 'id - NBD_MTA_ID_DIRTY_BITMAP' [Vladimir], rework logic
on zero-length requests to be clearer [Vladimir], rebase to earlier
changes
[..]
+/*
+ * nbd_co_block_status_payload_read
+ * Called when a client wants a subset of negotiated contexts via a
+ * BLOCK_STATUS payload. Check the payload for valid length and
+ * contents. On success, return 0 with request updated to effective
+ * length. If request was invalid but all payload consumed, return 0
+ * with request->len and request->contexts->count set to 0 (which will
+ * trigger an appropriate NBD_EINVAL response later on). Return
+ * negative errno if the payload was not fully consumed.
+ */
+static int
+nbd_co_block_status_payload_read(NBDClient *client, NBDRequest *request,
+ Error **errp)
+{
+int payload_len = request->len;
payload_len should be uint64_t
+g_autofree char *buf = NULL;
+size_t count, i, nr_bitmaps;
+uint32_t id;
+
otherwise, we may do something unexpected here, when reqeuest->len is too big
for int:
+if (payload_len > NBD_MAX_BUFFER_SIZE) {
+error_setg(errp, "len (%" PRIu64 ") is larger than max len (%u)",
+ request->len, NBD_MAX_BUFFER_SIZE);
+return -EINVAL;
+}
+
+assert(client->contexts.exp == client->exp);
+nr_bitmaps = client->exp->nr_export_bitmaps;
+request->contexts = g_new0(NBDMetaContexts, 1);
+request->contexts->exp = client->exp;
+
+if (payload_len % sizeof(uint32_t) ||
+payload_len < sizeof(NBDBlockStatusPayload) ||
+payload_len > (sizeof(NBDBlockStatusPayload) +
+ sizeof(id) * client->contexts.count)) {
+goto skip;
+}
[..]
* connection right away, -EAGAIN to indicate we were interrupted and the
@@ -2505,7 +2593,18 @@ static int coroutine_fn
nbd_co_receive_request(NBDRequestData *req,
break;
case NBD_CMD_BLOCK_STATUS:
-request->contexts = >contexts;
+if (extended_with_payload) {
+ret = nbd_co_block_status_payload_read(client, request, errp);
+if (ret < 0) {
+return ret;
+}
+/* payload now consumed */
+check_length = extended_with_payload = false;
why set extended_with_payload to false? it's a bit misleading. And you don't do
this for WRITE request.
+payload_len = 0;
+valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN;
+} else {
+request->contexts = >contexts;
+}
valid_flags |= NBD_CMD_FLAG_REQ_ONE;
break;
[..]
with payload_len changed to uint64_t, your squash-in applied and s/>/>=/ fixed:
Reviewed-by: Vladimir Sementsov-Ogievskiy
--
Best regards,
Vladimir
Re: [Libguestfs] [PATCH v7 12/12] nbd/server: Add FLAG_PAYLOAD support to CMD_BLOCK_STATUS
On 05.10.23 16:49, Eric Blake wrote:
On Wed, Oct 04, 2023 at 04:55:02PM -0500, Eric Blake wrote:
+static int
+nbd_co_block_status_payload_read(NBDClient *client, NBDRequest *request,
+ Error **errp)
[..]
+for (i = 0; i < count; i++) {
+id = ldl_be_p(buf + sizeof(NBDBlockStatusPayload) + sizeof(id) * i);
+if (id == NBD_META_ID_BASE_ALLOCATION) {
+if (request->contexts->base_allocation) {
+goto skip;
+}
should we also check that base_allocation is negotiated?
Oh, good point. Without that check, the client can pass in random id
numbers that it never negotiated. I've queued 1-11 and will probably
send a pull request for those this week, while respinning this patch
to fix the remaining issues you pointed out.
I'm squashing in the following. If you can review it today, I'll
include it in my pull request this afternoon; if not, we still have
time before soft freeze to get it in the next batch.
diff --git i/nbd/server.c w/nbd/server.c
index 30816b42386..62654579cbc 100644
--- i/nbd/server.c
+++ w/nbd/server.c
@@ -2478,19 +2478,22 @@ nbd_co_block_status_payload_read(NBDClient *client,
NBDRequest *request,
for (i = 0; i < count; i++) {
id = ldl_be_p(buf + sizeof(NBDBlockStatusPayload) + sizeof(id) * i);
if (id == NBD_META_ID_BASE_ALLOCATION) {
-if (request->contexts->base_allocation) {
+if (!client->contexts.base_allocation ||
+request->contexts->base_allocation) {
goto skip;
}
request->contexts->base_allocation = true;
} else if (id == NBD_META_ID_ALLOCATION_DEPTH) {
-if (request->contexts->allocation_depth) {
+if (!client->contexts.allocation_depth ||
+request->contexts->allocation_depth) {
goto skip;
}
request->contexts->allocation_depth = true;
} else {
-int idx = id - NBD_META_ID_DIRTY_BITMAP;
+unsigned idx = id - NBD_META_ID_DIRTY_BITMAP;
-if (idx > nr_bitmaps || request->contexts->bitmaps[idx]) {
+if (idx > nr_bitmaps || !client->contexts.bitmaps[idx] ||
Oops, I didn't notice: s/>/>=/, as nr_bitmaps is length of array.
+request->contexts->bitmaps[idx]) {
goto skip;
}
request->contexts->bitmaps[idx] = true;
diff --git i/nbd/trace-events w/nbd/trace-events
index 3cf2d00e458..00ae3216a11 100644
--- i/nbd/trace-events
+++ w/nbd/trace-events
@@ -70,7 +70,7 @@ nbd_co_send_chunk_read(uint64_t cookie, uint64_t offset, void
*data, uint64_t si
nbd_co_send_chunk_read_hole(uint64_t cookie, uint64_t offset, uint64_t size) "Send structured read
hole reply: cookie = %" PRIu64 ", offset = %" PRIu64 ", len = %" PRIu64
nbd_co_send_extents(uint64_t cookie, unsigned int extents, uint32_t id, uint64_t length, int last)
"Send block status reply: cookie = %" PRIu64 ", extents = %u, context = %d (extents cover
%" PRIu64 " bytes, last chunk = %d)"
nbd_co_send_chunk_error(uint64_t cookie, int err, const char *errname, const char *msg)
"Send structured error reply: cookie = %" PRIu64 ", error = %d (%s), msg = '%s'"
-nbd_co_receive_block_status_payload_compliance(uint64_t from, int len) "client sent unusable
block status payload: from=0x%" PRIx64 ", len=0x%x"
+nbd_co_receive_block_status_payload_compliance(uint64_t from, uint64_t len) "client sent
unusable block status payload: from=0x%" PRIx64 ", len=0x%" PRIx64
nbd_co_receive_request_decode_type(uint64_t cookie, uint16_t type, const char *name) "Decoding type:
cookie = %" PRIu64 ", type = %" PRIu16 " (%s)"
nbd_co_receive_request_payload_received(uint64_t cookie, uint64_t len) "Payload received:
cookie = %" PRIu64 ", len = %" PRIu64
nbd_co_receive_ext_payload_compliance(uint64_t from, uint64_t len) "client sent
non-compliant write without payload flag: from=0x%" PRIx64 ", len=0x%" PRIx64
--
Best regards,
Vladimir
Re: [PATCH v6 02/14] qmp_shell.py: _fill_completion() use .command() instead of .cmd()
On Thu, Oct 05, 2023 at 04:55:38PM +0300, Vladimir Sementsov-Ogievskiy wrote: > We just want to ignore failure, so we don't need low level .cmd(). This > helps further renaming .command() to .cmd(). > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > python/qemu/qmp/qmp_shell.py | 20 ++-- > 1 file changed, 14 insertions(+), 6 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
Re: [PATCH v6 01/14] python/qemu/qmp/legacy: cmd(): drop cmd_id unused argument
On Thu, Oct 05, 2023 at 04:55:37PM +0300, Vladimir Sementsov-Ogievskiy wrote: > The argument is unused, let's drop it for now, as we are going to > refactor the interface and don't want to refactor unused things. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > python/qemu/qmp/legacy.py | 6 +- > 1 file changed, 1 insertion(+), 5 deletions(-) Reviewed-by: Eric Blake Although I welcome John's opinion as to how long we have to maintain legacy.py at all, given that its name implies there is a better thing we should have converted to already. -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
Re: [PULL 1/9] Python/iotests: Add type hint for nbd module
On Wed, Oct 04, 2023 at 03:46:05PM -0400, John Snow wrote:
> The test bails gracefully if this module isn't installed, but linters
> need a little help understanding that. It's enough to just declare the
> type in this case.
>
> (Fixes pylint complaining about use of an uninitialized variable because
> it isn't wise enough to understand the notrun call is noreturn.)
>
> Signed-off-by: John Snow
> ---
> tests/qemu-iotests/tests/nbd-multiconn | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
Since there's questions about this pull request seeming to be the
first time this patch has appeared on list, I'll go ahead and review
it here on the assumption that a v2 pull request is warranted.
>
> diff --git a/tests/qemu-iotests/tests/nbd-multiconn
> b/tests/qemu-iotests/tests/nbd-multiconn
> index 478a1eaba27..7e686a786ea 100755
> --- a/tests/qemu-iotests/tests/nbd-multiconn
> +++ b/tests/qemu-iotests/tests/nbd-multiconn
> @@ -20,6 +20,8 @@
>
> import os
> from contextlib import contextmanager
> +from types import ModuleType
> +
> import iotests
> from iotests import qemu_img_create, qemu_io
>
> @@ -28,7 +30,7 @@ disk = os.path.join(iotests.test_dir, 'disk')
> size = '4M'
> nbd_sock = os.path.join(iotests.sock_dir, 'nbd_sock')
> nbd_uri = 'nbd+unix:///{}?socket=' + nbd_sock
> -
> +nbd: ModuleType
Is it possible to put this closer to the code actually using 'nbd', as
in this region?
| if __name__ == '__main__':
| try:
| # Easier to use libnbd than to try and set up parallel
| # 'qemu-nbd --list' or 'qemu-io' processes, but not all systems
| # have libnbd installed.
| import nbd # type: ignore
but then again, open_nbd() right after your current location utilizes
the variable, so I guess not. I trust your judgment on silencing the
linters, so whether or not you move it (if moving is even possible),
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
[PATCH v6 05/14] python/qemu: rename command() to cmd()
Use a shorter name. We are going to move in iotests from qmp() to
command() where possible. But command() is longer than qmp() and don't
look better. Let's rename.
You can simply grep for '\.command(' and for 'def command(' to check
that everything is updated (command() in tests/docker/docker.py is
unrelated).
Signed-off-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Daniel P. Berrangé
---
docs/devel/testing.rst| 10 +-
python/qemu/machine/machine.py| 8 +-
python/qemu/qmp/legacy.py | 2 +-
python/qemu/qmp/qmp_shell.py | 2 +-
python/qemu/utils/qemu_ga_client.py | 2 +-
python/qemu/utils/qom.py | 8 +-
python/qemu/utils/qom_common.py | 2 +-
python/qemu/utils/qom_fuse.py | 6 +-
scripts/cpu-x86-uarch-abi.py | 8 +-
scripts/device-crash-test | 8 +-
scripts/render_block_graph.py | 8 +-
tests/avocado/avocado_qemu/__init__.py| 4 +-
tests/avocado/cpu_queries.py | 5 +-
tests/avocado/hotplug_cpu.py | 10 +-
tests/avocado/info_usernet.py | 4 +-
tests/avocado/machine_arm_integratorcp.py | 6 +-
tests/avocado/machine_m68k_nextcube.py| 4 +-
tests/avocado/machine_mips_malta.py | 6 +-
tests/avocado/machine_s390_ccw_virtio.py | 28 ++--
tests/avocado/migration.py| 10 +-
tests/avocado/pc_cpu_hotplug_props.py | 2 +-
tests/avocado/version.py | 4 +-
tests/avocado/virtio_check_params.py | 6 +-
tests/avocado/virtio_version.py | 5 +-
tests/avocado/x86_cpu_model_versions.py | 13 +-
tests/migration/guestperf/engine.py | 150 +++---
tests/qemu-iotests/256| 34 ++---
tests/qemu-iotests/257| 36 +++---
28 files changed, 198 insertions(+), 193 deletions(-)
diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index 5d1fc0aa95..21525e9aae 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -1014,8 +1014,8 @@ class. Here's a simple usage example:
"""
def test_qmp_human_info_version(self):
self.vm.launch()
- res = self.vm.command('human-monitor-command',
-command_line='info version')
+ res = self.vm.cmd('human-monitor-command',
+command_line='info version')
self.assertRegexpMatches(res, r'^(\d+\.\d+\.\d)')
To execute your test, run:
@@ -1065,15 +1065,15 @@ and hypothetical example follows:
first_machine.launch()
second_machine.launch()
- first_res = first_machine.command(
+ first_res = first_machine.cmd(
'human-monitor-command',
command_line='info version')
- second_res = second_machine.command(
+ second_res = second_machine.cmd(
'human-monitor-command',
command_line='info version')
- third_res = self.get_vm(name='third_machine').command(
+ third_res = self.get_vm(name='third_machine').cmd(
'human-monitor-command',
command_line='info version')
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index dd1a79cb37..c4e80544bd 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -697,16 +697,16 @@ def qmp(self, cmd: str,
self._quit_issued = True
return ret
-def command(self, cmd: str,
-conv_keys: bool = True,
-**args: Any) -> QMPReturnValue:
+def cmd(self, cmd: str,
+conv_keys: bool = True,
+**args: Any) -> QMPReturnValue:
"""
Invoke a QMP command.
On success return the response dict.
On failure raise an exception.
"""
qmp_args = self._qmp_args(conv_keys, args)
-ret = self._qmp.command(cmd, **qmp_args)
+ret = self._qmp.cmd(cmd, **qmp_args)
if cmd == 'quit':
self._quit_issued = True
return ret
diff --git a/python/qemu/qmp/legacy.py b/python/qemu/qmp/legacy.py
index e5fa1ce9c4..22a2b5616e 100644
--- a/python/qemu/qmp/legacy.py
+++ b/python/qemu/qmp/legacy.py
@@ -207,7 +207,7 @@ def cmd_raw(self, name: str,
qmp_cmd['arguments'] = args
return self.cmd_obj(qmp_cmd)
-def command(self, cmd: str, **kwds: object) -> QMPReturnValue:
+def cmd(self, cmd: str, **kwds: object) -> QMPReturnValue:
"""
Build and send a QMP command to the monitor, report errors if any
"""
diff --git a/python/qemu/qmp/qmp_shell.py b/python/qemu/qmp/qmp_shell.py
index 988d79c01b..98e684e9e8 100644
--- a/python/qemu/qmp/qmp_shell.py
+++ b/python/qemu/qmp/qmp_shell.py
@@ -202,7 +202,7 @@ def close(self) -> None:
def _fill_completion(self) -> None:
try:
-cmds =
[PATCH v6 04/14] python: rename QEMUMonitorProtocol.cmd() to cmd_raw()
Having cmd() and command() methods in one class doesn't look good. Rename cmd() to cmd_raw(), to show its meaning better. We also want to rename command() to cmd() in future, so this commit is a necessary step. Signed-off-by: Vladimir Sementsov-Ogievskiy --- python/qemu/machine/machine.py | 2 +- python/qemu/qmp/legacy.py | 4 ++-- tests/qemu-iotests/iotests.py | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py index 35d5a672db..dd1a79cb37 100644 --- a/python/qemu/machine/machine.py +++ b/python/qemu/machine/machine.py @@ -692,7 +692,7 @@ def qmp(self, cmd: str, conv_keys = True qmp_args = self._qmp_args(conv_keys, args) -ret = self._qmp.cmd(cmd, args=qmp_args) +ret = self._qmp.cmd_raw(cmd, args=qmp_args) if cmd == 'quit' and 'error' not in ret and 'return' in ret: self._quit_issued = True return ret diff --git a/python/qemu/qmp/legacy.py b/python/qemu/qmp/legacy.py index fe115e301c..e5fa1ce9c4 100644 --- a/python/qemu/qmp/legacy.py +++ b/python/qemu/qmp/legacy.py @@ -194,8 +194,8 @@ def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage: ) ) -def cmd(self, name: str, -args: Optional[Dict[str, object]] = None) -> QMPMessage: +def cmd_raw(self, name: str, +args: Optional[Dict[str, object]] = None) -> QMPMessage: """ Build a QMP command and send it to the QMP Monitor. diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index ef66fbd62b..8ffd9fb660 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -460,7 +460,7 @@ def __init__(self, *args: str, instance_id: str = 'a', qmp: bool = False): def qmp(self, cmd: str, args: Optional[Dict[str, object]] = None) \ -> QMPMessage: assert self._qmp is not None -return self._qmp.cmd(cmd, args) +return self._qmp.cmd_raw(cmd, args) def get_qmp(self) -> QEMUMonitorProtocol: assert self._qmp is not None -- 2.34.1
[PATCH v6 01/14] python/qemu/qmp/legacy: cmd(): drop cmd_id unused argument
The argument is unused, let's drop it for now, as we are going to
refactor the interface and don't want to refactor unused things.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
python/qemu/qmp/legacy.py | 6 +-
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/python/qemu/qmp/legacy.py b/python/qemu/qmp/legacy.py
index e1e9383978..fe115e301c 100644
--- a/python/qemu/qmp/legacy.py
+++ b/python/qemu/qmp/legacy.py
@@ -195,20 +195,16 @@ def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage:
)
def cmd(self, name: str,
-args: Optional[Dict[str, object]] = None,
-cmd_id: Optional[object] = None) -> QMPMessage:
+args: Optional[Dict[str, object]] = None) -> QMPMessage:
"""
Build a QMP command and send it to the QMP Monitor.
:param name: command name (string)
:param args: command arguments (dict)
-:param cmd_id: command id (dict, list, string or int)
"""
qmp_cmd: QMPMessage = {'execute': name}
if args:
qmp_cmd['arguments'] = args
-if cmd_id:
-qmp_cmd['id'] = cmd_id
return self.cmd_obj(qmp_cmd)
def command(self, cmd: str, **kwds: object) -> QMPReturnValue:
--
2.34.1
[PATCH v6 13/14] tests/vm/basevm.py: use cmd() instead of qmp()
We don't expect failure here and need 'result' object. cmd() is better
in this case.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
tests/vm/basevm.py | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
index a97e23b0ce..8aef4cff96 100644
--- a/tests/vm/basevm.py
+++ b/tests/vm/basevm.py
@@ -312,8 +312,8 @@ def boot(self, img, extra_args=[]):
self._guest = guest
# Init console so we can start consuming the chars.
self.console_init()
-usernet_info = guest.qmp("human-monitor-command",
- command_line="info usernet").get("return")
+usernet_info = guest.cmd("human-monitor-command",
+ command_line="info usernet")
self.ssh_port = get_info_usernet_hostfwd_port(usernet_info)
if not self.ssh_port:
raise Exception("Cannot find ssh port from 'info usernet':\n%s" % \
--
2.34.1
[PATCH v6 09/14] iotests: refactor some common qmp result checks into generic pattern
To simplify further conversion.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
tests/qemu-iotests/040 | 3 ++-
tests/qemu-iotests/147 | 3 ++-
tests/qemu-iotests/155 | 4 ++--
tests/qemu-iotests/218 | 4 ++--
tests/qemu-iotests/296 | 3 ++-
5 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 5601a4873c..e61e7f2433 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -62,9 +62,10 @@ class ImageCommitTestCase(iotests.QMPTestCase):
self.assert_no_active_block_jobs()
if node_names:
result = self.vm.qmp('block-commit', device='drive0',
top_node=top, base_node=base)
+self.assert_qmp(result, 'return', {})
else:
result = self.vm.qmp('block-commit', device='drive0', top=top,
base=base)
-self.assert_qmp(result, 'return', {})
+self.assert_qmp(result, 'return', {})
self.wait_for_complete(need_ready)
def run_default_commit_test(self):
diff --git a/tests/qemu-iotests/147 b/tests/qemu-iotests/147
index 47dfa62e6b..770b73e2f4 100755
--- a/tests/qemu-iotests/147
+++ b/tests/qemu-iotests/147
@@ -159,10 +159,11 @@ class BuiltinNBD(NBDBlockdevAddBase):
if export_name is None:
result = self.server.qmp('nbd-server-add', device='nbd-export')
+self.assert_qmp(result, 'return', {})
else:
result = self.server.qmp('nbd-server-add', device='nbd-export',
name=export_name)
-self.assert_qmp(result, 'return', {})
+self.assert_qmp(result, 'return', {})
if export_name2 is not None:
result = self.server.qmp('nbd-server-add', device='nbd-export',
diff --git a/tests/qemu-iotests/155 b/tests/qemu-iotests/155
index eadda52615..d3e1b7401e 100755
--- a/tests/qemu-iotests/155
+++ b/tests/qemu-iotests/155
@@ -181,6 +181,7 @@ class MirrorBaseClass(BaseClass):
result = self.vm.qmp(self.cmd, job_id='mirror-job',
device='source',
sync=sync, target='target',
auto_finalize=False)
+self.assert_qmp(result, 'return', {})
else:
if self.existing:
mode = 'existing'
@@ -190,8 +191,7 @@ class MirrorBaseClass(BaseClass):
sync=sync, target=target_img,
format=iotests.imgfmt, mode=mode,
node_name='target', auto_finalize=False)
-
-self.assert_qmp(result, 'return', {})
+self.assert_qmp(result, 'return', {})
self.vm.run_job('mirror-job', auto_finalize=False,
pre_finalize=self.openBacking, auto_dismiss=True)
diff --git a/tests/qemu-iotests/218 b/tests/qemu-iotests/218
index 6320c4cb56..5e74c55b6a 100755
--- a/tests/qemu-iotests/218
+++ b/tests/qemu-iotests/218
@@ -61,14 +61,14 @@ def start_mirror(vm, speed=None, buf_size=None):
sync='full',
speed=speed,
buf_size=buf_size)
+assert ret['return'] == {}
else:
ret = vm.qmp('blockdev-mirror',
job_id='mirror',
device='source',
target='target',
sync='full')
-
-assert ret['return'] == {}
+assert ret['return'] == {}
log('')
diff --git a/tests/qemu-iotests/296 b/tests/qemu-iotests/296
index 0d21b740a7..19a674c5ae 100755
--- a/tests/qemu-iotests/296
+++ b/tests/qemu-iotests/296
@@ -133,9 +133,10 @@ class EncryptionSetupTestCase(iotests.QMPTestCase):
if reOpen:
result = vm.qmp(command, options=[opts])
+self.assert_qmp(result, 'return', {})
else:
result = vm.qmp(command, **opts)
-self.assert_qmp(result, 'return', {})
+self.assert_qmp(result, 'return', {})
###
--
2.34.1
[PATCH v6 07/14] iotests: QemuStorageDaemon: add cmd() method like in QEMUMachine.
Add similar method for consistency.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
tests/qemu-iotests/iotests.py | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 8ffd9fb660..6cc50f0b50 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -38,7 +38,7 @@
from contextlib import contextmanager
from qemu.machine import qtest
-from qemu.qmp.legacy import QMPMessage, QEMUMonitorProtocol
+from qemu.qmp.legacy import QMPMessage, QMPReturnValue, QEMUMonitorProtocol
from qemu.utils import VerboseProcessError
# Use this logger for logging messages directly from the iotests module
@@ -466,6 +466,11 @@ def get_qmp(self) -> QEMUMonitorProtocol:
assert self._qmp is not None
return self._qmp
+def cmd(self, cmd: str, args: Optional[Dict[str, object]] = None) \
+-> QMPReturnValue:
+assert self._qmp is not None
+return self._qmp.cmd(cmd, **(args or {}))
+
def stop(self, kill_signal=15):
self._p.send_signal(kill_signal)
self._p.wait()
--
2.34.1
[PATCH v6 08/14] iotests: add some missed checks of qmp result
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
tests/qemu-iotests/041| 1 +
tests/qemu-iotests/151| 1 +
tests/qemu-iotests/152| 2 ++
tests/qemu-iotests/tests/migrate-bitmaps-test | 2 ++
4 files changed, 6 insertions(+)
diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 8429958bf0..4d7a829b65 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -1087,6 +1087,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
result = self.vm.qmp('blockdev-snapshot-sync', node_name='img1',
snapshot_file=quorum_snapshot_file,
snapshot_node_name="snap1");
+self.assert_qmp(result, 'return', {})
result = self.vm.qmp('drive-mirror', job_id='job0', device='quorum0',
sync='full', node_name='repair0', replaces="img1",
diff --git a/tests/qemu-iotests/151 b/tests/qemu-iotests/151
index b4d1bc2553..668d0c1e9c 100755
--- a/tests/qemu-iotests/151
+++ b/tests/qemu-iotests/151
@@ -159,6 +159,7 @@ class TestActiveMirror(iotests.QMPTestCase):
sync='full',
copy_mode='write-blocking',
speed=1)
+self.assert_qmp(result, 'return', {})
self.vm.hmp_qemu_io('source', 'break write_aio A')
self.vm.hmp_qemu_io('source', 'aio_write 0 1M') # 1
diff --git a/tests/qemu-iotests/152 b/tests/qemu-iotests/152
index 4e179c340f..b73a0d08a2 100755
--- a/tests/qemu-iotests/152
+++ b/tests/qemu-iotests/152
@@ -43,6 +43,7 @@ class TestUnaligned(iotests.QMPTestCase):
def test_unaligned(self):
result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
granularity=65536, target=target_img)
+self.assert_qmp(result, 'return', {})
self.complete_and_wait()
self.vm.shutdown()
self.assertEqual(iotests.image_size(test_img),
iotests.image_size(target_img),
@@ -51,6 +52,7 @@ class TestUnaligned(iotests.QMPTestCase):
def test_unaligned_with_update(self):
result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
granularity=65536, target=target_img)
+self.assert_qmp(result, 'return', {})
self.wait_ready()
self.vm.hmp_qemu_io('drive0', 'write 0 512')
self.complete_and_wait(wait_ready=False)
diff --git a/tests/qemu-iotests/tests/migrate-bitmaps-test
b/tests/qemu-iotests/tests/migrate-bitmaps-test
index 59f3357580..8668caae1e 100755
--- a/tests/qemu-iotests/tests/migrate-bitmaps-test
+++ b/tests/qemu-iotests/tests/migrate-bitmaps-test
@@ -101,6 +101,7 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
sha256 = get_bitmap_hash(self.vm_a)
result = self.vm_a.qmp('migrate', uri=mig_cmd)
+self.assert_qmp(result, 'return', {})
while True:
event = self.vm_a.event_wait('MIGRATION')
if event['data']['status'] == 'completed':
@@ -176,6 +177,7 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
self.assert_qmp(result, 'return', {})
result = self.vm_a.qmp('migrate', uri=mig_cmd)
+self.assert_qmp(result, 'return', {})
while True:
event = self.vm_a.event_wait('MIGRATION')
if event['data']['status'] == 'completed':
--
2.34.1
[PATCH v6 12/14] iotests.py: pause_job(): drop return value
The returned value is unused. It's simple to check by command
git grep -B 3 '\.pause_job('
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
tests/qemu-iotests/iotests.py | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 6cc50f0b50..467faca43c 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -1338,8 +1338,7 @@ def pause_job(self, job_id='job0', wait=True):
result = self.vm.qmp('block-job-pause', device=job_id)
self.assert_qmp(result, 'return', {})
if wait:
-return self.pause_wait(job_id)
-return result
+self.pause_wait(job_id)
def case_skip(self, reason):
'''Skip this test case'''
--
2.34.1
[PATCH v6 06/14] python/machine.py: upgrade vm.cmd() method
The method is not popular in iotests, we prefer use vm.qmp() and then check success by hand.. But that's not optimal. To simplify movement to vm.cmd() let's support same interface improvements like in vm.qmp(). Signed-off-by: Vladimir Sementsov-Ogievskiy --- python/qemu/machine/machine.py | 12 +++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py index c4e80544bd..352e15b074 100644 --- a/python/qemu/machine/machine.py +++ b/python/qemu/machine/machine.py @@ -698,13 +698,23 @@ def qmp(self, cmd: str, return ret def cmd(self, cmd: str, -conv_keys: bool = True, +args_dict: Optional[Dict[str, object]] = None, +conv_keys: Optional[bool] = None, **args: Any) -> QMPReturnValue: """ Invoke a QMP command. On success return the response dict. On failure raise an exception. """ +if args_dict is not None: +assert not args +assert conv_keys is None +args = args_dict +conv_keys = False + +if conv_keys is None: +conv_keys = True + qmp_args = self._qmp_args(conv_keys, args) ret = self._qmp.cmd(cmd, **qmp_args) if cmd == 'quit': -- 2.34.1
[PATCH v6 11/14] iotests: drop some extra ** in qmp() call
qmp() method supports passing dict (if it doesn't need substituting
'_' to '-' in keys). So, drop some extra '**' operators.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
tests/qemu-iotests/040| 4 +-
tests/qemu-iotests/041| 14 +++---
tests/qemu-iotests/129| 2 +-
tests/qemu-iotests/147| 2 +-
tests/qemu-iotests/155| 2 +-
tests/qemu-iotests/264| 12 ++---
tests/qemu-iotests/295| 5 +-
tests/qemu-iotests/296| 15 +++---
tests/qemu-iotests/tests/migrate-bitmaps-test | 4 +-
.../tests/mirror-ready-cancel-error | 50 +--
10 files changed, 54 insertions(+), 56 deletions(-)
diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index e61e7f2433..4b8bf09a5d 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -774,7 +774,7 @@ class TestCommitWithFilters(iotests.QMPTestCase):
result = self.vm.qmp('object-add', qom_type='throttle-group', id='tg')
self.assert_qmp(result, 'return', {})
-result = self.vm.qmp('blockdev-add', **{
+result = self.vm.qmp('blockdev-add', {
'node-name': 'top-filter',
'driver': 'throttle',
'throttle-group': 'tg',
@@ -935,7 +935,7 @@ class TestCommitWithOverriddenBacking(iotests.QMPTestCase):
self.vm.launch()
# Use base_b instead of base_a as the backing of top
-result = self.vm.qmp('blockdev-add', **{
+result = self.vm.qmp('blockdev-add', {
'node-name': 'top',
'driver': iotests.imgfmt,
'file': {
diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 550e4dc391..3aef42aec8 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -236,7 +236,7 @@ class TestSingleBlockdev(TestSingleDrive):
args = {'driver': iotests.imgfmt,
'node-name': self.qmp_target,
'file': { 'filename': target_img, 'driver': 'file' } }
-result = self.vm.qmp("blockdev-add", **args)
+result = self.vm.qmp("blockdev-add", args)
self.assert_qmp(result, 'return', {})
def test_mirror_to_self(self):
@@ -963,7 +963,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
#assemble the quorum block device from the individual files
args = { "driver": "quorum", "node-name": "quorum0",
"vote-threshold": 2, "children": [ "img0", "img1", "img2" ] }
-result = self.vm.qmp("blockdev-add", **args)
+result = self.vm.qmp("blockdev-add", args)
self.assert_qmp(result, 'return', {})
@@ -1278,7 +1278,7 @@ class TestReplaces(iotests.QMPTestCase):
"""
Check that we can replace filter nodes.
"""
-result = self.vm.qmp('blockdev-add', **{
+result = self.vm.qmp('blockdev-add', {
'driver': 'copy-on-read',
'node-name': 'filter0',
'file': {
@@ -1319,7 +1319,7 @@ class TestFilters(iotests.QMPTestCase):
self.vm = iotests.VM().add_device('virtio-scsi,id=vio-scsi')
self.vm.launch()
-result = self.vm.qmp('blockdev-add', **{
+result = self.vm.qmp('blockdev-add', {
'node-name': 'target',
'driver': iotests.imgfmt,
'file': {
@@ -1355,7 +1355,7 @@ class TestFilters(iotests.QMPTestCase):
os.remove(backing_img)
def test_cor(self):
-result = self.vm.qmp('blockdev-add', **{
+result = self.vm.qmp('blockdev-add', {
'node-name': 'filter',
'driver': 'copy-on-read',
'file': self.filterless_chain
@@ -1384,7 +1384,7 @@ class TestFilters(iotests.QMPTestCase):
assert target_map[1]['depth'] == 0
def test_implicit_mirror_filter(self):
-result = self.vm.qmp('blockdev-add', **self.filterless_chain)
+result = self.vm.qmp('blockdev-add', self.filterless_chain)
self.assert_qmp(result, 'return', {})
# We need this so we can query from above the mirror node
@@ -1418,7 +1418,7 @@ class TestFilters(iotests.QMPTestCase):
def test_explicit_mirror_filter(self):
# Same test as above, but this time we give the mirror filter
# a node-name so it will not be invisible
-result = self.vm.qmp('blockdev-add', **self.filterless_chain)
+result = self.vm.qmp('blockdev-add', self.filterless_chain)
self.assert_qmp(result, 'return', {})
# We need this so we can query from above the mirror node
diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index
[PATCH v6 03/14] scripts/cpu-x86-uarch-abi.py: use .command() instead of .cmd()
Here we don't expect a failure. In case on failure we'll crash on
trying to access ['return']. Let's better use .command() that clearly
raise on failure.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
scripts/cpu-x86-uarch-abi.py | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/scripts/cpu-x86-uarch-abi.py b/scripts/cpu-x86-uarch-abi.py
index 82ff07582f..893afd1b35 100644
--- a/scripts/cpu-x86-uarch-abi.py
+++ b/scripts/cpu-x86-uarch-abi.py
@@ -69,7 +69,7 @@
shell = QEMUMonitorProtocol(sock)
shell.connect()
-models = shell.cmd("query-cpu-definitions")
+models = shell.command("query-cpu-definitions")
# These QMP props don't correspond to CPUID fatures
# so ignore them
@@ -85,7 +85,7 @@
names = []
-for model in models["return"]:
+for model in models:
if "alias-of" in model:
continue
names.append(model["name"])
@@ -93,12 +93,12 @@
models = {}
for name in sorted(names):
-cpu = shell.cmd("query-cpu-model-expansion",
- { "type": "static",
- "model": { "name": name }})
+cpu = shell.command("query-cpu-model-expansion",
+{ "type": "static",
+ "model": { "name": name }})
got = {}
-for (feature, present) in cpu["return"]["model"]["props"].items():
+for (feature, present) in cpu["model"]["props"].items():
if present and feature not in skip:
got[feature] = True
--
2.34.1
[PATCH v6 02/14] qmp_shell.py: _fill_completion() use .command() instead of .cmd()
We just want to ignore failure, so we don't need low level .cmd(). This
helps further renaming .command() to .cmd().
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
python/qemu/qmp/qmp_shell.py | 20 ++--
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/python/qemu/qmp/qmp_shell.py b/python/qemu/qmp/qmp_shell.py
index 619ab42ced..988d79c01b 100644
--- a/python/qemu/qmp/qmp_shell.py
+++ b/python/qemu/qmp/qmp_shell.py
@@ -91,14 +91,21 @@
import sys
from typing import (
IO,
+Dict,
Iterator,
List,
NoReturn,
Optional,
Sequence,
+cast,
)
-from qemu.qmp import ConnectError, QMPError, SocketAddrT
+from qemu.qmp import (
+ConnectError,
+ExecuteError,
+QMPError,
+SocketAddrT,
+)
from qemu.qmp.legacy import (
QEMUMonitorProtocol,
QMPBadPortError,
@@ -194,11 +201,12 @@ def close(self) -> None:
super().close()
def _fill_completion(self) -> None:
-cmds = self.cmd('query-commands')
-if 'error' in cmds:
-return
-for cmd in cmds['return']:
-self._completer.append(cmd['name'])
+try:
+cmds = cast(List[Dict[str, str]], self.command('query-commands'))
+for cmd in cmds:
+self._completer.append(cmd['name'])
+except ExecuteError:
+pass
def _completer_setup(self) -> None:
self._completer = QMPCompleter()
--
2.34.1
[PATCH v6 10/14] iotests: drop some occasional semicolons
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
tests/qemu-iotests/041 | 2 +-
tests/qemu-iotests/196 | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 4d7a829b65..550e4dc391 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -1086,7 +1086,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
def test_after_a_quorum_snapshot(self):
result = self.vm.qmp('blockdev-snapshot-sync', node_name='img1',
snapshot_file=quorum_snapshot_file,
- snapshot_node_name="snap1");
+ snapshot_node_name="snap1")
self.assert_qmp(result, 'return', {})
result = self.vm.qmp('drive-mirror', job_id='job0', device='quorum0',
diff --git a/tests/qemu-iotests/196 b/tests/qemu-iotests/196
index 76509a5ad1..27c1629be3 100755
--- a/tests/qemu-iotests/196
+++ b/tests/qemu-iotests/196
@@ -46,7 +46,7 @@ class TestInvalidateAutoclear(iotests.QMPTestCase):
def test_migration(self):
result = self.vm_a.qmp('migrate', uri='exec:cat>' + migfile)
-self.assert_qmp(result, 'return', {});
+self.assert_qmp(result, 'return', {})
self.assertNotEqual(self.vm_a.event_wait("STOP"), None)
with open(disk, 'r+b') as f:
--
2.34.1
[PATCH v6 00/14] iotests: use vm.cmd()
Hi all!
Let's get rid of pattern
result = self.vm.qmp(...)
self.assert_qmp(result, 'return', {})
And switch to just
self.vm.cmd(...)
v6: rebase on master
Vladimir Sementsov-Ogievskiy (14):
python/qemu/qmp/legacy: cmd(): drop cmd_id unused argument
qmp_shell.py: _fill_completion() use .command() instead of .cmd()
scripts/cpu-x86-uarch-abi.py: use .command() instead of .cmd()
python: rename QEMUMonitorProtocol.cmd() to cmd_raw()
python/qemu: rename command() to cmd()
python/machine.py: upgrade vm.cmd() method
iotests: QemuStorageDaemon: add cmd() method like in QEMUMachine.
iotests: add some missed checks of qmp result
iotests: refactor some common qmp result checks into generic pattern
iotests: drop some occasional semicolons
iotests: drop some extra ** in qmp() call
iotests.py: pause_job(): drop return value
tests/vm/basevm.py: use cmd() instead of qmp()
python: use vm.cmd() instead of vm.qmp() where appropriate
docs/devel/testing.rst| 10 +-
python/qemu/machine/machine.py| 20 +-
python/qemu/qmp/legacy.py | 10 +-
python/qemu/qmp/qmp_shell.py | 20 +-
python/qemu/utils/qemu_ga_client.py | 2 +-
python/qemu/utils/qom.py | 8 +-
python/qemu/utils/qom_common.py | 2 +-
python/qemu/utils/qom_fuse.py | 6 +-
scripts/cpu-x86-uarch-abi.py | 8 +-
scripts/device-crash-test | 8 +-
scripts/render_block_graph.py | 8 +-
tests/avocado/avocado_qemu/__init__.py| 4 +-
tests/avocado/cpu_queries.py | 5 +-
tests/avocado/hotplug_cpu.py | 10 +-
tests/avocado/info_usernet.py | 4 +-
tests/avocado/machine_arm_integratorcp.py | 6 +-
tests/avocado/machine_m68k_nextcube.py| 4 +-
tests/avocado/machine_mips_malta.py | 6 +-
tests/avocado/machine_s390_ccw_virtio.py | 28 +-
tests/avocado/migration.py| 10 +-
tests/avocado/pc_cpu_hotplug_props.py | 2 +-
tests/avocado/version.py | 4 +-
tests/avocado/virtio_check_params.py | 6 +-
tests/avocado/virtio_version.py | 5 +-
tests/avocado/vnc.py | 16 +-
tests/avocado/x86_cpu_model_versions.py | 13 +-
tests/migration/guestperf/engine.py | 150 +++---
tests/qemu-iotests/030| 168 +++---
tests/qemu-iotests/040| 171 +++
tests/qemu-iotests/041| 482 --
tests/qemu-iotests/045| 15 +-
tests/qemu-iotests/055| 62 +--
tests/qemu-iotests/056| 77 ++-
tests/qemu-iotests/093| 42 +-
tests/qemu-iotests/118| 225
tests/qemu-iotests/124| 102 ++--
tests/qemu-iotests/129| 14 +-
tests/qemu-iotests/132| 5 +-
tests/qemu-iotests/139| 45 +-
tests/qemu-iotests/147| 30 +-
tests/qemu-iotests/151| 56 +-
tests/qemu-iotests/152| 8 +-
tests/qemu-iotests/155| 55 +-
tests/qemu-iotests/165| 8 +-
tests/qemu-iotests/196| 3 +-
tests/qemu-iotests/205| 6 +-
tests/qemu-iotests/218| 105 ++--
tests/qemu-iotests/245| 245 -
tests/qemu-iotests/256| 34 +-
tests/qemu-iotests/257| 36 +-
tests/qemu-iotests/264| 31 +-
tests/qemu-iotests/281| 21 +-
tests/qemu-iotests/295| 16 +-
tests/qemu-iotests/296| 21 +-
tests/qemu-iotests/298| 13 +-
tests/qemu-iotests/300| 54 +-
tests/qemu-iotests/iotests.py | 21 +-
.../tests/export-incoming-iothread| 6 +-
.../qemu-iotests/tests/graph-changes-while-io | 6 +-
tests/qemu-iotests/tests/image-fleecing | 3 +-
.../tests/migrate-bitmaps-postcopy-test | 31 +-
tests/qemu-iotests/tests/migrate-bitmaps-test | 45 +-
.../qemu-iotests/tests/migrate-during-backup | 41 +-
.../qemu-iotests/tests/migration-permissions | 9 +-
.../tests/mirror-ready-cancel-error | 74 ++-
tests/qemu-iotests/tests/mirror-top-perms | 16 +-
tests/qemu-iotests/tests/nbd-multiconn| 12 +-
tests/qemu-iotests/tests/reopen-file | 3 +-
.../qemu-iotests/tests/stream-error-on-reset | 6 +-
tests/vm/basevm.py| 4 +-
70 files changed, 1188 insertions(+), 1614 deletions(-)
Re: [Libguestfs] [PATCH v7 12/12] nbd/server: Add FLAG_PAYLOAD support to CMD_BLOCK_STATUS
On Wed, Oct 04, 2023 at 04:55:02PM -0500, Eric Blake wrote:
> > > +static int
> > > +nbd_co_block_status_payload_read(NBDClient *client, NBDRequest *request,
> > > + Error **errp)
> >
> > [..]
> > > +for (i = 0; i < count; i++) {
> > > +id = ldl_be_p(buf + sizeof(NBDBlockStatusPayload) + sizeof(id) *
> > > i);
> > > +if (id == NBD_META_ID_BASE_ALLOCATION) {
> > > +if (request->contexts->base_allocation) {
> > > +goto skip;
> > > +}
> >
> > should we also check that base_allocation is negotiated?
>
> Oh, good point. Without that check, the client can pass in random id
> numbers that it never negotiated. I've queued 1-11 and will probably
> send a pull request for those this week, while respinning this patch
> to fix the remaining issues you pointed out.
I'm squashing in the following. If you can review it today, I'll
include it in my pull request this afternoon; if not, we still have
time before soft freeze to get it in the next batch.
diff --git i/nbd/server.c w/nbd/server.c
index 30816b42386..62654579cbc 100644
--- i/nbd/server.c
+++ w/nbd/server.c
@@ -2478,19 +2478,22 @@ nbd_co_block_status_payload_read(NBDClient *client,
NBDRequest *request,
for (i = 0; i < count; i++) {
id = ldl_be_p(buf + sizeof(NBDBlockStatusPayload) + sizeof(id) * i);
if (id == NBD_META_ID_BASE_ALLOCATION) {
-if (request->contexts->base_allocation) {
+if (!client->contexts.base_allocation ||
+request->contexts->base_allocation) {
goto skip;
}
request->contexts->base_allocation = true;
} else if (id == NBD_META_ID_ALLOCATION_DEPTH) {
-if (request->contexts->allocation_depth) {
+if (!client->contexts.allocation_depth ||
+request->contexts->allocation_depth) {
goto skip;
}
request->contexts->allocation_depth = true;
} else {
-int idx = id - NBD_META_ID_DIRTY_BITMAP;
+unsigned idx = id - NBD_META_ID_DIRTY_BITMAP;
-if (idx > nr_bitmaps || request->contexts->bitmaps[idx]) {
+if (idx > nr_bitmaps || !client->contexts.bitmaps[idx] ||
+request->contexts->bitmaps[idx]) {
goto skip;
}
request->contexts->bitmaps[idx] = true;
diff --git i/nbd/trace-events w/nbd/trace-events
index 3cf2d00e458..00ae3216a11 100644
--- i/nbd/trace-events
+++ w/nbd/trace-events
@@ -70,7 +70,7 @@ nbd_co_send_chunk_read(uint64_t cookie, uint64_t offset, void
*data, uint64_t si
nbd_co_send_chunk_read_hole(uint64_t cookie, uint64_t offset, uint64_t size)
"Send structured read hole reply: cookie = %" PRIu64 ", offset = %" PRIu64 ",
len = %" PRIu64
nbd_co_send_extents(uint64_t cookie, unsigned int extents, uint32_t id,
uint64_t length, int last) "Send block status reply: cookie = %" PRIu64 ",
extents = %u, context = %d (extents cover %" PRIu64 " bytes, last chunk = %d)"
nbd_co_send_chunk_error(uint64_t cookie, int err, const char *errname, const
char *msg) "Send structured error reply: cookie = %" PRIu64 ", error = %d (%s),
msg = '%s'"
-nbd_co_receive_block_status_payload_compliance(uint64_t from, int len) "client
sent unusable block status payload: from=0x%" PRIx64 ", len=0x%x"
+nbd_co_receive_block_status_payload_compliance(uint64_t from, uint64_t len)
"client sent unusable block status payload: from=0x%" PRIx64 ", len=0x%" PRIx64
nbd_co_receive_request_decode_type(uint64_t cookie, uint16_t type, const char
*name) "Decoding type: cookie = %" PRIu64 ", type = %" PRIu16 " (%s)"
nbd_co_receive_request_payload_received(uint64_t cookie, uint64_t len)
"Payload received: cookie = %" PRIu64 ", len = %" PRIu64
nbd_co_receive_ext_payload_compliance(uint64_t from, uint64_t len) "client
sent non-compliant write without payload flag: from=0x%" PRIx64 ", len=0x%"
PRIx64
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
Re: [PULL 0/9] Python patches
On Thu, 5 Oct 2023 at 00:49, Philippe Mathieu-Daudé wrote: > > Hi John, > > On 4/10/23 21:46, John Snow wrote: > > The following changes since commit da1034094d375afe9e3d8ec8980550ea0f06f7e0: > > > >Merge tag 'for-upstream' of https://gitlab.com/bonzini/qemu into staging > > (2023-10-03 07:43:44 -0400) > > > > are available in the Git repository at: > > > >https://gitlab.com/jsnow/qemu.git tags/python-pull-request > > > > for you to fetch changes up to 4d7a663cbe8343e884b88e44bd88d37dd0a470e5: > > > >Python: test Python 3.12 (2023-10-04 15:19:00 -0400) > > > > > > Python pullreq > > > > Buffering improvements for qemu machine, minor changes to support the > > newly released Python 3.12 > > > > > > > > John Snow (9): > >Python/iotests: Add type hint for nbd module > >python/machine: move socket setup out of _base_args property > >python/machine: close sock_pair in cleanup path > >python/console_socket: accept existing FD in initializer > >python/machine: use socketpair() for console connections > >python/machine: use socketpair() for qtest connection > >python/machine: remove unused sock_dir argument > >python/qmp: remove Server.wait_closed() call for Python 3.12 > >Python: test Python 3.12 > > Is that a pull request or a patch series to be reviewed? Strange, some of the patches have Reviewed-by tags but others do not. I could not find a "Add type hint for nbd module" patch on the mailing list before this pull request, so I guess it hasn't been reviewed. I'll hold off from merging this series for now. John: Please make sure all patches have been on the mailing list for review before sending a pull request. Stefan
Re: [PATCH v3 16/16] trace/control: Clean up global variable shadowing
On Wed, Oct 04, 2023 at 02:00:19PM +0200, Philippe Mathieu-Daudé wrote: > Fix: > > trace/control.c:288:34: error: declaration shadows a variable in the global > scope [-Werror,-Wshadow] > void trace_opt_parse(const char *optarg) >^ > > /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/getopt.h:77:14: > note: previous declaration is here > extern char *optarg;/* getopt(3) external variables */ >^ > > Signed-off-by: Philippe Mathieu-Daudé > --- > trace/control.h | 4 ++-- > trace/control.c | 4 ++-- > 2 files changed, 4 insertions(+), 4 deletions(-) Reviewed-by: Stefan Hajnoczi signature.asc Description: PGP signature
Re: [PULL 0/1] Block patches
Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/8.2 for any user-visible changes. signature.asc Description: PGP signature
Re: How to tame CI?
On 26.07.23 16:32, Thomas Huth wrote: On 26/07/2023 15.00, Peter Maydell wrote: On Wed, 26 Jul 2023 at 13:06, Juan Quintela wrote: To make things easier, this is the part that show how it breaks (this is the gcov test): 357/423 qemu:block / io-qcow2-copy-before-write ERROR 6.38s exit status 1 PYTHON=/builds/juan.quintela/qemu/build/pyvenv/bin/python3 MALLOC_PERTURB_=44 /builds/juan.quintela/qemu/build/pyvenv/bin/python3 /builds/juan.quintela/qemu/build/../tests/qemu-iotests/check -tap -qcow2 copy-before-write --source-dir /builds/juan.quintela/qemu/tests/qemu-iotests --build-dir /builds/juan.quintela/qemu/build/tests/qemu-iotests ― ✀ ― stderr: --- /builds/juan.quintela/qemu/tests/qemu-iotests/tests/copy-before-write.out +++ /builds/juan.quintela/qemu/build/scratch/qcow2-file-copy-before-write/copy-before-write.out.bad @@ -1,5 +1,21 @@ - +...F +== +FAIL: test_timeout_break_snapshot (__main__.TestCbwError) +-- +Traceback (most recent call last): + File "/builds/juan.quintela/qemu/tests/qemu-iotests/tests/copy-before-write", line 210, in test_timeout_break_snapshot + self.assertEqual(log, """\ +AssertionError: 'wrot[195 chars]read 1048576/1048576 bytes at offset 0\n1 MiB,[46 chars]c)\n' != 'wrot[195 chars]read failed: Permission denied\n' + wrote 524288/524288 bytes at offset 0 + 512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + wrote 524288/524288 bytes at offset 524288 + 512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) ++ read failed: Permission denied +- read 1048576/1048576 bytes at offset 0 +- 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + This iotest failing is an intermittent that I've seen running pullreqs on master. I tend to see it on the s390 host. I suspect a race condition somewhere where it fails if the host is heavily loaded. It's obviously a failure in an iotest, so let's CC: the corresponding people (done now). Sorry for long delay. Does it still fail? In the test we expect that copy-before-write operation fails (because of throttling and timeout), and therefore snapshot is broken and next read from snapshot should fail. But most probably the copy-before-write operation succeeded in this case for some reason.. I don't think that throttling and timeouts in block layer can guarantee some determinism.. But usually it works. we use throttling with bps-write = 300 * 1024, i.e. 300KB per second. and cbw-timeout is set to 1 second. Then we do write 512K, then the comment say: # We need second write to trigger throttling and we write another 512K. first 512K are written, and we should wait 512/300 = 1.7 seconds since _start_ of that write before issuing the second one.. But if write was slow we may have to wait less than a second from finish of the first write start the second one. Then timeout will not fire. I see two possible ways to fix that: 1. decrease bps-write a bit. For example to 200 BPS. 2. rework the test to use null-co instead of real images. This way we will not suffer from unstable IO duration. So, is the problem still fire sometimes? -- Best regards, Vladimir
Re: [RFC PATCH v2 21/22] qapi: Inline and remove QERR_UNSUPPORTED definition
Please ignore this copy, it has the recipients messed up.
Re: [RFC PATCH v2 21/22] qapi: Inline and remove QERR_UNSUPPORTED definition
Philippe Mathieu-Daudé writes:
> Address the comment added in commit 4629ed1e98
> ("qerror: Finally unused, clean up"), from 2015:
>
> /*
>* These macros will go away, please don't use
>* in new code, and do not add new ones!
>*/
>
> Mechanical transformation using:
>
> $ sed -i -e 's/QERR_UNSUPPORTED/"this feature or command is not currently
> supported"/' \
> $(git grep -wl QERR_UNSUPPORTED)
>
> then manually removing the definition in include/qapi/qmp/qerror.h.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> RFC: Not sure what is the best way to change the comment
> in qga/qapi-schema.json...
> ---
> qga/qapi-schema.json | 5 +++--
> include/qapi/qmp/qerror.h | 3 ---
> qga/commands-bsd.c| 8 +++
> qga/commands-posix.c | 46 +++
> qga/commands-win32.c | 22 +--
> 5 files changed, 41 insertions(+), 43 deletions(-)
>
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index b720dd4379..51683f4dc2 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -6,8 +6,9 @@
##
# = General note concerning the use of guest agent interfaces
> #
> # "unsupported" is a higher-level error than the errors that
> # individual commands might document. The caller should always be
> -# prepared to receive QERR_UNSUPPORTED, even if the given command
> -# doesn't specify it, or doesn't document any failure mode at all.
> +# prepared to receive the "this feature or command is not currently
> supported"
> +# message, even if the given command doesn't specify it, or doesn't document
> +# any failure mode at all.
> ##
>
> ##
The comment is problematic before the patch. It's a doc comment,
i.e. it goes into the "QEMU Guest Agent Protocol Reference" manual,
where QERR_UNSUPPORTED is meaningless. Back when the comment was added
(commit 9481ecd737b "qga schema: document generic QERR_UNSUPPORTED"), it
was still internal documentation, where QERR_UNSUPPORTED made sense. It
became external documentation four years later (commit 56e8bdd46a8
"build-sys: add qapi doc generation targets")
I'm not sure how useful the comment is.
I guess we could instead simply point out that clients should always be
prepared for errors, even when the command doesn't document any, simply
because commands need not exist in all versions or builds of qemu-ga.
> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
> index 840831cc6a..7606f4525d 100644
> --- a/include/qapi/qmp/qerror.h
> +++ b/include/qapi/qmp/qerror.h
> @@ -17,7 +17,4 @@
> * add new ones!
> */
>
> -#define QERR_UNSUPPORTED \
> -"this feature or command is not currently supported"
> -
> #endif /* QERROR_H */
> diff --git a/qga/commands-bsd.c b/qga/commands-bsd.c
> index 17bddda1cf..11536f148f 100644
> --- a/qga/commands-bsd.c
> +++ b/qga/commands-bsd.c
> @@ -152,25 +152,25 @@ int qmp_guest_fsfreeze_do_thaw(Error **errp)
>
> GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
> {
> -error_setg(errp, QERR_UNSUPPORTED);
> +error_setg(errp, "this feature or command is not currently supported");
> return NULL;
> }
>
> GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
> {
> -error_setg(errp, QERR_UNSUPPORTED);
> +error_setg(errp, "this feature or command is not currently supported");
> return NULL;
> }
>
> GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
> {
> -error_setg(errp, QERR_UNSUPPORTED);
> +error_setg(errp, "this feature or command is not currently supported");
> return NULL;
> }
>
> GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
> {
> -error_setg(errp, QERR_UNSUPPORTED);
> +error_setg(errp, "this feature or command is not currently supported");
> return NULL;
> }
These are all commands that are not supported by this build of qemu-ga.
We could use the opportunity to improve the error message: scratch
"feature or ". Or maybe change the message to "this command is not
supported in this version of qemu-ga".
More of the same below, marked [*].
Taking a step back... Do we really need to make this a failure of its
own? Why not fail exactly as if the command didn't exist? Why would a
client ever care for the difference between "command doesn't exist in
this build of qemu-ga (but it does in other builds of this version of
qemu-ga)" and "command doesn't exist in this build of qemu-ga (and it
won't in any build of this version of qemu-ga)"?
If clients don't care, we could instead unregister these commands,
i.e. undo qmp_register_command(). The command will then fail exactly
like any other unknown command. We still need to provide the functions
to make the linker happy (unless we play with weak symbols), but they
can simply abort().
Michael and/or Konstantin, do you have comments as maintainers? A
preference even?
Hmm, there's yet another mechanism to disable commands:
qmp_disable_command() & friends. Looks
Re: [RFC PATCH v2 21/22] qapi: Inline and remove QERR_UNSUPPORTED definition
Philippe Mathieu-Daudé writes:
> Address the comment added in commit 4629ed1e98
> ("qerror: Finally unused, clean up"), from 2015:
>
> /*
>* These macros will go away, please don't use
>* in new code, and do not add new ones!
>*/
>
> Mechanical transformation using:
>
> $ sed -i -e 's/QERR_UNSUPPORTED/"this feature or command is not currently
> supported"/' \
> $(git grep -wl QERR_UNSUPPORTED)
>
> then manually removing the definition in include/qapi/qmp/qerror.h.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> RFC: Not sure what is the best way to change the comment
> in qga/qapi-schema.json...
> ---
> qga/qapi-schema.json | 5 +++--
> include/qapi/qmp/qerror.h | 3 ---
> qga/commands-bsd.c| 8 +++
> qga/commands-posix.c | 46 +++
> qga/commands-win32.c | 22 +--
> 5 files changed, 41 insertions(+), 43 deletions(-)
>
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index b720dd4379..51683f4dc2 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -6,8 +6,9 @@
##
# = General note concerning the use of guest agent interfaces
> #
> # "unsupported" is a higher-level error than the errors that
> # individual commands might document. The caller should always be
> -# prepared to receive QERR_UNSUPPORTED, even if the given command
> -# doesn't specify it, or doesn't document any failure mode at all.
> +# prepared to receive the "this feature or command is not currently
> supported"
> +# message, even if the given command doesn't specify it, or doesn't document
> +# any failure mode at all.
> ##
>
> ##
The comment is problematic before the patch. It's a doc comment,
i.e. it goes into the "QEMU Guest Agent Protocol Reference" manual,
where QERR_UNSUPPORTED is meaningless. Back when the comment was added
(commit 9481ecd737b "qga schema: document generic QERR_UNSUPPORTED"), it
was still internal documentation, where QERR_UNSUPPORTED made sense. It
became external documentation four years later (commit 56e8bdd46a8
"build-sys: add qapi doc generation targets")
I'm not sure how useful the comment is.
I guess we could instead simply point out that clients should always be
prepared for errors, even when the command doesn't document any, simply
because commands need not exist in all versions or builds of qemu-ga.
> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
> index 840831cc6a..7606f4525d 100644
> --- a/include/qapi/qmp/qerror.h
> +++ b/include/qapi/qmp/qerror.h
> @@ -17,7 +17,4 @@
> * add new ones!
> */
>
> -#define QERR_UNSUPPORTED \
> -"this feature or command is not currently supported"
> -
> #endif /* QERROR_H */
> diff --git a/qga/commands-bsd.c b/qga/commands-bsd.c
> index 17bddda1cf..11536f148f 100644
> --- a/qga/commands-bsd.c
> +++ b/qga/commands-bsd.c
> @@ -152,25 +152,25 @@ int qmp_guest_fsfreeze_do_thaw(Error **errp)
>
> GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
> {
> -error_setg(errp, QERR_UNSUPPORTED);
> +error_setg(errp, "this feature or command is not currently supported");
> return NULL;
> }
>
> GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
> {
> -error_setg(errp, QERR_UNSUPPORTED);
> +error_setg(errp, "this feature or command is not currently supported");
> return NULL;
> }
>
> GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
> {
> -error_setg(errp, QERR_UNSUPPORTED);
> +error_setg(errp, "this feature or command is not currently supported");
> return NULL;
> }
>
> GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
> {
> -error_setg(errp, QERR_UNSUPPORTED);
> +error_setg(errp, "this feature or command is not currently supported");
> return NULL;
> }
These are all commands that are not supported by this build of qemu-ga.
We could use the opportunity to improve the error message: scratch
"feature or ". Or maybe change the message to "this command is not
supported in this version of qemu-ga".
More of the same below, marked [*].
Taking a step back... Do we really need to make this a failure of its
own? Why not fail exactly as if the command didn't exist? Why would a
client ever care for the difference between "command doesn't exist in
this build of qemu-ga (but it does in other builds of this version of
qemu-ga)" and "command doesn't exist in this build of qemu-ga (and it
won't in any build of this version of qemu-ga)"?
If clients don't care, we could instead unregister these commands,
i.e. undo qmp_register_command(). The command will then fail exactly
like any other unknown command. We still need to provide the functions
to make the linker happy (unless we play with weak symbols), but they
can simply abort().
Michael and/or Konstantin, do you have comments as maintainers? A
preference even?
Hmm, there's yet another mechanism to disable commands:
qmp_disable_command() & friends. Looks
Re: [PATCH v2 1/2] hw/ide: reset: cancel async DMA operation before resetting state
On Wednesday, 6 September 2023 Fiona Ebner wrote: > If there is a pending DMA operation during ide_bus_reset(), the fact > that the IDEState is already reset before the operation is canceled > can be problematic. In particular, ide_dma_cb() might be called and > then use the reset IDEState which contains the signature after the > reset. When used to construct the IO operation this leads to > ide_get_sector() returning 0 and nsector being 1. This is particularly > bad, because a write command will thus destroy the first sector which > often contains a partition table or similar. Tested-by: simon.r...@nutanix.com
Re: [PATCH v3 14/16] softmmu/vl: Clean up global variable shadowing
On 5/10/23 10:59, Markus Armbruster wrote:
Philippe Mathieu-Daudé writes:
Fix:
softmmu/vl.c:1069:44: error: declaration shadows a variable in the global
scope [-Werror,-Wshadow]
static void parse_display_qapi(const char *optarg)
^
softmmu/vl.c:1224:39: error: declaration shadows a variable in the global
scope [-Werror,-Wshadow]
static void monitor_parse(const char *optarg, const char *mode, bool pretty)
^
softmmu/vl.c:1634:17: error: declaration shadows a variable in the global
scope [-Werror,-Wshadow]
const char *optarg = qdict_get_try_str(qdict, "type");
^
softmmu/vl.c:1784:45: error: declaration shadows a variable in the global
scope [-Werror,-Wshadow]
static void object_option_parse(const char *optarg)
^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/getopt.h:77:14:
note: previous declaration is here
extern char *optarg;/* getopt(3) external variables */
^
Signed-off-by: Philippe Mathieu-Daudé
How much we care about the shadowing is unclear, but that doesn't matter
if the patches make sense even if we pretend global @optarg doesn't
exist. Let's check that.
---
softmmu/vl.c | 26 +-
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 98e071e63b..ae1ff9887d 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -1066,12 +1066,12 @@ static void select_vgahw(const MachineClass
*machine_class, const char *p)
}
}
-static void parse_display_qapi(const char *optarg)
+static void parse_display_qapi(const char *optstr)
{
DisplayOptions *opts;
Visitor *v;
-v = qobject_input_visitor_new_str(optarg, "type", _fatal);
+v = qobject_input_visitor_new_str(optstr, "type", _fatal);
visit_type_DisplayOptions(v, NULL, , _fatal);
QAPI_CLONE_MEMBERS(DisplayOptions, , opts);
The actual argument is a string that is either JSON or KEY=VALUE,...
The fact that it's always an option argument now (actually the value of
global @optarg) is irrelevant here.
parse_display_qapi() passes its parameter to
qobject_input_visitor_new_str() parameter @str, which passes it to
qobject_from_json() parameter @string if JSON, or else to keyval_parse()
parameter @params.
I'd rename @optarg to @str here, like you do in the next hunk, to not
suggest a connection to CLI. Not a demand.
OK.
-static void object_option_parse(const char *optarg)
+static void object_option_parse(const char *optstr)
{
QemuOpts *opts;
const char *type;
Visitor *v;
-if (optarg[0] == '{') {
-QObject *obj = qobject_from_json(optarg, _fatal);
+if (optstr[0] == '{') {
+QObject *obj = qobject_from_json(optstr, _fatal);
v = qobject_input_visitor_new(obj);
qobject_unref(obj);
} else {
opts = qemu_opts_parse_noisily(qemu_find_opts("object"),
- optarg, true);
+ optstr, true);
if (!opts) {
exit(1);
}
Same argument as for parse_display_qapi(), and same suggestion.
If this goes though my tree, I can implement my two suggestions, if you
agree.
Sure, thank you!
Reviewed-by: Markus Armbruster
[PATCH v2] hw/ide/ahci: fix legacy software reset
From: Niklas Cassel
Legacy software contains a standard mechanism for generating a reset to a
Serial ATA device - setting the SRST (software reset) bit in the Device
Control register.
Serial ATA has a more robust mechanism called COMRESET, also referred to
as port reset. A port reset is the preferred mechanism for error
recovery and should be used in place of software reset.
Commit e2a5d9b3d9c3 ("hw/ide/ahci: simplify and document PxCI handling")
improved the handling of PxCI, such that PxCI gets cleared after handling
a non-NCQ, or NCQ command (instead of incorrectly clearing PxCI after
receiving an arbitrary FIS).
However, simply clearing PxCI after a non-NCQ, or NCQ command, is not
enough, we also need to clear PxCI when receiving a SRST in the Device
Control register.
This fixes an issue for FreeBSD where the device would fail to reset.
The problem was not noticed in Linux, because Linux uses a COMRESET
instead of a legacy software reset by default.
Fixes: e2a5d9b3d9c3 ("hw/ide/ahci: simplify and document PxCI handling")
Reported-by: Marcin Juszkiewicz
Signed-off-by: Niklas Cassel
---
Changes since v1: write the D2H FIS before clearing PxCI.
hw/ide/ahci.c | 16
1 file changed, 16 insertions(+)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index babdd7b458..7269dabbdb 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1254,10 +1254,26 @@ static void handle_reg_h2d_fis(AHCIState *s, int port,
case STATE_RUN:
if (cmd_fis[15] & ATA_SRST) {
s->dev[port].port_state = STATE_RESET;
+/*
+ * When setting SRST in the first H2D FIS in the reset
sequence,
+ * the device does not send a D2H FIS. Host software thus has
to
+ * set the "Clear Busy upon R_OK" bit such that PxCI (and BUSY)
+ * gets cleared. See AHCI 1.3.1, section 10.4.1 Software Reset.
+ */
+if (opts & AHCI_CMD_CLR_BUSY) {
+ahci_clear_cmd_issue(ad, slot);
+}
}
break;
case STATE_RESET:
if (!(cmd_fis[15] & ATA_SRST)) {
+/*
+ * When clearing SRST in the second H2D FIS in the reset
+ * sequence, the device will send a D2H FIS. See SATA 3.5a
Gold,
+ * section 11.4 Software reset protocol.
+ */
+ahci_clear_cmd_issue(ad, slot);
+ahci_write_fis_d2h(ad, false);
ahci_reset_port(s, port);
}
break;
--
2.41.0
[PATCH] hw/ide/ahci: fix legacy software reset
From: Niklas Cassel
Legacy software contains a standard mechanism for generating a reset to a
Serial ATA device - setting the SRST (software reset) bit in the Device
Control register.
Serial ATA has a more robust mechanism called COMRESET, also referred to
as port reset. A port reset is the preferred mechanism for error
recovery and should be used in place of software reset.
Commit e2a5d9b3d9c3 ("hw/ide/ahci: simplify and document PxCI handling")
improved the handling of PxCI, such that PxCI gets cleared after handling
a non-NCQ, or NCQ command (instead of incorrectly clearing PxCI after
receiving an arbitrary FIS).
However, simply clearing PxCI after a non-NCQ, or NCQ command, is not
enough, we also need to clear PxCI when receiving a SRST in the Device
Control register.
This fixes an issue for FreeBSD where the device would fail to reset.
The problem was not noticed in Linux, because Linux uses a COMRESET
instead of a legacy software reset by default.
Fixes: e2a5d9b3d9c3 ("hw/ide/ahci: simplify and document PxCI handling")
Reported-by: Marcin Juszkiewicz
Signed-off-by: Niklas Cassel
---
hw/ide/ahci.c | 16
1 file changed, 16 insertions(+)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index babdd7b458..3a8b97c325 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1254,10 +1254,26 @@ static void handle_reg_h2d_fis(AHCIState *s, int port,
case STATE_RUN:
if (cmd_fis[15] & ATA_SRST) {
s->dev[port].port_state = STATE_RESET;
+/*
+ * When setting SRST in the first H2D FIS in the reset
sequence,
+ * the device does not send a D2H FIS. Host software thus has
to
+ * set the "Clear Busy upon R_OK" bit such that PxCI (and BUSY)
+ * gets cleared. See AHCI 1.3.1, section 10.4.1 Software Reset.
+ */
+if (opts & AHCI_CMD_CLR_BUSY) {
+ahci_clear_cmd_issue(ad, slot);
+}
}
break;
case STATE_RESET:
if (!(cmd_fis[15] & ATA_SRST)) {
+/*
+ * When clearing SRST in the second H2D FIS in the reset
+ * sequence, the device will send a D2H FIS. See SATA 3.5a
Gold,
+ * section 11.4 Software reset protocol.
+ */
+ahci_write_fis_d2h(ad, false);
+ahci_clear_cmd_issue(ad, slot);
ahci_reset_port(s, port);
}
break;
--
2.41.0
Re: [PATCH v2 00/22] qapi: Kill 'qapi/qmp/qerror.h' for good
Philippe Mathieu-Daudé writes: > Since v1: > - Fixed checkpatch warnings (Juan) > - Added R-b tags > - New patch for 'vcpu_dirty_limit' > > Hi, > > This is kind of a selfish series. I'm really tired to grep > and read this comment from 2015 in qapi/qmp/qerror.h: > /* >* These macros will go away, please don't use >* in new code, and do not add new ones! >*/ > > Besides, these definitions are still added in recent code > (see for example commit 09f9ec9913 from June 2023). So > let's finish with this 8 years old technical debt. Gee, a late birthday present for me! Thank you! > Overall it took me 3h: 1h to find the correct Coccinelle > doc about Python use and read it again [*], then 1h to > adapt the script for each patch, rest is testing and > writing comments, so the scripts used could be used as > reference later. By the time you're done, it'll likely be 6h or more...
Re: [PATCH v3 14/16] softmmu/vl: Clean up global variable shadowing
Philippe Mathieu-Daudé writes:
> Fix:
>
> softmmu/vl.c:1069:44: error: declaration shadows a variable in the global
> scope [-Werror,-Wshadow]
> static void parse_display_qapi(const char *optarg)
> ^
> softmmu/vl.c:1224:39: error: declaration shadows a variable in the global
> scope [-Werror,-Wshadow]
> static void monitor_parse(const char *optarg, const char *mode, bool pretty)
> ^
> softmmu/vl.c:1634:17: error: declaration shadows a variable in the global
> scope [-Werror,-Wshadow]
> const char *optarg = qdict_get_try_str(qdict, "type");
> ^
> softmmu/vl.c:1784:45: error: declaration shadows a variable in the global
> scope [-Werror,-Wshadow]
> static void object_option_parse(const char *optarg)
> ^
>
> /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/getopt.h:77:14:
> note: previous declaration is here
> extern char *optarg;/* getopt(3) external variables */
>^
>
> Signed-off-by: Philippe Mathieu-Daudé
How much we care about the shadowing is unclear, but that doesn't matter
if the patches make sense even if we pretend global @optarg doesn't
exist. Let's check that.
> ---
> softmmu/vl.c | 26 +-
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 98e071e63b..ae1ff9887d 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -1066,12 +1066,12 @@ static void select_vgahw(const MachineClass
> *machine_class, const char *p)
> }
> }
>
> -static void parse_display_qapi(const char *optarg)
> +static void parse_display_qapi(const char *optstr)
> {
> DisplayOptions *opts;
> Visitor *v;
>
> -v = qobject_input_visitor_new_str(optarg, "type", _fatal);
> +v = qobject_input_visitor_new_str(optstr, "type", _fatal);
>
> visit_type_DisplayOptions(v, NULL, , _fatal);
> QAPI_CLONE_MEMBERS(DisplayOptions, , opts);
The actual argument is a string that is either JSON or KEY=VALUE,...
The fact that it's always an option argument now (actually the value of
global @optarg) is irrelevant here.
parse_display_qapi() passes its parameter to
qobject_input_visitor_new_str() parameter @str, which passes it to
qobject_from_json() parameter @string if JSON, or else to keyval_parse()
parameter @params.
I'd rename @optarg to @str here, like you do in the next hunk, to not
suggest a connection to CLI. Not a demand.
> @@ -1221,21 +1221,21 @@ static int mon_init_func(void *opaque, QemuOpts
> *opts, Error **errp)
> return monitor_init_opts(opts, errp);
> }
>
> -static void monitor_parse(const char *optarg, const char *mode, bool pretty)
> +static void monitor_parse(const char *str, const char *mode, bool pretty)
> {
> static int monitor_device_index = 0;
> QemuOpts *opts;
> const char *p;
> char label[32];
>
> -if (strstart(optarg, "chardev:", )) {
> +if (strstart(str, "chardev:", )) {
> snprintf(label, sizeof(label), "%s", p);
> } else {
> snprintf(label, sizeof(label), "compat_monitor%d",
> monitor_device_index);
> -opts = qemu_chr_parse_compat(label, optarg, true);
> +opts = qemu_chr_parse_compat(label, str, true);
> if (!opts) {
> -error_report("parse error: %s", optarg);
> +error_report("parse error: %s", str);
> exit(1);
> }
> }
The actual argument is either @optarg or a string literal, but that's
again irrelevant here.
> @@ -1631,13 +1631,13 @@ static const QEMUOption *lookup_opt(int argc, char
> **argv,
>
> static MachineClass *select_machine(QDict *qdict, Error **errp)
> {
> -const char *optarg = qdict_get_try_str(qdict, "type");
> +const char *machine_type = qdict_get_try_str(qdict, "type");
> GSList *machines = object_class_get_list(TYPE_MACHINE, false);
> MachineClass *machine_class;
> Error *local_err = NULL;
>
> -if (optarg) {
> -machine_class = find_machine(optarg, machines);
> +if (machine_type) {
> +machine_class = find_machine(machine_type, machines);
> qdict_del(qdict, "type");
> if (!machine_class) {
> error_setg(_err, "unsupported machine type");
Obvious improvement.
> @@ -1781,20 +1781,20 @@ static void object_option_add_visitor(Visitor *v)
> QTAILQ_INSERT_TAIL(_opts, opt, next);
> }
>
> -static void object_option_parse(const char *optarg)
> +static void object_option_parse(const char *optstr)
> {
> QemuOpts *opts;
> const char *type;
> Visitor *v;
>
> -if (optarg[0] == '{') {
> -QObject *obj = qobject_from_json(optarg, _fatal);
> +if (optstr[0] == '{') {
> +QObject *obj = qobject_from_json(optstr, _fatal);
>
> v = qobject_input_visitor_new(obj);
>
Re: [RFC] migration/block-dirty-bitmap: make loading bitmap for device with iothread future-proof
Am 04.10.23 um 18:13 schrieb Vladimir Sementsov-Ogievskiy:
>> @@ -833,7 +836,10 @@ static int dirty_bitmap_load_start(QEMUFile *f,
>> DBMLoadState *s)
>> bdrv_disable_dirty_bitmap(s->bitmap);
>> if (flags & DIRTY_BITMAP_MIG_START_FLAG_ENABLED) {
>> + AioContext *ctx = bdrv_get_aio_context(s->bs);
>> + aio_context_acquire(ctx);
>> bdrv_dirty_bitmap_create_successor(s->bitmap, _err);
>> + aio_context_release(ctx);
>
> Would not this deadlock in current code? When we have the only one aio
> context and therefore we are already in it?
>
Yes, I noticed that myself a bit later ;)
Am 01.08.23 um 09:57 schrieb Fiona Ebner:
> And the patch itself also got an issue. AFAICT, when
> qcow2_co_load_vmstate() is called, we already have acquired the context
> for the drive we load the snapshot from, and since
> polling/AIO_WAIT_WHILE requires that the caller has acquired the context
> exactly once, we'd need to distinguish if the dirty bitmap is for that
> drive (can't acquire the context a second time) or a different drive
> (need to acquire the context for the first time).
Quoted from a reply in this thread
https://lists.nongnu.org/archive/html/qemu-devel/2023-08/msg7.html
Am 04.10.23 um 18:13 schrieb Vladimir Sementsov-Ogievskiy:
> If as Juan said, we rework incoming migration coroutine to be a separate
> thread, this patch becomes more correct, I think..
Yes, it would become an issue when the function is called from a
non-coroutine context.
> If keep coroutine, I think, we should check are we already in that aio
> context, and if so we should not acquire it.
In coroutine context, we don't need to acquire it, but it shouldn't hurt
either and this approach should work for non-coroutine context too. The
question is if such conditional lock-taking is acceptable (do we already
have something similar somewhere?) or if it can be avoided somehow like
it was preferred in another one of my patches:
Am 05.05.23 um 16:59 schrieb Peter Xu:
> On Fri, May 05, 2023 at 03:46:52PM +0200, Fiona Ebner wrote:
>> To fix it, ensure that the BQL is held during setup. To avoid changing
>> the behavior for migration too, introduce conditionals for the setup
>> callbacks that need the BQL and only take the lock if it's not already
>> held.
>
> The major complexity of this patch is the "conditionally taking" part.
Quoted from
https://lists.nongnu.org/archive/html/qemu-devel/2023-05/msg01514.html
Re: [PATCH v2 11/22] qapi: Inline QERR_INVALID_PARAMETER_VALUE definition (constant value)
Philippe Mathieu-Daudé wrote:
> vcpu_dirty_limit
>
> Address the comment added in commit 4629ed1e98
> ("qerror: Finally unused, clean up"), from 2015:
>
> /*
>* These macros will go away, please don't use
>* in new code, and do not add new ones!
>*/
>
> Mechanical transformation using the following
> coccinelle semantic patch:
>
> @match@
> expression errp;
> constant param;
> constant value;
> @@
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE, param, value);
>
> @script:python strformat depends on match@
> param << match.param;
> value << match.value;
> fixedfmt; // new var
> @@
> fixedfmt = "\"Parameter '%s' expects %s\"" % (param[1:-1], value[1:-1])
> coccinelle.fixedfmt = cocci.make_ident(fixedfmt)
>
> @replace@
> expression match.errp;
> constant match.param;
> constant match.value;
> identifier strformat.fixedfmt;
> @@
> -error_setg(errp, QERR_INVALID_PARAMETER_VALUE, param, value);
> +error_setg(errp, fixedfmt);
>
> Then manually splitting lines over 90 characters.
>
> Reviewed-by: Juan Quintela
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Juan Quintela
Re: [PATCH v2 10/22] qapi: Correct error message for 'vcpu_dirty_limit' parameter
Philippe Mathieu-Daudé wrote: > QERR_INVALID_PARAMETER_VALUE is defined as: > > #define QERR_INVALID_PARAMETER_VALUE \ > "Parameter '%s' expects %s" > > The current error is formatted as: > > "Parameter 'vcpu_dirty_limit' expects is invalid, it must greater then 1 > MB/s" > > Replace by: > > "Parameter 'vcpu_dirty_limit' is invalid, it must greater then 1 MB/s" > > Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Juan Quintela
