20.10.2017 18:07, Eric Blake wrote:
On 10/20/2017 03:34 AM, Vladimir Sementsov-Ogievskiy wrote:
20.10.2017 01:26, Eric Blake wrote:
Consolidate the check for a zero-length payload to an option
into a new function, nbd_check_zero_length(); this check will
also be used when introducing support for structured replies.
By sticking a catch-all check at the end of the loop for
processing options, we can simplify several of the intermediate
cases.
Signed-off-by: Eric Blake <ebl...@redhat.com>
looks like two patches in one, however I'm not against (considering my
big patches =)).
I've already put an r-b here but suddenly understood a hidden behavior
change you've made,
which may considered like a bug, see below.
+/* nbd_check_zero_length: Handle any unexpected payload.
+ * Return:
+ * -errno on error, errp is set
+ * 0 on successful negotiation, errp is not set
+ */
+static int nbd_check_zero_length(NBDClient *client, uint32_t length,
+ uint32_t option, Error **errp)
+{
+ if (!length) {
+ return 0;
+ }
+ if (nbd_drop(client->ioc, length, errp) < 0) {
+ return -EIO;
+ }
+ return nbd_negotiate_send_rep_err(client->ioc,
NBD_REP_ERR_INVALID, option,
+ errp, "option %s should have
zero length",
may be quotes around %s or your trace-notation %d (%s) would be more
readable
quotes don't hurt, but since none of the option names contain spaces,
two of them: "export name" and "structured reply"
it's not quite as important as when you are quoting a message sent over
the wire.
+ nbd_opt_lookup(option));
+}
+
/* nbd_negotiate_options
* Process all NBD_OPT_* client option commands, during fixed newstyle
* negotiation.
@@ -674,7 +672,11 @@ static int nbd_negotiate_options(NBDClient
*client, uint16_t myflags,
}
switch (option) {
case NBD_OPT_STARTTLS:
- tioc = nbd_negotiate_handle_starttls(client, length,
errp);
+ ret = nbd_check_zero_length(client, length, option,
errp);
+ if (ret < 0) {
+ return ret;
+ }
no, you should not continue if length>0 (old behavior).
nbd_negotiate_send_rep_err returns 0 on success
in nbd_check_zero_length().
Oh, good catch. But it's subtler than that. In the old code,
nbd_negotiate_handle_starttls() returns NULL on non-zero length (even if
it sent a message to the client), because we really want to kill the
connection if a client can't turn on TLS correctly...
@@ -712,9 +711,9 @@ static int nbd_negotiate_options(NBDClient
*client, uint16_t myflags,
} else if (fixedNewstyle) {
switch (option) {
case NBD_OPT_LIST:
- ret = nbd_negotiate_handle_list(client, length, errp);
- if (ret < 0) {
- return ret;
+ ret = nbd_check_zero_length(client, length, option,
errp);
+ if (!ret) {
the same here
while nbd_negotiate_handle_list() used to return 0 if the client sent
non-zero length (we handled the incorrect message from the client just
fine, and can continue listening for more options).
Maybe I can fix it with a tri-state return: 1 if correct length, 0 if
nonzero length but error message sent successfully, and negative on
transmission failure; although then it's trickier for callers. I'll
have to think about it...
case NBD_OPT_STARTTLS:
- if (nbd_drop(client->ioc, length, errp) < 0) {
- return -EIO;
- }
- if (client->tlscreds) {
+ if (length) {
+ ret = nbd_check_zero_length(client, length,
option, errp);
Maybe explicitly checking for length at each caller is the simplest
approach for getting the decision logic correct, since I really wasn't
able to abstract a clean "failure to communicate" vs. "error message
sent, go on to next message or abort connection as appropriate" vs.
"everything validated, proceed with rest of handing current option".
--
Best regards,
Vladimir
