On 3/8/19 6:52 AM, Richard W.M. Jones wrote:
None are supported at present, so this always returns an empty list.
We have to start somewhere :) Makes no difference to the clients, other than the fact that we still negotiate with qemu is a good sign that the handshake is correct.
+++ b/server/connections.c @@ -926,6 +926,90 @@ _negotiate_handshake_newstyle_options (struct connection *conn) conn->structured_replies = true; break;
+ case NBD_OPT_LIST_META_CONTEXT: + case NBD_OPT_SET_META_CONTEXT: + { + uint32_t opt_index; + uint32_t exportnamelen; + uint32_t nr_queries; + uint32_t querylen; + const char *what; + + optname = name_of_nbd_opt (option); + if (conn_recv_full (conn, data, optlen, "read: %s: %m", optname) == -1) + return -1; + + if (!conn->structured_replies) { + if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_INVALID) + == -1) + return -1; + continue; + } + + /* Minimum length of the option payload is the 32 bit export + * name plus a zero length export name plus 32 bit number of
s/name plus/name length plus/
+ * queries followed by no queries. + */ + what = "optlen < 8"; + if (optlen < 8) { + opt_meta_invalid_option_len: + debug ("newstyle negotiation: %s: invalid option length: %s", + optname, what); + + if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_INVALID) + == -1) + return -1; + continue; + } + + /* Discard the export name. */ + memcpy (&exportnamelen, &data[0], 4); + exportnamelen = be32toh (exportnamelen); + opt_index = 4 + exportnamelen;
Hmm - the NBD protocol says that if the client requests a meta-context for export A, then changes its mind and does NBD_OPT_GO for export B, that the meta-context negotiated for A does not necessarily apply. For a server that cares about export names (such as qemu serving multiple exports on one server), this requires checking that the export finally selected matches the export involved in the context negotiation. But I think you are fine in nbdkit - our code already has multiple places where we are effectively stating that all possible client names are aliases to the same export; and thus an odd client requesting the context via one alias and then using a second alias for OPT_GO should not be surprised that the context is still served, despite the change in name between client requests (and even then, the spec says the client shouldn't try to use NBD_CMD_BLOCK_STATUS unless it had a successful SET with at least one context returned, which we don't have yet).
+ + /* Read the number of queries. */ + what = "reading number of queries"; + if (opt_index+4 > optlen) + goto opt_meta_invalid_option_len; + memcpy (&nr_queries, &data[opt_index], 4); + nr_queries = be32toh (nr_queries); + opt_index += 4; + + /* nr_queries == 0 means return all meta contexts. */
Well, it means return all contexts for LIST, but it means select no contexts (undoing any previous election) for SET.
+ if (nr_queries == 0) { + /* Nothing is supported now. */ + if (send_newstyle_option_reply (conn, option, NBD_REP_ACK) == -1) + return -1;
The if branch...
+ } + else { + /* Read and answer each query. */ + while (nr_queries > 0) { + what = "reading query string length"; + if (opt_index+4 > optlen) + goto opt_meta_invalid_option_len; + memcpy (&querylen, &data[opt_index], 4); + querylen = be32toh (querylen); + opt_index += 4; + what = "reading query string"; + if (opt_index + querylen > optlen) + goto opt_meta_invalid_option_len; + + debug ("newstyle negotiation: %s: %s %.*s", + optname, + option == NBD_OPT_LIST_META_CONTEXT ? "query" : "set", + (int) querylen, &data[opt_index]); + + /* Ignore query - nothing is supported. */ + + opt_index += querylen; + nr_queries--; + } + if (send_newstyle_option_reply (conn, option, NBD_REP_ACK) == -1) + return -1;
...and else branch post-loop are identical for now; we could share the code by dropping the if altogether (as the loop would skip when nr_queries == 0).
+ } + } + break; + default: /* Unknown option. */ if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_UNSUP) == -1)
As mentioned above, a compliant client should not be sending NBD_CMD_BLOCK_STATUS because we never successfully negotiated a context. Still, a non-compliant client might try NBD_CMD_BLOCK_STATUS; I don't see any code handling that, but assume we're okay with our default handling of an unrecognized NBD_CMD. And if I'm reading the NBD spec correctly (or rather, if I designed structured replies correctly, since I wrote that part of theh spec), NBD_CMD_READ is the only command required to return errors via structured replies; all other commands can still send simple errors even if they would send structured replies on success. A comment tweak is minor, and the code LGTM. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org