On 6/29/19 8:28 AM, Eric Blake wrote: We guard against underflow... nbd_internal_run (h, cmd_issue) == -1) ...but even though we inserted in the list, we fail to increment if nbd_internal_run() encountered an error. Also, the count is too low if the server managed to get a reply to us without us blocking. Obvious fix pushed. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org

Most of the states were calling SET_NEXT_STATE(%.DEAD) then using return -1 on error, to reflect the fact that they had also called set_error() and wanted the caller to notice the failure. Unfortunately, the state machine engine refuses to run the entry code of the next state when the current state returned -1, which meant the DEAD state entry code never runs. A concrete example of the problems this creates: qemu-nbd defaults to allowing only one connection at a time (newer nbdkit with the noparallel filter can do the same, althoug it's not as picky on export names). The DEAD state claims to close() the fd as soon as we detect a problem, where an example problem is requesting an export name not present on the server (NBD_OPT_GO fails, so we move to the DEAD state since we can't retry nbd_set_export_name without a new handle). But since the libnbd code gave up by returning -1, the DEAD state cleanup never runs, and we end up leaving the connection fd open instead, blocking out a second connection until we use nbd_close() to free all resources. Worse, we haven't yet wired up nbd_close into the python bindings; until that happens, there is no way in nbdsh to wipe out an existing failed connection and replace it with another attempt where we set the correct export name, short of completely exiting nbdsh: $ qemu-nbd -k /tmp/a -f raw -x a file $ ./run nbdsh nbd> h.connect_unix('/tmp/a') Traceback (most recent call last): File "/usr/lib64/python3.7/code.py", line 90, in runcode exec(code, self.locals) File "<console>", line 1, in <module> File "/home/eblake/libnbd/python/nbd.py", line 340, in connect_unix return libnbdmod.connect_unix (self._o, unixsocket) nbd.Error: nbd_connect_unix: handshake: server has no export named '': No such file or directory (ENOENT) nbd> h = nbd.NBD() nbd> h.set_export_name('a') nbd> h.connect_unix('/tmp/a') ... HANGS ... This patch does not address the missing Python binding, but does fix the cleanup problem by ensuring that any transition to DEAD closes the client end of the fd right away, rather than waiting until the final nbd_close(), while still returning an error to the caller. This is done by mandating that other states never move to DEAD without first setting an error but returning 0, then letting DEAD's code return -1. --- generator/generator | 30 +++++++--- generator/states-connect.c | 24 ++++---- generator/states-issue-command.c | 4 +- generator/states-magic.c | 6 +- generator/states-newstyle-opt-export-name.c | 8 +-- generator/states-newstyle-opt-go.c | 20 +++---- .../states-newstyle-opt-set-meta-context.c | 24 ++++---- generator/states-newstyle-opt-starttls.c | 18 +++--- .../states-newstyle-opt-structured-reply.c | 10 ++-- generator/states-newstyle.c | 6 +- generator/states-oldstyle.c | 4 +- generator/states-reply-simple.c | 2 +- generator/states-reply-structured.c | 58 +++++++++---------- generator/states-reply.c | 8 +-- generator/states.c | 4 +- 15 files changed, 121 insertions(+), 105 deletions(-) diff --git a/generator/generator b/generator/generator index 5cc3e80..45a030f 100755 --- a/generator/generator +++ b/generator/generator @@ -47,15 +47,26 @@ if not (Sys.file_exists "lib/handle.c") then * Each handle starts in the top level START state. * * When you enter a state, the associated C code for that state - * runs. If the C code calls SET_NEXT_STATE then the connection - * enters the next state without blocking. If the C code does _not_ - * call SET_NEXT_STATE before returning then the state machine - * blocks and will not be re-entered until an external event - * happens (see below). + * runs. If the C code calls SET_NEXT_STATE and returns 0 then + * the connection enters the next state without blocking. If the + * C code calls SET_NEXT_STATE_AND_BLOCK and returns 0 then the + * connection blocks, but will resume with the code for the next + * state on the next external event. If the C code does _not_ + * call either macro but returns 0, the state machine is blocked + * and will not be re-entered until an external event happens + * (see below), where the same C code will be executed again on + * re-entry. If the C code calls returns -1 after using + * set_error(), then the state machine blocks and the caller + * should report failure; the next external event will resume the + * state machine according to whether SET_NEXT_STATE was used. * * There are various end states such as CLOSED and DEAD. These - * are not special, it's just that they have no way to move to - * another state. + * are not special in relation to the above state transition rules, + * it's just that they have no way to move to another state. However, + * the DEAD state expects that set_error() was used in the previous + * state, and will return -1 itself after performing cleanup actions; + * the earlier state that wants to transition to DEAD should return 0 + * rather than -1, so as not to bypass this cleanup. * * An external event is something like the file descriptor being * ready to read or write, or the main program calling a function @@ -2708,7 +2719,10 @@ let generate_lib_states_c () = pr " abort (); /* Should never happen, but keeps GCC happy. */\n"; pr " }\n"; pr "\n"; - pr " if (r == -1) return -1;\n"; + pr " if (r == -1) {\n"; + pr " assert (nbd_get_error () != NULL);\n"; + pr " return -1;\n"; + pr " }\n"; pr " } while (!blocked);\n"; pr " return 0;\n"; pr "}\n"; diff --git a/generator/states-connect.c b/generator/states-connect.c index c6a4ae7..9e2e1d4 100644 --- a/generator/states-connect.c +++ b/generator/states-connect.c @@ -55,12 +55,12 @@ disable_nagle (int sock) if (fd == -1) { SET_NEXT_STATE (%.DEAD); set_error (errno, "socket"); - return -1; + return 0; } h->sock = nbd_internal_socket_create (fd); if (!h->sock) { SET_NEXT_STATE (%.DEAD); - return -1; + return 0; } disable_nagle (fd); @@ -70,7 +70,7 @@ disable_nagle (int sock) if (errno != EINPROGRESS) { SET_NEXT_STATE (%.DEAD); set_error (errno, "connect"); - return -1; + return 0; } } return 0; @@ -83,7 +83,7 @@ disable_nagle (int sock) SOL_SOCKET, SO_ERROR, &status, &len) == -1) { SET_NEXT_STATE (%.DEAD); set_error (errno, "getsockopt: SO_ERROR"); - return -1; + return 0; } /* This checks the status of the original connect call. */ if (status == 0) { @@ -93,7 +93,7 @@ disable_nagle (int sock) else { SET_NEXT_STATE (%.DEAD); set_error (status, "connect"); - return -1; + return 0; } CONNECT_UNIX.START: @@ -107,7 +107,7 @@ disable_nagle (int sock) if (namelen > sizeof sun.sun_path) { set_error (ENAMETOOLONG, "socket name too long: %s", h->unixsocket); SET_NEXT_STATE (%.DEAD); - return -1; + return 0; } memcpy (sun.sun_path, h->unixsocket, namelen); len = sizeof sun; @@ -175,7 +175,7 @@ disable_nagle (int sock) h->sock = nbd_internal_socket_create (fd); if (!h->sock) { SET_NEXT_STATE (%.DEAD); - return -1; + return 0; } disable_nagle (fd); @@ -198,7 +198,7 @@ disable_nagle (int sock) SOL_SOCKET, SO_ERROR, &status, &len) == -1) { SET_NEXT_STATE (%.DEAD); set_error (errno, "getsockopt: SO_ERROR"); - return -1; + return 0; } /* This checks the status of the original connect call. */ if (status == 0) @@ -228,7 +228,7 @@ disable_nagle (int sock) if (socketpair (AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0, sv) == -1) { SET_NEXT_STATE (%.DEAD); set_error (errno, "socketpair"); - return -1; + return 0; } pid = fork (); @@ -237,7 +237,7 @@ disable_nagle (int sock) set_error (errno, "fork"); close (sv[0]); close (sv[1]); - return -1; + return 0; } if (pid == 0) { /* child - run command */ close (0); @@ -268,14 +268,14 @@ disable_nagle (int sock) fcntl (sv[0], F_SETFL, flags|O_NONBLOCK) == -1) { SET_NEXT_STATE (%.DEAD); close (sv[0]); - return -1; + return 0; } h->sock = nbd_internal_socket_create (sv[0]); if (!h->sock) { SET_NEXT_STATE (%.DEAD); close (sv[0]); - return -1; + return 0; } /* The sockets are connected already, we can jump directly to diff --git a/generator/states-issue-command.c b/generator/states-issue-command.c index 35f3c79..8890e1c 100644 --- a/generator/states-issue-command.c +++ b/generator/states-issue-command.c @@ -54,7 +54,7 @@ ISSUE_COMMAND.SEND_REQUEST: switch (send_from_wbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: SET_NEXT_STATE (%PREPARE_WRITE_PAYLOAD); } return 0; @@ -85,7 +85,7 @@ ISSUE_COMMAND.SEND_WRITE_PAYLOAD: switch (send_from_wbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: SET_NEXT_STATE (%FINISH); } return 0; diff --git a/generator/states-magic.c b/generator/states-magic.c index 93c92fc..de8d235 100644 --- a/generator/states-magic.c +++ b/generator/states-magic.c @@ -27,7 +27,7 @@ MAGIC.RECV_MAGIC: switch (recv_into_rbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: SET_NEXT_STATE (%CHECK_MAGIC); } return 0; @@ -38,7 +38,7 @@ if (strncmp (h->sbuf.new_handshake.nbdmagic, "NBDMAGIC", 8) != 0) { SET_NEXT_STATE (%.DEAD); set_error (0, "handshake: server did not send expected NBD magic"); - return -1; + return 0; } version = be64toh (h->sbuf.new_handshake.version); @@ -49,7 +49,7 @@ else { SET_NEXT_STATE (%.DEAD); set_error (0, "handshake: server is not either an oldstyle or fixed newstyle NBD server"); - return -1; + return 0; } return 0; diff --git a/generator/states-newstyle-opt-export-name.c b/generator/states-newstyle-opt-export-name.c index 968cea8..ec73136 100644 --- a/generator/states-newstyle-opt-export-name.c +++ b/generator/states-newstyle-opt-export-name.c @@ -31,7 +31,7 @@ NEWSTYLE.OPT_EXPORT_NAME.SEND: switch (send_from_wbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: h->wbuf = h->export_name; h->wlen = strlen (h->export_name); @@ -41,7 +41,7 @@ NEWSTYLE.OPT_EXPORT_NAME.SEND_EXPORT: switch (send_from_wbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: h->rbuf = &h->sbuf; h->rlen = sizeof h->sbuf.export_name_reply; @@ -53,7 +53,7 @@ NEWSTYLE.OPT_EXPORT_NAME.RECV_REPLY: switch (recv_into_rbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: SET_NEXT_STATE (%CHECK_REPLY); } return 0; @@ -66,7 +66,7 @@ eflags = be16toh (h->sbuf.export_name_reply.eflags); if (nbd_internal_set_size_and_flags (h, exportsize, eflags) == -1) { SET_NEXT_STATE (%.DEAD); - return -1; + return 0; } SET_NEXT_STATE (%.READY); return 0; diff --git a/generator/states-newstyle-opt-go.c b/generator/states-newstyle-opt-go.c index e245c75..49875a5 100644 --- a/generator/states-newstyle-opt-go.c +++ b/generator/states-newstyle-opt-go.c @@ -34,7 +34,7 @@ const uint32_t exportnamelen = strlen (h->export_name); switch (send_from_wbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: h->sbuf.len = htobe32 (exportnamelen); h->wbuf = &h->sbuf; @@ -46,7 +46,7 @@ NEWSTYLE.OPT_GO.SEND_EXPORTNAMELEN: switch (send_from_wbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: h->wbuf = h->export_name; h->wlen = strlen (h->export_name); @@ -57,7 +57,7 @@ NEWSTYLE.OPT_GO.SEND_EXPORT: switch (send_from_wbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: h->sbuf.nrinfos = 0; h->wbuf = &h->sbuf; @@ -68,7 +68,7 @@ NEWSTYLE.OPT_GO.SEND_NRINFOS: switch (send_from_wbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: h->rbuf = &h->sbuf; h->rlen = sizeof h->sbuf.or.option_reply; @@ -78,11 +78,11 @@ NEWSTYLE.OPT_GO.RECV_REPLY: switch (recv_into_rbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: if (prepare_for_reply_payload (h, NBD_OPT_GO) == -1) { SET_NEXT_STATE (%.DEAD); - return -1; + return 0; } SET_NEXT_STATE (%RECV_REPLY_PAYLOAD); } @@ -90,7 +90,7 @@ NEWSTYLE.OPT_GO.RECV_REPLY_PAYLOAD: switch (recv_into_rbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: SET_NEXT_STATE (%CHECK_REPLY); } return 0; @@ -121,13 +121,13 @@ if (len != sizeof h->sbuf.or.payload.export) { SET_NEXT_STATE (%.DEAD); set_error (0, "handshake: incorrect NBD_INFO_EXPORT option reply length"); - return -1; + return 0; } exportsize = be64toh (h->sbuf.or.payload.export.exportsize); eflags = be16toh (h->sbuf.or.payload.export.eflags); if (nbd_internal_set_size_and_flags (h, exportsize, eflags) == -1) { SET_NEXT_STATE (%.DEAD); - return -1; + return 0; } break; default: @@ -177,7 +177,7 @@ } } SET_NEXT_STATE (%.DEAD); - return -1; + return 0; } } /* END STATE MACHINE */ diff --git a/generator/states-newstyle-opt-set-meta-context.c b/generator/states-newstyle-opt-set-meta-context.c index a00a411..7904fe7 100644 --- a/generator/states-newstyle-opt-set-meta-context.c +++ b/generator/states-newstyle-opt-set-meta-context.c @@ -53,7 +53,7 @@ NEWSTYLE.OPT_SET_META_CONTEXT.SEND: switch (send_from_wbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: h->sbuf.len = htobe32 (strlen (h->export_name)); h->wbuf = &h->sbuf.len; @@ -65,7 +65,7 @@ NEWSTYLE.OPT_SET_META_CONTEXT.SEND_EXPORTNAMELEN: switch (send_from_wbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: h->wbuf = h->export_name; h->wlen = strlen (h->export_name); @@ -76,7 +76,7 @@ NEWSTYLE.OPT_SET_META_CONTEXT.SEND_EXPORTNAME: switch (send_from_wbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: h->sbuf.nrqueries = htobe32 (nbd_internal_string_list_length (h->request_meta_contexts)); @@ -89,7 +89,7 @@ NEWSTYLE.OPT_SET_META_CONTEXT.SEND_NRQUERIES: switch (send_from_wbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: h->querynum = 0; SET_NEXT_STATE (%PREPARE_NEXT_QUERY); @@ -115,7 +115,7 @@ const char *query = h->request_meta_contexts[h->querynum]; switch (send_from_wbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: h->wbuf = query; h->wlen = strlen (query); @@ -125,7 +125,7 @@ NEWSTYLE.OPT_SET_META_CONTEXT.SEND_QUERY: switch (send_from_wbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: h->querynum++; SET_NEXT_STATE (%PREPARE_NEXT_QUERY); @@ -140,11 +140,11 @@ NEWSTYLE.OPT_SET_META_CONTEXT.RECV_REPLY: switch (recv_into_rbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: if (prepare_for_reply_payload (h, NBD_OPT_SET_META_CONTEXT) == -1) { SET_NEXT_STATE (%.DEAD); - return -1; + return 0; } SET_NEXT_STATE (%RECV_REPLY_PAYLOAD); } @@ -152,7 +152,7 @@ NEWSTYLE.OPT_SET_META_CONTEXT.RECV_REPLY_PAYLOAD: switch (recv_into_rbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: SET_NEXT_STATE (%CHECK_REPLY); } return 0; @@ -178,7 +178,7 @@ if (meta_context == NULL) { set_error (errno, "malloc"); SET_NEXT_STATE (%.DEAD); - return -1; + return 0; } meta_context->context_id = be32toh (h->sbuf.or.payload.context.context.context_id); @@ -189,7 +189,7 @@ set_error (errno, "strdup"); SET_NEXT_STATE (%.DEAD); free (meta_context); - return -1; + return 0; } debug (h, "negotiated %s with context ID %" PRIu32, meta_context->name, meta_context->context_id); @@ -202,7 +202,7 @@ /* Anything else is an error, ignore it */ if (handle_reply_error (h) == -1) { SET_NEXT_STATE (%.DEAD); - return -1; + return 0; } debug (h, "handshake: unexpected error from " diff --git a/generator/states-newstyle-opt-starttls.c b/generator/states-newstyle-opt-starttls.c index 61f254f..0a18db0 100644 --- a/generator/states-newstyle-opt-starttls.c +++ b/generator/states-newstyle-opt-starttls.c @@ -36,7 +36,7 @@ NEWSTYLE.OPT_STARTTLS.SEND: switch (send_from_wbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: h->rbuf = &h->sbuf; h->rlen = sizeof (h->sbuf.or.option_reply); @@ -46,11 +46,11 @@ NEWSTYLE.OPT_STARTTLS.RECV_REPLY: switch (recv_into_rbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: if (prepare_for_reply_payload (h, NBD_OPT_STARTTLS) == -1) { SET_NEXT_STATE (%.DEAD); - return -1; + return 0; } SET_NEXT_STATE (%RECV_REPLY_PAYLOAD); } @@ -58,7 +58,7 @@ NEWSTYLE.OPT_STARTTLS.RECV_REPLY_PAYLOAD: switch (recv_into_rbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: SET_NEXT_STATE (%CHECK_REPLY); } return 0; @@ -73,7 +73,7 @@ new_sock = nbd_internal_crypto_create_session (h, h->sock); if (new_sock == NULL) { SET_NEXT_STATE (%.DEAD); - return -1; + return 0; } h->sock = new_sock; if (nbd_internal_crypto_is_reading (h)) @@ -85,7 +85,7 @@ default: if (handle_reply_error (h) == -1) { SET_NEXT_STATE (%.DEAD); - return -1; + return 0; } /* Server refused to upgrade to TLS. If h->tls is not require (2) @@ -95,7 +95,7 @@ SET_NEXT_STATE (%.DEAD); set_error (ENOTSUP, "handshake: server refused TLS, " "but handle TLS setting is require (2)"); - return -1; + return 0; } debug (h, @@ -112,7 +112,7 @@ r = nbd_internal_crypto_handshake (h); if (r == -1) { SET_NEXT_STATE (%.DEAD); - return -1; + return 0; } if (r == 0) { /* Finished handshake. */ @@ -135,7 +135,7 @@ r = nbd_internal_crypto_handshake (h); if (r == -1) { SET_NEXT_STATE (%.DEAD); - return -1; + return 0; } if (r == 0) { /* Finished handshake. */ diff --git a/generator/states-newstyle-opt-structured-reply.c b/generator/states-newstyle-opt-structured-reply.c index 65d5958..d932248 100644 --- a/generator/states-newstyle-opt-structured-reply.c +++ b/generator/states-newstyle-opt-structured-reply.c @@ -30,7 +30,7 @@ NEWSTYLE.OPT_STRUCTURED_REPLY.SEND: switch (send_from_wbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: h->rbuf = &h->sbuf; h->rlen = sizeof h->sbuf.or.option_reply; @@ -40,11 +40,11 @@ NEWSTYLE.OPT_STRUCTURED_REPLY.RECV_REPLY: switch (recv_into_rbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: if (prepare_for_reply_payload (h, NBD_OPT_STRUCTURED_REPLY) == -1) { SET_NEXT_STATE (%.DEAD); - return -1; + return 0; } SET_NEXT_STATE (%RECV_REPLY_PAYLOAD); } @@ -52,7 +52,7 @@ NEWSTYLE.OPT_STRUCTURED_REPLY.RECV_REPLY_PAYLOAD: switch (recv_into_rbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: SET_NEXT_STATE (%CHECK_REPLY); } return 0; @@ -69,7 +69,7 @@ default: if (handle_reply_error (h) == -1) { SET_NEXT_STATE (%.DEAD); - return -1; + return 0; } debug (h, "structured replies are not supported by this server"); diff --git a/generator/states-newstyle.c b/generator/states-newstyle.c index 912ecb5..c8f817e 100644 --- a/generator/states-newstyle.c +++ b/generator/states-newstyle.c @@ -119,7 +119,7 @@ handle_reply_error (struct nbd_handle *h) NEWSTYLE.RECV_GFLAGS: switch (recv_into_rbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: SET_NEXT_STATE (%CHECK_GFLAGS); } return 0; @@ -133,7 +133,7 @@ handle_reply_error (struct nbd_handle *h) SET_NEXT_STATE (%.DEAD); set_error (ENOTSUP, "handshake: server is not fixed newstyle, " "but handle TLS setting is require (2)"); - return -1; + return 0; } cflags = h->gflags & (NBD_FLAG_FIXED_NEWSTYLE|NBD_FLAG_NO_ZEROES); @@ -145,7 +145,7 @@ handle_reply_error (struct nbd_handle *h) NEWSTYLE.SEND_CFLAGS: switch (send_from_wbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: /* Start sending options. */ if ((h->gflags & NBD_FLAG_FIXED_NEWSTYLE) == 0) diff --git a/generator/states-oldstyle.c b/generator/states-oldstyle.c index b5618af..668931b 100644 --- a/generator/states-oldstyle.c +++ b/generator/states-oldstyle.c @@ -32,7 +32,7 @@ OLDSTYLE.RECV_REMAINING: switch (recv_into_rbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: SET_NEXT_STATE (%CHECK); } return 0; @@ -51,7 +51,7 @@ if (nbd_internal_set_size_and_flags (h, exportsize, eflags) == -1) { SET_NEXT_STATE (%.DEAD); - return -1; + return 0; } SET_NEXT_STATE (%.READY); diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c index cab72d6..23b6b5f 100644 --- a/generator/states-reply-simple.c +++ b/generator/states-reply-simple.c @@ -52,7 +52,7 @@ struct command_in_flight *cmd = h->reply_cmd; switch (recv_into_rbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 1: save_reply_state (h); SET_NEXT_STATE (%.READY); diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c index 91c6215..9a8677d 100644 --- a/generator/states-reply-structured.c +++ b/generator/states-reply-structured.c @@ -37,7 +37,7 @@ REPLY.STRUCTURED_REPLY.RECV_REMAINING: switch (recv_into_rbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 1: save_reply_state (h); SET_NEXT_STATE (%.READY); @@ -70,14 +70,14 @@ if (length > MAX_REQUEST_SIZE + sizeof h->sbuf.sr.payload.offset_data) { set_error (0, "invalid server reply length"); SET_NEXT_STATE (%.DEAD); - return -1; + return 0; } if (NBD_REPLY_TYPE_IS_ERR (type)) { if (length < sizeof h->sbuf.sr.payload.error.error) { SET_NEXT_STATE (%.DEAD); set_error (0, "too short length in structured reply 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); return 0; @@ -109,12 +109,12 @@ "cmd->type=%" PRIu16 ", " "this is likely to be a bug in the server", cmd->type); - return -1; + return 0; } if (length < sizeof h->sbuf.sr.payload.offset_data) { SET_NEXT_STATE (%.DEAD); set_error (0, "too short length in NBD_REPLY_TYPE_OFFSET_DATA"); - return -1; + return 0; } h->rbuf = &h->sbuf.sr.payload.offset_data; h->rlen = sizeof h->sbuf.sr.payload.offset_data; @@ -128,12 +128,12 @@ "cmd->type=%" PRIu16 ", " "this is likely to be a bug in the server", cmd->type); - return -1; + return 0; } if (length != sizeof h->sbuf.sr.payload.offset_hole) { SET_NEXT_STATE (%.DEAD); set_error (0, "invalid length in NBD_REPLY_TYPE_OFFSET_HOLE"); - return -1; + return 0; } h->rbuf = &h->sbuf.sr.payload.offset_hole; h->rlen = sizeof h->sbuf.sr.payload.offset_hole; @@ -147,18 +147,18 @@ "cmd->type=%" PRIu16 ", " "this is likely to be a bug in the server", cmd->type); - return -1; + return 0; } /* XXX We should be able to skip the bad reply in these two cases. */ if (length < 12 || ((length-4) & 7) != 0) { SET_NEXT_STATE (%.DEAD); set_error (0, "invalid length in NBD_REPLY_TYPE_BLOCK_STATUS"); - return -1; + return 0; } if (cmd->cb.fn.extent == NULL) { SET_NEXT_STATE (%.DEAD); set_error (0, "not expecting NBD_REPLY_TYPE_BLOCK_STATUS here"); - return -1; + return 0; } /* We read the context ID followed by all the entries into a * single array and deal with it at the end. @@ -168,7 +168,7 @@ if (h->bs_entries == NULL) { SET_NEXT_STATE (%.DEAD); set_error (errno, "malloc"); - return -1; + return 0; } h->rbuf = h->bs_entries; h->rlen = length; @@ -178,14 +178,14 @@ else { SET_NEXT_STATE (%.DEAD); set_error (0, "unknown structured reply type (%" PRIu16 ")", type); - return -1; + return 0; } REPLY.STRUCTURED_REPLY.RECV_ERROR: uint32_t length, msglen; switch (recv_into_rbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 1: save_reply_state (h); SET_NEXT_STATE (%.READY); @@ -197,7 +197,7 @@ msglen > sizeof h->sbuf.sr.payload.error.msg) { SET_NEXT_STATE (%.DEAD); set_error (0, "error message length too large"); - return -1; + return 0; } h->rbuf = h->sbuf.sr.payload.error.msg; @@ -211,7 +211,7 @@ uint16_t type; switch (recv_into_rbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 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 +258,7 @@ uint16_t type; switch (recv_into_rbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 1: save_reply_state (h); SET_NEXT_STATE (%.READY); @@ -289,7 +289,7 @@ "cmd->count=%" PRIu32 ", " "this is likely to be a bug in the server", offset, cmd->offset, cmd->count); - return -1; + return 0; } if (cmd->type == NBD_CMD_READ && cmd->cb.fn.read) { int scratch = error; @@ -319,7 +319,7 @@ uint32_t length; switch (recv_into_rbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 1: save_reply_state (h); SET_NEXT_STATE (%.READY); @@ -343,7 +343,7 @@ "offset=%" PRIu64 ", cmd->offset=%" PRIu64 ", " "this is likely to be a bug in the server", offset, cmd->offset); - return -1; + return 0; } /* Now this is the byte offset in the read buffer. */ offset -= cmd->offset; @@ -355,7 +355,7 @@ "cmd->count=%" PRIu32 ", " "this is likely to be a bug in the server", offset, length, cmd->count); - return -1; + return 0; } /* Set up to receive the data directly to the user buffer. */ @@ -371,7 +371,7 @@ uint32_t length; switch (recv_into_rbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 1: save_reply_state (h); SET_NEXT_STATE (%.READY); @@ -401,7 +401,7 @@ uint32_t length; switch (recv_into_rbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 1: save_reply_state (h); SET_NEXT_STATE (%.READY); @@ -422,7 +422,7 @@ "offset=%" PRIu64 ", cmd->offset=%" PRIu64 ", " "this is likely to be a bug in the server", offset, cmd->offset); - return -1; + return 0; } /* Now this is the byte offset in the read buffer. */ offset -= cmd->offset; @@ -434,7 +434,7 @@ "cmd->count=%" PRIu32 ", " "this is likely to be a bug in the server", offset, length, cmd->count); - return -1; + return 0; } /* The spec states that 0-length requests are unspecified, but @@ -464,7 +464,7 @@ struct meta_context *meta_context; switch (recv_into_rbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 1: save_reply_state (h); SET_NEXT_STATE (%.READY); diff --git a/generator/states-reply.c b/generator/states-reply.c index 6fb0a7a..88274bc 100644 --- a/generator/states-reply.c +++ b/generator/states-reply.c @@ -85,7 +85,7 @@ save_reply_state (struct nbd_handle *h) /* sock->ops->recv called set_error already. */ SET_NEXT_STATE (%.DEAD); - return -1; + return 0; } if (r == 0) { SET_NEXT_STATE (%.CLOSED); @@ -99,7 +99,7 @@ save_reply_state (struct nbd_handle *h) REPLY.RECV_REPLY: switch (recv_into_rbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 1: SET_NEXT_STATE (%.READY); return 0; case 0: SET_NEXT_STATE (%CHECK_SIMPLE_OR_STRUCTURED_REPLY); } @@ -120,7 +120,7 @@ save_reply_state (struct nbd_handle *h) else { SET_NEXT_STATE (%.DEAD); /* We've probably lost synchronization. */ set_error (0, "invalid reply magic"); - return -1; + return 0; } /* NB: This works for both simple and structured replies because the @@ -141,7 +141,7 @@ save_reply_state (struct nbd_handle *h) SET_NEXT_STATE (%.DEAD); set_error (0, "no matching handle found for server reply, " "this is probably a bug in the server"); - return -1; + return 0; } h->reply_cmd = cmd; return 0; diff --git a/generator/states.c b/generator/states.c index b0dab83..deea73c 100644 --- a/generator/states.c +++ b/generator/states.c @@ -125,11 +125,13 @@ send_from_wbuf (struct nbd_handle *h) return 0; DEAD: + /* The caller should have used set_error() before reaching here */ + assert (nbd_get_error ()); if (h->sock) { h->sock->ops->close (h->sock); h->sock = NULL; } - return 0; + return -1; CLOSED: if (h->sock) { -- 2.20.1

On 6/29/19 8:28 AM, Eric Blake wrote: This inserts the list to the head of cmds_done, which breaks its use as a FIFO queue for clients using nbd_aio_peek_command_completed to process messages in server order. I'll post a fix that keeps things in order. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org

As mentioned in the previous patch, there are situations where an aio client wants instant notification when a given command is complete, rather than having to maintain a separate data structure to track all in-flight commands and then iterate over that structure to learn which commands are complete. It's also desirable when writing a server validation program (such as for checking structured reads for compliance) to be able to clean up the associated opaque data and have a final chance to change the overall command status. Introduce new nbd_aio_FOO_notify functions for each command. Rewire the existing nbd_aio_FOO to forward to the new command. (Perhaps the generator could reduce some of the boilerplate duplication, if a later patch wants to refactor this). --- docs/libnbd.pod | 22 +++- generator/generator | 278 +++++++++++++++++++++++++++++++++++++++++--- lib/rw.c | 99 ++++++++++++++-- 3 files changed, 374 insertions(+), 25 deletions(-) diff --git a/docs/libnbd.pod b/docs/libnbd.pod index ede2539..93e80d4 100644 --- a/docs/libnbd.pod +++ b/docs/libnbd.pod @@ -169,7 +169,27 @@ has completed: } For almost all high level synchronous calls (eg. C<nbd_pread>) there -is a low level asynchronous equivalent (eg. C<nbd_aio_pread>). +are two low level asynchronous equivalents (eg. C<nbd_aio_pread> for +starting a command, and C<nbd_aio_pread_notify> for also registering +a callback to be invoked right before the command is complete). + +=head1 CALLBACKS + +Some of the high-level commands (C<nbd_pread_structured>, +C<nbd_block_status>) involve the use of a callback function invoked by +the state machine at appropriate points in the server's reply before +the overall command is complete. Also, all of the low-level commands +have a notify variant that registers a callback function used right +before the command is marked complete. These callback functions +include a parameter C<error> containing the value of any error +detected so far; if the callback function fails, it should assign back +into C<error> and return C<-1> to change the resulting error of the +overall command. + +The callbacks are invoked at a point where the libnbd lock is held; as +such, it is unsafe for the callback to call any C<nbd_*> APIs on the +same nbd object, as it would cause deadlock. Functions that take two +callback pointers share the same opaque data for both calls. =head1 ERROR HANDLING diff --git a/generator/generator b/generator/generator index c5988e2..fe73c15 100755 --- a/generator/generator +++ b/generator/generator @@ -1708,9 +1708,40 @@ on the connection."; Issue a read command to the NBD server. This returns the unique positive 64 bit handle for this command, or C<-1> on error. To check if the command completed, call -C<nbd_aio_command_completed>. Note that you must ensure +C<nbd_aio_command_completed>, or use C<nbd_aio_pread_notify>. +Note that you must ensure C<buf> is valid until the command +has completed. Other parameters behave as documented in +C<nbd_pread>."; + }; + + "aio_pread_notify", { + default_call with + args = [ BytesPersistOut ("buf", "count"); UInt64 "offset"; + Opaque "data"; + CallbackPersist ("notify", [ Opaque "data"; Int64 "handle"; + Mutable (Int "error") ]); + Flags "flags" ]; + ret = RInt64; + permitted_states = [ Connected ]; + shortdesc = "read from the NBD server, and notify on completion"; + longdesc = "\ +Issue a read command to the NBD server. This returns the +unique positive 64 bit handle for this command, or C<-1> on +error. If this command returns a handle, then the C<notify> +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>."; +parameters behave as documented in C<nbd_pread>. + +The C<notify> callback is called with the same C<data> passed to +this function, C<handle> set to the return value of this function, +and C<error> containing the command's result so far. The callback +may modify the overall status of the command by storing into +C<error> and returning C<-1>, although attempts to undo non-zero +status back to zero are ignored. The callback cannot call C<nbd_*> +APIs on the same handle since it holds the handle lock and will +cause a deadlock."; }; "aio_pread_structured", { @@ -1730,8 +1761,43 @@ parameters behave as documented in C<nbd_pread>."; Issue a read command to the NBD server. This returns the unique positive 64 bit handle for this command, or C<-1> on error. To check if the command completed, call -C<nbd_aio_command_completed>. Parameters behave as documented -in C<nbd_pread_structured>."; +C<nbd_aio_command_completed>, or use +C<nbd_aio_pread_structured_notify>. Parameters behave as +documented in C<nbd_pread_structured>."; + }; + + "aio_pread_structured_notify", { + default_call with + args = [ BytesPersistOut ("buf", "count"); UInt64 "offset"; + Opaque "data"; + CallbackPersist ("chunk", [ Opaque "data"; + BytesIn ("subbuf", "count"); + UInt64 "offset"; + Mutable (Int "error"); + Int "status" ]); + CallbackPersist ("notify", [ Opaque "data"; Int64 "handle"; + Mutable (Int "error") ]); + Flags "flags" ]; + ret = RInt64; + permitted_states = [ Connected ]; + shortdesc = "read from the NBD server, and notify on completion"; + longdesc = "\ +Issue a read command to the NBD server. This returns the +unique positive 64 bit handle for this command, or C<-1> on +error. If this command returns a handle, then the C<notify> +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 C<notify> callback is called with the same C<data> passed to +this function, C<handle> set to the return value of this function, +and C<error> containing the command's result so far. The callback +may modify the overall status of the command by storing into +C<error> and returning C<-1>, although attempts to undo non-zero +status back to zero are ignored. The callback cannot call C<nbd_*> +APIs on the same handle since it holds the handle lock and will +cause a deadlock."; }; "aio_pwrite", { @@ -1744,9 +1810,40 @@ in C<nbd_pread_structured>."; Issue a write command to the NBD server. This returns the unique positive 64 bit handle for this command, or C<-1> on error. To check if the command completed, call -C<nbd_aio_command_completed>. Note that you must ensure +C<nbd_aio_command_completed>, or use C<nbd_aio_pwrite_notify>. +Note that you must ensure C<buf> is valid until the command +has completed. Other parameters behave as documented in +C<nbd_pwrite>."; + }; + + "aio_pwrite_notify", { + default_call with + args = [ BytesPersistIn ("buf", "count"); UInt64 "offset"; + Opaque "data"; + CallbackPersist ("notify", [ Opaque "data"; Int64 "handle"; + Mutable (Int "error") ]); + Flags "flags" ]; + ret = RInt64; + permitted_states = [ Connected ]; + shortdesc = "write to the NBD server, and notify on completion"; + longdesc = "\ +Issue a write command to the NBD server. This returns the +unique positive 64 bit handle for this command, or C<-1> on +error. If this command returns a handle, then the C<notify> +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_pwrite>."; +parameters behave as documented in C<nbd_pwrite>. + +The C<notify> callback is called with the same C<data> passed to +this function, C<handle> set to the return value of this function, +and C<error> containing the command's result so far. The callback +may modify the overall status of the command by storing into +C<error> and returning C<-1>, although attempts to undo non-zero +status back to zero are ignored. The callback cannot call C<nbd_*> +APIs on the same handle since it holds the handle lock and will +cause a deadlock."; }; "aio_disconnect", { @@ -1780,8 +1877,36 @@ however, C<nbd_shutdown> will call this function if appropriate."; Issue the flush command to the NBD server. This returns the unique positive 64 bit handle for this command, or C<-1> on error. To check if the command completed, call -C<nbd_aio_command_completed>. Parameters behave as documented -in C<nbd_flush>."; +C<nbd_aio_command_completed>, or use C<nbd_aio_flush_notify>. +Parameters behave as documented in C<nbd_flush>."; + }; + + "aio_flush_notify", { + default_call with + args = [ Opaque "data"; + CallbackPersist ("notify", [ Opaque "data"; Int64 "handle"; + Mutable (Int "error") ]); + Flags "flags" ]; + ret = RInt64; + permitted_states = [ Connected ]; + shortdesc = "send flush command to the NBD server, and notify on completion"; + longdesc = "\ +Issue the flush command to the NBD server. This returns the +unique positive 64 bit handle for this command, or C<-1> on +error. If this command returns a handle, then the C<notify> +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 C<notify> callback is called with the same C<data> passed to +this function, C<handle> set to the return value of this function, +and C<error> containing the command's result so far. The callback +may modify the overall status of the command by storing into +C<error> and returning C<-1>, although attempts to undo non-zero +status back to zero are ignored. The callback cannot call C<nbd_*> +APIs on the same handle since it holds the handle lock and will +cause a deadlock."; }; "aio_trim", { @@ -1794,8 +1919,37 @@ in C<nbd_flush>."; Issue a trim command to the NBD server. This returns the unique positive 64 bit handle for this command, or C<-1> on error. To check if the command completed, call -C<nbd_aio_command_completed>. Parameters behave as documented -in C<nbd_trim>."; +C<nbd_aio_command_completed>, or use C<nbd_aio_trim_notify>. +Parameters behave as documented in C<nbd_trim>."; + }; + + "aio_trim_notify", { + default_call with + args = [ UInt64 "count"; UInt64 "offset"; + Opaque "data"; + CallbackPersist ("notify", [ Opaque "data"; Int64 "handle"; + Mutable (Int "error") ]); + Flags "flags" ]; + ret = RInt64; + permitted_states = [ Connected ]; + shortdesc = "send trim command to the NBD server, and notify on completion"; + longdesc = "\ +Issue a trim command to the NBD server. This returns the +unique positive 64 bit handle for this command, or C<-1> on +error. If this command returns a handle, then the C<notify> +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 C<notify> callback is called with the same C<data> passed to +this function, C<handle> set to the return value of this function, +and C<error> containing the command's result so far. The callback +may modify the overall status of the command by storing into +C<error> and returning C<-1>, although attempts to undo non-zero +status back to zero are ignored. The callback cannot call C<nbd_*> +APIs on the same handle since it holds the handle lock and will +cause a deadlock."; }; "aio_cache", { @@ -1808,8 +1962,37 @@ in C<nbd_trim>."; Issue the cache (prefetch) command to the NBD server. This returns the unique positive 64 bit handle for this command, or C<-1> on error. To check if the command completed, call -C<nbd_aio_command_completed>. Parameters behave as documented -in C<nbd_cache>."; +C<nbd_aio_command_completed>, or use C<nbd_aio_cache_notify>. +Parameters behave as documented in C<nbd_cache>."; + }; + + "aio_cache_notify", { + default_call with + args = [ UInt64 "count"; UInt64 "offset"; + Opaque "data"; + CallbackPersist ("notify", [ Opaque "data"; Int64 "handle"; + Mutable (Int "error") ]); + Flags "flags" ]; + ret = RInt64; + permitted_states = [ Connected ]; + shortdesc = "send cache (prefetch) command to the NBD server, and notify on completion"; + longdesc = "\ +Issue the cache (prefetch) command to the NBD server. This +returns the unique positive 64 bit handle for this command, or +C<-1> on error. If this command returns a handle, then the C<notify> +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 C<notify> callback is called with the same C<data> passed to +this function, C<handle> set to the return value of this function, +and C<error> containing the command's result so far. The callback +may modify the overall status of the command by storing into +C<error> and returning C<-1>, although attempts to undo non-zero +status back to zero are ignored. The callback cannot call C<nbd_*> +APIs on the same handle since it holds the handle lock and will +cause a deadlock."; }; "aio_zero", { @@ -1822,8 +2005,37 @@ in C<nbd_cache>."; Issue a write zeroes command to the NBD server. This returns the unique positive 64 bit handle for this command, or C<-1> on error. To check if the command completed, call -C<nbd_aio_command_completed>. Parameters behave as documented -in C<nbd_zero>."; +C<nbd_aio_command_completed>, or use C<nbd_aio_zero_notify>. +Parameters behave as documented in C<nbd_zero>."; + }; + + "aio_zero_notify", { + default_call with + args = [ UInt64 "count"; UInt64 "offset"; + Opaque "data"; + CallbackPersist ("notify", [ Opaque "data"; Int64 "handle"; + Mutable (Int "error") ]); + Flags "flags" ]; + ret = RInt64; + permitted_states = [ Connected ]; + shortdesc = "send write zeroes command to the NBD server, and notify on completion"; + longdesc = "\ +Issue a write zeroes command to the NBD server. This returns the +unique positive 64 bit handle for this command, or C<-1> on +error. If this command returns a handle, then the C<notify> +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 C<notify> callback is called with the same C<data> passed to +this function, C<handle> set to the return value of this function, +and C<error> containing the command's result so far. The callback +may modify the overall status of the command by storing into +C<error> and returning C<-1>, although attempts to undo non-zero +status back to zero are ignored. The callback cannot call C<nbd_*> +APIs on the same handle since it holds the handle lock and will +cause a deadlock."; }; "aio_block_status", { @@ -1843,8 +2055,42 @@ in C<nbd_zero>."; Send the block status command to the NBD server. This returns the unique positive 64 bit handle for this command, or C<-1> on error. To check if the command completed, call -C<nbd_aio_command_completed>. Parameters behave as documented -in C<nbd_block_status>."; +C<nbd_aio_command_completed>, or use C<nbd_aio_block_status_notify>. +Parameters behave as documented in C<nbd_block_status>."; + }; + + "aio_block_status_notify", { + default_call with + args = [ UInt64 "count"; UInt64 "offset"; + Opaque "data"; + CallbackPersist ("extent", [Opaque "data"; String "metacontext"; + UInt64 "offset"; + ArrayAndLen (UInt32 "entries", + "nr_entries"); + Mutable (Int "error") ]); + CallbackPersist ("notify", [ Opaque "data"; Int64 "handle"; + Mutable (Int "error") ]); + Flags "flags" ]; + ret = RInt64; + permitted_states = [ Connected ]; + shortdesc = "send block status command to the NBD server, and notify on completion"; + longdesc = "\ +Send the block status command to the NBD server. This returns the +unique positive 64 bit handle for this command, or C<-1> on +error. If this command returns a handle, then the C<notify> +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 C<notify> callback is called with the same C<data> passed to +this function, C<handle> set to the return value of this function, +and C<error> containing the command's result so far. The callback +may modify the overall status of the command by storing into +C<error> and returning C<-1>, although attempts to undo non-zero +status back to zero are ignored. The callback cannot call C<nbd_*> +APIs on the same handle since it holds the handle lock and will +cause a deadlock."; }; "aio_get_fd", { diff --git a/lib/rw.c b/lib/rw.c index 53cd521..93388a9 100644 --- a/lib/rw.c +++ b/lib/rw.c @@ -249,6 +249,17 @@ int64_t nbd_unlocked_aio_pread (struct nbd_handle *h, void *buf, size_t count, uint64_t offset, uint32_t flags) { + return nbd_unlocked_aio_pread_notify (h, buf, count, offset, NULL, NULL, + flags); +} + +int64_t +nbd_unlocked_aio_pread_notify (struct nbd_handle *h, void *buf, + size_t count, uint64_t offset, + void *opaque, notify_fn notify, uint32_t flags) +{ + struct command_cb cb = { .opaque = opaque, .notify = notify, }; + /* We could silently accept flag DF, but it really only makes sense * with callbacks, because otherwise there is no observable change * except that the server may fail where it would otherwise succeed. @@ -259,7 +270,7 @@ nbd_unlocked_aio_pread (struct nbd_handle *h, void *buf, } return nbd_internal_command_common (h, 0, NBD_CMD_READ, offset, count, - buf, NULL); + buf, &cb); } int64_t @@ -267,7 +278,18 @@ nbd_unlocked_aio_pread_structured (struct nbd_handle *h, void *buf, size_t count, uint64_t offset, void *opaque, read_fn read, uint32_t flags) { - struct command_cb cb = { .opaque = opaque, .fn.read = read, }; + return nbd_unlocked_aio_pread_structured_notify (h, buf, count, offset, + opaque, read, NULL, flags); +} + +int64_t +nbd_unlocked_aio_pread_structured_notify (struct nbd_handle *h, void *buf, + size_t count, uint64_t offset, + void *opaque, read_fn read, + notify_fn notify, uint32_t flags) +{ + struct command_cb cb = { .opaque = opaque, .fn.read = read, + .notify = notify, }; if ((flags & ~LIBNBD_CMD_FLAG_DF) != 0) { set_error (EINVAL, "invalid flag: %" PRIu32, flags); @@ -289,6 +311,17 @@ nbd_unlocked_aio_pwrite (struct nbd_handle *h, const void *buf, size_t count, uint64_t offset, uint32_t flags) { + return nbd_unlocked_aio_pwrite_notify (h, buf, count, offset, NULL, NULL, + flags); +} + +int64_t +nbd_unlocked_aio_pwrite_notify (struct nbd_handle *h, const void *buf, + size_t count, uint64_t offset, + void *opaque, notify_fn notify, uint32_t flags) +{ + struct command_cb cb = { .opaque = opaque, .notify = notify, }; + if (nbd_unlocked_read_only (h) == 1) { set_error (EINVAL, "server does not support write operations"); return -1; @@ -306,12 +339,21 @@ nbd_unlocked_aio_pwrite (struct nbd_handle *h, const void *buf, } return nbd_internal_command_common (h, flags, NBD_CMD_WRITE, offset, count, - (void *) buf, NULL); + (void *) buf, &cb); } int64_t nbd_unlocked_aio_flush (struct nbd_handle *h, uint32_t flags) { + return nbd_unlocked_aio_flush_notify (h, NULL, NULL, flags); +} + +int64_t +nbd_unlocked_aio_flush_notify (struct nbd_handle *h, void *opaque, + notify_fn notify, uint32_t flags) +{ + struct command_cb cb = { .opaque = opaque, .notify = notify, }; + if (nbd_unlocked_can_flush (h) != 1) { set_error (EINVAL, "server does not support flush operations"); return -1; @@ -323,7 +365,7 @@ nbd_unlocked_aio_flush (struct nbd_handle *h, uint32_t flags) } return nbd_internal_command_common (h, 0, NBD_CMD_FLUSH, 0, 0, - NULL, NULL); + NULL, &cb); } int64_t @@ -331,6 +373,16 @@ nbd_unlocked_aio_trim (struct nbd_handle *h, uint64_t count, uint64_t offset, uint32_t flags) { + return nbd_unlocked_aio_trim_notify (h, count, offset, NULL, NULL, flags); +} + +int64_t +nbd_unlocked_aio_trim_notify (struct nbd_handle *h, + uint64_t count, uint64_t offset, + void *opaque, notify_fn notify, uint32_t flags) +{ + struct command_cb cb = { .opaque = opaque, .notify = notify, }; + if (nbd_unlocked_read_only (h) == 1) { set_error (EINVAL, "server does not support write operations"); return -1; @@ -353,13 +405,23 @@ nbd_unlocked_aio_trim (struct nbd_handle *h, } return nbd_internal_command_common (h, flags, NBD_CMD_TRIM, offset, count, - NULL, NULL); + NULL, &cb); } int64_t nbd_unlocked_aio_cache (struct nbd_handle *h, uint64_t count, uint64_t offset, uint32_t flags) { + return nbd_unlocked_aio_cache_notify (h, count, offset, NULL, NULL, flags); +} + +int64_t +nbd_unlocked_aio_cache_notify (struct nbd_handle *h, + uint64_t count, uint64_t offset, + void *opaque, notify_fn notify, uint32_t flags) +{ + struct command_cb cb = { .opaque = opaque, .notify = notify, }; + /* Actually according to the NBD protocol document, servers do exist * that support NBD_CMD_CACHE but don't advertise the * NBD_FLAG_SEND_CACHE bit, but we ignore those. @@ -375,7 +437,7 @@ nbd_unlocked_aio_cache (struct nbd_handle *h, } return nbd_internal_command_common (h, 0, NBD_CMD_CACHE, offset, count, - NULL, NULL); + NULL, &cb); } int64_t @@ -383,6 +445,16 @@ nbd_unlocked_aio_zero (struct nbd_handle *h, uint64_t count, uint64_t offset, uint32_t flags) { + return nbd_unlocked_aio_zero_notify (h, count, offset, NULL, NULL, flags); +} + +int64_t +nbd_unlocked_aio_zero_notify (struct nbd_handle *h, + uint64_t count, uint64_t offset, + void *opaque, notify_fn notify, uint32_t flags) +{ + struct command_cb cb = { .opaque = opaque, .notify = notify, }; + if (nbd_unlocked_read_only (h) == 1) { set_error (EINVAL, "server does not support write operations"); return -1; @@ -405,7 +477,7 @@ nbd_unlocked_aio_zero (struct nbd_handle *h, } return nbd_internal_command_common (h, flags, NBD_CMD_WRITE_ZEROES, offset, - count, NULL, NULL); + count, NULL, &cb); } int64_t @@ -414,7 +486,18 @@ nbd_unlocked_aio_block_status (struct nbd_handle *h, void *data, extent_fn extent, uint32_t flags) { - struct command_cb cb = { .opaque = data, .fn.extent = extent, }; + return nbd_unlocked_aio_block_status_notify (h, count, offset, data, extent, + NULL, flags); +} + +int64_t +nbd_unlocked_aio_block_status_notify (struct nbd_handle *h, + uint64_t count, uint64_t offset, + void *data, extent_fn extent, + notify_fn notify, uint32_t flags) +{ + struct command_cb cb = { .opaque = data, .fn.extent = extent, + .notify = notify, }; if (!h->structured_replies) { set_error (ENOTSUP, "server does not support structured replies"); -- 2.20.1