[Libguestfs] [PATCH nbdkit 3/8] server: Implement Block Status requests to read allocation status.

[Eric Blake]

On 3/20/19 5:11 PM, Richard W.M. Jones wrote:
> This commit implements the core NBD protocol for the "base:allocation"
> Block Status replies.
> ---
>  server/internal.h                    |   7 ++
>  server/protocol.h                    |  15 +++
>  server/protocol-handshake-newstyle.c |  81 ++++++++++++++-
>  server/protocol.c                    | 141 ++++++++++++++++++++++++---
>  4 files changed, 229 insertions(+), 15 deletions(-)
> 


> +static int
> +send_structured_reply_block_status (struct connection *conn,
> +                                    uint64_t handle,
> +                                    uint16_t cmd, uint16_t flags,
> +                                    uint32_t count, uint64_t offset,
> +                                    struct nbdkit_extents *extents)
> +{
> +  ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&conn->write_lock);
> +  struct structured_reply reply;
> +  size_t nr_extents = nbdkit_extents_size (extents);
> +  uint32_t context_id;
> +  size_t i;
> +  int r;
> +
> +  assert (cmd == NBD_CMD_BLOCK_STATUS);
> +

Worth asserting conn->meta_context_base_allocation?


> +
> +  /* Send each block descriptor. */
> +  for (i = 0; i < nr_extents; ++i) {

Where does the list terminate after 1 extent if REQ_ONE was set? Also,
if REQ_ONE is set but the plugin provided coalesced status beyond the
request, this would need to clamp the answer to the requested length.

> +    const struct nbdkit_extent e = nbdkit_get_extent (extents, i);
> +    struct block_descriptor bd;
> +
> +    if (i == 0)
> +      assert (e.offset == offset);
> +
> +    bd.length = htobe32 (e.length);
> +    bd.status_flags = htobe32 (e.type & 3);
> +
> +    r = conn->send (conn, &bd, sizeof bd);
> +    if (r == -1) {
> +      nbdkit_error ("write reply: %s: %m", name_of_nbd_cmd (cmd));
> +      return connection_set_status (conn, -1);
> +    }
> +  }

Where does the list terminate once you send the final extent that
overlaps the end of the original request? (If you implement my idea of
tracking a maximum offset in nbdkit_extents in patch 1, this may be
picked up automatically)



> @@ -498,15 +605,23 @@ protocol_recv_request_send_reply (struct connection *conn)
>    }
>  
>    /* Currently we prefer to send simple replies for everything except
> -   * where we have to (ie. NBD_CMD_READ when structured_replies have
> -   * been negotiated).  However this prevents us from sending
> -   * human-readable error messages to the client, so we should
> -   * reconsider this in future.
> +   * where we have to (ie. NBD_CMD_READ and NBD_CMD_BLOCK_STATUS when
> +   * structured_replies have been negotiated).  However this prevents
> +   * us from sending human-readable error messages to the client, so
> +   * we should reconsider this in future.
>     */
> -  if (conn->structured_replies && cmd == NBD_CMD_READ) {
> -    if (!error)
> -      return send_structured_reply_read (conn, request.handle, cmd,
> -                                         buf, count, offset);
> +  if (conn->structured_replies &&
> +      (cmd == NBD_CMD_READ || cmd == NBD_CMD_BLOCK_STATUS)) {
> +    if (!error) {
> +      if (cmd == NBD_CMD_READ)
> +        return send_structured_reply_read (conn, request.handle, cmd,
> +                                           buf, count, offset);
> +      else /* NBD_CMD_BLOCK_STATUS */
> +        return send_structured_reply_block_status (conn, request.handle,
> +                                                   cmd, flags,
> +                                                   count, offset,
> +                                                   extents);
> +    }
>      else
>        return send_structured_reply_error (conn, request.handle, cmd, error);

Ah, so you are already sending a structured error, even though the
protocol didn't require it (which is the qemu bug I just sent a patch
for today). Technically, the protocol requires a structured error for
NBD_CMD_READ, but permits a simple error for NBD_CMD_BLOCK_STATUS; but
portability wise, since qemu 2.12 through 3.1 choke on the simple error,
this works around those versions of qemu.

-- 
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/20190323/177ffc04/attachment.sig>