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