nbd patches for 2017-09-06

- Daniel P. Berrange: [0/2] Fix / skip recent iotests with LUKS driver
 - Eric Blake: [0/3] nbd: Use common read/write-all qio functions
 -----BEGIN PGP SIGNATURE-----
 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg
 
 iQEcBAABCAAGBQJZsBGjAAoJEKeha0olJ0NqVRoH/iiNEB2SlZFFl5W++wf3Ekq/
 lvtZjK3rxpvRXvy6LiRsYVs27Etc8E9aSw2UK6aaqgA3qR8g3zdmwUZb9w3slkeI
 OXedt0fS5IpQ4UP0ORUBb/LgyOgW3uA0UjHBTEAKl0SyvFPx+TrTZXxqQUqlAc9A
 lFaA0g71xvfqWWhXmt0PQjRr9bBEpe+4L4NgOypa+Z3xbBAektx390S8N/b/P8fC
 FNwAqBPTY5XAgJGnEhL9EUOdUWnVgoyG1MR63puJzULYi+2+TlpR2w030qRif75b
 h7TqYUvwKLnoqMyhBb5LmyhcqwNdphz/1DsEudk18XGuvC94WYkopC3rT7TPWLs=
 =vGUc
 -----END PGP SIGNATURE-----

Merge remote-tracking branch 'remotes/ericb/tags/pull-nbd-2017-09-06' into staging

nbd patches for 2017-09-06

- Daniel P. Berrange: [0/2] Fix / skip recent iotests with LUKS driver
- Eric Blake: [0/3] nbd: Use common read/write-all qio functions

# gpg: Signature made Wed 06 Sep 2017 16:17:55 BST
# gpg:                using RSA key 0xA7A16B4A2527436A
# gpg: Good signature from "Eric Blake <eblake@redhat.com>"
# gpg:                 aka "Eric Blake (Free Software Programmer) <ebb9@byu.net>"
# gpg:                 aka "[jpeg image of size 6874]"
# Primary key fingerprint: 71C2 CC22 B1C4 6029 27D2  F3AA A7A1 6B4A 2527 436A

* remotes/ericb/tags/pull-nbd-2017-09-06:
  nbd: Use new qio_channel_*_all() functions
  io: Add new qio_channel_read{, v}_all_eof functions
  io: Yield rather than wait when already in coroutine
  iotests: blacklist 194 with the luks driver
  iotests: rewrite 192 to use _launch_qemu to fix LUKS support

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

block/nbd-client.c

@@ -121,7 +121,7 @@ static int nbd_co_send_request(BlockDriverState *bs,
                                QEMUIOVector *qiov)
 {
     NBDClientSession *s = nbd_get_client_session(bs);
-    int rc, ret, i;
+    int rc, i;
 
     qemu_co_mutex_lock(&s->send_mutex);
     while (s->in_flight == MAX_NBD_REQUESTS) {
@@ -156,9 +156,9 @@ 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 && !s->quit) {
-            ret = nbd_rwv(s->ioc, qiov->iov, qiov->niov, request->len, false,
+            assert(request->len == iov_size(qiov->iov, qiov->niov));
-                          NULL);
+            if (qio_channel_writev_all(s->ioc, qiov->iov, qiov->niov,
-            if (ret != request->len) {
+                                       NULL) < 0) {
                 rc = -EIO;
             }
         }
@@ -184,7 +184,6 @@ static void nbd_co_receive_reply(NBDClientSession *s,
                                  QEMUIOVector *qiov)
 {
     int i = HANDLE_TO_INDEX(s, request->handle);
-    int ret;
 
     /* Wait until we're woken up by nbd_read_reply_entry.  */
     s->requests[i].receiving = true;
@@ -195,9 +194,9 @@ static void nbd_co_receive_reply(NBDClientSession *s,
         reply->error = EIO;
     } else {
         if (qiov && reply->error == 0) {
-            ret = nbd_rwv(s->ioc, qiov->iov, qiov->niov, request->len, true,
+            assert(request->len == iov_size(qiov->iov, qiov->niov));
-                          NULL);
+            if (qio_channel_readv_all(s->ioc, qiov->iov, qiov->niov,
-            if (ret != request->len) {
+                                      NULL) < 0) {
                 reply->error = EIO;
                 s->quit = true;
             }

include/block/nbd.h

@@ -155,8 +155,6 @@ struct NBDExportInfo {
 };
 typedef struct NBDExportInfo NBDExportInfo;
 
-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,
                           QCryptoTLSCreds *tlscreds, const char *hostname,
                           QIOChannel **outioc, NBDExportInfo *info,

include/io/channel.h

@@ -268,6 +268,36 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc,
                                 size_t nfds,
                                 Error **errp);
 
+/**
+ * qio_channel_readv_all_eof:
+ * @ioc: the channel object
+ * @iov: the array of memory regions to read data into
+ * @niov: the length of the @iov array
+ * @errp: pointer to a NULL-initialized error object
+ *
+ * Read data from the IO channel, storing it in the
+ * memory regions referenced by @iov. Each element
+ * in the @iov will be fully populated with data
+ * before the next one is used. The @niov parameter
+ * specifies the total number of elements in @iov.
+ *
+ * The function will wait for all requested data
+ * to be read, yielding from the current coroutine
+ * if required.
+ *
+ * If end-of-file occurs before any data is read,
+ * no error is reported; otherwise, if it occurs
+ * before all requested data has been read, an error
+ * will be reported.
+ *
+ * Returns: 1 if all bytes were read, 0 if end-of-file
+ *          occurs without data, or -1 on error
+ */
+int qio_channel_readv_all_eof(QIOChannel *ioc,
+                              const struct iovec *iov,
+                              size_t niov,
+                              Error **errp);
+
 /**
  * qio_channel_readv_all:
  * @ioc: the channel object
@@ -382,6 +412,28 @@ ssize_t qio_channel_write(QIOChannel *ioc,
                           size_t buflen,
                           Error **errp);
 
+/**
+ * qio_channel_read_all_eof:
+ * @ioc: the channel object
+ * @buf: the memory region to read data into
+ * @buflen: the number of bytes to @buf
+ * @errp: pointer to a NULL-initialized error object
+ *
+ * Reads @buflen bytes into @buf, possibly blocking or (if the
+ * channel is non-blocking) yielding from the current coroutine
+ * multiple times until the entire content is read. If end-of-file
+ * occurs immediately it is not an error, but if it occurs after
+ * data has been read it will return an error rather than a
+ * short-read. Otherwise behaves as qio_channel_read().
+ *
+ * Returns: 1 if all bytes were read, 0 if end-of-file occurs
+ *          without data, or -1 on error
+ */
+int qio_channel_read_all_eof(QIOChannel *ioc,
+                             char *buf,
+                             size_t buflen,
+                             Error **errp);
+
 /**
  * qio_channel_read_all:
  * @ioc: the channel object
@@ -401,6 +453,7 @@ int qio_channel_read_all(QIOChannel *ioc,
                          char *buf,
                          size_t buflen,
                          Error **errp);
+
 /**
  * qio_channel_write_all:
  * @ioc: the channel object

io/channel.c

@@ -86,16 +86,16 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc,
 }
 
 
-
+int qio_channel_readv_all_eof(QIOChannel *ioc,
-int qio_channel_readv_all(QIOChannel *ioc,
+                              const struct iovec *iov,
-                          const struct iovec *iov,
+                              size_t niov,
-                          size_t niov,
+                              Error **errp)
-                          Error **errp)
 {
     int ret = -1;
     struct iovec *local_iov = g_new(struct iovec, niov);
     struct iovec *local_iov_head = local_iov;
     unsigned int nlocal_iov = niov;
+    bool partial = false;
 
     nlocal_iov = iov_copy(local_iov, nlocal_iov,
                           iov, niov,
@@ -105,26 +105,52 @@ int qio_channel_readv_all(QIOChannel *ioc,
         ssize_t len;
         len = qio_channel_readv(ioc, local_iov, nlocal_iov, errp);
         if (len == QIO_CHANNEL_ERR_BLOCK) {
-            qio_channel_wait(ioc, G_IO_IN);
+            if (qemu_in_coroutine()) {
+                qio_channel_yield(ioc, G_IO_IN);
+            } else {
+                qio_channel_wait(ioc, G_IO_IN);
+            }
             continue;
         } else if (len < 0) {
             goto cleanup;
         } else if (len == 0) {
-            error_setg(errp,
+            if (partial) {
-                       "Unexpected end-of-file before all bytes were read");
+                error_setg(errp,
+                           "Unexpected end-of-file before all bytes were read");
+            } else {
+                ret = 0;
+            }
             goto cleanup;
         }
 
+        partial = true;
         iov_discard_front(&local_iov, &nlocal_iov, len);
     }
 
-    ret = 0;
+    ret = 1;
 
  cleanup:
     g_free(local_iov_head);
     return ret;
 }
 
+int qio_channel_readv_all(QIOChannel *ioc,
+                          const struct iovec *iov,
+                          size_t niov,
+                          Error **errp)
+{
+    int ret = qio_channel_readv_all_eof(ioc, iov, niov, errp);
+
+    if (ret == 0) {
+        ret = -1;
+        error_setg(errp,
+                   "Unexpected end-of-file before all bytes were read");
+    } else if (ret == 1) {
+        ret = 0;
+    }
+    return ret;
+}
+
 int qio_channel_writev_all(QIOChannel *ioc,
                            const struct iovec *iov,
                            size_t niov,
@@ -143,7 +169,11 @@ int qio_channel_writev_all(QIOChannel *ioc,
         ssize_t len;
         len = qio_channel_writev(ioc, local_iov, nlocal_iov, errp);
         if (len == QIO_CHANNEL_ERR_BLOCK) {
-            qio_channel_wait(ioc, G_IO_OUT);
+            if (qemu_in_coroutine()) {
+                qio_channel_yield(ioc, G_IO_OUT);
+            } else {
+                qio_channel_wait(ioc, G_IO_OUT);
+            }
             continue;
         }
         if (len < 0) {
@@ -197,6 +227,16 @@ ssize_t qio_channel_write(QIOChannel *ioc,
 }
 
 
+int qio_channel_read_all_eof(QIOChannel *ioc,
+                             char *buf,
+                             size_t buflen,
+                             Error **errp)
+{
+    struct iovec iov = { .iov_base = buf, .iov_len = buflen };
+    return qio_channel_readv_all_eof(ioc, &iov, 1, errp);
+}
+
+
 int qio_channel_read_all(QIOChannel *ioc,
                          char *buf,
                          size_t buflen,

nbd/common.c

@@ -20,51 +20,6 @@
 #include "qapi/error.h"
 #include "nbd-internal.h"
 
-/* nbd_wr_syncv
- * 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_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);
-    struct iovec *local_iov_head = local_iov;
-    unsigned int nlocal_iov = niov;
-
-    nlocal_iov = iov_copy(local_iov, nlocal_iov, iov, niov, 0, length);
-
-    while (nlocal_iov > 0) {
-        ssize_t len;
-        if (do_read) {
-            len = qio_channel_readv(ioc, local_iov, nlocal_iov, errp);
-        } else {
-            len = qio_channel_writev(ioc, local_iov, nlocal_iov, errp);
-        }
-        if (len == QIO_CHANNEL_ERR_BLOCK) {
-            /* errp should not be set */
-            assert(qemu_in_coroutine());
-            qio_channel_yield(ioc, do_read ? G_IO_IN : G_IO_OUT);
-            continue;
-        }
-        if (len < 0) {
-            done = -EIO;
-            goto cleanup;
-        }
-
-        if (do_read && len == 0) {
-            break;
-        }
-
-        iov_discard_front(&local_iov, &nlocal_iov, len);
-        done += len;
-    }
-
- cleanup:
-    g_free(local_iov_head);
-    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)

nbd/nbd-internal.h

@@ -85,28 +85,14 @@
 static inline int nbd_read_eof(QIOChannel *ioc, void *buffer, size_t size,
                                Error **errp)
 {
-    struct iovec iov = { .iov_base = buffer, .iov_len = size };
+    int ret;
-    ssize_t ret;
-
-    /* Sockets are kept in blocking mode in the negotiation phase.  After
-     * that, a non-readable socket simply means that another thread stole
-     * our request/reply.  Synchronization is done with recv_coroutine, so
-     * that this is coroutine-safe.
-     */
 
     assert(size);
-
+    ret = qio_channel_read_all_eof(ioc, buffer, size, errp);
-    ret = nbd_rwv(ioc, &iov, 1, size, true, errp);
+    if (ret < 0) {
-    if (ret <= 0) {
+        ret = -EIO;
-        return ret;
     }
-
+    return ret;
-    if (ret != size) {
-        error_setg(errp, "End of file");
-        return -EINVAL;
-    }
-
-    return 1;
 }
 
 /* nbd_read
@@ -115,14 +101,7 @@ static inline int nbd_read_eof(QIOChannel *ioc, void *buffer, size_t size,
 static inline int nbd_read(QIOChannel *ioc, void *buffer, size_t size,
                            Error **errp)
 {
-    int ret = nbd_read_eof(ioc, buffer, size, errp);
+    return qio_channel_read_all(ioc, buffer, size, errp) < 0 ? -EIO : 0;
-
-    if (ret == 0) {
-        ret = -EINVAL;
-        error_setg(errp, "End of file");
-    }
-
-    return ret < 0 ? ret : 0;
 }
 
 /* nbd_write
@@ -131,13 +110,7 @@ static inline int nbd_read(QIOChannel *ioc, void *buffer, size_t size,
 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 };
+    return qio_channel_write_all(ioc, buffer, size, errp) < 0 ? -EIO : 0;
-
-    ssize_t ret = nbd_rwv(ioc, &iov, 1, size, false, errp);
-
-    assert(ret < 0 || ret == size);
-
-    return ret < 0 ? ret : 0;
 }
 
 struct NBDTLSHandshakeData {

tests/qemu-iotests/083.out

@@ -69,12 +69,12 @@ read failed: Input/output error
 
 === Check disconnect 4 reply ===
 
-End of file
+Unexpected end-of-file before all bytes were read
 read failed: Input/output error
 
 === Check disconnect 8 reply ===
 
-End of file
+Unexpected end-of-file before all bytes were read
 read failed: Input/output error
 
 === Check disconnect before data ===
@@ -180,12 +180,12 @@ read failed: Input/output error
 
 === Check disconnect 4 reply ===
 
-End of file
+Unexpected end-of-file before all bytes were read
 read failed: Input/output error
 
 === Check disconnect 8 reply ===
 
-End of file
+Unexpected end-of-file before all bytes were read
 read failed: Input/output error
 
 === Check disconnect before data ===

tests/qemu-iotests/192

@@ -37,6 +37,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 # get standard environment, filters and checks
 . ./common.rc
 . ./common.filter
+. ./common.qemu
 
 _supported_fmt generic
 _supported_proto file
@@ -49,13 +50,21 @@ fi
 size=64M
 _make_test_img $size
 
-{
+if test "$IMGOPTSSYNTAX" = "true"
-echo "nbd_server_start unix:$TEST_DIR/nbd"
+then
-echo "nbd_server_add -w drive0"
+    DRIVE_ARG=if=ide,id=drive0,$TEST_IMG
-echo "q"
+else
-} | $QEMU -nodefaults -display none -monitor stdio \
+    DRIVE_ARG=if=ide,id=drive0,format=$IMGFMT,file=$TEST_IMG
-    -drive format=$IMGFMT,file=$TEST_IMG,if=ide,id=drive0 \
+fi
-    -incoming defer 2>&1 | _filter_testdir | _filter_qemu | _filter_hmp
+
+qemu_comm_method="monitor"
+_launch_qemu -drive $DRIVE_ARG -incoming defer
+h=$QEMU_HANDLE
+QEMU_COMM_TIMEOUT=1
+
+_send_qemu_cmd $h "nbd_server_start unix:$TEST_DIR/nbd" "(qemu)"
+_send_qemu_cmd $h "nbd_server_add -w drive0" "(qemu)"
+_send_qemu_cmd $h "q" "(qemu)"
 
 # success, all done
 echo "*** done"

tests/qemu-iotests/194

@@ -21,6 +21,7 @@
 
 import iotests
 
+iotests.verify_image_format(unsupported_fmts=['luks'])
 iotests.verify_platform(['linux'])
 
 with iotests.FilePath('source.img') as source_img_path, \

tests/qemu-iotests/iotests.py

@@ -421,9 +421,11 @@ def notrun(reason):
     print '%s not run: %s' % (seq, reason)
     sys.exit(0)
 
-def verify_image_format(supported_fmts=[]):
+def verify_image_format(supported_fmts=[], unsupported_fmts=[]):
     if supported_fmts and (imgfmt not in supported_fmts):
         notrun('not suitable for this image format: %s' % imgfmt)
+    if unsupported_fmts and (imgfmt in unsupported_fmts):
+        notrun('not suitable for this image format: %s' % imgfmt)
 
 def verify_platform(supported_oses=['linux']):
     if True not in [sys.platform.startswith(x) for x in supported_oses]: