[Libguestfs] [PATCH libnbd PROPOSAL] Add APIs for listing exports from an NBD server.

Eric Blake eblake at redhat.com
Tue Jul 21 15:18:24 UTC 2020


On 7/20/20 11:47 AM, Richard W.M. Jones wrote:
> On Mon, Jul 20, 2020 at 10:50:51AM -0500, Eric Blake wrote:
>> Is it worth trying to have some sort of a callback mode where the
>> callback function is called once per export name advertised while
>> the connection attempt is underway, rather than this post-mortem
>> query of the data copied into the nbd handle?  But as your added
>> example code showed, this wasn't too bad to code with.
> 
> My reasoning not to have a callback is that it's both a little harder
> to use, but more importantly ties us to a specific set of per-export
> fields.  For example if we had a callback which was:
> 
>    int cb (const char *export_name);
> 
> then you could only have an export name, but my reading of the
> protocol spec is that in fact there's already another field we are
> ignoring (some sort of description - not sure what servers actually
> set it), and there's scope for more fields in future.
> 
> With the current API we can add more fields in future.

As mentioned in the nbdkit counterpart thread, the existing 
NBD_REP_SERVER only sets a description string (no room for further 
extension without adding a new NBD_OPT_/NBD_REP pair - although I also 
outlined such a pair in that thread).  But 'qemu-nbd -D' is such a 
server that sets the description, so yes, we might as well expose it now.

> 
>>> +++ b/generator/states-newstyle-opt-list.c
>>
>>> +STATE_MACHINE {
>>> + NEWSTYLE.OPT_LIST.START:
>>> +  if (!h->list_exports) {
>>> +    SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START);
>>> +    return 0;
>>> +  }
>>> +
>>> +  h->sbuf.option.version = htobe64 (NBD_NEW_VERSION);
>>> +  h->sbuf.option.option = htobe32 (NBD_OPT_LIST);
>>> +  h->sbuf.option.optlen = 0;
>>> +  h->wbuf = &h->sbuf;
>>> +  h->wlen = sizeof (h->sbuf.option);
>>> +  SET_NEXT_STATE (%SEND);
>>> +  return 0;
>>
>> Is it worth enhancing the code to add another list mode where we can
>> also query NBD_OPT_INFO on each export thus listed?  (Compare to
>> 'qemu-nbd --list').  Hmm, if we do that, list mode may need to be an
>> enum instead of a bool.  But that would be a followup because it
>> adds more complexity; this is already a good addition on its own.
> 
> I'm actually a little unclear on the protocol spec around this.  Would
> we be able to issue N x NBD_OPT_INFO (after NBD_OPT_LIST, on the same
> connection)?  I was envisaging opening one new connection per export
> and just using the usual APIs to fetch the data.

Yes, you can send N x NBD_OPT_INFO, and even NBD_OPT_GO, all on the same 
connection.  My 'qemu-nbd --list' implementation does:

NBD_OPT_LIST
foreach name received:
   NBD_OPT_INFO
NBD_OPT_ABORT

to gracefully end the connection.

The more I think about it, the more I wonder if we can just insert more 
states into the state machine, by letting the user opt-in to NBD_OPT 
control.  Something like:

back-compat with libnbd 1.2 usage:

nbd_create
nbd_set_export_name("name")
nbd_connect*
   open socket
   NBD_OPT_GO(name from earlier)
nbd_aio_is_ready

but where the opt-in client with 1.4 API uses:

nbd_create
nbd_set_opt_mode(true)
nbd_connect*
   open socket
nbd_opt_list(cb)
   NBD_OPT_LIST
   cb(name, description)
   cb(name, description)
   ...
nbd_opt_go("name")
   NBD_OPT_GO("name")
nbd_aio_is_ready

That is, without opt mode, you _have_ to set the export name before 
nbd_connect*, because nbd_connect will automatically jump into nbd_go; 
but when you set opt mode true, you can set the export name directly 
with the new nbd_opt_go, and can in the meantime can call as many API 
managing other NBD_OPT as we decide to implement.

So that would mean the following APIs to be added (or modifying what 
we've already pushed with your initial experiment):

nbd_set_opt_mode(bool) - enable or disable opt mode
nbd_opt_list(cb) - valid only during opt mode, to send NBD_OPT_LIST
nbd_opt_abort() - valid only during opt mode, to end connection gracefully
nbd_opt_info(name) - valid only during opt mode, to send NBD_OPT_INFO
nbd_opt_go(name) - valid only during opt mode, to send NBD_OPT_GO

existing APIs change as follows:
nbd_can_* - additionally becomes valid during opt mode after a 
successful nbd_opt_info
nbd_connect* - performs nbd_opt_go(name) with the prior 
nbd_set_export_name when not in opt mode, or stops while still in 
negotiation when in opt mode


> 
>>> +  case NBD_REP_ACK:
>>> +    /* Finished receiving the list. */
>>> +    SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START);
>>> +    return 0;
>>> +
>>> +  default:
>>> +    if (handle_reply_error (h) == -1) {
>>> +      SET_NEXT_STATE (%.DEAD);
>>
>> Would this be better as %.CLOSED, or even a transition into a state
>> where we send a clean NBD_OPT_ABORT for graceful shutdown rather
>> than abrupt disconnect from the server?
> 
> In fact we cannot move to CLOSED.  The reason is obscure, it's because
> you hit this code while doing the connect:
> 
>    https://github.com/libguestfs/libnbd/blob/3599c79480df2932b3dba0b6f9c689634768c423/lib/connect.c#L47

Again, that's because our current state machine is calling NBD_OPT_GO 
unconditionally, and with the name hardcoded by the last 
nbd_set_export_name.  With a new opt-in opt mode, we then let the client 
pick the export name at the time the client is done with opt mode and 
calls nbd_opt_go.

> 
> It doesn't really work to have connect APIs return an error for what
> is not in fact an error.  However to complicate things, with qemu-nbd
> -x we do return an error from connect* each time because we cannot set
> the export name correctly until we've fetched the export names.  Hence
> the example ignores the return value from connect (unless ENOTSUP).
> 
> This is a bit of a mess ...

Good thing we aren't committed to the API yet - but when we do get it 
nailed down, v1.4 makes sense.


>>> +char *
>>> +nbd_unlocked_get_list_export_name (struct nbd_handle *h,
>>> +                                   int i)
>>> +{
>>> +  char *name;
>>> +
>>> +  if (!h->list_exports) {
>>> +    set_error (EINVAL, "list exports mode not selected on this handle");
>>> +    return NULL;
>>> +  }
>>> +  if (i < 0 || i >= (int) h->nr_exports) {
>>
>> That cast makes sense, but looks odd - any server that tries to send
>> more than 2G export names (by assuming that our size_t > 32 bits) is
>> sending a LOT of traffic in response to a single NBD_OPT_LIST
>> command. Would it be better to just track the list size as int, and
>> change the state machine to fail if the server hasn't completed its
>> listing in X number of responses (perhaps where X is 1 million, or
>> determined by the user, or...)?
> 
> Yes we should definitely limit this to something reasonable.  There's
> not a good way to handle servers which can serve any export name
> (nbdkit info as you pointed out).

The more I think about it, the more I like the idea of a callback (the 
client deals with the callback as it sees fit, rather than making libnbd 
store the memory to copy what it read from the wire just to hand it to 
the client later).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org




More information about the Libguestfs mailing list