Re: [Qemu-devel] [PATCH v3 09/13] nbd: Minimal structured read for server

[Vladimir Sementsov-Ogievskiy]

13.10.2017 19:15, Eric Blake wrote:
> On 10/13/2017 11:00 AM, Eric Blake wrote:
> > On 10/12/2017 04:53 AM, Vladimir Sementsov-Ogievskiy wrote:
> > > 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.
> > >
> > > @@ -1304,10 +1375,17 @@ static int nbd_co_receive_request(NBDRequestData *req, 
> > > NBDRequest *request,
> > >                      (uint64_t)client->exp->size);
> > >           return request->type == NBD_CMD_WRITE ? -ENOSPC : -EINVAL;
> > >       }
> > > -    if (request->flags & ~(NBD_CMD_FLAG_FUA | NBD_CMD_FLAG_NO_HOLE)) {
> > > +    if (request->flags & ~(NBD_CMD_FLAG_FUA | NBD_CMD_FLAG_NO_HOLE |
> > > +                           NBD_CMD_FLAG_DF))
> > This says we accept the client sending NBD_CMD_FLAG_DF, but where did we
> > advertise it?  I see no reason to advertise it unless a client
> > negotiates structured replies, at which point the obvious place to set
> > it is when we reply to the negotiation (and NBD_OPT_INFO prior to that
> > point won't see the flag, but the NBD_OPT_GO will see it).
> Oh, one more tweak: if we didn't advertise the flag, the client
> shouldn't be sending it.
>
> > So, here's what I'm squashing, before adding:
> >
> > Reviewed-by: Eric Blake <address@hidden>
> I'm also adding:
>
> diff --git i/nbd/server.c w/nbd/server.c
> index b4966ed8b1..acad2ceb59 100644
> --- i/nbd/server.c
> +++ w/nbd/server.c
> @@ -1315,6 +1315,7 @@ static int nbd_co_receive_request(NBDRequestData
> *req, NBDRequest *request,
>                                     Error **errp)
>   {
>       NBDClient *client = req->client;
> +    int valid_flags;
>
>       g_assert(qemu_in_coroutine());
>       assert(client->recv_coroutine == qemu_coroutine_self());
> @@ -1376,20 +1377,15 @@ static int nbd_co_receive_request(NBDRequestData
> *req, NBDRequest *request,
>                      (uint64_t)client->exp->size);
>           return request->type == NBD_CMD_WRITE ? -ENOSPC : -EINVAL;
>       }
> -    if (request->flags & ~(NBD_CMD_FLAG_FUA | NBD_CMD_FLAG_NO_HOLE |
> -                           NBD_CMD_FLAG_DF))
> -    {
> -        error_setg(errp, "unsupported flags (got 0x%x)", request->flags);
> -        return -EINVAL;
> +    valid_flags = NBD_CMD_FLAG_FUA;
> +    if (request->type == NBD_CMD_READ && client->structured_reply) {
> +        valid_flags |= NBD_CMD_FLAG_DF;
> +    } else if (request->type == NBD_CMD_WRITE_ZEROES) {
> +        valid_flags |= NBD_CMD_FLAG_NO_HOLE;
>       }
> -    if (request->type != NBD_CMD_READ && (request->flags &
> NBD_CMD_FLAG_DF)) {
> -        error_setg(errp, "DF flag used with command %d (%s) which is
> not READ",
> -                   request->type, nbd_cmd_lookup(request->type));
> -        return -EINVAL;
> -    }
> -    if (request->type != NBD_CMD_WRITE_ZEROES &&
> -        (request->flags & NBD_CMD_FLAG_NO_HOLE)) {
> -        error_setg(errp, "unexpected flags (got 0x%x)", request->flags);
> +    if (request->flags & ~valid_flags) {
> +        error_setg(errp, "unsupported flags for command %s (got 0x%x)",
> +                   nbd_cmd_lookup(request->type), request->flags);
>           return -EINVAL;
>       }

ok for me.

--
Best regards,
Vladimir