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

[Richard W.M. Jones]

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.

> >+++ 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.

> >+  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

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 ...

> >+int
> >+nbd_unlocked_set_list_exports (struct nbd_handle *h, bool list)
> >+{
> >+  h->list_exports = true;
> 
> s/true/list/ (you never really tested clearing the mode...)

Oops.

> >+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).

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org