Re: [Qemu-devel] [PATCH v5 06/11] nbd: Minimal structured read for server

[Vladimir Sementsov-Ogievskiy]

20.10.2017 01:26, Eric Blake wrote:
> From: Vladimir Sementsov-Ogievskiy <address@hidden>
>
> Minimal implementation of structured read: one structured reply chunk,
> no segmentation.
> Minimal structured error implementation: no text message.
> Support DF flag, but just ignore it, as there is no segmentation any
> way.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v5: correct DF flag spelling, include errname in trace, handle any bogus
> payload from option
> v4: better _DF flag handling, convert errno to wire format, add
> comments and tracing, rework structured error for less churn when adding
> text message later, don't kill connection on redundant client option
> ---
>   nbd/server.c     | 106 +++++++++++++++++++++++++++++++++++++++++++++++++------
>   nbd/trace-events |   2 ++
>   2 files changed, 98 insertions(+), 10 deletions(-)
>
> diff --git a/nbd/server.c b/nbd/server.c
> index b3f7e0b18e..9be93c4a52 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -100,6 +100,8 @@ struct NBDClient {
>       QTAILQ_ENTRY(NBDClient) next;
>       int nb_requests;
>       bool closing;
> +
> +    bool structured_reply;
>   };
>
>   /* That's all folks */
> @@ -754,6 +756,22 @@ static int nbd_negotiate_options(NBDClient *client, 
> uint16_t myflags,
>                                                        "TLS not configured");
>                   }
>                   break;
> +
> +            case NBD_OPT_STRUCTURED_REPLY:
> +                if (length) {
> +                    ret = nbd_check_zero_length(client, length, option, errp);

here same thing like in previous patch, about success in 
nbd_check_zero_length

> +                } else if (client->structured_reply) {
> +                    ret = nbd_negotiate_send_rep_err(
> +                        client->ioc, NBD_REP_ERR_INVALID, option, errp,
> +                        "structured reply already negotiated");
> +                } else {
> +                    ret = nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
> +                                                 option, errp);
> +                }

you've dropped "if (ret < 0) { return ret }", but the following two 
lines should not be
executed if ret < 0.. May be it doesn't matter (we will abort connection 
anyway after switch {}) but
it looks strange.

> +                client->structured_reply = true;
> +                myflags |= NBD_FLAG_SEND_DF;
> +                break;
> +
>               default:
>                   if (nbd_drop(client->ioc, length, errp) < 0) {
>                       return -EIO;
> @@ -1228,6 +1246,60 @@ static int nbd_co_send_simple_reply(NBDClient *client,
>       return nbd_co_send_iov(client, iov, len ? 2 : 1, errp);
>   }

[...]


--
Best regards,
Vladimir