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

[Richard W.M. Jones]

On Thu, Feb 17, 2022 at 10:39:04AM -0600, Eric Blake wrote:
> > +{
> > +  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?

Actually I copied the number straight out of the NBD spec!

  NBD_INFO_BLOCK_SIZE (3)
  ...
  The length MUST be 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...

Unclear.  There's an argument that if the plugin returns an error then
we should stop the connection, but also an argument that this is just
an optional info message.  Maybe return -1 makes more sense?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org