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

[Eric Blake]

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

[replying as I read, so I may seem to change my mind as I go...]

>>
>> As I read the protocol, I don't see "default" as a permissible name of
>> the default export, just "".
> 
> Sorry, I meant that if Richard implements NBD_OPT_LIST he could either
> return an export name of "" or "default". Or for that matter "nbdkit"
> or "hello world". Just "default" might display better than "". As he's
> ignoring whatever comes back in NBD_OPT_EXPORT_NAME it doesn't really
> matter what he returns in NBD_OPT_LIST.

Except that qemu client DOES check that the returned list includes the
name that qemu plans on sending to NBD_OPT_EXPORT_NAME (that is, if the
client wants to connect to "foo", it expects "foo" in the list returned
by NBD_OPT_LIST).

Option 1: I think what I would prefer is a doc patch to the NBD protocol
that explicitly states that returning 0 names followed by NBD_REP_ACK
(current nbdkit behavior) implies that the server doesn't honor names at
all, and will let ANY name work for NBD_OPT_EXPORT_NAME/NBD_OPT_GO; then
patch qemu client to treat 0 names the same as NBD_REP_UNSUP (the server
doesn't honor names, so assume any name will work).

> 
>> 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
> 
> Yes. This is a valid way of saying "I have no exports that work".

Except that nbdkit is using it as a way of saying "ALL export names
work" - and that's the special case I'm leaning towards documenting, and
fixing qemu client to support.

> 
>> (rather annoying in that it tells you nothing about whether
>> NBD_OPT_EXPORT_NAME/NBD_OPT_GO will succeed anyways).
> 
> If there are no exports then NBD_OPT_GO can't be expected to work.

Option 2: An alternative solution would be to allow nbdkit to fail
NBD_OPT_LIST with NBD_REP_ERR_UNSUP, at which point qemu client of 2.6
should just ignore the failure and proceed on to NBD_OPT_EXPORT_NAME.
It is the fact that it is returning NBD_REP_ACK with 0 names that is
giving qemu grief.

> 
>>  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)?
> 
> I don't think so. A server might legitimately serve an empty list to
> one IP and a non-empty list to another IP depending on configuration.

Okay, in such a case, returning 0 names to mean NBD_OPT_GO will never
succeed makes sense.  So I'd argue that NBD_OPT_LIST should NOT succeed
in the case where names are ignored, and that nbdkit could be fixed by
merely ALWAYS returning NBD_REP_ERR_UNSUP to NBD_OPT_LIST.

>>> 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.
> 
> I think the correct thing for nbdkit to do would be to return a single
> entry with an arbitrary name.

Still not ideal, because qemu 2.6 won't connect if the arbitrary name
doesn't match qemu's expectations.  But qemu 2.6 WILL connect if nbdkit
rejects the command instead of succeeding the command with 0 exports (my
Option 2 above).

> 
> I can't see much harm in a client being 'nice' if it gets an UNSUP
> error, but other errors (e.g. TLSREQD) have to be respected as errors.

That's what qemu 2.6 does: UNSUP errors are special cased to continue
negotiation, all other errors are treated as "can't possibly succeed" so
it terminates immediately.  The interoperability problem is that
returning 0 names (or even 1 name but where the arbitrary name doesn't
match qemu's expectation) is ALSO treated as grounds for "my export is
not available, give up".  So the more I type, the more I'm leaning
towards Option 2: nbdkit should reject NBD_OPT_LIST with
NBD_REP_ERR_UNSUP, rather than succeeding it.

> 
>> 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.
> 
> I think it's simpler than that. Servers claiming to implement fixed
> newstyle in my view need to implement all the options unless the options
> are marked as optional. I admit (now) those could be clarified.

But to date, I think ALL of the options (except NBD_OPT_EXPORT_NAME)
_are_ optional.  In fact, I'm arguing that per Option 2, we WANT
NBD_OPT_LIST to be optional for servers that don't care about names.



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

signature.asc
Description: OpenPGP digital signature