Richard W.M. Jones
2022-Feb-17 14:36 UTC
[Libguestfs] [PATCH nbdkit v2 3/7] server: protocol: Send reply to NBD_INFO_BLOCK_SIZE if requested
If the client requests NBD_INFO_BLOCK_SIZE, _and_ either the plugin or
a filter provides this information, then send a reply. Otherwise
ignore the client request.
---
server/protocol-handshake-newstyle.c | 69 ++++++++++++++++++++++++++--
TODO | 3 +-
2 files changed, 66 insertions(+), 6 deletions(-)
diff --git a/server/protocol-handshake-newstyle.c
b/server/protocol-handshake-newstyle.c
index 5c073af6..f3cba48f 100644
--- a/server/protocol-handshake-newstyle.c
+++ b/server/protocol-handshake-newstyle.c
@@ -195,6 +195,38 @@ send_newstyle_option_reply_info_str (uint32_t option,
uint32_t reply,
return 0;
}
+/* Send reply containing NBD_INFO_BLOCK_SIZE. */
+static int
+send_newstyle_option_reply_info_block_size (uint32_t option, uint32_t reply,
+ uint16_t info,
+ uint32_t minimum,
+ uint32_t preferred,
+ uint32_t maximum)
+{
+ GET_CONN;
+ struct nbd_fixed_new_option_reply fixed_new_option_reply;
+ struct nbd_fixed_new_option_reply_info_block_size block_size;
+
+ fixed_new_option_reply.magic = htobe64 (NBD_REP_MAGIC);
+ fixed_new_option_reply.option = htobe32 (option);
+ fixed_new_option_reply.reply = htobe32 (reply);
+ fixed_new_option_reply.replylen = htobe32 (14);
+ block_size.info = htobe16 (info);
+ block_size.minimum = htobe32 (minimum);
+ block_size.preferred = htobe32 (preferred);
+ block_size.maximum = htobe32 (maximum);
+
+ if (conn->send (&fixed_new_option_reply,
+ sizeof fixed_new_option_reply, SEND_MORE) == -1 ||
+ conn->send (&block_size,
+ sizeof block_size, 0) == -1) {
+ nbdkit_error ("write: %s: %m", name_of_nbd_opt (option));
+ return -1;
+ }
+
+ return 0;
+}
+
static int
send_newstyle_option_reply_meta_context (uint32_t option, uint32_t reply,
uint32_t context_id,
@@ -589,10 +621,10 @@ negotiate_handshake_newstyle_options (void)
exportsize) == -1)
return -1;
- /* For now we send NBD_INFO_NAME and NBD_INFO_DESCRIPTION if
- * requested, and ignore all other info requests (including
- * NBD_INFO_EXPORT if it was requested, because we replied
- * already above).
+ /* For now we send NBD_INFO_NAME, NBD_INFO_DESCRIPTION and
+ * NBD_INFO_BLOCK_SIZE if requested, and ignore all other info
+ * requests (including NBD_INFO_EXPORT if it was requested,
+ * because we replied already above).
*/
for (i = 0; i < nrinfos; ++i) {
memcpy (&info, &data[4 + exportnamelen + 2 + i*2], 2);
@@ -637,6 +669,35 @@ negotiate_handshake_newstyle_options (void)
return -1;
}
break;
+ case NBD_INFO_BLOCK_SIZE:
+ {
+ uint32_t minimum, preferred, maximum;
+ int r = backend_block_size (conn->top_context,
+ &minimum, &preferred,
&maximum);
+
+ if (r == -1) {
+ debug ("newstyle negotiation: %s: "
+ "NBD_INFO_BLOCK_SIZE: error from plugin, "
+ "ignoring client request",
+ optname);
+ break;
+ }
+ if (minimum == 0) {
+ debug ("newstyle negotiation: %s: "
+ "NBD_INFO_BLOCK_SIZE: client requested but "
+ "no plugin or filter provided block size
information, "
+ "ignoring client request",
+ optname);
+ break;
+ }
+ if (send_newstyle_option_reply_info_block_size
+ (option,
+ NBD_REP_INFO,
+ NBD_INFO_BLOCK_SIZE,
+ minimum, preferred, maximum) == -1)
+ return -1;
+ }
+ break;
default:
debug ("newstyle negotiation: %s: "
"ignoring NBD_INFO_* request %u (%s)",
diff --git a/TODO b/TODO
index a4a0e0e7..35e68d15 100644
--- a/TODO
+++ b/TODO
@@ -31,8 +31,7 @@ General ideas for improvements
https://www.redhat.com/archives/libguestfs/2018-January/msg00149.html
* More NBD protocol features. The currently missing features are
- structured replies for sparse reads, block size constraints, and
- online resize.
+ structured replies for sparse reads, and online resize.
* Add a callback to let plugins request minimum alignment for the
buffer to pread/pwrite; useful for a plugin utilizing O_DIRECT or
--
2.35.1
Eric Blake
2022-Feb-17 16:39 UTC
[Libguestfs] [PATCH nbdkit v2 3/7] server: protocol: Send reply to NBD_INFO_BLOCK_SIZE if requested
On Thu, Feb 17, 2022 at 02:36:44PM +0000, Richard W.M. Jones wrote:> If the client requests NBD_INFO_BLOCK_SIZE, _and_ either the plugin or > a filter provides this information, then send a reply. Otherwise > ignore the client request. > --- > server/protocol-handshake-newstyle.c | 69 ++++++++++++++++++++++++++-- > TODO | 3 +- > 2 files changed, 66 insertions(+), 6 deletions(-)It may be worth advertising the size solely based on whether the plugin provided .block_size with non-zero values, even if the client didn't request it, to more fully mirror what qemu-nbd does (but that's where the 'bool strict' parameter might come in handy, as qemu-nbd advertises different numbers based on whether the client requested or not). But as I mentioned in 1/7, that's something I can play with in my followup series, so we can see whether we like it.> +++ b/server/protocol-handshake-newstyle.c > @@ -195,6 +195,38 @@ send_newstyle_option_reply_info_str (uint32_t option, uint32_t reply, > return 0; > } > > +/* Send reply containing NBD_INFO_BLOCK_SIZE. */ > +static int > +send_newstyle_option_reply_info_block_size (uint32_t option, uint32_t reply, > + uint16_t info, > + uint32_t minimum, > + uint32_t preferred, > + uint32_t maximum)At any rate, this helper function is good, regardless of whether we only call it on client request, or whether we call it as often as the plugin gives us values to advertise.> +{ > + GET_CONN; > + struct nbd_fixed_new_option_reply fixed_new_option_reply; > + struct nbd_fixed_new_option_reply_info_block_size block_size; > + > + fixed_new_option_reply.magic = htobe64 (NBD_REP_MAGIC); > + fixed_new_option_reply.option = htobe32 (option); > + fixed_new_option_reply.reply = htobe32 (reply); > + fixed_new_option_reply.replylen = htobe32 (14);Worth a sizeof computation instead of a magic number here, to explain why we got at 14?> @@ -637,6 +669,35 @@ negotiate_handshake_newstyle_options (void) > return -1; > } > break; > + case NBD_INFO_BLOCK_SIZE: > + { > + uint32_t minimum, preferred, maximum; > + int r = backend_block_size (conn->top_context, > + &minimum, &preferred, &maximum); > + > + if (r == -1) { > + debug ("newstyle negotiation: %s: " > + "NBD_INFO_BLOCK_SIZE: error from plugin, " > + "ignoring client request", > + optname); > + break;Should this be 'return -1', so that the plugin isn't forced to continue serving later requests after reporting a failure at determining block sizes it wants to use? But I can also see your argument that if we trust the plugin to be robust, our communication with the client is still in sync so it isn't a fatal error...> + } > + if (minimum == 0) { > + debug ("newstyle negotiation: %s: " > + "NBD_INFO_BLOCK_SIZE: client requested but " > + "no plugin or filter provided block size information, " > + "ignoring client request", > + optname); > + break;This break makes sense.> + } > + if (send_newstyle_option_reply_info_block_size > + (option, > + NBD_REP_INFO, > + NBD_INFO_BLOCK_SIZE, > + minimum, preferred, maximum) == -1) > + return -1; > + } > + break;...and this return -1 is absolutely right as it indicates we are out of sync communicating with the client. So despite my ramblings, I think I ended up with: ACK whether or not you s/14/sizeof(...)/ -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org