search for: finish_command

...r/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; uint64_t handle; uint32_t error; uint64_t offset; @@ -234,7 +233,6 @@...

...29a037f3452686719504aea31f311d. --- lib/command.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/command.c b/lib/command.c index 970acf6e5..bc469de59 100644 --- a/lib/command.c +++ b/lib/command.c @@ -791,6 +791,9 @@ guestfs_int_cmd_pipe_run (struct command *cmd, const char *mode) finish_command (cmd); + if (cmd->g->verbose) + debug_command (cmd); + /* Various options cannot be used here. */ assert (!cmd->capture_errors); assert (!cmd->stdout_callback); -- 2.13.2

...reply-simple.c +++ b/generator/states-reply-simple.c @@ -69,7 +69,6 @@ cmd->offset, LIBNBD_READ_DATA, &error) == -1) cmd->error = error ? error : EPROTO; - FREE_CALLBACK (cmd->cb.fn.chunk); } SET_NEXT_STATE (%^FINISH_COMMAND); diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c index 2e327ce..58e83d4 100644 --- a/generator/states-reply-structured.c +++ b/generator/states-reply-structured.c @@ -295,7 +295,6 @@ } if (cmd->type == NBD_CMD_READ && cmd->cb.fn.ch...

...PLY.STRUCTURED_REPLY.RECV_OFFSET_DATA -> REPLY.STRUCTURED_REPLY.RECV_OFFSET_DATA_DATA libnbd: debug: nbd5: nbd_pread: transition: REPLY.STRUCTURED_REPLY.RECV_OFFSET_DATA_DATA -> REPLY.STRUCTURED_REPLY.FINISH libnbd: debug: nbd5: nbd_pread: transition: REPLY.STRUCTURED_REPLY.FINISH -> REPLY.FINISH_COMMAND nbdkit: libnbd: debug: nbd5: nbd_pread: transition: REPLY.FINISH_COMMAND -> READY debug: libnbd: debug: nbd5: nbd_pread: leave: ret=0 starting worker thread full.3 nbdkit: debug: starting worker thread full.12libnbd: debug: nbd5: nbd_pread: enter: buf=<buf> count=512 offset=512 flags=0x0...

...= be64toh (h->sbuf.simple_reply.cookie); > + cookie = be64toh (h->sbuf.reply.hdr.cookie); > /* Find the command amongst the commands in flight. If the server sends > * a reply for an unknown cookie, FINISH will diagnose that later. > */ > @@ -189,7 +192,7 @@ REPLY.FINISH_COMMAND: > * handle (our cookie) is stored at the same offset. See the > * STATIC_ASSERT above in state REPLY.START that confirmed this. > */ > - cookie = be64toh (h->sbuf.simple_reply.cookie); > + cookie = be64toh (h->sbuf.reply.hdr.cookie); > /* Find the command...

...r/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) { + assert (cmd->error == 0); + errno = 0; + if (cmd->cb.fn.read (cmd->cb.opaque, cmd->data, cmd->count, + cmd->offset, LIBNBD_READ_DATA) == -1) +...

...r/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) { + assert (cmd->error == 0); + errno = 0; + if (cmd->cb.fn.read (cmd->cb.opaque, cmd->data, cmd->count, + cmd->offset, 0, LIBNBD_READ_DATA) == -1)...

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

Very much a work in progress as there are still many tests using qemu-io which are candidates for conversion. You'll notice at the end of test-full.sh that the new test has some duplicated code which looks as if it ought to be refactored into a Python function. When I tried to do that, I got loads of strange Python problems which may indicate bugs in nbdsh itself or problems with my

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

...cmd->error = error ? error : EPROTO; - 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 */ + FREE_CALLBACK (cmd->cb.fn.chunk); } SET_NEXT_STATE (%^FINISH_COMMAND); diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c index 7c4d63e..62ae3ad 100644 --- a/generator/states-reply-structured.c +++ b/generator/states-reply-structured.c @@ -307,11 +307,8 @@ &scratch) == -1) if...

...ply-simple.c > @@ -69,7 +69,6 @@ > cmd->offset, LIBNBD_READ_DATA, > &error) == -1) > cmd->error = error ? error : EPROTO; > - FREE_CALLBACK (cmd->cb.fn.chunk); > } > > SET_NEXT_STATE (%^FINISH_COMMAND); > diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c > index 2e327ce..58e83d4 100644 > --- a/generator/states-reply-structured.c > +++ b/generator/states-reply-structured.c > @@ -295,7 +295,6 @@ > } > if (cmd->type == NBD_C...

...assert (cmd); + assert (cmd); /* guaranteed by CHECK */ assert (cmd->extent_fn); assert (h->bs_entries); assert (length >= 12); @@ -460,8 +419,10 @@ flags = be16toh (h->sbuf.sr.structured_reply.flags); if (flags & NBD_REPLY_FLAG_DONE) SET_NEXT_STATE (%^FINISH_COMMAND); - else + else { + h->reply_cmd = NULL; SET_NEXT_STATE (%.READY); + } return 0; } /* END STATE MACHINE */ diff --git a/generator/states-reply.c b/generator/states-reply.c index f0ef47c..54f98c5 100644 --- a/generator/states-reply.c +++ b/generator/states-reply.c @@ -36,6 +36,8 @...

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

...ata, + NULL, 0, NULL, 0, NULL); + if (cmd->type == NBD_CMD_READ && cmd->cb.fn.read) + cmd->cb.fn.read (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data, + NULL, 0, 0, 0, NULL); + cmd->cb.fn.read = NULL; SET_NEXT_STATE (%^FINISH_COMMAND); + } else { h->reply_cmd = NULL; SET_NEXT_STATE (%.READY); -- 2.20.1

Commits 3c27f3d91e1566854747bbe844186783fc84f3a8 and 1b6f0daa9ae7fcc94e389232d0c397816cda973d added an internal API for running commands asynchronously. It is only used by the copy-in and copy-out APIs. Unfortunately this made the command code very complex: it was almost impossible to redirect stderr to a file, and there were a lot of long-range dependencies through the file. It was also buggy:

...== NBD_CMD_READ && cmd->cb.fn.chunk) { cmd->cb.fn.chunk (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data, NULL, 0, 0, 0, NULL); + nbd_internal_free_callback (h, cmd->cb.fn_user_data); + } cmd->cb.fn.chunk = NULL; SET_NEXT_STATE (%^FINISH_COMMAND); } diff --git a/generator/states-reply.c b/generator/states-reply.c index 389317e..d5cba1a 100644 --- a/generator/states-reply.c +++ b/generator/states-reply.c @@ -175,6 +175,7 @@ save_reply_state (struct nbd_handle *h) assert (cmd->type != NBD_CMD_DISC); r = cmd->cb.completion...

..."cmd->count=%" PRIu32 ", " + "this is likely to be a bug in the server", + offset, cmd->offset, cmd->count); + return -1; + } + } + if (flags & NBD_REPLY_FLAG_DONE) SET_NEXT_STATE (%^FINISH_COMMAND); else diff --git a/lib/internal.h b/lib/internal.h index e7be05b..7ad6219 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -143,6 +143,7 @@ struct nbd_handle { uint32_t len; uint16_t nrinfos; uint32_t nrqueries; + uint64_t offset; } sbuf; /* Issuing a command must...

...cmd->error = errno ? errno : EPROTO; + } + else + /* Emit a debug message, but ignore it. */ + debug (h, "server sent unexpected meta context ID %" PRIu32, + context_id); + } if (flags & NBD_REPLY_FLAG_DONE) SET_NEXT_STATE (%^FINISH_COMMAND); diff --git a/interop/dirty-bitmap.c b/interop/dirty-bitmap.c index 8d34173..59be0c8 100644 --- a/interop/dirty-bitmap.c +++ b/interop/dirty-bitmap.c @@ -23,6 +23,8 @@ #include <string.h> #include <unistd.h> #include <assert.h> +#include <stdbool.h> +#include <errno.h...

..., + cmd->offset, LIBNBD_READ_DATA, &error) == -1) cmd->error = error ? error : EPROTO; - cmd->cb.fn.read = NULL; /* because we've freed it */ + cmd->cb.fn.chunk = NULL; /* because we've freed it */ } SET_NEXT_STATE (%^FINISH_COMMAND); diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c index ff5b727..cdd9f10 100644 --- a/generator/states-reply-structured.c +++ b/generator/states-reply-structured.c @@ -304,7 +304,7 @@ valid_flags (struct nbd_handle *h) offset, cmd->offse...