Re: [Nbd] Thoughts about the handshake protocol

[Goswin von Brederlow <goswin-v-b@...186...>]

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.

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

>> - If any further reserved bits (1-15) are not ACKed by the client or any
>>   of the client options (bits 16-31) are not understood the server may
>>   repsond to the first option with an error explaining what required
>>   feature is not met and terminate the connection even though that reply
>>   has nothing to do with the requested option.
>
> There's no need to define this as "may respond to". The server replies
> can be structured, and we can define that the NBD_OPT_END must also be
> the last message sent by the server. If there is something going wrong,
> the server can send a message that signals the client does not implement
> a required option, with an error message in the payload.
>
> What the server sends to the client may be in response to what the
> client sends to the server, but it does not *have* to be.

That works too.

>> - 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.:#+]?

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

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

>> - New option NBD_OPT_AUTO_SYNC
>>   Run in sync mode until the first FUA/FLUSH command is used.
>
> Don't see the benefit, but sure, why not.

Paranoid mode. Make the data safe but allow faster operations if the
client is aware of the need to FUA/FLUSH.

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

>> - New options NBD_OPT_PRIVATE_1-255
>>   Client defined options, only valid after NBD_OPT_ENABLE_PRIVATE
>>   returned success. Used for experimental features between client and
>>   server of the same make.
>
> Right. Or for stable features between client and server that are
> specific to that particular implementation; doesn't necessarily have to
> be experimental.
>
>> Does that sound better?
>
> Sure.
>
> While we're at it: I'd also like to define the error codes a bit better.
> Currently, the error codes are defined as 'whatever errno happened to
> contain at the time the sever encountered an error'. Unfortunately,
> that's not very well standardized; i.e., there's a difference in errno
> not only from one OS to another, but also from one architecture to
> another.
>
> So instead, it might be better to have nbd-specific error codes, so that
> the client might be able to fix up a request in case of some error codes
> (like "oversized request", or similar, which the client would be able to
> handle in theory). These could also be used in the option haggling
> phase.

+1.

MfG
        Goswin