qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH 02/17] nbd/client: refactor nbd_read_eof


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [Qemu-devel] [PATCH 02/17] nbd/client: refactor nbd_read_eof
Date: Mon, 7 Aug 2017 15:05:46 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1

07.08.2017 14:42, Eric Blake wrote:
On 08/04/2017 10:14 AM, Vladimir Sementsov-Ogievskiy wrote:
Refactor nbd_read_eof to return 1 on success, 0 on eof, when no
data was read and <0 for other cases, because returned size of
read data is not actually used.

Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
---
  nbd/nbd-internal.h         | 34 +++++++++++++++++++++++++---------
  nbd/client.c               |  5 -----
  tests/qemu-iotests/083.out |  4 ++--
  3 files changed, 27 insertions(+), 16 deletions(-)

diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index 396ddb4d3e..3fb0b6098a 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -77,21 +77,37 @@
  #define NBD_ESHUTDOWN  108
/* nbd_read_eof
- * Tries to read @size bytes from @ioc. Returns number of bytes actually read.
- * May return a value >= 0 and < size only on EOF, i.e. when iteratively called
- * qio_channel_readv() returns 0. So, there is no need to call nbd_read_eof
- * iteratively.
+ * Tries to read @size bytes from @ioc.
+ * Returns 1 on success
+ *         0 on eof, when no data was read (errp is not set)
+ *         -EINVAL on eof, when some data < @size was read until eof
+ *         < 0 on read fail
In general, mixing negative errno value and generic < 0 in the same
function is most likely ambiguous.

Hmm, but this is entirely what we do so often:

if (,,) return -EINVAL;

return some_other_func().

last two lines may be rewritten like this:
+ * < 0 on fail


   */
-static inline ssize_t nbd_read_eof(QIOChannel *ioc, void *buffer, size_t size,
-                                   Error **errp)
+static inline int nbd_read_eof(QIOChannel *ioc, void *buffer, size_t size,
+                               Error **errp)
  {
      struct iovec iov = { .iov_base = buffer, .iov_len = size };
+    ssize_t ret;
+
      /* Sockets are kept in blocking mode in the negotiation phase.  After
       * that, a non-readable socket simply means that another thread stole
       * our request/reply.  Synchronization is done with recv_coroutine, so
       * that this is coroutine-safe.
       */
-    return nbd_rwv(ioc, &iov, 1, size, true, errp);
+
+    assert(size > 0);
Effectively the same as assert(size != 0).

+
+    ret = nbd_rwv(ioc, &iov, 1, size, true, errp);
+    if (ret <= 0) {
+        return ret;
+    }
So this is a negative errno (or 0 on EOF),

if it is < 0, it can be only -EIO, specified in nbd_rwv "by hand". it is unrelated to read read/write errno's


+
+    if (ret != size) {
+        error_setg(errp, "End of file");
+        return -EINVAL;
and so is this. Which makes the function documentation not quite
accurate; you aren't mixing a generic < 0.

hmm.. my wordings are weird sometimes, sorry for that :(. and thank you for your patience.


+    }
+
+    return 1;
  }
/* nbd_read
@@ -100,9 +116,9 @@ static inline ssize_t nbd_read_eof(QIOChannel *ioc, void 
*buffer, size_t size,
  static inline int nbd_read(QIOChannel *ioc, void *buffer, size_t size,
                             Error **errp)
  {
-    ssize_t ret = nbd_read_eof(ioc, buffer, size, errp);
+    int ret = nbd_read_eof(ioc, buffer, size, errp);
- if (ret >= 0 && ret != size) {
+    if (ret == 0) {
          ret = -EINVAL;
          error_setg(errp, "End of file");
Why do we have to set errp here instead of in nbd_read_eof()? Is there
ever any case where hitting early EOF is not something that should be
treated as an error?

yes. it is the only usage of nbd_read_eof - in nbd_receive_reply. This used to understand that there no more replies to read.


      }
diff --git a/nbd/client.c b/nbd/client.c
index f1c16b588f..4556056daa 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -925,11 +925,6 @@ ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply 
*reply, Error **errp)
          return ret;
      }
- if (ret != sizeof(buf)) {
-        error_setg(errp, "read failed");
-        return -EINVAL;
-    }
-
      /* Reply
         [ 0 ..  3]    magic   (NBD_REPLY_MAGIC)
         [ 4 ..  7]    error   (0 == no error)
diff --git a/tests/qemu-iotests/083.out b/tests/qemu-iotests/083.out
index a24c6bfece..d3bea1b2f5 100644
--- a/tests/qemu-iotests/083.out
+++ b/tests/qemu-iotests/083.out
@@ -69,12 +69,12 @@ read failed: Input/output error
=== Check disconnect 4 reply === -read failed
+End of file
  read failed: Input/output error
At least you tracked that your changes tweak the error message.  But I'm
not yet convinced whether this change simplifies anything.  Is there a
later patch that is easier to write with the new semantics which was not
possible with the pre-patch semantics?

This patch just moves error handling one level down (do not propagate unused information). And removes (with the following patch) last remains of ssize_t and returning number of bytes in nbd/client.c - for consistency.
Let nbd_rwv  to be the only function returning number of bytes.



--
Best regards,
Vladimir



reply via email to

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