Eric Blake
2020-Aug-11 17:38 UTC
Re: [Libguestfs] [libnbd PATCH] API: Add nbd_set_opt_mode to expose NEGOTIATING state
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
Eric Blake
2020-Aug-11 18:02 UTC
Re: [Libguestfs] [libnbd PATCH] API: Add nbd_set_opt_mode to expose NEGOTIATING state
On 8/11/20 12:38 PM, Eric Blake wrote:> 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. >>> >> 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.I missed this question on my first pass, although I somewhat answered it above. When the user has tls=1, then letting the user decide when to attempt STARTTLS makes sense (the user may get a rather consistent failure of everything else if the server requires TLS). I don't know the TLS code well enough to know if it has modes where interactive passwords might be needed in place of pointing at certificates, but that is also a reason for why fine-grained control over when to try TLS makes sense. For tls=2, defaulting to automatic STARTTLS before reaching state Negotiating makes the most sense, but if user control for things like password entry is needed, that's still a place where we could add a knob.>> >> Should setting the opt flag cause a failure if we connect to an >> oldstyle server? (I can see arguments both ways.)We don't know if the server is oldstyle until after the magic numbers have been parsed. As coded in this patch, state Negotiating can only be reached from a newstyle-fixed server (not even a plain newstyle server will get here, since that must go straight to NBD_OPT_EXPORT_NAME which has no error detection short of killing the connection). So our options after the user calls nbd_set_opt_mode(nbd, true) are: 1. silently proceed to nbd_aio_is_ready; the user never got a chance to reach Negotiating, and can readily deduce that this was because the server didn't support negotiation. The lack of negotiation does not stop the user from using the export. 2. error out at the magic number detection to inform the user that negotiation mode is not reachable with the given server. But the user can always create a new nbd handle, and retry again this time without setting nbd_set_opt_mode. I coded 1, but 2 also makes sense. I could go either way, if you have a preference.>> >> The updated list-exports example certainly shows the advantage of the >> new API.And more improvements to come once I convert the existing list_export code into nbd_opt_list(callback), so that I can also improve --list mode in nbdinfo.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?The existing NEWSTYLE.START handles gflags/cflags prior to moving into NBD_OPT code, and was only ever reachable once. This patch adds a transition from NEGOTIATING to NEWSTYLE.START on command issue; and any time you change groups, the state machine requires you to jump to the new group's START state. Therefore, NEWSTYLE.START is now entered multiple times, and has to know _where_ within the group to resume operations. The change you pointed to in states-magic.c, as well as all nbd_opt_* command issues, will each set h->sbuf.option.option as the witness of why NEWSTYLE is being (re-)entered; since 0 is not a valid NBD_OPT_ code, that is our witness to fall through to the normal gflags/cflags on the first pass, when non-zero, it is our witness of which other nbd_opt_* command wants to fast-forward to the appropriate existing sub-group to handle that option. I'll see if I can add a decent comment. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Eric Blake
2020-Aug-11 19:04 UTC
Re: [Libguestfs] [libnbd PATCH] API: Add nbd_set_opt_mode to expose NEGOTIATING state
On 8/11/20 12:38 PM, Eric Blake wrote:>>> @@ -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.I spoke too soon, originally hoping to get rid of h->protocol altogether. But we still set h->protocol = "oldstyle" in a different file, and have nothing else in h that lets us tell oldstyle apart from newstyle (only newstyle-fixed is easy to detect in h->gflags). Rather, this hunk was code motion, to make nbd_get_protocol work in Negotiating state (we were previously setting it in NEWSTYLE.FINISH, which is too late). But again, that sort of change can be separated out to make it easier to see (adding the Negotiating state done in a different patch than allowing existing commands to be callable in Negotiating state). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Richard W.M. Jones
2020-Aug-11 20:31 UTC
Re: [Libguestfs] [libnbd PATCH] API: Add nbd_set_opt_mode to expose NEGOTIATING state
On Tue, Aug 11, 2020 at 01:02:40PM -0500, Eric Blake wrote:> >>Should setting the opt flag cause a failure if we connect to an > >>oldstyle server? (I can see arguments both ways.) > > We don't know if the server is oldstyle until after the magic > numbers have been parsed. As coded in this patch, state Negotiating > can only be reached from a newstyle-fixed server (not even a plain > newstyle server will get here, since that must go straight to > NBD_OPT_EXPORT_NAME which has no error detection short of killing > the connection). So our options after the user calls > nbd_set_opt_mode(nbd, true) are: > > 1. silently proceed to nbd_aio_is_ready; the user never got a chance > to reach Negotiating, and can readily deduce that this was because > the server didn't support negotiation. The lack of negotiation does > not stop the user from using the export. > > 2. error out at the magic number detection to inform the user that > negotiation mode is not reachable with the given server. But the > user can always create a new nbd handle, and retry again this time > without setting nbd_set_opt_mode. > > I coded 1, but 2 also makes sense. I could go either way, if you > have a preference.2 is horrible for consumers of the API, so 1 is easier to consume. It may depend on how we expect this to be used. If it's only ever for listing exports, then because oldstyle servers don't have exports there's no need to interrupt the connection. It might be worth a test especially since it's just testing against “nbdkit -o” and making sure it doesn't do anything bad/stupid. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
Seemingly Similar Threads
- Re: [libnbd PATCH] API: Add nbd_set_opt_mode to expose NEGOTIATING state
- Re: [libnbd PATCH] API: Add nbd_set_opt_mode to expose NEGOTIATING state
- [libnbd PATCH] API: Add nbd_set_opt_mode to expose NEGOTIATING state
- [libnbd PATCH v2 06/13] api: Add nbd_opt_abort and nbd_aio_opt_abort
- [libnbd PATCH v4 20/25] generator: Actually request extended headers