search for: nbd_reply_type_error_offset

...r all others */ + h->rbuf = NULL; + h->rlen = length; + switch (type) { + case NBD_REPLY_TYPE_ERROR: + if (length != 0) { + SET_NEXT_STATE (%.DEAD); + set_error (0, "error payload length too large"); + return -1; + } + break; + case NBD_REPLY_TYPE_ERROR_OFFSET: + if (length != sizeof h->sbuf.offset) { + SET_NEXT_STATE (%.DEAD); + set_error (0, "error payload length too large"); + return -1; + } + h->rbuf = &h->sbuf.offset; + break; + } + SET_NEXT_STATE (%RECV_ERROR_TAIL); + } + retur...

...on as they are available rather than waiting for the entire buffer to be reconstructed. As such, it makes sense to have an optional callback invoked each time we finish reading a chunk (for simple replies, we behave as if everything was a single data chunk). This also requires special handling of NBD_REPLY_TYPE_ERROR_OFFSET, as that is the one situation where we want to call the callback to inform them that the server pointed out a specific offset for the error; all other errors continue to affect only the final pread return value without any use of the callback. If any callback fails, and if no prior error was set,...

...on as they are available rather than waiting for the entire buffer to be reconstructed. As such, it makes sense to have an optional callback invoked each time we finish reading a chunk (for simple replies, we behave as if everything was a single data chunk). This also requires special handling of NBD_REPLY_TYPE_ERROR_OFFSET, as that is the one situation where we want to call the callback to inform them that the server pointed out a specific offset for the error - a client can utilize that knowledge to form a wrapper that permits short reads (rather than all-or-nothing failures). But as that is the only error with an o...

...Int for the callback status rather than an enum for ease of wiring things up to the various language bindings. It's easy to test callback behavior for LIBNBD_READ_DATA/HOLE (the next patch will add interop tests with qemu-nbd), but at present, I don't know of any server that responds with NBD_REPLY_TYPE_ERROR_OFFSET, which in turn makes testing LIBNBD_READ_ERROR difficult. I used the following hack on top of qemu.git commit d1bf88e5 to trigger an offset error for local testing: | diff --git i/nbd/server.c w/nbd/server.c | index 10faedcfc55d..804dced0bc8d 100644 | --- i/nbd/server.c | +++ w/nbd/server.c | @@ -...

...t;<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,...

...kie; > + }; > + } hdr; > union { > uint64_t align_; /* Start sr.payload on an 8-byte alignment */ > struct nbd_structured_reply_offset_data offset_data; > @@ -250,7 +260,7 @@ struct nbd_handle { > uint64_t offset; /* Only used for NBD_REPLY_TYPE_ERROR_OFFSET */ > } NBD_ATTRIBUTE_PACKED error; > } payload; > - } sr; > + } reply; > uint16_t gflags; > uint32_t cflags; > uint32_t len; > diff --git a/generator/states-reply.c b/generator/states-reply.c > index bd6336a8..af5f6135 100644 > ---...

...anguage bindings. Still, it required quite a few tweaks to the generator to support an Int type in a callback. It's easy to test callback behavior for LIBNBD_READ_DATA/HOLE (the next patch will add interop tests with qemu-nbd), but at present, I don't know of any server that responds with NBD_REPLY_TYPE_ERROR_OFFSET, which in turn makes testing LIBNBD_READ_ERROR difficult. I used the following hack on top of qemu.git commit d1bf88e5 to trigger an offset error for local testing: | diff --git i/nbd/server.c w/nbd/server.c | index 10faedcfc55d..804dced0bc8d 100644 | --- i/nbd/server.c | +++ w/nbd/server.c | @@ -...

...es. */ > +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,...

...*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...

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):

Since v1: - rebase to applied patches - split out support for Int in callbacks - sort of test that callbacks work in OCaml (see comment in patch 5) - rename API to nbd_pread_structured - expose error as explicit parameter to callback Eric Blake (5): generator: Allow Int in callbacks states: Wire in a read callback states: Add nbd_pread_structured API states: Add tests for

...& (1<<15))) /* 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 @@ -213,7 +206,6 @@ extern const char *name_of_nbd_reply_type (int); #define NBD_REPLY_TYPE_ERROR_OFFSET NBD_REPLY_TYPE_ERR (2) /* NBD commands. */ -extern const char *name_of_nbd_cmd (int); #define NBD_CMD_READ 0 #define NBD_CMD_WRITE 1 #define NBD_CMD_DISC 2 /* Disconnect. */ @@ -223,7 +215,6 @@ extern const char *name_of_nbd_cmd (int); #define NBD_CMD_WR...

Rather than moving the connection to DEAD, we can just ignore further contexts to the existing command handle, and fail the overall command with the same errno as the failed callback. Eric Blake (2): states: Track cmd->error as errno, not wire value api: Recover from block status callback failure generator/generator | 5 ++- generator/states-reply-simple.c | 2 +-

...st 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_BLOCK_STATUS 5 #define NBD_REPLY_TYPE_ERROR ((1<<15) + 1) #define NBD_REPLY_TYPE_ERROR_OFFSET ((1<<15) + 2) @@ -199,10 +212,12 @@ extern const char *name_of_nbd_cmd (int); #define NBD_CMD_FLUSH 3 #define NBD_CMD_TRIM 4 #define NBD_CMD_WRITE_ZEROES 6 +#define NBD_CMD_BLOCK_STATUS 7 extern const char *name_of_nbd_cmd_flag (int); #define NBD_CM...

I'm hoping to implement .extents for the nbd plugin; this is a prerequisite. I'm not sure about patch 3 - if we like it, I'll squash it to 2, if we don't, I think we are okay just dropping it. I'm also wondering if we have to worry about malicious plugins that don't populate the entire .pread buffer in an effort to get nbdkit to expose portions of the heap; my patch 7 loses

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

...eturn 0; case 1: save_reply_state (h); SET_NEXT_STATE (%.READY); @@ -235,14 +235,14 @@ if (length != 0) { SET_NEXT_STATE (%.DEAD); set_error (0, "error payload length too large"); - return -1; + return 0; } break; case NBD_REPLY_TYPE_ERROR_OFFSET: if (length != sizeof h->sbuf.sr.payload.error.offset) { SET_NEXT_STATE (%.DEAD); set_error (0, "invalid error payload length"); - return -1; + return 0; } h->rbuf = &h->sbuf.sr.payload.error.offset; break; @@ -258,7...

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

I still need to wire in the use of *_notify functions into nbdkit to prove whether it makes the code any faster or easier to maintain, but at least the added example shows one good use case for the new API. Eric Blake (6): api: Add nbd_aio_in_flight generator: Allow DEAD state actions to run generator: Allow Int64 in callbacks states: Prepare for aio notify callback api: Add new

I want to post this but mainly for discussion and early review. It's not safe for these patches to all go upstream yet (because not all filters have been checked/adjusted), but if any patches were to go upstream then probably 1 & 2 only are safe. File, VDDK, memory and data plugins all work, although I have only done minimal testing on them. The current tests, such as they are, all