Re: [Nbd] [PATCHv3] docs/proto.md: Clarify SHOULD / MUST / MAY etc

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

Hi Alex,

On Wed, Apr 06, 2016 at 08:46:55PM +0100, Alex Bligh wrote:
[...]
>  server SHOULD refuse to allow oldstyle negotiations on the newstyle
>  port. For debugging purposes, the server MAY change the port on which to
> -listen for newstyle negotiation, but this should not happen for
> +listen for newstyle negotiation, but this SHOULD NOT happen for
>  production purposes.

Not sure if that was meant as a normative should not, but it doesn't do
much harm, I suppose.

>  The initial few exchanges in newstyle negotiation look as follows:
> @@ -135,7 +141,7 @@ S: 16 bits, transmission flags
>  S: 124 bytes, zeroes (reserved) (unless `NBD_FLAG_C_NO_ZEROES` was
>     negotiated by the client)  
>  
> -If the server is unwilling to allow the export, it should close the
> +If the server is unwilling to allow the export, it SHOULD close the
>  connection.
>  
>  The reason that the flags field is 16 bits large and not 32 as in the
> @@ -160,9 +166,9 @@ To fix these two issues, the following changes were implemented:
>  
>  - The server will set the handshake flag `NBD_FLAG_FIXED_NEWSTYLE`, to
>    signal that it supports fixed newstyle negotiation.
> -- The client should reply with `NBD_FLAG_C_FIXED_NEWSTYLE` set in its flags
> +- The client SHOULD reply with `NBD_FLAG_C_FIXED_NEWSTYLE` set in its flags
>    field too, though its side of the protocol does not change incompatibly.
> -- The client may now send other options to the server as appropriate, in
> +- The client MAY now send other options to the server as appropriate, in
>    the generic format for sending an option as described above.
>  - The server MUST NOT send a response to `NBD_OPT_EXPORT_NAME` until all
>    other pending option requests have had their final reply.
> @@ -174,7 +180,7 @@ S: 32 bits, the option as sent by the client to which this is a reply
>  S: 32 bits, reply type (e.g., `NBD_REP_ACK` for successful completion,
>     or `NBD_REP_ERR_UNSUP` to mark use of an option not known by this
>     server  
> -S: 32 bits, length of the reply. This may be zero for some replies, in
> +S: 32 bits, length of the reply. This MAY be zero for some replies, in
>     which case the next field is not sent  

Same here (but ah well)

>  S: any data as required by the reply (e.g., an export name in the case
>     of `NBD_REP_SERVER`)  
> @@ -294,7 +300,7 @@ order.
>  This field of 16 bits is sent by the server after the `INIT_PASSWD` and
>  the first magic number.
>  
> -- bit 0, `NBD_FLAG_FIXED_NEWSTYLE`; should be set by servers that
> +- bit 0, `NBD_FLAG_FIXED_NEWSTYLE`; MUST be set by servers that
>    support the fixed newstyle protocol

Right, good catch.

>  - bit 1, `NBD_FLAG_NO_ZEROES`; if set, and if the client replies with
>    `NBD_FLAG_C_NO_ZEROES` in the client flags field, the server MUST NOT
> @@ -309,16 +315,16 @@ NOT set any of these flags during oldstyle negotiation.
>  This field of 16 bits is sent by the server after option haggling, or
>  immediately after the handshake flags field in oldstyle negotiation:
>  
> -- bit 0, `NBD_FLAG_HAS_FLAGS`; should always be 1
> -- bit 1, `NBD_FLAG_READ_ONLY`; should be set to 1 if the export is
> +- bit 0, `NBD_FLAG_HAS_FLAGS`; MUST always be 1

Right.

> +- bit 1, `NBD_FLAG_READ_ONLY`; MUST be set to 1 if the export is
>    read-only

NAK. A server may not be able to detect that an export is read-only
(e.g., because the backing file is stored on a read-only filesystem, and
it only detects that upon the first write). In that case, it would
announce the export as read-write, but any write would result in EPERM
due to the backing store being read-only.

[...]
>  ## Experimental extensions
>  
> @@ -752,19 +758,19 @@ Therefore these commands share common documentation.
>        server has multiple alternate names for a single export, or a
>        default export was specified.
>  
> -    The server MUST NOT fail an NDB_OPT_GO sent with the same parameters
> -    as a previous NBD_OPT_INFO which returned successfully (i.e. with
> +    The server MUST NOT fail an `NDB_OPT_GO` sent with the same parameters
> +    as a previous `NBD_OPT_INFO` which returned successfully (i.e. with
>      `NBD_REP_SERVER`) unless in the intervening time the client has
>      negotiated other options. The server MUST return the same transmission
> -    flags with NDB_OPT_GO as a previous NDB_OPT_INFO unless in the
> +    flags with `NDB_OPT_GO` as a previous `NDB_OPT_INFO` unless in the

These are typographical in a normative patch ;-)

>      intervening time the client has negotiated other options.
>      The values of the transmission flags MAY differ from what was sent
>      earlier in response to an earlier `NBD_OPT_INFO` (if any), and/or
>      the server MAY fail the request, based on other options that were
>      negotiated in the meantime.
>  
> -    For backwards compatibility, clients should be prepared to also
> -    handle `NBD_REP_ERR_UNSUP`. In this case, they should fall back to
> +    For backwards compatibility, clients MUST be prepared to also
> +    handle `NBD_REP_ERR_UNSUP`. In this case, they MUST fall back to
>      using `NBD_OPT_EXPORT_NAME`.

Here you make UNSUP be a MUST, but earlier you made it a SHOULD. I think
SHOULD is right (at some point, servers that don't support a given
option are just too old, and a client would be within its right to
reject it), but at any rate we should be consistent.

>      The reply to an `NBD_OPT_GO` is identical to the reply to `NBD_OPT_INFO`
> @@ -798,7 +804,7 @@ command flag.
>  
>  * `NBD_CMD_WRITE_ZEROES`
>  
> -    A write request with no payload. Length and offset define the location
> +    A write request with no payload. *Offset* and *Length* define the location

Length doesn't need a capital start anymore if you swap it with Offset.

[...]

No further comments.

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