15.03.2018 17:58, Eric Blake wrote:
On 03/15/2018 05:19 AM, Wouter Verhelst wrote:Hi Eric, On a general note, reviewing these kinds of changes is easier if you send separate patches for normative changes and for reflowing the text; otherwise I have to read every paragraph you touched twice, hunting for the changes you made. This is a waste of everyone's time ;-)Okay, I'll improve my submission flow along those lines.
So, let me review starting from v2 :)
On Wed, Mar 14, 2018 at 05:52:44PM -0500, Eric Blake wrote: [...]+The query string with no `[leaf-name]` is only valid within +`NBD_OPT_LIST_META_CONTEXT`. If a `[leaf-name]' requested by the +client is not recognized, the server MUST ignore it rather than +reporting an error.I think we discussed this point when we originally spec'd the BLOCK_STATUS extension, and decided at the time that we wanted to leave this up to the extension to decide. However, I do agree that it's probably a bad idea to have only a namespace for SET. Can you perhaps reword it so that this becomes a requirement for the base: namespace, and only a SHOULD (but not a MUST) for other namespaces?Yes, that makes sense; I'll send a v2 along those lines (including practicing the split between normative changes and reflowing).#### `base:allocation` metadata contextThe `base:allocation` metadata context is the basic "allocated at all"@@ -1261,11 +1268,6 @@ of the newstyle negotiation. Return a list of `NBD_REP_META_CONTEXT` replies, one per context, followed by an `NBD_REP_ACK`.- If the query string is syntactically invalid, the server SHOULD send- `NBD_REP_ERR_INVALID`. If the query string is syntactically valid - but finds no metadata contexts, the server MUST send a single - reply of type `NBD_REP_ACK`. -[...]+ If the query string is syntactically invalid (such as lacking the + colon that would delimit the namespace, or using a leaf name that + is invalid for a known namespace), the server MAY fail the requestThis turns a SHOULD into a MAY. Any specific reason for that?Yes. When the client requests 2 queries, but the server replies with an error, the client does not know which of the two queries failed (well, maybe the server was nice and replied with the matches it recognized before a final error packet in place of the usual NBD_REP_ACK - but we didn't require that of servers). This puts the burden on the client to then call two more SET commands, with one query each, to learn which of the two queries (if not both) that the server reported an error on. On the other hand, if the server replies with success, the client can easily read through the returned results to see which queries the server enabled, and assume that a query that was not enabled was due to either the server not supporting it or the client not asking for it correctly. We already document that the server can return an array of 0 contexts before the NBD_REP_ACK to mean that none of the client's requests were available; allowing the server to NOT fail an invalid request means this also works if it means none of the client's requests were syntactically valid.Furthermore, the example that came up earlier was the fact that namespace queries are spelled 'namespace:leaf'. It is thus syntactically invalid if you have 'barename' with no colon - however, since NBD allows strings up to 4k in length, a server that has to read all 4k just to determine whether 'x-really...long' had a colon in order to diagnose that the namespace was ill-formed. But if the server only plans on handling the 'base:' namespace, it knows after reading just 5 bytes whether the string belongs to something it can handle vs. something to be ignored. Implementation-wise, stack-allocating 5 bytes for a read is trivial; but stack-allocating 4k bytes for a read risks overflowing onto a guard page on systems with 4k stacks, so the server may have to switch to malloc()d storage for a read JUST for determining whether to issue an error.
Agree with this, furthermore, my latest implementation, PULL'ed for 2.12 follows this logic.
Thus, allowing a MAY fail means that a server that wants nice quality of implementation will validate that all queries at least fit the 'namespace:' pattern, even if namespace approaches 4k; but that a server that does not diagnose errors in namespaces it does not care about is not a problem. After all, the client can still learn which namespaces the server replied to, and decide from there if the server's reply met the client's needs.Meanwhile, a syntax error such as a length mismatch (where one of the queries includes a length that would require reading beyond the initial length of the overall NBD_OPT_ header) should still obviously be an NBD_REP_ERR_INVALID. Maybe that needs an explicit mention?
Good idea to mention it I think. But IMHO, it's actually not a syntax error, at least, not a syntax error of query itself..
Other than that, LGTM.
-- Best regards, Vladimir