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

[Qemu-devel] [PATCH 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"

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 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
   write to socket returns EAGAIN. In first implementation nbd_wr_syncv
   just loops while getting EAGAIN, current implementation yields in
   this case.
   Why to get rid of it:
   - it is normal for r/w functions to be synchronous, so having
     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'

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..630f553134 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 are no needs 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 02/12] nbd: make nbd_drop public

[Vladimir Sementsov-Ogievskiy]

Following commit will reuse it for nbd server too.

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 630f553134..d1c2bed471 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 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.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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 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 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 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 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 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 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 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 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 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

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

[Vladimir Sementsov-Ogievskiy]

31.05.2017 19:55, Vladimir Sementsov-Ogievskiy wrote:
> 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
>     write to socket returns EAGAIN. In first implementation nbd_wr_syncv

not bad to add here the following note in parentheses. Please add it in 
flight, if no more updates required for the series.
(was wr_sync in 7a5ca8648b)

>     just loops while getting EAGAIN, current implementation yields in
>     this case.
>     Why to get rid of it:
>     - it is normal for r/w functions to be synchronous, so having
>       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'
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---

-- 
Best regards,
Vladimir

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

[Eric Blake]

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

On 05/31/2017 11:55 AM, Vladimir Sementsov-Ogievskiy wrote:
> 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

s/fact,/fact/

>    write to socket returns EAGAIN. In first implementation nbd_wr_syncv
>    just loops while getting EAGAIN, current implementation yields in
>    this case.

As mentioned in your followup, you may want to rewrite this to:

_sync was originally used (back in commit 7a5ca864 when it was named
wr_sync) to indicate that we looped rather than returned on EAGAIN.  But
now we use qio_channel which yields on our behalf rather than giving us
EAGAIN.

>    Why to get rid of it:
>    - it is normal for r/w functions to be synchronous, so having
>      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'
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---

The maintainer may be willing to tweak the commit message without
needing a v2.


> +++ 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 are no needs to call nbd_read_eof

As long as you are touching this:

s/are no needs/is no need/

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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: 604 bytes --]

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

[Eric Blake]

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

On 05/31/2017 11:55 AM, Vladimir Sementsov-Ogievskiy wrote:
> Following commit will reuse it for nbd server too.
> 
> 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(-)
> 
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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: 604 bytes --]

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

[Eric Blake]

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

On 05/31/2017 11:55 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.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  nbd/server.c | 107 ++++++++++++-----------------------------------------------
>  1 file changed, 22 insertions(+), 85 deletions(-)


>     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;

This says that you are ignoring errors (via errp) rather than reporting
them (because we LOG() it instead).  You do clean it up later, but it
would be nice to mention that in the commit message.

But because we have the later error fixups, I'm okay with:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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: 604 bytes --]

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

[Sementsov-Ogievskiy Vladimir]

On 31.05.2017 21:46, Eric Blake wrote:
> On 05/31/2017 11:55 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 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
> s/fact,/fact/
>
>>     write to socket returns EAGAIN. In first implementation nbd_wr_syncv
>>     just loops while getting EAGAIN, current implementation yields in
>>     this case.
> As mentioned in your followup, you may want to rewrite this to:
>
> _sync was originally used (back in commit 7a5ca864 when it was named
> wr_sync) to indicate that we looped rather than returned on EAGAIN.  But
> now we use qio_channel which yields on our behalf rather than giving us
> EAGAIN.

hmm, I like my wording (with adding note "... implementaion nbd_wr_syncv 
(was wr_sync in 7a5ca8648b) just ...")  more, because:
1. not only nbd_wr_syncv has that suffix, so nbd_wr_syncv should be 
mentioned (as we mention wr_sync)
2. I don't say about contrast between old and current, I say that they 
are similar.


>
>>     Why to get rid of it:
>>     - it is normal for r/w functions to be synchronous, so having
>>       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'
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
> The maintainer may be willing to tweak the commit message without
> needing a v2.

I hope for it)

>
>
>> +++ 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 are no needs to call nbd_read_eof
> As long as you are touching this:
>
> s/are no needs/is no need/
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>

-- 
Best regards,
Vladimir.

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

[Vladimir Sementsov-Ogievskiy]

01.06.2017 11:03, Sementsov-Ogievskiy Vladimir wrote:
>
>
> On 31.05.2017 21:46, Eric Blake wrote:
>> On 05/31/2017 11:55 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> 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
>> s/fact,/fact/
>>
>>>     write to socket returns EAGAIN. In first implementation 
>>> nbd_wr_syncv
>>>     just loops while getting EAGAIN, current implementation yields in
>>>     this case.
>> As mentioned in your followup, you may want to rewrite this to:
>>
>> _sync was originally used (back in commit 7a5ca864 when it was named
>> wr_sync) to indicate that we looped rather than returned on EAGAIN.  But
>> now we use qio_channel which yields on our behalf rather than giving us
>> EAGAIN.
>
> hmm, I like my wording (with adding note "... implementaion 
> nbd_wr_syncv (was wr_sync in 7a5ca8648b) just ...")  more, because:
> 1. not only nbd_wr_syncv has that suffix, so nbd_wr_syncv should be 
> mentioned (as we mention wr_sync)
> 2. I don't say about contrast between old and current, I say that they 
> are similar.
>

Finally, are you OK with my wording? If I reroll, can I add your r-b?

>
>>
>>>     Why to get rid of it:
>>>     - it is normal for r/w functions to be synchronous, so having
>>>       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'
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>> The maintainer may be willing to tweak the commit message without
>> needing a v2.
>
> I hope for it)
>
>>
>>
>>> +++ 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 are no needs to call 
>>> nbd_read_eof
>> As long as you are touching this:
>>
>> s/are no needs/is no need/
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
>

-- 
Best regards,
Vladimir

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

[Eric Blake]

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

On 06/02/2017 07:00 AM, Vladimir Sementsov-Ogievskiy wrote:

>>>> 2. _sync suffix
>>>>     _sync is related to the fact, that nbd_wr_syncv doesn't return if
>>> s/fact,/fact/
>>>
>>>>     write to socket returns EAGAIN. In first implementation
>>>> nbd_wr_syncv
>>>>     just loops while getting EAGAIN, current implementation yields in
>>>>     this case.
>>> As mentioned in your followup, you may want to rewrite this to:
>>>
>>> _sync was originally used (back in commit 7a5ca864 when it was named
>>> wr_sync) to indicate that we looped rather than returned on EAGAIN.  But
>>> now we use qio_channel which yields on our behalf rather than giving us
>>> EAGAIN.
>>
>> hmm, I like my wording (with adding note "... implementaion
>> nbd_wr_syncv (was wr_sync in 7a5ca8648b) just ...")  more, because:
>> 1. not only nbd_wr_syncv has that suffix, so nbd_wr_syncv should be
>> mentioned (as we mention wr_sync)
>> 2. I don't say about contrast between old and current, I say that they
>> are similar.
>>
> 
> Finally, are you OK with my wording? If I reroll, can I add your r-b?

What final wording are you proposing (full paragraph, not a snippet)?

>>>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>

At any rate, I already gave R-b for the code, so finessing the commit
message doesn't change that if the code remains unchanged.

-- 
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: 604 bytes --]

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

[Vladimir Sementsov-Ogievskiy]

02.06.2017 16:49, Eric Blake wrote:
> On 06/02/2017 07:00 AM, Vladimir Sementsov-Ogievskiy wrote:
>
>>>>> 2. _sync suffix
>>>>>      _sync is related to the fact, that nbd_wr_syncv doesn't return if
>>>> s/fact,/fact/
>>>>
>>>>>      write to socket returns EAGAIN. In first implementation
>>>>> nbd_wr_syncv
>>>>>      just loops while getting EAGAIN, current implementation yields in
>>>>>      this case.
>>>> As mentioned in your followup, you may want to rewrite this to:
>>>>
>>>> _sync was originally used (back in commit 7a5ca864 when it was named
>>>> wr_sync) to indicate that we looped rather than returned on EAGAIN.  But
>>>> now we use qio_channel which yields on our behalf rather than giving us
>>>> EAGAIN.
>>> hmm, I like my wording (with adding note "... implementaion
>>> nbd_wr_syncv (was wr_sync in 7a5ca8648b) just ...")  more, because:
>>> 1. not only nbd_wr_syncv has that suffix, so nbd_wr_syncv should be
>>> mentioned (as we mention wr_sync)
>>> 2. I don't say about contrast between old and current, I say that they
>>> are similar.
>>>
>> Finally, are you OK with my wording? If I reroll, can I add your r-b?
> What final wording are you proposing (full paragraph, not a snippet)?

2. _sync suffix
    _sync is related to the fact that nbd_wr_syncv doesn't return if
    write to socket returns EAGAIN. In first implementation nbd_wr_syncv
    (was wr_sync in 7a5ca8648b) just loops while getting EAGAIN, current
    implementation yields in this case.
    Why to get rid of it:
    - it is normal for r/w functions to be synchronous, so having
      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

>
>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
> At any rate, I already gave R-b for the code, so finessing the commit
> message doesn't change that if the code remains unchanged.
>

-- 
Best regards,
Vladimir

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

[Eric Blake]

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

On 06/02/2017 08:54 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Finally, are you OK with my wording? If I reroll, can I add your r-b?
>> What final wording are you proposing (full paragraph, not a snippet)?
> 
> 2. _sync suffix
>    _sync is related to the fact that nbd_wr_syncv doesn't return if

s/if/if a/

>    write to socket returns EAGAIN. In first implementation nbd_wr_syncv

s/In first implementation/The first implementation of/

>    (was wr_sync in 7a5ca8648b) just loops while getting EAGAIN, current

s/current/the current/

>    implementation yields in this case.
>    Why to get rid of it:

maybe: s/Why/Why we want/

>    - it is normal for r/w functions to be synchronous, so having

s/having/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

Thanks for bearing with me, and letting me help on the grammar subtleties.

>>
>>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> At any rate, I already gave R-b for the code, so finessing the commit
>> message doesn't change that if the code remains unchanged.
>>
> 

-- 
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: 604 bytes --]

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

[Vladimir Sementsov-Ogievskiy]

02.06.2017 17:15, Eric Blake wrote:
> On 06/02/2017 08:54 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> Finally, are you OK with my wording? If I reroll, can I add your r-b?
>>> What final wording are you proposing (full paragraph, not a snippet)?
>> 2. _sync suffix
>>     _sync is related to the fact that nbd_wr_syncv doesn't return if
> s/if/if a/
>
>>     write to socket returns EAGAIN. In first implementation nbd_wr_syncv
> s/In first implementation/The first implementation of/
>
>>     (was wr_sync in 7a5ca8648b) just loops while getting EAGAIN, current
> s/current/the current/
>
>>     implementation yields in this case.
>>     Why to get rid of it:
> maybe: s/Why/Why we want/
>
>>     - it is normal for r/w functions to be synchronous, so having
> s/having/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
> Thanks for bearing with me, and letting me help on the grammar subtleties.

No problem, thank you too)

>
>>>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> At any rate, I already gave R-b for the code, so finessing the commit
>>> message doesn't change that if the code remains unchanged.
>>>

-- 
Best regards,
Vladimir