Re: [Qemu-devel] [PATCH 11/18] nbd: BLOCK_STATUS for bitmap export: server part

[Eric Blake]

On 02/03/2017 09:47 AM, Vladimir Sementsov-Ogievskiy wrote:
> Only one meta context type is defined: qemu-bitmap:<bitmap-name>.

Why 'qemu-bitmap:' instead of 'qemu:' for our namespace?  I guess it's
okay, though.

> Maximum one query is allowed for NBD_OPT_{SET,LIST}_META_CONTEXT,
> NBD_REP_ERR_TOO_BIG is returned otherwise.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---

This is only a high-level review; I may come back and look more closely
at details (and compare against the proposed spec) when I have more time.

>  /* Request flags, sent from client to server during transmission phase */
>  #define NBD_CMD_FLAG_FUA        (1 << 0) /* 'force unit access' during write 
> */
> @@ -142,6 +155,7 @@ enum {
>      NBD_CMD_TRIM = 4,
>      /* 5 reserved for failed experiment NBD_CMD_CACHE */
>      NBD_CMD_WRITE_ZEROES = 6,
> +    NBD_CMD_BLOCK_STATUS = 7
>  };

Please keep the trailing comma (it makes it easier for later patches to
add new values without having to modify existing lines).

>  
>  #define NBD_DEFAULT_PORT     10809
> @@ -163,6 +177,7 @@ enum {
>  #define NBD_REPLY_TYPE_NONE 0
>  #define NBD_REPLY_TYPE_OFFSET_DATA 1
>  #define NBD_REPLY_TYPE_OFFSET_HOLE 2
> +#define NBD_REPLY_TYPE_BLOCK_STATUS 5
>  #define NBD_REPLY_TYPE_ERROR ((1 << 15) + 1)
>  #define NBD_REPLY_TYPE_ERROR_OFFSET ((1 << 15) + 2)

Might be worth formatting these so that the definitions all start in the
same column (if so, it affects an earlier patch in the series as well).

>  
> diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
> index 3284bfc85a..fbbcf69925 100644
> --- a/nbd/nbd-internal.h
> +++ b/nbd/nbd-internal.h
> @@ -83,6 +83,10 @@
>  #define NBD_OPT_PEEK_EXPORT     (4)
>  #define NBD_OPT_STARTTLS        (5)
>  #define NBD_OPT_STRUCTURED_REPLY (8)
> +#define NBD_OPT_LIST_META_CONTEXT (9)
> +#define NBD_OPT_SET_META_CONTEXT  (10)
> +
> +#define NBD_META_NS_BITMAPS "qemu-dirty-bitmap"

I still find it odd that we've split our #defines across two different
headers, not your fault.

This name does not match your commit message.  Why can't we use just
'qemu:' as our namespace?

>  
>  /* NBD errors are based on errno numbers, so there is a 1:1 mapping,
>   * but only a limited set of errno values is specified in the protocol.
> @@ -105,6 +109,8 @@ static inline const char *nbd_opt_name(int opt)
>      case NBD_OPT_PEEK_EXPORT: return "peek_export";
>      case NBD_OPT_STARTTLS: return "tls";
>      case NBD_OPT_STRUCTURED_REPLY: return "structured_reply";
> +    case NBD_OPT_LIST_META_CONTEXT: return "list_meta_context";
> +    case NBD_OPT_SET_META_CONTEXT: return "set_meta_context";

Same question as earlier as to whether this violates checkpatch
formatting, and whether it belongs in a .c instead of inline in the header.

> 
>  
> +static int nbd_negotiate_read_size_string(QIOChannel *ioc, char **str,
> +                                          uint32_t max_len)

I probably would have split the creation of this helper function into
its own patch.  Also, can it be utilized in any of the existing code, or
is the new code the only client?

> +
> +/* start handle LIST_META_CONTEXT and SET_META_CONTEXT requests
> + * @opt          should be NBD_OPT_LIST_META_CONTEXT or 
> NBD_OPT_SET_META_CONTEXT
> + * @length       related option data to read
> + * @nb_queries   out parameter, number of queries specified by client
> + * @bs           out parameter, bs for export, selected by client
> + *               will be zero if some not critical error occured and error 
> reply
> + *               was sent.

Wrong spelling of 'occurred', and awkward grammar.  How about:

will be zero if an error reply is successfully sent

> + *
> + * Returns:
> + *   Err. code < 0 on critical error

s/Err./Error/

> + *   Number of bytes read otherwise (will be equal to length on non critical
> + *     error or if there no queries in request)

will be equal to length if there were no errors, or no queries in the
request

> + */
> +static int nbd_negotiate_opt_meta_context_start(NBDClient *client, uint32_t 
> opt,
> +                                                uint32_t length,
> +                                                uint32_t *nb_queries,
> +                                                BlockDriverState **bs)
> +{
> +    int ret;
> +    NBDExport *exp;
> +    char *export_name;

Uninitialized...[1]

> +    int nb_read = 0;
> +
> +    if (!client->structured_reply) {
> +        uint32_t tail = length - nb_read;
> +        LOG("Structured reply is not negotiated");

This LOG() is redundant...[2]

> +
> +        if (nbd_negotiate_drop_sync(client->ioc, tail) != tail) {
> +            return -EIO;
> +        }
> +        ret = nbd_negotiate_send_rep_err(client->ioc, NBD_REP_ERR_INVALID, 
> opt,
> +                                         "Structured reply is not 
> negotiated");

[2]...because this function also calls LOG().

> +        g_free(export_name);

[1]...so this crashes.  Oops.

> +
> +        if (ret < 0) {
> +            return ret;
> +        } else {
> +            *bs = NULL;
> +            *nb_queries = 0;
> +            return length;
> +        }
> +    }
> +


> +static int nbd_negotiate_set_meta_context(NBDClient *client, uint32_t length)
> +{
> +    int ret;
> +    BlockDriverState *bs;
> +    uint32_t nb_queries;
> +    int nb_read;
> +
> +    nb_read = nbd_negotiate_opt_meta_context_start(client,
> +                                                   NBD_OPT_SET_META_CONTEXT,
> +                                                   length, &nb_queries, &bs);
> +    if (nb_read < 0) {
> +        return nb_read;
> +    }
> +    if (bs == NULL) {
> +        /* error reply was already sent by 
> nbd_negotiate_opt_meta_context_start
> +         * */
> +        return 0;
> +    }
> +
> +    if (nb_queries == 0) {
> +        return nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
> +                                      NBD_OPT_SET_META_CONTEXT);
> +    }
> +
> +    if (nb_queries > 1) {
> +        return nbd_negotiate_send_rep_err(client->ioc, NBD_REP_ERR_TOO_BIG,
> +                                          NBD_OPT_SET_META_CONTEXT,
> +                                          "Only one exporting context is"
> +                                          "supported");

Why only one? What's wrong with supporting both 'qemu:xyz' (the dirty
bitmap, with whatever prefix name we actually settle on) and
'base:allocation' at the same time?


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature