search for: nbd_reply_flag_don

...red.c index 7c4d63e..62ae3ad 100644 --- a/generator/states-reply-structured.c +++ b/generator/states-reply-structured.c @@ -307,11 +307,8 @@ &scratch) == -1) if (cmd->error == 0) cmd->error = scratch; - if (flags & NBD_REPLY_FLAG_DONE) { - if (cmd->cb.fn.chunk.free) - cmd->cb.fn.chunk.free (cmd->cb.fn.chunk.user_data); - cmd->cb.fn.chunk.callback = NULL; /* because we've freed it */ - } + if (flags & NBD_REPLY_FLAG_DONE) + FREE_CALLBACK (cmd->cb.fn.chunk...

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

...nt from successful reads: inform the callback about the * current error rather than any earlier one. If the callback fails @@ -307,8 +306,6 @@ &scratch) == -1) if (cmd->error == 0) cmd->error = scratch; - if (flags & NBD_REPLY_FLAG_DONE) - FREE_CALLBACK (cmd->cb.fn.chunk); } } @@ -390,15 +387,12 @@ assert (cmd); /* guaranteed by CHECK */ if (cmd->cb.fn.chunk.callback) { int error = cmd->error; - uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags); if (CALL_C...

...rm the callback about the > * current error rather than any earlier one. If the callback fails > @@ -307,8 +306,6 @@ > &scratch) == -1) > if (cmd->error == 0) > cmd->error = scratch; > - if (flags & NBD_REPLY_FLAG_DONE) > - FREE_CALLBACK (cmd->cb.fn.chunk); > } > } > > @@ -390,15 +387,12 @@ > assert (cmd); /* guaranteed by CHECK */ > if (cmd->cb.fn.chunk.callback) { > int error = cmd->error; > - uint16_t flags = be16toh (h->sbuf.s...

...CK (cmd->cb.fn.chunk, + cmd->data + (offset - cmd->offset), + 0, offset, LIBNBD_READ_ERROR, + &scratch) == -1) if (cmd->error == 0) cmd->error = scratch; if (flags & NBD_REPLY_FLAG_DONE) @@ -392,10 +392,9 @@ int error = cmd->error; uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags); - if (cmd->cb.fn.chunk.callback (cmd->cb.fn.chunk.user_data, - cmd->data + (offset - cmd->offset), -...

...ad (struct connection *conn, assert (cmd == NBD_CMD_READ); + if (conn->cork) { + r = conn->cork (conn, true); + assert (r == 0); /* For now, only uncorking can fail */ + } + reply.magic = htobe32 (NBD_STRUCTURED_REPLY_MAGIC); reply.handle = handle; reply.flags = htobe16 (NBD_REPLY_FLAG_DONE); @@ -459,6 +477,14 @@ send_structured_reply_read (struct connection *conn, return connection_set_status (conn, -1); } + if (conn->cork) { + r = conn->cork (conn, false); + if (r == -1) { + nbdkit_error ("write uncork: %s: %m", name_of_nbd_cmd (cmd)); + re...

...it only needed for C code? At any rate, the idea makes sense as a lighter-weight way for libnbd to always inform the callback about 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...

...-structured.c +++ b/generator/states-reply-structured.c @@ -540,11 +540,20 @@ valid_flags (struct nbd_handle *h) return 0; REPLY.STRUCTURED_REPLY.FINISH: + struct command *cmd = h->reply_cmd; uint16_t flags; flags = be16toh (h->sbuf.sr.structured_reply.flags); - if (flags & NBD_REPLY_FLAG_DONE) + if (flags & NBD_REPLY_FLAG_DONE) { + if (cmd->type == NBD_CMD_BLOCK_STATUS && cmd->cb.fn.extent) + cmd->cb.fn.extent (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data, + NULL, 0, NULL, 0, NULL); + if (cmd->type == NBD_CMD_READ &&...

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

Patches 1 & 2 are rather complex, but the end result is that we pass closures + user_data + free function in single struct parameters as I described previously in this email: https://www.redhat.com/archives/libguestfs/2019-August/msg00210.html Patch 3 adds a convenient FREE_CALLBACK macro which seems a worthwhile simplification if you buy into 1 & 2. Patch 4 adds another macro which is

When the server sets NBD_REPLY_FLAG_DONE on a data or block status chunk, we can use that fact to pass (VALID|FREE) and avoid a separate callback later just for FREE. --- As I've been promising in other threads... generator/states-reply-structured.c | 33 +++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(...

...-reply-structured.c @@ -18,17 +18,6 @@ /* State machine for parsing structured replies from the server. */ -static unsigned -valid_flags (struct nbd_handle *h) -{ - unsigned valid = LIBNBD_CALLBACK_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 */ { @@ -306,18 +295,18 @@ valid_flags (struct nbd_handle *h) } if (cmd->type == NBD_CMD_READ && cmd->cb.fn.chunk) { int scratch = error; - u...

...*/ > - if (cmd->cb.fn.read (cmd->cb.fn_user_data, > + if (cmd->cb.fn.read (LIBNBD_CALLBACK_VALID, cmd->cb.fn_user_data, > cmd->data + (offset - cmd->offset), [2] Here, we know if we will NOT call the read callback again if NBD_REPLY_FLAG_DONE is set (so if it is set, we could pass VALID|FREE here; but if it is not set, we still have to take care of passing FREE later). > @@ -499,7 +499,7 @@ > /* Call the caller's extent function. */ > int error = cmd->error; > > - if (cmd->cb.fn.extent (cm...

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

...-reply-structured.c @@ -18,17 +18,6 @@ /* State machine for parsing structured replies from the server. */ -static unsigned -valid_flags (struct nbd_handle *h) -{ - unsigned valid = LIBNBD_CALLBACK_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 */ { @@ -306,20 +295,23 @@ valid_flags (struct nbd_handle *h) } if (cmd->type == NBD_CMD_READ && cmd->cb.fn.chunk.callback) { int scratch = error; -...

...rs via a structured reply, as the error can then be accompanied by a string payload to present to a human user. A structured reply MAY occupy multiple structured chunk messages -(all with the same value for "handle"), and the +(all with the same value for "cookie"), and the `NBD_REPLY_FLAG_DONE` reply flag is used to identify the final chunk. Unless further documented by individual requests below, the chunks MAY be sent in any order, except that the chunk with @@ -418,7 +418,7 @@ A structured reply chunk message looks as follows: S: 32 bits, 0x668e33ef, magic (`NBD_STRUCTURED_REPLY_M...

...nbd/server.c | @@ -1838,12 +1838,30 @@ static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client, | | trace_nbd_co_send_structured_read_hole(handle, offset + progress, | pnum); | - set_be_chunk(&chunk.h, final ? NBD_REPLY_FLAG_DONE : 0, | + set_be_chunk(&chunk.h, 0, | NBD_REPLY_TYPE_OFFSET_HOLE, | handle, sizeof(chunk) - sizeof(chunk.h)); | stq_be_p(&chunk.offset, offset + progress); | stl_be_p(&chunk.length, pnum); |...

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

Slightly RFC, as I need more time to investigate why Unix sockets appeared to degrade with this patch. But as TCP sockets (over loopback to localhost) and TLS sessions (regardless of underlying Unix or TCP) both showed improvements, this looks like a worthwhile series. Eric Blake (2): server: Add support for corking server: Cork around grouped transmission send()s server/internal.h | 3

...h" +EOF -# Match the precise 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...