1:02 p.m.
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