Re: [Qemu-devel] [PATCH 12/18] nbd: Less allocation during NBD_OPT_LIST

[Alex Bligh]

On 8 Apr 2016, at 23:05, Eric Blake <address@hidden> wrote:

> Since we know that the maximum name we are willing to accept
> is small enough to stack-allocate, rework the iteration over
> NBD_OPT_LIST responses to reuse a stack buffer rather than
> allocating every time.  Furthermore, we don't even have to
> allocate if we know the server's length doesn't match what
> we are searching for.
> 
> Not fixed here: Upstream NBD Protocol recently added this
> clarification:
> https://github.com/yoe/nbd/blob/18918eb/doc/proto.md#conventions
> 
> Where this document refers to a string, then unless otherwise
> stated, that string is a sequence of UTF-8 code points, which
> is not NUL terminated, MUST NOT contain NUL characters, SHOULD
> be no longer than 256 bytes and MUST be no longer than 4096
> bytes. This applies to export names and error messages (amongst
> others).
> 
> To be fully compliant to that, we need to bump our export name
> limit from 255 to at least 256, and need to decide whether we
> can bump it higher (bumping it all the way to 4096 is annoying
> in that we could no longer safely stack-allocate a worst-case
> string, so we may still want to take the leeway offered by SHOULD
> to force a reasonable smaller limit).

Is there a limit in qemu-world to safe stack allocation? I thought
that was (in general) only a kernel consideration? (probably my
ignorance here).

Otherwise:


Reviewed-by: Alex Bligh <address@hidden>

> 
> Signed-off-by: Eric Blake <address@hidden>
> ---
> nbd/client.c | 130 +++++++++++++++++++++++++++++------------------------------
> 1 file changed, 65 insertions(+), 65 deletions(-)
> 
> diff --git a/nbd/client.c b/nbd/client.c
> index b2dfc11..d4e37d5 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -230,14 +230,17 @@ static int nbd_handle_reply_err(QIOChannel *ioc, 
> nbd_opt_reply *reply,
>     return result;
> }
> 
> -static int nbd_receive_list(QIOChannel *ioc, char **name, Error **errp)
> +/* Return -1 if unrecoverable error occurs, 0 if NBD_OPT_LIST is
> + * unsupported, 1 if iteration is done, 2 to keep looking, and 3 if
> + * this entry matches want. */
> +static int nbd_receive_list(QIOChannel *ioc, const char *want, Error **errp)
> {
>     nbd_opt_reply reply;
>     uint32_t len;
>     uint32_t namelen;
> +    char name[NBD_MAX_NAME_SIZE + 1];
>     int error;
> 
> -    *name = NULL;
>     if (nbd_receive_option_reply(ioc, NBD_OPT_LIST, &reply, errp) < 0) {
>         return -1;
>     }
> @@ -252,97 +255,94 @@ static int nbd_receive_list(QIOChannel *ioc, char 
> **name, Error **errp)
>             error_setg(errp, "length too long for option end");
>             return -1;
>         }
> -    } else if (reply.type == NBD_REP_SERVER) {
> -        if (len < sizeof(namelen) || len > NBD_MAX_BUFFER_SIZE) {
> -            error_setg(errp, "incorrect option length %"PRIu32, len);
> -            return -1;
> -        }
> -        if (read_sync(ioc, &namelen, sizeof(namelen)) != sizeof(namelen)) {
> -            error_setg(errp, "failed to read option name length");
> -            return -1;
> -        }
> -        namelen = be32_to_cpu(namelen);
> -        len -= sizeof(namelen);
> -        if (len < namelen) {
> -            error_setg(errp, "incorrect option name length");
> -            return -1;
> -        }
> -        if (namelen > NBD_MAX_NAME_SIZE) {
> -            error_setg(errp, "export name length too long %" PRIu32, 
> namelen);
> -            return -1;
> -        }
> -
> -        *name = g_new0(char, namelen + 1);
> -        if (read_sync(ioc, *name, namelen) != namelen) {
> -            error_setg(errp, "failed to read export name");
> -            g_free(*name);
> -            *name = NULL;
> -            return -1;
> -        }
> -        (*name)[namelen] = '\0';
> -        len -= namelen;
> -        if (drop_sync(ioc, len) != len) {
> -            error_setg(errp, "failed to read export description");
> -            g_free(*name);
> -            *name = NULL;
> -            return -1;
> -        }
> -    } else {
> +        return 1;
> +    } else if (reply.type != NBD_REP_SERVER) {
>         error_setg(errp, "Unexpected reply type %" PRIx32 " expected %x",
>                    reply.type, NBD_REP_SERVER);
>         return -1;
>     }
> -    return 1;
> +
> +    if (len < sizeof(namelen) || len > NBD_MAX_BUFFER_SIZE) {
> +        error_setg(errp, "incorrect option length %"PRIu32, len);
> +        return -1;
> +    }
> +    if (read_sync(ioc, &namelen, sizeof(namelen)) != sizeof(namelen)) {
> +        error_setg(errp, "failed to read option name length");
> +        return -1;
> +    }
> +    namelen = be32_to_cpu(namelen);
> +    len -= sizeof(namelen);
> +    if (len < namelen) {
> +        error_setg(errp, "incorrect option name length");
> +        return -1;
> +    }
> +    if (namelen != strlen(want)) {
> +        if (drop_sync(ioc, len) != len) {
> +            error_setg(errp, "failed to skip export name with wrong length");
> +            return -1;
> +        }
> +        return 2;
> +    }
> +
> +    assert(namelen < sizeof(name));
> +    if (read_sync(ioc, name, namelen) != namelen) {
> +        error_setg(errp, "failed to read export name");
> +        return -1;
> +    }
> +    name[namelen] = '\0';
> +    len -= namelen;
> +    if (drop_sync(ioc, len) != len) {
> +        error_setg(errp, "failed to read export description");
> +        return -1;
> +    }
> +    return strcmp(name, want) == 0 ? 3 : 2;
> }
> 
> 
> +/* Return -1 on failure, 0 if wantname is an available export. */
> static int nbd_receive_query_exports(QIOChannel *ioc,
>                                      const char *wantname,
>                                      Error **errp)
> {
>     bool foundExport = false;
> 
> -    TRACE("Querying export list");
> +    TRACE("Querying export list for '%s'", wantname);
>     if (nbd_send_option_request(ioc, NBD_OPT_LIST, 0, NULL, errp) < 0) {
>         return -1;
>     }
> 
>     TRACE("Reading available export names");
>     while (1) {
> -        char *name = NULL;
> -        int ret = nbd_receive_list(ioc, &name, errp);
> +        int ret = nbd_receive_list(ioc, wantname, errp);
> 
> -        if (ret < 0) {
> -            g_free(name);
> -            name = NULL;
> +        switch (ret) {
> +        default:
> +            /* Server gave unexpected reply */
> +            assert(ret < 0);
>             return -1;
> -        }
> -        if (ret == 0) {
> +        case 0:
>             /* Server doesn't support export listing, so
>              * we will just assume an export with our
>              * wanted name exists */
> -            foundExport = true;
> -            break;
> -        }
> -        if (name == NULL) {
> -            TRACE("End of export name list");
> +            return 0;
> +        case 1:
> +            /* Done iterating. */
> +            if (!foundExport) {
> +                error_setg(errp, "No export with name '%s' available",
> +                           wantname);
> +                return -1;
> +            }
> +            return 0;
> +        case 2:
> +            /* Wasn't this one, keep going. */
>             break;
> -        }
> -        if (g_str_equal(name, wantname)) {
> +        case 3:
> +            /* Found a match, but must finish parsing reply. */
> +            TRACE("Found desired export name '%s'", wantname);
>             foundExport = true;
> -            TRACE("Found desired export name '%s'", name);
> -        } else {
> -            TRACE("Ignored export name '%s'", name);
> +            break;
>         }
> -        g_free(name);
> -    }
> -
> -    if (!foundExport) {
> -        error_setg(errp, "No export with name '%s' available", wantname);
> -        return -1;
>     }
> -
> -    return 0;
> }
> 
> static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
> -- 
> 2.5.5
> 
> 

-- 
Alex Bligh