This series is mostly about 9p request cancellation. It fixes a

long standing bug (read "specification violation") where the server
 would send an invalid response when the client has cancelled an
 in-flight request. This was causing annoying spurious EINTR returns
 in linux. The fix comes with some related testing in QTEST.
 
 Other patches are code cleanup and improvements.
 -----BEGIN PGP SIGNATURE-----
 
 iQIzBAABCAAdFiEEtIKLr5QxQM7yo0kQcdTV5YIvc9YFAlp0OmMACgkQcdTV5YIv
 c9bM2w/+ORsH7ifYVoPZDwPdhLAVg8t4aU7ak7kWSOeuTZhE8WA6GL8T4rjnhZZ4
 PbFZTye0fOsZZaT30sHUIwI/CGewvdx0kMJuX339XfwaWvsrF/8tHFA8QdreclVf
 YlLJwD2yZRujWPCU8k5D3hUfH2YD/w68b9Hv7K2hgsPw1Zoq7pLi/B2/BK/62G1d
 35nFG97xDmcScSybrIeLwaCIKW/wgjHsWgUgAVMpgaSwtu/JTtEG5JYCZLGO6HVa
 p67zdVQbCKQcdPLLllcrnGdWlA4CGxCBe+WMQyL3eJ2VDEH6XTBX8DiLstIVlXfj
 vXb7vjlsYxE6rdeiMXRZWDGK+jZIePmDsI6ILlpyL0rEphuBnjEMljCxyBnOp7gS
 dMTFmhfP4mApaYxeENlpQ5noDeQ/Jr1c9YeeBh9UnZ7r16/m3f2/sEIh+6QTL7YD
 iR7ZG5LF0eP5SJKoZNV0i4123A5I7v1ekn5N5q6Kf4PMcY11507lxZksPhckdPgp
 FPhsJJ1JQzWMcfE1mKeiZXq5Cys4EUhUK6UrOFp6tO38H/3YMuYMAz25D46Xk3Zx
 qGuptCaov9uPi+5uqKuG3aJ9nMIMW9ZJdMSWo2+kb4EX4hMiOewjOJtaKA11QeS+
 YoEr3szKtVnPUKsiqm+lhHL6ec5B8NQB7f8Hl88pkoLBGQzMaWU=
 =9T0i
 -----END PGP SIGNATURE-----

Merge remote-tracking branch 'remotes/gkurz/tags/for-upstream' into staging

This series is mostly about 9p request cancellation. It fixes a
long standing bug (read "specification violation") where the server
would send an invalid response when the client has cancelled an
in-flight request. This was causing annoying spurious EINTR returns
in linux. The fix comes with some related testing in QTEST.

Other patches are code cleanup and improvements.

# gpg: Signature made Fri 02 Feb 2018 10:16:03 GMT
# gpg:                using RSA key 71D4D5E5822F73D6
# gpg: Good signature from "Greg Kurz <groug@kaod.org>"
# gpg:                 aka "Gregory Kurz <gregory.kurz@free.fr>"
# gpg:                 aka "[jpeg image of size 3330]"
# Primary key fingerprint: B482 8BAF 9431 40CE F2A3  4910 71D4 D5E5 822F 73D6

* remotes/gkurz/tags/for-upstream:
  tests/virtio-9p: explicitly handle potential integer overflows
  tests: virtio-9p: add FLUSH operation test
  libqos/virtio: return length written into used descriptor
  tests: virtio-9p: add WRITE operation test
  tests: virtio-9p: add LOPEN operation test
  tests: virtio-9p: use the synth backend
  tests: virtio-9p: wait for completion in the test code
  tests: virtio-9p: move request tag to the test functions
  9pfs: Correctly handle cancelled requests
  9pfs: drop v9fs_register_transport()

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

hw/9pfs/9p-synth.c

@@ -19,6 +19,7 @@
 #include "qemu/rcu.h"
 #include "qemu/rcu_queue.h"
 #include "qemu/cutils.h"
+#include "sysemu/qtest.h"
 
 /* Root node for synth file system */
 static V9fsSynthNode synth_root = {
@@ -514,6 +515,26 @@ static int synth_unlinkat(FsContext *ctx, V9fsPath *dir,
     return -1;
 }
 
+static ssize_t v9fs_synth_qtest_write(void *buf, int len, off_t offset,
+                                      void *arg)
+{
+    return 1;
+}
+
+static ssize_t v9fs_synth_qtest_flush_write(void *buf, int len, off_t offset,
+                                            void *arg)
+{
+    bool should_block = !!*(uint8_t *)buf;
+
+    if (should_block) {
+        /* This will cause the server to call us again until we're cancelled */
+        errno = EINTR;
+        return -1;
+    }
+
+    return 1;
+}
+
 static int synth_init(FsContext *ctx, Error **errp)
 {
     QLIST_INIT(&synth_root.child);
@@ -527,6 +548,37 @@ static int synth_init(FsContext *ctx, Error **errp)
 
     /* Mark the subsystem is ready for use */
     synth_fs = 1;
+
+    if (qtest_enabled()) {
+        V9fsSynthNode *node = NULL;
+        int i, ret;
+
+        /* Directory hierarchy for WALK test */
+        for (i = 0; i < P9_MAXWELEM; i++) {
+            char *name = g_strdup_printf(QTEST_V9FS_SYNTH_WALK_FILE, i);
+
+            ret = qemu_v9fs_synth_mkdir(node, 0700, name, &node);
+            assert(!ret);
+            g_free(name);
+        }
+
+        /* File for LOPEN test */
+        ret = qemu_v9fs_synth_add_file(NULL, 0, QTEST_V9FS_SYNTH_LOPEN_FILE,
+                                       NULL, NULL, ctx);
+        assert(!ret);
+
+        /* File for WRITE test */
+        ret = qemu_v9fs_synth_add_file(NULL, 0, QTEST_V9FS_SYNTH_WRITE_FILE,
+                                       NULL, v9fs_synth_qtest_write, ctx);
+        assert(!ret);
+
+        /* File for FLUSH test */
+        ret = qemu_v9fs_synth_add_file(NULL, 0, QTEST_V9FS_SYNTH_FLUSH_FILE,
+                                       NULL, v9fs_synth_qtest_flush_write,
+                                       ctx);
+        assert(!ret);
+    }
+
     return 0;
 }
 

hw/9pfs/9p-synth.h

@@ -49,4 +49,17 @@ int qemu_v9fs_synth_add_file(V9fsSynthNode *parent, int mode,
                              const char *name, v9fs_synth_read read,
                              v9fs_synth_write write, void *arg);
 
+/* qtest stuff */
+
+#define QTEST_V9FS_SYNTH_WALK_FILE "WALK%d"
+#define QTEST_V9FS_SYNTH_LOPEN_FILE "LOPEN"
+#define QTEST_V9FS_SYNTH_WRITE_FILE "WRITE"
+
+/* Any write to the "FLUSH" file is handled one byte at a time by the
+ * backend. If the byte is zero, the backend returns success (ie, 1),
+ * otherwise it forces the server to try again forever. Thus allowing
+ * the client to cancel the request.
+ */
+#define QTEST_V9FS_SYNTH_FLUSH_FILE "FLUSH"
+
 #endif

hw/9pfs/9p.c

@@ -24,6 +24,7 @@
 #include "coth.h"
 #include "trace.h"
 #include "migration/blocker.h"
+#include "sysemu/qtest.h"
 
 int open_fd_hw;
 int total_open_fd;
@@ -630,6 +631,24 @@ static void coroutine_fn pdu_complete(V9fsPDU *pdu, ssize_t len)
     V9fsState *s = pdu->s;
     int ret;
 
+    /*
+     * The 9p spec requires that successfully cancelled pdus receive no reply.
+     * Sending a reply would confuse clients because they would
+     * assume that any EINTR is the actual result of the operation,
+     * rather than a consequence of the cancellation. However, if
+     * the operation completed (succesfully or with an error other
+     * than caused be cancellation), we do send out that reply, both
+     * for efficiency and to avoid confusing the rest of the state machine
+     * that assumes passing a non-error here will mean a successful
+     * transmission of the reply.
+     */
+    bool discard = pdu->cancelled && len == -EINTR;
+    if (discard) {
+        trace_v9fs_rcancel(pdu->tag, pdu->id);
+        pdu->size = 0;
+        goto out_notify;
+    }
+
     if (len < 0) {
         int err = -len;
         len = 7;
@@ -3485,7 +3504,8 @@ void pdu_submit(V9fsPDU *pdu, P9MsgHeader *hdr)
 }
 
 /* Returns 0 on success, 1 on failure. */
-int v9fs_device_realize_common(V9fsState *s, Error **errp)
+int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
+                               Error **errp)
 {
     int i, len;
     struct stat stat;
@@ -3493,6 +3513,9 @@ int v9fs_device_realize_common(V9fsState *s, Error **errp)
     V9fsPath path;
     int rc = 1;
 
+    assert(!s->transport);
+    s->transport = t;
+
     /* initialize pdu allocator */
     QLIST_INIT(&s->free_list);
     QLIST_INIT(&s->active_list);

hw/9pfs/9p.h

@@ -346,7 +346,8 @@ void v9fs_path_sprintf(V9fsPath *path, const char *fmt, ...);
 void v9fs_path_copy(V9fsPath *lhs, V9fsPath *rhs);
 int v9fs_name_to_path(V9fsState *s, V9fsPath *dirpath,
                       const char *name, V9fsPath *path);
-int v9fs_device_realize_common(V9fsState *s, Error **errp);
+int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
+                               Error **errp);
 void v9fs_device_unrealize_common(V9fsState *s, Error **errp);
 
 V9fsPDU *pdu_alloc(V9fsState *s);
@@ -366,11 +367,4 @@ struct V9fsTransport {
     void        (*push_and_notify)(V9fsPDU *pdu);
 };
 
-static inline int v9fs_register_transport(V9fsState *s, const V9fsTransport *t)
-{
-    assert(!s->transport);
-    s->transport = t;
-    return 0;
-}
-
 #endif

hw/9pfs/trace-events

@@ -1,6 +1,7 @@
 # See docs/devel/tracing.txt for syntax documentation.
 
 # hw/9pfs/virtio-9p.c
+v9fs_rcancel(uint16_t tag, uint8_t id) "tag %d id %d"
 v9fs_rerror(uint16_t tag, uint8_t id, int err) "tag %d id %d err %d"
 v9fs_version(uint16_t tag, uint8_t id, int32_t msize, char* version) "tag %d id %d msize %d version %s"
 v9fs_version_return(uint16_t tag, uint8_t id, int32_t msize, char* version) "tag %d id %d msize %d version %s"

hw/9pfs/virtio-9p-device.c

@@ -198,17 +198,13 @@ static void virtio_9p_device_realize(DeviceState *dev, Error **errp)
     V9fsVirtioState *v = VIRTIO_9P(dev);
     V9fsState *s = &v->state;
 
-    if (v9fs_device_realize_common(s, errp)) {
-        goto out;
+    if (v9fs_device_realize_common(s, &virtio_9p_transport, errp)) {
+        return;
     }
 
     v->config_size = sizeof(struct virtio_9p_config) + strlen(s->fsconf.tag);
     virtio_init(vdev, "virtio-9p", VIRTIO_ID_9P, v->config_size);
     v->vq = virtio_add_queue(vdev, MAX_REQ, handle_9p_output);
-    v9fs_register_transport(s, &virtio_9p_transport);
-
-out:
-    return;
 }
 
 static void virtio_9p_device_unrealize(DeviceState *dev, Error **errp)

hw/9pfs/xen-9p-backend.c

@@ -446,7 +446,6 @@ static int xen_9pfs_connect(struct XenDevice *xendev)
     xen_9pdev->id = s->fsconf.fsdev_id =
         g_strdup_printf("xen9p%d", xendev->dev);
     xen_9pdev->tag = s->fsconf.tag = xenstore_read_fe_str(xendev, "tag");
-    v9fs_register_transport(s, &xen_9p_transport);
     fsdev = qemu_opts_create(qemu_find_opts("fsdev"),
             s->fsconf.tag,
             1, NULL);
@@ -455,7 +454,7 @@ static int xen_9pfs_connect(struct XenDevice *xendev)
     qemu_opt_set(fsdev, "security_model", xen_9pdev->security_model, NULL);
     qemu_opts_set_id(fsdev, s->fsconf.fsdev_id);
     qemu_fsdev_add(fsdev);
-    v9fs_device_realize_common(s, NULL);
+    v9fs_device_realize_common(s, &xen_9p_transport, NULL);
 
     return 0;