[Top][All Lists]

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

Re: [Qemu-block] [PATCH] nbd/server: fix space read

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [Qemu-block] [PATCH] nbd/server: fix space read
Date: Thu, 8 Mar 2018 18:20:09 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0

08.03.2018 18:17, Vladimir Sementsov-Ogievskiy wrote:
08.03.2018 14:50, Vladimir Sementsov-Ogievskiy wrote:
05.03.2018 22:47, Eric Blake wrote:
On 03/05/2018 12:04 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
error reply, representing channel-io error on previous try to
send a reply.

Good catch.

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.

Note: nbd_co_send_structured_error is moved without changes to be
called from nbd_co_send_sparse_read.

Might be easier to read as two patches, one for the code motion, the other for using the new code.  But I'm not going to insist on a split.

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

PS: this all shows that nbd_trip is tooo complex (and yes, I remember,
that its final semantics was made by myself :(.. It should be
refactored into several smaller parts. Do you have any ideas?

The complexity here is that we should handle channel errors (fatal) and export errors(non fatal) in different ways, and both of error types may
have errp, which should be handled differently too..

May be, the best way is to make separate functions for each command,
avoiding code duplication by using helper-functions instead of common code
in nbd_trip.

Yes, splitting nbd_trip into smaller helper functions may be worthwhile.

  nbd/server.c | 64 +++++++++++++++++++++++++++++++++++-------------------------
  1 file changed, 37 insertions(+), 27 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 4990a5826e..ea6b9467e4 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -21,6 +21,7 @@
  #include "qapi/error.h"
  #include "trace.h"
  #include "nbd-internal.h"
+#include "qemu/error-report.h"

I'm a bit worried about this one.  The server has previously not needed this, so it may be the wrong thing to pull it in now.

  static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client,
                                                  uint64_t handle,
                                                  uint64_t offset,

It doesn't help that we are lacking comments on the contract of this function.

@@ -1361,8 +1386,15 @@ 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));

Indentation looks off.

+            error_report("%s", msg);

Do we really need to report this on the server's stderr, or is sending the reply to the client good enough?

I'm just trying to save current behavior in nbd_trip... Why not? Is it fair that client knows about some strange io error on server-side, but server logs lacks this information?

May be, it worth switching it from error_report to trace, but it should be a separate patch, changing all places around.

stop, it is already traced in nbd_co_send_simple_reply and nbd_co_send_structured_error. So, we can safely drop it.

and it needs "qemu/error-report.h".. nbd_trip uses error_report_err instead. error_report_err calls error_report anyway.

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

So if we have an early return here, then pre-patch, we were unconditionally setting errp and returning -1 (even if the client is still alive); post-patch errp is only set if we failed to do I/O with the client.  That change is right, but harder to see without comments giving a function contract.

@@ -1567,7 +1575,7 @@ static coroutine_fn void nbd_trip(void *opaque)
                                            request.from, req->data, request.len,
              if (ret < 0) {
-                goto reply;
+                goto replied;
              goto done;
@@ -1664,6 +1672,8 @@ reply:
                                         req->data, reply_data_len, &local_err);

This part makes sense.

      if (ret < 0) {
          error_prepend(&local_err, "Failed to send reply: ");
          goto disconnect;

Best regards,

reply via email to

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