[Qemu-devel] [PATCH 0/5] nbd server fixing and refactoring before BLOCK_STATUS

[Qemu-devel] [PATCH 0/5] nbd server fixing and refactoring before BLOCK_STATUS

[Vladimir Sementsov-Ogievskiy]

01 and 02 are splitted and updated "[PATCH] nbd/server: fix space read",
others are new.

Vladimir Sementsov-Ogievskiy (5):
  nbd/server: move nbd_co_send_structured_error up
  nbd/server: fix sparse read
  nbd/server: fix: check client->closing before reply sending
  nbd/server: refactor nbd_trip: cmd_read and generic reply
  nbd/server: refactor nbd_trip: split out nbd_handle_request

 nbd/server.c | 302 ++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 165 insertions(+), 137 deletions(-)

-- 
2.11.1

[Qemu-devel] [PATCH 1/5] nbd/server: move nbd_co_send_structured_error up

[Vladimir Sementsov-Ogievskiy]

To be reused in nbd_co_send_sparse_read() in the following patch.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/server.c | 48 ++++++++++++++++++++++++------------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 4990a5826e..c406b0656d 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1341,6 +1341,30 @@ static int coroutine_fn nbd_co_send_structured_read(NBDClient *client,
     return nbd_co_send_iov(client, iov, 2, errp);
 }
 
+static int coroutine_fn nbd_co_send_structured_error(NBDClient *client,
+                                                     uint64_t handle,
+                                                     uint32_t error,
+                                                     const char *msg,
+                                                     Error **errp)
+{
+    NBDStructuredError chunk;
+    int nbd_err = system_errno_to_nbd_errno(error);
+    struct iovec iov[] = {
+        {.iov_base = &chunk, .iov_len = sizeof(chunk)},
+        {.iov_base = (char *)msg, .iov_len = msg ? strlen(msg) : 0},
+    };
+
+    assert(nbd_err);
+    trace_nbd_co_send_structured_error(handle, nbd_err,
+                                       nbd_err_lookup(nbd_err), msg ? msg : "");
+    set_be_chunk(&chunk.h, NBD_REPLY_FLAG_DONE, NBD_REPLY_TYPE_ERROR, handle,
+                 sizeof(chunk) - sizeof(chunk.h) + iov[1].iov_len);
+    stl_be_p(&chunk.error, nbd_err);
+    stw_be_p(&chunk.message_length, iov[1].iov_len);
+
+    return nbd_co_send_iov(client, iov, 1 + !!iov[1].iov_len, errp);
+}
+
 static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client,
                                                 uint64_t handle,
                                                 uint64_t offset,
@@ -1400,30 +1424,6 @@ static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client,
     return ret;
 }
 
-static int coroutine_fn nbd_co_send_structured_error(NBDClient *client,
-                                                     uint64_t handle,
-                                                     uint32_t error,
-                                                     const char *msg,
-                                                     Error **errp)
-{
-    NBDStructuredError chunk;
-    int nbd_err = system_errno_to_nbd_errno(error);
-    struct iovec iov[] = {
-        {.iov_base = &chunk, .iov_len = sizeof(chunk)},
-        {.iov_base = (char *)msg, .iov_len = msg ? strlen(msg) : 0},
-    };
-
-    assert(nbd_err);
-    trace_nbd_co_send_structured_error(handle, nbd_err,
-                                       nbd_err_lookup(nbd_err), msg ? msg : "");
-    set_be_chunk(&chunk.h, NBD_REPLY_FLAG_DONE, NBD_REPLY_TYPE_ERROR, handle,
-                 sizeof(chunk) - sizeof(chunk.h) + iov[1].iov_len);
-    stl_be_p(&chunk.error, nbd_err);
-    stw_be_p(&chunk.message_length, iov[1].iov_len);
-
-    return nbd_co_send_iov(client, iov, 1 + !!iov[1].iov_len, errp);
-}
-
 /* nbd_co_receive_request
  * Collect a client request. Return 0 if request looks valid, -EIO to drop
  * connection right away, and any other negative value to report an error to
-- 
2.11.1

Re: [Qemu-devel] [PATCH 1/5] nbd/server: move nbd_co_send_structured_error up

[Eric Blake]

On 03/08/2018 12:46 PM, Vladimir Sementsov-Ogievskiy wrote:
> To be reused in nbd_co_send_sparse_read() in the following patch.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   nbd/server.c | 48 ++++++++++++++++++++++++------------------------
>   1 file changed, 24 insertions(+), 24 deletions(-)
> 
Reviewed-by: Eric Blake <eblake@redhat.com>

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

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

[Vladimir Sementsov-Ogievskiy]

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

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

[Eric Blake]

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

[Qemu-devel] [PATCH 3/5] nbd/server: fix: check client->closing before reply sending

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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?

 nbd/server.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index e0de431e10..97b45a21fa 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1547,10 +1547,6 @@ static coroutine_fn void nbd_trip(void *opaque)
         goto disconnect;
     }
 
-    if (ret < 0) {
-        goto reply;
-    }
-
     if (client->closing) {
         /*
          * The client may be closed when we are blocked in
@@ -1559,6 +1555,10 @@ static coroutine_fn void nbd_trip(void *opaque)
         goto done;
     }
 
+    if (ret < 0) {
+        goto reply;
+    }
+
     switch (request.type) {
     case NBD_CMD_READ:
         /* XXX: NBD Protocol only documents use of FUA with WRITE */
-- 
2.11.1

Re: [Qemu-devel] [PATCH 3/5] nbd/server: fix: check client->closing before reply sending

[Eric Blake]

On 03/08/2018 12:46 PM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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?
> 
>   nbd/server.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index e0de431e10..97b45a21fa 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1547,10 +1547,6 @@ static coroutine_fn void nbd_trip(void *opaque)

More context:

     ret = nbd_co_receive_request(req, &request, &local_err);
     client->recv_coroutine = NULL;
     nbd_client_receive_next_request(client);
     if (ret == -EIO) {

>           goto disconnect;
>       }
>   

Calling nbd_client_receive_next_request() checks whether recv_coroutine 
is NULL (it is, because we just set it that way) and whether we are up 
to our maximum in parallel request handling; so it likely queues another 
coroutine to call nbd_trip() again.  However, when the next nbd_trip() 
is invoked, the first thing it does (after a trace call) is check 
client->closing, at which point it is a nop.

Your argument is that if ret was -EIO, we goto disconnect (which sets 
client->closing if it was not already set), and thus the just-scheduled 
next nbd_trip() will be a nop.  If ret was anything else, we used to try 
to reply to the client no matter what, which generally works; although 
if client->closing is already set, the attempt to reply is instead 
likely to fail and result in a later attempt to goto disconnect - but at 
that point disconnect is a nop since client->closing is already set. 
Whereas your patch skips the reply to the client if client->closing (and 
can't reach the disconnect code, but that doesn't matter as the 
disconnect attempt did nothing).  Swapping the check for client->closing 
to be earlier than the reply to the client thus looks safe.

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.

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

Re: [Qemu-devel] [PATCH 3/5] nbd/server: fix: check client->closing before reply sending

[Eric Blake]

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 <vsementsov@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

[Qemu-devel] [PATCH 4/5] nbd/server: refactor nbd_trip: cmd_read and generic reply

[Vladimir Sementsov-Ogievskiy]

nbd_trip has difficult logic of sending reply: it tries to use one
code path for all replies. It is ok for simple replies, but is not
comfortable for structured replies. Also, two types of error (and
corresponding message in local_err) - fatal (leading to disconnect)
and not-fatal (just to be sent to the client) are difficult to follow.

To make things a bit clearer, the following is done:
 - split CMD_READ logic to separate function. It is the most difficult
   command for now, and it is definitely cramped inside nbd_trip. Also,
   it is difficult to follow CMD_READ logic, shared between
   "case NBD_CMD_READ" and "if"s under "reply:" label.
 - create separate helper function nbd_send_generic_reply() and use it
   both in new nbd_do_cmd_read and for other command in nbd_trip instead
   of common code-path under "reply:" label in nbd_trip. The helper
   supports error message, so logic with local_err in nbd_trip is
   simplified.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/server.c | 164 ++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 88 insertions(+), 76 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 97b45a21fa..a2f5f73d52 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1520,6 +1520,70 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request,
     return 0;
 }
 
+/* Send simple reply without a payload or structured error
+ * @error_msg is ignored if @ret >= 0 */
+static coroutine_fn int nbd_send_generic_reply(NBDClient *client,
+                                               uint64_t handle,
+                                               int ret,
+                                               const char *error_msg,
+                                               Error **errp)
+{
+    if (client->structured_reply && ret < 0) {
+        return nbd_co_send_structured_error(client, handle, -ret, error_msg,
+                                            errp);
+    } else {
+        return nbd_co_send_simple_reply(client, handle, ret < 0 ? -ret : 0,
+                                        NULL, 0, errp);
+    }
+}
+
+/* Handle NBD_CMD_READ request.
+ * Return -errno if sending fails. Other errors are reported directly to the
+ * client as an error reply. */
+static coroutine_fn int nbd_do_cmd_read(NBDClient *client, NBDRequest *request,
+                                        uint8_t *data, Error **errp)
+{
+    int ret;
+    NBDExport *exp = client->exp;
+
+    assert(request->type == NBD_CMD_READ);
+
+    /* XXX: NBD Protocol only documents use of FUA with WRITE */
+    if (request->flags & NBD_CMD_FLAG_FUA) {
+        ret = blk_co_flush(exp->blk);
+        if (ret < 0) {
+            return nbd_send_generic_reply(client, request->handle, ret,
+                                          "flush failed", errp);
+        }
+    }
+
+    if (client->structured_reply && !(request->flags & NBD_CMD_FLAG_DF) &&
+        request->len) {
+        return nbd_co_send_sparse_read(client, request->handle, request->from,
+                                       data, request->len, errp);
+    }
+
+    ret = blk_pread(exp->blk, request->from + exp->dev_offset, data,
+                    request->len);
+    if (ret < 0) {
+        return nbd_send_generic_reply(client, request->handle, ret,
+                                      "reading from file failed", errp);
+    }
+
+    if (client->structured_reply) {
+        if (request->len) {
+            return nbd_co_send_structured_read(client, request->handle,
+                                               request->from, data,
+                                               request->len, true, errp);
+        } else {
+            return nbd_co_send_structured_done(client, request->handle, errp);
+        }
+    } else {
+        return nbd_co_send_simple_reply(client, request->handle, 0,
+                                        data, request->len, errp);
+    }
+}
+
 /* Owns a reference to the NBDClient passed as opaque.  */
 static coroutine_fn void nbd_trip(void *opaque)
 {
@@ -1529,7 +1593,6 @@ static coroutine_fn void nbd_trip(void *opaque)
     NBDRequest request = { 0 };    /* GCC thinks it can be used uninitialized */
     int ret;
     int flags;
-    int reply_data_len = 0;
     Error *local_err = NULL;
     char *msg = NULL;
 
@@ -1556,39 +1619,21 @@ static coroutine_fn void nbd_trip(void *opaque)
     }
 
     if (ret < 0) {
-        goto reply;
+        /* It's not a -EIO, so, accordingly to nbd_co_receive_request()
+         * semantics, we should return the error to the client. */
+        Error *export_err = local_err;
+
+        local_err = NULL;
+        ret = nbd_send_generic_reply(client, request.handle, -EINVAL,
+                                     error_get_pretty(export_err), &local_err);
+        error_free(export_err);
+
+        goto replied;
     }
 
     switch (request.type) {
     case NBD_CMD_READ:
-        /* XXX: NBD Protocol only documents use of FUA with WRITE */
-        if (request.flags & NBD_CMD_FLAG_FUA) {
-            ret = blk_co_flush(exp->blk);
-            if (ret < 0) {
-                error_setg_errno(&local_err, -ret, "flush failed");
-                break;
-            }
-        }
-
-        if (client->structured_reply && !(request.flags & NBD_CMD_FLAG_DF) &&
-            request.len) {
-            ret = nbd_co_send_sparse_read(req->client, request.handle,
-                                          request.from, req->data, request.len,
-                                          &local_err);
-            if (ret < 0) {
-                goto replied;
-            }
-            goto done;
-        }
-
-        ret = blk_pread(exp->blk, request.from + exp->dev_offset,
-                        req->data, request.len);
-        if (ret < 0) {
-            error_setg_errno(&local_err, -ret, "reading from file failed");
-            break;
-        }
-
-        reply_data_len = request.len;
+        ret = nbd_do_cmd_read(client, &request, req->data, &local_err);
 
         break;
     case NBD_CMD_WRITE:
@@ -1598,9 +1643,8 @@ static coroutine_fn void nbd_trip(void *opaque)
         }
         ret = blk_pwrite(exp->blk, request.from + exp->dev_offset,
                          req->data, request.len, flags);
-        if (ret < 0) {
-            error_setg_errno(&local_err, -ret, "writing to file failed");
-        }
+        ret = nbd_send_generic_reply(client, request.handle, ret,
+                                     "writing to file failed", &local_err);
 
         break;
     case NBD_CMD_WRITE_ZEROES:
@@ -1613,9 +1657,8 @@ static coroutine_fn void nbd_trip(void *opaque)
         }
         ret = blk_pwrite_zeroes(exp->blk, request.from + exp->dev_offset,
                                 request.len, flags);
-        if (ret < 0) {
-            error_setg_errno(&local_err, -ret, "writing to file failed");
-        }
+        ret = nbd_send_generic_reply(client, request.handle, ret,
+                                     "writing to file failed", &local_err);
 
         break;
     case NBD_CMD_DISC:
@@ -1624,56 +1667,25 @@ static coroutine_fn void nbd_trip(void *opaque)
 
     case NBD_CMD_FLUSH:
         ret = blk_co_flush(exp->blk);
-        if (ret < 0) {
-            error_setg_errno(&local_err, -ret, "flush failed");
-        }
+        ret = nbd_send_generic_reply(client, request.handle, ret,
+                                     "flush failed", &local_err);
 
         break;
     case NBD_CMD_TRIM:
         ret = blk_co_pdiscard(exp->blk, request.from + exp->dev_offset,
                               request.len);
-        if (ret < 0) {
-            error_setg_errno(&local_err, -ret, "discard failed");
-        }
+        ret = nbd_send_generic_reply(client, request.handle, ret,
+                                     "discard failed", &local_err);
 
         break;
     default:
-        error_setg(&local_err, "invalid request type (%" PRIu32 ") received",
-                   request.type);
-        ret = -EINVAL;
-    }
-
-reply:
-    if (local_err) {
-        /* If we get here, local_err was not a fatal error, and should be sent
-         * to the client. */
-        assert(ret < 0);
-        msg = g_strdup(error_get_pretty(local_err));
-        error_report_err(local_err);
-        local_err = NULL;
+        msg = g_strdup_printf("invalid request type (%" PRIu32 ") received",
+                              request.type);
+        ret = nbd_send_generic_reply(client, request.handle, -EINVAL, msg,
+                                     &local_err);
+        g_free(msg);
     }
 
-    if (client->structured_reply &&
-        (ret < 0 || request.type == NBD_CMD_READ)) {
-        if (ret < 0) {
-            ret = nbd_co_send_structured_error(req->client, request.handle,
-                                               -ret, msg, &local_err);
-        } else if (reply_data_len) {
-            ret = nbd_co_send_structured_read(req->client, request.handle,
-                                              request.from, req->data,
-                                              reply_data_len, true,
-                                              &local_err);
-        } else {
-            ret = nbd_co_send_structured_done(req->client, request.handle,
-                                              &local_err);
-        }
-    } else {
-        ret = nbd_co_send_simple_reply(req->client, request.handle,
-                                       ret < 0 ? -ret : 0,
-                                       req->data, reply_data_len, &local_err);
-    }
-    g_free(msg);
-
 replied:
     if (ret < 0) {
         error_prepend(&local_err, "Failed to send reply: ");
-- 
2.11.1

Re: [Qemu-devel] [PATCH 4/5] nbd/server: refactor nbd_trip: cmd_read and generic reply

[Eric Blake]

On 03/08/2018 12:46 PM, Vladimir Sementsov-Ogievskiy wrote:
> nbd_trip has difficult logic of sending reply: it tries to use one
> code path for all replies. It is ok for simple replies, but is not
> comfortable for structured replies. Also, two types of error (and
> corresponding message in local_err) - fatal (leading to disconnect)
> and not-fatal (just to be sent to the client) are difficult to follow.
> 
> To make things a bit clearer, the following is done:
>   - split CMD_READ logic to separate function. It is the most difficult
>     command for now, and it is definitely cramped inside nbd_trip. Also,
>     it is difficult to follow CMD_READ logic, shared between
>     "case NBD_CMD_READ" and "if"s under "reply:" label.

Yay - I already admitted when adding sparse read replies that splitting 
the logic was weird.

>   - create separate helper function nbd_send_generic_reply() and use it
>     both in new nbd_do_cmd_read and for other command in nbd_trip instead

s/command/commands/

>     of common code-path under "reply:" label in nbd_trip. The helper
>     supports error message, so logic with local_err in nbd_trip is

s/supports/supports an/

>     simplified.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   nbd/server.c | 164 ++++++++++++++++++++++++++++++++---------------------------
>   1 file changed, 88 insertions(+), 76 deletions(-)

Gains a few lines, but I think the end result is more legible.

> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 97b45a21fa..a2f5f73d52 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1520,6 +1520,70 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request,
>       return 0;
>   }
>   
> +/* Send simple reply without a payload or structured error

s/payload or/payload, or a /

> + * @error_msg is ignored if @ret >= 0 */

Maybe mention the return value (0 if connection is still live, -errno on 
failure to communicate to client)

> +static coroutine_fn int nbd_send_generic_reply(NBDClient *client,
> +                                               uint64_t handle,
> +                                               int ret,
> +                                               const char *error_msg,
> +                                               Error **errp)
> +{
> +    if (client->structured_reply && ret < 0) {
> +        return nbd_co_send_structured_error(client, handle, -ret, error_msg,
> +                                            errp);
> +    } else {
> +        return nbd_co_send_simple_reply(client, handle, ret < 0 ? -ret : 0,
> +                                        NULL, 0, errp);
> +    }
> +}
> +
> +/* Handle NBD_CMD_READ request.
> + * Return -errno if sending fails. Other errors are reported directly to the
> + * client as an error reply. */
> +static coroutine_fn int nbd_do_cmd_read(NBDClient *client, NBDRequest *request,
> +                                        uint8_t *data, Error **errp)
> +{

> +
>   /* Owns a reference to the NBDClient passed as opaque.  */
>   static coroutine_fn void nbd_trip(void *opaque)
>   {
> @@ -1529,7 +1593,6 @@ static coroutine_fn void nbd_trip(void *opaque)
>       NBDRequest request = { 0 };    /* GCC thinks it can be used uninitialized */
>       int ret;
>       int flags;
> -    int reply_data_len = 0;
>       Error *local_err = NULL;
>       char *msg = NULL;
>   
> @@ -1556,39 +1619,21 @@ static coroutine_fn void nbd_trip(void *opaque)
>       }
>   
>       if (ret < 0) {
> -        goto reply;
> +        /* It's not a -EIO, so, accordingly to nbd_co_receive_request()

It wasn't -EIO, so according to nbd_co_receive_request()

> +         * semantics, we should return the error to the client. */
> +        Error *export_err = local_err;
> +
> +        local_err = NULL;
> +        ret = nbd_send_generic_reply(client, request.handle, -EINVAL,
> +                                     error_get_pretty(export_err), &local_err);
> +        error_free(export_err);
> +
> +        goto replied;
>       }
>   
>       switch (request.type) {
>       case NBD_CMD_READ:

> -        reply_data_len = request.len;
> +        ret = nbd_do_cmd_read(client, &request, req->data, &local_err);
>   
>           break;

We could also collapse the empty line before break.

>       case NBD_CMD_WRITE:
> @@ -1598,9 +1643,8 @@ static coroutine_fn void nbd_trip(void *opaque)
>           }
>           ret = blk_pwrite(exp->blk, request.from + exp->dev_offset,
>                            req->data, request.len, flags);
> -        if (ret < 0) {
> -            error_setg_errno(&local_err, -ret, "writing to file failed");
> -        }
> +        ret = nbd_send_generic_reply(client, request.handle, ret,
> +                                     "writing to file failed", &local_err);

I like how this works out.

>   
>           break;
>       default:
> -        error_setg(&local_err, "invalid request type (%" PRIu32 ") received",
> -                   request.type);
> -        ret = -EINVAL;
> -    }
> -
> -reply:
> -    if (local_err) {
> -        /* If we get here, local_err was not a fatal error, and should be sent
> -         * to the client. */
> -        assert(ret < 0);
> -        msg = g_strdup(error_get_pretty(local_err));
> -        error_report_err(local_err);
> -        local_err = NULL;
> +        msg = g_strdup_printf("invalid request type (%" PRIu32 ") received",
> +                              request.type);
> +        ret = nbd_send_generic_reply(client, request.handle, -EINVAL, msg,
> +                                     &local_err);
> +        g_free(msg);

I wonder if nbd_send_generic_reply() would be any easier to use if it 
took varargs and formatted the argument.  But for now, this is okay.

Only comment and grammar tweaks, so
Reviewed-by: Eric Blake <eblake@redhat.com>

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

[Qemu-devel] [PATCH 5/5] nbd/server: refactor nbd_trip: split out nbd_handle_request

[Vladimir Sementsov-Ogievskiy]

Split out request handling logic.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/server.c | 129 +++++++++++++++++++++++++++++++----------------------------
 1 file changed, 67 insertions(+), 62 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index a2f5f73d52..7b236f404e 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1584,17 +1584,79 @@ static coroutine_fn int nbd_do_cmd_read(NBDClient *client, NBDRequest *request,
     }
 }
 
+/* Handle NBD request.
+ * Return -errno if sending fails. Other errors are reported directly to the
+ * client as an error reply. */
+static coroutine_fn int nbd_handle_request(NBDClient *client,
+                                           NBDRequest *request,
+                                           uint8_t *data, Error **errp)
+{
+    int ret;
+    int flags;
+    NBDExport *exp = client->exp;
+    char *msg;
+
+    switch (request->type) {
+    case NBD_CMD_READ:
+        return nbd_do_cmd_read(client, request, data, errp);
+
+    case NBD_CMD_WRITE:
+        flags = 0;
+        if (request->flags & NBD_CMD_FLAG_FUA) {
+            flags |= BDRV_REQ_FUA;
+        }
+        ret = blk_pwrite(exp->blk, request->from + exp->dev_offset,
+                         data, request->len, flags);
+
+        return nbd_send_generic_reply(client, request->handle, ret,
+                                      "writing to file failed", errp);
+    case NBD_CMD_WRITE_ZEROES:
+        flags = 0;
+        if (request->flags & NBD_CMD_FLAG_FUA) {
+            flags |= BDRV_REQ_FUA;
+        }
+        if (!(request->flags & NBD_CMD_FLAG_NO_HOLE)) {
+            flags |= BDRV_REQ_MAY_UNMAP;
+        }
+        ret = blk_pwrite_zeroes(exp->blk, request->from + exp->dev_offset,
+                                request->len, flags);
+
+        return nbd_send_generic_reply(client, request->handle, ret,
+                                      "writing to file failed", errp);
+    case NBD_CMD_DISC:
+        /* unreachable, thanks to special case in nbd_co_receive_request() */
+        abort();
+
+    case NBD_CMD_FLUSH:
+        ret = blk_co_flush(exp->blk);
+
+        return nbd_send_generic_reply(client, request->handle, ret,
+                                      "flush failed", errp);
+    case NBD_CMD_TRIM:
+        ret = blk_co_pdiscard(exp->blk, request->from + exp->dev_offset,
+                              request->len);
+
+        return nbd_send_generic_reply(client, request->handle, ret,
+                                      "discard failed", errp);
+    default:
+        msg = g_strdup_printf("invalid request type (%" PRIu32 ") received",
+                              request->type);
+        ret = nbd_send_generic_reply(client, request->handle, -EINVAL, msg,
+                                     errp);
+        g_free(msg);
+
+        return ret;
+    }
+}
+
 /* Owns a reference to the NBDClient passed as opaque.  */
 static coroutine_fn void nbd_trip(void *opaque)
 {
     NBDClient *client = opaque;
-    NBDExport *exp = client->exp;
     NBDRequestData *req;
     NBDRequest request = { 0 };    /* GCC thinks it can be used uninitialized */
     int ret;
-    int flags;
     Error *local_err = NULL;
-    char *msg = NULL;
 
     trace_nbd_trip();
     if (client->closing) {
@@ -1627,66 +1689,9 @@ static coroutine_fn void nbd_trip(void *opaque)
         ret = nbd_send_generic_reply(client, request.handle, -EINVAL,
                                      error_get_pretty(export_err), &local_err);
         error_free(export_err);
-
-        goto replied;
-    }
-
-    switch (request.type) {
-    case NBD_CMD_READ:
-        ret = nbd_do_cmd_read(client, &request, req->data, &local_err);
-
-        break;
-    case NBD_CMD_WRITE:
-        flags = 0;
-        if (request.flags & NBD_CMD_FLAG_FUA) {
-            flags |= BDRV_REQ_FUA;
-        }
-        ret = blk_pwrite(exp->blk, request.from + exp->dev_offset,
-                         req->data, request.len, flags);
-        ret = nbd_send_generic_reply(client, request.handle, ret,
-                                     "writing to file failed", &local_err);
-
-        break;
-    case NBD_CMD_WRITE_ZEROES:
-        flags = 0;
-        if (request.flags & NBD_CMD_FLAG_FUA) {
-            flags |= BDRV_REQ_FUA;
-        }
-        if (!(request.flags & NBD_CMD_FLAG_NO_HOLE)) {
-            flags |= BDRV_REQ_MAY_UNMAP;
-        }
-        ret = blk_pwrite_zeroes(exp->blk, request.from + exp->dev_offset,
-                                request.len, flags);
-        ret = nbd_send_generic_reply(client, request.handle, ret,
-                                     "writing to file failed", &local_err);
-
-        break;
-    case NBD_CMD_DISC:
-        /* unreachable, thanks to special case in nbd_co_receive_request() */
-        abort();
-
-    case NBD_CMD_FLUSH:
-        ret = blk_co_flush(exp->blk);
-        ret = nbd_send_generic_reply(client, request.handle, ret,
-                                     "flush failed", &local_err);
-
-        break;
-    case NBD_CMD_TRIM:
-        ret = blk_co_pdiscard(exp->blk, request.from + exp->dev_offset,
-                              request.len);
-        ret = nbd_send_generic_reply(client, request.handle, ret,
-                                     "discard failed", &local_err);
-
-        break;
-    default:
-        msg = g_strdup_printf("invalid request type (%" PRIu32 ") received",
-                              request.type);
-        ret = nbd_send_generic_reply(client, request.handle, -EINVAL, msg,
-                                     &local_err);
-        g_free(msg);
+    } else {
+        ret = nbd_handle_request(client, &request, req->data, &local_err);
     }
-
-replied:
     if (ret < 0) {
         error_prepend(&local_err, "Failed to send reply: ");
         goto disconnect;
-- 
2.11.1

Re: [Qemu-devel] [PATCH 5/5] nbd/server: refactor nbd_trip: split out nbd_handle_request

[Eric Blake]

On 03/08/2018 12:46 PM, Vladimir Sementsov-Ogievskiy wrote:
> Split out request handling logic.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   nbd/server.c | 129 +++++++++++++++++++++++++++++++----------------------------
>   1 file changed, 67 insertions(+), 62 deletions(-)
> 

> +
> +    switch (request->type) {
> +    case NBD_CMD_READ:
> +        return nbd_do_cmd_read(client, request, data, errp);
> +
> +    case NBD_CMD_WRITE:
> +        flags = 0;
> +        if (request->flags & NBD_CMD_FLAG_FUA) {
> +            flags |= BDRV_REQ_FUA;
> +        }
> +        ret = blk_pwrite(exp->blk, request->from + exp->dev_offset,
> +                         data, request->len, flags);
> +
> +        return nbd_send_generic_reply(client, request->handle, ret,
> +                                      "writing to file failed", errp);
> +    case NBD_CMD_WRITE_ZEROES:

Inconsistent spacing between return and the next case label.

But switching whitespace is trivial, so

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [Qemu-devel] [PATCH 0/5] nbd server fixing and refactoring before BLOCK_STATUS

[Eric Blake]

On 03/08/2018 12:46 PM, Vladimir Sementsov-Ogievskiy wrote:
> 01 and 02 are splitted and updated "[PATCH] nbd/server: fix space read",
> others are new.
> 
> Vladimir Sementsov-Ogievskiy (5):
>    nbd/server: move nbd_co_send_structured_error up
>    nbd/server: fix sparse read
>    nbd/server: fix: check client->closing before reply sending
>    nbd/server: refactor nbd_trip: cmd_read and generic reply
>    nbd/server: refactor nbd_trip: split out nbd_handle_request

I had a tough time applying this one:

Applying: nbd/server: move nbd_co_send_structured_error up
Applying: nbd/server: fix: check client->closing before reply sending
Applying: nbd/server: refactor nbd_trip: cmd_read and generic reply
error: sha1 information is lacking or useless (nbd/server.c).
error: could not build fake ancestor
Patch failed at 0004 nbd/server: refactor nbd_trip: cmd_read and generic 
reply
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

I think I've resolved the conflicts correctly, but if I get through 
reviewing this series and posting it to my NBD queue, you may want to 
double-check things.

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

Re: [Qemu-devel] [PATCH 0/5] nbd server fixing and refactoring before BLOCK_STATUS

[Vladimir Sementsov-Ogievskiy]

09.03.2018 19:41, Eric Blake wrote:
> On 03/08/2018 12:46 PM, Vladimir Sementsov-Ogievskiy wrote:
>> 01 and 02 are splitted and updated "[PATCH] nbd/server: fix space read",
>> others are new.
>>
>> Vladimir Sementsov-Ogievskiy (5):
>>    nbd/server: move nbd_co_send_structured_error up
>>    nbd/server: fix sparse read
>>    nbd/server: fix: check client->closing before reply sending
>>    nbd/server: refactor nbd_trip: cmd_read and generic reply
>>    nbd/server: refactor nbd_trip: split out nbd_handle_request
>
> I had a tough time applying this one:
>
> Applying: nbd/server: move nbd_co_send_structured_error up
> Applying: nbd/server: fix: check client->closing before reply sending
> Applying: nbd/server: refactor nbd_trip: cmd_read and generic reply
> error: sha1 information is lacking or useless (nbd/server.c).
> error: could not build fake ancestor
> Patch failed at 0004 nbd/server: refactor nbd_trip: cmd_read and 
> generic reply
> The copy of the patch that failed is found in: .git/rebase-apply/patch
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
>
> I think I've resolved the conflicts correctly, but if I get through 
> reviewing this series and posting it to my NBD queue, you may want to 
> double-check things.
>

Oh, sorry for this. I should have to rebase it(

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 0/5] nbd server fixing and refactoring before BLOCK_STATUS

[Eric Blake]

On 03/09/2018 10:41 AM, Eric Blake wrote:
> On 03/08/2018 12:46 PM, Vladimir Sementsov-Ogievskiy wrote:
>> 01 and 02 are splitted and updated "[PATCH] nbd/server: fix space read",
>> others are new.
>>
>> Vladimir Sementsov-Ogievskiy (5):
>>    nbd/server: move nbd_co_send_structured_error up
>>    nbd/server: fix sparse read
>>    nbd/server: fix: check client->closing before reply sending
>>    nbd/server: refactor nbd_trip: cmd_read and generic reply
>>    nbd/server: refactor nbd_trip: split out nbd_handle_request
> 
> I had a tough time applying this one:
> 
> Applying: nbd/server: move nbd_co_send_structured_error up
> Applying: nbd/server: fix: check client->closing before reply sending
> Applying: nbd/server: refactor nbd_trip: cmd_read and generic reply
> error: sha1 information is lacking or useless (nbd/server.c).
> error: could not build fake ancestor
> Patch failed at 0004 nbd/server: refactor nbd_trip: cmd_read and generic 
> reply

Aha - I see my problem - the patches were applied out of order because 
only patch 2 had a 'v2' in the subject line.  If I tell git to apply 
them one at a time, instead of trying to apply them on 'maildir/*' where 
the sorting botches the ordering, things work better.

> I think I've resolved the conflicts correctly, but if I get through 
> reviewing this series and posting it to my NBD queue, you may want to 
> double-check things.

No need to repost this series, I'm doing a lot better now that I see my 
mistake.

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

Re: [Qemu-devel] [PATCH 0/5] nbd server fixing and refactoring before BLOCK_STATUS

[Eric Blake]

On 03/08/2018 12:46 PM, Vladimir Sementsov-Ogievskiy wrote:
> 01 and 02 are splitted and updated "[PATCH] nbd/server: fix space read",
> others are new.
> 
> Vladimir Sementsov-Ogievskiy (5):
>    nbd/server: move nbd_co_send_structured_error up
>    nbd/server: fix sparse read
>    nbd/server: fix: check client->closing before reply sending
>    nbd/server: refactor nbd_trip: cmd_read and generic reply
>    nbd/server: refactor nbd_trip: split out nbd_handle_request
> 

Thanks; queued to my NBD tree, with tweaks made per discussion on the 
various patches.

Pull request should make soft freeze for 2.12.

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