12:38 p.m.
On 8/11/20 12:12 PM, Richard W.M. Jones wrote:
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.
Yes, good idea.
State-wise, the existing flow was:
Created
- Progress only by command issue - namely one of nbd_connect_*
Connecting
- Progress by aio_notify_read/aio_notify_write (as driven by
aio_poll), and progresses through socket establishment, magic numbers,
and handshaking
Loop of:
Ready
- Progress by command issue (I/O commands) or aio_notify_read
Processing
- Progress by aio_notify_read/aio_notify_write, commands are queued
rather than causing state transitions
finally, end in Closed (clean disconnect) or Dead (dead socket or
protocol error)
the new flow breaks Connecting into two halves, adding one more point
where command issue can cause a state transition:
Created
- Progress only by command issue - namely one of nbd_connect_*
Connecting
- Progress by aio_notify_read/aio_notify_write (as driven by
aio_poll), and progresses through socket establishment and magic numbers
Loop of:
Negotiating
- Progress by command issue (nbd_opt_*)
Connecting
- Progress by aio_notify_read/aio_notify_write, no commands
accepted, and processes remaining handshaking
Before getting back to Ready and the normal loop.
One thing to remember is that handshaking is always synchronous - the
client is not allowed to send the next NBD_OPT_* until after the server
has finished replying to the previous. Since everything is in lockstep,
nothing needs to be queued, and we don't have quite the complexity of
Ready handling simultaneous command issue, read notify (for completion
of previous commands) and write notify (when the outgoing queue can make
progress again). The state machine still shows as Connecting as it
makes progress towards Ready for anything that is not waiting on a user
command, and skipping state Negotiating is all the more that is required
for old clients to not see a difference. But where old clients jumped
straight to Dead on any error reply to an NBD_OPT request, the new mode
can jump back to Negotiating to give the client a chance to try
something else, moving back into Connecting once a command is started.
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.
For now, OPT_STRUCTURED_REPLY is still automatic, although I might
expose it to the user to attempt (I'm certainly thinking about what else
to expose or keep automatic in followup patches; letting the user
control OPT_STARTTLS when tls=1 (but not when tls=0 or tls=2) may be
useful).
Don't we need nbd_aio_opt_go etc?
Probably - that was one of my questions. In fact, adding aio_opt_* is
easy (the sync version would call the aio version, which kicks off the
NBD_OPT_ write; the difference is that the aio version then returns
immediately, probably in aio_is_connecting, while the sync version uses
aio_poll until it is back to either aio_is_negotiating, aio_is_ready, or
aio_is_dead).
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.
Doable. Probably even something we could separate out, to keep the meat
of this patch more focused.
> +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.
Sure; I'll try and make that split for v2, as well as adding aio_opt_*.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org