Instead of using magic numbers, track connection status via an enum. While at it, convert functions whose return value is not used to instead be void. No semantic change. --- server/internal.h | 16 +++++-- server/connections.c | 29 ++++++------ server/protocol.c | 107 ++++++++++++++++++++++--------------------- server/public.c | 3 +- server/test-public.c | 4 +- 5 files changed, 84 insertions(+), 75 deletions(-) diff --git a/server/internal.h b/server/internal.h index ed52c1bf..fa658364 100644 --- a/server/internal.h +++ b/server/internal.h @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2013-2021 Red Hat Inc. + * Copyright (C) 2013-2022 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -232,13 +232,19 @@ struct context { int can_cache; }; +typedef enum { + STATUS_DEAD, /* Connection is closed */ + STATUS_CLIENT_DONE, /* Client has sent NBD_CMD_DISC */ + STATUS_ACTIVE, /* Client can make requests */ +} conn_status; + struct connection { pthread_mutex_t request_lock; pthread_mutex_t read_lock; pthread_mutex_t write_lock; pthread_mutex_t status_lock; - int status; /* 1 for more I/O with client, 0 for shutdown, -1 on error */ + conn_status status; int status_pipe[2]; /* track status changes via poll when nworkers > 1 */ void *crypto_session; int nworkers; @@ -264,8 +270,8 @@ struct connection { }; extern void handle_single_connection (int sockin, int sockout); -extern int connection_get_status (void); -extern int connection_set_status (int value); +extern conn_status connection_get_status (void); +extern void connection_set_status (conn_status value); /* protocol-handshake.c */ extern int protocol_handshake (void); @@ -280,7 +286,7 @@ extern int protocol_handshake_oldstyle (void); extern int protocol_handshake_newstyle (void); /* protocol.c */ -extern int protocol_recv_request_send_reply (void); +extern void protocol_recv_request_send_reply (void); /* The context ID of base:allocation. As far as I can tell it doesn't * matter what this is as long as nbdkit always returns the same diff --git a/server/connections.c b/server/connections.c index 29e4cd58..27177d70 100644 --- a/server/connections.c +++ b/server/connections.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2013-2021 Red Hat Inc. + * Copyright (C) 2013-2022 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -64,11 +64,11 @@ static int raw_send_other (const void *buf, size_t len, int flags); #endif static void raw_close (void); -int +conn_status connection_get_status (void) { GET_CONN; - int r; + conn_status r; if (conn->nworkers && pthread_mutex_lock (&conn->status_lock)) @@ -80,11 +80,9 @@ connection_get_status (void) return r; } -/* Update the status if the new value is lower than the existing value. - * For convenience, return the incoming value. - */ -int -connection_set_status (int value) +/* Update the status if the new value is lower than the existing value. */ +void +connection_set_status (conn_status value) { GET_CONN; @@ -92,7 +90,7 @@ connection_set_status (int value) pthread_mutex_lock (&conn->status_lock)) abort (); if (value < conn->status) { - if (conn->nworkers && conn->status > 0) { + if (conn->nworkers && conn->status > STATUS_CLIENT_DONE) { char c = 0; assert (conn->status_pipe[1] >= 0); @@ -104,7 +102,6 @@ connection_set_status (int value) if (conn->nworkers && pthread_mutex_unlock (&conn->status_lock)) abort (); - return value; } struct worker_data { @@ -125,7 +122,7 @@ connection_worker (void *data) threadlocal_set_conn (conn); free (worker); - while (!quit && connection_get_status () > 0) + while (!quit && connection_get_status () > STATUS_CLIENT_DONE) protocol_recv_request_send_reply (); debug ("exiting worker thread %s", threadlocal_get_name ()); free (name); @@ -177,7 +174,7 @@ handle_single_connection (int sockin, int sockout) if (!nworkers) { /* No need for a separate thread. */ debug ("handshake complete, processing requests serially"); - while (!quit && connection_get_status () > 0) + while (!quit && connection_get_status () > STATUS_CLIENT_DONE) protocol_recv_request_send_reply (); } else { @@ -196,13 +193,13 @@ handle_single_connection (int sockin, int sockout) if (unlikely (!worker)) { perror ("malloc"); - connection_set_status (-1); + connection_set_status (STATUS_DEAD); goto wait; } if (unlikely (asprintf (&worker->name, "%s.%d", plugin_name, nworkers) < 0)) { perror ("asprintf"); - connection_set_status (-1); + connection_set_status (STATUS_DEAD); free (worker); goto wait; } @@ -212,7 +209,7 @@ handle_single_connection (int sockin, int sockout) if (unlikely (err)) { errno = err; perror ("pthread_create"); - connection_set_status (-1); + connection_set_status (STATUS_DEAD); free (worker); goto wait; } @@ -264,7 +261,7 @@ new_connection (int sockin, int sockout, int nworkers) goto error1; } - conn->status = 1; + conn->status = STATUS_ACTIVE; conn->nworkers = nworkers; if (nworkers) { #ifdef HAVE_PIPE2 diff --git a/server/protocol.c b/server/protocol.c index 2ac77055..d1e01502 100644 --- a/server/protocol.c +++ b/server/protocol.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2013-2021 Red Hat Inc. + * Copyright (C) 2013-2022 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -362,7 +362,7 @@ nbd_errno (int error, uint16_t flags) } } -static int +static void send_simple_reply (uint64_t handle, uint16_t cmd, uint16_t flags, const char *buf, uint32_t count, uint32_t error) @@ -380,7 +380,8 @@ send_simple_reply (uint64_t handle, uint16_t cmd, uint16_t flags, r = conn->send (&reply, sizeof reply, f); if (r == -1) { nbdkit_error ("write reply: %s: %m", name_of_nbd_cmd (cmd)); - return connection_set_status (-1); + connection_set_status (STATUS_DEAD); + return; } /* Send the read data buffer. */ @@ -388,14 +389,12 @@ send_simple_reply (uint64_t handle, uint16_t cmd, uint16_t flags, r = conn->send (buf, count, 0); if (r == -1) { nbdkit_error ("write data: %s: %m", name_of_nbd_cmd (cmd)); - return connection_set_status (-1); + connection_set_status (STATUS_DEAD); } } - - return 1; /* command processed ok */ } -static int +static void send_structured_reply_read (uint64_t handle, uint16_t cmd, const char *buf, uint32_t count, uint64_t offset) { @@ -421,7 +420,8 @@ send_structured_reply_read (uint64_t handle, uint16_t cmd, r = conn->send (&reply, sizeof reply, SEND_MORE); if (r == -1) { nbdkit_error ("write reply: %s: %m", name_of_nbd_cmd (cmd)); - return connection_set_status (-1); + connection_set_status (STATUS_DEAD); + return; } /* Send the offset + read data buffer. */ @@ -429,16 +429,15 @@ send_structured_reply_read (uint64_t handle, uint16_t cmd, r = conn->send (&offset_data, sizeof offset_data, SEND_MORE); if (r == -1) { nbdkit_error ("write data: %s: %m", name_of_nbd_cmd (cmd)); - return connection_set_status (-1); + connection_set_status (STATUS_DEAD); + return; } r = conn->send (buf, count, 0); if (r == -1) { nbdkit_error ("write data: %s: %m", name_of_nbd_cmd (cmd)); - return connection_set_status (-1); + connection_set_status (STATUS_DEAD); } - - return 1; /* command processed ok */ } /* Convert a list of extents into NBD_REPLY_TYPE_BLOCK_STATUS blocks. @@ -523,7 +522,7 @@ extents_to_block_descriptors (struct nbdkit_extents *extents, return blocks; } -static int +static void send_structured_reply_block_status (uint64_t handle, uint16_t cmd, uint16_t flags, uint32_t count, uint64_t offset, @@ -543,8 +542,10 @@ send_structured_reply_block_status (uint64_t handle, blocks = extents_to_block_descriptors (extents, flags, count, offset, &nr_blocks); - if (blocks == NULL) - return connection_set_status (-1); + if (blocks == NULL) { + connection_set_status (STATUS_DEAD); + return; + } reply.magic = htobe32 (NBD_STRUCTURED_REPLY_MAGIC); reply.handle = handle; @@ -556,7 +557,8 @@ send_structured_reply_block_status (uint64_t handle, r = conn->send (&reply, sizeof reply, SEND_MORE); if (r == -1) { nbdkit_error ("write reply: %s: %m", name_of_nbd_cmd (cmd)); - return connection_set_status (-1); + connection_set_status (STATUS_DEAD); + return; } /* Send the base:allocation context ID. */ @@ -564,7 +566,8 @@ send_structured_reply_block_status (uint64_t handle, r = conn->send (&context_id, sizeof context_id, SEND_MORE); if (r == -1) { nbdkit_error ("write reply: %s: %m", name_of_nbd_cmd (cmd)); - return connection_set_status (-1); + connection_set_status (STATUS_DEAD); + return; } /* Send each block descriptor. */ @@ -573,14 +576,12 @@ send_structured_reply_block_status (uint64_t handle, i == nr_blocks - 1 ? 0 : SEND_MORE); if (r == -1) { nbdkit_error ("write reply: %s: %m", name_of_nbd_cmd (cmd)); - return connection_set_status (-1); + connection_set_status (STATUS_DEAD); } } - - return 1; /* command processed ok */ } -static int +static void send_structured_reply_error (uint64_t handle, uint16_t cmd, uint16_t flags, uint32_t error) { @@ -599,7 +600,8 @@ send_structured_reply_error (uint64_t handle, uint16_t cmd, uint16_t flags, r = conn->send (&reply, sizeof reply, SEND_MORE); if (r == -1) { nbdkit_error ("write error reply: %m"); - return connection_set_status (-1); + connection_set_status (STATUS_DEAD); + return; } /* Send the error. */ @@ -608,18 +610,17 @@ send_structured_reply_error (uint64_t handle, uint16_t cmd, uint16_t flags, r = conn->send (&error_data, sizeof error_data, 0); if (r == -1) { nbdkit_error ("write data: %s: %m", name_of_nbd_cmd (cmd)); - return connection_set_status (-1); + connection_set_status (STATUS_DEAD); } /* No human readable error message at the moment. */ - - return 1; /* command processed ok */ } -int +void protocol_recv_request_send_reply (void) { GET_CONN; int r; + conn_status cs; struct nbd_request request; uint16_t cmd, flags; uint32_t magic, count, error = 0; @@ -630,24 +631,27 @@ protocol_recv_request_send_reply (void) /* Read the request packet. */ { ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&conn->read_lock); - r = connection_get_status (); - if (r <= 0) - return r; + cs = connection_get_status (); + if (cs <= STATUS_CLIENT_DONE) + return; r = conn->recv (&request, sizeof request); if (r == -1) { nbdkit_error ("read request: %m"); - return connection_set_status (-1); + connection_set_status (STATUS_DEAD); + return; } if (r == 0) { debug ("client closed input socket, closing connection"); - return connection_set_status (0); /* disconnect */ + connection_set_status (STATUS_CLIENT_DONE); /* disconnect */ + return; } magic = be32toh (request.magic); if (magic != NBD_REQUEST_MAGIC) { nbdkit_error ("invalid request: 'magic' field is incorrect (0x%x)", magic); - return connection_set_status (-1); + connection_set_status (STATUS_DEAD); + return; } flags = be16toh (request.flags); @@ -658,14 +662,17 @@ protocol_recv_request_send_reply (void) if (cmd == NBD_CMD_DISC) { debug ("client sent %s, closing connection", name_of_nbd_cmd (cmd)); - return connection_set_status (0); /* disconnect */ + connection_set_status (STATUS_CLIENT_DONE); /* disconnect */ + return; } /* Validate the request. */ if (!validate_request (cmd, flags, offset, count, &error)) { if (cmd == NBD_CMD_WRITE && - skip_over_write_buffer (conn->sockin, count) < 0) - return connection_set_status (-1); + skip_over_write_buffer (conn->sockin, count) < 0) { + connection_set_status (STATUS_DEAD); + return; + } goto send_reply; } @@ -677,8 +684,10 @@ protocol_recv_request_send_reply (void) if (buf == NULL) { error = ENOMEM; if (cmd == NBD_CMD_WRITE && - skip_over_write_buffer (conn->sockin, count) < 0) - return connection_set_status (-1); + skip_over_write_buffer (conn->sockin, count) < 0) { + connection_set_status (STATUS_DEAD); + return; + } goto send_reply; } } @@ -702,13 +711,14 @@ protocol_recv_request_send_reply (void) } if (r == -1) { nbdkit_error ("read data: %s: %m", name_of_nbd_cmd (cmd)); - return connection_set_status (-1); + connection_set_status (STATUS_DEAD); + return; } } } /* Perform the request. Only this part happens inside the request lock. */ - if (quit || !connection_get_status ()) { + if (quit || connection_get_status () == STATUS_CLIENT_DONE) { error = ESHUTDOWN; } else { @@ -720,8 +730,8 @@ protocol_recv_request_send_reply (void) /* Send the reply packet. */ send_reply: - if (connection_get_status () < 0) - return -1; + if (connection_get_status () < STATUS_CLIENT_DONE) + return; if (error != 0) { /* Since we're about to send only the limited NBD_E* errno to the @@ -742,19 +752,14 @@ protocol_recv_request_send_reply (void) (cmd == NBD_CMD_READ || cmd == NBD_CMD_BLOCK_STATUS)) { if (!error) { if (cmd == NBD_CMD_READ) - return send_structured_reply_read (request.handle, cmd, - buf, count, offset); + send_structured_reply_read (request.handle, cmd, buf, count, offset); else /* NBD_CMD_BLOCK_STATUS */ - return send_structured_reply_block_status (request.handle, - cmd, flags, - count, offset, - extents); + send_structured_reply_block_status (request.handle, cmd, flags, + count, offset, extents); } else - return send_structured_reply_error (request.handle, cmd, flags, - error); + send_structured_reply_error (request.handle, cmd, flags, error); } else - return send_simple_reply (request.handle, cmd, flags, buf, count, - error); + send_simple_reply (request.handle, cmd, flags, buf, count, error); } diff --git a/server/public.c b/server/public.c index 472ca623..6a9840bb 100644 --- a/server/public.c +++ b/server/public.c @@ -727,7 +727,8 @@ nbdkit_nanosleep (unsigned sec, unsigned nsec) */ bool has_quit = quit; assert (has_quit || - (conn && conn->nworkers > 0 && connection_get_status () < 1) || + (conn && conn->nworkers > 0 && + connection_get_status () < STATUS_ACTIVE) || (conn && (fds[2].revents & (POLLRDHUP | POLLHUP | POLLERR | POLLNVAL)))); if (has_quit) diff --git a/server/test-public.c b/server/test-public.c index 1cb759d1..1d83354f 100644 --- a/server/test-public.c +++ b/server/test-public.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2018-2021 Red Hat Inc. + * Copyright (C) 2018-2022 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -83,7 +83,7 @@ threadlocal_get_context (void) abort (); } -int +conn_status connection_get_status (void) { abort (); -- 2.37.3

If we detect a fatal error but do not close the socket back to the client, then we can create deadlock where we are stuck waiting to read NBD_CMD_DISC from the client but where the client is stuck waiting to read our reply. Try harder to explicitly inform the client any time that we have detected when our connection has degraded enough to warrant that we won't do any more writing, even if we still hang on to the socket for a bit longer for further reads. --- server/internal.h | 2 +- server/connections.c | 27 ++++++++++++++++++++------- server/crypto.c | 22 +++++++++++++--------- 3 files changed, 34 insertions(+), 17 deletions(-) diff --git a/server/internal.h b/server/internal.h index fa658364..69b4302c 100644 --- a/server/internal.h +++ b/server/internal.h @@ -194,7 +194,7 @@ typedef int (*connection_recv_function) (void *buf, size_t len) typedef int (*connection_send_function) (const void *buf, size_t len, int flags) __attribute__((__nonnull__ (1))); -typedef void (*connection_close_function) (void); +typedef void (*connection_close_function) (int how); /* struct context stores data per connection and backend. Primarily * this is the filter or plugin handle, but other state is also stored diff --git a/server/connections.c b/server/connections.c index 27177d70..1b6183df 100644 --- a/server/connections.c +++ b/server/connections.c @@ -62,7 +62,7 @@ static int raw_send_socket (const void *buf, size_t len, int flags); #ifndef WIN32 static int raw_send_other (const void *buf, size_t len, int flags); #endif -static void raw_close (void); +static void raw_close (int how); conn_status connection_get_status (void) @@ -97,6 +97,8 @@ connection_set_status (conn_status value) if (write (conn->status_pipe[1], &c, 1) != 1 && errno != EAGAIN) debug ("failed to notify pipe-to-self: %m"); } + if (conn->status >= STATUS_CLIENT_DONE && value < STATUS_CLIENT_DONE) + conn->close (SHUT_WR); conn->status = value; } if (conn->nworkers && @@ -348,7 +350,7 @@ free_connection (struct connection *conn) if (!conn) return; - conn->close (); + conn->close (SHUT_RDWR); /* Don't call the plugin again if quit has been set because the main * thread will be in the process of unloading it. The plugin.unload @@ -397,6 +399,7 @@ raw_send_socket (const void *vbuf, size_t len, int flags) ssize_t r; int f = 0; + assert (sock >= 0); #ifdef MSG_MORE if (flags & SEND_MORE) f |= MSG_MORE; @@ -427,6 +430,7 @@ raw_send_other (const void *vbuf, size_t len, int flags) const char *buf = vbuf; ssize_t r; + assert (sock >= 0); while (len > 0) { r = write (sock, buf, len); if (r == -1) { @@ -489,12 +493,21 @@ raw_recv (void *vbuf, size_t len) * close, so this function ignores errors. */ static void -raw_close (void) +raw_close (int how) { GET_CONN; - if (conn->sockin >= 0) - closesocket (conn->sockin); - if (conn->sockout >= 0 && conn->sockin != conn->sockout) - closesocket (conn->sockout); + if (conn->sockout >= 0 && how == SHUT_WR) { + if (conn->sockin == conn->sockout) + shutdown (conn->sockout, how); + else + closesocket (conn->sockout); + conn->sockout = -1; + } + else { + if (conn->sockin >= 0) + closesocket (conn->sockin); + if (conn->sockout >= 0 && conn->sockin != conn->sockout) + closesocket (conn->sockout); + } } diff --git a/server/crypto.c b/server/crypto.c index 1f605083..72486bf8 100644 --- a/server/crypto.c +++ b/server/crypto.c @@ -412,7 +412,7 @@ crypto_send (const void *vbuf, size_t len, int flags) * close, so this function ignores errors. */ static void -crypto_close (void) +crypto_close (int how) { GET_CONN; gnutls_session_t session = conn->crypto_session; @@ -420,17 +420,21 @@ crypto_close (void) assert (session != NULL); - gnutls_transport_get_int2 (session, &sockin, &sockout); + if (how == SHUT_WR) + gnutls_bye (session, GNUTLS_SHUT_WR); + else { + gnutls_transport_get_int2 (session, &sockin, &sockout); - gnutls_bye (session, GNUTLS_SHUT_RDWR); + gnutls_bye (session, GNUTLS_SHUT_RDWR); - if (sockin >= 0) - closesocket (sockin); - if (sockout >= 0 && sockin != sockout) - closesocket (sockout); + if (sockin >= 0) + closesocket (sockin); + if (sockout >= 0 && sockin != sockout) + closesocket (sockout); - gnutls_deinit (session); - conn->crypto_session = NULL; + gnutls_deinit (session); + conn->crypto_session = NULL; + } } #ifdef WIN32 -- 2.37.3

We have already exposed nbdkit_shutdown() as a way for a plugin to request that nbdkit itself shuts down when convenient (taking away the current and all other client connections). But we lacked a way for a plugin (or filter) to drop the current client, while still leaving nbdkit up for other clients. This has utility for testing client reconnect logic, as well as for emulating things like qemu's propensity to hard-disconnect on NBD_CMD_WRITE with more than 32M of data (even though nbdkit would normally accept up to 64M of data). Add a new API nbdkit_disconnect(int force) for that purpose, utilizing the logic already built in to connections.c. If force is true, we close the socket immediately (the client won't get responses to the current or any other parallel commands); if force is false, we mark the socket as not accepting further commands (current commands can still give their response to the client, but new requests from the current client are ignored with ESHUTDOWN until the client gives NBD_CMD_DISC). This patch provides the C interface (using int, to allow for future extensions beyond our current two values), and straightforward bindings to languages that already have nbdkit_shutdown bound (here, we use bool where it exists, since we don't have to worry as much about potential future API breaks if we need a third state in the future). Upcoming patches will then work on extending the sh/eval plugin to take advantage of the API, as well as enhancing the blocksize-policy filter to more closely emulate qemu. The logic for connection status changes slightly - there is now a fourth state tracking when we want a soft shutdown, but are still waiting for the client to send NBD_CMD_DISC - existing commands still get to send their reply unmolested, but future commands get an immediate ESHUTDOWN error. Rather than re-checking the state between dropping the read lock and grabbing the write lock, we now go by the state present after recv() first gives us a result at the start of a transaction. The testsuite covers both hard and soft disconnects, and in both plaintext and tls. --- docs/nbdkit-plugin.pod | 17 ++++- include/nbdkit-common.h | 3 +- tests/Makefile.am | 28 ++++++++ server/internal.h | 1 + server/connections.c | 3 +- server/nbdkit.syms | 3 +- server/protocol.c | 4 +- server/public.c | 14 +++- server/test-public.c | 10 ++- plugins/ocaml/NBDKit.mli | 5 +- plugins/ocaml/NBDKit.ml | 1 + plugins/ocaml/bindings.c | 12 +++- plugins/python/modfunctions.c | 14 ++++ plugins/rust/src/lib.rs | 6 ++ tests/test-disconnect-tls.sh | 126 +++++++++++++++++++++++++++++++++ tests/test-disconnect.sh | 100 ++++++++++++++++++++++++++ tests/test-disconnect-plugin.c | 95 +++++++++++++++++++++++++ 17 files changed, 431 insertions(+), 11 deletions(-) create mode 100755 tests/test-disconnect-tls.sh create mode 100755 tests/test-disconnect.sh create mode 100644 tests/test-disconnect-plugin.c diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod index 6e74ccad..d338cde8 100644 --- a/docs/nbdkit-plugin.pod +++ b/docs/nbdkit-plugin.pod @@ -1339,7 +1339,8 @@ Plugins and filters can call L<exit(3)> in the configuration phase Once nbdkit has started serving connections, plugins and filters should not call L<exit(3)>. However they may instruct nbdkit to shut -down by calling C<nbdkit_shutdown>: +down by calling C<nbdkit_shutdown>, or to disconnect a single client +by calling C<nbdkit_disconnect>: void nbdkit_shutdown (void); @@ -1349,6 +1350,20 @@ the plugin and all filters are unloaded cleanly which may take some time. Further callbacks from nbdkit into the plugin or filter may occur after you have called this. + void nbdkit_disconnect (int force); + +This function requests that the current connection be disconnected +(I<note> that it does not affect other connections, and the client may +try to reconnect). It is only useful from connected callbacks (that +is, after C<.open> and before C<.close>). If C<force> is true, nbdkit +will disconnect the client immediately, and the client will not +receive any response to the current command or any other commands in +flight in parallel threads; otherwise, nbdkit will not accept any new +commands from the client (failing all commands other than +C<NBD_CMD_DISC> with C<ESHUTDOWN>), but will allow existing commands +to complete gracefully. Either way, the next callback for the current +connection should be C<.close>. + =head1 PARSING COMMAND LINE PARAMETERS =head2 Parsing numbers diff --git a/include/nbdkit-common.h b/include/nbdkit-common.h index b0dcf715..dfdbab68 100644 --- a/include/nbdkit-common.h +++ b/include/nbdkit-common.h @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2013-2020 Red Hat Inc. + * Copyright (C) 2013-2022 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -137,6 +137,7 @@ NBDKIT_EXTERN_DECL (int64_t, nbdkit_peer_pid, (void)); NBDKIT_EXTERN_DECL (int64_t, nbdkit_peer_uid, (void)); NBDKIT_EXTERN_DECL (int64_t, nbdkit_peer_gid, (void)); NBDKIT_EXTERN_DECL (void, nbdkit_shutdown, (void)); +NBDKIT_EXTERN_DECL (void, nbdkit_disconnect, (int force)); NBDKIT_EXTERN_DECL (const char *, nbdkit_strdup_intern, (const char *str)); diff --git a/tests/Makefile.am b/tests/Makefile.am index 530b22bd..e951381d 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -253,6 +253,8 @@ TESTS += \ test-long-name.sh \ test-flush.sh \ test-swap.sh \ + test-disconnect.sh \ + test-disconnect-tls.sh \ test-shutdown.sh \ test-nbdkit-backend-debug.sh \ test-read-password.sh \ @@ -293,6 +295,8 @@ EXTRA_DIST += \ test-read-password.sh \ test-read-password-interactive.sh \ test-read-password-plugin.c \ + test-disconnect.sh \ + test-disconnect-tls.sh \ test-shutdown.sh \ test-single-from-file.sh \ test-single-sh.sh \ @@ -376,6 +380,30 @@ test_flush_plugin_la_LDFLAGS = \ $(NULL) test_flush_plugin_la_LIBADD = $(IMPORT_LIBRARY_ON_WINDOWS) +# check_LTLIBRARIES won't build a shared library (see automake manual). +# So we have to do this and add a dependency. +noinst_LTLIBRARIES += \ + test-disconnect-plugin.la \ + $(NULL) +test-disconnect.sh: test-disconnect-plugin.la +test-disconnect-tls.sh: test-disconnect-plugin.la keys.psk + +test_disconnect_plugin_la_SOURCES = \ + test-disconnect-plugin.c \ + $(top_srcdir)/include/nbdkit-plugin.h \ + $(NULL) +test_disconnect_plugin_la_CPPFLAGS = \ + -I$(top_srcdir)/include \ + -I$(top_builddir)/include \ + $(NULL) +test_disconnect_plugin_la_CFLAGS = $(WARNINGS_CFLAGS) +# For use of the -rpath option, see: +# https://lists.gnu.org/archive/html/libtool/2007-07/msg00067.html +test_disconnect_plugin_la_LDFLAGS = \ + -module -avoid-version -shared $(NO_UNDEFINED_ON_WINDOWS) -rpath /nowhere \ + $(NULL) +test_disconnect_plugin_la_LIBADD = $(IMPORT_LIBRARY_ON_WINDOWS) + # check_LTLIBRARIES won't build a shared library (see automake manual). # So we have to do this and add a dependency. noinst_LTLIBRARIES += \ diff --git a/server/internal.h b/server/internal.h index 69b4302c..229d707a 100644 --- a/server/internal.h +++ b/server/internal.h @@ -235,6 +235,7 @@ struct context { typedef enum { STATUS_DEAD, /* Connection is closed */ STATUS_CLIENT_DONE, /* Client has sent NBD_CMD_DISC */ + STATUS_SHUTDOWN, /* Server wants soft shutdown */ STATUS_ACTIVE, /* Client can make requests */ } conn_status; diff --git a/server/connections.c b/server/connections.c index 1b6183df..4d776f2a 100644 --- a/server/connections.c +++ b/server/connections.c @@ -90,7 +90,8 @@ connection_set_status (conn_status value) pthread_mutex_lock (&conn->status_lock)) abort (); if (value < conn->status) { - if (conn->nworkers && conn->status > STATUS_CLIENT_DONE) { + if (conn->nworkers && conn->status > STATUS_CLIENT_DONE && + value <= STATUS_CLIENT_DONE) { char c = 0; assert (conn->status_pipe[1] >= 0); diff --git a/server/nbdkit.syms b/server/nbdkit.syms index 0e897680..45cf3b45 100644 --- a/server/nbdkit.syms +++ b/server/nbdkit.syms @@ -1,5 +1,5 @@ # nbdkit -# Copyright (C) 2018-2021 Red Hat Inc. +# Copyright (C) 2018-2022 Red Hat Inc. # # Redistribution and use in source and binary forms, with or without # modification, are permitted provided that the following conditions are @@ -44,6 +44,7 @@ nbdkit_context_get_backend; nbdkit_context_set_next; nbdkit_debug; + nbdkit_disconnect; nbdkit_error; nbdkit_export_name; nbdkit_exports_count; diff --git a/server/protocol.c b/server/protocol.c index d1e01502..cc1e4ed8 100644 --- a/server/protocol.c +++ b/server/protocol.c @@ -631,10 +631,10 @@ protocol_recv_request_send_reply (void) /* Read the request packet. */ { ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&conn->read_lock); + r = conn->recv (&request, sizeof request); cs = connection_get_status (); if (cs <= STATUS_CLIENT_DONE) return; - r = conn->recv (&request, sizeof request); if (r == -1) { nbdkit_error ("read request: %m"); connection_set_status (STATUS_DEAD); @@ -718,7 +718,7 @@ protocol_recv_request_send_reply (void) } /* Perform the request. Only this part happens inside the request lock. */ - if (quit || connection_get_status () == STATUS_CLIENT_DONE) { + if (quit || cs < STATUS_ACTIVE) { error = ESHUTDOWN; } else { diff --git a/server/public.c b/server/public.c index 6a9840bb..c2f67451 100644 --- a/server/public.c +++ b/server/public.c @@ -728,7 +728,7 @@ nbdkit_nanosleep (unsigned sec, unsigned nsec) bool has_quit = quit; assert (has_quit || (conn && conn->nworkers > 0 && - connection_get_status () < STATUS_ACTIVE) || + connection_get_status () < STATUS_SHUTDOWN) || (conn && (fds[2].revents & (POLLRDHUP | POLLHUP | POLLERR | POLLNVAL)))); if (has_quit) @@ -1097,3 +1097,15 @@ nbdkit_printf_intern (const char *fmt, ...) va_end (ap); return ret; } + +NBDKIT_DLL_PUBLIC void +nbdkit_disconnect (int force) +{ + struct connection *conn = threadlocal_get_conn (); + + if (!conn) { + debug ("no connection in this thread, ignoring disconnect request"); + return; + } + connection_set_status (force ? STATUS_DEAD : STATUS_SHUTDOWN); +} diff --git a/server/test-public.c b/server/test-public.c index 1d83354f..4e4d8a2e 100644 --- a/server/test-public.c +++ b/server/test-public.c @@ -63,6 +63,8 @@ nbdkit_debug (const char *fs, ...) bool listen_stdin; bool configured; +bool verbose; +int tls; volatile int quit; #ifndef WIN32 @@ -89,14 +91,18 @@ connection_get_status (void) abort (); } +void +connection_set_status (conn_status v) +{ + abort (); +} + const char * backend_default_export (struct backend *b, int readonly) { abort (); } -int tls; - /* Unit tests. */ static bool diff --git a/plugins/ocaml/NBDKit.mli b/plugins/ocaml/NBDKit.mli index cc389ca0..cdfacf69 100644 --- a/plugins/ocaml/NBDKit.mli +++ b/plugins/ocaml/NBDKit.mli @@ -1,6 +1,6 @@ (* hey emacs, this is OCaml code: -*- tuareg -*- *) (* nbdkit OCaml interface - * Copyright (C) 2014-2020 Red Hat Inc. + * Copyright (C) 2014-2022 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -166,6 +166,9 @@ val export_name : unit -> string (** Binding for [nbdkit_shutdown]. Requests the server shut down. *) val shutdown : unit -> unit +(** Binding for [nbdkit_disconnect]. Requests disconnecting current client. *) +val disconnect : bool -> unit + (** Print a debug message when nbdkit is in verbose mode. *) val debug : ('a, unit, string, unit) format4 -> 'a diff --git a/plugins/ocaml/NBDKit.ml b/plugins/ocaml/NBDKit.ml index 3ad72acb..c94f4d57 100644 --- a/plugins/ocaml/NBDKit.ml +++ b/plugins/ocaml/NBDKit.ml @@ -166,6 +166,7 @@ external realpath : string -> string = "ocaml_nbdkit_realpath" external nanosleep : int -> int -> unit = "ocaml_nbdkit_nanosleep" external export_name : unit -> string = "ocaml_nbdkit_export_name" external shutdown : unit -> unit = "ocaml_nbdkit_shutdown" [@@noalloc] +external disconnect : bool -> unit = "ocaml_nbdkit_disconnect" [@@noalloc] external _debug : string -> unit = "ocaml_nbdkit_debug" [@@noalloc] let debug fs = ksprintf _debug fs external version : unit -> string = "ocaml_nbdkit_version" diff --git a/plugins/ocaml/bindings.c b/plugins/ocaml/bindings.c index ba95fb4a..c6c152b9 100644 --- a/plugins/ocaml/bindings.c +++ b/plugins/ocaml/bindings.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2014-2020 Red Hat Inc. + * Copyright (C) 2014-2022 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -165,6 +165,16 @@ ocaml_nbdkit_shutdown (value unitv) CAMLreturn (Val_unit); } +/* NB: noalloc function. */ +NBDKIT_DLL_PUBLIC value +ocaml_nbdkit_disconnect (value boolv) +{ + CAMLparam1 (boolv); + + nbdkit_disconnect (Bool_val (boolv)); + CAMLreturn (Val_unit); +} + /* NB: noalloc function. */ NBDKIT_DLL_PUBLIC value ocaml_nbdkit_debug (value strv) diff --git a/plugins/python/modfunctions.c b/plugins/python/modfunctions.c index 4cd45c3b..ac693923 100644 --- a/plugins/python/modfunctions.c +++ b/plugins/python/modfunctions.c @@ -93,6 +93,18 @@ do_shutdown (PyObject *self, PyObject *args) Py_RETURN_NONE; } +/* nbdkit.disconnect */ +static PyObject * +do_disconnect (PyObject *self, PyObject *args) +{ + int force; + + if (!PyArg_ParseTuple (args, "p:disconnect", &force)) + return NULL; + nbdkit_disconnect (force); + Py_RETURN_NONE; +} + /* nbdkit.parse_size */ static PyObject * parse_size (PyObject *self, PyObject *args) @@ -121,6 +133,8 @@ static PyMethodDef NbdkitMethods[] = { "Store an errno value prior to throwing an exception" }, { "shutdown", do_shutdown, METH_NOARGS, "Request asynchronous shutdown" }, + { "disconnect", do_disconnect, METH_VARARGS, + "Request disconnection from current client" }, { NULL } }; diff --git a/plugins/rust/src/lib.rs b/plugins/rust/src/lib.rs index 128334ef..a5b88e85 100644 --- a/plugins/rust/src/lib.rs +++ b/plugins/rust/src/lib.rs @@ -1046,6 +1046,7 @@ extern "C" { fn nbdkit_peer_name( addr: *mut libc::sockaddr, addrlen: *mut libc::socklen_t) -> c_int; fn nbdkit_shutdown(); + fn nbdkit_disconnect(force: bool); fn nbdkit_stdio_safe() -> c_int; } @@ -1106,6 +1107,11 @@ pub fn shutdown() { unsafe { nbdkit_shutdown() }; } +/// Request nbdkit to disconnect the current client. +pub fn disconnect(force: bool) { + unsafe { nbdkit_disconnect(force) }; +} + #[doc(hidden)] #[repr(C)] pub struct Plugin { diff --git a/tests/test-disconnect-tls.sh b/tests/test-disconnect-tls.sh new file mode 100755 index 00000000..00049b07 --- /dev/null +++ b/tests/test-disconnect-tls.sh @@ -0,0 +1,126 @@ +#!/usr/bin/env bash +# nbdkit +# Copyright (C) 2019-2022 Red Hat Inc. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# +# * Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# * Neither the name of Red Hat nor the names of its contributors may be +# used to endorse or promote products derived from this software without +# specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. + +source ./functions.sh +set -x + +requires nbdsh -c 'exit(not h.supports_tls())' + +# Does the nbdkit binary support TLS? +if ! nbdkit --dump-config | grep -sq tls=yes; then + echo "$0: nbdkit built without TLS support" + exit 77 +fi + +# Did we create the PSK keys file? +# Probably 'certtool' is missing. +if [ ! -s keys.psk ]; then + echo "$0: PSK keys file was not created by the test harness" + exit 77 +fi + +plugin=.libs/test-disconnect-plugin.$SOEXT +requires test -f $plugin + +sock=$(mktemp -u /tmp/nbdkit-test-sock.XXXXXX) +files="disconnect-tls.pid $sock" +cleanup_fn rm -f $files + +# Start nbdkit with the disconnect plugin, which has delayed reads and +# does disconnect on write based on export name. +start_nbdkit -P disconnect-tls.pid --tls require --tls-psk=keys.psk \ + -U $sock $plugin + +pid=`cat disconnect-tls.pid` + +# We can't use 'nbdsh -u "$uri" because of nbd_set_uri_allow_local_file. +# Empty export name does soft disconnect on write; the write and the +# pending read should still succeed, but second read attempt should fail. +nbdsh -c ' +import errno + +h.set_tls(nbd.TLS_REQUIRE) +h.set_tls_psk_file("keys.psk") +h.set_tls_username("qemu") +h.connect_unix("'"$sock"'") + +buf = nbd.Buffer(1) +c1 = h.aio_pread(buf, 1) +c2 = h.aio_pwrite(buf, 2) +h.poll(-1) +assert h.aio_peek_command_completed() == c2 +h.aio_command_completed(c2) +c3 = h.aio_pread(buf, 3) +h.poll(-1) +assert h.aio_peek_command_completed() == c3 +try: + h.aio_command_completed(c3) + assert False +except nbd.Error as ex: + assert ex.errnum == errno.ESHUTDOWN +h.poll(-1) +assert h.aio_peek_command_completed() == c1 +h.aio_command_completed(c1) +h.shutdown() +' + +# Non-empty export name does hard disconnect on write. The write and the +# pending read should fail with lost connection. +nbdsh -c ' +import errno + +h.set_tls(nbd.TLS_REQUIRE) +h.set_tls_psk_file("keys.psk") +h.set_tls_username("qemu") +h.set_export_name("a") +h.connect_unix("'"$sock"'") + +buf = nbd.Buffer(1) +c1 = h.aio_pread(buf, 1) +c2 = h.aio_pwrite(buf, 2) +while h.aio_in_flight() > 1: + h.poll(-1) +assert h.aio_is_ready() is False +try: + h.aio_command_completed(c1) + assert False +except nbd.Error as ex: + assert ex.errnum == errno.ENOTCONN +try: + h.aio_command_completed(c2) + assert False +except nbd.Error as ex: + assert ex.errnum == errno.ENOTCONN +' + +# nbdkit should still be running +kill -s 0 $pid diff --git a/tests/test-disconnect.sh b/tests/test-disconnect.sh new file mode 100755 index 00000000..1551dc03 --- /dev/null +++ b/tests/test-disconnect.sh @@ -0,0 +1,100 @@ +#!/usr/bin/env bash +# nbdkit +# Copyright (C) 2019-2022 Red Hat Inc. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# +# * Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# * Neither the name of Red Hat nor the names of its contributors may be +# used to endorse or promote products derived from this software without +# specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. + +source ./functions.sh +set -x + +requires_nbdsh_uri + +plugin=.libs/test-disconnect-plugin.$SOEXT +requires test -f $plugin + +sock=$(mktemp -u /tmp/nbdkit-test-sock.XXXXXX) +files="disconnect.pid $sock" +cleanup_fn rm -f $files + +# Start nbdkit with the disconnect plugin, which has delayed reads and +# does disconnect on write based on export name. +start_nbdkit -P disconnect.pid -U $sock $plugin + +pid=`cat disconnect.pid` + +# Empty export name does soft disconnect on write; the write and the +# pending read should still succeed, but second read attempt should fail. +nbdsh -u "nbd+unix:///?socket=$sock" -c ' +import errno + +buf = nbd.Buffer(1) +c1 = h.aio_pread(buf, 1) +c2 = h.aio_pwrite(buf, 2) +h.poll(-1) +assert h.aio_peek_command_completed() == c2 +h.aio_command_completed(c2) +c3 = h.aio_pread(buf, 3) +h.poll(-1) +assert h.aio_peek_command_completed() == c3 +try: + h.aio_command_completed(c3) + assert False +except nbd.Error as ex: + assert ex.errnum == errno.ESHUTDOWN +h.poll(-1) +assert h.aio_peek_command_completed() == c1 +h.aio_command_completed(c1) +h.shutdown() +' + +# Non-empty export name does hard disconnect on write. The write and the +# pending read should fail with lost connection. +nbdsh -u "nbd+unix:///a?socket=$sock" -c ' +import errno + +buf = nbd.Buffer(1) +c1 = h.aio_pread(buf, 1) +c2 = h.aio_pwrite(buf, 2) +while h.aio_in_flight() > 1: + h.poll(-1) +assert h.aio_is_ready() is False +try: + h.aio_command_completed(c1) + assert False +except nbd.Error as ex: + assert ex.errnum == errno.ENOTCONN +try: + h.aio_command_completed(c2) + assert False +except nbd.Error as ex: + assert ex.errnum == errno.ENOTCONN +' + +# nbdkit should still be running +kill -s 0 $pid diff --git a/tests/test-disconnect-plugin.c b/tests/test-disconnect-plugin.c new file mode 100644 index 00000000..181b262f --- /dev/null +++ b/tests/test-disconnect-plugin.c @@ -0,0 +1,95 @@ +/* nbdkit + * Copyright (C) 2013-2022 Red Hat Inc. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * + * * Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * + * * Neither the name of Red Hat nor the names of its contributors may be + * used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, + * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A + * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF + * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, + * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT + * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + */ + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <stdbool.h> + +#include <nbdkit-plugin.h> + +static void +disconnect_unload (void) +{ + nbdkit_debug ("clean disconnect"); +} + +static void * +disconnect_open (int readonly) +{ + return NBDKIT_HANDLE_NOT_NEEDED; +} + +static int64_t +disconnect_get_size (void *handle) +{ + return 1024*1024; +} + +#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL + +/* Reads are delayed to show effect of disconnect on in-flight commands */ +static int +disconnect_pread (void *handle, void *buf, uint32_t count, uint64_t offset) +{ + memset (buf, 0, count); + if (nbdkit_nanosleep (2, 0) == -1) + nbdkit_debug ("read delay ended early, returning success anyway"); + return 0; +} + +/* Writing causes a disconnect; export name determines severity. */ +static int +disconnect_pwrite (void *handle, const void *buf, uint32_t count, + uint64_t offset) +{ + const char *name = nbdkit_export_name (); + bool hard = name && *name; + nbdkit_debug ("%s disconnect triggered!", hard ? "hard" : "soft"); + nbdkit_disconnect (hard); + /* Despite the disconnect, we still claim the write succeeded */ + return 0; +} + +static struct nbdkit_plugin plugin = { + .name = "disconnect", + .version = PACKAGE_VERSION, + .unload = disconnect_unload, + .open = disconnect_open, + .get_size = disconnect_get_size, + .pread = disconnect_pread, + .pwrite = disconnect_pwrite, +}; + +NBDKIT_REGISTER_PLUGIN(plugin) -- 2.37.3

Allow this filter to emulate qemu's behavior of a hard disconnect on write attempts larger than 32M. --- .../nbdkit-blocksize-policy-filter.pod | 21 ++++ tests/Makefile.am | 12 +- filters/blocksize-policy/policy.c | 27 ++++- tests/test-blocksize-write-disconnect.sh | 107 ++++++++++++++++++ 4 files changed, 164 insertions(+), 3 deletions(-) create mode 100755 tests/test-blocksize-write-disconnect.sh diff --git a/filters/blocksize-policy/nbdkit-blocksize-policy-filter.pod b/filters/blocksize-policy/nbdkit-blocksize-policy-filter.pod index 691ad289..a377829f 100644 --- a/filters/blocksize-policy/nbdkit-blocksize-policy-filter.pod +++ b/filters/blocksize-policy/nbdkit-blocksize-policy-filter.pod @@ -10,6 +10,7 @@ maximum block size, and apply error policy [blocksize-minimum=N] [blocksize-preferred=N] [blocksize-maximum=N] + [blocksize-write-disconnect=N] =head1 DESCRIPTION @@ -49,6 +50,13 @@ read-modify-write for an unaligned write). With this filter you can use C<blocksize-error-policy=error> to reject these requests in the filter with an EINVAL error. The plugin will not see them. +Normally, nbdkit will accept write requests up to 64M in length, and +reply with a gracful error message rather than a hard disconnect for a +buffer up to twice that large. But many other servers (for example, +qemu-nbd) will give a hard disconnect for a write request larger than +32M. With this filter you can use C<blocksize-write-disconnect=32M> +to emulate the behavior of other servers. + =head2 Combining with L<nbdkit-blocksize-filter(1)> A related filter is L<nbdkit-blocksize-filter(1)>. That filter can @@ -87,6 +95,19 @@ means pass the request through to the plugin. Use C<error> to return an EINVAL error back to the client. The plugin will not see the badly formed request in this case. +=item B<blocksize-write-disconnect=>N + +(nbdkit E<ge> 1.34) + +If a client sends a write request which is larger than the specified +I<size> (using the usual size modifiers like C<32M>), abruptly close +the connection. This can be used to emulate qemu's behavior of +disconnecting for write requests larger than 32M, rather than nbdkit's +default of keeping the connection alive for write requests up to 128M +(although nbdkit does not let the plugin see requests larger than +64M). The write disconnect size is independent of any advertised +maximum block size or its accompanying error policy. + =item B<blocksize-minimum=>N =item B<blocksize-preferred=>N diff --git a/tests/Makefile.am b/tests/Makefile.am index e951381d..d59b797c 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -1464,8 +1464,16 @@ EXTRA_DIST += \ $(NULL) # blocksize-policy filter test. -TESTS += test-blocksize-policy.sh test-blocksize-error-policy.sh -EXTRA_DIST += test-blocksize-policy.sh test-blocksize-error-policy.sh +TESTS += \ + test-blocksize-policy.sh \ + test-blocksize-error-policy.sh \ + test-blocksize-write-disconnect.sh \ + $(NULL) +EXTRA_DIST += \ + test-blocksize-policy.sh \ + test-blocksize-error-policy.sh \ + test-blocksize-write-disconnect.sh \ + $(NULL) # cache filter test. TESTS += \ diff --git a/filters/blocksize-policy/policy.c b/filters/blocksize-policy/policy.c index 4ec07d36..f7cff2f1 100644 --- a/filters/blocksize-policy/policy.c +++ b/filters/blocksize-policy/policy.c @@ -42,11 +42,13 @@ #include <nbdkit-filter.h> #include "ispowerof2.h" +#include "rounding.h" /* Block size constraints configured on the command line (0 = unset). */ static uint32_t config_minimum; static uint32_t config_preferred; static uint32_t config_maximum; +static uint32_t config_disconnect; /* Error policy. */ static enum { EP_ALLOW, EP_ERROR } error_policy = EP_ALLOW; @@ -90,6 +92,12 @@ policy_config (nbdkit_next_config *next, nbdkit_backend *nxdata, config_maximum = r; return 0; } + else if (strcmp (key, "blocksize-write-disconnect") == 0) { + r = nbdkit_parse_size (value); + if (r == -1 || r > UINT32_MAX) goto parse_error; + config_disconnect = r; + return 0; + } return next (nxdata, key, value); } @@ -147,6 +155,14 @@ policy_config_complete (nbdkit_next_config_complete *next, } } + if (config_minimum && config_disconnect) { + if (config_disconnect <= config_minimum) { + nbdkit_error ("blocksize-write-disonnect must be larger than " + "blocksize-minimum"); + return -1; + } + } + return next (nxdata); } @@ -192,6 +208,8 @@ policy_block_size (nbdkit_next *next, void *handle, if (config_maximum) *maximum = config_maximum; + else if (config_disconnect) + *maximum = ROUND_DOWN (config_disconnect, *minimum); else *maximum = 0xffffffff; } @@ -220,7 +238,7 @@ policy_block_size (nbdkit_next *next, void *handle, * below. * * The 'data' flag is true for pread and pwrite (where we check the - * maximum bound). We don't check maximum for non-data- carrying + * maximum bound). We don't check maximum for non-data-carrying * calls like zero. * * The NBD specification mandates EINVAL for block size constraint @@ -303,6 +321,13 @@ policy_pwrite (nbdkit_next *next, void *handle, const void *buf, uint32_t count, uint64_t offset, uint32_t flags, int *err) { + if (config_disconnect && count > config_disconnect) { + nbdkit_error ("disconnecting client due to oversize write request"); + nbdkit_disconnect (true); + *err = ESHUTDOWN; + return -1; + } + if (check_policy (next, handle, "pwrite", true, count, offset, err) == -1) return -1; diff --git a/tests/test-blocksize-write-disconnect.sh b/tests/test-blocksize-write-disconnect.sh new file mode 100755 index 00000000..14a35100 --- /dev/null +++ b/tests/test-blocksize-write-disconnect.sh @@ -0,0 +1,107 @@ +#!/usr/bin/env bash +# nbdkit +# Copyright (C) 2019-2022 Red Hat Inc. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# +# * Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# * Neither the name of Red Hat nor the names of its contributors may be +# used to endorse or promote products derived from this software without +# specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. + +source ./functions.sh +set -e +set -x + +requires_plugin eval +requires nbdsh -c 'print(h.get_block_size)' +requires nbdsh -c 'print(h.get_strict_mode)' +requires_nbdsh_uri +requires dd iflag=count_bytes </dev/null + +# Libnbd does not let us test pwrite larger than 64M, so we can't +# test nbdkit's graceful behavior of writes up to 128M. +# In this test, odd size writes fail with EINVAL from the filter (size 1 too +# small, all others unaligned); evens 2 to 8M pass, 8M+2 to 16M fail with +# ENOMEM from the plugin, 16M+2 to 32M fail with EINVAL from the filter, +# 32M+1 to 64M kill the connection (ENOTCONN visible to client), and +# 64M+1 and above fails with ERANGE in libnbd. + +nbdkit -v -U - eval \ + block_size="echo 2 4096 16M" \ + get_size="echo 64M" \ + pread=' dd if=/dev/zero count=$3 iflag=count_bytes ' \ + pwrite=' if test $3 -gt $((8*1024*1024)); then + echo ENOMEM >&2; exit 1 + else + dd of=/dev/null + fi' \ + --filter=blocksize-policy \ + blocksize-error-policy=error blocksize-write-disconnect=32M \ + --run ' +nbdsh -u "$uri" -c " +import errno + +def check(h, size, expect_value, expect_traffic=True): + assert h.aio_is_ready() is True + buf = b\"0\" * size + if hasattr(h, \"stats_bytes_sent\"): + start = h.stats_bytes_sent() + try: + h.pwrite(buf, 0) + assert expect_value == 0 + except nbd.Error as ex: + assert expect_value == ex.errnum + if hasattr(h, \"stats_bytes_sent\"): + if expect_traffic: + assert h.stats_bytes_sent() > start + else: + assert h.stats_bytes_sent() == start + +h.set_strict_mode(0) # Bypass client-side safety checks +# Beyond 64M +check(h, 64*1024*1024 + 1, errno.ERANGE, False) +check(h, 64*1024*1024 + 2, errno.ERANGE, False) +# Small reads +check(h, 1, errno.EINVAL) +check(h, 2, 0) +# Near 8M boundary +check(h, 8*1024*1024 - 2, 0) +check(h, 8*1024*1024 - 1, errno.EINVAL) +check(h, 8*1024*1024, 0) +check(h, 8*1024*1024 + 1, errno.EINVAL) +check(h, 8*1024*1024 + 2, errno.ENOMEM) +# Near 16M boundary +check(h, 16*1024*1024 - 2, errno.ENOMEM) +check(h, 16*1024*1024 - 1, errno.EINVAL) +check(h, 16*1024*1024, errno.ENOMEM) +check(h, 16*1024*1024 + 1, errno.EINVAL) +check(h, 16*1024*1024 + 2, errno.EINVAL) +# Near 32M boundary +check(h, 32*1024*1024 - 2, errno.EINVAL) +check(h, 32*1024*1024 - 1, errno.EINVAL) +check(h, 32*1024*1024, errno.EINVAL) +check(h, 32*1024*1024 + 1, errno.ENOTCONN) +assert h.aio_is_ready() is False +"' -- 2.37.3