nbd: read_sync and friends: return 0 on success

functions read_sync, drop_sync, write_sync, and also
nbd_negotiate_write, nbd_negotiate_read, nbd_negotiate_drop_sync
returns number of processed bytes. But what this number can be,
except requested number of bytes?

Actually, underlying nbd_wr_syncv function returns a value >= 0 and
!= requested_bytes only on eof on read operation. So, firstly, it is
impossible on write (let's add an assert) and on read it actually
means, that communication is broken (except nbd_receive_reply, see
below).

Most of callers operate like this:
   if (func(..., size) != size) {
       /* error path */
   }
, i.e.:
  1. They are not interested in partial success
  2. Extra duplications in code (especially bad are duplications of
     magic numbers)
  3. User doesn't see actual error message, as return code is lost.
     (this patch doesn't fix this point, but it makes fixing easier)

Several callers handles ret >= 0 and != requested-size separately, by
just returning EINVAL in this case. This patch makes read_sync and
friends return EINVAL in this case, so final behavior is the same.

And only one caller - nbd_receive_reply() does something not so
obvious. It returns EINVAL for ret > 0 and != requested-size, like
previous group, but for ret == 0 it returns 0. The only caller of
nbd_receive_reply() - nbd_read_reply_entry() handles ret == 0 in the
same way as ret < 0, so for now it doesn't matter. However, in
following commits error path handling will be improved and we'll need
to distinguish success from fail in this case too. So, this patch adds
separate helper for this case - read_sync_eof.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20170516094533.6160-3-vsementsov@virtuozzo.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

1 files changed, 25 insertions, 38 deletions

diff --git a/nbd/client.c b/nbd/client.c
index a58fb02cb4..6b74a628f1 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -86,9 +86,9 @@ static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
 
 */
 
-/* Discard length bytes from channel.  Return -errno on failure, or
- * the amount of bytes consumed. */
-static ssize_t drop_sync(QIOChannel *ioc, size_t size)
+/* Discard length bytes from channel.  Return -errno on failure and 0 on
+ * success*/
+static int drop_sync(QIOChannel *ioc, size_t size)
 {
     ssize_t ret = 0;
     char small[1024];
@@ -96,14 +96,13 @@ static ssize_t drop_sync(QIOChannel *ioc, size_t size)
 
     buffer = sizeof(small) >= size ? small : g_malloc(MIN(65536, size));
     while (size > 0) {
-        ssize_t count = read_sync(ioc, buffer, MIN(65536, size));
+        ssize_t count = MIN(65536, size);
+        ret = read_sync(ioc, buffer, MIN(65536, size));
 
-        if (count <= 0) {
+        if (ret < 0) {
             goto cleanup;
         }
-        assert(count <= size);
         size -= count;
-        ret += count;
     }
 
  cleanup:
@@ -136,12 +135,12 @@ static int nbd_send_option_request(QIOChannel *ioc, uint32_t opt,
     stl_be_p(&req.option, opt);
     stl_be_p(&req.length, len);
 
-    if (write_sync(ioc, &req, sizeof(req)) != sizeof(req)) {
+    if (write_sync(ioc, &req, sizeof(req)) < 0) {
         error_setg(errp, "Failed to send option request header");
         return -1;
     }
 
-    if (len && write_sync(ioc, (char *) data, len) != len) {
+    if (len && write_sync(ioc, (char *) data, len) < 0) {
         error_setg(errp, "Failed to send option request data");
         return -1;
     }
@@ -170,7 +169,7 @@ static int nbd_receive_option_reply(QIOChannel *ioc, uint32_t opt,
                                     nbd_opt_reply *reply, Error **errp)
 {
     QEMU_BUILD_BUG_ON(sizeof(*reply) != 20);
-    if (read_sync(ioc, reply, sizeof(*reply)) != sizeof(*reply)) {
+    if (read_sync(ioc, reply, sizeof(*reply)) < 0) {
         error_setg(errp, "failed to read option reply");
         nbd_send_opt_abort(ioc);
         return -1;
@@ -219,7 +218,7 @@ static int nbd_handle_reply_err(QIOChannel *ioc, nbd_opt_reply *reply,
             goto cleanup;
         }
         msg = g_malloc(reply->length + 1);
-        if (read_sync(ioc, msg, reply->length) != reply->length) {
+        if (read_sync(ioc, msg, reply->length) < 0) {
             error_setg(errp, "failed to read option error message");
             goto cleanup;
         }
@@ -321,7 +320,7 @@ static int nbd_receive_list(QIOChannel *ioc, const char *want, bool *match,
         nbd_send_opt_abort(ioc);
         return -1;
     }
-    if (read_sync(ioc, &namelen, sizeof(namelen)) != sizeof(namelen)) {
+    if (read_sync(ioc, &namelen, sizeof(namelen)) < 0) {
         error_setg(errp, "failed to read option name length");
         nbd_send_opt_abort(ioc);
         return -1;
@@ -334,7 +333,7 @@ static int nbd_receive_list(QIOChannel *ioc, const char *want, bool *match,
         return -1;
     }
     if (namelen != strlen(want)) {
-        if (drop_sync(ioc, len) != len) {
+        if (drop_sync(ioc, len) < 0) {
             error_setg(errp, "failed to skip export name with wrong length");
             nbd_send_opt_abort(ioc);
             return -1;
@@ -343,14 +342,14 @@ static int nbd_receive_list(QIOChannel *ioc, const char *want, bool *match,
     }
 
     assert(namelen < sizeof(name));
-    if (read_sync(ioc, name, namelen) != namelen) {
+    if (read_sync(ioc, name, namelen) < 0) {
         error_setg(errp, "failed to read export name");
         nbd_send_opt_abort(ioc);
         return -1;
     }
     name[namelen] = '\0';
     len -= namelen;
-    if (drop_sync(ioc, len) != len) {
+    if (drop_sync(ioc, len) < 0) {
         error_setg(errp, "failed to read export description");
         nbd_send_opt_abort(ioc);
         return -1;
@@ -477,7 +476,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
         goto fail;
     }
 
-    if (read_sync(ioc, buf, 8) != 8) {
+    if (read_sync(ioc, buf, 8) < 0) {
         error_setg(errp, "Failed to read data");
         goto fail;
     }
@@ -503,7 +502,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
         goto fail;
     }
 
-    if (read_sync(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
+    if (read_sync(ioc, &magic, sizeof(magic)) < 0) {
         error_setg(errp, "Failed to read magic");
         goto fail;
     }
@@ -515,8 +514,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
         uint16_t globalflags;
         bool fixedNewStyle = false;
 
-        if (read_sync(ioc, &globalflags, sizeof(globalflags)) !=
-            sizeof(globalflags)) {
+        if (read_sync(ioc, &globalflags, sizeof(globalflags)) < 0) {
             error_setg(errp, "Failed to read server flags");
             goto fail;
         }
@@ -534,8 +532,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
         }
         /* client requested flags */
         clientflags = cpu_to_be32(clientflags);
-        if (write_sync(ioc, &clientflags, sizeof(clientflags)) !=
-            sizeof(clientflags)) {
+        if (write_sync(ioc, &clientflags, sizeof(clientflags)) < 0) {
             error_setg(errp, "Failed to send clientflags field");
             goto fail;
         }
@@ -573,13 +570,13 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
         }
 
         /* Read the response */
-        if (read_sync(ioc, &s, sizeof(s)) != sizeof(s)) {
+        if (read_sync(ioc, &s, sizeof(s)) < 0) {
             error_setg(errp, "Failed to read export length");
             goto fail;
         }
         *size = be64_to_cpu(s);
 
-        if (read_sync(ioc, flags, sizeof(*flags)) != sizeof(*flags)) {
+        if (read_sync(ioc, flags, sizeof(*flags)) < 0) {
             error_setg(errp, "Failed to read export flags");
             goto fail;
         }
@@ -596,14 +593,14 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
             goto fail;
         }
 
-        if (read_sync(ioc, &s, sizeof(s)) != sizeof(s)) {
+        if (read_sync(ioc, &s, sizeof(s)) < 0) {
             error_setg(errp, "Failed to read export length");
             goto fail;
         }
         *size = be64_to_cpu(s);
         TRACE("Size is %" PRIu64, *size);
 
-        if (read_sync(ioc, &oldflags, sizeof(oldflags)) != sizeof(oldflags)) {
+        if (read_sync(ioc, &oldflags, sizeof(oldflags)) < 0) {
             error_setg(errp, "Failed to read export flags");
             goto fail;
         }
@@ -619,7 +616,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
     }
 
     TRACE("Size is %" PRIu64 ", export flags %" PRIx16, *size, *flags);
-    if (zeroes && drop_sync(ioc, 124) != 124) {
+    if (zeroes && drop_sync(ioc, 124) < 0) {
         error_setg(errp, "Failed to read reserved block");
         goto fail;
     }
@@ -744,7 +741,6 @@ int nbd_disconnect(int fd)
 ssize_t nbd_send_request(QIOChannel *ioc, NBDRequest *request)
 {
     uint8_t buf[NBD_REQUEST_SIZE];
-    ssize_t ret;
 
     TRACE("Sending request to server: "
           "{ .from = %" PRIu64", .len = %" PRIu32 ", .handle = %" PRIu64
@@ -759,16 +755,7 @@ ssize_t nbd_send_request(QIOChannel *ioc, NBDRequest *request)
     stq_be_p(buf + 16, request->from);
     stl_be_p(buf + 24, request->len);
 
-    ret = write_sync(ioc, buf, sizeof(buf));
-    if (ret < 0) {
-        return ret;
-    }
-
-    if (ret != sizeof(buf)) {
-        LOG("writing to socket failed");
-        return -EINVAL;
-    }
-    return 0;
+    return write_sync(ioc, buf, sizeof(buf));
 }
 
 ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply)
@@ -777,7 +764,7 @@ ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply)
     uint32_t magic;
     ssize_t ret;
 
-    ret = read_sync(ioc, buf, sizeof(buf));
+    ret = read_sync_eof(ioc, buf, sizeof(buf));
     if (ret <= 0) {
         return ret;
     }