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

[Eric Blake]

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