Re: [Qemu-block] [PATCH 3/9] nbd: BLOCK_STATUS for standard get_block_status function: server part

[Eric Blake]

On 02/15/2018 07:51 AM, Vladimir Sementsov-Ogievskiy wrote:
> Minimal realization: only one extent in server answer is supported.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>   include/block/nbd.h |  33 ++++++
>   nbd/common.c        |  10 ++
>   nbd/server.c        | 310 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>   3 files changed, 352 insertions(+), 1 deletion(-)

> @@ -200,9 +227,15 @@ 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

Stale; see nbd.git commit 56c77720 which changed this to 3.

> +++ b/nbd/server.c
> @@ -82,6 +82,15 @@ struct NBDExport {
>   
>   static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
>   
> +/* NBDExportMetaContexts represents list of selected by
> + * NBD_OPT_SET_META_CONTEXT contexts to be exported. */

represents a list of contexts to be exported, as selected by 
NBD_OPT_SET_META_CONTEXT.

> +typedef struct NBDExportMetaContexts {
> +    char export_name[NBD_MAX_NAME_SIZE + 1];

Would this work as const char * pointing at some other storage, instead 
of having to copy into this storage?

> +    bool valid; /* means that negotiation of the option finished without
> +                   errors */
> +    bool base_allocation; /* export base:allocation context (block status) */
> +} NBDExportMetaContexts;
> +
>   struct NBDClient {

> @@ -636,6 +646,201 @@ static QIOChannel 
> *nbd_negotiate_handle_starttls(NBDClient *client,
>       return QIO_CHANNEL(tioc);
>   }
>   
> +/* nbd_alloc_read_size_string
> + *
> + * Read string in format
> + *   uint32_t len
> + *   len bytes string (not 0-terminated)
> + * String is allocated and pointer returned as @buf
> + *
> + * Return -errno on I/O error, 0 if option was completely handled by
> + * sending a reply about inconsistent lengths, or 1 on success. */
> +static int nbd_alloc_read_size_string(NBDClient *client, char **buf,
> +                                      Error **errp)
> +{
> +    int ret;
> +    uint32_t len;
> +
> +    ret = nbd_opt_read(client, &len, sizeof(len), errp);
> +    if (ret <= 0) {
> +        return ret;
> +    }
> +    cpu_to_be32s(&len);
> +
> +    *buf = g_try_malloc(len + 1);

I'd rather check that len is sane prior to trying to malloc.  Otherwise, 
a malicious client can convince us to waste time/space doing a large 
malloc before we finally realize that we can't read that many bytes 
after all.  And in fact, if you check len in advance, you can probably 
just use g_malloc() instead of g_try_malloc() (g_try_malloc() makes 
sense on a 1M allocation, where we can still allocate smaller stuff in 
reporting the error; but since NBD limits strings to 4k, if we fail at 
allocating 4k, we are probably already so hosed that our attempts to 
report the failure will also run out of memory and abort, so why not 
just abort now).

> +    if (*buf == NULL) {
> +        error_setg(errp, "No memory");
> +        return -ENOMEM;
> +    }
> +    (*buf)[len] = '\0';
> +
> +    ret = nbd_opt_read(client, *buf, len, errp);
> +    if (ret <= 0) {
> +        g_free(*buf);
> +        *buf = NULL;
> +    }
> +
> +    return ret;
> +}
> +
> +/* nbd_read_size_string

Will resume review here...

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org