[Libguestfs] [libnbd PATCH] API: Add nbd_set_opt_mode to expose NEGOTIATING state

[Richard W.M. Jones]

I think this needs extra documentation in docs/libnbd.pod because it's
definitely not clear just from the rather thin manual page for
set_opt_mode how it works.  docs/libnbd.pod is a good place for a
broader description of how it works.

IIUC let me try to explain: we get through as far as the end of group
OPT_STRUCTURED_REPLY before we check the opt flag, and then the
remainder of opt negotiation can be controlled by the caller.  (I
really need to write a states -> dot graph visualizer!)  When we've
got to the negotiating state we can then get the list of exports, set
the export name we want, and then finish the connection with
nbd_opt_go.

Don't we need nbd_aio_opt_go etc?

Is there a case where you might want to influence TLS negotiation?  I
can't think of one right now.  Something about supplying a client
password from the command line maybe.

Should setting the opt flag cause a failure if we connect to an
oldstyle server?  (I can see arguments both ways.)

The updated list-exports example certainly shows the advantage of the
new API.

More comments inline below ...

On Mon, Aug 10, 2020 at 09:09:11PM -0500, Eric Blake wrote:
> diff --git a/generator/states-magic.c b/generator/states-magic.c
> index 944728d..2ad3a96 100644
> --- a/generator/states-magic.c
> +++ b/generator/states-magic.c
...
> @@ -42,8 +42,10 @@ STATE_MACHINE {
>    }
> 
>    version = be64toh (h->sbuf.new_handshake.version);
> -  if (version == NBD_NEW_VERSION)
> +  if (version == NBD_NEW_VERSION) {
> +    h->sbuf.option.option = 0;
>      SET_NEXT_STATE (%.NEWSTYLE.START);
> +  }
>    else if (version == NBD_OLD_VERSION)
>      SET_NEXT_STATE (%.OLDSTYLE.START);
>    else {

What does this hunk do?

> diff --git a/generator/states-newstyle.c b/generator/states-newstyle.c
> index 573f724..e61f373 100644
> --- a/generator/states-newstyle.c
> +++ b/generator/states-newstyle.c
> @@ -1,5 +1,5 @@
>  /* nbd client library in userspace: state machine
> - * Copyright (C) 2013-2019 Red Hat Inc.
> + * Copyright (C) 2013-2020 Red Hat Inc.
>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Lesser General Public
> @@ -112,6 +112,25 @@ handle_reply_error (struct nbd_handle *h)
> 
>  STATE_MACHINE {
>   NEWSTYLE.START:
> +  if (h->opt_mode) {
> +    switch (h->sbuf.option.option) {
> +    case NBD_OPT_GO:
> +      SET_NEXT_STATE (%OPT_SET_META_CONTEXT.START);
> +      return 0;
> +    case NBD_OPT_ABORT:
> +      h->sbuf.option.version = htobe64 (NBD_NEW_VERSION);
> +      h->sbuf.option.option = htobe32 (NBD_OPT_ABORT);
> +      h->sbuf.option.optlen = htobe32 (0);
> +      h->wbuf = &h->sbuf;
> +      h->wlen = sizeof h->sbuf.option;
> +      SET_NEXT_STATE (%SEND_OPT_ABORT);
> +      return 0;
> +    case 0:
> +      break;
> +    default:
> +      abort ();
> +    }
> +  }
>    h->rbuf = &h->sbuf;
>    h->rlen = sizeof h->sbuf.gflags;
>    SET_NEXT_STATE (%RECV_GFLAGS);

And this hunk which I guess is related to above but I couldn't quite
work out why it's needed either.

> @@ -136,6 +155,11 @@ STATE_MACHINE {
>      return 0;
>    }
> 
> +  if ((h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE) == 0)
> +    h->protocol = "newstyle";
> +  else
> +    h->protocol = "newstyle-fixed";
> +
>    cflags = h->gflags;
>    h->sbuf.cflags = htobe32 (cflags);
>    h->wbuf = &h->sbuf;

This makes me think maybe we should just derive this string from the
gflags when the caller calls get_protocol.

> diff --git a/lib/is-state.c b/lib/is-state.c
> index 1a85c7a..e019e53 100644
> --- a/lib/is-state.c
> +++ b/lib/is-state.c
> @@ -1,5 +1,5 @@
>  /* NBD client library in userspace
> - * Copyright (C) 2013-2019 Red Hat Inc.
> + * Copyright (C) 2013-2020 Red Hat Inc.
>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Lesser General Public
> @@ -58,6 +58,12 @@ nbd_internal_is_state_connecting (enum state state)
>    return is_connecting_group (group);
>  }
> 
> +bool
> +nbd_internal_is_state_negotiating (enum state state)
> +{
> +  return state == STATE_NEGOTIATING;
> +}
> +
>  bool
>  nbd_internal_is_state_ready (enum state state)
>  {

When I was reading the rest of the patch I thought it would have been
easier to review if the addition of the new state was in a separate
commit.

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