Re: [Qemu-devel] [Nbd] [PULL 23/28] nbd: always query export list in fixed new style protocol

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5399 bytes --]

On 05/17/2016 09:22 AM, Alex Bligh wrote:

>>> nbdkit's implemention of NBD_OPT_LIST returns an error, because there
>>> is no such thing as a list of export names supported (in effect nbdkit
>>> allows any export name).
> 
> nbdkit is non-compliant in that case. Support of NBD_OPT_LIST is
> compulsory, even if you support it by returning a nameless export
> (or default). Moreover support of export names is compulsory
> (even if you have a single fixed one called "" or "default").

Where does the protocol state that? I don't see any mandatory NBD_OPT in
the protocol, except for NBD_OPT_EXPORT_NAME (as I read it, a server may
reply with NBD_REP_ERR_UNSUP for every other option).  I'm fine if we
WANT to make NBD_OPT_LIST mandatory (and either document under
NBD_OPT_LIST that NBD_REP_ERR_UNSUP cannot be used, or document under
NBD_REP_ERR_UNSUP that it cannot be used with NBD_OPT_LIST), but that
would make the current nbdkit non-conforming; so it might be nicer to
make a change to the protocol document that instead permits current
nbdkit behavior and puts the burden on clients to interoperate when
NBD_OPT_LIST is not supported.

> 
> This is assuming nbdkit supports 'fixed newstyle'; if nbdkit merely
> supports 'newstyle' negotiation, then we know qemu won't connect
> to it as modern qemu only supports servers that support 'fixed newstyle'
> I believe.

Not quite true. Qemu as a client is perfectly fine connecting with a
plain 'newstyle' server (and not just 'fixed newstyle'); but will limit
itself to JUST NBD_OPT_EXPORT_NAME in that case.  So nbdkit must be
exposing 'fixed newstyle' for qemu to be attempting NBD_OPT_LIST.

> 
>> Perhaps nbdkit should implement NBD_OPT_LIST returning just "" (the
>> default name) as its only list entry?
> 
> Or "default".

As I read the protocol, I don't see "default" as a permissible name of
the default export, just "".

Also, we currently state that NBD_OPT_LIST has zero or more
NBD_REP_SERVER replies, which means that it is valid for the command to
succeed with NBD_REP_ACK _without_ advertising any exports at all
(rather annoying in that it tells you nothing about whether
NBD_OPT_EXPORT_NAME/NBD_OPT_GO will succeed anyways).  Should we reword
that to require that if NBD_REP_ACK is sent, then at least one
NBD_REP_SERVER reply was given (where "" is the obvious name, but at the
same time where "" is not mandatory if some other name is given)?

> 
>> And at some point, nbdkit should probably implement NBD_OPT_GO (which is
>> a nicer replacement to NBD_OPT_EXPORT_NAME; I've got patches pending for
>> qemu to implement that).
>>
>> In fact, if NBD_OPT_GO is supported, then my patches to qemu won't even
>> try NBD_OPT_LIST.
> 
> Sure, but NBD_OPT_INFO/GO is still an experimental protocol extension.

Hopefully not much longer; we have multiple implementations converging
on interoperable use of it.

> 
>>> Therefore I don't believe the assumption made here -- that you can
>>> list export names and choose them on the client side -- is a valid
>>> one.
>>
>> Qemu is not choosing an export name, so much as proving whether any
>> OTHER errors can be detected (such as export name not present, or TLS
>> required).  It _should_ be gracefully ignoring NBD_OPT_LIST errors if
>> such errors don't definitively prove that the export will not succeed,
>> but I guess this is not happening.  I'll see if I can tweak the qemu
>> logic to be more forgiving of nbdkit's current behavior.
> 
> Whilst that is fair enough, NBD_OPT_LIST is a mandatory part of the
> specification. If a server doesn't implement mandatory parts of
> the specification, then bad things will happen.

Again, I don't see how the protocol justifies that claim. Maybe it
should, but I'd like to see your reasoning for stating that it is
mandatory that the server support it.

> 
> My interpretation of NBD_OPT_LIST failing would be 'this server
> doesn't have anything it can export'.

Indeed, and that's why qemu as a client is currently dropping the
connection with nbdkit.  But I would also make that interpretation for
NBD_OPT_LIST succeeding with NBD_REP_ACK with 0 NBD_REP_SERVER replies -
so maybe it is worth a note in the protocol how to detect servers that
are exporting exactly one volume and don't care what name you pass, then
tweaking either nbdkit, qemu, or both to comply to that added protocol
wording.

> 
>>> Naturally the protocol document
>>> (https://github.com/yoe/nbd/blob/master/doc/proto.md) isn't clear on
>>> this case.
>>
>> You're right that we may also want to tweak the NBD protocol to make
>> this interoperability point obvious.
> 
> I can't actually see the issue here. It explains what needs to be
> implemented by the server, and that includes NBD_OPT_LIST. Very
> happy to add some clarity, but I'm not sure where it's needed.

I've hinted at it above - either an explicit statement that servers
cannot reject NBD_OPT_LIST with NBD_REP_UNSUP, and that if they have no
other exports, then the SHOULD include an NBD_REP_SERVER with name "";
or an explicit statement that if a server rejects NBD_OPT_LIST, then the
client SHOULD assume that any name will work for
NBD_OPT_EXPORT_NAME/NBD_OPT_GO.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]