Re: [Nbd] Thoughts about the handshake protocol

[Wouter Verhelst <w@...112...>]

On Tue, May 31, 2011 at 10:28:38AM +0200, Goswin von Brederlow wrote:
> Wouter Verhelst <w@...112...> writes:
> 
> > On Tue, May 31, 2011 at 09:03:32AM +0200, Goswin von Brederlow wrote:
> >> Ok. So you will use up the first reserved bit now. My suggestion then
> >> would be to do this all through setting a flag instead of making
> >> NBD_OPT_EXPORT_NAME 'special':
> >> 
> >> Bit 0 (server+client): support new-style handshake V2
> >> 
> >> Required changes:
> >> 
> >> - NBD_OPT_EXPORT_NAME now recieves a reply containing size+flags of the
> >>   export. No trailing zeroes. This option is required before NBD_OPT_END.
> >
> > It also selects that export for use. I suppose a second option could be
> > added that gets the info but does not select it for use later on, if
> > needs be, but I don't think that would be necessary.
> 
> I would simply allow repeated use of this option. The last use is the
> export that will be used.

That could work, too.

> >> - All options are acknowledged by the server
> >>   S: 0x49484156454F5054
> >>   S: 32 bits denoting the requested option
> >>   S: 32 bit error code (0 == SUCCESS)
> >>   S: 32 bit length of option data
> >>   S: length bytes of data
> >> 
> >>   On success the payload is option specific, on error the payload is
> >>   plain text describing the error.
> >
> > Hmm. It seems more efficient if the server only replies when it has to:
> > - The client is using an option that the server does not understand: the
> >   server must reply with an error
> > - The client is using an option that requests some information from the
> >   server (like the "list exports" one you talked about)
> 
> Then how long should the client wait for the server to send back an
> error before it sends the next option?
> 
> E.g:
> 
> C: NBD_OPT_EXPORT_NAME foo
> S: size+flags
> C: NBD_OPT_SYNC
> -- wait --
> C: NBD_OPT_END
> -- wait --
> C - switching to nbd mode
> S: error, NBD_OPT_SYNC not known
> 
> At that point the kernel will be quite confused and kill the connection.

No, not that way.

C: NBD_OPT_EXPORT_NAME foo
S: size + flags
C: NBD_OPT_FOO
C: NBD_OPT_END
S: error, NBD_OPT_SYNC not known
S: NBD_OPT_END
C: disconnect or switch to kernel

[...]
> >> - New option NBD_OPT_END
> >>   This must be the last option send. On success the NBD protocol
> >>   starts. On error the connection is terminated.
> >
> > Right, with an error message in the payload.
> >
> >> Optional features (i.e. just reserve the numbers for the options,
> >> clients don't have to implement them):
> >> 
> >> - New option NBD_OPT_LIST_EXPORTS
> >>   Return a list of available exports (array of 0 terminated strings?)
> >
> > Mmm. I'd prefer a newline-separated list of export names, that's going
> > to be easier -- and there's no need to support complex export names.
> 
> Works too. What can an export name be? Any UTF-8 string not containing 0
> and \n? Only printable ascii chars? [-_0-9a-zA-Z.:#+]?

I'd say letters and numbers, and must not start with a number.

> >> - New option NBD_OPT_SYNC
> >>   Open the export with O_SYNC
> >
> > Maybe make the 'sync' per-export option a tristate (yes, no, maybe, with
> > 'maybe' defined as 'yes if the client asks for it, no otherwise').
> 
> "maybe" would translate into the server sending back an error on
> NBD_OPT_SYNC.

No, it would not -- on the contrary.

> >> - New option NBD_OPT_READ_ONLY
> >>   Open the export read-only
> >
> > No need for the client to ask the server that -- if it wants to open an
> > export read-only, all it needs to do is not issue write requests.
> 
> Safety feature. Prevents accidental writes. Prevents unionfs from
> copying the file on open and other changes (e.g. mtime update) on other
> filesystems. Also usefull in combination with something I forgot:
> 
> - New option NBD_OPT_EXCLUSIVE
>   The device must only be opened by one client for write or many client
>   read-only. Fail if someone else has it open for writing already and
>   fail future clients trying to open it for writing.
> 
> Exclusive should probably be default since most use cases won't like
> multiple clients accessing a device. So maybe it should be
> NPT_OPT_SHARED with reverse semantic.

With that, it could make sense I guess. But it'd be hard to do this in a
non-multithreaded implementation of nbd. I guess patches are welcome :-)

[...]
> >> - New option NBD_OPT_ENABLE_PRIVATE
> >>   Enable private options between client and server. Used for
> >>   experimental features between client and server of the same make.
> >>   Client payload: client identifier
> >>   Server payload: defined by client
> >
> > I'd prefer to define that as "server identifier", rather than "defined
> > by client". Or make it "defined by implementor", that works too.
> 
> I ment defined by implementor of the client here. The server may only
> report success if it knows what the client implementation expects in
> return.

Mmm. Might also make sense to allow the ENABLE_PRIVATE option to be
specified multiple times, and to allow all messages enabled by all
private identifiers that were successfully negotiated, then.

[...]
-- 
The volume of a pizza of thickness a and radius z can be described by
the following formula:

pi zz a