[Libguestfs] [PATCH nbdkit] server: Implement minimal implementation of set/list metadata contexts.

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190308/997b5665/attachment.sig>