On 03/09/2018 03:51 PM, Eric Blake wrote:
On 03/08/2018 12:46 PM, Vladimir Sementsov-Ogievskiy wrote:

In the subject: s/reply sending/sending reply/

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
---

It's like an RFC. I'm not sure, but this place looks like a bug. Shouldn't we chack client-closing even before nbd_client_receive_next_request() call?


Your RFC question is whether we can just check client->closing before checking ret, and skip nbd_client_receive_next_request() in that case (in other words, why even bother to queue up a coroutine that will do nothing, if we already know the client is going away).  And my answer is yes, I think that makes more sense.  So that would be:

diff --git c/nbd/server.c w/nbd/server.c
index 5f292064af0..b230ecb4fb8 100644
--- c/nbd/server.c
+++ w/nbd/server.c
@@ -1543,14 +1543,6 @@ static coroutine_fn void nbd_trip(void *opaque)
      req = nbd_request_get(client);
      ret = nbd_co_receive_request(req, &request, &local_err);
      client->recv_coroutine = NULL;
-    nbd_client_receive_next_request(client);
-    if (ret == -EIO) {
-        goto disconnect;
-    }
-
-    if (ret < 0) {
-        goto reply;
-    }

      if (client->closing) {
          /*
@@ -1560,6 +1552,15 @@ static coroutine_fn void nbd_trip(void *opaque)
          goto done;
      }

+    nbd_client_receive_next_request(client);
+    if (ret == -EIO) {
+        goto disconnect;
+    }
+
+    if (ret < 0) {
+        goto reply;
+    }
+
      switch (request.type) {
      case NBD_CMD_READ:
          /* XXX: NBD Protocol only documents use of FUA with WRITE */


Unless this revised form fails testing or gets any other review comments, I will go ahead and amend your commit in this manner.

I figured out why iotests were failing for an unrelated issue (thanks to Max for recognizing the symptoms on IRC), and my changes still passed, so I'm squashing them in as part of staging this series on my NBD queue.

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

Reply via email to