Eric,
(comments taken unless otherwise detailed).
>> +
>> +If either the client or the server detects a violation of a
>> +mandatory condition ('MUST' etc.) by the other party, it MAY
>> +initiate a hard disconnect.
>> +
>> +A client MAY use a soft disconnect to terminate the session
>> +whenever it wishes.
>> +
>> +A party that is mandated by this document to terminate the
>> +session MUST initiate a hard disconnect if it is not possible
>> +to use a soft disconnect. Such circumstances include: where
>> +that party is the server and it cannot return an error
>> +(e.g. after an `NBD_OPT_EXPORT_NAME` it cannot satisfy),
>> +and where that party is the client following a failed TLS
>> +negotiation.
>> +
>> +A party MUST NOT initiate a hard disconnect save where set out
>> +in this section. Therefore, unless a client's situation falls
>> +within the provisions of the previous paragraph, it MUST NOT
>> +use a hard disconnect, and hence its only option to terminate
>> +the session is via a soft disconnect.
>
> Inconsistency between "set out in this section" (which includes hard
> disconnect permitted by a violation of a MUST) and "falls within the
> provisions of the previous paragraph" (which does not). I'm not sure
> how best to word-smith it, maybe "falls within other provisions of this
> section" ?
So the circumstance that is missing is the client may also
disconnect after breach of 'MUST'. I will wordsmith.
>
>> +
>> There is no requirement for the client or server to complete a
>> negotiation if it does not wish to do so. Either end MAY simply
>> -close the TCP connection (though see below regarding prior use
>> -of NBD_OPT_ABORT). Under certain circumstances either
>> -the client or the server may be required by this document to close
>> -the TCP connection. In each case, this is referred to as 'terminate
>> -the session'.
>> -
>> -If the client wishes to terminate the session in the negotiation
>> -phase, and is not doing so because it is required to do so
>> -by this document, it SHOULD send NBD_OPT_ABORT first if the protocol
>> -permits. There are instances where this is impossible, such as after
>> -an NBD_OPT_EXPORTNAME has been issued, or on an unsuccessful
>> -negotiation of TLS. For instance, if the client does not find an
>> -export it is looking for, it MAY simply send an NBD_OPT_ABORT
>> -and close the TCP connection.
>> +terminate the session. In the client's case, if it wishes to
>> +do so it MUST use soft disconnect.
>
> And of course, if the client violates this, the server MAY use hard
> disconnect - except the disconnection has already happened ;) Should we
> use SHOULD here? Using MUST means I'll have to patch qemu (but that's
> probably a good thing).
I think it has to be a 'MUST' for else a hard disconnect is being
permitted, which is inconsistent.
>> +
>> +In the server's case it MUST (save where set out above) simply
>> +error inbound options until the client gets the hint that it is
>> +unwelcome, except that if a server believes a client's behaviour
>> +constitutes a denial of service, it MAY initiate a hard disconnect.
>> +If the server is in the process of being shut down it MAY
>> +error inflight options and SHOULD error further options received
>> +(other than an `NBD_OPT_ABORT`) with `NBD_REP_ERR_SHUTDOWN`.
>> +
>> +If the client receives `NBD_REP_ERR_SHUTDOWN` it MUST initiate
>> +a soft disconnect.
>
> And an older client that doesn't understand this particular error, but
> keeps on talking anyway, forms a protocol violation where the server can
> now within its rights do a hard termination :)
Indeed!
>> - defined by the experimental `INFO` extension; see below.
>> + Defined by the experimental `INFO` extension; see below.
>
> Unrelated cleanup, if you want to squash this one in on an earlier
> typo-fix commit ;)
Too late, and sshhh ... :-)
>> +
>> +* `NBD_REP_ERR_SHUTDOWN` (2^32 + 7)
>> +
>> + The server is unwilling to continue negotiation as it has been
>> + shut down.
>
> s/has been/is in the process of being/ ?
Indeed. And under ESHUTDOWN.
>> @@ -960,6 +1057,7 @@ The following error values are defined:
>> * `ENOSPC` (28), No space left on device.
>> * `EOVERFLOW` (75), Value too large; SHOULD NOT be sent outside of the
>> experimental `STRUCTURED_REPLY` extension; see below.
>> +* `ESHUTDOWN` (108), Server has been shut down.
>>
>> The server SHOULD return `ENOSPC` if it receives a write request
>> including one or more sectors beyond the size of the device. It SHOULD
>> @@ -1124,7 +1222,7 @@ command flag.
>> insufficient space.
>>
>> If an error occurs, the server SHOULD set the appropriate error code
>> - in the error field. The server MAY then close the connection.
>> + in the error field. The server MAY then initiate a hard disconnect.
>
> Wow - in light of the rest of this patch, this sentence is a huge
> loophole. It states that the server can reply with ESHUTDOWN (or any
> other error), then immediately choose to do a hard disconnect without
> waiting for the client to react to the ESHUTDOWN with a soft disconnect.
> I'm wondering if we need to tone down the wording here, or at least add
> something to the effect of the server should first wait for further
> client action before initiating a hard disconnect after sending an error.
This is simply a mistake. It MUST set the error, and it disconnecting
is not permitted.
I was simply searching for 'close the connection' and replacing it
with new terminology. I didn't actually check that.
Whoever wrote that copied it unwittingly from NBD_CMD_WRITE, which
has the same provision. That is I suspect so the server doesn't
have to eat the data, but doing a hard disconnect because of
a write error is unreasonable, and eating data is cheap, so I shall
fix that one too.
Alex
>
> Overall looking nicer than v1 (always a good sign when we make progress)
>
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
--
Alex Bligh
Attachment:
signature.asc
Description: Message signed with OpenPGP using GPGMail