qemu-devel
[Top][All Lists]
Advanced

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

Re: [PATCH v2 for-7.1 3/9] nbd: remove peppering of nbd_client_connected


From: Paolo Bonzini
Subject: Re: [PATCH v2 for-7.1 3/9] nbd: remove peppering of nbd_client_connected
Date: Sat, 23 Apr 2022 10:30:49 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.0

Hi,

thanks for the careful review and sorry I'm only replying now.

On 4/15/22 19:01, Vladimir Sementsov-Ogievskiy wrote:
@@ -982,11 +978,6 @@ static bool nbd_reply_chunk_iter_receive(BDRVNBDState *s,
      NBDReply local_reply;
      NBDStructuredReplyChunk *chunk;
      Error *local_err = NULL;
-    if (!nbd_client_connected(s)) {
-        error_setg(&local_err, "Connection closed");
-        nbd_iter_channel_error(iter, -EIO, &local_err);
-        goto break_loop;
-    }

Probably we should still check iter->ret here. It's strange to start new iteration, when user set iter->ret in previous iteration of NBD_FOREACH_REPLY_CHUNK()

Or, maybe we should set iter->done in nbd_iter_channel_error ?

Yes, this second one is a possibility. I chose to check iter->ret below because it was a bit more self-contained ("before reading again check that the error code is not overwritten"), but it is also less obvious.

Paolo

      if (iter->done) {
          /* Previous iteration was last. */
@@ -1007,7 +998,7 @@ static bool nbd_reply_chunk_iter_receive(BDRVNBDState *s,
      }
      /* Do not execute the body of NBD_FOREACH_REPLY_CHUNK for simple reply. */
-    if (nbd_reply_is_simple(reply) || !nbd_client_connected(s)) {
+    if (nbd_reply_is_simple(reply) || iter->ret < 0) {

And then here, may be we can just goto break_loop from previous "if (ret < 0)". Then we'll not have to check iter->ret.

          goto break_loop;
      }

anyway:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>

(a bit weak, as it really hard to imagine all these paths and possible consequences :/





reply via email to

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