10:18 a.m.
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/3599c79480df2932b3dba0b6f9c6896...
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