qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH for-4.0] nbd: Permit simple error to NBD_CMD_BLO


From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH for-4.0] nbd: Permit simple error to NBD_CMD_BLOCK_STATUS
Date: Mon, 25 Mar 2019 11:05:11 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.0

On 3/25/19 9:44 AM, Eric Blake wrote:

>>>
>>> @@ -758,9 +756,11 @@ static int 
>>> nbd_co_receive_blockstatus_reply(NBDClientSession *s,
>>>           payload = NULL;
>>>       }
>>>
>>> -    if (!extent->length && !iter.err) {
>>> -        error_setg(&iter.err,
>>> -                   "Server did not reply with any status extents");
>>> +    if (!extent->length && !iter.request_ret) {
>>
>> Hmm, I don't see, what is changed. You just don't set ret and err if there 
>> already
>> set request_ret.. So, finally it may lead to other return code in 
>> nbd_client_co_block_status
>> and local_err will not be set and traced. But there is no connection close 
>> in this case
>> in current code, s->quit is not set.
> 
> And that's okay. The whole point of this is that if the server replied
> with an error message (iter.request_ret), then it is okay that the
> server did not give us any extent->length, and the caller will turn
> iter.request_ret into the EINVAL reply (or whatever other errno value
> the server sent) to .bdrv_co_block_status() while keeping the connection
> alive.  But if the server did NOT send an error reply (iter.request_ret
> is 0), but also did not send us any status, then we need to turn that
> into an error condition (because our caller expected us to make progress
> on success, even though the server did not make progress).

More to the point, the behavior of qemu for a (structured) error reply
to NBD_CMD_BLOCK_STATUS with no extent->length was to keep the
connection alive (both before and after commit 7f86068d) - the
difference in behavior for this hunk of the patch is only visible for
simple error replies. (Here's the hacks I applied to the server, to test
forced error replies to NBD_CMD_BLOCK_STATUS, whether structured or simple:


From 22b4181f887d3b782695dbb11fcc1759281fc51e Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Sat, 23 Mar 2019 07:19:33 -0500
Subject: [PATCH 1/2] nbd: Debug patch to force NBD_CMD_BLOCK_STATUS failure

Hack to test whether the client is robust to block status failure.

Signed-off-by: Eric Blake <address@hidden>
---
 nbd/server.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/nbd/server.c b/nbd/server.c
index fd013a2817a..94d0c9f4e9e 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2269,6 +2269,8 @@ static coroutine_fn int
nbd_handle_request(NBDClient *client,
                                       "discard failed", errp);

     case NBD_CMD_BLOCK_STATUS:
+        return nbd_send_generic_reply(client, request->handle, -EINVAL,
+                                      "no status for you today", errp);
         if (!request->len) {
             return nbd_send_generic_reply(client, request->handle, -EINVAL,
                                           "need non-zero length", errp);
-- 
2.20.1


From 1f3076c31deee0f226c6af911a0b636f5cbb7faf Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Sat, 23 Mar 2019 07:19:33 -0500
Subject: [PATCH 2/2] nbd: Debug patch to force simple errors to all but
 NBD_CMD_READ

When structured replies are negotiated, the NBD spec still allows
simple error replies to any command other than READ.  Make it easy to
test this scenario.

Signed-off-by: Eric Blake <address@hidden>
---
 nbd/server.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 94d0c9f4e9e..35f549abbf9 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2148,16 +2148,16 @@ static int nbd_co_receive_request(NBDRequestData
*req, NBDRequest *request,
  * Returns 0 if connection is still live, -errno on failure to talk to
client
  */
 static coroutine_fn int nbd_send_generic_reply(NBDClient *client,
-                                               uint64_t handle,
+                                               NBDRequest *request,
                                                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,
+    if (client->structured_reply && ret < 0 && request->type ==
NBD_CMD_READ) {
+        return nbd_co_send_structured_error(client, request->handle,
-ret, error_msg,
                                             errp);
     } else {
-        return nbd_co_send_simple_reply(client, handle, ret < 0 ? -ret : 0,
+        return nbd_co_send_simple_reply(client, request->handle, ret <
0 ? -ret : 0,
                                         NULL, 0, errp);
     }
 }
@@ -2177,7 +2177,7 @@ static coroutine_fn int nbd_do_cmd_read(NBDClient
*client, NBDRequest *request,
     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,
+            return nbd_send_generic_reply(client, request, ret,
                                           "flush failed", errp);
         }
     }
@@ -2192,7 +2192,7 @@ static coroutine_fn int nbd_do_cmd_read(NBDClient
*client, NBDRequest *request,
     ret = blk_pread(exp->blk, request->from + exp->dev_offset, data,
                     request->len);
     if (ret < 0 || request->type == NBD_CMD_CACHE) {
-        return nbd_send_generic_reply(client, request->handle, ret,
+        return nbd_send_generic_reply(client, request, ret,
                                       "reading from file failed", errp);
     }

@@ -2234,7 +2234,7 @@ static coroutine_fn int
nbd_handle_request(NBDClient *client,
         }
         ret = blk_pwrite(exp->blk, request->from + exp->dev_offset,
                          data, request->len, flags);
-        return nbd_send_generic_reply(client, request->handle, ret,
+        return nbd_send_generic_reply(client, request, ret,
                                       "writing to file failed", errp);

     case NBD_CMD_WRITE_ZEROES:
@@ -2247,7 +2247,7 @@ static coroutine_fn int
nbd_handle_request(NBDClient *client,
         }
         ret = blk_pwrite_zeroes(exp->blk, request->from + exp->dev_offset,
                                 request->len, flags);
-        return nbd_send_generic_reply(client, request->handle, ret,
+        return nbd_send_generic_reply(client, request, ret,
                                       "writing to file failed", errp);

     case NBD_CMD_DISC:
@@ -2256,7 +2256,7 @@ static coroutine_fn int
nbd_handle_request(NBDClient *client,

     case NBD_CMD_FLUSH:
         ret = blk_co_flush(exp->blk);
-        return nbd_send_generic_reply(client, request->handle, ret,
+        return nbd_send_generic_reply(client, request, ret,
                                       "flush failed", errp);

     case NBD_CMD_TRIM:
@@ -2265,14 +2265,14 @@ static coroutine_fn int
nbd_handle_request(NBDClient *client,
         if (ret == 0 && request->flags & NBD_CMD_FLAG_FUA) {
             ret = blk_co_flush(exp->blk);
         }
-        return nbd_send_generic_reply(client, request->handle, ret,
+        return nbd_send_generic_reply(client, request, ret,
                                       "discard failed", errp);

     case NBD_CMD_BLOCK_STATUS:
-        return nbd_send_generic_reply(client, request->handle, -EINVAL,
+        return nbd_send_generic_reply(client, request, -EINVAL,
                                       "no status for you today", errp);
         if (!request->len) {
-            return nbd_send_generic_reply(client, request->handle, -EINVAL,
+            return nbd_send_generic_reply(client, request, -EINVAL,
                                           "need non-zero length", errp);
         }
         if (client->export_meta.valid &&
@@ -2306,7 +2306,7 @@ static coroutine_fn int
nbd_handle_request(NBDClient *client,

             return ret;
         } else {
-            return nbd_send_generic_reply(client, request->handle, -EINVAL,
+            return nbd_send_generic_reply(client, request, -EINVAL,
                                           "CMD_BLOCK_STATUS not
negotiated",
                                           errp);
         }
@@ -2314,7 +2314,7 @@ static coroutine_fn int
nbd_handle_request(NBDClient *client,
     default:
         msg = g_strdup_printf("invalid request type (%" PRIu32 ")
received",
                               request->type);
-        ret = nbd_send_generic_reply(client, request->handle, -EINVAL, msg,
+        ret = nbd_send_generic_reply(client, request, -EINVAL, msg,
                                      errp);
         g_free(msg);
         return ret;
@@ -2359,7 +2359,7 @@ static coroutine_fn void nbd_trip(void *opaque)
         Error *export_err = local_err;

         local_err = NULL;
-        ret = nbd_send_generic_reply(client, request.handle, -EINVAL,
+        ret = nbd_send_generic_reply(client, &request, -EINVAL,
                                      error_get_pretty(export_err),
&local_err);
         error_free(export_err);
     } else {
-- 
2.20.1


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

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

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