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

2016-05-22 Thread Richard W.M. Jones 
On Sat, May 21, 2016 at 11:53:12PM +0200, Wouter Verhelst wrote:
> On Tue, May 17, 2016 at 10:50:01AM -0600, Eric Blake wrote:
> > 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.
> 
> I think this makes most sense. If you don't look at export names, you
> effectively don't support them, and you can't be expected to send a list
> of "supported" names, because *everything* is supported (or, put a
> different way, nothing is).
> 
> I note that nbdkit has been patched to now send the empty name, which
> is also fine as a way to fix interoperability in this particular case --

nbdkit now fully supports export names via the '-e' option.
This discussion is moot.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

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

2016-05-21 Thread Wouter Verhelst 
On Tue, May 17, 2016 at 10:50:01AM -0600, Eric Blake wrote:
> 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.

I think this makes most sense. If you don't look at export names, you
effectively don't support them, and you can't be expected to send a list
of "supported" names, because *everything* is supported (or, put a
different way, nothing is).

I note that nbdkit has been patched to now send the empty name, which
is also fine as a way to fix interoperability in this particular case --
but for the general case, I think if we want to define ways for a server
to explicitly say that it doesn't support export names, returning
ERR_UNSUP to OPT_LIST seems cleaner.

(That does mean I'll have to fix nbd-client, since it currently assumes
that fixed newstyle implies support for OPT_LIST -- but that should be
an easy enough patch)

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

Sortof. Like I said, nbd-client assumes that servers which support fixed
newstyle will also support OPT_LIST -- but that's mostly an artefact of
the fact that OPT_LIST was used as a test case for fixed newstyle, and
isn't necessarily a good enough reason to make that a required part of
the protocol.

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
   people in the world who think they really understand all of its rules,
   and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12

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

2016-05-17 Thread Richard W.M. Jones 

Just to close this thread out, I have implemented the exportname in
qemu.  NBD_OPT_LIST returns the exportname.  nbdkit now interoperates
with qemu 2.5 and 2.6.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

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

2016-05-17 Thread Alex Bligh 

On 17 May 2016, at 17:56, Eric Blake  wrote:

> That's why I wonder if we need to document a minimum cutoff at which
> clients should assume will always be serviced, and which servers should
> not treat as an attack, and whether it should be larger than 32.

I don't think we need a specific number, but I think a one sentence
statement that servers are in general permitted to disconnect clients
which are behaving in a denial of service attack type manner would be
useful.

Given nbdkit only ever has one export, it seems eminently reasonable
for it to use a lower number.

--
Alex Bligh






signature.asc
Description: Message signed with OpenPGP using GPGMail

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

2016-05-17 Thread Alex Bligh 

On 17 May 2016, at 17:50, Eric Blake  wrote:
> 
> 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).
...
> 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.

Option 3: (which I would prefer) fix the wording indicating what
options are mandatory (I think they all should be but that's another
discussion really), and in the meantime simply have nbdkit return
the empty string as an export name in NBD_OPT_LIST as you
originally suggested, which is what qemu will connect to by default,
and which is what other clients connect to by default.

I don't want to be an arse about this, but I think the standard is
meant to be be normative, IE influence behaviour, rather than just
document existing behaviour. In this instance I think nbdkit
is actually wrong (returning zero entries cannot be right as
that explicitly says nothing is exported, whereas an error is
forgiveable perhaps), and the author is OK to fix it, so I think
we should fix the software to confirm to the standard, not the
standard to conform to the software.

--
Alex Bligh






signature.asc
Description: Message signed with OpenPGP using GPGMail

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

2016-05-17 Thread Eric Blake 
On 05/17/2016 10:26 AM, Alex Bligh wrote:
> 
> On 17 May 2016, at 16:54, Richard W.M. Jones  wrote:
> 
>> On Tue, May 17, 2016 at 04:22:06PM +0100, Alex Bligh wrote:
>>> 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").
>>
>> The protocol doc doesn't mention "default" (assuming you mean that
>> literal string).  It says:
> 
> As per my message to Eric, I meant use an arbitrary piece of
> text in your reply to NBD_OPT_LIST (it doesn't matter what it is,
> 'default' is not special). It doesn't matter because you ignore
> what is passed in NBD_OPT_EXPORT_NAME. I was just suggesting making
> things more readable for clients that did a list.
> 
>>> 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.
>>
>> It implements it, it's just that there wasn't a way to implement
>> anything other than returning an error since we accept anything as an
>> export name.
> 
> In which case you should just return an export name. Any
> export name will work. The fact you don't have names for your
> exports doesn't mean (in my book) you can error the list
> request. You have one export. You don't know what it's name is,
> but you should list it.

Except that the qemu 2.6 client is specifically checking that the name
it is looking for is present in the list, and gives up trying if the
name was not found. And there is no way for the server to know a priori
which name the client is trying to match.  An arbitrary name is less
likely to be found; the empty name "" is the most likely arbitrary name,
but even then, there is no guarantee that the qemu client is using the
"" default name, and since NBD_OPT_EXPORT_NAME doesn't care, I think
it's nicer to state that NBD_OPT_LIST should ONLY be supported if names
matter to NBD_OPT_EXPORT_NAME (and should return NBD_REP_ERR_UNSUP when
names don't matter).

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



signature.asc
Description: OpenPGP digital signature

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

2016-05-17 Thread Eric Blake 
On 05/17/2016 10:39 AM, Richard W.M. Jones wrote:
> On Tue, May 17, 2016 at 09:59:02AM -0600, Eric Blake wrote:
>> On 05/17/2016 09:52 AM, Eric Blake wrote:
> 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)?
>>
>> Okay, I just downloaded nbdkit code and checked; nbdkit is sending 0
>> entries and NBD_REP_ACK (for success), and NOT an error code; so it is
>> implying that there are NO known export names but that the command was
>> recognized.
> 
> Bleah you are right.  I misread the code I wrote :-(
> 
> We can add a empty string list entry there easily enough.

Will adding the empty string break qemu 2.5 connections?  qemu 2.6  is
picky in that if the name it is requesting is not in the advertised
list, it gives up; but if there is NO advertised list, it assumes the
name will just work.  Since nbdkit has the latter behavior, I'm thinking
that nbdkit returning NBD_REP_ERR_UNSUP instead of NBD_REP_ACK is a
smarter course of action.

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



signature.asc
Description: OpenPGP digital signature

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

2016-05-17 Thread Eric Blake 
On 05/17/2016 10:41 AM, Richard W.M. Jones wrote:
> On Tue, May 17, 2016 at 10:05:50AM -0600, Eric Blake wrote:
>> On 05/17/2016 09:58 AM, Richard W.M. Jones wrote:
>>> On Tue, May 17, 2016 at 09:52:30AM -0600, Eric Blake wrote:
 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.
>>>
>>> The purpose of nbdkit is to be a server for qemu, to be a replacement
>>> for qemu-nbd in cases where proprietary code cannot be combined with
>>> qemu for copyright/licensing/legal reasons.  So we aim to make sure we
>>> can interoperate with qemu.  No need to change any standards for
>>> nbdkit!  Clarifying standards documents is OK though.
>>
>> I also noticed that nbdkit forcefully rejects a client that sends more
>> than 32 NBD_OPT_ commands, even though this is perfectly valid behavior
>> on the part of the client.  Maybe the protocol should document a
>> (higher) limit on minimum number of options a client can expect to be
>> serviced before the server dropping the connection because the client is
>> wasting the server's time.
> 
> This, as you say, is a brake on clients that try to waste time by
> sending infinite numbers of options.  Is there any danger that 32 is
> too small?

Yes. Consider a client that connects to a server that lists more than 32
exports in NBD_OPT_LIST, then the client calls (the still-experimental)
NBD_OPT_INFO on each of those exports, to learn further details about
each export, before finally using NBD_OPT_GO to pick the one the user
likes best.

That's why I wonder if we need to document a minimum cutoff at which
clients should assume will always be serviced, and which servers should
not treat as an attack, and whether it should be larger than 32.

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



signature.asc
Description: OpenPGP digital signature

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

2016-05-17 Thread 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

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

2016-05-17 Thread Richard W.M. Jones 
On Tue, May 17, 2016 at 09:59:02AM -0600, Eric Blake wrote:
> On 05/17/2016 09:52 AM, Eric Blake wrote:
> >>> 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)?
> 
> Okay, I just downloaded nbdkit code and checked; nbdkit is sending 0
> entries and NBD_REP_ACK (for success), and NOT an error code; so it is
> implying that there are NO known export names but that the command was
> recognized.

Bleah you are right.  I misread the code I wrote :-(

We can add a empty string list entry there easily enough.

Rich.

> >>
> >> 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.
> 
> Except that nbdkit is NOT failing NBD_OPT_LIST, just merely listing 0
> replies.
> 
> >  But I would also make that interpretation for
> > NBD_OPT_LIST succeeding with NBD_REP_ACK with 0 NBD_REP_SERVER replies -
> 
> which is the nbdkit behavior
> 
> > 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.
> 
> 
> -- 
> Eric Blake   eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 



-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

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

2016-05-17 Thread Richard W.M. Jones 
On Tue, May 17, 2016 at 10:05:50AM -0600, Eric Blake wrote:
> On 05/17/2016 09:58 AM, Richard W.M. Jones wrote:
> > On Tue, May 17, 2016 at 09:52:30AM -0600, Eric Blake wrote:
> >> 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.
> > 
> > The purpose of nbdkit is to be a server for qemu, to be a replacement
> > for qemu-nbd in cases where proprietary code cannot be combined with
> > qemu for copyright/licensing/legal reasons.  So we aim to make sure we
> > can interoperate with qemu.  No need to change any standards for
> > nbdkit!  Clarifying standards documents is OK though.
> 
> I also noticed that nbdkit forcefully rejects a client that sends more
> than 32 NBD_OPT_ commands, even though this is perfectly valid behavior
> on the part of the client.  Maybe the protocol should document a
> (higher) limit on minimum number of options a client can expect to be
> serviced before the server dropping the connection because the client is
> wasting the server's time.

This, as you say, is a brake on clients that try to waste time by
sending infinite numbers of options.  Is there any danger that 32 is
too small?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

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

2016-05-17 Thread Alex Bligh 

On 17 May 2016, at 16:54, Richard W.M. Jones  wrote:

> On Tue, May 17, 2016 at 04:22:06PM +0100, Alex Bligh wrote:
>> 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").
> 
> The protocol doc doesn't mention "default" (assuming you mean that
> literal string).  It says:

As per my message to Eric, I meant use an arbitrary piece of
text in your reply to NBD_OPT_LIST (it doesn't matter what it is,
'default' is not special). It doesn't matter because you ignore
what is passed in NBD_OPT_EXPORT_NAME. I was just suggesting making
things more readable for clients that did a list.

>> 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.
> 
> It implements it, it's just that there wasn't a way to implement
> anything other than returning an error since we accept anything as an
> export name.

In which case you should just return an export name. Any
export name will work. The fact you don't have names for your
exports doesn't mean (in my book) you can error the list
request. You have one export. You don't know what it's name is,
but you should list it.

-- 
Alex Bligh

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

2016-05-17 Thread Alex Bligh 

On 17 May 2016, at 16:52, Eric Blake  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

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

2016-05-17 Thread Eric Blake 
On 05/17/2016 09:58 AM, Richard W.M. Jones wrote:
> On Tue, May 17, 2016 at 09:52:30AM -0600, Eric Blake wrote:
>> 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.
> 
> The purpose of nbdkit is to be a server for qemu, to be a replacement
> for qemu-nbd in cases where proprietary code cannot be combined with
> qemu for copyright/licensing/legal reasons.  So we aim to make sure we
> can interoperate with qemu.  No need to change any standards for
> nbdkit!  Clarifying standards documents is OK though.

I also noticed that nbdkit forcefully rejects a client that sends more
than 32 NBD_OPT_ commands, even though this is perfectly valid behavior
on the part of the client.  Maybe the protocol should document a
(higher) limit on minimum number of options a client can expect to be
serviced before the server dropping the connection because the client is
wasting the server's time.

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



signature.asc
Description: OpenPGP digital signature

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

2016-05-17 Thread Eric Blake 
On 05/17/2016 09:52 AM, Eric Blake wrote:
>>> 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)?

Okay, I just downloaded nbdkit code and checked; nbdkit is sending 0
entries and NBD_REP_ACK (for success), and NOT an error code; so it is
implying that there are NO known export names but that the command was
recognized.

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

Except that nbdkit is NOT failing NBD_OPT_LIST, just merely listing 0
replies.

>  But I would also make that interpretation for
> NBD_OPT_LIST succeeding with NBD_REP_ACK with 0 NBD_REP_SERVER replies -

which is the nbdkit behavior

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


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



signature.asc
Description: OpenPGP digital signature

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

2016-05-17 Thread Richard W.M. Jones 
On Tue, May 17, 2016 at 09:52:30AM -0600, Eric Blake wrote:
> 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.

The purpose of nbdkit is to be a server for qemu, to be a replacement
for qemu-nbd in cases where proprietary code cannot be combined with
qemu for copyright/licensing/legal reasons.  So we aim to make sure we
can interoperate with qemu.  No need to change any standards for
nbdkit!  Clarifying standards documents is OK though.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

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

2016-05-17 Thread Richard W.M. Jones 
On Tue, May 17, 2016 at 04:22:06PM +0100, Alex Bligh wrote:
> 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").

The protocol doc doesn't mention "default" (assuming you mean that
literal string).  It says:

  A special, "empty", name (i.e., the length field is zero and no name
  is specified), is reserved for a "default" export, to be used in
  cases where explicitly specifying an export name makes no sense.

which seems to indicate only the empty string should be used for this
case.

Unfortunately qemu 2.5 (as a client) treated exportname == "" as
meaning that the old-style protocol should be used.  This is at least
fixed in qemu 2.6, so nbdkit could now answer NBD_OPT_LIST with a
single empty string, although we lose backwards compatibility with
qemu 2.5.

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

It implements it, it's just that there wasn't a way to implement
anything other than returning an error since we accept anything as an
export name.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

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

2016-05-17 Thread Eric Blake 
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

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

2016-05-17 Thread Alex Bligh 
Eric, Richard,

On 17 May 2016, at 16:09, Eric Blake  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" 
>>> 
>>> 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