[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH v2 2/5] nbd/server: fix sparse read

From: Eric Blake
Subject: Re: [Qemu-block] [PATCH v2 2/5] nbd/server: fix sparse read
Date: Fri, 9 Mar 2018 13:55:10 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0

On 03/08/2018 12:46 PM, Vladimir Sementsov-Ogievskiy wrote:
In case of io error in nbd_co_send_sparse_read we should not
"goto reply:", as it is fatal error and common behavior is
disconnect in this case. We should not try to send client an

s/send/send the/

error reply, representing channel-io error on previous try to
send a reply.

s/representing .../since we already hit a channel-io error on our previous attempt to send a reply/

Fix this by handle block-status error in nbd_co_send_sparse_read,


so nbd_co_send_sparse_read fails only on io error. Then just skip
common "reply:" code path in nbd_trip.

Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>

v2: code movement splitted to 01
     remove error_report
     fix indent
     fix commit message subject line
     add comment to nbd_co_send_sparse_read (be free to adjust it)

Not a problem, I know English is not your native tongue, so I don't mind touching it up, and I already admire your efforts at programming in a second language (a skill I lack).

  nbd/server.c | 17 ++++++++++++++---
  1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index c406b0656d..e0de431e10 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1365,6 +1365,10 @@ static int coroutine_fn 
nbd_co_send_structured_error(NBDClient *client,
      return nbd_co_send_iov(client, iov, 1 + !!iov[1].iov_len, errp);
+/* Do sparse read and send structured reply to the client.

s/Do/Do a/

+ * Returns -errno if sending fails. bdrv_block_status_above() fail is not


+ * considered as an error, but reported to the client instead (as a structured
+ * error). */

The comment tail */ usually gets its own line.

Here's what I used:

/* Do a sparse read and send the structured reply to the client.
 * Returns -errno if sending fails. bdrv_block_status_above() failure is
 * reported to the client, at which point this function succeeds.

  static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client,
                                                  uint64_t handle,
                                                  uint64_t offset,
@@ -1385,8 +1389,13 @@ static int coroutine_fn 
nbd_co_send_sparse_read(NBDClient *client,
          bool final;
if (status < 0) {
-            error_setg_errno(errp, -status, "unable to check for holes");
-            return status;
+            char *msg = g_strdup_printf("unable to check for holes: %s",
+                                        strerror(-status));

I had to double-check block/io.c, but it looks like bdrv_block-status_above() does indeed document a return of negative errno on failure. (My suspicion is that the documentation might be wrong, because I did not audit error returns when doing my byte-based work; but a quick audit of a couple of random drivers shows that at least iscsi.c and qed.c appear to comply.)

+            ret = nbd_co_send_structured_error(client, handle, -status, msg,
+                                               errp);
+            g_free(msg);
+            return ret;

I wonder if a separate patch should clean this up to let nbd_co_send_structured_error() do message formatting (making it varargs) - but doesn't affect this patch.

With grammar tweaks,
Reviewed-by: Eric Blake <address@hidden

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]