search for: simple_reply

This was v3 patch 2/22, reworked to address the confusion about how a structured reply header is read in two pieces before getting to the payload portion. I'm still working on rebasing the rest of my v3 series (patches 1, 3-22) from other comments given, but this seemed independent enough that it's worth posting now rather than holding it up for the rest of the series. Eric Blake (4):

...yout (on x86_64) from: > > offset simple structured > +------------------------------------------------------------+ > | union sbuf | > | +---------------------+------------------------------+ | > | | struct simple_reply | struct sr | | > | | +-----------------+ | +--------------------------+ | | > | | | | | | struct structured_reply | | | > | | | | | | +----------------------+ | | | > | 0 | | uint32_t magic | | | | uint32_t magic...

I thought this should produce a fairly dramatic performance gain. In fact I couldn't measure any performance difference at all. I think what's happening is we're actually paying an extra syscall (to discover the socket would block) and then doing the poll anyway. So I don't know if it's worth having this patch. It could be argued that it makes the code shorter (therefore

...| 2 + 4 files changed, 45 insertions(+), 64 deletions(-) diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c index 7e5340c..12536e0 100644 --- a/generator/states-reply-simple.c +++ b/generator/states-reply-simple.c @@ -20,24 +20,15 @@ /* STATE MACHINE */ { REPLY.SIMPLE_REPLY.START: - struct command_in_flight *cmd; + struct command_in_flight *cmd = h->reply_cmd; uint32_t error; uint64_t handle; error = be32toh (h->sbuf.simple_reply.error); handle = be64toh (h->sbuf.simple_reply.handle); - /* Find the command amongst the commands in flight. */ -...

...P_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 +/* Str...

..., 9 deletions(-) > > diff --git a/generator/states-reply.c b/generator/states-reply.c > index 5be3431..f0ef47c 100644 > --- a/generator/states-reply.c > +++ b/generator/states-reply.c > @@ -36,7 +36,7 @@ > h->rbuf = &h->sbuf; > h->rlen = sizeof h->sbuf.simple_reply; > > - r = h->sock->ops->recv (h->sock, h->rbuf, h->rlen); > + r = h->sock->ops->recv (h, h->sock, h->rbuf, h->rlen); Do we need to pass both h and h->sock, or can the caller access h->sock through h? (Maybe I should read patch 4 first?) Oth...

...nbd_request { uint32_t magic; /* NBD_REQUEST_MAGIC. */ uint16_t flags; /* Request flags. */ uint16_t type; /* Request type. */ @@ -161,14 +174,14 @@ struct request { } __attribute__((packed)); /* Simple reply (server -> client). */ -struct simple_reply { +struct nbd_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)); /* Structured reply (server -> client). */ -struct...

This isn't quite finished because not all of the tests or examples have been updated, but it demonstrates an idea: Should we forget about the concept of having multiple connections managed under a single handle? In this patch there is a single ‘struct nbd_handle *’ which manages a single state machine and connection (and therefore no nbd_connection). To connect to a multi-conn server you must

I've mentioned this topic before (in fact, the idea of adding NBD_CMD_FLAG_DF was first mentioned at [1]), but finally finished enough of an implementation to feel confident in posting it. I'd still like to add something under examples/ that uses the new API to implement strict checking of a server's structured replies read implementation (ensure that a server never sends data after

...er (converted to local errno - value) on success, or -1 on read failure. */ + value) on success, or -1 on read failure. If structured replies + were negotiated, fd is set to -1 if there are still more replies + expected. */ static int nbd_reply_raw (struct handle *h, int *fd) { - struct simple_reply rep; + union { + struct simple_reply simple; + struct structured_reply structured; + } rep; struct transaction *trans; - void *buf; + void *buf = NULL; uint32_t count; + int error = NBD_SUCCESS; + bool more = false; + uint64_t offset = 0; /* absolute offset of structured read chu...

...ACK_VALID; + uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags); + + if (flags & NBD_REPLY_FLAG_DONE) + valid |= LIBNBD_CALLBACK_FREE; + return valid; +} + +/*----- End of prologue. -----*/ + /* STATE MACHINE */ { REPLY.STRUCTURED_REPLY.START: /* We've only read the simple_reply. The structured_reply is longer, @@ -293,16 +306,19 @@ } if (cmd->type == NBD_CMD_READ && cmd->cb.fn.read) { int scratch = error; + unsigned valid = valid_flags (h); /* Different from successful reads: inform the callback about the *...

...+++-- 5 files changed, 13 insertions(+), 9 deletions(-) diff --git a/generator/states-reply.c b/generator/states-reply.c index 5be3431..f0ef47c 100644 --- a/generator/states-reply.c +++ b/generator/states-reply.c @@ -36,7 +36,7 @@ h->rbuf = &h->sbuf; h->rlen = sizeof h->sbuf.simple_reply; - r = h->sock->ops->recv (h->sock, h->rbuf, h->rlen); + r = h->sock->ops->recv (h, h->sock, h->rbuf, h->rlen); if (r == -1) { /* This should never happen because when we enter this state we * should have notification that the socket is ready to...

...hat 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)...

...*----------------------------------------------------------------------*) diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c index 935f6d2..ddc91ce 100644 --- a/generator/states-reply-simple.c +++ b/generator/states-reply-simple.c @@ -49,9 +49,22 @@ return 0; REPLY.SIMPLE_REPLY.RECV_READ_PAYLOAD: + struct command_in_flight *cmd = h->reply_cmd; + switch (recv_into_rbuf (h)) { case -1: SET_NEXT_STATE (%.DEAD); return -1; - case 0: SET_NEXT_STATE (%^FINISH_COMMAND); + case 0: + /* guaranteed by START */ + assert (cmd); + if (cmd->cb.fn.read) { +...

...*----------------------------------------------------------------------*) diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c index 935f6d2..87622a0 100644 --- a/generator/states-reply-simple.c +++ b/generator/states-reply-simple.c @@ -49,9 +49,22 @@ return 0; REPLY.SIMPLE_REPLY.RECV_READ_PAYLOAD: + struct command_in_flight *cmd = h->reply_cmd; + switch (recv_into_rbuf (h)) { case -1: SET_NEXT_STATE (%.DEAD); return -1; - case 0: SET_NEXT_STATE (%^FINISH_COMMAND); + case 0: + /* guaranteed by START */ + assert (cmd); + if (cmd->cb.fn.read) { +...

...00644 --- a/lib/internal.h +++ b/lib/internal.h @@ -127,6 +127,7 @@ struct nbd_connection { } __attribute__((packed)) context; } payload; } __attribute__((packed)) or; + struct nbd_export_name_option_reply export_name_reply; struct nbd_request request; struct nbd_simple_reply simple_reply; struct { diff --git a/lib/nbd-protocol.h b/lib/nbd-protocol.h index 398403a..4bda056 100644 --- a/lib/nbd-protocol.h +++ b/lib/nbd-protocol.h @@ -74,6 +74,13 @@ struct nbd_new_option { /* option data follows */ } __attribute__((packed)); +/* Newstyle handshake OPT_EXPORT_NAM...

...gotiated, fd is set to -1 if there are still more replies + were negotiated, trans_out is set to NULL if there are still more replies expected. */ static int -nbd_reply_raw (struct handle *h, int *fd) +nbd_reply_raw (struct handle *h, struct transaction **trans_out) { union { struct simple_reply simple; @@ -387,7 +382,7 @@ nbd_reply_raw (struct handle *h, int *fd) bool zero = false; /* if len, whether to read or memset */ uint16_t errlen; - *fd = -1; + *trans_out = NULL; /* magic and handle overlap between simple and structured replies */ if (read_full (h->fd, &rep, s...

...conn->send (conn, name, namelen, 0) == -1) { nbdkit_error ("write: %s: %m", name_of_nbd_opt (option)); return -1; diff --git a/server/protocol.c b/server/protocol.c index 0e054ee..5967622 100644 --- a/server/protocol.c +++ b/server/protocol.c @@ -393,12 +393,13 @@ send_simple_reply (struct connection *conn, ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&conn->write_lock); struct simple_reply reply; int r; + int f = (cmd == NBD_CMD_READ && !error) ? SEND_MORE : 0; reply.magic = htobe32 (NBD_SIMPLE_REPLY_MAGIC); reply.handle = handle; reply.error = htobe32...

We should have only one NBD protocol file. Let's make nbdkit's version the canonical one, and use it in libnbd. Rich.

Updated based on other changes that have happened in the meantime: - rely more on cleanup.h (throughout) - split structured read for easier review (patch 2 and 3 were combined in v1) - rely on nbdkit not leaking a server's partial answer (patch 3) - add tests (patch 5) - other bug fixes I found while testing it - drop EOVERFLOW patch for now; it will be separate once upstream NBD protocol