[Libguestfs] [PATCH v3 12/14] nbd/client: Request extended headers during negotiation
Vladimir Sementsov-Ogievskiy
vsementsov at yandex-team.ru
Wed May 31 17:39:53 UTC 2023
On 15.05.23 22:53, Eric Blake wrote:
> 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
why must not? It should gracefully report ENOTSUP? Or not?
> is used to connect to the kernel module (as nbd.ko does not expect
> them), 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 <eblake at redhat.com>
> ---
> block/nbd.c | 5 +--
> nbd/client-connection.c | 2 +-
> nbd/client.c | 38 ++++++++++++-------
> qemu-nbd.c | 3 ++
> tests/qemu-iotests/223.out | 6 +++
> tests/qemu-iotests/233.out | 5 +++
> tests/qemu-iotests/241.out | 3 ++
> tests/qemu-iotests/307.out | 5 +++
> .../tests/nbd-qemu-allocation.out | 1 +
> 9 files changed, 51 insertions(+), 17 deletions(-)
>
> diff --git a/block/nbd.c b/block/nbd.c
> index 150dfe7170c..db107ff0806 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -1146,10 +1146,9 @@ static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s,
>
> switch (chunk->type) {
> case NBD_REPLY_TYPE_BLOCK_STATUS_EXT:
> - wide = true;
> - /* fallthrough */
> case NBD_REPLY_TYPE_BLOCK_STATUS:
> - if (s->info.extended_headers != wide) {
> + wide = chunk->type == NBD_REPLY_TYPE_BLOCK_STATUS_EXT;
> + if ((s->info.header_style == NBD_HEADER_EXTENDED) != wide) {
O, that's a part of previous commit. Also, initialization of wide to false becomes unneeded.
> trace_nbd_extended_headers_compliance("block_status");
> }
> if (received) {
> diff --git a/nbd/client-connection.c b/nbd/client-connection.c
> index 62d75af0bb3..8e0606cadf0 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.header_style = NBD_HEADER_STRUCTURED,
> + .initial_info.header_style = NBD_HEADER_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 e5db3c8b79d..7edddfd2f83 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -879,11 +879,12 @@ static int nbd_list_meta_contexts(QIOChannel *ioc,
> * 1: server is newstyle, but can only accept EXPORT_NAME
> * 2: server is newstyle, but lacks structured replies
> * 3: server is newstyle and set up for structured replies
> + * 4: server is newstyle and set up for extended headers
> */
> static int nbd_start_negotiate(AioContext *aio_context, QIOChannel *ioc,
> QCryptoTLSCreds *tlscreds,
> const char *hostname, QIOChannel **outioc,
> - bool structured_reply, bool *zeroes,
> + NBDHeaderStyle style, bool *zeroes,
> Error **errp)
> {
> ERRP_GUARD();
> @@ -961,15 +962,23 @@ static int nbd_start_negotiate(AioContext *aio_context, QIOChannel *ioc,
> if (fixedNewStyle) {
> int result = 0;
>
> - if (structured_reply) {
> + if (style >= NBD_HEADER_EXTENDED) {
> + result = nbd_request_simple_option(ioc,
> + NBD_OPT_EXTENDED_HEADERS,
> + false, errp);
> + if (result) {
> + return result < 0 ? -EINVAL : 4;
> + }
> + }
> + if (style >= NBD_HEADER_STRUCTURED) {
> result = nbd_request_simple_option(ioc,
> NBD_OPT_STRUCTURED_REPLY,
> false, errp);
> - if (result < 0) {
> - return -EINVAL;
> + if (result) {
> + return result < 0 ? -EINVAL : 3;
> }
> }
> - return 2 + result;
> + return 2;
> } else {
> return 1;
> }
> @@ -1031,8 +1040,7 @@ int nbd_receive_negotiate(AioContext *aio_context, QIOChannel *ioc,
> trace_nbd_receive_negotiate_name(info->name);
>
> result = nbd_start_negotiate(aio_context, ioc, tlscreds, hostname, outioc,
> - info->header_style >= NBD_HEADER_STRUCTURED,
> - &zeroes, errp);
> + info->header_style, &zeroes, errp);
>
> info->header_style = NBD_HEADER_SIMPLE;
> info->base_allocation = false;
> @@ -1041,8 +1049,10 @@ int nbd_receive_negotiate(AioContext *aio_context, QIOChannel *ioc,
> }
>
> switch (result) {
> + case 4: /* newstyle, with extended headers */
> case 3: /* newstyle, with structured replies */
> - info->header_style = NBD_HEADER_STRUCTURED;
> + /* Relies on encoding of _STRUCTURED and _EXTENDED */
> + info->header_style = result - 2;
I'd prefer explicit
info->header_style = (result == 4 ? NBD_HEADER_EXTENDED : NBD_HEADER_STRUCTURED);
with no comment
> if (base_allocation) {
> result = nbd_negotiate_simple_meta_context(ioc, info, errp);
> if (result < 0) {
> @@ -1151,8 +1161,8 @@ int nbd_receive_export_list(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
> QIOChannel *sioc = NULL;
>
> *info = NULL;
> - result = nbd_start_negotiate(NULL, ioc, tlscreds, hostname, &sioc, true,
> - NULL, errp);
> + result = nbd_start_negotiate(NULL, ioc, tlscreds, hostname, &sioc,
> + NBD_HEADER_EXTENDED, NULL, errp);
> if (tlscreds && sioc) {
> ioc = sioc;
> }
> @@ -1160,6 +1170,7 @@ int nbd_receive_export_list(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
> switch (result) {
> case 2:
> case 3:
> + case 4:
> /* 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. */
> @@ -1180,8 +1191,9 @@ int nbd_receive_export_list(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
> memset(&array[count - 1], 0, sizeof(*array));
> array[count - 1].name = name;
> array[count - 1].description = desc;
> - array[count - 1].header_style = result == 3 ?
> - NBD_HEADER_STRUCTURED : NBD_HEADER_SIMPLE;
> +
> + /* Depends on values of _SIMPLE, _STRUCTURED, and _EXTENDED */
> + array[count - 1].header_style = result - 2;
Personally, I don't like enum-based arithmetics.. It's something to be very careful with and check enum definition every time.
Maybe, add enum NBDConnectionStyle : NBD_STYLE_OLD, NBD_STYLE_EXPORT_NAME, NBD_STYLE_SIMPLE, NBD_STYLE_STRUCTURED, NBD_STYLE_EXTENDED,
and a simple function nbd_con_style_to_hdr_style: NBDConnectionStyle -> NBDHeaderStyle
Or, better, nbd_start_negotiate() may return only 0/1/2 as success values, and additionally set OUT argument *header_style?
And anyway, 0/1/2/3/4 - sounds like too much magic numeric logic
(I feel, that's all a kind of taste and don't want to insist anyway)
> }
>
> for (i = 0; i < count; i++) {
> @@ -1197,7 +1209,7 @@ int nbd_receive_export_list(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
> break;
> }
>
> - if (result == 3 &&
> + if (result >= 3 &&
> nbd_list_meta_contexts(ioc, &array[i], errp) < 0) {
> goto out;
> }
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 6ff45308a9c..8c35442626a 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -238,6 +238,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].header_style >= NBD_HEADER_EXTENDED ?
> + "64-bit" : "32-bit");
> if (list[i].n_contexts) {
> printf(" available meta contexts: %d\n", list[i].n_contexts);
> for (j = 0; j < list[i].n_contexts; j++) {
> diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
> index 26fb347c5da..b98582c38ea 100644
> --- a/tests/qemu-iotests/223.out
> +++ b/tests/qemu-iotests/223.out
> @@ -87,6 +87,7 @@ exports available: 3
> min block: 1
> opt block: 4096
> max block: 33554432
> + transaction size: 64-bit
> available meta contexts: 2
> base:allocation
> qemu:dirty-bitmap:b
> @@ -97,6 +98,7 @@ exports available: 3
> min block: 1
> opt block: 4096
> max block: 33554432
> + transaction size: 64-bit
> available meta contexts: 2
> base:allocation
> qemu:dirty-bitmap:b2
> @@ -106,6 +108,7 @@ exports available: 3
> min block: 1
> opt block: 4096
> max block: 33554432
> + transaction size: 64-bit
> available meta contexts: 2
> base:allocation
> qemu:dirty-bitmap:b3
> @@ -206,6 +209,7 @@ exports available: 3
> min block: 1
> opt block: 4096
> max block: 33554432
> + transaction size: 64-bit
> available meta contexts: 2
> base:allocation
> qemu:dirty-bitmap:b
> @@ -216,6 +220,7 @@ exports available: 3
> min block: 1
> opt block: 4096
> max block: 33554432
> + transaction size: 64-bit
> available meta contexts: 2
> base:allocation
> qemu:dirty-bitmap:b2
> @@ -225,6 +230,7 @@ exports available: 3
> min block: 1
> opt block: 4096
> max block: 33554432
> + transaction size: 64-bit
> available meta contexts: 2
> base:allocation
> qemu:dirty-bitmap:b3
> diff --git a/tests/qemu-iotests/233.out b/tests/qemu-iotests/233.out
> index 237c82767ea..33cb622ecf0 100644
> --- a/tests/qemu-iotests/233.out
> +++ b/tests/qemu-iotests/233.out
> @@ -53,6 +53,11 @@ exports available: 1
> export: ''
> size: 67108864
> min block: 1
> + opt block: 4096
> + max block: 33554432
> + transaction size: 64-bit
> + available meta contexts: 1
> + base:allocation
>
> == check TLS with different CA fails ==
> qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': The certificate hasn't got a known issuer
> diff --git a/tests/qemu-iotests/241.out b/tests/qemu-iotests/241.out
> index 88e8cfcd7e2..a9efb876521 100644
> --- a/tests/qemu-iotests/241.out
> +++ b/tests/qemu-iotests/241.out
> @@ -6,6 +6,7 @@ exports available: 1
> export: ''
> size: 1024
> min block: 1
> + transaction size: 64-bit
> [{ "start": 0, "length": 1000, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET},
> { "start": 1000, "length": 24, "depth": 0, "present": true, "zero": true, "data": false, "offset": OFFSET}]
> 1 KiB (0x400) bytes allocated at offset 0 bytes (0x0)
> @@ -16,6 +17,7 @@ exports available: 1
> export: ''
> size: 1024
> min block: 512
> + transaction size: 64-bit
> [{ "start": 0, "length": 1024, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET}]
> 1 KiB (0x400) bytes allocated at offset 0 bytes (0x0)
> WARNING: Image format was not specified for 'TEST_DIR/t.raw' and probing guessed raw.
> @@ -28,6 +30,7 @@ exports available: 1
> export: ''
> size: 1024
> min block: 1
> + transaction size: 64-bit
> [{ "start": 0, "length": 1000, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET},
> { "start": 1000, "length": 24, "depth": 0, "present": true, "zero": true, "data": false, "offset": OFFSET}]
> 1 KiB (0x400) bytes allocated at offset 0 bytes (0x0)
> diff --git a/tests/qemu-iotests/307.out b/tests/qemu-iotests/307.out
> index 390f05d1b78..2b9a6a67a1a 100644
> --- a/tests/qemu-iotests/307.out
> +++ b/tests/qemu-iotests/307.out
> @@ -19,6 +19,7 @@ exports available: 1
> min block: XXX
> opt block: XXX
> max block: XXX
> + transaction size: 64-bit
> available meta contexts: 1
> base:allocation
>
> @@ -47,6 +48,7 @@ exports available: 1
> min block: XXX
> opt block: XXX
> max block: XXX
> + transaction size: 64-bit
> available meta contexts: 1
> base:allocation
>
> @@ -78,6 +80,7 @@ exports available: 2
> min block: XXX
> opt block: XXX
> max block: XXX
> + transaction size: 64-bit
> available meta contexts: 1
> base:allocation
> export: 'export1'
> @@ -87,6 +90,7 @@ exports available: 2
> min block: XXX
> opt block: XXX
> max block: XXX
> + transaction size: 64-bit
> available meta contexts: 1
> base:allocation
>
> @@ -113,6 +117,7 @@ exports available: 1
> min block: XXX
> opt block: XXX
> max block: XXX
> + transaction size: 64-bit
> available meta contexts: 1
> base:allocation
>
> diff --git a/tests/qemu-iotests/tests/nbd-qemu-allocation.out b/tests/qemu-iotests/tests/nbd-qemu-allocation.out
> index 9d938db24e6..659276032b0 100644
> --- a/tests/qemu-iotests/tests/nbd-qemu-allocation.out
> +++ b/tests/qemu-iotests/tests/nbd-qemu-allocation.out
> @@ -21,6 +21,7 @@ exports available: 1
> min block: 1
> opt block: 4096
> max block: 33554432
> + transaction size: 64-bit
> available meta contexts: 2
> base:allocation
> qemu:allocation-depth
--
Best regards,
Vladimir
More information about the Libguestfs
mailing list