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 10:28:38AM +0200, Goswin von Brederlow wrote:
>> 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
>
> [...]

So NBD_OPT_END would allways have a reply. That would work in this case
too.

But what if you have conflicting options A | B? The client first tries
to set option A. If the server doesn't know that it wants to fall back
to B. But it doesn't want option B unless A fails.

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

The new style calls for them to be the full path, so examples would be:

/dev/sda1
/dev/vg/lv
/dev/disk/by-id/ata-SAMSUNG_HD154UI_S1Y6J1LS606898
/dev/disk/by-path/pci-0000:00:0b.0-scsi-0:0:0:0
/dev/disk/by-uuid/384ab672-f643-4200-892f-ee5bb021581a
/srv/nbd/test.img
/home/mrvn/nbd-test/bäd-näme

I initialy suggested 0 terminated because then every valid filename is a
valid export name. But I won't cry if the last example doesn't work. I
thing the others are essential though. So letters, numbers and "-/:_."

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

I ment the opposite. :)

yes - allways report the export with sync flag set, no other mode possible
maybe - NBD_OPT_SYNC succeeds
no - NBD_OPT_SYNC fails

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

I think when some feature becomes important enough that multiple
implementations want to support it it should get an official number and
documentation. I wouldn't expect backward compatibility for experimental
options and such so other clients copying those options would be risky.

MfG
        Goswin