From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36530) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1anYTS-0007um-2Z for qemu-devel@nongnu.org; Tue, 05 Apr 2016 17:25:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1anYTO-00078O-RY for qemu-devel@nongnu.org; Tue, 05 Apr 2016 17:25:22 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48167) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1anYTO-00078C-Je for qemu-devel@nongnu.org; Tue, 05 Apr 2016 17:25:18 -0400 References: <1459888946-27233-1-git-send-email-alex@alex.org.uk> From: Eric Blake Message-ID: <57042D3C.2040103@redhat.com> Date: Tue, 5 Apr 2016 15:25:16 -0600 MIME-Version: 1.0 In-Reply-To: <1459888946-27233-1-git-send-email-alex@alex.org.uk> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="f2MxSn44v2e3lapGaPKthwN6R28QsdsvC" Subject: Re: [Qemu-devel] [PATCHv2] Amend NBD_OPT_SELECT (now NBD_OPT_INFO) and NBD_OPT_GO documentation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Bligh , Wouter Verhelst Cc: "nbd-general@lists.sourceforge.net" , "qemu-devel@nongnu.org" This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --f2MxSn44v2e3lapGaPKthwN6R28QsdsvC Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 04/05/2016 02:42 PM, Alex Bligh wrote: > Amend the NBD_OPT_SELECT and NBD_OPT_GO documentation as > follows: >=20 > * Make the documentation much more concise. >=20 > Signed-off-by: Alex Bligh > --- > doc/proto.md | 142 ++++++++++++++++++++++++++++++++-------------------= -------- > 1 file changed, 76 insertions(+), 66 deletions(-) >=20 > Changes since v1: >=20 > * Change NBD_OPT_SELECT to be called NBD_OPT_INFO >=20 > * Remove the 'selection' aspect of that command, so that > it now merely returns information. This is to avoid > the server storing state. >=20 > * Reorder the fields of an NBD_OPT_INFO / NBD_OPT_GO reply > so the variable length elements are at the end. >=20 > * Make the documentation much more concise. >=20 > * All Erik Blake's minor modifications It's Eric, but you're not the first (and probably not the last) to use alternative spellings :) > @@ -417,7 +417,7 @@ during option haggling in the fixed newstyle negoti= ation. > encoded data suitable for direct display to a human being; with > no embedded or terminating NUL characters. > =20 > - The experimental `SELECT` extension (see below) is a client > + The experimental `INFO` extension (see below) is a client > request where the extra data has a definition other than a > UTF-8 message. Not your fault, but as long as we're touching this, maybe reword it: The experimental `INFO` extension (see below) adds two client option requests where the extra data has a definition other than a UTF-8 message= =2E > =20 > -To remedy this, a `SELECT` extension is envisioned. This extension add= s > +To remedy this, an `INFO` extension is envisioned. This extension adds= > two option requests and one error reply type, and extends one existing= > option reply type. > =20 > -* `NBD_OPT_SELECT` > +Both options have identical formats for requests and replies. The > +only difference is that after a successful reply to `NBD_OPT_GO` > +(i.e. an `NBD_REP_SERVER`), transmission mode is entered immediately. > +Therefore these commands share common documentation. > =20 > - The client wishes to select the export with the given name for use= > - in the transmission phase, but does not yet want to move to the > - transmission phase. > +* `NBD_OPT_INFO` and `NBD_OPT_GO` > =20 > - Data: > + `NBD_OPT_INFO`: The client wishes to get details about export with= the s/export/an export/ > + given name for use in the transmission phase, but does not yet wan= t > + to move to the transmission phase. Maybe a mention that "when successful, this option provides more details than `NBD_OPT_LIST`". > =20 > - - 32 bits, length of name (unsigned) > - - Name of the export > + `NBD_OPT_GO`: The client wishes to terminate the negotiation phase= and Not your fault, but we're inconsistent on "negotiation phase" vs. "handshake phase". I wouldn't worry about it here; we could do a global cleanup in a separate patch if it bugs someone enough. > + progress to the transmission phase. This client MAY issue this com= mand after > + an `NBD_OPT_INFO`, or MAY issue it without a previous `NBD_OPT_INF= O`. > + `NBD_OPT_GO` can thus be used as a substitute for `NBD_OPT_EXPORT_= NAME` > + that returns errors. The phrasing makes it sound like EXPORT_NAME returns errors. Better wording might be: `NBD_OPT_GO` can thus be used as an improved version of `NBD_OPT_EXPORT_NAME` that is capable of returning errors. > - - Servers MUST NOT send `NBD_REP_ERR_UNSUP` as a reply to this > - message if they do not also send it as a reply to the > - `NBD_OPT_SELECT` message. > + Additionally, if TLS has not been negotiated, the server MAY reply= > + with `NBD_REP_ERR_TLS_REQD` (instead of `NBD_REP_ERR_UNKNOWN`) > + to requests for exports that are unknown. This is so that clients > + that have not negotiated TLS cannot enumerate exports. > + > + In the case of `NBD_REP_SERVER`, the message's data takes on a dif= ferent > + interpretation than the default (so as to provide additional > + binary information normally sent in reply to `NBD_OPT_EXPORT_NAME`= , > + in place of the default UTF-8 free-form string). The option reply = length > + MUST be *length of name* + 14, and the option data has the followi= ng layout: > + > + - 64 bits, size of the export in bytes (unsigned) > + - 16 bits, transmission flags. > + - 32 bits, length of name (unsigned) Not nicely aligned to a natural C struct, but that's nothing new (nor anything to worry about). At least the 'size' and 'transmission' fields are in the same relative positions as they are for the reply to NBD_OPT_EXPORT_NAME, so that part of the client code can be reused. However: Down the road, I hope to add an extension that will also let us return minimum/preferred/maximum I/O sizing for an export; I anticipate that for NBD_OPT_EXPORT_NAME, it would be carved out of the tail of 124 reserved bytes of zeroes (and if NBD_FLAG_C_NO_ZEROES is negotiated, and if my information adds 12 bytes, then NBD_FLAG_C_NO_ZEROES changes meaning to only eliminate the remaining tail of 112 zeroes after the added information). To keep the server and client code shareable between NBD_OPT_EXPORT_NAME and NBD_REP_SERVER, while still adding the extra sizing information in the reply, we'd have to inject the extra information in between 'transmission flags' and 'length of name'. If new information is always stuck at the end of the option reply, then the relative offset between 'transmission flags' and 'sizing hints' is no longer constant. Another thing I don't like: By default, NBD_REP_SERVER puts the 'length of name' field first; I'm not sure if there's a strong reason to change that. Maybe this layout would be slightly nicer: - 32 bits, length of name - 64 bits, size of export - 16 bits, transmission flags - up to 124 bytes, variable based on other negotiated options (default of 0 bytes) - length of name bytes: name where the option reply must be at least (rather than exactly) '*length of name* + 14' bytes (and at most length of name + 138), and where the tail end *length of name* bytes form the export name (because I _do_ like your idea of putting variable-length names last), and any other intermediate bytes cover the variable information based on other negotiated options, such that those other negotiated options can also consume some of the 124 reserved zeroes at the end of NBD_EXPORT_NAME's reply. Yeah, it's a bit weird that 'length of name' is separated by a variable amount of data before 'name', but it then lets us make the 'NBD_OPT_EXPORT_NAME' reply be a strict subset of the 'NBD_REP_SERVER' layout, which makes for nicer code sharing on that front (but it's still not the same as the original NBD_REP_SERVER sticking all extra information after the variable-length name field). > + - Name of the export. This name MAY be different from the one > + given in the `NBD_OPT_INFO` or `NBD_OPT_GO` option in case the s= erver has Long line; want to reflow this paragraph? > + multiple alternate names for a single export, or a default > + export was specified. > + > + The server MUST NOT fail an NDB_OPT_GO sent with the same paramete= rs > + as a previous NBD_OPT_INFO which returned successfully (i.e. with > + `NBD_REP_SERVER`) unless in the intervening time the client has > + negotiated other options. Not quite the same wording as before, but I think it still nicely ensures that a server WON'T fail with NBD_REP_ERR_UNSUP on GO after succeeding on INFO. > The server MUST return the same transmission > + flags with NDB_OPT_GO as a previous NDB_OPT_INFO unless in the > + intervening time the client has negotiated other options. > + The values of the transmission flags MAY differ from what was sent= > + earlier in response to an earlier `NBD_OPT_INFO` (if any), and/or > + the server may fail the request, based on other options that were Do we want s/may/MAY/ here? > + negotiated in the meantime. > + > + For backwards compatibility, clients should be prepared to also > + handle `NBD_REP_ERR_UNSUP`. In this case, they should fall back to= > + using `NBD_OPT_EXPORT_NAME`. > + > + The reply to an `NBD_OPT_GO` is identical to the reply to `NBD_OPT= _INFO` > + save that if the reply indicates success (i.e. is `NBD_REP_SERVER`= ), > + the client and the server both immediatedly enter the transmission= s/immediatedly/immediately/ > + phase. The server MUST NOT send any zero padding bytes after the > + `NBD_REP_SERVER` data, whether or not the client negotiated the > + `NBD_FLAG_C_NO_ZEROES` flag. After sending this reply the server M= UST > + immediately move to the transmission phase, and after receiving th= is > + reply, the client MUST immediately move to the transmission phase;= > + therefore, the server MUST NOT send this particular reply until al= l > + other pending option requests have been sent by the server. s/requests/replies/ --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --f2MxSn44v2e3lapGaPKthwN6R28QsdsvC Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJXBC08AAoJEKeha0olJ0NqfNYH+QHRx8JX9AzLRoDjWRmFHnBy JxCW7Ysl4tL7SQWinm8YqMeB994AZNDC3c9Vy1a1SA2jSOXeW42ZlZ07Qh4D6BVS vQvuvxhXKO0q4EM5xjIGWWvDAOZHSHnsCz0XV2vWCOWCeqI2iYakH1DIOk9PfULB uyiTCSg5Y83FhT4Kk2yPF944muaeMmaWCldlT48+vBpA6IQeuE8QedUTtEG4NR0D ZLw0gtya/At9aKEykU3D3hXOp0hq1QX1vZsCU0XGj5zSUDvJgKvIWDZzT/yhNBbJ 7hqV0QxqXkaNuzbK/JT7/utVzhDJruUEsSYaV5ilpDyxhQP5gk5RDeDCjFwd99E= =MzlS -----END PGP SIGNATURE----- --f2MxSn44v2e3lapGaPKthwN6R28QsdsvC--