search for: nbd_reply_type_non

...ng and sufficient, and we'd rather defer > such checking to an optional callback function. But a callback > function will only be reached per chunk, while we still want to fail > the overall read if the callback function was never called because the > server erroneously replied with NBD_REPLY_TYPE_NONE with no other > chunks instead of an expected NBD_REPLY_TYPE_ERROR*. For this specific > error case, the bookkeeping is much simpler - we merely track if we've > seen at least one data chunk. I guess the implicit assumption here is that count > 0, which IIRC the NBD protocol deman...

...d: %m"); + return nbd_mark_dead (h); + } + if (read_full (h->fd, buf, len)) { + free (buf); + return nbd_mark_dead (h); + } + len = 0; + } + more = !(rep.structured.flags & NBD_REPLY_FLAG_DONE); + switch (rep.structured.type) { + case NBD_REPLY_TYPE_NONE: + if (rep.structured.length) { + nbdkit_error ("NBD_REPLY_TYPE_NONE with invalid payload"); + free (buf); + return nbd_mark_dead (h); + } + if (more) { + nbdkit_error ("NBD_REPLY_TYPE_NONE without done flag"); + return nbd_mark...

...EPLY_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/plugin...

...sections of the source file. -/^extern const char \*name_of_/,/^$/ { +declare -A functions=( + [global_flag]=NBD_FLAG_FIXED_NEWSTYLE + [flag]=NBD_FLAG_HAS_FLAGS + [opt]=NBD_OPT_EXPORT_NAME + [rep]=NBD_REP_ACK + [info]=NBD_INFO_EXPORT + [reply]=NBD_REPLY_FLAG_DONE + [reply_type]=NBD_REPLY_TYPE_NONE + [cmd]=NBD_CMD_READ + [cmd_flag]=NBD_CMD_FLAG_FUA + [error]=NBD_SUCCESS +) - # Convert extern function prototype into a definition. - s/extern \(const char \*name_of_.*\) (int);/\1 (int fl) {\ - switch (fl) {/; - - # Convert #define lines into cases. - s/^#define \([_A-Z]*\).*/ c...

...t the last invocation. For nbd_aio_pread_structured and nbd_aio_block_status, the chunk/extent callback can either match server state (if the server replies replies with NBD_REPLY_FLAG_DONE on an OFFSET_DATA/BLOCK_STATUS reply, we could set VALID|FREE; if the server defers NBD_REPLY_FLAG_DONE to an NBD_REPLY_TYPE_NONE packet then the two flags will definitely not be set at the same time), or we could always defer the FREE flag until the overall command is ready to retire; for nbd_aio_FOO_callback, it may indeed be easier to set VALID|FREE on the single use of the callback at the time it is ready to retire. But...

...s were non-overlapping and sufficient, and we'd rather defer such checking to an optional callback function. But a callback function will only be reached per chunk, while we still want to fail the overall read if the callback function was never called because the server erroneously replied with NBD_REPLY_TYPE_NONE with no other chunks instead of an expected NBD_REPLY_TYPE_ERROR*. For this specific error case, the bookkeeping is much simpler - we merely track if we've seen at least one data chunk. --- generator/states-reply-simple.c | 1 + generator/states-reply-structured.c | 2 ++ lib/aio.c...

...--------------*) diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c index 6337dad..2125e41 100644 --- a/generator/states-reply-structured.c +++ b/generator/states-reply-structured.c @@ -103,7 +103,7 @@ set_error (0, "NBD_REPLY_FLAG_DONE must be set in NBD_REPLY_TYPE_NONE"); return -1; } - SET_NEXT_STATE (%^FINISH_COMMAND); + SET_NEXT_STATE (%FINISH); return 0; } else if (type == NBD_REPLY_TYPE_OFFSET_DATA) { @@ -225,7 +225,6 @@ REPLY.STRUCTURED_REPLY.RECV_ERROR_TAIL: struct command_in_flight *cmd; - uint16_t flags; uint6...

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

...er > 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 late...

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

Patch 1 played with an early draft of Rich's Fedora 30 libnbd package: https://bugzilla.redhat.com/show_bug.cgi?id=1713767#c17 Note that comment 21 provides a newer package 0.1.1-1 with a different API; and that libnbd has more unreleased API changes in the pipeline (whether that will be called 0.2 or 0.1.2); so we'll have to tweak things based on what is actually available in distros.

On Mon, Jul 22, 2019 at 10:08:25AM +0100, Richard W.M. Jones wrote: > On Sat, Jul 20, 2019 at 07:38:45AM +0100, Richard W.M. Jones wrote: > > More thoughts on callbacks, etc. following on from: > > https://www.redhat.com/archives/libguestfs/2019-July/thread.html#00184 > > > > Closure lifetimes > > ----------------- Here's a possibly better idea which still

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

libnbd-0.1.2-1 is now available in Fedora 29/30 updates-testing, although it was not compiled against libxml2 so it lacks uri support (I ended up testing patch 4 with a self-built libnbd). Diffs since v1 - rebase to master, bump from libnbd 0.1 to 0.1.2, add URI support, better timing results Still not done - patch 5 needs associated tests Eric Blake (5): nbd: Check for libnbd nbd:

...uctured reply payload: %m"); - return nbdplug_mark_dead (h); - } - if (read_full (h->fd, payload, len)) - return nbdplug_mark_dead (h); - len = 0; - } - more = !(rep.structured.flags & NBD_REPLY_FLAG_DONE); - switch (rep.structured.type) { - case NBD_REPLY_TYPE_NONE: - if (rep.structured.length) { - nbdkit_error ("NBD_REPLY_TYPE_NONE with invalid payload"); - return nbdplug_mark_dead (h); - } - if (more) { - nbdkit_error ("NBD_REPLY_TYPE_NONE without done flag"); - return nbdplug_mark_dead (h); -...

...uctured reply payload: %m"); - return nbdplug_mark_dead (h); - } - if (read_full (h->fd, payload, len)) - return nbdplug_mark_dead (h); - len = 0; - } - more = !(rep.structured.flags & NBD_REPLY_FLAG_DONE); - switch (rep.structured.type) { - case NBD_REPLY_TYPE_NONE: - if (rep.structured.length) { - nbdkit_error ("NBD_REPLY_TYPE_NONE with invalid payload"); - return nbdplug_mark_dead (h); - } - if (more) { - nbdkit_error ("NBD_REPLY_TYPE_NONE without done flag"); - return nbdplug_mark_dead (h); -...

...ding structured reply payload: %m"); - return nbd_mark_dead (h); - } - if (read_full (h->fd, payload, len)) - return nbd_mark_dead (h); - len = 0; - } - more = !(rep.structured.flags & NBD_REPLY_FLAG_DONE); - switch (rep.structured.type) { - case NBD_REPLY_TYPE_NONE: - if (rep.structured.length) { - nbdkit_error ("NBD_REPLY_TYPE_NONE with invalid payload"); - return nbd_mark_dead (h); - } - if (more) { - nbdkit_error ("NBD_REPLY_TYPE_NONE without done flag"); - return nbd_mark_dead (h); - } -...

libnbd-0.1.4-1 is now available in Fedora 29/30 updates testing. Diffs since v2 - rebase to master, bump from libnbd 0.1.2 to 0.1.3+, add tests to TLS usage which flushed out the need to turn relative pathnames into absolute, doc tweaks Now that the testsuite covers TLS and libnbd has been fixed to provide the things I found lacking when developing v2, I'm leaning towards pushing this on

...eply error"); - return -1; + return 0; } h->rbuf = &h->sbuf.sr.payload.error.error; h->rlen = sizeof h->sbuf.sr.payload.error.error; @@ -88,12 +88,12 @@ if (length != 0) { SET_NEXT_STATE (%.DEAD); set_error (0, "invalid length in NBD_REPLY_TYPE_NONE"); - return -1; + return 0; } if (!(flags & NBD_REPLY_FLAG_DONE)) { SET_NEXT_STATE (%.DEAD); set_error (0, "NBD_REPLY_FLAG_DONE must be set in NBD_REPLY_TYPE_NONE"); - return -1; + return 0; } SET_NEXT_STATE (%FINISH); r...

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