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

[Alex Bligh]

Eric, Richard,

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

> [adding nbd-list]
> 
> On 05/17/2016 03:53 AM, Richard W.M. Jones wrote:
>> On Tue, Feb 16, 2016 at 05:34:41PM +0100, Paolo Bonzini wrote:
>>> From: "Daniel P. Berrange" <address@hidden>
>>> 
>>> With the new style protocol, the NBD client will currenetly
>>> send NBD_OPT_EXPORT_NAME as the first (and indeed only)
>>> option it wants. The problem is that the NBD protocol spec
>>> does not allow for returning an error message with the
>>> NBD_OPT_EXPORT_NAME option. So if the server mandates use
>>> of TLS, the client will simply see an immediate connection
>>> close after issuing NBD_OPT_EXPORT_NAME which is not user
>>> friendly.
>>> 
> 
>> I just bisected qemu 2.6 to find out where it breaks interop with
>> nbdkit, and it is this commit.
>> 
>> nbdkit implements the newstyle protocol, but doesn't implement export
>> names (yet, maybe in future).  It processes the NBD_OPT_EXPORT_NAME
>> option, but ignores the export name and carries on regardless.
>> 
>> 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").

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.

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

Or "default".

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

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

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

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

--
Alex Bligh

signature.asc
Description: Message signed with OpenPGP using GPGMail