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
error reply, representing channel-io error on previous try 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 <vsementsov@virtuozzo.com>
---

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)

 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.
+ * 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). */
 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));
+
+            ret = nbd_co_send_structured_error(client, handle, -status, msg,
+                                               errp);
+            g_free(msg);
+            return ret;
         }
         assert(pnum && pnum <= size - progress);
         final = progress + pnum == size;
@@ -1567,7 +1576,7 @@ static coroutine_fn void nbd_trip(void *opaque)
                                           request.from, req->data, request.len,
                                           &local_err);
             if (ret < 0) {
-                goto reply;
+                goto replied;
             }
             goto done;
         }
@@ -1664,6 +1673,8 @@ reply:
                                        req->data, reply_data_len, &local_err);
     }
     g_free(msg);
+
+replied:
     if (ret < 0) {
         error_prepend(&local_err, "Failed to send reply: ");
         goto disconnect;
-- 
2.11.1

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,

s/handle/handling/

> 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 <vsementsov@virtuozzo.com>
> ---
> 
> 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

s/fail/failure/

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

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