[Qemu-devel] [PATCH v2 00/12] nbd refactoring part 1

[Qemu-devel] [PATCH v2 00/12] nbd refactoring part 1

[Vladimir Sementsov-Ogievskiy]

This is based on my "nbd: error path refactoring" series.
This is an update of first part of my "nbd errors and traces refactoring"

v2:
01: update commit msg
    grammar in comment
    add Eric's r-b
02: add Eric's r-b
03: update commit msg
    add Eric's r-b


v1:
changes from patches 01-10 of "nbd errors and traces refactoring" (mostly
proposed by Eric):
01 new patch
02-03 - split of old 01
08 and 12 - minor changes in commit messages
04-12 - add r-b by Eric

Also, of course, renaming in 01 is "visible" in other patches.

Vladimir Sementsov-Ogievskiy (12):
  nbd: rename read_sync and friends
  nbd: make nbd_drop public
  nbd/server: get rid of nbd_negotiate_read and friends
  nbd/server: get rid of ssize_t
  nbd/server: refactor nbd_co_send_reply
  nbd/server: get rid of EAGAIN dead code
  nbd/server: refactor nbd_co_receive_request
  nbd/server: remove NBDClientNewData
  nbd/server: nbd_negotiate: fix error path
  nbd/server: get rid of fail: return rc
  nbd/server: rename rc to ret
  nbd/server: refactor nbd_trip

 block/nbd-client.c  |   8 +-
 include/block/nbd.h |   8 +-
 nbd/client.c        |  64 ++++-------
 nbd/common.c        |  34 +++++-
 nbd/nbd-internal.h  |  28 ++---
 nbd/server.c        | 319 +++++++++++++++++-----------------------------------
 6 files changed, 172 insertions(+), 289 deletions(-)

-- 
2.11.1

[Qemu-devel] [PATCH v2 01/12] nbd: rename read_sync and friends

[Vladimir Sementsov-Ogievskiy]

Rename
  nbd_wr_syncv -> nbd_rwv
  read_sync -> nbd_read
  read_sync_eof -> nbd_read_eof
  write_sync -> nbd_write
  drop_sync -> nbd_drop

1. nbd_ prefix
   read_sync and write_sync are already shared, so it is good to have a
   namespace prefix. drop_sync will be shared, and read_sync_eof is
   related to read_sync, so let's rename them all.

2. _sync suffix
   _sync is related to the fact that nbd_wr_syncv doesn't return if a
   write to socket returns EAGAIN. The first implementation of
   nbd_wr_syncv (was wr_sync in 7a5ca8648b) just loops while getting
   EAGAIN, the current implementation yields in this case.
   Why we want to get rid of it:
   - it is normal for r/w functions to be synchronous, so having an
     additional suffix for it looks redundant (contrariwise, we have
     _aio suffix for async functions)
   - _sync suffix in block layer is used when function does flush (so
     using it for other thing is confusing a bit)
   - keep function names short after adding nbd_ prefix

3. for nbd_wr_syncv let's use more common notation 'rw'

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/nbd-client.c  |  8 ++++----
 include/block/nbd.h |  8 ++------
 nbd/client.c        | 42 +++++++++++++++++++++---------------------
 nbd/common.c        |  8 ++------
 nbd/nbd-internal.h  | 26 +++++++++++++-------------
 nbd/server.c        | 12 ++++++------
 6 files changed, 48 insertions(+), 56 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 09d955bc4d..3e080ca7f0 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -140,8 +140,8 @@ static int nbd_co_send_request(BlockDriverState *bs,
         qio_channel_set_cork(s->ioc, true);
         rc = nbd_send_request(s->ioc, request);
         if (rc >= 0) {
-            ret = nbd_wr_syncv(s->ioc, qiov->iov, qiov->niov, request->len,
-                               false, NULL);
+            ret = nbd_rwv(s->ioc, qiov->iov, qiov->niov, request->len, false,
+                          NULL);
             if (ret != request->len) {
                 rc = -EIO;
             }
@@ -169,8 +169,8 @@ static void nbd_co_receive_reply(NBDClientSession *s,
         reply->error = EIO;
     } else {
         if (qiov && reply->error == 0) {
-            ret = nbd_wr_syncv(s->ioc, qiov->iov, qiov->niov, request->len,
-                               true, NULL);
+            ret = nbd_rwv(s->ioc, qiov->iov, qiov->niov, request->len, true,
+                          NULL);
             if (ret != request->len) {
                 reply->error = EIO;
             }
diff --git a/include/block/nbd.h b/include/block/nbd.h
index e0c64e4981..0a5ecd0e5c 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -123,12 +123,8 @@ enum {
  * aren't overflowing some other buffer. */
 #define NBD_MAX_NAME_SIZE 256
 
-ssize_t nbd_wr_syncv(QIOChannel *ioc,
-                     struct iovec *iov,
-                     size_t niov,
-                     size_t length,
-                     bool do_read,
-                     Error **errp);
+ssize_t nbd_rwv(QIOChannel *ioc, struct iovec *iov, size_t niov, size_t length,
+                bool do_read, Error **errp);
 int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
                           QCryptoTLSCreds *tlscreds, const char *hostname,
                           QIOChannel **outioc,
diff --git a/nbd/client.c b/nbd/client.c
index f9e1d75be4..08050c39b3 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -88,7 +88,7 @@ static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
 
 /* Discard length bytes from channel.  Return -errno on failure and 0 on
  * success*/
-static int drop_sync(QIOChannel *ioc, size_t size, Error **errp)
+static int nbd_drop(QIOChannel *ioc, size_t size, Error **errp)
 {
     ssize_t ret = 0;
     char small[1024];
@@ -97,7 +97,7 @@ static int drop_sync(QIOChannel *ioc, size_t size, Error **errp)
     buffer = sizeof(small) >= size ? small : g_malloc(MIN(65536, size));
     while (size > 0) {
         ssize_t count = MIN(65536, size);
-        ret = read_sync(ioc, buffer, MIN(65536, size), errp);
+        ret = nbd_read(ioc, buffer, MIN(65536, size), errp);
 
         if (ret < 0) {
             goto cleanup;
@@ -135,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), errp) < 0) {
+    if (nbd_write(ioc, &req, sizeof(req), errp) < 0) {
         error_prepend(errp, "Failed to send option request header");
         return -1;
     }
 
-    if (len && write_sync(ioc, (char *) data, len, errp) < 0) {
+    if (len && nbd_write(ioc, (char *) data, len, errp) < 0) {
         error_prepend(errp, "Failed to send option request data");
         return -1;
     }
@@ -169,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), errp) < 0) {
+    if (nbd_read(ioc, reply, sizeof(*reply), errp) < 0) {
         error_prepend(errp, "failed to read option reply");
         nbd_send_opt_abort(ioc);
         return -1;
@@ -218,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, errp) < 0) {
+        if (nbd_read(ioc, msg, reply->length, errp) < 0) {
             error_prepend(errp, "failed to read option error message");
             goto cleanup;
         }
@@ -320,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), errp) < 0) {
+    if (nbd_read(ioc, &namelen, sizeof(namelen), errp) < 0) {
         error_prepend(errp, "failed to read option name length");
         nbd_send_opt_abort(ioc);
         return -1;
@@ -333,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, errp) < 0) {
+        if (nbd_drop(ioc, len, errp) < 0) {
             error_prepend(errp, "failed to skip export name with wrong length");
             nbd_send_opt_abort(ioc);
             return -1;
@@ -342,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, errp) < 0) {
+    if (nbd_read(ioc, name, namelen, errp) < 0) {
         error_prepend(errp, "failed to read export name");
         nbd_send_opt_abort(ioc);
         return -1;
     }
     name[namelen] = '\0';
     len -= namelen;
-    if (drop_sync(ioc, len, errp) < 0) {
+    if (nbd_drop(ioc, len, errp) < 0) {
         error_prepend(errp, "failed to read export description");
         nbd_send_opt_abort(ioc);
         return -1;
@@ -476,7 +476,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
         goto fail;
     }
 
-    if (read_sync(ioc, buf, 8, errp) < 0) {
+    if (nbd_read(ioc, buf, 8, errp) < 0) {
         error_prepend(errp, "Failed to read data");
         goto fail;
     }
@@ -502,7 +502,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
         goto fail;
     }
 
-    if (read_sync(ioc, &magic, sizeof(magic), errp) < 0) {
+    if (nbd_read(ioc, &magic, sizeof(magic), errp) < 0) {
         error_prepend(errp, "Failed to read magic");
         goto fail;
     }
@@ -514,7 +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), errp) < 0) {
+        if (nbd_read(ioc, &globalflags, sizeof(globalflags), errp) < 0) {
             error_prepend(errp, "Failed to read server flags");
             goto fail;
         }
@@ -532,7 +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), errp) < 0) {
+        if (nbd_write(ioc, &clientflags, sizeof(clientflags), errp) < 0) {
             error_prepend(errp, "Failed to send clientflags field");
             goto fail;
         }
@@ -570,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), errp) < 0) {
+        if (nbd_read(ioc, &s, sizeof(s), errp) < 0) {
             error_prepend(errp, "Failed to read export length");
             goto fail;
         }
         *size = be64_to_cpu(s);
 
-        if (read_sync(ioc, flags, sizeof(*flags), errp) < 0) {
+        if (nbd_read(ioc, flags, sizeof(*flags), errp) < 0) {
             error_prepend(errp, "Failed to read export flags");
             goto fail;
         }
@@ -593,14 +593,14 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
             goto fail;
         }
 
-        if (read_sync(ioc, &s, sizeof(s), errp) < 0) {
+        if (nbd_read(ioc, &s, sizeof(s), errp) < 0) {
             error_prepend(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), errp) < 0) {
+        if (nbd_read(ioc, &oldflags, sizeof(oldflags), errp) < 0) {
             error_prepend(errp, "Failed to read export flags");
             goto fail;
         }
@@ -616,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, errp) < 0) {
+    if (zeroes && nbd_drop(ioc, 124, errp) < 0) {
         error_prepend(errp, "Failed to read reserved block");
         goto fail;
     }
@@ -759,7 +759,7 @@ ssize_t nbd_send_request(QIOChannel *ioc, NBDRequest *request)
     stq_be_p(buf + 16, request->from);
     stl_be_p(buf + 24, request->len);
 
-    return write_sync(ioc, buf, sizeof(buf), NULL);
+    return nbd_write(ioc, buf, sizeof(buf), NULL);
 }
 
 ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp)
@@ -768,7 +768,7 @@ ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp)
     uint32_t magic;
     ssize_t ret;
 
-    ret = read_sync_eof(ioc, buf, sizeof(buf), errp);
+    ret = nbd_read_eof(ioc, buf, sizeof(buf), errp);
     if (ret <= 0) {
         return ret;
     }
diff --git a/nbd/common.c b/nbd/common.c
index bd81637ab9..d6b719ddaa 100644
--- a/nbd/common.c
+++ b/nbd/common.c
@@ -24,12 +24,8 @@
  * The function may be called from coroutine or from non-coroutine context.
  * When called from non-coroutine context @ioc must be in blocking mode.
  */
-ssize_t nbd_wr_syncv(QIOChannel *ioc,
-                     struct iovec *iov,
-                     size_t niov,
-                     size_t length,
-                     bool do_read,
-                     Error **errp)
+ssize_t nbd_rwv(QIOChannel *ioc, struct iovec *iov, size_t niov, size_t length,
+                bool do_read, Error **errp)
 {
     ssize_t done = 0;
     struct iovec *local_iov = g_new(struct iovec, niov);
diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index d6071640a0..753cb9d9c5 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -94,14 +94,14 @@
 #define NBD_ENOSPC     28
 #define NBD_ESHUTDOWN  108
 
-/* read_sync_eof
+/* 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 are no needs to call read_sync_eof
+ * qio_channel_readv() returns 0. So, there is no need to call nbd_read_eof
  * iteratively.
  */
-static inline ssize_t read_sync_eof(QIOChannel *ioc, void *buffer, size_t size,
-                                    Error **errp)
+static inline ssize_t nbd_read_eof(QIOChannel *ioc, void *buffer, size_t size,
+                                   Error **errp)
 {
     struct iovec iov = { .iov_base = buffer, .iov_len = size };
     /* Sockets are kept in blocking mode in the negotiation phase.  After
@@ -109,16 +109,16 @@ static inline ssize_t read_sync_eof(QIOChannel *ioc, void *buffer, size_t size,
      * our request/reply.  Synchronization is done with recv_coroutine, so
      * that this is coroutine-safe.
      */
-    return nbd_wr_syncv(ioc, &iov, 1, size, true, errp);
+    return nbd_rwv(ioc, &iov, 1, size, true, errp);
 }
 
-/* read_sync
+/* nbd_read
  * Reads @size bytes from @ioc. Returns 0 on success.
  */
-static inline int read_sync(QIOChannel *ioc, void *buffer, size_t size,
-                            Error **errp)
+static inline int nbd_read(QIOChannel *ioc, void *buffer, size_t size,
+                           Error **errp)
 {
-    ssize_t ret = read_sync_eof(ioc, buffer, size, errp);
+    ssize_t ret = nbd_read_eof(ioc, buffer, size, errp);
 
     if (ret >= 0 && ret != size) {
         ret = -EINVAL;
@@ -128,15 +128,15 @@ static inline int read_sync(QIOChannel *ioc, void *buffer, size_t size,
     return ret < 0 ? ret : 0;
 }
 
-/* write_sync
+/* nbd_write
  * Writes @size bytes to @ioc. Returns 0 on success.
  */
-static inline int write_sync(QIOChannel *ioc, const void *buffer, size_t size,
-                             Error **errp)
+static inline int nbd_write(QIOChannel *ioc, const void *buffer, size_t size,
+                            Error **errp)
 {
     struct iovec iov = { .iov_base = (void *) buffer, .iov_len = size };
 
-    ssize_t ret = nbd_wr_syncv(ioc, &iov, 1, size, false, errp);
+    ssize_t ret = nbd_rwv(ioc, &iov, 1, size, false, errp);
 
     assert(ret < 0 || ret == size);
 
diff --git a/nbd/server.c b/nbd/server.c
index ee59e5d234..abaf9fb890 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -124,7 +124,7 @@ static int nbd_negotiate_read(QIOChannel *ioc, void *buffer, size_t size)
                                   nbd_negotiate_continue,
                                   qemu_coroutine_self(),
                                   NULL);
-    ret = read_sync(ioc, buffer, size, NULL);
+    ret = nbd_read(ioc, buffer, size, NULL);
     g_source_remove(watch);
     return ret;
 
@@ -142,7 +142,7 @@ static int nbd_negotiate_write(QIOChannel *ioc, const void *buffer, size_t size)
                                   nbd_negotiate_continue,
                                   qemu_coroutine_self(),
                                   NULL);
-    ret = write_sync(ioc, buffer, size, NULL);
+    ret = nbd_write(ioc, buffer, size, NULL);
     g_source_remove(watch);
     return ret;
 }
@@ -694,7 +694,7 @@ static ssize_t nbd_receive_request(QIOChannel *ioc, NBDRequest *request)
     uint32_t magic;
     ssize_t ret;
 
-    ret = read_sync(ioc, buf, sizeof(buf), NULL);
+    ret = nbd_read(ioc, buf, sizeof(buf), NULL);
     if (ret < 0) {
         return ret;
     }
@@ -745,7 +745,7 @@ static ssize_t nbd_send_reply(QIOChannel *ioc, NBDReply *reply)
     stl_be_p(buf + 4, reply->error);
     stq_be_p(buf + 8, reply->handle);
 
-    return write_sync(ioc, buf, sizeof(buf), NULL);
+    return nbd_write(ioc, buf, sizeof(buf), NULL);
 }
 
 #define MAX_NBD_REQUESTS 16
@@ -1048,7 +1048,7 @@ static ssize_t nbd_co_send_reply(NBDRequestData *req, NBDReply *reply,
         qio_channel_set_cork(client->ioc, true);
         rc = nbd_send_reply(client->ioc, reply);
         if (rc >= 0) {
-            ret = write_sync(client->ioc, req->data, len, NULL);
+            ret = nbd_write(client->ioc, req->data, len, NULL);
             if (ret < 0) {
                 rc = -EIO;
             }
@@ -1123,7 +1123,7 @@ static ssize_t nbd_co_receive_request(NBDRequestData *req,
     if (request->type == NBD_CMD_WRITE) {
         TRACE("Reading %" PRIu32 " byte(s)", request->len);
 
-        if (read_sync(client->ioc, req->data, request->len, NULL) < 0) {
+        if (nbd_read(client->ioc, req->data, request->len, NULL) < 0) {
             LOG("reading from socket failed");
             rc = -EIO;
             goto out;
-- 
2.11.1

[Qemu-devel] [PATCH v2 02/12] nbd: make nbd_drop public

[Vladimir Sementsov-Ogievskiy]

Following commit will reuse it for nbd server too.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/client.c       | 26 --------------------------
 nbd/common.c       | 26 ++++++++++++++++++++++++++
 nbd/nbd-internal.h |  2 ++
 3 files changed, 28 insertions(+), 26 deletions(-)

diff --git a/nbd/client.c b/nbd/client.c
index 08050c39b3..215857f65d 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -86,32 +86,6 @@ static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
 
 */
 
-/* Discard length bytes from channel.  Return -errno on failure and 0 on
- * success*/
-static int nbd_drop(QIOChannel *ioc, size_t size, Error **errp)
-{
-    ssize_t ret = 0;
-    char small[1024];
-    char *buffer;
-
-    buffer = sizeof(small) >= size ? small : g_malloc(MIN(65536, size));
-    while (size > 0) {
-        ssize_t count = MIN(65536, size);
-        ret = nbd_read(ioc, buffer, MIN(65536, size), errp);
-
-        if (ret < 0) {
-            goto cleanup;
-        }
-        size -= count;
-    }
-
- cleanup:
-    if (buffer != small) {
-        g_free(buffer);
-    }
-    return ret;
-}
-
 /* Send an option request.
  *
  * The request is for option @opt, with @data containing @len bytes of
diff --git a/nbd/common.c b/nbd/common.c
index d6b719ddaa..6b5c1b7b02 100644
--- a/nbd/common.c
+++ b/nbd/common.c
@@ -65,6 +65,32 @@ ssize_t nbd_rwv(QIOChannel *ioc, struct iovec *iov, size_t niov, size_t length,
     return done;
 }
 
+/* Discard length bytes from channel.  Return -errno on failure and 0 on
+ * success */
+int nbd_drop(QIOChannel *ioc, size_t size, Error **errp)
+{
+    ssize_t ret = 0;
+    char small[1024];
+    char *buffer;
+
+    buffer = sizeof(small) >= size ? small : g_malloc(MIN(65536, size));
+    while (size > 0) {
+        ssize_t count = MIN(65536, size);
+        ret = nbd_read(ioc, buffer, MIN(65536, size), errp);
+
+        if (ret < 0) {
+            goto cleanup;
+        }
+        size -= count;
+    }
+
+ cleanup:
+    if (buffer != small) {
+        g_free(buffer);
+    }
+    return ret;
+}
+
 
 void nbd_tls_handshake(QIOTask *task,
                        void *opaque)
diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index 753cb9d9c5..39bfed177c 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -153,4 +153,6 @@ struct NBDTLSHandshakeData {
 void nbd_tls_handshake(QIOTask *task,
                        void *opaque);
 
+int nbd_drop(QIOChannel *ioc, size_t size, Error **errp);
+
 #endif
-- 
2.11.1

[Qemu-devel] [PATCH v2 03/12] nbd/server: get rid of nbd_negotiate_read and friends

[Vladimir Sementsov-Ogievskiy]

Functions nbd_negotiate_{read,write,drop_sync} were introduced in
1a6245a5b, when nbd_rwv (was nbd_wr_sync) was working through
qemu_co_sendv_recvv (the path is nbd_wr_sync -> qemu_co_{recv/send} ->
qemu_co_send_recv -> qemu_co_sendv_recvv), which just yields, without
setting any handlers. But starting from ff82911cd nbd_rwv (was
nbd_wr_syncv) works through qio_channel_yield() which sets handlers, so
watchers are redundant in nbd_negotiate_{read,write,drop_sync}, then,
let's just use nbd_{read,write,drop} functions.

Functions nbd_{read,write,drop} has errp parameter, which is unused in
this patch. This will be fixed later.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c | 107 ++++++++++++-----------------------------------------------
 1 file changed, 22 insertions(+), 85 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index abaf9fb890..b830cd23d3 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -104,69 +104,6 @@ struct NBDClient {
 
 static void nbd_client_receive_next_request(NBDClient *client);
 
-static gboolean nbd_negotiate_continue(QIOChannel *ioc,
-                                       GIOCondition condition,
-                                       void *opaque)
-{
-    qemu_coroutine_enter(opaque);
-    return TRUE;
-}
-
-static int nbd_negotiate_read(QIOChannel *ioc, void *buffer, size_t size)
-{
-    ssize_t ret;
-    guint watch;
-
-    assert(qemu_in_coroutine());
-    /* Negotiation are always in main loop. */
-    watch = qio_channel_add_watch(ioc,
-                                  G_IO_IN,
-                                  nbd_negotiate_continue,
-                                  qemu_coroutine_self(),
-                                  NULL);
-    ret = nbd_read(ioc, buffer, size, NULL);
-    g_source_remove(watch);
-    return ret;
-
-}
-
-static int nbd_negotiate_write(QIOChannel *ioc, const void *buffer, size_t size)
-{
-    ssize_t ret;
-    guint watch;
-
-    assert(qemu_in_coroutine());
-    /* Negotiation are always in main loop. */
-    watch = qio_channel_add_watch(ioc,
-                                  G_IO_OUT,
-                                  nbd_negotiate_continue,
-                                  qemu_coroutine_self(),
-                                  NULL);
-    ret = nbd_write(ioc, buffer, size, NULL);
-    g_source_remove(watch);
-    return ret;
-}
-
-static int nbd_negotiate_drop_sync(QIOChannel *ioc, size_t size)
-{
-    ssize_t ret;
-    uint8_t *buffer = g_malloc(MIN(65536, size));
-
-    while (size > 0) {
-        size_t count = MIN(65536, size);
-        ret = nbd_negotiate_read(ioc, buffer, count);
-        if (ret < 0) {
-            g_free(buffer);
-            return ret;
-        }
-
-        size -= count;
-    }
-
-    g_free(buffer);
-    return 0;
-}
-
 /* Basic flow for negotiation
 
    Server         Client
@@ -205,22 +142,22 @@ static int nbd_negotiate_send_rep_len(QIOChannel *ioc, uint32_t type,
           type, opt, len);
 
     magic = cpu_to_be64(NBD_REP_MAGIC);
-    if (nbd_negotiate_write(ioc, &magic, sizeof(magic)) < 0) {
+    if (nbd_write(ioc, &magic, sizeof(magic), NULL) < 0) {
         LOG("write failed (rep magic)");
         return -EINVAL;
     }
     opt = cpu_to_be32(opt);
-    if (nbd_negotiate_write(ioc, &opt, sizeof(opt)) < 0) {
+    if (nbd_write(ioc, &opt, sizeof(opt), NULL) < 0) {
         LOG("write failed (rep opt)");
         return -EINVAL;
     }
     type = cpu_to_be32(type);
-    if (nbd_negotiate_write(ioc, &type, sizeof(type)) < 0) {
+    if (nbd_write(ioc, &type, sizeof(type), NULL) < 0) {
         LOG("write failed (rep type)");
         return -EINVAL;
     }
     len = cpu_to_be32(len);
-    if (nbd_negotiate_write(ioc, &len, sizeof(len)) < 0) {
+    if (nbd_write(ioc, &len, sizeof(len), NULL) < 0) {
         LOG("write failed (rep data length)");
         return -EINVAL;
     }
@@ -255,7 +192,7 @@ nbd_negotiate_send_rep_err(QIOChannel *ioc, uint32_t type,
     if (ret < 0) {
         goto out;
     }
-    if (nbd_negotiate_write(ioc, msg, len) < 0) {
+    if (nbd_write(ioc, msg, len, NULL) < 0) {
         LOG("write failed (error message)");
         ret = -EIO;
     } else {
@@ -286,15 +223,15 @@ static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp)
     }
 
     len = cpu_to_be32(name_len);
-    if (nbd_negotiate_write(ioc, &len, sizeof(len)) < 0) {
+    if (nbd_write(ioc, &len, sizeof(len), NULL) < 0) {
         LOG("write failed (name length)");
         return -EINVAL;
     }
-    if (nbd_negotiate_write(ioc, name, name_len) < 0) {
+    if (nbd_write(ioc, name, name_len, NULL) < 0) {
         LOG("write failed (name buffer)");
         return -EINVAL;
     }
-    if (nbd_negotiate_write(ioc, desc, desc_len) < 0) {
+    if (nbd_write(ioc, desc, desc_len, NULL) < 0) {
         LOG("write failed (description buffer)");
         return -EINVAL;
     }
@@ -308,7 +245,7 @@ static int nbd_negotiate_handle_list(NBDClient *client, uint32_t length)
     NBDExport *exp;
 
     if (length) {
-        if (nbd_negotiate_drop_sync(client->ioc, length) < 0) {
+        if (nbd_drop(client->ioc, length, NULL) < 0) {
             return -EIO;
         }
         return nbd_negotiate_send_rep_err(client->ioc,
@@ -339,7 +276,7 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length)
         LOG("Bad length received");
         goto fail;
     }
-    if (nbd_negotiate_read(client->ioc, name, length) < 0) {
+    if (nbd_read(client->ioc, name, length, NULL) < 0) {
         LOG("read failed");
         goto fail;
     }
@@ -372,7 +309,7 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
     TRACE("Setting up TLS");
     ioc = client->ioc;
     if (length) {
-        if (nbd_negotiate_drop_sync(ioc, length) < 0) {
+        if (nbd_drop(ioc, length, NULL) < 0) {
             return NULL;
         }
         nbd_negotiate_send_rep_err(ioc, NBD_REP_ERR_INVALID, NBD_OPT_STARTTLS,
@@ -436,7 +373,7 @@ static int nbd_negotiate_options(NBDClient *client)
         ...           Rest of request
     */
 
-    if (nbd_negotiate_read(client->ioc, &flags, sizeof(flags)) < 0) {
+    if (nbd_read(client->ioc, &flags, sizeof(flags), NULL) < 0) {
         LOG("read failed");
         return -EIO;
     }
@@ -462,7 +399,7 @@ static int nbd_negotiate_options(NBDClient *client)
         uint32_t clientflags, length;
         uint64_t magic;
 
-        if (nbd_negotiate_read(client->ioc, &magic, sizeof(magic)) < 0) {
+        if (nbd_read(client->ioc, &magic, sizeof(magic), NULL) < 0) {
             LOG("read failed");
             return -EINVAL;
         }
@@ -472,15 +409,15 @@ static int nbd_negotiate_options(NBDClient *client)
             return -EINVAL;
         }
 
-        if (nbd_negotiate_read(client->ioc, &clientflags,
-                               sizeof(clientflags)) < 0)
+        if (nbd_read(client->ioc, &clientflags,
+                      sizeof(clientflags), NULL) < 0)
         {
             LOG("read failed");
             return -EINVAL;
         }
         clientflags = be32_to_cpu(clientflags);
 
-        if (nbd_negotiate_read(client->ioc, &length, sizeof(length)) < 0) {
+        if (nbd_read(client->ioc, &length, sizeof(length), NULL) < 0) {
             LOG("read failed");
             return -EINVAL;
         }
@@ -510,7 +447,7 @@ static int nbd_negotiate_options(NBDClient *client)
                 return -EINVAL;
 
             default:
-                if (nbd_negotiate_drop_sync(client->ioc, length) < 0) {
+                if (nbd_drop(client->ioc, length, NULL) < 0) {
                     return -EIO;
                 }
                 ret = nbd_negotiate_send_rep_err(client->ioc,
@@ -548,7 +485,7 @@ static int nbd_negotiate_options(NBDClient *client)
                 return nbd_negotiate_handle_export_name(client, length);
 
             case NBD_OPT_STARTTLS:
-                if (nbd_negotiate_drop_sync(client->ioc, length) < 0) {
+                if (nbd_drop(client->ioc, length, NULL) < 0) {
                     return -EIO;
                 }
                 if (client->tlscreds) {
@@ -567,7 +504,7 @@ static int nbd_negotiate_options(NBDClient *client)
                 }
                 break;
             default:
-                if (nbd_negotiate_drop_sync(client->ioc, length) < 0) {
+                if (nbd_drop(client->ioc, length, NULL) < 0) {
                     return -EIO;
                 }
                 ret = nbd_negotiate_send_rep_err(client->ioc,
@@ -656,12 +593,12 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData *data)
             TRACE("TLS cannot be enabled with oldstyle protocol");
             goto fail;
         }
-        if (nbd_negotiate_write(client->ioc, buf, sizeof(buf)) < 0) {
+        if (nbd_write(client->ioc, buf, sizeof(buf), NULL) < 0) {
             LOG("write failed");
             goto fail;
         }
     } else {
-        if (nbd_negotiate_write(client->ioc, buf, 18) < 0) {
+        if (nbd_write(client->ioc, buf, 18, NULL) < 0) {
             LOG("write failed");
             goto fail;
         }
@@ -676,7 +613,7 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData *data)
         stq_be_p(buf + 18, client->exp->size);
         stw_be_p(buf + 26, client->exp->nbdflags | myflags);
         len = client->no_zeroes ? 10 : sizeof(buf) - 18;
-        if (nbd_negotiate_write(client->ioc, buf + 18, len) < 0) {
+        if (nbd_write(client->ioc, buf + 18, len, NULL) < 0) {
             LOG("write failed");
             goto fail;
         }
-- 
2.11.1

[Qemu-devel] [PATCH v2 04/12] nbd/server: get rid of ssize_t

[Vladimir Sementsov-Ogievskiy]

Now nbd_read and friends return int, so get rid of ssize_t.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index b830cd23d3..8985df86b4 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -625,11 +625,11 @@ fail:
     return rc;
 }
 
-static ssize_t nbd_receive_request(QIOChannel *ioc, NBDRequest *request)
+static int nbd_receive_request(QIOChannel *ioc, NBDRequest *request)
 {
     uint8_t buf[NBD_REQUEST_SIZE];
     uint32_t magic;
-    ssize_t ret;
+    int ret;
 
     ret = nbd_read(ioc, buf, sizeof(buf), NULL);
     if (ret < 0) {
@@ -663,7 +663,7 @@ static ssize_t nbd_receive_request(QIOChannel *ioc, NBDRequest *request)
     return 0;
 }
 
-static ssize_t nbd_send_reply(QIOChannel *ioc, NBDReply *reply)
+static int nbd_send_reply(QIOChannel *ioc, NBDReply *reply)
 {
     uint8_t buf[NBD_REPLY_SIZE];
 
@@ -969,11 +969,10 @@ void nbd_export_close_all(void)
     }
 }
 
-static ssize_t nbd_co_send_reply(NBDRequestData *req, NBDReply *reply,
-                                 int len)
+static int nbd_co_send_reply(NBDRequestData *req, NBDReply *reply, int len)
 {
     NBDClient *client = req->client;
-    ssize_t rc, ret;
+    int rc, ret;
 
     g_assert(qemu_in_coroutine());
     qemu_co_mutex_lock(&client->send_lock);
@@ -1003,11 +1002,10 @@ static ssize_t nbd_co_send_reply(NBDRequestData *req, NBDReply *reply,
  * and any other negative value to report an error to the client
  * (although the caller may still need to disconnect after reporting
  * the error).  */
-static ssize_t nbd_co_receive_request(NBDRequestData *req,
-                                      NBDRequest *request)
+static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request)
 {
     NBDClient *client = req->client;
-    ssize_t rc;
+    int rc;
 
     g_assert(qemu_in_coroutine());
     assert(client->recv_coroutine == qemu_coroutine_self());
@@ -1105,7 +1103,7 @@ static coroutine_fn void nbd_trip(void *opaque)
     NBDRequestData *req;
     NBDRequest request = { 0 };    /* GCC thinks it can be used uninitialized */
     NBDReply reply;
-    ssize_t ret;
+    int ret;
     int flags;
 
     TRACE("Reading request.");
-- 
2.11.1

[Qemu-devel] [PATCH v2 05/12] nbd/server: refactor nbd_co_send_reply

[Vladimir Sementsov-Ogievskiy]

As nbd_write never returns value > 0, we can get rid of extra ret.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 8985df86b4..dd6b241761 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -972,7 +972,7 @@ void nbd_export_close_all(void)
 static int nbd_co_send_reply(NBDRequestData *req, NBDReply *reply, int len)
 {
     NBDClient *client = req->client;
-    int rc, ret;
+    int rc;
 
     g_assert(qemu_in_coroutine());
     qemu_co_mutex_lock(&client->send_lock);
@@ -983,9 +983,9 @@ static int nbd_co_send_reply(NBDRequestData *req, NBDReply *reply, int len)
     } else {
         qio_channel_set_cork(client->ioc, true);
         rc = nbd_send_reply(client->ioc, reply);
-        if (rc >= 0) {
-            ret = nbd_write(client->ioc, req->data, len, NULL);
-            if (ret < 0) {
+        if (rc == 0) {
+            rc = nbd_write(client->ioc, req->data, len, NULL);
+            if (rc < 0) {
                 rc = -EIO;
             }
         }
-- 
2.11.1

[Qemu-devel] [PATCH v2 06/12] nbd/server: get rid of EAGAIN dead code

[Vladimir Sementsov-Ogievskiy]

For now nbd_read never returns EAGAIN. So, don't handle it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index dd6b241761..b0bb263596 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -997,11 +997,12 @@ static int nbd_co_send_reply(NBDRequestData *req, NBDReply *reply, int len)
     return rc;
 }
 
-/* Collect a client request.  Return 0 if request looks valid, -EAGAIN
- * to keep trying the collection, -EIO to drop connection right away,
- * and any other negative value to report an error to the client
- * (although the caller may still need to disconnect after reporting
- * the error).  */
+/* nbd_co_receive_request
+ * Collect a client request. Return 0 if request looks valid, -EIO to drop
+ * connection right away, and any other negative value to report an error to
+ * the client (although the caller may still need to disconnect after reporting
+ * the error).
+ */
 static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request)
 {
     NBDClient *client = req->client;
@@ -1011,9 +1012,7 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request)
     assert(client->recv_coroutine == qemu_coroutine_self());
     rc = nbd_receive_request(client->ioc, request);
     if (rc < 0) {
-        if (rc != -EAGAIN) {
-            rc = -EIO;
-        }
+        rc = -EIO;
         goto out;
     }
 
@@ -1114,9 +1113,6 @@ static coroutine_fn void nbd_trip(void *opaque)
 
     req = nbd_request_get(client);
     ret = nbd_co_receive_request(req, &request);
-    if (ret == -EAGAIN) {
-        goto done;
-    }
     if (ret == -EIO) {
         goto out;
     }
-- 
2.11.1

[Qemu-devel] [PATCH v2 07/12] nbd/server: refactor nbd_co_receive_request

[Vladimir Sementsov-Ogievskiy]

Move function tail, about receiving next request out of the function.
Error path is simplified and nbd_co_receive_request becomes more
corresponding to its name.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c | 41 +++++++++++++----------------------------
 1 file changed, 13 insertions(+), 28 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index b0bb263596..8ac095d6bc 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1006,14 +1006,11 @@ static int nbd_co_send_reply(NBDRequestData *req, NBDReply *reply, int len)
 static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request)
 {
     NBDClient *client = req->client;
-    int rc;
 
     g_assert(qemu_in_coroutine());
     assert(client->recv_coroutine == qemu_coroutine_self());
-    rc = nbd_receive_request(client->ioc, request);
-    if (rc < 0) {
-        rc = -EIO;
-        goto out;
+    if (nbd_receive_request(client->ioc, request) < 0) {
+        return -EIO;
     }
 
     TRACE("Decoding type");
@@ -1027,8 +1024,7 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request)
         /* Special case: we're going to disconnect without a reply,
          * whether or not flags, from, or len are bogus */
         TRACE("Request type is DISCONNECT");
-        rc = -EIO;
-        goto out;
+        return -EIO;
     }
 
     /* Check for sanity in the parameters, part 1.  Defer as many
@@ -1036,22 +1032,19 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request)
      * payload, so we can try and keep the connection alive.  */
     if ((request->from + request->len) < request->from) {
         LOG("integer overflow detected, you're probably being attacked");
-        rc = -EINVAL;
-        goto out;
+        return -EINVAL;
     }
 
     if (request->type == NBD_CMD_READ || request->type == NBD_CMD_WRITE) {
         if (request->len > NBD_MAX_BUFFER_SIZE) {
             LOG("len (%" PRIu32" ) is larger than max len (%u)",
                 request->len, NBD_MAX_BUFFER_SIZE);
-            rc = -EINVAL;
-            goto out;
+            return -EINVAL;
         }
 
         req->data = blk_try_blockalign(client->exp->blk, request->len);
         if (req->data == NULL) {
-            rc = -ENOMEM;
-            goto out;
+            return -ENOMEM;
         }
     }
     if (request->type == NBD_CMD_WRITE) {
@@ -1059,8 +1052,7 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request)
 
         if (nbd_read(client->ioc, req->data, request->len, NULL) < 0) {
             LOG("reading from socket failed");
-            rc = -EIO;
-            goto out;
+            return -EIO;
         }
         req->complete = true;
     }
@@ -1070,28 +1062,19 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request)
         LOG("operation past EOF; From: %" PRIu64 ", Len: %" PRIu32
             ", Size: %" PRIu64, request->from, request->len,
             (uint64_t)client->exp->size);
-        rc = request->type == NBD_CMD_WRITE ? -ENOSPC : -EINVAL;
-        goto out;
+        return request->type == NBD_CMD_WRITE ? -ENOSPC : -EINVAL;
     }
     if (request->flags & ~(NBD_CMD_FLAG_FUA | NBD_CMD_FLAG_NO_HOLE)) {
         LOG("unsupported flags (got 0x%x)", request->flags);
-        rc = -EINVAL;
-        goto out;
+        return -EINVAL;
     }
     if (request->type != NBD_CMD_WRITE_ZEROES &&
         (request->flags & NBD_CMD_FLAG_NO_HOLE)) {
         LOG("unexpected flags (got 0x%x)", request->flags);
-        rc = -EINVAL;
-        goto out;
+        return -EINVAL;
     }
 
-    rc = 0;
-
-out:
-    client->recv_coroutine = NULL;
-    nbd_client_receive_next_request(client);
-
-    return rc;
+    return 0;
 }
 
 /* Owns a reference to the NBDClient passed as opaque.  */
@@ -1113,6 +1096,8 @@ static coroutine_fn void nbd_trip(void *opaque)
 
     req = nbd_request_get(client);
     ret = nbd_co_receive_request(req, &request);
+    client->recv_coroutine = NULL;
+    nbd_client_receive_next_request(client);
     if (ret == -EIO) {
         goto out;
     }
-- 
2.11.1

[Qemu-devel] [PATCH v2 08/12] nbd/server: remove NBDClientNewData

[Vladimir Sementsov-Ogievskiy]

"co" field of NBDClientNewData has never been used, all the way back to
its declaration in commit 1a6245a5. So let's just use client pointer
instead of extra structure.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c | 25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 8ac095d6bc..d376563527 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -535,14 +535,8 @@ static int nbd_negotiate_options(NBDClient *client)
     }
 }
 
-typedef struct {
-    NBDClient *client;
-    Coroutine *co;
-} NBDClientNewData;
-
-static coroutine_fn int nbd_negotiate(NBDClientNewData *data)
+static coroutine_fn int nbd_negotiate(NBDClient *client)
 {
-    NBDClient *client = data->client;
     char buf[8 + 8 + 8 + 128];
     int rc;
     const uint16_t myflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM |
@@ -1268,16 +1262,15 @@ static void nbd_client_receive_next_request(NBDClient *client)
 
 static coroutine_fn void nbd_co_client_start(void *opaque)
 {
-    NBDClientNewData *data = opaque;
-    NBDClient *client = data->client;
+    NBDClient *client = opaque;
     NBDExport *exp = client->exp;
 
     if (exp) {
         nbd_export_get(exp);
     }
-    if (nbd_negotiate(data)) {
+    if (nbd_negotiate(client)) {
         client_close(client);
-        goto out;
+        return;
     }
     qemu_co_mutex_init(&client->send_lock);
 
@@ -1286,9 +1279,6 @@ static coroutine_fn void nbd_co_client_start(void *opaque)
     }
 
     nbd_client_receive_next_request(client);
-
-out:
-    g_free(data);
 }
 
 void nbd_client_new(NBDExport *exp,
@@ -1298,7 +1288,7 @@ void nbd_client_new(NBDExport *exp,
                     void (*close_fn)(NBDClient *))
 {
     NBDClient *client;
-    NBDClientNewData *data = g_new(NBDClientNewData, 1);
+    Coroutine *co;
 
     client = g_malloc0(sizeof(NBDClient));
     client->refcount = 1;
@@ -1314,7 +1304,6 @@ void nbd_client_new(NBDExport *exp,
     object_ref(OBJECT(client->ioc));
     client->close = close_fn;
 
-    data->client = client;
-    data->co = qemu_coroutine_create(nbd_co_client_start, data);
-    qemu_coroutine_enter(data->co);
+    co = qemu_coroutine_create(nbd_co_client_start, client);
+    qemu_coroutine_enter(co);
 }
-- 
2.11.1

[Qemu-devel] [PATCH v2 09/12] nbd/server: nbd_negotiate: fix error path

[Vladimir Sementsov-Ogievskiy]

Current code will return 0 on this nbd_write fail, as rc is 0
after successful nbd_negotiate_options. Fix this.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/nbd/server.c b/nbd/server.c
index d376563527..ec163ad829 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -607,7 +607,8 @@ static coroutine_fn int nbd_negotiate(NBDClient *client)
         stq_be_p(buf + 18, client->exp->size);
         stw_be_p(buf + 26, client->exp->nbdflags | myflags);
         len = client->no_zeroes ? 10 : sizeof(buf) - 18;
-        if (nbd_write(client->ioc, buf + 18, len, NULL) < 0) {
+        rc = nbd_write(client->ioc, buf + 18, len, NULL);
+        if (rc < 0) {
             LOG("write failed");
             goto fail;
         }
-- 
2.11.1

[Qemu-devel] [PATCH v2 10/12] nbd/server: get rid of fail: return rc

[Vladimir Sementsov-Ogievskiy]

"goto fail" error handling scheme is not needed for just returning
error code. Better is return it immediately.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index ec163ad829..2f1c5b0a5b 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -265,7 +265,6 @@ static int nbd_negotiate_handle_list(NBDClient *client, uint32_t length)
 
 static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length)
 {
-    int rc = -EINVAL;
     char name[NBD_MAX_NAME_SIZE + 1];
 
     /* Client sends:
@@ -274,11 +273,11 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length)
     TRACE("Checking length");
     if (length >= sizeof(name)) {
         LOG("Bad length received");
-        goto fail;
+        return -EINVAL;
     }
     if (nbd_read(client->ioc, name, length, NULL) < 0) {
         LOG("read failed");
-        goto fail;
+        return -EINVAL;
     }
     name[length] = '\0';
 
@@ -287,14 +286,13 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length)
     client->exp = nbd_export_find(name);
     if (!client->exp) {
         LOG("export not found");
-        goto fail;
+        return -EINVAL;
     }
 
     QTAILQ_INSERT_TAIL(&client->exp->clients, client, next);
     nbd_export_get(client->exp);
-    rc = 0;
-fail:
-    return rc;
+
+    return 0;
 }
 
 /* Handle NBD_OPT_STARTTLS. Return NULL to drop connection, or else the
@@ -564,7 +562,6 @@ static coroutine_fn int nbd_negotiate(NBDClient *client)
      */
 
     qio_channel_set_blocking(client->ioc, false, NULL);
-    rc = -EINVAL;
 
     TRACE("Beginning negotiation.");
     memset(buf, 0, sizeof(buf));
@@ -585,21 +582,21 @@ static coroutine_fn int nbd_negotiate(NBDClient *client)
     if (oldStyle) {
         if (client->tlscreds) {
             TRACE("TLS cannot be enabled with oldstyle protocol");
-            goto fail;
+            return -EINVAL;
         }
         if (nbd_write(client->ioc, buf, sizeof(buf), NULL) < 0) {
             LOG("write failed");
-            goto fail;
+            return -EINVAL;
         }
     } else {
         if (nbd_write(client->ioc, buf, 18, NULL) < 0) {
             LOG("write failed");
-            goto fail;
+            return -EINVAL;
         }
         rc = nbd_negotiate_options(client);
         if (rc != 0) {
             LOG("option negotiation failed");
-            goto fail;
+            return rc;
         }
 
         TRACE("advertising size %" PRIu64 " and flags %x",
@@ -610,14 +607,13 @@ static coroutine_fn int nbd_negotiate(NBDClient *client)
         rc = nbd_write(client->ioc, buf + 18, len, NULL);
         if (rc < 0) {
             LOG("write failed");
-            goto fail;
+            return rc;
         }
     }
 
     TRACE("Negotiation succeeded.");
-    rc = 0;
-fail:
-    return rc;
+
+    return 0;
 }
 
 static int nbd_receive_request(QIOChannel *ioc, NBDRequest *request)
-- 
2.11.1

[Qemu-devel] [PATCH v2 11/12] nbd/server: rename rc to ret

[Vladimir Sementsov-Ogievskiy]

For consistency use 'ret' name for saving return code everywhere
in the file.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 2f1c5b0a5b..209a3d4e1e 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -211,15 +211,15 @@ static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp)
     uint32_t len;
     const char *name = exp->name ? exp->name : "";
     const char *desc = exp->description ? exp->description : "";
-    int rc;
+    int ret;
 
     TRACE("Advertising export name '%s' description '%s'", name, desc);
     name_len = strlen(name);
     desc_len = strlen(desc);
     len = name_len + desc_len + sizeof(len);
-    rc = nbd_negotiate_send_rep_len(ioc, NBD_REP_SERVER, NBD_OPT_LIST, len);
-    if (rc < 0) {
-        return rc;
+    ret = nbd_negotiate_send_rep_len(ioc, NBD_REP_SERVER, NBD_OPT_LIST, len);
+    if (ret < 0) {
+        return ret;
     }
 
     len = cpu_to_be32(name_len);
@@ -536,7 +536,7 @@ static int nbd_negotiate_options(NBDClient *client)
 static coroutine_fn int nbd_negotiate(NBDClient *client)
 {
     char buf[8 + 8 + 8 + 128];
-    int rc;
+    int ret;
     const uint16_t myflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM |
                               NBD_FLAG_SEND_FLUSH | NBD_FLAG_SEND_FUA |
                               NBD_FLAG_SEND_WRITE_ZEROES);
@@ -593,10 +593,10 @@ static coroutine_fn int nbd_negotiate(NBDClient *client)
             LOG("write failed");
             return -EINVAL;
         }
-        rc = nbd_negotiate_options(client);
-        if (rc != 0) {
+        ret = nbd_negotiate_options(client);
+        if (ret != 0) {
             LOG("option negotiation failed");
-            return rc;
+            return ret;
         }
 
         TRACE("advertising size %" PRIu64 " and flags %x",
@@ -604,10 +604,10 @@ static coroutine_fn int nbd_negotiate(NBDClient *client)
         stq_be_p(buf + 18, client->exp->size);
         stw_be_p(buf + 26, client->exp->nbdflags | myflags);
         len = client->no_zeroes ? 10 : sizeof(buf) - 18;
-        rc = nbd_write(client->ioc, buf + 18, len, NULL);
-        if (rc < 0) {
+        ret = nbd_write(client->ioc, buf + 18, len, NULL);
+        if (ret < 0) {
             LOG("write failed");
-            return rc;
+            return ret;
         }
     }
 
@@ -963,21 +963,21 @@ void nbd_export_close_all(void)
 static int nbd_co_send_reply(NBDRequestData *req, NBDReply *reply, int len)
 {
     NBDClient *client = req->client;
-    int rc;
+    int ret;
 
     g_assert(qemu_in_coroutine());
     qemu_co_mutex_lock(&client->send_lock);
     client->send_coroutine = qemu_coroutine_self();
 
     if (!len) {
-        rc = nbd_send_reply(client->ioc, reply);
+        ret = nbd_send_reply(client->ioc, reply);
     } else {
         qio_channel_set_cork(client->ioc, true);
-        rc = nbd_send_reply(client->ioc, reply);
-        if (rc == 0) {
-            rc = nbd_write(client->ioc, req->data, len, NULL);
-            if (rc < 0) {
-                rc = -EIO;
+        ret = nbd_send_reply(client->ioc, reply);
+        if (ret == 0) {
+            ret = nbd_write(client->ioc, req->data, len, NULL);
+            if (ret < 0) {
+                ret = -EIO;
             }
         }
         qio_channel_set_cork(client->ioc, false);
@@ -985,7 +985,7 @@ static int nbd_co_send_reply(NBDRequestData *req, NBDReply *reply, int len)
 
     client->send_coroutine = NULL;
     qemu_co_mutex_unlock(&client->send_lock);
-    return rc;
+    return ret;
 }
 
 /* nbd_co_receive_request
-- 
2.11.1

[Qemu-devel] [PATCH v2 12/12] nbd/server: refactor nbd_trip

[Vladimir Sementsov-Ogievskiy]

- do not use 'goto error_reply' outside a switch to jump into the
  middle of the switch's default case label
- reduce code duplication

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c | 53 ++++++++++++++++++++---------------------------------
 1 file changed, 20 insertions(+), 33 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 209a3d4e1e..198eabe338 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1078,6 +1078,7 @@ static coroutine_fn void nbd_trip(void *opaque)
     NBDReply reply;
     int ret;
     int flags;
+    int reply_data_len = 0;
 
     TRACE("Reading request.");
     if (client->closing) {
@@ -1090,7 +1091,7 @@ static coroutine_fn void nbd_trip(void *opaque)
     client->recv_coroutine = NULL;
     nbd_client_receive_next_request(client);
     if (ret == -EIO) {
-        goto out;
+        goto disconnect;
     }
 
     reply.handle = request.handle;
@@ -1098,7 +1099,7 @@ static coroutine_fn void nbd_trip(void *opaque)
 
     if (ret < 0) {
         reply.error = -ret;
-        goto error_reply;
+        goto reply;
     }
 
     if (client->closing) {
@@ -1119,7 +1120,7 @@ static coroutine_fn void nbd_trip(void *opaque)
             if (ret < 0) {
                 LOG("flush failed");
                 reply.error = -ret;
-                goto error_reply;
+                break;
             }
         }
 
@@ -1128,12 +1129,12 @@ static coroutine_fn void nbd_trip(void *opaque)
         if (ret < 0) {
             LOG("reading from file failed");
             reply.error = -ret;
-            goto error_reply;
+            break;
         }
 
+        reply_data_len = request.len;
         TRACE("Read %" PRIu32" byte(s)", request.len);
-        if (nbd_co_send_reply(req, &reply, request.len) < 0)
-            goto out;
+
         break;
     case NBD_CMD_WRITE:
         TRACE("Request type is WRITE");
@@ -1141,7 +1142,7 @@ static coroutine_fn void nbd_trip(void *opaque)
         if (exp->nbdflags & NBD_FLAG_READ_ONLY) {
             TRACE("Server is read-only, return error");
             reply.error = EROFS;
-            goto error_reply;
+            break;
         }
 
         TRACE("Writing to device");
@@ -1155,21 +1156,16 @@ static coroutine_fn void nbd_trip(void *opaque)
         if (ret < 0) {
             LOG("writing to file failed");
             reply.error = -ret;
-            goto error_reply;
         }
 
-        if (nbd_co_send_reply(req, &reply, 0) < 0) {
-            goto out;
-        }
         break;
-
     case NBD_CMD_WRITE_ZEROES:
         TRACE("Request type is WRITE_ZEROES");
 
         if (exp->nbdflags & NBD_FLAG_READ_ONLY) {
             TRACE("Server is read-only, return error");
             reply.error = EROFS;
-            goto error_reply;
+            break;
         }
 
         TRACE("Writing to device");
@@ -1186,14 +1182,9 @@ static coroutine_fn void nbd_trip(void *opaque)
         if (ret < 0) {
             LOG("writing to file failed");
             reply.error = -ret;
-            goto error_reply;
         }
 
-        if (nbd_co_send_reply(req, &reply, 0) < 0) {
-            goto out;
-        }
         break;
-
     case NBD_CMD_DISC:
         /* unreachable, thanks to special case in nbd_co_receive_request() */
         abort();
@@ -1206,9 +1197,7 @@ static coroutine_fn void nbd_trip(void *opaque)
             LOG("flush failed");
             reply.error = -ret;
         }
-        if (nbd_co_send_reply(req, &reply, 0) < 0) {
-            goto out;
-        }
+
         break;
     case NBD_CMD_TRIM:
         TRACE("Request type is TRIM");
@@ -1218,21 +1207,19 @@ static coroutine_fn void nbd_trip(void *opaque)
             LOG("discard failed");
             reply.error = -ret;
         }
-        if (nbd_co_send_reply(req, &reply, 0) < 0) {
-            goto out;
-        }
+
         break;
     default:
         LOG("invalid request type (%" PRIu32 ") received", request.type);
         reply.error = EINVAL;
-    error_reply:
-        /* We must disconnect after NBD_CMD_WRITE if we did not
-         * read the payload.
-         */
-        if (nbd_co_send_reply(req, &reply, 0) < 0 || !req->complete) {
-            goto out;
-        }
-        break;
+    }
+
+reply:
+    /* We must disconnect after NBD_CMD_WRITE if we did not
+     * read the payload.
+     */
+    if (nbd_co_send_reply(req, &reply, reply_data_len) < 0 || !req->complete) {
+        goto disconnect;
     }
 
     TRACE("Request/Reply complete");
@@ -1242,7 +1229,7 @@ done:
     nbd_client_put(client);
     return;
 
-out:
+disconnect:
     nbd_request_put(req);
     client_close(client);
     nbd_client_put(client);
-- 
2.11.1

[Qemu-devel] ping Re: [PATCH v2 00/12] nbd refactoring part 1

[Vladimir Sementsov-Ogievskiy]

ping... What about this?

02.06.2017 18:01, Vladimir Sementsov-Ogievskiy wrote:
> This is based on my "nbd: error path refactoring" series.
> This is an update of first part of my "nbd errors and traces refactoring"
>
> v2:
> 01: update commit msg
>      grammar in comment
>      add Eric's r-b
> 02: add Eric's r-b
> 03: update commit msg
>      add Eric's r-b
>
>
> v1:
> changes from patches 01-10 of "nbd errors and traces refactoring" (mostly
> proposed by Eric):
> 01 new patch
> 02-03 - split of old 01
> 08 and 12 - minor changes in commit messages
> 04-12 - add r-b by Eric
>
> Also, of course, renaming in 01 is "visible" in other patches.
>
> Vladimir Sementsov-Ogievskiy (12):
>    nbd: rename read_sync and friends
>    nbd: make nbd_drop public
>    nbd/server: get rid of nbd_negotiate_read and friends
>    nbd/server: get rid of ssize_t
>    nbd/server: refactor nbd_co_send_reply
>    nbd/server: get rid of EAGAIN dead code
>    nbd/server: refactor nbd_co_receive_request
>    nbd/server: remove NBDClientNewData
>    nbd/server: nbd_negotiate: fix error path
>    nbd/server: get rid of fail: return rc
>    nbd/server: rename rc to ret
>    nbd/server: refactor nbd_trip
>
>   block/nbd-client.c  |   8 +-
>   include/block/nbd.h |   8 +-
>   nbd/client.c        |  64 ++++-------
>   nbd/common.c        |  34 +++++-
>   nbd/nbd-internal.h  |  28 ++---
>   nbd/server.c        | 319 +++++++++++++++++-----------------------------------
>   6 files changed, 172 insertions(+), 289 deletions(-)
>

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 12/12] nbd/server: refactor nbd_trip

[Paolo Bonzini]

On 02/06/2017 17:01, Vladimir Sementsov-Ogievskiy wrote:
> - do not use 'goto error_reply' outside a switch to jump into the
>   middle of the switch's default case label
> - reduce code duplication
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>

As a followup, maybe the switch statement can be extracted to its own
function.  This way the "goto reply" can be avoided.  I'll take a look.

Apart from this, the series looks good.  I'll queue it tomorrow.

Paolo


> ---
>  nbd/server.c | 53 ++++++++++++++++++++---------------------------------
>  1 file changed, 20 insertions(+), 33 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 209a3d4e1e..198eabe338 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1078,6 +1078,7 @@ static coroutine_fn void nbd_trip(void *opaque)
>      NBDReply reply;
>      int ret;
>      int flags;
> +    int reply_data_len = 0;
>  
>      TRACE("Reading request.");
>      if (client->closing) {
> @@ -1090,7 +1091,7 @@ static coroutine_fn void nbd_trip(void *opaque)
>      client->recv_coroutine = NULL;
>      nbd_client_receive_next_request(client);
>      if (ret == -EIO) {
> -        goto out;
> +        goto disconnect;
>      }
>  
>      reply.handle = request.handle;
> @@ -1098,7 +1099,7 @@ static coroutine_fn void nbd_trip(void *opaque)
>  
>      if (ret < 0) {
>          reply.error = -ret;
> -        goto error_reply;
> +        goto reply;
>      }
>  
>      if (client->closing) {
> @@ -1119,7 +1120,7 @@ static coroutine_fn void nbd_trip(void *opaque)
>              if (ret < 0) {
>                  LOG("flush failed");
>                  reply.error = -ret;
> -                goto error_reply;
> +                break;
>              }
>          }
>  
> @@ -1128,12 +1129,12 @@ static coroutine_fn void nbd_trip(void *opaque)
>          if (ret < 0) {
>              LOG("reading from file failed");
>              reply.error = -ret;
> -            goto error_reply;
> +            break;
>          }
>  
> +        reply_data_len = request.len;
>          TRACE("Read %" PRIu32" byte(s)", request.len);
> -        if (nbd_co_send_reply(req, &reply, request.len) < 0)
> -            goto out;
> +
>          break;
>      case NBD_CMD_WRITE:
>          TRACE("Request type is WRITE");
> @@ -1141,7 +1142,7 @@ static coroutine_fn void nbd_trip(void *opaque)
>          if (exp->nbdflags & NBD_FLAG_READ_ONLY) {
>              TRACE("Server is read-only, return error");
>              reply.error = EROFS;
> -            goto error_reply;
> +            break;
>          }
>  
>          TRACE("Writing to device");
> @@ -1155,21 +1156,16 @@ static coroutine_fn void nbd_trip(void *opaque)
>          if (ret < 0) {
>              LOG("writing to file failed");
>              reply.error = -ret;
> -            goto error_reply;
>          }
>  
> -        if (nbd_co_send_reply(req, &reply, 0) < 0) {
> -            goto out;
> -        }
>          break;
> -
>      case NBD_CMD_WRITE_ZEROES:
>          TRACE("Request type is WRITE_ZEROES");
>  
>          if (exp->nbdflags & NBD_FLAG_READ_ONLY) {
>              TRACE("Server is read-only, return error");
>              reply.error = EROFS;
> -            goto error_reply;
> +            break;
>          }
>  
>          TRACE("Writing to device");
> @@ -1186,14 +1182,9 @@ static coroutine_fn void nbd_trip(void *opaque)
>          if (ret < 0) {
>              LOG("writing to file failed");
>              reply.error = -ret;
> -            goto error_reply;
>          }
>  
> -        if (nbd_co_send_reply(req, &reply, 0) < 0) {
> -            goto out;
> -        }
>          break;
> -
>      case NBD_CMD_DISC:
>          /* unreachable, thanks to special case in nbd_co_receive_request() */
>          abort();
> @@ -1206,9 +1197,7 @@ static coroutine_fn void nbd_trip(void *opaque)
>              LOG("flush failed");
>              reply.error = -ret;
>          }
> -        if (nbd_co_send_reply(req, &reply, 0) < 0) {
> -            goto out;
> -        }
> +
>          break;
>      case NBD_CMD_TRIM:
>          TRACE("Request type is TRIM");
> @@ -1218,21 +1207,19 @@ static coroutine_fn void nbd_trip(void *opaque)
>              LOG("discard failed");
>              reply.error = -ret;
>          }
> -        if (nbd_co_send_reply(req, &reply, 0) < 0) {
> -            goto out;
> -        }
> +
>          break;
>      default:
>          LOG("invalid request type (%" PRIu32 ") received", request.type);
>          reply.error = EINVAL;
> -    error_reply:
> -        /* We must disconnect after NBD_CMD_WRITE if we did not
> -         * read the payload.
> -         */
> -        if (nbd_co_send_reply(req, &reply, 0) < 0 || !req->complete) {
> -            goto out;
> -        }
> -        break;
> +    }
> +
> +reply:
> +    /* We must disconnect after NBD_CMD_WRITE if we did not
> +     * read the payload.
> +     */
> +    if (nbd_co_send_reply(req, &reply, reply_data_len) < 0 || !req->complete) {
> +        goto disconnect;
>      }
>  
>      TRACE("Request/Reply complete");
> @@ -1242,7 +1229,7 @@ done:
>      nbd_client_put(client);
>      return;
>  
> -out:
> +disconnect:
>      nbd_request_put(req);
>      client_close(client);
>      nbd_client_put(client);
> 

Re: [Qemu-devel] [PATCH v2 03/12] nbd/server: get rid of nbd_negotiate_read and friends

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1924 bytes --]

On 06/02/2017 10:01 AM, Vladimir Sementsov-Ogievskiy wrote:
> Functions nbd_negotiate_{read,write,drop_sync} were introduced in
> 1a6245a5b, when nbd_rwv (was nbd_wr_sync) was working through
> qemu_co_sendv_recvv (the path is nbd_wr_sync -> qemu_co_{recv/send} ->
> qemu_co_send_recv -> qemu_co_sendv_recvv), which just yields, without
> setting any handlers. But starting from ff82911cd nbd_rwv (was
> nbd_wr_syncv) works through qio_channel_yield() which sets handlers, so
> watchers are redundant in nbd_negotiate_{read,write,drop_sync}, then,
> let's just use nbd_{read,write,drop} functions.
> 
> Functions nbd_{read,write,drop} has errp parameter, which is unused in
> this patch. This will be fixed later.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  nbd/server.c | 107 ++++++++++++-----------------------------------------------
>  1 file changed, 22 insertions(+), 85 deletions(-)

I did not realize it at the time, but this patch plugs a
denial-of-service security hole against malicious clients that were able
to trigger an assertion failure in the server by sending garbage during
negotiation; which was a regression introduced in the mentioned commit
ff82911cd.  This has now been assigned the identifier CVE-2017-7539

The fact that we have now had 4 CVEs against qemu's NBD implementation
in the last year means we are not doing a very good job of unit testing
either the server or the client against a malicious partner; I'm still
trying to figure out ways that we can improve our testsuite coverage
(testing that a sane client can still connect happens during
qemu-iotests, but most of our CVEs have happened due to poor reactions
to out-of-spec clients).

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]