[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

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



>> 
>> -- 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
>> +- bit 1, `NBD_FLAG_READ_ONLY`; MUST be set to 1 if the export is
>>   read-only
>> -- bit 2, `NBD_FLAG_SEND_FLUSH`; should be set to 1 if the server
>> +- bit 2, `NBD_FLAG_SEND_FLUSH`; MUST be set to 1 if the server
>>   supports `NBD_CMD_FLUSH` commands
> 
> A server may be capable of parsing NBD_CMD_FLUSH, but have a per-export
> policy on whether flushes are acceptable.  In fact, it may be entirely
> reasonable for a server that sets NBD_FLAG_READ_ONLY to intentionally
> leave NBD_FLAG_SEND_FLUSH clear, even though the same server accepts
> flush on normal connections.  I think this one should stay at SHOULD.

The problem here is that they are neither MUST or SHOULD in RFC2119
sense. What you are saying is that the server MAY set it them to 1
if it wishes to support NBD_CMD_FLUSH commands. The effect in
terms of normative behaviour is (a) on the client (i.e. the client may
only use the command if the bit is set), and (b) on the server in the
sense of the server MUST NOT set the bit if it doesn't support
NBD_CMD_FLUSH (which it doesn't currently say - i.e. currently
the normative behaviour applies only if it does support
NBD_CMD_FLUSH). Clearly these need a rephrase.

> 
>> -- bit 3, `NBD_FLAG_SEND_FUA`; should be set to 1 if the server supports
>> +- bit 3, `NBD_FLAG_SEND_FUA`; MUST be set to 1 if the server supports
>>   the `NBD_CMD_FLAG_FUA` flag
> 
> Again, FUA is (currently) documented to only have an impact on reads, so
> I argue that SHOULD allows a server that intentionally clears this bit
> when NBD_FLAG_READ_ONLY is set.

Ditto

>> -- bit 4, `NBD_FLAG_ROTATIONAL`; should be set to 1 to let the client
>> +- bit 4, `NBD_FLAG_ROTATIONAL`; MUST be set to 1 to let the client
>>   schedule I/O accesses as for a rotational medium
> 
> The wording sounds like the server always has to set this.  Maybe:
> 
> MUST be set to 1 if the server wants to inform the client to schedule
> I/O accesses as for a rotational medium.

Ditto - 'MUST if' doesn't work. It's really 'MAY ... in order to'

> 
>> -- bit 5, `NBD_FLAG_SEND_TRIM`; should be set to 1 if the server supports
>> +- bit 5, `NBD_FLAG_SEND_TRIM`; MUST be set to 1 if the server supports
>>   `NBD_CMD_TRIM` commands
> 
> Again, SHOULD may be better since a read-only export can't trim.

Ditto

>> +    the server is unwilling to perform TLS, it SHOULD reply with
>> +    `NBD_REP_ERR_POLICY`. For backwards compatibility, a client SHOULD
>>     also be prepared to handle `NBD_REP_ERR_UNSUP`. If the client sent
> 
> see below at [1]
> 
>> @@ -670,7 +676,7 @@ documented as applicable to the given request.
>> Which error to return in any other case is not specified by the NBD
>> protocol.
>> 
>> -The server SHOULD AVOID returning ENOMEM if at all possible.
>> +The server SHOULD avoid returning `ENOMEM` if at all possible.
> 
> The server SHOULD NOT return `ENOMEM` if at all possible.

+1

>> -    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`.
> 
> [1] - why is this MUST, when the NBD_OPT_STARTTLS uses SHOULD?

I agree they should be consistent. I think MUST is reasonable / better
as else you are saying 'you can write a non-back compatible client
and still be compliant).

> Arguably, a client that gets NBD_REP_ERR_UNSUP on NBD_OPT_GO can
> disconnect rather than falling back to NBD_OPT_EXPORT_NAME, if it
> refuses to talk to old servers for whatever reason.
> 
>> 
>> @@ -1008,7 +1014,7 @@ error, and alters the reply to the `NBD_CMD_READ` request.
>>       server MUST use structured replies to the `NBD_CMD_READ`
>>       transmission request.  Other extensions that require structured
>>       replies may now be negotiated.
>> -    - For backwards compatibility, clients should be prepared to also
>> +    - For backwards compatibility, clients MUST be prepared to also
>>       handle `NBD_REP_ERR_UNSUP`; in this case, no structured replies
>>       will be sent.
> 
> Again, is SHOULD better than MUST here?

No I think clients should be required to be back compatible. There is
nothing however preventing them terminating the connection.

--
Alex Bligh




Attachment: signature.asc
Description: Message signed with OpenPGP using GPGMail


Reply to: