When using the nbd_aio_FOO_callback commands, there is nothing further to be learned about the command by calling nbd_aio_command_completed() compared to what the callback already had access to. There are still scenarios where manually retiring the command after the fact is useful (whether the return was 0 to keep the status unchanged, or -1 to alter the retirement status to *error), but by allowing a return value of 1, we can reduce the number of API calls required. We can also auto-retire the lone NBD_CMD_DISC request, reducing the amount of special casing in nbd_aio_[peek_]command_completed. Note that although this is not an API change, it does change ABI, but that's okay as we have not declared a stable interface yet. Update a couple of the tests and examples to give this coverage: examples/glib-main-loop no longer piles up unretired commands, and tests/server-death shows that even a command stranded by server death can still be auto-retired. --- This probably will conflict with Rich's work to improve callbacks to make it easier to free data on the last use of a callback. examples/glib-main-loop.c | 6 +++-- generator/generator | 49 ++++++++++++++++++++++----------------- generator/states-reply.c | 38 ++++++++++++++++++++---------- generator/states.c | 39 +++++++++++++++++++------------ lib/aio.c | 15 ++++-------- tests/server-death.c | 30 +++++++++++++++++++++++- 6 files changed, 116 insertions(+), 61 deletions(-) diff --git a/examples/glib-main-loop.c b/examples/glib-main-loop.c index c633c1d..2230077 100644 --- a/examples/glib-main-loop.c +++ b/examples/glib-main-loop.c @@ -188,6 +188,8 @@ finalize (GSource *sp) DEBUG (source, "finalize"); + assert (nbd_aio_in_flight (source->nbd) == 0); + assert (nbd_aio_peek_command_completed (source->nbd) == -1); nbd_close (source->nbd); } @@ -418,7 +420,7 @@ finished_read (void *vp, int64_t rcookie, int *error) /* Create a writer idle handler. */ g_idle_add (write_data, NULL); - return 0; + return 1; } /* This idle callback schedules a write. */ @@ -507,5 +509,5 @@ finished_write (void *vp, int64_t wcookie, int *error) g_main_loop_quit (loop); } - return 0; + return 1; } diff --git a/generator/generator b/generator/generator index fdacd71..e0a2805 100755 --- a/generator/generator +++ b/generator/generator @@ -1738,9 +1738,9 @@ unique positive 64 bit cookie for this command, or C<-1> on error. If this command returns a cookie, then C<callback> will be called when the server is done replying, although you must still use C<nbd_aio_command_completed> after -the callback to retire the command. Note that you must ensure -C<buf> is valid until the command has completed. Other -parameters behave as documented in C<nbd_pread>. +the callback to retire the command unless the callback returns C<1>. +Note that you must ensure C<buf> is valid until the command has +completed. Other parameters behave as documented in C<nbd_pread>. The C<callback> function is called with the same C<user_data> passed to this function, C<cookie> set to the return value of this function, @@ -1796,8 +1796,8 @@ unique positive 64 bit cookie for this command, or C<-1> on error. If this command returns a cookie, then C<callback> will be called when the server is done replying, although you must still use C<nbd_aio_command_completed> after -the callback to retire the command. Other parameters behave as -documented in C<nbd_pread_structured>. +the callback to retire the command unless the callback returns C<1>. +Other parameters behave as documented in C<nbd_pread_structured>. The C<callback> function is called with the same C<user_data> passed to this function, C<cookie> set to the return value of this function, @@ -1906,8 +1906,8 @@ unique positive 64 bit cookie for this command, or C<-1> on error. If this command returns a cookie, then C<callback> will be called when the server is done replying, although you must still use C<nbd_aio_command_completed> after -the callback to retire the command. Other parameters behave as -documented in C<nbd_flush>. +the callback to retire the command unless the callback returns C<1>. +Other parameters behave as documented in C<nbd_flush>. The C<callback> function is called with the same C<user_data> passed to this function, C<cookie> set to the return value of this function, @@ -1949,8 +1949,8 @@ unique positive 64 bit cookie for this command, or C<-1> on error. If this command returns a cookie, then C<callback> will be called when the server is done replying, although you must still use C<nbd_aio_command_completed> after -the callback to retire the command. Other parameters behave as -documented in C<nbd_trim>. +the callback to retire the command unless the callback returns C<1>. +Other parameters behave as documented in C<nbd_trim>. The C<callback> function is called with the same C<user_data> passed to this function, C<cookie> set to the return value of this function, @@ -1992,8 +1992,8 @@ returns the unique positive 64 bit cookie for this command, or C<-1> on error. If this command returns a cookie, then C<callback> will be called when the server is done replying, although you must still use C<nbd_aio_command_completed> after -the callback to retire the command. Other parameters behave as -documented in C<nbd_cache>. +the callback to retire the command unless the callback returns C<1>. +Other parameters behave as documented in C<nbd_cache>. The C<callback> function is called with the same C<user_data> passed to this function, C<cookie> set to the return value of this function, @@ -2035,8 +2035,8 @@ unique positive 64 bit cookie for this command, or C<-1> on error. If this command returns a cookie, then C<callback> will be called when the server is done replying, although you must still use C<nbd_aio_command_completed> after -the callback to retire the command. Other parameters behave as -documented in C<nbd_zero>. +the callback to retire the command unless the callback returns C<1>. +Other parameters behave as documented in C<nbd_zero>. The C<callback> function is called with the same C<user_data> passed to this function, C<cookie> set to the return value of this function, @@ -2090,8 +2090,8 @@ unique positive 64 bit cookie for this command, or C<-1> on error. If this command returns a cookie, then C<callback> will be called when the server is done replying, although you must still use C<nbd_aio_command_completed> after -the callback to retire the command. Other parameters behave as -documented in C<nbd_block_status>. +the callback to retire the command unless the callback returns C<1>. +Other parameters behave as documented in C<nbd_block_status>. The C<callback> function is called with the same C<user_data> passed to this function, C<cookie> set to the return value of this function, @@ -2267,7 +2267,10 @@ Return true if the command completed. If this function returns true then the command was successful and it has been retired. Return false if the command is still in flight. This can also fail with an error in case the command failed (in this case -the command is also retired). +the command is also retired). A command is retired either via +this command, or by using a completion callback which returns +C<1> (completion callbacks are registered via +C<nbd_aio_pread_callback> and similar). The C<cookie> parameter is the positive unique 64 bit cookie for the command, as returned by a call such as C<nbd_aio_pread>."; @@ -2279,10 +2282,11 @@ for the command, as returned by a call such as C<nbd_aio_pread>."; shortdesc = "check if any command has completed"; longdesc = "\ Return the unique positive 64 bit cookie of the first non-retired but -completed command, C<0> if no command is awaiting retirement, or C<-1> -on error. Any cookie returned by this function must still be passed to -C<nbd_aio_command_completed> to actually retire the command and learn -whether the command was successful."; +completed command, C<0> if there are in-flight commands but none of +them are awaiting retirement, or C<-1> on error including when there +are no in-flight commands. Any cookie returned by this function must +still be passed to C<nbd_aio_command_completed> to actually retire +the command and learn whether the command was successful."; }; "aio_in_flight", { @@ -3613,7 +3617,10 @@ for how to get further details of the error. void nbd_close (struct nbd_handle *nbd); -Closes the handle frees any associated resources. +Closes the handle and frees any associated resources. The final +status of any command that has not been retired (whether by +C<nbd_aio_command_completed> or by a low-level completion callback +returning C<1>) is lost. =head1 GETTING THE LATEST ERROR MESSAGE IN THE THREAD diff --git a/generator/states-reply.c b/generator/states-reply.c index 1a0c149..6ea43d5 100644 --- a/generator/states-reply.c +++ b/generator/states-reply.c @@ -149,6 +149,7 @@ save_reply_state (struct nbd_handle *h) REPLY.FINISH_COMMAND: struct command *prev_cmd, *cmd; uint64_t cookie; + bool retire; /* NB: This works for both simple and structured replies because the * handle (our cookie) is stored at the same offset. @@ -164,6 +165,23 @@ save_reply_state (struct nbd_handle *h) assert (cmd != NULL); assert (h->reply_cmd == cmd); h->reply_cmd = NULL; + retire = cmd->type == NBD_CMD_DISC; + + /* Notify the user */ + if (cmd->cb.callback) { + int error = cmd->error; + + assert (cmd->type != NBD_CMD_DISC); + switch (cmd->cb.callback (cmd->cb.user_data, cookie, &error)) { + case -1: + if (error) + cmd->error = error; + break; + case 1: + retire = true; + break; + } + } /* Move it to the end of the cmds_done list. */ if (prev_cmd != NULL) @@ -171,23 +189,19 @@ save_reply_state (struct nbd_handle *h) else h->cmds_in_flight = cmd->next; cmd->next = NULL; - if (h->cmds_done_tail != NULL) - h->cmds_done_tail = h->cmds_done_tail->next = cmd; + if (retire) + free (cmd); else { - assert (h->cmds_done == NULL); - h->cmds_done = h->cmds_done_tail = cmd; + if (h->cmds_done_tail != NULL) + h->cmds_done_tail = h->cmds_done_tail->next = cmd; + else { + assert (h->cmds_done == NULL); + h->cmds_done = h->cmds_done_tail = cmd; + } } h->in_flight--; assert (h->in_flight >= 0); - /* Notify the user */ - if (cmd->cb.callback) { - int error = cmd->error; - - if (cmd->cb.callback (cmd->cb.user_data, cookie, &error) == -1 && error) - cmd->error = error; - } - SET_NEXT_STATE (%.READY); return 0; diff --git a/generator/states.c b/generator/states.c index 69aa431..2d7e197 100644 --- a/generator/states.c +++ b/generator/states.c @@ -115,31 +115,40 @@ send_from_wbuf (struct nbd_handle *h) void abort_commands (struct nbd_handle *h, struct command **list) { - struct command *prev_cmd, *cmd; + struct command *next, *cmd; - for (cmd = *list, prev_cmd = NULL; - cmd != NULL; - prev_cmd = cmd, cmd = cmd->next) { + for (cmd = *list, *list = NULL; cmd != NULL; cmd = next) { + bool retire = cmd->type == NBD_CMD_DISC; + + next = cmd->next; if (cmd->cb.callback) { int error = cmd->error ? cmd->error : ENOTCONN; assert (cmd->type != NBD_CMD_DISC); - if (cmd->cb.callback (cmd->cb.user_data, cmd->cookie, - &error) == -1 && error) - cmd->error = error; + switch (cmd->cb.callback (cmd->cb.user_data, cmd->cookie, &error)) { + case -1: + if (error) + cmd->error = error; + break; + case 1: + retire = true; + break; + } } if (cmd->error == 0) cmd->error = ENOTCONN; - } - if (prev_cmd) { - if (h->cmds_done_tail) - h->cmds_done_tail->next = *list; + if (retire) + free (cmd); else { - assert (h->cmds_done == NULL); - h->cmds_done = *list; + cmd->next = NULL; + if (h->cmds_done_tail) + h->cmds_done_tail->next = cmd; + else { + assert (h->cmds_done == NULL); + h->cmds_done = cmd; + } + h->cmds_done_tail = cmd; } - h->cmds_done_tail = prev_cmd; - *list = NULL; } } diff --git a/lib/aio.c b/lib/aio.c index 8d7cb8d..5a9841e 100644 --- a/lib/aio.c +++ b/lib/aio.c @@ -69,11 +69,12 @@ nbd_unlocked_aio_command_completed (struct nbd_handle *h, if (cmd->cookie == cookie) break; } - if (!cmd || cmd->type == NBD_CMD_DISC) + if (!cmd) return 0; type = cmd->type; error = cmd->error; + assert (cmd->type != NBD_CMD_DISC); /* The spec states that a 0-length read request is unspecified; but * it is easy enough to treat it as successful as an extension. */ @@ -105,16 +106,10 @@ nbd_unlocked_aio_command_completed (struct nbd_handle *h, int64_t nbd_unlocked_aio_peek_command_completed (struct nbd_handle *h) { - /* Special case NBD_CMD_DISC, as it does not have a user-visible cookie */ - if (h->cmds_done && h->cmds_done->type == NBD_CMD_DISC) { - struct command *cmd = h->cmds_done; - - h->cmds_done = cmd->next; - free (cmd); - } - - if (h->cmds_done != NULL) + if (h->cmds_done != NULL) { + assert (h->cmds_done->type != NBD_CMD_DISC); return h->cmds_done->cookie; + } if (h->cmds_in_flight != NULL || h->cmds_to_issue != NULL) { set_error (0, "no in-flight command has completed yet"); diff --git a/tests/server-death.c b/tests/server-death.c index 4093c20..08e5358 100644 --- a/tests/server-death.c +++ b/tests/server-death.c @@ -30,6 +30,21 @@ #include <libnbd.h> +static bool trim_retired; +static const char *progname; + +static int +callback (void *ignored, int64_t cookie, int *error) +{ + if (*error != ENOTCONN) { + fprintf (stderr, "%s: unexpected error in trim callback: %s\n", + progname, strerror (*error)); + return 0; + } + trim_retired = 1; + return 1; +} + int main (int argc, char *argv[]) { @@ -47,6 +62,8 @@ main (int argc, char *argv[]) "--exit-with-parent", "--filter=delay", "memory", "size=1m", "delay-reads=5", NULL }; + progname = argv[0]; + /* We're going to kill the child, but don't want to wait for a zombie */ if (signal (SIGCHLD, SIG_IGN) == SIG_ERR) { fprintf (stderr, "%s: signal: %s\n", argv[0], strerror (errno)); @@ -80,11 +97,17 @@ main (int argc, char *argv[]) goto fail; } - /* Issue a read that should not complete yet. */ + /* Issue a read and trim that should not complete yet. Set up the + * trim to auto-retire via callback. + */ if ((cookie = nbd_aio_pread (nbd, buf, sizeof buf, 0, 0)) == -1) { fprintf (stderr, "%s: test failed: nbd_aio_pread\n", argv[0]); goto fail; } + if (nbd_aio_trim_callback (nbd, sizeof buf, 0, callback, NULL, 0) == -1) { + fprintf (stderr, "%s: test failed: nbd_aio_trim_callback\n", argv[0]); + goto fail; + } if (nbd_aio_peek_command_completed (nbd) != 0) { fprintf (stderr, "%s: test failed: nbd_aio_peek_command_completed\n", argv[0]); @@ -153,6 +176,11 @@ main (int argc, char *argv[]) } /* With all commands retired, no further command should be pending */ + if (!trim_retired) { + fprintf (stderr, "%s: test failed: nbd_aio_trim_callback not retired\n", + argv[0]); + goto fail; + } if (nbd_aio_peek_command_completed (nbd) != -1) { fprintf (stderr, "%s: test failed: nbd_aio_peek_command_completed\n", argv[0]); -- 2.20.1