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 <eblake@redhat.com>

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;
     }



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