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

Eric Blake eblake at redhat.com
Tue Aug 11 17:38:30 UTC 2020


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




More information about the Libguestfs mailing list