11:47 a.m.
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/3599c79480df2932b3dba0b6f9c6896...
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