Re: [PATCH] doc: Improvements to block-status
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.
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 context
The `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 request
This 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.
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?
Other than that, LGTM.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Reply to: