similar to: [PATCH] protocol: Ignore rest of option when replying with error

The code handling the NBD protocol was located in the same file as the code handling connections, for not really any reason except historical. This is quite a large code movement which splits out the protocol code into four new files: server/protocol-handshake.c initial handshake server/protocol-handshake-newstyle.c " " (newstyle)

--- src/connections.c | 233 +++++++++++++++++++++++++++++++++++++--------- src/protocol.h | 27 ++++-- 2 files changed, 209 insertions(+), 51 deletions(-) diff --git a/src/connections.c b/src/connections.c index ba6e91d..4e9b191 100644 --- a/src/connections.c +++ b/src/connections.c @@ -75,7 +75,9 @@ struct connection { void **handles; size_t nr_handles; + uint32_t cflags;

Since commit 86fdb48c6a5362d66865493d9d2172166f99722e we have stored the connection object in thread-local storage. In this very large, but mostly mechanical change we stop passing the connection pointer around everywhere, and instead use the value stored in thread-local storage. This assumes a 1-1 mapping between the connection and the current thread which is true in *most* places.

Fuzzing with afl found a bug where a 27 byte client sequence can cause nbdkit to report a strange error message: $ printf %s $'000\1IHAVEOPT000\6'$'000\7'$'000\1x00' | tr 0 '\0' | ./nbdkit -s memory size=1m >/dev/null nbdkit: memory: error: client exceeded maximum number of options (32) The culprit? The client is hanging up on a message boundary, so

qemu is about to add 'qemu-nbd --list', which exercises NBD_OPT_LIST and NBD_OPT_INFO to give the user as much detail as possible about an export without actually connecting to it. For that to display more than the export name when nbdkit is the server, we need to implement NBD_OPT_INFO. Thankfully, the NBD spec intentionally made the command very similar to NBD_OPT_GO, to the point that

None are supported at present, so this always returns an empty list. --- docs/nbdkit-protocol.pod | 4 ++ server/protocol.h | 2 + server/connections.c | 84 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 90 insertions(+) diff --git a/docs/nbdkit-protocol.pod b/docs/nbdkit-protocol.pod index 68438fa..4c65e00 100644 --- a/docs/nbdkit-protocol.pod +++

If a plugin's .open or .get_size or .can_write fails, right now that is fatal to the connection. When nbdkit was first implemented, this made sense (there was no way to report errors to oldstyle or NBD_OPT_EXPORT_NAME). But now that newstyle is around, it's rather abrupt to hang up on the client, and better is to return an error to NBD_OPT_GO, and let the client choose what to do (most

This is about the simplest implementation of NBD Structured Replies (SR) that we can do right now. It accepts NBD_OPT_STRUCTURED_REPLIES when negotiated by the client, but only sends back the simplest possible SR when required to by NBD_CMD_READ. The rest of the time it will send back simple replies as before. We do not modify the plugin API so plugins are unable to send complex SRs. Also we

Diff against libnbd’s copy of this file, and change this one until it matches. --- common/protocol/nbd-protocol.h | 76 +++++++++++++++++----------- server/protocol-handshake-newstyle.c | 26 +++++----- server/protocol-handshake-oldstyle.c | 4 +- server/protocol.c | 25 ++++----- tests/test-layers.c | 14 ++--- 5 files changed, 81 insertions(+), 64

This commit implements the core NBD protocol for the "base:allocation" Block Status replies. --- server/internal.h | 7 ++ server/protocol.h | 15 +++ server/protocol-handshake-newstyle.c | 81 ++++++++++++++- server/protocol.c | 141 ++++++++++++++++++++++++--- 4 files changed, 229 insertions(+), 15 deletions(-) diff --git

Up to nbdkit 1.22, we never advertised large export lists, so a client had no real reason to give more than 32 NBD_OPT_ commands before finally selecting an export or quitting, and we were justified in dropping indecisive chatty clients as being a waste of server resources. But now that we support .list_exports, it is reasonable for a client (such as 'qemu-nbd --list' or 'nbdinfo

There's no substantial difference over v1, I simply fixed a few whitespace issues, moved one struct around and tidied up the comments. Rich.

These are a couple of patches in preparation for the Block Status implementation. While the patches (especially the second one) are very large they are really just elementary code motion. Rich.

This is only lightly tested (against just qemu NBD client), and the code might be structured a little better as the _negotiate_handshake_newstyle_options function has now grown to be huge. Anyway works for me. Rich.

This commit implements the core NBD protocol for the "base:allocation" Block Status replies. --- server/internal.h | 7 + server/protocol.h | 17 +- server/protocol-handshake-newstyle.c | 79 ++++++++- server/protocol.c | 248 +++++++++++++++++++++++++-- 4 files changed, 335 insertions(+), 16 deletions(-) diff --git

Experimental and only very lightly tested so far. Rich.

I'm easily able to provoke NBD_REP_ERR_TLS_REQD (use nbd_set_tls(0) to talk to a server that requires encryption) and NBD_REP_ERR_UNKNOWN (forget to use nbd_set_export_name for qemu-nbd); it's nice to display a useful error for these rather than "unknown reply from NBD_OPT_GO: 0x80000005" or similar. Other errors are less common, but as long as we're decoding things, it

v1 was here: https://www.redhat.com/archives/libguestfs/2020-February/msg00081.html v2 replaces struct connection *conn = GET_CONN; with GET_CONN; which sets conn implicitly and asserts that it is non-NULL. If we actually want to test if conn is non-NULL or behave differently, then you must use threadlocal_get_conn() instead, and some existing uses do that. Rich.

The third patch is a large but mechanical change which gets rid of passing around struct connection * entirely within the server, preferring instead to reference the connection through thread-local storage. I hope this is a gateway to simplifying other parts of the code. Rich.

When we added support for .extents, we had nbdkit unconditionally support structured replies if the client requests them, and the plugin's .can_extents has no impact on what the server advertises. However, while the plugin API doesn't care whether the client requested SR, there is still a case to be made for allowing a filter to prevent SR, at least for testing purposese (such as