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

[Alex Bligh]

On 17 May 2016, at 16:52, Eric Blake <address@hidden> wrote:

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

Under "Option Types" it says:

    NBD_OPT_LIST (3) "Return zero or more NBD_REP_SERVER replies, one
    for each export, followed by NBD_REP_ACK or an error (such as
    NBD_REP_ERR_SHUTDOWN). The server MAY omit entries from this list if
    TLS has not been negotiated, the server is operating in SELECTIVETLS
    mode, and the entry concerned is a TLS-only export."

The language is imperative (if admittedly not RFC2119).

Also

    The server will reply to any option apart from NBD_OPT_EXPORT_NAME
    with reply packets in the following format:

again not in RFC2119 words, but says 'will' not 'MAY'.

I think the burden here is on the server to implement the protocol
as described.

I now accept that the wording should be cleared up :-)

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

OK. I think the problem is that if nbdkit supports 'fixed newstyle'
it should really support all of the options. Alternatively it can
support just 'newstyle' and then nothing will send anything bar
NBD_OPT_EXPORT_NAME.

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

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.

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

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

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

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

Indeed.

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

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.

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

--
Alex Bligh

signature.asc
Description: Message signed with OpenPGP using GPGMail