Re: [Qemu-devel] [PATCH 15/18] nbd: Implement NBD_OPT_GO on server

[Alex Bligh]

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

> NBD_OPT_EXPORT_NAME is lousy: it requires us to close the connection
> rather than report an error.  Upstream NBD recently added NBD_OPT_GO
> as the improved version of the option that does what we want, along
> with NBD_OPT_INFO that returns the same information but does not
> transition to transmission phase.
> 
> Signed-off-by: Eric Blake <address@hidden>


Reviewed-by: Alex Bligh <address@hidden>

> ---
> nbd/server.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 109 insertions(+), 13 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 379df8c..e68e83c 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -233,17 +233,19 @@ static int nbd_negotiate_send_rep(QIOChannel *ioc, 
> uint32_t type, uint32_t opt)
>     return nbd_negotiate_send_rep_len(ioc, type, opt, 0);
> }
> 
> -/* Send an NBD_REP_SERVER reply to NBD_OPT_LIST, including payload.
> +/* Send the common part of an NBD_REP_SERVER reply for the given option,
> + * and include extra_len in the advertised payload.
>  * Return -errno to kill connection, 0 to continue negotiation */
> -static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp)
> +static int nbd_negotiate_send_rep_server(QIOChannel *ioc, NBDExport *exp,
> +                                         uint32_t opt, uint32_t extra_len)
> {
>     uint32_t len;
>     int rc;
> 
>     TRACE("Advertising export name '%s'", exp->name ? exp->name : "");
>     len = strlen(exp->name);
> -    rc = nbd_negotiate_send_rep_len(ioc, NBD_REP_SERVER, NBD_OPT_LIST,
> -                                    len + sizeof(len));
> +    rc = nbd_negotiate_send_rep_len(ioc, NBD_REP_SERVER, opt,
> +                                    len + sizeof(len) + extra_len);
>     if (rc < 0) {
>         return rc;
>     }
> @@ -261,6 +263,15 @@ static int nbd_negotiate_send_rep_list(QIOChannel *ioc, 
> NBDExport *exp)
>     return 0;
> }
> 
> +/* Send an NBD_REP_SERVER reply to NBD_OPT_LIST, including payload.
> + * Return -errno to kill connection, 0 to continue negotiation. */
> +static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp)
> +{
> +    return nbd_negotiate_send_rep_server(ioc, exp, NBD_OPT_LIST, 0);
> +}
> +
> +/* Send a sequence of replies to NBD_OPT_LIST.
> + * Return -errno to kill connection, 0 to continue negotiation. */
> static int nbd_negotiate_handle_list(NBDClient *client, uint32_t length)
> {
>     NBDExport *exp;
> @@ -283,6 +294,8 @@ static int nbd_negotiate_handle_list(NBDClient *client, 
> uint32_t length)
>     return nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, NBD_OPT_LIST);
> }
> 
> +/* Send a reply to NBD_OPT_EXPORT_NAME.
> + * Return -errno to kill connection, 0 to end negotiation. */
> static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t 
> length)
> {
>     int rc = -EINVAL;
> @@ -318,6 +331,73 @@ fail:
> }
> 
> 
> +/* Handle NBD_OPT_INFO and NBD_OPT_GO.
> + * Return -errno to kill connection, 0 if ready for next option, and 1
> + * to move into transmission phase.  */
> +static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length,
> +                                     uint32_t opt, uint16_t myflags)
> +{
> +    int rc;
> +    char name[NBD_MAX_NAME_SIZE + 1];
> +    NBDExport *exp;
> +    uint64_t size;
> +    uint16_t flags;
> +
> +    /* Client sends:
> +        [20 ..  xx]   export name (length bytes)
> +     */
> +    TRACE("Checking length");
> +    if (length >= sizeof(name)) {
> +        if (nbd_negotiate_drop_sync(client->ioc, length) != length) {
> +            return -EIO;
> +        }
> +        return nbd_negotiate_send_rep(client->ioc, NBD_REP_ERR_INVALID, opt);
> +    }
> +    if (nbd_negotiate_read(client->ioc, name, length) != length) {
> +        LOG("read failed");
> +        return -EIO;
> +    }
> +    name[length] = '\0';
> +
> +    TRACE("Client requested info on export '%s'", name);
> +
> +    exp = nbd_export_find(name);
> +    if (!exp) {
> +        return nbd_negotiate_send_rep(client->ioc, NBD_REP_ERR_UNKNOWN, opt);
> +    }
> +
> +    QEMU_BUILD_BUG_ON(NBD_FINAL_REPLY_SIZE != sizeof(size) + sizeof(flags));
> +    rc = nbd_negotiate_send_rep_server(client->ioc, exp, opt,
> +                                       NBD_FINAL_REPLY_SIZE);
> +    if (rc < 0) {
> +        return rc;
> +    }
> +
> +    assert((exp->nbdflags & ~65535) == 0);
> +    size = cpu_to_be64(exp->size);
> +    flags = cpu_to_be16(exp->nbdflags | myflags);
> +
> +    if (nbd_negotiate_write(client->ioc, &size, sizeof(size)) !=
> +        sizeof(size)) {
> +        LOG("write failed");
> +        return -EIO;
> +    }
> +    if (nbd_negotiate_write(client->ioc, &flags, sizeof(flags)) !=
> +        sizeof(flags)) {
> +        LOG("write failed");
> +        return -EIO;
> +    }
> +
> +    if (opt == NBD_OPT_GO) {
> +        client->exp = exp;
> +        QTAILQ_INSERT_TAIL(&client->exp->clients, client, next);
> +        nbd_export_get(client->exp);
> +        rc = 1;
> +    }
> +    return rc;
> +}
> +
> +
> static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
>                                                  uint32_t length)
> {
> @@ -366,7 +446,10 @@ static QIOChannel 
> *nbd_negotiate_handle_starttls(NBDClient *client,
> }
> 
> 
> -static int nbd_negotiate_options(NBDClient *client)
> +/* Loop over all client options, during fixed newstyle negotiation.
> + * Return -errno to kill connection, 0 on successful NBD_OPT_EXPORT_NAME,
> + * 1 on successful NBD_OPT_GO.  */
> +static int nbd_negotiate_options(NBDClient *client, uint16_t myflags)
> {
>     uint32_t flags;
>     bool fixedNewstyle = false;
> @@ -480,6 +563,16 @@ static int nbd_negotiate_options(NBDClient *client)
>             case NBD_OPT_EXPORT_NAME:
>                 return nbd_negotiate_handle_export_name(client, length);
> 
> +            case NBD_OPT_INFO:
> +            case NBD_OPT_GO:
> +                ret = nbd_negotiate_handle_info(client, length, clientflags,
> +                                                myflags);
> +                if (ret) {
> +                    assert(ret < 0 || clientflags == NBD_OPT_GO);
> +                    return ret;
> +                }
> +                break;
> +
>             case NBD_OPT_STARTTLS:
>                 if (nbd_negotiate_drop_sync(client->ioc, length) != length) {
>                     return -EIO;
> @@ -584,18 +677,21 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData 
> *data)
>             LOG("write failed");
>             goto fail;
>         }
> -        rc = nbd_negotiate_options(client);
> -        if (rc != 0) {
> +        rc = nbd_negotiate_options(client, myflags);
> +        if (rc < 0) {
>             LOG("option negotiation failed");
>             goto fail;
>         }
> 
> -        stq_be_p(buf + 18, client->exp->size);
> -        stw_be_p(buf + 26, client->exp->nbdflags | myflags);
> -        len = client->no_zeroes ? 10 : sizeof(buf) - 18;
> -        if (nbd_negotiate_write(client->ioc, buf + 18, len) != len) {
> -            LOG("write failed");
> -            goto fail;
> +        if (!rc) {
> +            /* If options ended with NBD_OPT_GO, we already sent this. */
> +            stq_be_p(buf + 18, client->exp->size);
> +            stw_be_p(buf + 26, client->exp->nbdflags | myflags);
> +            len = client->no_zeroes ? 10 : sizeof(buf) - 18;
> +            if (nbd_negotiate_write(client->ioc, buf + 18, len) != len) {
> +                LOG("write failed");
> +                goto fail;
> +            }
>         }
>     }
> 
> -- 
> 2.5.5
> 
> 

-- 
Alex Bligh