similar to: [nbdkit PATCH] connections: Implement NBD_OPT_INFO

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;

There's no substantial difference over v1, I simply fixed a few whitespace issues, moved one struct around and tidied up the comments. 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.

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.

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

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

This allows plugins (or filters) to read the export name which was passed to the server from the client. UNFINISHED: - Needs tests --- TODO | 8 ++++++ docs/nbdkit-plugin.pod | 39 ++++++++++++++++++++++++++++ server/connections.c | 1 + server/internal.h | 1 + server/protocol-handshake-newstyle.c | 30

The first one is the nastiest - it is an assertion failure caused by a spec-compliant client and introduced by our security fix that was released in 1.14.1. Eric Blake (4): server: Fix regression for NBD_OPT_INFO before NBD_OPT_GO server: Fix back-to-back SET_META_CONTEXT server: Forbid NUL in export and context names server: Fix OPT_GO on different export than SET_META_CONTEXT

The NBD spec says that additional information in response to NBD_OPT_GO or NBD_OPT_INFO, such as NBD_INFO_NAME, is optional, but also recommends that if a client hints about wanting a particular piece of information, that it is better for the server to reply to those known hints. This is also the the only way for a client to learn the canonical name that is used when connecting to the default

Since the v1 series (0/4, at [1]), I've applied patches 1 and 2, rewritten patch 3 [Forbid NUL in export and context names] into patch 4 here, patch 4 there turned into patch 6 here, and everything else here is new. [1]https://www.redhat.com/archives/libguestfs/2019-September/msg00180.html I don't know if there is a handy reusable function for checking whether a string contains valid

The previous incomplete patch was here: https://www.redhat.com/archives/libguestfs/2019-September/msg00049.html based on earlier discussion here: https://www.redhat.com/archives/libguestfs/2019-September/msg00047.html In v2: - The previous patch was incomplete. This version completes it by adding tests and extending nbdkit-sh-plugin. - nbdkit_export_name now returns NULL for error,

This replaces 13/13 of my v2 series; and now that it has pretty good testsuite coverage and demonstrable performance improvement to nbdinfo, I'm going ahead and pushing this now. We may still want to add further nbd_opt_* commands for other fine-grained tuning of negotiation, but for now, I think things have stabilized on this end, and I can return to polishing .list_exports on the nbdkit

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.

This is the sort of thing I had in mind for option (1) here: https://www.redhat.com/archives/libguestfs/2019-September/msg00047.html It does reveal that the way we currently list exports is naive to say the least ... Rich.

The NBD spec was recently patched (nbd.git commit 7827f3ae and friends) to require NBD_OPT_GO for baseline interoperability, with the aim of fewer servers and clients falling back to NBD_OPT_EXPORT_NAME. And since nbdkit as server recently started supporting NBD_OPT_GO (commit f7dd9799), our nbd client as plugin should take advantage of it. This patch is a prerequisite to teaching the nbd plugin

Most known NBD clients do not bother with NBD_OPT_INFO (except for clients like 'qemu-nbd --list' that don't ever intend to connect), but go straight to NBD_OPT_GO. However, it's not too hard to hack up qemu to add in an extra client step (whether info on the same name, or more interestingly, info on a different name), as a patch against qemu commit 6f214b30445: | diff --git

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.

With some crufty sed scripts we can generate functions that map from NBD protocol flags (eg. NBD_CMD_READ) to strings ("NBD_CMD_READ"). This works on GNU sed and with FreeBSD, also with GNU sed's --posix option, so I guess the sed code is POSIX-compatible. Rich.

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