[Libguestfs] [nbdkit PATCH v2 7/7] server: Better newstyle .open failure handling

[Richard W.M. Jones]

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 <eblake at redhat.com>
> ---
>  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