[PATCH nbdkit] Minimal implementation of NBD Structured Replies.
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 do not handle human-readable error messages yet because that would require changes in how we handle nbdkit_error(). Also we do not understand NBD_CMD_FLAG_DF, but that seems to be OK because (a) we don't advertize the feature and (b) we only send back a single chunk anyway. --- docs/nbdkit-protocol.pod | 6 +- server/protocol.h | 59 +++++++++--- plugins/nbd/nbd.c | 4 +- server/connections.c | 189 ++++++++++++++++++++++++++++++++------- tests/test-layers.c | 2 +- 5 files changed, 212 insertions(+), 48 deletions(-) diff --git a/docs/nbdkit-protocol.pod b/docs/nbdkit-protocol.pod index c44db6a..68438fa 100644 --- a/docs/nbdkit-protocol.pod +++ b/docs/nbdkit-protocol.pod @@ -122,7 +122,11 @@ Supported in nbdkit E<ge> 1.9.9. =item Structured Replies -I<Not supported>. +Supported in nbdkit E<ge> 1.11.8. + +However we don’t expose the capability to send structured replies to +plugins yet, nor do we send human-readable error messages using this +facility. =item Block Status diff --git a/server/protocol.h b/server/protocol.h index 003fc45..0aadd46 100644 --- a/server/protocol.h +++ b/server/protocol.h @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2013-2018 Red Hat Inc. + * Copyright (C) 2013-2019 Red Hat Inc. * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -98,12 +98,13 @@ extern const char *name_of_nbd_flag (int); /* NBD options (new style handshake only). */ extern const char *name_of_nbd_opt (int); -#define NBD_OPT_EXPORT_NAME 1 -#define NBD_OPT_ABORT 2 -#define NBD_OPT_LIST 3 -#define NBD_OPT_STARTTLS 5 -#define NBD_OPT_INFO 6 -#define NBD_OPT_GO 7 +#define NBD_OPT_EXPORT_NAME 1 +#define NBD_OPT_ABORT 2 +#define NBD_OPT_LIST 3 +#define NBD_OPT_STARTTLS 5 +#define NBD_OPT_INFO 6 +#define NBD_OPT_GO 7 +#define NBD_OPT_STRUCTURED_REPLY 8 extern const char *name_of_nbd_rep (int); #define NBD_REP_ACK 1 @@ -144,15 +145,49 @@ struct request { uint32_t count; /* Request length. */ } __attribute__((packed)); -/* Reply (server -> client). */ -struct reply { - uint32_t magic; /* NBD_REPLY_MAGIC. */ +/* Simple reply (server -> client). */ +struct simple_reply { + uint32_t magic; /* NBD_SIMPLE_REPLY_MAGIC. */ uint32_t error; /* NBD_SUCCESS or one of NBD_E*. */ uint64_t handle; /* Opaque handle. */ } __attribute__((packed)); -#define NBD_REQUEST_MAGIC 0x25609513 -#define NBD_REPLY_MAGIC 0x67446698 +/* Structured reply (server -> client). */ +struct structured_reply { + uint32_t magic; /* NBD_STRUCTURED_REPLY_MAGIC. */ + uint16_t flags; /* NBD_REPLY_FLAG_* */ + uint16_t type; /* NBD_REPLY_TYPE_* */ + uint64_t handle; /* Opaque handle. */ + uint32_t length; /* Length of payload which follows. */ +} __attribute__((packed)); + +struct structured_reply_offset_data { + uint64_t offset; /* offset */ + /* Followed by data. */ +} __attribute__((packed)); + +struct structured_reply_error { + uint32_t error; /* NBD_E* error number */ + uint16_t len; /* Length of human readable error. */ + /* Followed by human readable error string. */ +} __attribute__((packed)); + +#define NBD_REQUEST_MAGIC 0x25609513 +#define NBD_SIMPLE_REPLY_MAGIC 0x67446698 +#define NBD_STRUCTURED_REPLY_MAGIC 0x668e33ef + +/* Structured reply flags. */ +extern const char *name_of_nbd_reply_flag (int); +#define NBD_REPLY_FLAG_DONE (1<<0) + +/* Structured reply types. */ +extern const char *name_of_nbd_reply_type (int); +#define NBD_REPLY_TYPE_NONE 0 +#define NBD_REPLY_TYPE_OFFSET_DATA 1 +#define NBD_REPLY_TYPE_OFFSET_HOLE 2 +#define NBD_REPLY_TYPE_BLOCK_STATUS 3 +#define NBD_REPLY_TYPE_ERROR 32769 +#define NBD_REPLY_TYPE_ERROR_OFFSET 32770 /* NBD commands. */ extern const char *name_of_nbd_cmd (int); diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c index 674f4a4..2f494cd 100644 --- a/plugins/nbd/nbd.c +++ b/plugins/nbd/nbd.c @@ -345,7 +345,7 @@ nbd_request (struct handle *h, uint16_t flags, uint16_t type, uint64_t offset, static int nbd_reply_raw (struct handle *h, int *fd) { - struct reply rep; + struct simple_reply rep; struct transaction *trans; void *buf; uint32_t count; @@ -353,7 +353,7 @@ nbd_reply_raw (struct handle *h, int *fd) *fd = -1; if (read_full (h->fd, &rep, sizeof rep) < 0) return nbd_mark_dead (h); - if (be32toh (rep.magic) != NBD_REPLY_MAGIC) + if (be32toh (rep.magic) != NBD_SIMPLE_REPLY_MAGIC) return nbd_mark_dead (h); nbdkit_debug ("received reply for cookie %#" PRIx64 ", status %s", rep.handle, name_of_nbd_error(be32toh (rep.error))); diff --git a/server/connections.c b/server/connections.c index 51d4912..aeb27f8 100644 --- a/server/connections.c +++ b/server/connections.c @@ -86,6 +86,7 @@ struct connection { bool can_fua; bool can_multi_conn; bool using_tls; + bool structured_replies; int sockin, sockout; connection_recv_function recv; @@ -905,6 +906,26 @@ _negotiate_handshake_newstyle_options (struct connection *conn) break; + case NBD_OPT_STRUCTURED_REPLY: + if (optlen != 0) { + if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_INVALID) + == -1) + return -1; + if (conn_recv_full (conn, data, optlen, + "read: %s: %m", name_of_nbd_opt (option)) == -1) + return -1; + continue; + } + + debug ("newstyle negotiation: %s: client requested structured replies", + name_of_nbd_opt (option)); + + if (send_newstyle_option_reply (conn, option, NBD_REP_ACK) == -1) + return -1; + + conn->structured_replies = true; + break; + default: /* Unknown option. */ if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_UNSUP) == -1) @@ -1224,12 +1245,123 @@ nbd_errno (int error) } } +static int +send_simple_reply (struct connection *conn, + uint64_t handle, uint16_t cmd, + const char *buf, uint32_t count, + uint32_t error) +{ + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&conn->write_lock); + struct simple_reply reply; + int r; + + reply.magic = htobe32 (NBD_SIMPLE_REPLY_MAGIC); + reply.handle = handle; + reply.error = htobe32 (nbd_errno (error)); + + r = conn->send (conn, &reply, sizeof reply); + if (r == -1) { + nbdkit_error ("write reply: %s: %m", name_of_nbd_cmd (cmd)); + return set_status (conn, -1); + } + + /* Send the read data buffer. */ + if (cmd == NBD_CMD_READ && !error) { + r = conn->send (conn, buf, count); + if (r == -1) { + nbdkit_error ("write data: %s: %m", name_of_nbd_cmd (cmd)); + return set_status (conn, -1); + } + } + + return 1; /* command processed ok */ +} + +static int +send_structured_reply_read (struct connection *conn, + uint64_t handle, uint16_t cmd, + const char *buf, uint32_t count, uint64_t offset) +{ + /* Once we are really using structured replies and sending data back + * in chunks, we'll be able to grab the write lock for each chunk, + * allowing other threads to interleave replies. As we're not doing + * that yet we acquire the lock for the whole function. + */ + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&conn->write_lock); + struct structured_reply reply; + struct structured_reply_offset_data offset_data; + int r; + + assert (cmd == NBD_CMD_READ); + + reply.magic = htobe32 (NBD_STRUCTURED_REPLY_MAGIC); + reply.handle = handle; + reply.flags = htobe16 (NBD_REPLY_FLAG_DONE); + reply.type = htobe16 (NBD_REPLY_TYPE_OFFSET_DATA); + reply.length = htobe32 (sizeof offset_data + count); + + r = conn->send (conn, &reply, sizeof reply); + if (r == -1) { + nbdkit_error ("write reply: %s: %m", name_of_nbd_cmd (cmd)); + return set_status (conn, -1); + } + + /* Send the offset + read data buffer. */ + offset_data.offset = htobe64 (offset); + r = conn->send (conn, &offset_data, sizeof offset_data); + if (r == -1) { + nbdkit_error ("write data: %s: %m", name_of_nbd_cmd (cmd)); + return set_status (conn, -1); + } + + r = conn->send (conn, buf, count); + if (r == -1) { + nbdkit_error ("write data: %s: %m", name_of_nbd_cmd (cmd)); + return set_status (conn, -1); + } + + return 1; /* command processed ok */ +} + +static int +send_structured_reply_error (struct connection *conn, + uint64_t handle, uint16_t cmd, uint32_t error) +{ + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&conn->write_lock); + struct structured_reply reply; + struct structured_reply_error error_data; + int r; + + reply.magic = htobe32 (NBD_STRUCTURED_REPLY_MAGIC); + reply.handle = handle; + reply.flags = htobe16 (NBD_REPLY_FLAG_DONE); + reply.type = htobe16 (NBD_REPLY_TYPE_ERROR); + reply.length = htobe32 (sizeof error_data + 0 /* no human readable error */); + + r = conn->send (conn, &reply, sizeof reply); + if (r == -1) { + nbdkit_error ("write error reply: %m"); + return set_status (conn, -1); + } + + /* Send the error. */ + error_data.error = htobe32 (error); + error_data.len = htobe16 (0); + r = conn->send (conn, &error_data, sizeof error_data); + if (r == -1) { + nbdkit_error ("write data: %s: %m", name_of_nbd_cmd (cmd)); + return set_status (conn, -1); + } + /* No human readable error message at the moment. */ + + return 1; /* command processed ok */ +} + static int recv_request_send_reply (struct connection *conn) { int r; struct request request; - struct reply reply; uint16_t cmd, flags; uint32_t magic, count, error = 0; uint64_t offset; @@ -1317,40 +1449,33 @@ recv_request_send_reply (struct connection *conn) /* Send the reply packet. */ send_reply: - { - ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&conn->write_lock); - if (get_status (conn) < 0) - return -1; - reply.magic = htobe32 (NBD_REPLY_MAGIC); - reply.handle = request.handle; - reply.error = htobe32 (nbd_errno (error)); + if (get_status (conn) < 0) + return -1; - if (error != 0) { - /* Since we're about to send only the limited NBD_E* errno to the - * client, don't lose the information about what really happened - * on the server side. Make sure there is a way for the operator - * to retrieve the real error. - */ - debug ("sending error reply: %s", strerror (error)); - } - - r = conn->send (conn, &reply, sizeof reply); - if (r == -1) { - nbdkit_error ("write reply: %s: %m", name_of_nbd_cmd (cmd)); - return set_status (conn, -1); - } - - /* Send the read data buffer. */ - if (cmd == NBD_CMD_READ && !error) { - r = conn->send (conn, buf, count); - if (r == -1) { - nbdkit_error ("write data: %s: %m", name_of_nbd_cmd (cmd)); - return set_status (conn, -1); - } - } + if (error != 0) { + /* Since we're about to send only the limited NBD_E* errno to the + * client, don't lose the information about what really happened + * on the server side. Make sure there is a way for the operator + * to retrieve the real error. + */ + debug ("sending error reply: %s", strerror (error)); } - return 1; /* command processed ok */ + /* Currently we prefer to send simple replies for everything except + * where we have to (ie. NBD_CMD_READ when structured_replies have + * been negotiated). However this prevents us from sending + * human-readable error messages to the client, so we should + * reconsider this in future. + */ + if (conn->structured_replies && cmd == NBD_CMD_READ) { + if (!error) + return send_structured_reply_read (conn, request.handle, cmd, + buf, count, offset); + else + return send_structured_reply_error (conn, request.handle, cmd, error); + } + else + return send_simple_reply (conn, request.handle, cmd, buf, count, error); } /* Write buffer to conn->sockout and either succeed completely diff --git a/tests/test-layers.c b/tests/test-layers.c index c8d15d2..6149c02 100644 --- a/tests/test-layers.c +++ b/tests/test-layers.c @@ -92,7 +92,7 @@ main (int argc, char *argv[]) struct new_handshake_finish handshake_finish; uint16_t eflags; struct request request; - struct reply reply; + struct simple_reply reply; char data[512]; #ifndef HAVE_EXIT_WITH_PARENT -- 2.20.1
On 3/8/19 4:04 AM, Richard W.M. Jones wrote:
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.
Yep, sounds compliant, and similar to how I added it in qemu. I'll read through this one in detail, but interoperability with qemu is already a good sign that it's workable.
Also we do not handle human-readable error messages yet because that would require changes in how we handle nbdkit_error().
Also we do not understand NBD_CMD_FLAG_DF, but that seems to be OK because (a) we don't advertize the feature and (b) we only send back a single chunk anyway.
Or, we COULD advertise it because we always honor it (but that's a larger diffstat, and thus at odds with "minimal implementation"). Either way works.
+/* Structured reply types. */ +extern const char *name_of_nbd_reply_type (int); +#define NBD_REPLY_TYPE_NONE 0 +#define NBD_REPLY_TYPE_OFFSET_DATA 1 +#define NBD_REPLY_TYPE_OFFSET_HOLE 2 +#define NBD_REPLY_TYPE_BLOCK_STATUS 3 +#define NBD_REPLY_TYPE_ERROR 32769 +#define NBD_REPLY_TYPE_ERROR_OFFSET 32770
Worth writing these later ones in hex or via a helper macro that does ((1 << 15) | value)? Or would that mess up the generated protocol-to-lookup magic?
+++ b/plugins/nbd/nbd.c @@ -345,7 +345,7 @@ nbd_request (struct handle *h, uint16_t flags, uint16_t type, uint64_t offset, static int nbd_reply_raw (struct handle *h, int *fd) { - struct reply rep; + struct simple_reply rep; struct transaction *trans; void *buf; uint32_t count; @@ -353,7 +353,7 @@ nbd_reply_raw (struct handle *h, int *fd) *fd = -1; if (read_full (h->fd, &rep, sizeof rep) < 0) return nbd_mark_dead (h); - if (be32toh (rep.magic) != NBD_REPLY_MAGIC) + if (be32toh (rep.magic) != NBD_SIMPLE_REPLY_MAGIC) return nbd_mark_dead (h); nbdkit_debug ("received reply for cookie %#" PRIx64 ", status %s", rep.handle, name_of_nbd_error(be32toh (rep.error)));
Teaching the nbd plugin to negotiate structured replies as the client is obviously a separate patch, I'm fine if you leave that to me :)
+static int +send_structured_reply_read (struct connection *conn, + uint64_t handle, uint16_t cmd, + const char *buf, uint32_t count, uint64_t offset) +{ + /* Once we are really using structured replies and sending data back + * in chunks, we'll be able to grab the write lock for each chunk, + * allowing other threads to interleave replies. As we're not doing + * that yet we acquire the lock for the whole function. + */
Fair enough.
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&conn->write_lock); + struct structured_reply reply; + struct structured_reply_offset_data offset_data; + int r; + + assert (cmd == NBD_CMD_READ); + + reply.magic = htobe32 (NBD_STRUCTURED_REPLY_MAGIC); + reply.handle = handle; + reply.flags = htobe16 (NBD_REPLY_FLAG_DONE); + reply.type = htobe16 (NBD_REPLY_TYPE_OFFSET_DATA); + reply.length = htobe32 (sizeof offset_data + count);
This line is correct, but I had to remind myself of C precedence rules on this one; writing 'count + sizeof offset_data' instead has the same effect without worrying whether sizeof binds with higher or lower precedence than +.
@@ -1317,40 +1449,33 @@ recv_request_send_reply (struct connection *conn)
/* Send the reply packet. */ send_reply: - { - ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&conn->write_lock); - if (get_status (conn) < 0) - return -1; - reply.magic = htobe32 (NBD_REPLY_MAGIC); - reply.handle = request.handle; - reply.error = htobe32 (nbd_errno (error)); + if (get_status (conn) < 0) + return -1;
Hmm, previously get_status() was checked under lock. But since it is a thread-local variable, we should be safe grabbing it while unlocked. ACK. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Fri, Mar 08, 2019 at 09:03:05AM -0600, Eric Blake wrote:
On 3/8/19 4:04 AM, Richard W.M. Jones wrote:
Also we do not understand NBD_CMD_FLAG_DF, but that seems to be OK because (a) we don't advertize the feature and (b) we only send back a single chunk anyway.
Or, we COULD advertise it because we always honor it (but that's a larger diffstat, and thus at odds with "minimal implementation"). Either way works.
What's also unclear to me is how NBD_CMD_FLAG_DF interacts with NBD_CMD_BLOCK_STATUS. What does it mean for extents which are by their nature fragmented?
+/* Structured reply types. */ +extern const char *name_of_nbd_reply_type (int); +#define NBD_REPLY_TYPE_NONE 0 +#define NBD_REPLY_TYPE_OFFSET_DATA 1 +#define NBD_REPLY_TYPE_OFFSET_HOLE 2 +#define NBD_REPLY_TYPE_BLOCK_STATUS 3 +#define NBD_REPLY_TYPE_ERROR 32769 +#define NBD_REPLY_TYPE_ERROR_OFFSET 32770
Worth writing these later ones in hex or via a helper macro that does ((1 << 15) | value)? Or would that mess up the generated protocol-to-lookup magic?
Could do it either way really. The sed magic uses the symbol (eg. NBD_REPLY_TYPE_ERROR) not the value so either should work. I'll play around with it to see which looks nicer.
+ reply.magic = htobe32 (NBD_STRUCTURED_REPLY_MAGIC); + reply.handle = handle; + reply.flags = htobe16 (NBD_REPLY_FLAG_DONE); + reply.type = htobe16 (NBD_REPLY_TYPE_OFFSET_DATA); + reply.length = htobe32 (sizeof offset_data + count);
This line is correct, but I had to remind myself of C precedence rules on this one; writing 'count + sizeof offset_data' instead has the same effect without worrying whether sizeof binds with higher or lower precedence than +.
Yup I'll change this. Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
On 3/8/19 10:05 AM, Richard W.M. Jones wrote:
On Fri, Mar 08, 2019 at 09:03:05AM -0600, Eric Blake wrote:
On 3/8/19 4:04 AM, Richard W.M. Jones wrote:
Also we do not understand NBD_CMD_FLAG_DF, but that seems to be OK because (a) we don't advertize the feature and (b) we only send back a single chunk anyway.
Or, we COULD advertise it because we always honor it (but that's a larger diffstat, and thus at odds with "minimal implementation"). Either way works.
What's also unclear to me is how NBD_CMD_FLAG_DF interacts with NBD_CMD_BLOCK_STATUS. What does it mean for extents which are by their nature fragmented?
FLAG_DF can only be requested for NBD_CMD_READ. It requires the server to advertise that it is supported before the client can use it (we could advertise it now while we don't have sparse reads, but we'd have to be more careful once we do figure out how plugins can do fragmented reads). Part of the reason that it is advertised is that NBD_CMD_READ is pre-existing, so we didn't want to break existing servers not expecting a new flag. FLAG_REQ_ONE can only be requested for NBD_CMD_BLOCK_STATUS. It does not require the server to advertise support: the client can blindly use it, because all known servers that implement BLOCK_STATUS can easily support it, and because BLOCK_STATUS is new, we can expect all future implementations to be able to comply without the worry of back-compat that CMD_READ has. Note that since we still don't negotiate any contexts, we don't have to worry about it, yet. There's no command where you can mix the two flags (even though they have distinct bit values); setting both flags should fail with -EINVAL due to a non-permitted flag for the given command. Hopefully that helps. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
participants (2)
-
Eric Blake -
Richard W.M. Jones