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
signature.asc
Description: Message signed with OpenPGP using GPGMail
------------------------------------------------------------------------------ Find and fix application performance issues faster with Applications Manager Applications Manager provides deep performance insights into multiple tiers of your business applications. It resolves application problems quickly and reduces your MTTR. Get your free trial! https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
_______________________________________________ Nbd-general mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/nbd-general
