All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 11/11] tests/9pfs: add local Tmkdir test
  2020-10-02 11:52 [PATCH v2 00/11] 9pfs: add tests using local fs driver Christian Schoenebeck
                   ` (9 preceding siblings ...)
  2020-10-02 11:51 ` [PATCH v2 01/11] libqos/qgraph: add qemu_name to QOSGraphNode Christian Schoenebeck
@ 2020-10-02 11:51 ` Christian Schoenebeck
  2020-10-02 12:56   ` Daniel P. Berrangé
  10 siblings, 1 reply; 22+ messages in thread
From: Christian Schoenebeck @ 2020-10-02 11:51 UTC (permalink / raw)
  To: qemu-devel
  Cc: Thomas Huth, Laurent Vivier, Paolo Bonzini,
	Emanuele Giuseppe Esposito, Greg Kurz

This test case uses the 9pfs 'local' driver to create a directory
and then checks if the expected directory was actually created
(as real directory) on host side.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/virtio-9p-test.c | 139 +++++++++++++++++++++++++++++++++++
 1 file changed, 139 insertions(+)

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index af7e169d3a..93161a4b35 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -18,6 +18,62 @@
 #define QVIRTIO_9P_TIMEOUT_US (10 * 1000 * 1000)
 static QGuestAllocator *alloc;
 
+/*
+ * Used to auto generate new fids. Start with arbitrary high value to avoid
+ * collision with hard coded fids in basic test code.
+ */
+static uint32_t fid_generator = 1000;
+
+static uint32_t genfid(void)
+{
+    return fid_generator++;
+}
+
+/**
+ * Splits the @a in string by @a delim into individual (non empty) strings
+ * and outputs them to @a out. The output array @a out is NULL terminated.
+ *
+ * Output array @a out must be freed by calling split_free().
+ *
+ * @returns number of individual elements in output array @a out (without the
+ *          final NULL terminating element)
+ */
+static int split(const char *in, const char *delim, char ***out)
+{
+    int n = 0, i = 0;
+    char *tmp, *p;
+
+    tmp = g_strdup(in);
+    for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim)) {
+        if (strlen(p) > 0) {
+            ++n;
+        }
+    }
+    g_free(tmp);
+
+    *out = g_malloc0(n * sizeof(char *) + 1); /* last element NULL delimiter */
+
+    tmp = g_strdup(in);
+    for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim)) {
+        if (strlen(p) > 0) {
+            (*out)[i++] = g_strdup(p);
+        }
+    }
+    g_free(tmp);
+
+    return n;
+}
+
+static void split_free(char ***out)
+{
+    int i;
+    for (i = 0; (*out)[i]; ++i) {
+        g_free((*out)[i]);
+    }
+    g_free(*out);
+    *out = NULL;
+}
+
 static void pci_config(void *obj, void *data, QGuestAllocator *t_alloc)
 {
     QVirtio9P *v9p = obj;
@@ -201,6 +257,7 @@ static const char *rmessage_name(uint8_t id)
         id == P9_RWALK ? "RWALK" :
         id == P9_RLOPEN ? "RLOPEN" :
         id == P9_RWRITE ? "RWRITE" :
+        id == P9_RMKDIR ? "RMKDIR" :
         id == P9_RFLUSH ? "RFLUSH" :
         id == P9_RREADDIR ? "READDIR" :
         "<unknown>";
@@ -578,6 +635,39 @@ static bool fs_dirents_contain_name(struct V9fsDirent *e, const char* name)
     return false;
 }
 
+/* size[4] Tmkdir tag[2] dfid[4] name[s] mode[4] gid[4] */
+static P9Req *v9fs_tmkdir(QVirtio9P *v9p, uint32_t dfid, const char *name,
+                          uint32_t mode, uint32_t gid, uint16_t tag)
+{
+    P9Req *req;
+
+    uint32_t body_size = 4 + 4 + 4;
+    uint16_t string_size = v9fs_string_size(name);
+
+    g_assert_cmpint(body_size, <=, UINT32_MAX - string_size);
+    body_size += string_size;
+
+    req = v9fs_req_init(v9p, body_size, P9_TMKDIR, tag);
+    v9fs_uint32_write(req, dfid);
+    v9fs_string_write(req, name);
+    v9fs_uint32_write(req, mode);
+    v9fs_uint32_write(req, gid);
+    v9fs_req_send(req);
+    return req;
+}
+
+/* size[4] Rmkdir tag[2] qid[13] */
+static void v9fs_rmkdir(P9Req *req, v9fs_qid *qid)
+{
+    v9fs_req_recv(req, P9_RMKDIR);
+    if (qid) {
+        v9fs_memread(req, qid, 13);
+    } else {
+        v9fs_memskip(req, 13);
+    }
+    v9fs_req_free(req);
+}
+
 /* basic readdir test where reply fits into a single response message */
 static void fs_readdir(void *obj, void *data, QGuestAllocator *t_alloc)
 {
@@ -877,6 +967,30 @@ static void fs_flush_ignored(void *obj, void *data, QGuestAllocator *t_alloc)
     g_free(wnames[0]);
 }
 
+static void fs_mkdir(void *obj, void *data, QGuestAllocator *t_alloc,
+                     const char *path, const char *cname)
+{
+    QVirtio9P *v9p = obj;
+    alloc = t_alloc;
+    char **wnames;
+    char *const name = g_strdup(cname);
+    P9Req *req;
+    const uint32_t fid = genfid();
+
+    int nwnames = split(path, "/", &wnames);
+
+    req = v9fs_twalk(v9p, 0, fid, nwnames, wnames, 0);
+    v9fs_req_wait_for_reply(req, NULL);
+    v9fs_rwalk(req, NULL, NULL);
+
+    req = v9fs_tmkdir(v9p, fid, name, 0750, 0, 0);
+    v9fs_req_wait_for_reply(req, NULL);
+    v9fs_rmkdir(req, NULL);
+
+    g_free(name);
+    split_free(&wnames);
+}
+
 static void fs_readdir_split_128(void *obj, void *data,
                                  QGuestAllocator *t_alloc)
 {
@@ -895,6 +1009,30 @@ static void fs_readdir_split_512(void *obj, void *data,
     fs_readdir_split(obj, data, t_alloc, 512);
 }
 
+
+/* tests using the 9pfs 'local' fs driver */
+
+static void fs_create_dir(void *obj, void *data, QGuestAllocator *t_alloc)
+{
+    QVirtio9P *v9p = obj;
+    struct stat st;
+    char *root_path = virtio_9p_test_path("");
+    char *new_dir = virtio_9p_test_path("01");
+
+    g_assert(root_path != NULL);
+
+    fs_attach(v9p, NULL, t_alloc);
+    fs_mkdir(v9p, data, t_alloc, "/", "01");
+
+    /* check if created directory really exists now ... */
+    g_assert(stat(new_dir, &st) == 0);
+    /* ... and is actually a directory */
+    g_assert((st.st_mode & S_IFMT) == S_IFDIR);
+
+    g_free(new_dir);
+    g_free(root_path);
+}
+
 static void *assign_9p_local_driver(GString *cmd_line, void *arg)
 {
     virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -934,6 +1072,7 @@ static void register_virtio_9p_test(void)
     /* 9pfs test cases using the 'local' filesystem driver */
     opts.before = assign_9p_local_driver;
     qos_add_test("local/config", "virtio-9p", pci_config,  &opts);
+    qos_add_test("local/create_dir", "virtio-9p", fs_create_dir, &opts);
 }
 
 libqos_init(register_virtio_9p_test);
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 08/11] tests/9pfs: introduce local tests
  2020-10-02 11:52 [PATCH v2 00/11] 9pfs: add tests using local fs driver Christian Schoenebeck
                   ` (6 preceding siblings ...)
  2020-10-02 11:51 ` [PATCH v2 05/11] tests/qtest/qos-test: dump environment variables if verbose Christian Schoenebeck
@ 2020-10-02 11:51 ` Christian Schoenebeck
  2020-10-02 14:26   ` Christian Schoenebeck
  2020-10-02 11:51 ` [PATCH v2 07/11] tests/9pfs: change qtest name prefix to synth Christian Schoenebeck
                   ` (2 subsequent siblings)
  10 siblings, 1 reply; 22+ messages in thread
From: Christian Schoenebeck @ 2020-10-02 11:51 UTC (permalink / raw)
  To: qemu-devel
  Cc: Thomas Huth, Laurent Vivier, Paolo Bonzini,
	Emanuele Giuseppe Esposito, Greg Kurz

This patch introduces 9pfs test cases using the 9pfs 'local'
filesystem driver which reads/writes/creates/deletes real files
and directories.

In this initial version, there is only one local test which actually
only checks if the 9pfs 'local' device was created successfully.

Before the 9pfs 'local' tests are run, a test directory 'qtest-9p-local'
is created (with world rwx permissions) under the current working
directory. At this point that test directory is not auto deleted yet.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/libqos/virtio-9p.c | 100 +++++++++++++++++++++++++++++++++
 tests/qtest/libqos/virtio-9p.h |   5 ++
 tests/qtest/virtio-9p-test.c   |  44 ++++++++++-----
 3 files changed, 135 insertions(+), 14 deletions(-)

diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index 2e300063e3..86e40e5d56 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -24,6 +24,63 @@
 #include "qgraph.h"
 
 static QGuestAllocator *alloc;
+static char *local_test_path;
+
+static char *strpr(const char* format, ...) GCC_FMT_ATTR(1, 2);
+
+/* Concatenates the passed 2 pathes. Returned result must be freed. */
+static char *concat_path(const char* a, const char* b)
+{
+    const int len = strlen(a) + strlen("/") + strlen(b);
+    char *path = g_malloc0(len + 1);
+    snprintf(path, len + 1, "%s/%s", a, b);
+    g_assert(strlen(path) == len);
+    return path;
+}
+
+/*
+ * Lazy sprintf() implementation which auto allocates buffer. Returned result
+ * must be freed.
+ */
+static char *strpr(const char* format, ...)
+{
+    va_list argp;
+
+    va_start(argp, format);
+    const int sz = vsnprintf(NULL, 0, format, argp) + 1;
+    va_end(argp);
+
+    g_assert(sz > 0);
+    char *s = g_malloc0(sz);
+
+    va_start(argp, format);
+    const int len = vsnprintf(s, sz, format, argp);
+    va_end(argp);
+
+    g_assert(len + 1 == sz);
+    return s;
+}
+
+static void init_local_test_path(void)
+{
+    char *pwd = get_current_dir_name();
+    local_test_path = concat_path(pwd, "qtest-9p-local");
+    free(pwd);
+}
+
+/* Creates the directory for the 9pfs 'local' filesystem driver to access. */
+static void create_local_test_dir(void)
+{
+    struct stat st;
+
+    g_assert(local_test_path != NULL);
+    mkdir(local_test_path, 0777);
+
+    /* ensure test directory exists now ... */
+    g_assert(stat(local_test_path, &st) == 0);
+    /* ... and is actually a directory */
+    g_assert((st.st_mode & S_IFMT) == S_IFDIR);
+}
 
 static void virtio_9p_cleanup(QVirtio9P *interface)
 {
@@ -146,11 +203,54 @@ static void *virtio_9p_pci_create(void *pci_bus, QGuestAllocator *t_alloc,
     return obj;
 }
 
+void virtio_9p_assign_local_driver(GString *cmd_line, const char *args)
+{
+    GRegex *regex;
+    char *s, *arg_repl;
+
+    g_assert_nonnull(local_test_path);
+
+    /* replace 'synth' driver by 'local' driver */
+    regex = g_regex_new("-fsdev synth,", 0, 0, NULL);
+    s = g_regex_replace_literal(
+        regex, cmd_line->str, -1, 0, "-fsdev local,", 0, NULL
+    );
+    g_string_assign(cmd_line, s);
+    g_free(s);
+    g_regex_unref(regex);
+
+    /* add 'path=...' to '-fsdev ...' group */
+    regex = g_regex_new("(-fsdev \\w+)(\\s*)", 0, 0, NULL);
+    arg_repl = strpr("\\1\\2,path='%s'", local_test_path);
+    s = g_regex_replace(
+        regex, cmd_line->str, -1, 0, arg_repl, 0, NULL
+    );
+    g_string_assign(cmd_line, s);
+    g_free(arg_repl);
+    g_free(s);
+    g_regex_unref(regex);
+
+    /* add passed args to '-fsdev ...' group */
+    regex = g_regex_new("(-fsdev \\w+)(\\s*)", 0, 0, NULL);
+    arg_repl = strpr("\\1\\2,%s", args);
+    s = g_regex_replace(
+        regex, cmd_line->str, -1, 0, arg_repl, 0, NULL
+    );
+    g_string_assign(cmd_line, s);
+    g_free(arg_repl);
+    g_free(s);
+    g_regex_unref(regex);
+}
+
 static void virtio_9p_register_nodes(void)
 {
     const char *str_simple = "fsdev=fsdev0,mount_tag=" MOUNT_TAG;
     const char *str_addr = "fsdev=fsdev0,addr=04.0,mount_tag=" MOUNT_TAG;
 
+    /* make sure test dir for the 'local' tests exists and is clean */
+    init_local_test_path();
+    create_local_test_dir();
+
     QPCIAddress addr = {
         .devfn = QPCI_DEVFN(4, 0),
     };
diff --git a/tests/qtest/libqos/virtio-9p.h b/tests/qtest/libqos/virtio-9p.h
index b1e6badc4a..326a603f72 100644
--- a/tests/qtest/libqos/virtio-9p.h
+++ b/tests/qtest/libqos/virtio-9p.h
@@ -44,4 +44,9 @@ struct QVirtio9PDevice {
     QVirtio9P v9p;
 };
 
+/**
+ * Prepares QEMU command line for 9pfs tests using the 'local' fs driver.
+ */
+void virtio_9p_assign_local_driver(GString *cmd_line, const char *args);
+
 #endif
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 3281153b9c..af7e169d3a 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -895,29 +895,45 @@ static void fs_readdir_split_512(void *obj, void *data,
     fs_readdir_split(obj, data, t_alloc, 512);
 }
 
+static void *assign_9p_local_driver(GString *cmd_line, void *arg)
+{
+    virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
+    return arg;
+}
+
 static void register_virtio_9p_test(void)
 {
-    qos_add_test("synth/config", "virtio-9p", pci_config, NULL);
-    qos_add_test("synth/version/basic", "virtio-9p", fs_version, NULL);
-    qos_add_test("synth/attach/basic", "virtio-9p", fs_attach, NULL);
-    qos_add_test("synth/walk/basic", "virtio-9p", fs_walk, NULL);
+
+    QOSGraphTestOptions opts = {
+    };
+
+    /* 9pfs test cases using the 'synth' filesystem driver */
+    qos_add_test("synth/config", "virtio-9p", pci_config, &opts);
+    qos_add_test("synth/version/basic", "virtio-9p", fs_version,  &opts);
+    qos_add_test("synth/attach/basic", "virtio-9p", fs_attach,  &opts);
+    qos_add_test("synth/walk/basic", "virtio-9p", fs_walk,  &opts);
     qos_add_test("synth/walk/no_slash", "virtio-9p", fs_walk_no_slash,
-                 NULL);
+                  &opts);
     qos_add_test("synth/walk/dotdot_from_root", "virtio-9p",
-                 fs_walk_dotdot, NULL);
-    qos_add_test("synth/lopen/basic", "virtio-9p", fs_lopen, NULL);
-    qos_add_test("synth/write/basic", "virtio-9p", fs_write, NULL);
+                 fs_walk_dotdot,  &opts);
+    qos_add_test("synth/lopen/basic", "virtio-9p", fs_lopen,  &opts);
+    qos_add_test("synth/write/basic", "virtio-9p", fs_write,  &opts);
     qos_add_test("synth/flush/success", "virtio-9p", fs_flush_success,
-                 NULL);
+                  &opts);
     qos_add_test("synth/flush/ignored", "virtio-9p", fs_flush_ignored,
-                 NULL);
-    qos_add_test("synth/readdir/basic", "virtio-9p", fs_readdir, NULL);
+                  &opts);
+    qos_add_test("synth/readdir/basic", "virtio-9p", fs_readdir,  &opts);
     qos_add_test("synth/readdir/split_512", "virtio-9p",
-                 fs_readdir_split_512, NULL);
+                 fs_readdir_split_512,  &opts);
     qos_add_test("synth/readdir/split_256", "virtio-9p",
-                 fs_readdir_split_256, NULL);
+                 fs_readdir_split_256,  &opts);
     qos_add_test("synth/readdir/split_128", "virtio-9p",
-                 fs_readdir_split_128, NULL);
+                 fs_readdir_split_128,  &opts);
+
+
+    /* 9pfs test cases using the 'local' filesystem driver */
+    opts.before = assign_9p_local_driver;
+    qos_add_test("local/config", "virtio-9p", pci_config,  &opts);
 }
 
 libqos_init(register_virtio_9p_test);
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 07/11] tests/9pfs: change qtest name prefix to synth
  2020-10-02 11:52 [PATCH v2 00/11] 9pfs: add tests using local fs driver Christian Schoenebeck
                   ` (7 preceding siblings ...)
  2020-10-02 11:51 ` [PATCH v2 08/11] tests/9pfs: introduce local tests Christian Schoenebeck
@ 2020-10-02 11:51 ` Christian Schoenebeck
  2020-10-02 11:51 ` [PATCH v2 01/11] libqos/qgraph: add qemu_name to QOSGraphNode Christian Schoenebeck
  2020-10-02 11:51 ` [PATCH v2 11/11] tests/9pfs: add local Tmkdir test Christian Schoenebeck
  10 siblings, 0 replies; 22+ messages in thread
From: Christian Schoenebeck @ 2020-10-02 11:51 UTC (permalink / raw)
  To: qemu-devel
  Cc: Thomas Huth, Laurent Vivier, Paolo Bonzini,
	Emanuele Giuseppe Esposito, Greg Kurz

All existing 9pfs test cases are using the 'synth' fs driver so far, which
means they are not accessing real files, but a purely simulated (in RAM
only) file system.

Let's make this clear by changing the prefix of the individual qtest case
names from 'fs/' to 'synth/'. That way they'll be easily distinguishable
from upcoming new 9pfs test cases supposed to be using a different fs
driver.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/virtio-9p-test.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index de30b717b6..3281153b9c 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -897,26 +897,26 @@ static void fs_readdir_split_512(void *obj, void *data,
 
 static void register_virtio_9p_test(void)
 {
-    qos_add_test("config", "virtio-9p", pci_config, NULL);
-    qos_add_test("fs/version/basic", "virtio-9p", fs_version, NULL);
-    qos_add_test("fs/attach/basic", "virtio-9p", fs_attach, NULL);
-    qos_add_test("fs/walk/basic", "virtio-9p", fs_walk, NULL);
-    qos_add_test("fs/walk/no_slash", "virtio-9p", fs_walk_no_slash,
+    qos_add_test("synth/config", "virtio-9p", pci_config, NULL);
+    qos_add_test("synth/version/basic", "virtio-9p", fs_version, NULL);
+    qos_add_test("synth/attach/basic", "virtio-9p", fs_attach, NULL);
+    qos_add_test("synth/walk/basic", "virtio-9p", fs_walk, NULL);
+    qos_add_test("synth/walk/no_slash", "virtio-9p", fs_walk_no_slash,
                  NULL);
-    qos_add_test("fs/walk/dotdot_from_root", "virtio-9p",
+    qos_add_test("synth/walk/dotdot_from_root", "virtio-9p",
                  fs_walk_dotdot, NULL);
-    qos_add_test("fs/lopen/basic", "virtio-9p", fs_lopen, NULL);
-    qos_add_test("fs/write/basic", "virtio-9p", fs_write, NULL);
-    qos_add_test("fs/flush/success", "virtio-9p", fs_flush_success,
+    qos_add_test("synth/lopen/basic", "virtio-9p", fs_lopen, NULL);
+    qos_add_test("synth/write/basic", "virtio-9p", fs_write, NULL);
+    qos_add_test("synth/flush/success", "virtio-9p", fs_flush_success,
                  NULL);
-    qos_add_test("fs/flush/ignored", "virtio-9p", fs_flush_ignored,
+    qos_add_test("synth/flush/ignored", "virtio-9p", fs_flush_ignored,
                  NULL);
-    qos_add_test("fs/readdir/basic", "virtio-9p", fs_readdir, NULL);
-    qos_add_test("fs/readdir/split_512", "virtio-9p",
+    qos_add_test("synth/readdir/basic", "virtio-9p", fs_readdir, NULL);
+    qos_add_test("synth/readdir/split_512", "virtio-9p",
                  fs_readdir_split_512, NULL);
-    qos_add_test("fs/readdir/split_256", "virtio-9p",
+    qos_add_test("synth/readdir/split_256", "virtio-9p",
                  fs_readdir_split_256, NULL);
-    qos_add_test("fs/readdir/split_128", "virtio-9p",
+    qos_add_test("synth/readdir/split_128", "virtio-9p",
                  fs_readdir_split_128, NULL);
 }
 
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 04/11] tests/qtest/qos-test: dump qos graph if verbose
  2020-10-02 11:52 [PATCH v2 00/11] 9pfs: add tests using local fs driver Christian Schoenebeck
  2020-10-02 11:51 ` [PATCH v2 09/11] tests/9pfs: wipe local 9pfs test directory Christian Schoenebeck
  2020-10-02 11:51 ` [PATCH v2 02/11] libqos/qgraph: add qos_node_create_driver_named() Christian Schoenebeck
@ 2020-10-02 11:51 ` Christian Schoenebeck
  2020-10-02 11:51 ` [PATCH v2 06/11] tests/qtest/qos-test: dump QEMU command " Christian Schoenebeck
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 22+ messages in thread
From: Christian Schoenebeck @ 2020-10-02 11:51 UTC (permalink / raw)
  To: qemu-devel
  Cc: Thomas Huth, Laurent Vivier, Paolo Bonzini,
	Emanuele Giuseppe Esposito, Greg Kurz

If qtests were run in verbose mode (i.e. if --verbose CL argument was
provided) then dump the generated qos graph (all nodes and edges,
along with their current individual availability status) to stdout.

See API doc comment on function qos_dump_graph() for details.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/qos-test.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/qtest/qos-test.c b/tests/qtest/qos-test.c
index 8fdf87b183..d98ef78613 100644
--- a/tests/qtest/qos-test.c
+++ b/tests/qtest/qos-test.c
@@ -322,6 +322,9 @@ int main(int argc, char **argv)
     qos_set_machines_devices_available();
 
     qos_graph_foreach_test_path(walk_path);
+    if (g_test_verbose()) {
+        qos_dump_graph();
+    }
     g_test_run();
     qtest_end();
     qos_graph_destroy();
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 06/11] tests/qtest/qos-test: dump QEMU command if verbose
  2020-10-02 11:52 [PATCH v2 00/11] 9pfs: add tests using local fs driver Christian Schoenebeck
                   ` (2 preceding siblings ...)
  2020-10-02 11:51 ` [PATCH v2 04/11] tests/qtest/qos-test: dump qos graph if verbose Christian Schoenebeck
@ 2020-10-02 11:51 ` Christian Schoenebeck
  2020-10-02 11:51 ` [PATCH v2 10/11] tests/9pfs: add virtio_9p_test_path() Christian Schoenebeck
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 22+ messages in thread
From: Christian Schoenebeck @ 2020-10-02 11:51 UTC (permalink / raw)
  To: qemu-devel
  Cc: Thomas Huth, Laurent Vivier, Paolo Bonzini,
	Emanuele Giuseppe Esposito, Greg Kurz

If qtests are run in verbose mode (i.e. if --verbose CL argument
was provided) then print the assembled qemu command line for each
test.

Instead of using g_test_message() rather use printf() in combination
with g_test_verbose(), to avoid g_test_message() cluttering the
output.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/qos-test.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/qtest/qos-test.c b/tests/qtest/qos-test.c
index fe240b32a7..b9f0942386 100644
--- a/tests/qtest/qos-test.c
+++ b/tests/qtest/qos-test.c
@@ -89,6 +89,9 @@ static void qos_set_machines_devices_available(void)
 
 static void restart_qemu_or_continue(char *path)
 {
+    if (g_test_verbose()) {
+        printf("Run QEMU with: '%s'\n", path);
+    }
     /* compares the current command line with the
      * one previously executed: if they are the same,
      * don't restart QEMU, if they differ, stop previous
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 02/11] libqos/qgraph: add qos_node_create_driver_named()
  2020-10-02 11:52 [PATCH v2 00/11] 9pfs: add tests using local fs driver Christian Schoenebeck
  2020-10-02 11:51 ` [PATCH v2 09/11] tests/9pfs: wipe local 9pfs test directory Christian Schoenebeck
@ 2020-10-02 11:51 ` Christian Schoenebeck
  2020-10-02 11:51 ` [PATCH v2 04/11] tests/qtest/qos-test: dump qos graph if verbose Christian Schoenebeck
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 22+ messages in thread
From: Christian Schoenebeck @ 2020-10-02 11:51 UTC (permalink / raw)
  To: qemu-devel
  Cc: Thomas Huth, Laurent Vivier, Paolo Bonzini,
	Emanuele Giuseppe Esposito, Greg Kurz

So far the qos subsystem of the qtest framework had the limitation
that only one instance of the same official QEMU (QMP) driver name
could be created for qtests. That's because a) the created qos
node names must always be unique, b) the node name must match the
official QEMU driver name being instantiated and c) all nodes are
in a global space shared by all tests.

This patch removes this limitation by introducing a new function
qos_node_create_driver_named() which allows test case authors to
specify a node name being different from the actual associated
QEMU driver name. It fills the new 'qemu_name' field of
QOSGraphNode for that purpose.

Adjust build_driver_cmd_line() and qos_graph_node_set_availability()
to correctly deal with either accessing node name vs. node's
qemu_name correctly.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/libqos/qgraph.c | 53 ++++++++++++++++++++++++++++++++++---
 tests/qtest/libqos/qgraph.h | 16 +++++++++++
 2 files changed, 66 insertions(+), 3 deletions(-)

diff --git a/tests/qtest/libqos/qgraph.c b/tests/qtest/libqos/qgraph.c
index e42f3eaafa..61faf6b27d 100644
--- a/tests/qtest/libqos/qgraph.c
+++ b/tests/qtest/libqos/qgraph.c
@@ -287,7 +287,8 @@ static void build_machine_cmd_line(QOSGraphNode *node, const char *args)
  */
 static void build_driver_cmd_line(QOSGraphNode *node)
 {
-    node->command_line = g_strconcat(" -device ", node->name, NULL);
+    const char *name = node->qemu_name ?: node->name;
+    node->command_line = g_strconcat(" -device ", name, NULL);
 }
 
 /* qos_print_cb(): callback prints all path found by the DFS algorithm. */
@@ -632,6 +633,15 @@ void qos_node_create_driver(const char *name, QOSCreateDriverFunc function)
     node->u.driver.constructor = function;
 }
 
+void qos_node_create_driver_named(const char *name, const char *qemu_name,
+                                  QOSCreateDriverFunc function)
+{
+    QOSGraphNode *node = create_node(name, QNODE_DRIVER);
+    node->qemu_name = g_strdup(qemu_name);
+    build_driver_cmd_line(node);
+    node->u.driver.constructor = function;
+}
+
 void qos_node_contains(const char *container, const char *contained,
                        QOSGraphEdgeOptions *opts, ...)
 {
@@ -664,7 +674,7 @@ void qos_node_consumes(const char *consumer, const char *interface,
     add_edge(interface, consumer, QEDGE_CONSUMED_BY, opts);
 }
 
-void qos_graph_node_set_availability(const char *node, bool av)
+static void qos_graph_node_set_availability_explicit(const char *node, bool av)
 {
     QOSGraphEdgeList *elist;
     QOSGraphNode *n = search_node(node);
@@ -679,9 +689,46 @@ void qos_graph_node_set_availability(const char *node, bool av)
     }
     QSLIST_FOREACH_SAFE(e, elist, edge_list, next) {
         if (e->type == QEDGE_CONTAINS || e->type == QEDGE_PRODUCES) {
-            qos_graph_node_set_availability(e->dest, av);
+            qos_graph_node_set_availability_explicit(e->dest, av);
+        }
+    }
+}
+
+/*
+ * Behaves as qos_graph_node_set_availability_explicit(), except that the
+ * former always matches by node name only, whereas this function matches both
+ * by node name and node's optional 'qemu_name' field.
+ */
+void qos_graph_node_set_availability(const char *node, bool av)
+{
+    GList *l;
+    QOSGraphEdgeList *elist;
+    QOSGraphEdge *e, *next;
+    QOSGraphNode *n;
+    GList *keys = g_hash_table_get_keys(node_table);
+
+    for (l = keys; l != NULL; l = l->next) {
+        const gchar *key = l->data;
+        n = g_hash_table_lookup(node_table, key);
+        /*
+         * node's 'qemu_name' is set if there is more than one device with
+         * the same QEMU (QMP) device name
+         */
+        const char *node_name = n->qemu_name ?: n->name;
+        if (g_strcmp0(node_name, node) == 0) {
+            n->available = av;
+            elist = get_edgelist(n->name);
+            if (elist) {
+                QSLIST_FOREACH_SAFE(e, elist, edge_list, next) {
+                    if (e->type == QEDGE_CONTAINS || e->type == QEDGE_PRODUCES)
+                    {
+                        qos_graph_node_set_availability_explicit(e->dest, av);
+                    }
+                }
+            }
         }
     }
+    g_list_free(keys);
 }
 
 void qos_graph_foreach_test_path(QOSTestCallback fn)
diff --git a/tests/qtest/libqos/qgraph.h b/tests/qtest/libqos/qgraph.h
index 5f63d352ca..f472949f68 100644
--- a/tests/qtest/libqos/qgraph.h
+++ b/tests/qtest/libqos/qgraph.h
@@ -452,6 +452,22 @@ void qos_node_create_machine_args(const char *name,
  */
 void qos_node_create_driver(const char *name, QOSCreateDriverFunc function);
 
+/**
+ * Behaves as qos_node_create_driver() with the extension of allowing to
+ * specify a different node name vs. associated QEMU device name.
+ *
+ * Use this function instead of qos_node_create_driver() if you need to create
+ * several instances of the same QEMU device. You are free to choose a custom
+ * node name, however the chosen node name must always be unique.
+ *
+ * @param name: custom, unique name of the node to be created
+ * @param qemu_name: actual (official) QEMU driver name the node shall be
+ *                   associated with
+ * @param function: driver constructor
+ */
+void qos_node_create_driver_named(const char *name, const char *qemu_name,
+                                  QOSCreateDriverFunc function);
+
 /**
  * qos_node_contains(): creates one or more edges of type QEDGE_CONTAINS
  * and adds them to the edge list mapped to @container in the
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 03/11] libqos/qgraph: add qos_dump_graph()
  2020-10-02 11:52 [PATCH v2 00/11] 9pfs: add tests using local fs driver Christian Schoenebeck
                   ` (4 preceding siblings ...)
  2020-10-02 11:51 ` [PATCH v2 10/11] tests/9pfs: add virtio_9p_test_path() Christian Schoenebeck
@ 2020-10-02 11:51 ` Christian Schoenebeck
  2020-10-02 11:51 ` [PATCH v2 05/11] tests/qtest/qos-test: dump environment variables if verbose Christian Schoenebeck
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 22+ messages in thread
From: Christian Schoenebeck @ 2020-10-02 11:51 UTC (permalink / raw)
  To: qemu-devel
  Cc: Thomas Huth, Laurent Vivier, Paolo Bonzini,
	Emanuele Giuseppe Esposito, Greg Kurz

This new function is purely for debugging purposes. It prints the
current qos graph to stdout and allows to identify problems in the
created qos graph e.g. when writing new qos tests.

Coloured output is used to mark available nodes in green colour,
whereas unavailable nodes are marked in red colour.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/libqos/qgraph.c | 54 +++++++++++++++++++++++++++++++++++++
 tests/qtest/libqos/qgraph.h | 20 ++++++++++++++
 2 files changed, 74 insertions(+)

diff --git a/tests/qtest/libqos/qgraph.c b/tests/qtest/libqos/qgraph.c
index 61faf6b27d..e70635750e 100644
--- a/tests/qtest/libqos/qgraph.c
+++ b/tests/qtest/libqos/qgraph.c
@@ -805,3 +805,57 @@ void qos_delete_cmd_line(const char *name)
         node->command_line = NULL;
     }
 }
+
+#define RED(txt) (    \
+    "\033[0;91m" txt  \
+    "\033[0m"         \
+)
+
+#define GREEN(txt) (    \
+    "\033[0;92m" txt  \
+    "\033[0m"         \
+)
+
+void qos_dump_graph(void)
+{
+    GList *keys;
+    GList *l;
+    QOSGraphEdgeList *list;
+    QOSGraphEdge *e, *next;
+    QOSGraphNode *dest_node, *node;
+
+    printf("ALL QGRAPH EDGES: {\n");
+    keys = g_hash_table_get_keys(edge_table);
+    for (l = keys; l != NULL; l = l->next) {
+        const gchar *key = l->data;
+        printf("\t src='%s'\n", key);
+        list = get_edgelist(key);
+        QSLIST_FOREACH_SAFE(e, list, edge_list, next) {
+            dest_node = g_hash_table_lookup(node_table, e->dest);
+            printf("\t\t|-> dest='%s' type=%d (node=%p)",
+                   e->dest, e->type, dest_node);
+            if (!dest_node) {
+                printf(RED(" <------- ERROR !"));
+            }
+            printf("\n");
+        }
+    }
+    g_list_free(keys);
+    printf("}\n");
+
+    printf("ALL QGRAPH NODES: {\n");
+    keys = g_hash_table_get_keys(node_table);
+    for (l = keys; l != NULL; l = l->next) {
+        const gchar *key = l->data;
+        node = g_hash_table_lookup(node_table, key);
+        printf("\t name='%s' ", key);
+        if (node->qemu_name) {
+            printf("qemu_name='%s' ", node->qemu_name);
+        }
+        printf("type=%d cmd_line='%s' [%s]\n",
+               node->type, node->command_line,
+               node->available ? GREEN("available") : RED("UNAVAILBLE"));
+    }
+    g_list_free(keys);
+    printf("}\n");
+}
diff --git a/tests/qtest/libqos/qgraph.h b/tests/qtest/libqos/qgraph.h
index f472949f68..07a32535f1 100644
--- a/tests/qtest/libqos/qgraph.h
+++ b/tests/qtest/libqos/qgraph.h
@@ -586,5 +586,25 @@ QOSGraphObject *qos_machine_new(QOSGraphNode *node, QTestState *qts);
 QOSGraphObject *qos_driver_new(QOSGraphNode *node, QOSGraphObject *parent,
                                QGuestAllocator *alloc, void *arg);
 
+/**
+ * Just for debugging purpose: prints all currently existing nodes and
+ * edges to stdout.
+ *
+ * All qtests add themselves to the overall qos graph by calling qgraph
+ * functions that add device nodes and edges between the individual graph
+ * nodes for tests. As the actual graph is assmbled at runtime by the qos
+ * subsystem, it is sometimes not obvious how the overall graph looks like.
+ * E.g. when writing new tests it may happen that those new tests are simply
+ * ignored by the qtest framework.
+ *
+ * This function allows to identify problems in the created qgraph. Keep in
+ * mind: only tests with a path down from the actual test case node (leaf) up
+ * to the graph's root node are actually executed by the qtest framework. And
+ * the qtest framework uses QMP to automatically check which QEMU drivers are
+ * actually currently available, and accordingly qos marks certain pathes as
+ * 'unavailable' in such cases (e.g. when QEMU was compiled without support for
+ * a certain feature).
+ */
+void qos_dump_graph(void);
 
 #endif
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 01/11] libqos/qgraph: add qemu_name to QOSGraphNode
  2020-10-02 11:52 [PATCH v2 00/11] 9pfs: add tests using local fs driver Christian Schoenebeck
                   ` (8 preceding siblings ...)
  2020-10-02 11:51 ` [PATCH v2 07/11] tests/9pfs: change qtest name prefix to synth Christian Schoenebeck
@ 2020-10-02 11:51 ` Christian Schoenebeck
  2020-10-02 11:51 ` [PATCH v2 11/11] tests/9pfs: add local Tmkdir test Christian Schoenebeck
  10 siblings, 0 replies; 22+ messages in thread
From: Christian Schoenebeck @ 2020-10-02 11:51 UTC (permalink / raw)
  To: qemu-devel
  Cc: Thomas Huth, Laurent Vivier, Paolo Bonzini,
	Emanuele Giuseppe Esposito, Greg Kurz

Add new member variable 'qemu_name' to struct QOSGraphNode.

This new member may be optionally set in case a different
name for the node (which must always be a unique name) vs.
its actually associated QEMU (QMP) device name is required.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/libqos/qgraph.c          | 1 +
 tests/qtest/libqos/qgraph_internal.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/tests/qtest/libqos/qgraph.c b/tests/qtest/libqos/qgraph.c
index fc49cfa879..e42f3eaafa 100644
--- a/tests/qtest/libqos/qgraph.c
+++ b/tests/qtest/libqos/qgraph.c
@@ -153,6 +153,7 @@ static QOSGraphNode *create_node(const char *name, QOSNodeType type)
 static void destroy_node(void *val)
 {
     QOSGraphNode *node = val;
+    g_free(node->qemu_name);
     g_free(node->command_line);
     g_free(node);
 }
diff --git a/tests/qtest/libqos/qgraph_internal.h b/tests/qtest/libqos/qgraph_internal.h
index 968fa69450..974985dce9 100644
--- a/tests/qtest/libqos/qgraph_internal.h
+++ b/tests/qtest/libqos/qgraph_internal.h
@@ -56,6 +56,7 @@ struct QOSGraphNode {
     bool available;     /* set by QEMU via QMP, used during graph walk */
     bool visited;       /* used during graph walk */
     char *name;         /* used to identify the node */
+    char *qemu_name;    /* optional: see qos_node_create_driver_named() */
     char *command_line; /* used to start QEMU at test execution */
     union {
         struct {
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 09/11] tests/9pfs: wipe local 9pfs test directory
  2020-10-02 11:52 [PATCH v2 00/11] 9pfs: add tests using local fs driver Christian Schoenebeck
@ 2020-10-02 11:51 ` Christian Schoenebeck
  2020-10-02 11:51 ` [PATCH v2 02/11] libqos/qgraph: add qos_node_create_driver_named() Christian Schoenebeck
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 22+ messages in thread
From: Christian Schoenebeck @ 2020-10-02 11:51 UTC (permalink / raw)
  To: qemu-devel
  Cc: Thomas Huth, Laurent Vivier, Paolo Bonzini,
	Emanuele Giuseppe Esposito, Greg Kurz

Before running the first 9pfs test case, make sure the test directory
for running the 9pfs 'local' tests on is entirely empty. For that
reason simply delete the test directory (if any) before (re)creating
it on test suite startup.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/libqos/virtio-9p.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index 86e40e5d56..6cd8c8964b 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -82,6 +82,18 @@ static void create_local_test_dir(void)
     g_assert((st.st_mode & S_IFMT) == S_IFDIR);
 }
 
+/* Deletes directory previously created by create_local_test_dir(). */
+static void remove_local_test_dir(void)
+{
+    g_assert(local_test_path != NULL);
+    char *cmd = strpr("rm -r '%s'\n", local_test_path);
+    int res = system(cmd);
+    if (res < 0) {
+        /* ignore error, dummy check to prevent compiler error */
+    }
+    g_free(cmd);
+}
+
 static void virtio_9p_cleanup(QVirtio9P *interface)
 {
     qvirtqueue_cleanup(interface->vdev->bus, interface->vq, alloc);
@@ -249,6 +261,7 @@ static void virtio_9p_register_nodes(void)
 
     /* make sure test dir for the 'local' tests exists and is clean */
     init_local_test_path();
+    remove_local_test_dir();
     create_local_test_dir();
 
     QPCIAddress addr = {
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 05/11] tests/qtest/qos-test: dump environment variables if verbose
  2020-10-02 11:52 [PATCH v2 00/11] 9pfs: add tests using local fs driver Christian Schoenebeck
                   ` (5 preceding siblings ...)
  2020-10-02 11:51 ` [PATCH v2 03/11] libqos/qgraph: add qos_dump_graph() Christian Schoenebeck
@ 2020-10-02 11:51 ` Christian Schoenebeck
  2020-10-02 11:51 ` [PATCH v2 08/11] tests/9pfs: introduce local tests Christian Schoenebeck
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 22+ messages in thread
From: Christian Schoenebeck @ 2020-10-02 11:51 UTC (permalink / raw)
  To: qemu-devel
  Cc: Thomas Huth, Laurent Vivier, Paolo Bonzini,
	Emanuele Giuseppe Esposito, Greg Kurz

If qtests are run in verbose mode (i.e. if --verbose CL argument
was provided) then print all environment variables to stdout
before running the individual tests.

Instead of using g_test_message() rather use printf() in combination
with g_test_verbose(), to avoid g_test_message() cluttering the
output.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/qos-test.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/qos-test.c b/tests/qtest/qos-test.c
index d98ef78613..fe240b32a7 100644
--- a/tests/qtest/qos-test.c
+++ b/tests/qtest/qos-test.c
@@ -313,9 +313,16 @@ static void walk_path(QOSGraphNode *orig_path, int len)
  *   machine/drivers/test objects
  * - Cleans up everything
  */
-int main(int argc, char **argv)
+int main(int argc, char **argv, char** envp)
 {
     g_test_init(&argc, &argv, NULL);
+    if (g_test_verbose()) {
+        printf("ENVIRONMENT VARIABLES: {\n");
+        for (char **env = envp; *env != 0; env++) {
+            printf("\t%s\n", *env);
+        }
+        printf("}\n");
+    }
     qos_graph_init();
     module_call_init(MODULE_INIT_QOM);
     module_call_init(MODULE_INIT_LIBQOS);
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 10/11] tests/9pfs: add virtio_9p_test_path()
  2020-10-02 11:52 [PATCH v2 00/11] 9pfs: add tests using local fs driver Christian Schoenebeck
                   ` (3 preceding siblings ...)
  2020-10-02 11:51 ` [PATCH v2 06/11] tests/qtest/qos-test: dump QEMU command " Christian Schoenebeck
@ 2020-10-02 11:51 ` Christian Schoenebeck
  2020-10-02 11:51 ` [PATCH v2 03/11] libqos/qgraph: add qos_dump_graph() Christian Schoenebeck
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 22+ messages in thread
From: Christian Schoenebeck @ 2020-10-02 11:51 UTC (permalink / raw)
  To: qemu-devel
  Cc: Thomas Huth, Laurent Vivier, Paolo Bonzini,
	Emanuele Giuseppe Esposito, Greg Kurz

This new public function virtio_9p_test_path() allows 9pfs
'local' tests to translate a path from guest scope to host
scope. For instance by passing an empty string it would
return the root path on host of the exported 9pfs tree.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/libqos/virtio-9p.c | 6 ++++++
 tests/qtest/libqos/virtio-9p.h | 5 +++++
 2 files changed, 11 insertions(+)

diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index 6cd8c8964b..ccab889a2b 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -94,6 +94,12 @@ static void remove_local_test_dir(void)
     g_free(cmd);
 }
 
+char *virtio_9p_test_path(const char *path)
+{
+    g_assert(local_test_path);
+    return concat_path(local_test_path, path);
+}
+
 static void virtio_9p_cleanup(QVirtio9P *interface)
 {
     qvirtqueue_cleanup(interface->vdev->bus, interface->vq, alloc);
diff --git a/tests/qtest/libqos/virtio-9p.h b/tests/qtest/libqos/virtio-9p.h
index 326a603f72..19a4d97454 100644
--- a/tests/qtest/libqos/virtio-9p.h
+++ b/tests/qtest/libqos/virtio-9p.h
@@ -49,4 +49,9 @@ struct QVirtio9PDevice {
  */
 void virtio_9p_assign_local_driver(GString *cmd_line, const char *args);
 
+/**
+ * Returns path on host to the passed guest path. Result must be freed.
+ */
+char *virtio_9p_test_path(const char *path);
+
 #endif
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 00/11] 9pfs: add tests using local fs driver
@ 2020-10-02 11:52 Christian Schoenebeck
  2020-10-02 11:51 ` [PATCH v2 09/11] tests/9pfs: wipe local 9pfs test directory Christian Schoenebeck
                   ` (10 more replies)
  0 siblings, 11 replies; 22+ messages in thread
From: Christian Schoenebeck @ 2020-10-02 11:52 UTC (permalink / raw)
  To: qemu-devel
  Cc: Thomas Huth, Laurent Vivier, Paolo Bonzini,
	Emanuele Giuseppe Esposito, Greg Kurz

The currently existing 9pfs test cases are all solely using the 9pfs 'synth'
fileystem driver, which is a very simple and purely simulated (in RAM only)
filesystem. There are issues though where the 'synth' fs driver is not
sufficient. For example the following two bugs need test cases running the
9pfs 'local' fs driver:

https://bugs.launchpad.net/qemu/+bug/1336794
https://bugs.launchpad.net/qemu/+bug/1877384

This patch set for that reason introduces 9pfs test cases using the 9pfs
'local' filesystem driver along to the already existing tests on 'synth'.
It consists of 3 parts:

1. libqos patches 1 and 2 remove a limitation of the qtest/libqos subsystem:
   support for more than one device using the same (official) QEMU device
   name.

2. Patches 3 to 6 enhance debugging issues with the qtest framework.

3. Patches 7 to 11 actually introduce 9pfs 'local' test cases using the qtest
   framework. These 'local' tests are adding a test directory 'qtest-9p-local'
   inside the current working directory (using get_current_dir()), which is
   typically the build directory, before running the tests. That test
   directory is automatically recreated next time the test suite is run again,
   to ensure the 9pfs 'local' tests always run consistently on a clean test
   directory. The test directory is used by the 'local' driver as root of its
   export path. So it will add/read/write/delete real files and directories
   inside that test directory.

v1->v2:

  * The libqos debugging features are now turned on by command line argument
    '--verbose' instead of using environment variables (patches 4, 5, 6).

  * The new 9pfs 'local' tests no longer depend on patches 1 and 2 by no
    longer using a libqos multi-device approach, but rather modifying the
    final QEMU command line for each test instead; see discussion of v1
    for reason (patches 7 to 11).

  * Use GCC_FMT_ATTR on helper function strpr() (patch 8).

  * Updated commit log comments.

Christian Schoenebeck (11):
  libqos/qgraph: add qemu_name to QOSGraphNode
  libqos/qgraph: add qos_node_create_driver_named()
  libqos/qgraph: add qos_dump_graph()
  tests/qtest/qos-test: dump qos graph if verbose
  tests/qtest/qos-test: dump environment variables if verbose
  tests/qtest/qos-test: dump QEMU command if verbose
  tests/9pfs: change qtest name prefix to synth
  tests/9pfs: introduce local tests
  tests/9pfs: wipe local 9pfs test directory
  tests/9pfs: add virtio_9p_test_path()
  tests/9pfs: add local Tmkdir test

 tests/qtest/libqos/qgraph.c          | 108 ++++++++++++++-
 tests/qtest/libqos/qgraph.h          |  36 +++++
 tests/qtest/libqos/qgraph_internal.h |   1 +
 tests/qtest/libqos/virtio-9p.c       | 119 ++++++++++++++++
 tests/qtest/libqos/virtio-9p.h       |  10 ++
 tests/qtest/qos-test.c               |  15 +-
 tests/qtest/virtio-9p-test.c         | 197 ++++++++++++++++++++++++---
 7 files changed, 461 insertions(+), 25 deletions(-)

-- 
2.20.1



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 11/11] tests/9pfs: add local Tmkdir test
  2020-10-02 11:51 ` [PATCH v2 11/11] tests/9pfs: add local Tmkdir test Christian Schoenebeck
@ 2020-10-02 12:56   ` Daniel P. Berrangé
  2020-10-02 13:41     ` Christian Schoenebeck
  2020-10-02 14:32     ` Greg Kurz
  0 siblings, 2 replies; 22+ messages in thread
From: Daniel P. Berrangé @ 2020-10-02 12:56 UTC (permalink / raw)
  To: Christian Schoenebeck
  Cc: Laurent Vivier, Thomas Huth, Emanuele Giuseppe Esposito,
	qemu-devel, Greg Kurz, Paolo Bonzini

On Fri, Oct 02, 2020 at 01:51:54PM +0200, Christian Schoenebeck wrote:
> This test case uses the 9pfs 'local' driver to create a directory
> and then checks if the expected directory was actually created
> (as real directory) on host side.
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  tests/qtest/virtio-9p-test.c | 139 +++++++++++++++++++++++++++++++++++
>  1 file changed, 139 insertions(+)
> 
> diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> index af7e169d3a..93161a4b35 100644
> --- a/tests/qtest/virtio-9p-test.c
> +++ b/tests/qtest/virtio-9p-test.c
> @@ -18,6 +18,62 @@
>  #define QVIRTIO_9P_TIMEOUT_US (10 * 1000 * 1000)
>  static QGuestAllocator *alloc;
>  
> +/*
> + * Used to auto generate new fids. Start with arbitrary high value to avoid
> + * collision with hard coded fids in basic test code.
> + */
> +static uint32_t fid_generator = 1000;
> +
> +static uint32_t genfid(void)
> +{
> +    return fid_generator++;
> +}
> +
> +/**
> + * Splits the @a in string by @a delim into individual (non empty) strings
> + * and outputs them to @a out. The output array @a out is NULL terminated.
> + *
> + * Output array @a out must be freed by calling split_free().
> + *
> + * @returns number of individual elements in output array @a out (without the
> + *          final NULL terminating element)
> + */
> +static int split(const char *in, const char *delim, char ***out)
> +{
> +    int n = 0, i = 0;
> +    char *tmp, *p;
> +
> +    tmp = g_strdup(in);
> +    for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim)) {
> +        if (strlen(p) > 0) {
> +            ++n;
> +        }
> +    }
> +    g_free(tmp);
> +
> +    *out = g_malloc0(n * sizeof(char *) + 1); /* last element NULL delimiter */

Surely this should be  (n + 1) * sizeof(char *), because the last
element still needs to be large enough to hold a pointer, not a
single extra byte.

> +
> +    tmp = g_strdup(in);
> +    for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim)) {
> +        if (strlen(p) > 0) {
> +            (*out)[i++] = g_strdup(p);
> +        }
> +    }
> +    g_free(tmp);
> +
> +    return n;
> +}

This seems to largely re-invent g_strsplit 

https://developer.gnome.org/glib/2.62/glib-String-Utility-Functions.html#g-strsplit

> +
> +static void split_free(char ***out)
> +{
> +    int i;
> +    for (i = 0; (*out)[i]; ++i) {
> +        g_free((*out)[i]);
> +    }
> +    g_free(*out);
> +    *out = NULL;
> +}

And g_strfreev



Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 11/11] tests/9pfs: add local Tmkdir test
  2020-10-02 12:56   ` Daniel P. Berrangé
@ 2020-10-02 13:41     ` Christian Schoenebeck
  2020-10-02 13:44       ` Daniel P. Berrangé
  2020-10-02 14:32     ` Greg Kurz
  1 sibling, 1 reply; 22+ messages in thread
From: Christian Schoenebeck @ 2020-10-02 13:41 UTC (permalink / raw)
  To: qemu-devel, Daniel P. Berrangé
  Cc: Laurent Vivier, Thomas Huth, Emanuele Giuseppe Esposito,
	Greg Kurz, Paolo Bonzini

On Freitag, 2. Oktober 2020 14:56:14 CEST Daniel P. Berrangé wrote:
> On Fri, Oct 02, 2020 at 01:51:54PM +0200, Christian Schoenebeck wrote:
> > This test case uses the 9pfs 'local' driver to create a directory
> > and then checks if the expected directory was actually created
> > (as real directory) on host side.
> > 
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> > 
> >  tests/qtest/virtio-9p-test.c | 139 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 139 insertions(+)
> > 
> > diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> > index af7e169d3a..93161a4b35 100644
> > --- a/tests/qtest/virtio-9p-test.c
> > +++ b/tests/qtest/virtio-9p-test.c
> > @@ -18,6 +18,62 @@
> > 
> >  #define QVIRTIO_9P_TIMEOUT_US (10 * 1000 * 1000)
> >  static QGuestAllocator *alloc;
> > 
> > +/*
> > + * Used to auto generate new fids. Start with arbitrary high value to
> > avoid + * collision with hard coded fids in basic test code.
> > + */
> > +static uint32_t fid_generator = 1000;
> > +
> > +static uint32_t genfid(void)
> > +{
> > +    return fid_generator++;
> > +}
> > +
> > +/**
> > + * Splits the @a in string by @a delim into individual (non empty)
> > strings
> > + * and outputs them to @a out. The output array @a out is NULL
> > terminated.
> > + *
> > + * Output array @a out must be freed by calling split_free().
> > + *
> > + * @returns number of individual elements in output array @a out (without
> > the + *          final NULL terminating element)
> > + */
> > +static int split(const char *in, const char *delim, char ***out)
> > +{
> > +    int n = 0, i = 0;
> > +    char *tmp, *p;
> > +
> > +    tmp = g_strdup(in);
> > +    for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim)) {
> > +        if (strlen(p) > 0) {
> > +            ++n;
> > +        }
> > +    }
> > +    g_free(tmp);
> > +
> > +    *out = g_malloc0(n * sizeof(char *) + 1); /* last element NULL
> > delimiter */
> Surely this should be  (n + 1) * sizeof(char *), because the last
> element still needs to be large enough to hold a pointer, not a
> single extra byte.

Right, good catch!

> > +
> > +    tmp = g_strdup(in);
> > +    for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim)) {
> > +        if (strlen(p) > 0) {
> > +            (*out)[i++] = g_strdup(p);
> > +        }
> > +    }
> > +    g_free(tmp);
> > +
> > +    return n;
> > +}
> 
> This seems to largely re-invent g_strsplit
> 
> https://developer.gnome.org/glib/2.62/glib-String-Utility-Functions.html#g-s
> trsplit

Yes, except that g_strsplit() outputs empty array elements as well. That's not 
desired/working for this patch.

> > +
> > +static void split_free(char ***out)
> > +{
> > +    int i;
> > +    for (i = 0; (*out)[i]; ++i) {
> > +        g_free((*out)[i]);
> > +    }
> > +    g_free(*out);
> > +    *out = NULL;
> > +}
> 
> And g_strfreev
> 
> 
> 
> Regards,
> Daniel

Best regards,
Christian Schoenebeck




^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 11/11] tests/9pfs: add local Tmkdir test
  2020-10-02 13:41     ` Christian Schoenebeck
@ 2020-10-02 13:44       ` Daniel P. Berrangé
  2020-10-02 14:09         ` Christian Schoenebeck
  0 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrangé @ 2020-10-02 13:44 UTC (permalink / raw)
  To: Christian Schoenebeck
  Cc: Laurent Vivier, Thomas Huth, Emanuele Giuseppe Esposito,
	qemu-devel, Greg Kurz, Paolo Bonzini

On Fri, Oct 02, 2020 at 03:41:07PM +0200, Christian Schoenebeck wrote:
> On Freitag, 2. Oktober 2020 14:56:14 CEST Daniel P. Berrangé wrote:
> > On Fri, Oct 02, 2020 at 01:51:54PM +0200, Christian Schoenebeck wrote:
> > > This test case uses the 9pfs 'local' driver to create a directory
> > > and then checks if the expected directory was actually created
> > > (as real directory) on host side.
> > > 
> > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > ---
> > > 
> > >  tests/qtest/virtio-9p-test.c | 139 +++++++++++++++++++++++++++++++++++
> > >  1 file changed, 139 insertions(+)
> > > 
> > > diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> > > index af7e169d3a..93161a4b35 100644
> > > --- a/tests/qtest/virtio-9p-test.c
> > > +++ b/tests/qtest/virtio-9p-test.c
> > > @@ -18,6 +18,62 @@
> > > 
> > >  #define QVIRTIO_9P_TIMEOUT_US (10 * 1000 * 1000)
> > >  static QGuestAllocator *alloc;
> > > 
> > > +/*
> > > + * Used to auto generate new fids. Start with arbitrary high value to
> > > avoid + * collision with hard coded fids in basic test code.
> > > + */
> > > +static uint32_t fid_generator = 1000;
> > > +
> > > +static uint32_t genfid(void)
> > > +{
> > > +    return fid_generator++;
> > > +}
> > > +
> > > +/**
> > > + * Splits the @a in string by @a delim into individual (non empty)
> > > strings
> > > + * and outputs them to @a out. The output array @a out is NULL
> > > terminated.
> > > + *
> > > + * Output array @a out must be freed by calling split_free().
> > > + *
> > > + * @returns number of individual elements in output array @a out (without
> > > the + *          final NULL terminating element)
> > > + */
> > > +static int split(const char *in, const char *delim, char ***out)
> > > +{
> > > +    int n = 0, i = 0;
> > > +    char *tmp, *p;
> > > +
> > > +    tmp = g_strdup(in);
> > > +    for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim)) {
> > > +        if (strlen(p) > 0) {
> > > +            ++n;
> > > +        }
> > > +    }
> > > +    g_free(tmp);
> > > +
> > > +    *out = g_malloc0(n * sizeof(char *) + 1); /* last element NULL
> > > delimiter */
> > Surely this should be  (n + 1) * sizeof(char *), because the last
> > element still needs to be large enough to hold a pointer, not a
> > single extra byte.
> 
> Right, good catch!
> 
> > > +
> > > +    tmp = g_strdup(in);
> > > +    for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim)) {
> > > +        if (strlen(p) > 0) {
> > > +            (*out)[i++] = g_strdup(p);
> > > +        }
> > > +    }
> > > +    g_free(tmp);
> > > +
> > > +    return n;
> > > +}
> > 
> > This seems to largely re-invent g_strsplit
> > 
> > https://developer.gnome.org/glib/2.62/glib-String-Utility-Functions.html#g-s
> > trsplit
> 
> Yes, except that g_strsplit() outputs empty array elements as well. That's not 
> desired/working for this patch.

Either make the caller ignore/skip over empty elements, or make
this method call g_strsplit and then filter out the empty elements.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 11/11] tests/9pfs: add local Tmkdir test
  2020-10-02 13:44       ` Daniel P. Berrangé
@ 2020-10-02 14:09         ` Christian Schoenebeck
  2020-10-02 14:23           ` Daniel P. Berrangé
  0 siblings, 1 reply; 22+ messages in thread
From: Christian Schoenebeck @ 2020-10-02 14:09 UTC (permalink / raw)
  To: qemu-devel, Daniel P. Berrangé
  Cc: Laurent Vivier, Thomas Huth, Emanuele Giuseppe Esposito,
	Greg Kurz, Paolo Bonzini

On Freitag, 2. Oktober 2020 15:44:40 CEST Daniel P. Berrangé wrote:
> On Fri, Oct 02, 2020 at 03:41:07PM +0200, Christian Schoenebeck wrote:
> > On Freitag, 2. Oktober 2020 14:56:14 CEST Daniel P. Berrangé wrote:
> > > On Fri, Oct 02, 2020 at 01:51:54PM +0200, Christian Schoenebeck wrote:
> > > > This test case uses the 9pfs 'local' driver to create a directory
> > > > and then checks if the expected directory was actually created
> > > > (as real directory) on host side.
> > > > 
> > > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > > ---
> > > > 
> > > >  tests/qtest/virtio-9p-test.c | 139
> > > >  +++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 139 insertions(+)
> > > > 
> > > > diff --git a/tests/qtest/virtio-9p-test.c
> > > > b/tests/qtest/virtio-9p-test.c
> > > > index af7e169d3a..93161a4b35 100644
> > > > --- a/tests/qtest/virtio-9p-test.c
> > > > +++ b/tests/qtest/virtio-9p-test.c
> > > > @@ -18,6 +18,62 @@
> > > > 
> > > >  #define QVIRTIO_9P_TIMEOUT_US (10 * 1000 * 1000)
> > > >  static QGuestAllocator *alloc;
> > > > 
> > > > +/*
> > > > + * Used to auto generate new fids. Start with arbitrary high value to
> > > > avoid + * collision with hard coded fids in basic test code.
> > > > + */
> > > > +static uint32_t fid_generator = 1000;
> > > > +
> > > > +static uint32_t genfid(void)
> > > > +{
> > > > +    return fid_generator++;
> > > > +}
> > > > +
> > > > +/**
> > > > + * Splits the @a in string by @a delim into individual (non empty)
> > > > strings
> > > > + * and outputs them to @a out. The output array @a out is NULL
> > > > terminated.
> > > > + *
> > > > + * Output array @a out must be freed by calling split_free().
> > > > + *
> > > > + * @returns number of individual elements in output array @a out
> > > > (without
> > > > the + *          final NULL terminating element)
> > > > + */
> > > > +static int split(const char *in, const char *delim, char ***out)
> > > > +{
> > > > +    int n = 0, i = 0;
> > > > +    char *tmp, *p;
> > > > +
> > > > +    tmp = g_strdup(in);
> > > > +    for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim))
> > > > {
> > > > +        if (strlen(p) > 0) {
> > > > +            ++n;
> > > > +        }
> > > > +    }
> > > > +    g_free(tmp);
> > > > +
> > > > +    *out = g_malloc0(n * sizeof(char *) + 1); /* last element NULL
> > > > delimiter */
> > > 
> > > Surely this should be  (n + 1) * sizeof(char *), because the last
> > > element still needs to be large enough to hold a pointer, not a
> > > single extra byte.
> > 
> > Right, good catch!
> > 
> > > > +
> > > > +    tmp = g_strdup(in);
> > > > +    for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim))
> > > > {
> > > > +        if (strlen(p) > 0) {
> > > > +            (*out)[i++] = g_strdup(p);
> > > > +        }
> > > > +    }
> > > > +    g_free(tmp);
> > > > +
> > > > +    return n;
> > > > +}
> > > 
> > > This seems to largely re-invent g_strsplit
> > > 
> > > https://developer.gnome.org/glib/2.62/glib-String-Utility-Functions.html
> > > #g-s trsplit
> > 
> > Yes, except that g_strsplit() outputs empty array elements as well. That's
> > not desired/working for this patch.
> 
> Either make the caller ignore/skip over empty elements, or make

Not an option, since it would create too much test code overhead. I really 
need [const int elements] and [array without empty elements].

> this method call g_strsplit and then filter out the empty elements.

Mmm, so you're suggesting to allocate a new array, copy elements from 
g_strsplit() array to the new array, and eventually pass that manually 
allocated array to g_strfreev()? Wouldn't that be a bit unsafe regarding 
potential future changes in how glib allocates/structures those string arrays?

At least I don't see a glib function that would filter string arrays out of 
the box.

Best regards,
Christian Schoenebeck




^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 11/11] tests/9pfs: add local Tmkdir test
  2020-10-02 14:09         ` Christian Schoenebeck
@ 2020-10-02 14:23           ` Daniel P. Berrangé
  2020-10-02 14:30             ` Christian Schoenebeck
  0 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrangé @ 2020-10-02 14:23 UTC (permalink / raw)
  To: Christian Schoenebeck
  Cc: Laurent Vivier, Thomas Huth, Emanuele Giuseppe Esposito,
	qemu-devel, Greg Kurz, Paolo Bonzini

On Fri, Oct 02, 2020 at 04:09:53PM +0200, Christian Schoenebeck wrote:
> On Freitag, 2. Oktober 2020 15:44:40 CEST Daniel P. Berrangé wrote:
> > On Fri, Oct 02, 2020 at 03:41:07PM +0200, Christian Schoenebeck wrote:
> > > On Freitag, 2. Oktober 2020 14:56:14 CEST Daniel P. Berrangé wrote:
> > > > On Fri, Oct 02, 2020 at 01:51:54PM +0200, Christian Schoenebeck wrote:
> > > > > This test case uses the 9pfs 'local' driver to create a directory
> > > > > and then checks if the expected directory was actually created
> > > > > (as real directory) on host side.
> > > > > 
> > > > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > > > ---
> > > > > 
> > > > >  tests/qtest/virtio-9p-test.c | 139
> > > > >  +++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 139 insertions(+)
> > > > > 
> > > > > diff --git a/tests/qtest/virtio-9p-test.c
> > > > > b/tests/qtest/virtio-9p-test.c
> > > > > index af7e169d3a..93161a4b35 100644
> > > > > --- a/tests/qtest/virtio-9p-test.c
> > > > > +++ b/tests/qtest/virtio-9p-test.c
> > > > > @@ -18,6 +18,62 @@
> > > > > 
> > > > >  #define QVIRTIO_9P_TIMEOUT_US (10 * 1000 * 1000)
> > > > >  static QGuestAllocator *alloc;
> > > > > 
> > > > > +/*
> > > > > + * Used to auto generate new fids. Start with arbitrary high value to
> > > > > avoid + * collision with hard coded fids in basic test code.
> > > > > + */
> > > > > +static uint32_t fid_generator = 1000;
> > > > > +
> > > > > +static uint32_t genfid(void)
> > > > > +{
> > > > > +    return fid_generator++;
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * Splits the @a in string by @a delim into individual (non empty)
> > > > > strings
> > > > > + * and outputs them to @a out. The output array @a out is NULL
> > > > > terminated.
> > > > > + *
> > > > > + * Output array @a out must be freed by calling split_free().
> > > > > + *
> > > > > + * @returns number of individual elements in output array @a out
> > > > > (without
> > > > > the + *          final NULL terminating element)
> > > > > + */
> > > > > +static int split(const char *in, const char *delim, char ***out)
> > > > > +{
> > > > > +    int n = 0, i = 0;
> > > > > +    char *tmp, *p;
> > > > > +
> > > > > +    tmp = g_strdup(in);
> > > > > +    for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim))
> > > > > {
> > > > > +        if (strlen(p) > 0) {
> > > > > +            ++n;
> > > > > +        }
> > > > > +    }
> > > > > +    g_free(tmp);
> > > > > +
> > > > > +    *out = g_malloc0(n * sizeof(char *) + 1); /* last element NULL
> > > > > delimiter */
> > > > 
> > > > Surely this should be  (n + 1) * sizeof(char *), because the last
> > > > element still needs to be large enough to hold a pointer, not a
> > > > single extra byte.
> > > 
> > > Right, good catch!
> > > 
> > > > > +
> > > > > +    tmp = g_strdup(in);
> > > > > +    for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim))
> > > > > {
> > > > > +        if (strlen(p) > 0) {
> > > > > +            (*out)[i++] = g_strdup(p);
> > > > > +        }
> > > > > +    }
> > > > > +    g_free(tmp);
> > > > > +
> > > > > +    return n;
> > > > > +}
> > > > 
> > > > This seems to largely re-invent g_strsplit
> > > > 
> > > > https://developer.gnome.org/glib/2.62/glib-String-Utility-Functions.html
> > > > #g-s trsplit
> > > 
> > > Yes, except that g_strsplit() outputs empty array elements as well. That's
> > > not desired/working for this patch.
> > 
> > Either make the caller ignore/skip over empty elements, or make
> 
> Not an option, since it would create too much test code overhead. I really 
> need [const int elements] and [array without empty elements].
> 
> > this method call g_strsplit and then filter out the empty elements.
> 
> Mmm, so you're suggesting to allocate a new array, copy elements from 
> g_strsplit() array to the new array, and eventually pass that manually 
> allocated array to g_strfreev()? Wouldn't that be a bit unsafe regarding 
> potential future changes in how glib allocates/structures those string arrays?

No need to realloate a new array - just shuffle down the elements in
the array you get back from g_strsplit to discard the empty ones. Sure
the array will end up with a series of NULLs at the end but that is
harmless.

both g_strsplit/g_strfreev work in terms of a normal allocated "char **"
array, so there's no risk of changes to way memory is allocated.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 08/11] tests/9pfs: introduce local tests
  2020-10-02 11:51 ` [PATCH v2 08/11] tests/9pfs: introduce local tests Christian Schoenebeck
@ 2020-10-02 14:26   ` Christian Schoenebeck
  2020-10-02 14:35     ` Daniel P. Berrangé
  0 siblings, 1 reply; 22+ messages in thread
From: Christian Schoenebeck @ 2020-10-02 14:26 UTC (permalink / raw)
  To: qemu-devel
  Cc: Thomas Huth, Laurent Vivier, Paolo Bonzini,
	Emanuele Giuseppe Esposito, Greg Kurz, Daniel P. Berrangé

On Freitag, 2. Oktober 2020 13:51:54 CEST Christian Schoenebeck wrote:
> This patch introduces 9pfs test cases using the 9pfs 'local'
> filesystem driver which reads/writes/creates/deletes real files
> and directories.
> 
> In this initial version, there is only one local test which actually
> only checks if the 9pfs 'local' device was created successfully.
> 
> Before the 9pfs 'local' tests are run, a test directory 'qtest-9p-local'
> is created (with world rwx permissions) under the current working
> directory. At this point that test directory is not auto deleted yet.
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  tests/qtest/libqos/virtio-9p.c | 100 +++++++++++++++++++++++++++++++++
>  tests/qtest/libqos/virtio-9p.h |   5 ++
>  tests/qtest/virtio-9p-test.c   |  44 ++++++++++-----
>  3 files changed, 135 insertions(+), 14 deletions(-)
> 
> diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
> index 2e300063e3..86e40e5d56 100644
> --- a/tests/qtest/libqos/virtio-9p.c
> +++ b/tests/qtest/libqos/virtio-9p.c
> @@ -24,6 +24,63 @@
>  #include "qgraph.h"
> 
>  static QGuestAllocator *alloc;
> +static char *local_test_path;
> +
> +static char *strpr(const char* format, ...) GCC_FMT_ATTR(1, 2);
> +
> +/* Concatenates the passed 2 pathes. Returned result must be freed. */
> +static char *concat_path(const char* a, const char* b)
> +{
> +    const int len = strlen(a) + strlen("/") + strlen(b);
> +    char *path = g_malloc0(len + 1);
> +    snprintf(path, len + 1, "%s/%s", a, b);
> +    g_assert(strlen(path) == len);
> +    return path;
> +}

Ok, but maybe I could make that concat_path() function wrap g_strconcat().

> +
> +/*
> + * Lazy sprintf() implementation which auto allocates buffer. Returned
> result + * must be freed.
> + */
> +static char *strpr(const char* format, ...)
> +{
> +    va_list argp;
> +
> +    va_start(argp, format);
> +    const int sz = vsnprintf(NULL, 0, format, argp) + 1;
> +    va_end(argp);
> +
> +    g_assert(sz > 0);
> +    char *s = g_malloc0(sz);
> +
> +    va_start(argp, format);
> +    const int len = vsnprintf(s, sz, format, argp);
> +    va_end(argp);
> +
> +    g_assert(len + 1 == sz);
> +    return s;
> +}

And this strpr() function entirely be replaced by g_strdup_printf().

> +
> +static void init_local_test_path(void)
> +{
> +    char *pwd = get_current_dir_name();
> +    local_test_path = concat_path(pwd, "qtest-9p-local");
> +    free(pwd);
> +}
> +
> +/* Creates the directory for the 9pfs 'local' filesystem driver to access.
> */ +static void create_local_test_dir(void)
> +{
> +    struct stat st;
> +
> +    g_assert(local_test_path != NULL);
> +    mkdir(local_test_path, 0777);
> +
> +    /* ensure test directory exists now ... */
> +    g_assert(stat(local_test_path, &st) == 0);
> +    /* ... and is actually a directory */
> +    g_assert((st.st_mode & S_IFMT) == S_IFDIR);
> +}
> 
>  static void virtio_9p_cleanup(QVirtio9P *interface)
>  {
> @@ -146,11 +203,54 @@ static void *virtio_9p_pci_create(void *pci_bus,
> QGuestAllocator *t_alloc, return obj;
>  }
> 
> +void virtio_9p_assign_local_driver(GString *cmd_line, const char *args)
> +{
> +    GRegex *regex;
> +    char *s, *arg_repl;
> +
> +    g_assert_nonnull(local_test_path);
> +
> +    /* replace 'synth' driver by 'local' driver */
> +    regex = g_regex_new("-fsdev synth,", 0, 0, NULL);
> +    s = g_regex_replace_literal(
> +        regex, cmd_line->str, -1, 0, "-fsdev local,", 0, NULL
> +    );
> +    g_string_assign(cmd_line, s);
> +    g_free(s);
> +    g_regex_unref(regex);
> +
> +    /* add 'path=...' to '-fsdev ...' group */
> +    regex = g_regex_new("(-fsdev \\w+)(\\s*)", 0, 0, NULL);
> +    arg_repl = strpr("\\1\\2,path='%s'", local_test_path);
> +    s = g_regex_replace(
> +        regex, cmd_line->str, -1, 0, arg_repl, 0, NULL
> +    );
> +    g_string_assign(cmd_line, s);
> +    g_free(arg_repl);
> +    g_free(s);
> +    g_regex_unref(regex);
> +
> +    /* add passed args to '-fsdev ...' group */
> +    regex = g_regex_new("(-fsdev \\w+)(\\s*)", 0, 0, NULL);
> +    arg_repl = strpr("\\1\\2,%s", args);
> +    s = g_regex_replace(
> +        regex, cmd_line->str, -1, 0, arg_repl, 0, NULL
> +    );
> +    g_string_assign(cmd_line, s);
> +    g_free(arg_repl);
> +    g_free(s);
> +    g_regex_unref(regex);
> +}
> +
>  static void virtio_9p_register_nodes(void)
>  {
>      const char *str_simple = "fsdev=fsdev0,mount_tag=" MOUNT_TAG;
>      const char *str_addr = "fsdev=fsdev0,addr=04.0,mount_tag=" MOUNT_TAG;
> 
> +    /* make sure test dir for the 'local' tests exists and is clean */
> +    init_local_test_path();
> +    create_local_test_dir();
> +
>      QPCIAddress addr = {
>          .devfn = QPCI_DEVFN(4, 0),
>      };
> diff --git a/tests/qtest/libqos/virtio-9p.h b/tests/qtest/libqos/virtio-9p.h
> index b1e6badc4a..326a603f72 100644
> --- a/tests/qtest/libqos/virtio-9p.h
> +++ b/tests/qtest/libqos/virtio-9p.h
> @@ -44,4 +44,9 @@ struct QVirtio9PDevice {
>      QVirtio9P v9p;
>  };
> 
> +/**
> + * Prepares QEMU command line for 9pfs tests using the 'local' fs driver.
> + */
> +void virtio_9p_assign_local_driver(GString *cmd_line, const char *args);
> +
>  #endif
> diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> index 3281153b9c..af7e169d3a 100644
> --- a/tests/qtest/virtio-9p-test.c
> +++ b/tests/qtest/virtio-9p-test.c
> @@ -895,29 +895,45 @@ static void fs_readdir_split_512(void *obj, void
> *data, fs_readdir_split(obj, data, t_alloc, 512);
>  }
> 
> +static void *assign_9p_local_driver(GString *cmd_line, void *arg)
> +{
> +    virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
> +    return arg;
> +}
> +
>  static void register_virtio_9p_test(void)
>  {
> -    qos_add_test("synth/config", "virtio-9p", pci_config, NULL);
> -    qos_add_test("synth/version/basic", "virtio-9p", fs_version, NULL);
> -    qos_add_test("synth/attach/basic", "virtio-9p", fs_attach, NULL);
> -    qos_add_test("synth/walk/basic", "virtio-9p", fs_walk, NULL);
> +
> +    QOSGraphTestOptions opts = {
> +    };
> +
> +    /* 9pfs test cases using the 'synth' filesystem driver */
> +    qos_add_test("synth/config", "virtio-9p", pci_config, &opts);
> +    qos_add_test("synth/version/basic", "virtio-9p", fs_version,  &opts);
> +    qos_add_test("synth/attach/basic", "virtio-9p", fs_attach,  &opts);
> +    qos_add_test("synth/walk/basic", "virtio-9p", fs_walk,  &opts);
>      qos_add_test("synth/walk/no_slash", "virtio-9p", fs_walk_no_slash,
> -                 NULL);
> +                  &opts);
>      qos_add_test("synth/walk/dotdot_from_root", "virtio-9p",
> -                 fs_walk_dotdot, NULL);
> -    qos_add_test("synth/lopen/basic", "virtio-9p", fs_lopen, NULL);
> -    qos_add_test("synth/write/basic", "virtio-9p", fs_write, NULL);
> +                 fs_walk_dotdot,  &opts);
> +    qos_add_test("synth/lopen/basic", "virtio-9p", fs_lopen,  &opts);
> +    qos_add_test("synth/write/basic", "virtio-9p", fs_write,  &opts);
>      qos_add_test("synth/flush/success", "virtio-9p", fs_flush_success,
> -                 NULL);
> +                  &opts);
>      qos_add_test("synth/flush/ignored", "virtio-9p", fs_flush_ignored,
> -                 NULL);
> -    qos_add_test("synth/readdir/basic", "virtio-9p", fs_readdir, NULL);
> +                  &opts);
> +    qos_add_test("synth/readdir/basic", "virtio-9p", fs_readdir,  &opts);
>      qos_add_test("synth/readdir/split_512", "virtio-9p",
> -                 fs_readdir_split_512, NULL);
> +                 fs_readdir_split_512,  &opts);
>      qos_add_test("synth/readdir/split_256", "virtio-9p",
> -                 fs_readdir_split_256, NULL);
> +                 fs_readdir_split_256,  &opts);
>      qos_add_test("synth/readdir/split_128", "virtio-9p",
> -                 fs_readdir_split_128, NULL);
> +                 fs_readdir_split_128,  &opts);
> +
> +
> +    /* 9pfs test cases using the 'local' filesystem driver */
> +    opts.before = assign_9p_local_driver;
> +    qos_add_test("local/config", "virtio-9p", pci_config,  &opts);
>  }
> 
>  libqos_init(register_virtio_9p_test);

Best regards,
Christian Schoenebeck




^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 11/11] tests/9pfs: add local Tmkdir test
  2020-10-02 14:23           ` Daniel P. Berrangé
@ 2020-10-02 14:30             ` Christian Schoenebeck
  0 siblings, 0 replies; 22+ messages in thread
From: Christian Schoenebeck @ 2020-10-02 14:30 UTC (permalink / raw)
  To: Daniel P. Berrangé
  Cc: qemu-devel, Laurent Vivier, Thomas Huth,
	Emanuele Giuseppe Esposito, Greg Kurz, Paolo Bonzini

On Freitag, 2. Oktober 2020 16:23:31 CEST Daniel P. Berrangé wrote:
> No need to realloate a new array - just shuffle down the elements in
> the array you get back from g_strsplit to discard the empty ones. Sure
> the array will end up with a series of NULLs at the end but that is
> harmless.
> 
> both g_strsplit/g_strfreev work in terms of a normal allocated "char **"
> array, so there's no risk of changes to way memory is allocated.

Ok then, if you say so ... fine with me.

Best regards,
Christian Schoenebeck




^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 11/11] tests/9pfs: add local Tmkdir test
  2020-10-02 12:56   ` Daniel P. Berrangé
  2020-10-02 13:41     ` Christian Schoenebeck
@ 2020-10-02 14:32     ` Greg Kurz
  2020-10-02 15:00       ` Christian Schoenebeck
  1 sibling, 1 reply; 22+ messages in thread
From: Greg Kurz @ 2020-10-02 14:32 UTC (permalink / raw)
  To: Daniel P. Berrangé
  Cc: Laurent Vivier, Thomas Huth, Emanuele Giuseppe Esposito,
	Christian Schoenebeck, qemu-devel, Paolo Bonzini

On Fri, 2 Oct 2020 13:56:14 +0100
Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Fri, Oct 02, 2020 at 01:51:54PM +0200, Christian Schoenebeck wrote:
> > This test case uses the 9pfs 'local' driver to create a directory
> > and then checks if the expected directory was actually created
> > (as real directory) on host side.
> > 
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> >  tests/qtest/virtio-9p-test.c | 139 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 139 insertions(+)
> > 
> > diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> > index af7e169d3a..93161a4b35 100644
> > --- a/tests/qtest/virtio-9p-test.c
> > +++ b/tests/qtest/virtio-9p-test.c
> > @@ -18,6 +18,62 @@
> >  #define QVIRTIO_9P_TIMEOUT_US (10 * 1000 * 1000)
> >  static QGuestAllocator *alloc;
> >  
> > +/*
> > + * Used to auto generate new fids. Start with arbitrary high value to avoid
> > + * collision with hard coded fids in basic test code.
> > + */
> > +static uint32_t fid_generator = 1000;
> > +
> > +static uint32_t genfid(void)
> > +{
> > +    return fid_generator++;
> > +}
> > +
> > +/**
> > + * Splits the @a in string by @a delim into individual (non empty) strings
> > + * and outputs them to @a out. The output array @a out is NULL terminated.
> > + *
> > + * Output array @a out must be freed by calling split_free().
> > + *
> > + * @returns number of individual elements in output array @a out (without the
> > + *          final NULL terminating element)
> > + */
> > +static int split(const char *in, const char *delim, char ***out)
> > +{
> > +    int n = 0, i = 0;
> > +    char *tmp, *p;
> > +
> > +    tmp = g_strdup(in);
> > +    for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim)) {
> > +        if (strlen(p) > 0) {
> > +            ++n;
> > +        }
> > +    }
> > +    g_free(tmp);
> > +
> > +    *out = g_malloc0(n * sizeof(char *) + 1); /* last element NULL delimiter */
> 
> Surely this should be  (n + 1) * sizeof(char *), because the last
> element still needs to be large enough to hold a pointer, not a
> single extra byte.
> 

If you decide to keep this split() function, maybe use g_new0(char *, n + 1) ?
This buys you the math and does type checking as an extra.

> > +
> > +    tmp = g_strdup(in);
> > +    for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim)) {
> > +        if (strlen(p) > 0) {
> > +            (*out)[i++] = g_strdup(p);
> > +        }
> > +    }
> > +    g_free(tmp);
> > +
> > +    return n;
> > +}
> 
> This seems to largely re-invent g_strsplit 
> 
> https://developer.gnome.org/glib/2.62/glib-String-Utility-Functions.html#g-strsplit
> 
> > +
> > +static void split_free(char ***out)
> > +{
> > +    int i;
> > +    for (i = 0; (*out)[i]; ++i) {
> > +        g_free((*out)[i]);
> > +    }
> > +    g_free(*out);
> > +    *out = NULL;
> > +}
> 
> And g_strfreev
> 
> 
> 
> Regards,
> Daniel



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 08/11] tests/9pfs: introduce local tests
  2020-10-02 14:26   ` Christian Schoenebeck
@ 2020-10-02 14:35     ` Daniel P. Berrangé
  0 siblings, 0 replies; 22+ messages in thread
From: Daniel P. Berrangé @ 2020-10-02 14:35 UTC (permalink / raw)
  To: Christian Schoenebeck
  Cc: Laurent Vivier, Thomas Huth, Emanuele Giuseppe Esposito,
	qemu-devel, Greg Kurz, Paolo Bonzini

On Fri, Oct 02, 2020 at 04:26:48PM +0200, Christian Schoenebeck wrote:
> On Freitag, 2. Oktober 2020 13:51:54 CEST Christian Schoenebeck wrote:
> > This patch introduces 9pfs test cases using the 9pfs 'local'
> > filesystem driver which reads/writes/creates/deletes real files
> > and directories.
> > 
> > In this initial version, there is only one local test which actually
> > only checks if the 9pfs 'local' device was created successfully.
> > 
> > Before the 9pfs 'local' tests are run, a test directory 'qtest-9p-local'
> > is created (with world rwx permissions) under the current working
> > directory. At this point that test directory is not auto deleted yet.
> > 
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> >  tests/qtest/libqos/virtio-9p.c | 100 +++++++++++++++++++++++++++++++++
> >  tests/qtest/libqos/virtio-9p.h |   5 ++
> >  tests/qtest/virtio-9p-test.c   |  44 ++++++++++-----
> >  3 files changed, 135 insertions(+), 14 deletions(-)
> > 
> > diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
> > index 2e300063e3..86e40e5d56 100644
> > --- a/tests/qtest/libqos/virtio-9p.c
> > +++ b/tests/qtest/libqos/virtio-9p.c
> > @@ -24,6 +24,63 @@
> >  #include "qgraph.h"
> > 
> >  static QGuestAllocator *alloc;
> > +static char *local_test_path;
> > +
> > +static char *strpr(const char* format, ...) GCC_FMT_ATTR(1, 2);
> > +
> > +/* Concatenates the passed 2 pathes. Returned result must be freed. */
> > +static char *concat_path(const char* a, const char* b)
> > +{
> > +    const int len = strlen(a) + strlen("/") + strlen(b);
> > +    char *path = g_malloc0(len + 1);
> > +    snprintf(path, len + 1, "%s/%s", a, b);
> > +    g_assert(strlen(path) == len);
> > +    return path;
> > +}
> 
> Ok, but maybe I could make that concat_path() function wrap g_strconcat().

Or even one of g_build_path or g_build_filename may be useful

> > +/*
> > + * Lazy sprintf() implementation which auto allocates buffer. Returned
> > result + * must be freed.
> > + */
> > +static char *strpr(const char* format, ...)
> > +{
> > +    va_list argp;
> > +
> > +    va_start(argp, format);
> > +    const int sz = vsnprintf(NULL, 0, format, argp) + 1;
> > +    va_end(argp);
> > +
> > +    g_assert(sz > 0);
> > +    char *s = g_malloc0(sz);
> > +
> > +    va_start(argp, format);
> > +    const int len = vsnprintf(s, sz, format, argp);
> > +    va_end(argp);
> > +
> > +    g_assert(len + 1 == sz);
> > +    return s;
> > +}
> 
> And this strpr() function entirely be replaced by g_strdup_printf().

Yep, its preferrable to use g_strdup_printf instead of manually
allocating.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 11/11] tests/9pfs: add local Tmkdir test
  2020-10-02 14:32     ` Greg Kurz
@ 2020-10-02 15:00       ` Christian Schoenebeck
  0 siblings, 0 replies; 22+ messages in thread
From: Christian Schoenebeck @ 2020-10-02 15:00 UTC (permalink / raw)
  To: qemu-devel
  Cc: Greg Kurz, Daniel P. Berrangé,
	Laurent Vivier, Thomas Huth, Emanuele Giuseppe Esposito,
	Paolo Bonzini

On Freitag, 2. Oktober 2020 16:32:55 CEST Greg Kurz wrote:
> On Fri, 2 Oct 2020 13:56:14 +0100
> 
> Daniel P. Berrangé <berrange@redhat.com> wrote:
> > On Fri, Oct 02, 2020 at 01:51:54PM +0200, Christian Schoenebeck wrote:
> > > This test case uses the 9pfs 'local' driver to create a directory
> > > and then checks if the expected directory was actually created
> > > (as real directory) on host side.
> > > 
> > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > ---
> > > 
> > >  tests/qtest/virtio-9p-test.c | 139 +++++++++++++++++++++++++++++++++++
> > >  1 file changed, 139 insertions(+)
> > > 
> > > diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> > > index af7e169d3a..93161a4b35 100644
> > > --- a/tests/qtest/virtio-9p-test.c
> > > +++ b/tests/qtest/virtio-9p-test.c
> > > @@ -18,6 +18,62 @@
> > > 
> > >  #define QVIRTIO_9P_TIMEOUT_US (10 * 1000 * 1000)
> > >  static QGuestAllocator *alloc;
> > > 
> > > +/*
> > > + * Used to auto generate new fids. Start with arbitrary high value to
> > > avoid + * collision with hard coded fids in basic test code.
> > > + */
> > > +static uint32_t fid_generator = 1000;
> > > +
> > > +static uint32_t genfid(void)
> > > +{
> > > +    return fid_generator++;
> > > +}
> > > +
> > > +/**
> > > + * Splits the @a in string by @a delim into individual (non empty)
> > > strings
> > > + * and outputs them to @a out. The output array @a out is NULL
> > > terminated.
> > > + *
> > > + * Output array @a out must be freed by calling split_free().
> > > + *
> > > + * @returns number of individual elements in output array @a out
> > > (without the + *          final NULL terminating element)
> > > + */
> > > +static int split(const char *in, const char *delim, char ***out)
> > > +{
> > > +    int n = 0, i = 0;
> > > +    char *tmp, *p;
> > > +
> > > +    tmp = g_strdup(in);
> > > +    for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim)) {
> > > +        if (strlen(p) > 0) {
> > > +            ++n;
> > > +        }
> > > +    }
> > > +    g_free(tmp);
> > > +
> > > +    *out = g_malloc0(n * sizeof(char *) + 1); /* last element NULL
> > > delimiter */> 
> > Surely this should be  (n + 1) * sizeof(char *), because the last
> > element still needs to be large enough to hold a pointer, not a
> > single extra byte.
> 
> If you decide to keep this split() function, maybe use g_new0(char *, n + 1)
> ? This buys you the math and does type checking as an extra.

Yes, that was my plan B.

But if Daniel sais its long-term safe to pass a re-shuffled array to 
g_strfreev(), well then I go that route.

Best regards,
Christian Schoenebeck




^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2020-10-02 15:19 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-02 11:52 [PATCH v2 00/11] 9pfs: add tests using local fs driver Christian Schoenebeck
2020-10-02 11:51 ` [PATCH v2 09/11] tests/9pfs: wipe local 9pfs test directory Christian Schoenebeck
2020-10-02 11:51 ` [PATCH v2 02/11] libqos/qgraph: add qos_node_create_driver_named() Christian Schoenebeck
2020-10-02 11:51 ` [PATCH v2 04/11] tests/qtest/qos-test: dump qos graph if verbose Christian Schoenebeck
2020-10-02 11:51 ` [PATCH v2 06/11] tests/qtest/qos-test: dump QEMU command " Christian Schoenebeck
2020-10-02 11:51 ` [PATCH v2 10/11] tests/9pfs: add virtio_9p_test_path() Christian Schoenebeck
2020-10-02 11:51 ` [PATCH v2 03/11] libqos/qgraph: add qos_dump_graph() Christian Schoenebeck
2020-10-02 11:51 ` [PATCH v2 05/11] tests/qtest/qos-test: dump environment variables if verbose Christian Schoenebeck
2020-10-02 11:51 ` [PATCH v2 08/11] tests/9pfs: introduce local tests Christian Schoenebeck
2020-10-02 14:26   ` Christian Schoenebeck
2020-10-02 14:35     ` Daniel P. Berrangé
2020-10-02 11:51 ` [PATCH v2 07/11] tests/9pfs: change qtest name prefix to synth Christian Schoenebeck
2020-10-02 11:51 ` [PATCH v2 01/11] libqos/qgraph: add qemu_name to QOSGraphNode Christian Schoenebeck
2020-10-02 11:51 ` [PATCH v2 11/11] tests/9pfs: add local Tmkdir test Christian Schoenebeck
2020-10-02 12:56   ` Daniel P. Berrangé
2020-10-02 13:41     ` Christian Schoenebeck
2020-10-02 13:44       ` Daniel P. Berrangé
2020-10-02 14:09         ` Christian Schoenebeck
2020-10-02 14:23           ` Daniel P. Berrangé
2020-10-02 14:30             ` Christian Schoenebeck
2020-10-02 14:32     ` Greg Kurz
2020-10-02 15:00       ` Christian Schoenebeck

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.