On 07/02/2017 00:01, Eric Blake wrote: > On 02/03/2017 09:47 AM, Vladimir Sementsov-Ogievskiy wrote: >> Minimal implementation of structured read: one data chunk + finishing >> none chunk. No segmentation. >> Minimal structured error implementation: no text message. >> Support DF flag, but just ignore it, as there is no segmentation any >> way. > > Might be worth adding that this is still an experimental extension to > the NBD spec, and therefore that this implementation serves as proof of > concept and may still need tweaking if anything major turns up before > promoting it to stable. It might also be worth a link to: > > https://github.com/NetworkBlockDevice/nbd/blob/extension-structured-reply/doc/proto.md Wouter's slides from FOSDEM said the state is "discussion complete, not yet implemented". Paolo >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy >> --- >> include/block/nbd.h | 31 +++++++++++++ >> nbd/nbd-internal.h | 2 + >> nbd/server.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++-- >> 3 files changed, 154 insertions(+), 4 deletions(-) >> >> diff --git a/include/block/nbd.h b/include/block/nbd.h >> index 3c65cf8d87..58b864f145 100644 >> --- a/include/block/nbd.h >> +++ b/include/block/nbd.h >> @@ -70,6 +70,25 @@ struct NBDSimpleReply { >> }; >> typedef struct NBDSimpleReply NBDSimpleReply; >> >> +typedef struct NBDStructuredReplyChunk { >> + uint32_t magic; >> + uint16_t flags; >> + uint16_t type; >> + uint64_t handle; >> + uint32_t length; >> +} QEMU_PACKED NBDStructuredReplyChunk; >> + >> +typedef struct NBDStructuredRead { >> + NBDStructuredReplyChunk h; >> + uint64_t offset; >> +} QEMU_PACKED NBDStructuredRead; >> + >> +typedef struct NBDStructuredError { >> + NBDStructuredReplyChunk h; >> + uint32_t error; >> + uint16_t message_length; >> +} QEMU_PACKED NBDStructuredError; > > Definitely a subset of all types added in the NBD protocol extension, > but reasonable for a minimal implementation. Might be worth adding > comments to the types... > >> >> +/* Structured reply flags */ >> +#define NBD_REPLY_FLAG_DONE 1 >> + >> +/* Structured reply types */ >> +#define NBD_REPLY_TYPE_NONE 0 >> +#define NBD_REPLY_TYPE_OFFSET_DATA 1 >> +#define NBD_REPLY_TYPE_OFFSET_HOLE 2 >> +#define NBD_REPLY_TYPE_ERROR ((1 << 15) + 1) >> +#define NBD_REPLY_TYPE_ERROR_OFFSET ((1 << 15) + 2) > > ...that correspond to these constants that will be used in the [h.]type > field. > > Also, it's a bit odd that you are defining constants that aren't > implemented here; I don't know if it is any cleaner to save the > definition for the unimplemented types until you actually implement them > (NBD_REPLY_TYPE_OFFSET_HOLE, NBD_REPLY_TYPE_ERROR_OFFSET). > >> +++ b/nbd/nbd-internal.h >> @@ -60,6 +60,7 @@ >> #define NBD_REPLY_SIZE (4 + 4 + 8) >> #define NBD_REQUEST_MAGIC 0x25609513 >> #define NBD_SIMPLE_REPLY_MAGIC 0x67446698 >> +#define NBD_STRUCTURED_REPLY_MAGIC 0x668e33ef >> #define NBD_OPTS_MAGIC 0x49484156454F5054LL >> #define NBD_CLIENT_MAGIC 0x0000420281861253LL >> #define NBD_REP_MAGIC 0x0003e889045565a9LL > > I would not be bothered if you wanted to reindent the other lines by 3 > spaces so that all the macro definitions start on the same column. But > I also won't require it. > >> @@ -81,6 +82,7 @@ >> #define NBD_OPT_LIST (3) >> #define NBD_OPT_PEEK_EXPORT (4) >> #define NBD_OPT_STARTTLS (5) >> +#define NBD_OPT_STRUCTURED_REPLY (8) > > Similar comments about consistency in the definition column. > >> +++ b/nbd/server.c >> @@ -100,6 +100,8 @@ struct NBDClient { >> QTAILQ_ENTRY(NBDClient) next; >> int nb_requests; >> bool closing; >> + >> + bool structured_reply; >> }; >> >> /* That's all folks */ >> @@ -573,6 +575,16 @@ static int nbd_negotiate_options(NBDClient *client) >> return ret; >> } >> break; >> + >> + case NBD_OPT_STRUCTURED_REPLY: >> + client->structured_reply = true; >> + ret = nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, >> + clientflags); >> + if (ret < 0) { >> + return ret; >> + } >> + break; >> + > > As written, you allow the client to negotiate this more than once. On > the one hand, we are idempotent, so it doesn't hurt if they do so; on > the other hand, it is a waste of bandwidth, and a client could abuse it > by sending an infinite stream of NBD_OPT_STRUCTURED_REPLY requests and > never moving into transmission phase, which is a mild form of > Denial-of-Service (they're hogging a socket from accomplishing useful > work for some other client). It would be acceptable if we wanted to > disconnect any client that sends this option more than once, although > the NBD spec does not require us to do so. Up to you if you think > that's worth adding. > >> >> +static void set_be_chunk(NBDStructuredReplyChunk *chunk, uint16_t flags, >> + uint16_t type, uint64_t handle, uint32_t length) > > I'm not sure I like the name of this helper. I know what you are doing: > go from native-endian local variables into network-byte-order storage in > preparation for transmission, done at the last possible moment. But I > also don't know if I have a good suggestion for a better name off hand. > >> +{ >> + stl_be_p(&chunk->magic, NBD_STRUCTURED_REPLY_MAGIC); >> + stw_be_p(&chunk->flags, flags); >> + stw_be_p(&chunk->type, type); >> + stq_be_p(&chunk->handle, handle); >> + stl_be_p(&chunk->length, length); >> +} >> + >> +static int nbd_co_send_iov(NBDClient *client, struct iovec *iov, unsigned niov) > > Probably should add the coroutine_fn annotation to this function and its > friends (yeah, the NBD code doesn't consistently use it yet, but it should). > >> @@ -1147,7 +1239,8 @@ static ssize_t nbd_co_receive_request(NBDRequestData *req, >> rc = request->type == NBD_CMD_WRITE ? -ENOSPC : -EINVAL; >> goto out; >> } >> - if (request->flags & ~(NBD_CMD_FLAG_FUA | NBD_CMD_FLAG_NO_HOLE)) { >> + if (request->flags & ~(NBD_CMD_FLAG_FUA | NBD_CMD_FLAG_NO_HOLE | >> + NBD_CMD_FLAG_DF)) { >> LOG("unsupported flags (got 0x%x)", request->flags); >> rc = -EINVAL; >> goto out; > > Missing a check that NBD_CMD_FLAG_DF is only set for NBD_CMD_READ (it is > not valid on any other command, at least in the current version of the > extension specification). > >> @@ -1444,6 +1559,8 @@ void nbd_client_new(NBDExport *exp, >> client->can_read = true; >> client->close = close_fn; >> >> + client->structured_reply = false; > > Dead assignment, since we used 'client = g_malloc0()' above. > > Overall looks like it matches the spec. >