[Libguestfs] [PATCH nbdkit v2 3/7] server: protocol: Send reply to NBD_INFO_BLOCK_SIZE if requested

[Eric Blake]

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