On Fri, Sep 27, 2019 at 11:48:49PM -0500, Eric Blake wrote: > If a plugin's .open or .get_size or .can_write fails, right now that > is fatal to the connection. When nbdkit was first implemented, this > made sense (there was no way to report errors to oldstyle or > NBD_OPT_EXPORT_NAME). But now that newstyle is around, it's rather > abrupt to hang up on the client, and better is to return an error to > NBD_OPT_GO, and let the client choose what to do (most clients will > probably hang up, whether gracefully with NBD_OPT_ABORT or abruptly, > rather than try other NBD_OPT_*, but _we_ shouldn't be the ones > forcing their hand). > > For an example of what this improves, if you run: > > $ nbdkit -fv sh - <<\EOF > case $1 in get_size) echo oops >&2; exit 1;; *) exit 2;; esac > EOF > > Pre-patch, qemu complains about the abrupt server death: > $ qemu-nbd --list > qemu-nbd: Failed to read option reply: Unexpected end-of-file before all > bytes were read > > Post-patch, qemu gets the desired error message, and exits gracefully: > $ qemu-nbd --list > qemu-nbd: Requested export not available > > Note that this does not fix the pre-existing problem that we can end > up calling .finalize even when .prepare was skipped or failed (that > latent problem was first exposed for the rare client that calls > NBD_OPT_INFO before NBD_OPT_GO, see commit a6b88b19); a later patch > will have to add better bookkeeping for that, and for better handling > of reopen requests from the retry filter. > > Signed-off-by: Eric Blake <[email protected]> > --- > server/protocol-handshake-newstyle.c | 32 +++++++++++++++++++++------- > server/protocol-handshake-oldstyle.c | 3 +++ > 2 files changed, 27 insertions(+), 8 deletions(-) > > diff --git a/server/protocol-handshake-newstyle.c > b/server/protocol-handshake-newstyle.c > index 2480d7a3..878fe53f 100644 > --- a/server/protocol-handshake-newstyle.c > +++ b/server/protocol-handshake-newstyle.c > @@ -198,7 +198,7 @@ conn_recv_full (struct connection *conn, void *buf, > size_t len, > > /* Sub-function of negotiate_handshake_newstyle_options below. It > * must be called on all non-error paths out of the options for-loop > - * in that function. > + * in that function, and must not cause any wire traffic. > */ > static int > finish_newstyle_options (struct connection *conn, uint64_t *exportsize) > @@ -322,7 +322,9 @@ negotiate_handshake_newstyle_options (struct connection > *conn) > if (check_export_name (conn, option, data, optlen, optlen, true) == -1) > return -1; > > - /* We have to finish the handshake by sending handshake_finish. */ > + /* We have to finish the handshake by sending handshake_finish. > + * On failure, we have to disconnect. > + */ > if (finish_newstyle_options (conn, &exportsize) == -1) > return -1; > > @@ -460,16 +462,30 @@ negotiate_handshake_newstyle_options (struct connection > *conn) > * or else we drop the support for that context. > */ > if (check_export_name (conn, option, &data[4], exportnamelen, > - optlen - 6, true) == -1) > - return -1; > + optlen - 6, true) == -1) { > + if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_INVALID) > + == -1) > + return -1; > + continue; > + } > > /* The spec is confusing, but it is required that we send back > * NBD_INFO_EXPORT, even if the client did not request it! > * qemu client in particular does not request this, but will > - * fail if we don't send it. > + * fail if we don't send it. Note that if .open fails, but we > + * succeed at .close, then we merely return an error to the > + * client and let them try another NBD_OPT, rather than > + * disconnecting. > */ > - if (finish_newstyle_options (conn, &exportsize) == -1) > - return -1; > + if (finish_newstyle_options (conn, &exportsize) == -1) { > + if (backend->finalize (backend, conn) == -1) > + return -1; > + backend_close (backend, conn); > + if (send_newstyle_option_reply (conn, option, > + NBD_REP_ERR_UNKNOWN) == -1) > + return -1; > + continue; > + } > > if (send_newstyle_option_reply_info_export (conn, option, > NBD_REP_INFO, > @@ -497,7 +513,7 @@ negotiate_handshake_newstyle_options (struct connection > *conn) > } > > /* Unlike NBD_OPT_EXPORT_NAME, NBD_OPT_GO sends back an ACK > - * or ERROR packet. > + * or ERROR packet. If this was NBD_OPT_LIST, call .close. > */ > if (send_newstyle_option_reply (conn, option, NBD_REP_ACK) == -1) > return -1; > diff --git a/server/protocol-handshake-oldstyle.c > b/server/protocol-handshake-oldstyle.c > index 4efc668b..45a1a486 100644 > --- a/server/protocol-handshake-oldstyle.c > +++ b/server/protocol-handshake-oldstyle.c > @@ -52,6 +52,9 @@ protocol_handshake_oldstyle (struct connection *conn) > > assert (tls != 2); /* Already filtered in main */ > > + /* With oldstyle, our only option if .open or friends fail is to > + * disconnect, as we cannot report the problem to the client. > + */ > if (protocol_common_open (conn, &exportsize, &eflags) == -1) > return -1;
Yes this seems fine. Probably best to check that the series doesn't break ‘make check-valgrind’ before pushing however. Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top _______________________________________________ Libguestfs mailing list [email protected] https://www.redhat.com/mailman/listinfo/libguestfs
