[PATCH v2 0/8] 9pfs: more local tests

[PATCH v2 0/8] 9pfs: more local tests

[Christian Schoenebeck]

Just a bunch of more test cases using the 9pfs 'local' fs driver backend,
namely for these 9p requests:

  * Tunlinkat, Tlcreate, Tsymlink and Tlink.

v1->v2:

  * Rebased on latest 9p.next queue HEAD.

  * Use do_*() as new naming convention for utility functions.

  * Drop unnecessary arguments of utility functions.

  * Always do 'alloc = t_alloc;' in toplevel test functions.

  * Split out do_hardlink() as utility function [patch 6].

Christian Schoenebeck (8):
  tests/9pfs: simplify do_mkdir()
  tests/9pfs: add local Tunlinkat directory test
  tests/9pfs: add local Tlcreate test
  tests/9pfs: add local Tunlinkat file test
  tests/9pfs: add local Tsymlink test
  tests/9pfs: add local Tunlinkat symlink test
  tests/9pfs: add local Tlink test
  tests/9pfs: add local Tunlinkat hard link test

 tests/qtest/virtio-9p-test.c | 405 ++++++++++++++++++++++++++++++++++-
 1 file changed, 397 insertions(+), 8 deletions(-)

-- 
2.20.1

[PATCH v2 1/8] tests/9pfs: simplify do_mkdir()

[Christian Schoenebeck]

Split out walking a directory path to a separate new utility function
do_walk() and use that function in do_mkdir().

The code difference saved this way is not much, but we'll use that new
do_walk() function in the upcoming patches, so it will avoid quite
some code duplication after all.

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

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 2ea555fa04..21807037df 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -583,6 +583,23 @@ static void do_version(QVirtio9P *v9p)
     g_free(server_version);
 }
 
+/* utility function: walk to requested dir and return fid for that dir */
+static uint32_t do_walk(QVirtio9P *v9p, const char *path)
+{
+    char **wnames;
+    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);
+
+    split_free(&wnames);
+    return fid;
+}
+
 static void fs_version(void *obj, void *data, QGuestAllocator *t_alloc)
 {
     alloc = t_alloc;
@@ -974,23 +991,17 @@ static void fs_flush_ignored(void *obj, void *data, QGuestAllocator *t_alloc)
 
 static void do_mkdir(QVirtio9P *v9p, const char *path, const char *cname)
 {
-    char **wnames;
     char *const name = g_strdup(cname);
+    uint32_t fid;
     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);
+    fid = do_walk(v9p, path);
 
     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,
-- 
2.20.1

Re: [PATCH v2 1/8] tests/9pfs: simplify do_mkdir()

[Greg Kurz]

On Wed, 21 Oct 2020 14:06:53 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> Split out walking a directory path to a separate new utility function
> do_walk() and use that function in do_mkdir().
> 
> The code difference saved this way is not much, but we'll use that new
> do_walk() function in the upcoming patches, so it will avoid quite
> some code duplication after all.
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  tests/qtest/virtio-9p-test.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> index 2ea555fa04..21807037df 100644
> --- a/tests/qtest/virtio-9p-test.c
> +++ b/tests/qtest/virtio-9p-test.c
> @@ -583,6 +583,23 @@ static void do_version(QVirtio9P *v9p)
>      g_free(server_version);
>  }
>  
> +/* utility function: walk to requested dir and return fid for that dir */
> +static uint32_t do_walk(QVirtio9P *v9p, const char *path)
> +{
> +    char **wnames;
> +    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);
> +
> +    split_free(&wnames);
> +    return fid;
> +}
> +
>  static void fs_version(void *obj, void *data, QGuestAllocator *t_alloc)
>  {
>      alloc = t_alloc;
> @@ -974,23 +991,17 @@ static void fs_flush_ignored(void *obj, void *data, QGuestAllocator *t_alloc)
>  
>  static void do_mkdir(QVirtio9P *v9p, const char *path, const char *cname)
>  {
> -    char **wnames;
>      char *const name = g_strdup(cname);
> +    uint32_t fid;
>      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);
> +    fid = do_walk(v9p, path);
>  
>      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,

[PATCH v2 2/8] tests/9pfs: add local Tunlinkat directory test

[Christian Schoenebeck]

This test case uses a Tunlinkat 9p request with flag AT_REMOVEDIR
(see 'man 2 unlink') to remove a directory from host's test directory.

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

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 21807037df..abd7e44648 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -258,6 +258,7 @@ static const char *rmessage_name(uint8_t id)
         id == P9_RLOPEN ? "RLOPEN" :
         id == P9_RWRITE ? "RWRITE" :
         id == P9_RMKDIR ? "RMKDIR" :
+        id == P9_RUNLINKAT ? "RUNLINKAT" :
         id == P9_RFLUSH ? "RFLUSH" :
         id == P9_RREADDIR ? "READDIR" :
         "<unknown>";
@@ -693,6 +694,33 @@ static void v9fs_rmkdir(P9Req *req, v9fs_qid *qid)
     v9fs_req_free(req);
 }
 
+/* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */
+static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char *name,
+                             uint32_t flags, uint16_t tag)
+{
+    P9Req *req;
+
+    uint32_t body_size = 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_TUNLINKAT, tag);
+    v9fs_uint32_write(req, dirfd);
+    v9fs_string_write(req, name);
+    v9fs_uint32_write(req, flags);
+    v9fs_req_send(req);
+    return req;
+}
+
+/* size[4] Runlinkat tag[2] */
+static void v9fs_runlinkat(P9Req *req)
+{
+    v9fs_req_recv(req, P9_RUNLINKAT);
+    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)
 {
@@ -1004,6 +1032,22 @@ static void do_mkdir(QVirtio9P *v9p, const char *path, const char *cname)
     g_free(name);
 }
 
+static void do_unlinkat(QVirtio9P *v9p, const char *atpath, const char *rpath,
+                        uint32_t flags)
+{
+    char *const name = g_strdup(rpath);
+    uint32_t fid;
+    P9Req *req;
+
+    fid = do_walk(v9p, atpath);
+
+    req = v9fs_tunlinkat(v9p, fid, name, flags, 0);
+    v9fs_req_wait_for_reply(req, NULL);
+    v9fs_runlinkat(req);
+
+    g_free(name);
+}
+
 static void fs_readdir_split_128(void *obj, void *data,
                                  QGuestAllocator *t_alloc)
 {
@@ -1050,6 +1094,32 @@ static void fs_create_dir(void *obj, void *data, QGuestAllocator *t_alloc)
     g_free(root_path);
 }
 
+static void fs_unlinkat_dir(void *obj, void *data, QGuestAllocator *t_alloc)
+{
+    QVirtio9P *v9p = obj;
+    alloc = t_alloc;
+    struct stat st;
+    char *root_path = virtio_9p_test_path("");
+    char *new_dir = virtio_9p_test_path("02");
+
+    g_assert(root_path != NULL);
+
+    do_attach(v9p);
+    do_mkdir(v9p, "/", "02");
+
+    /* 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);
+
+    do_unlinkat(v9p, "/", "02", AT_REMOVEDIR);
+    /* directory should be gone now */
+    g_assert(stat(new_dir, &st) != 0);
+
+    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");
@@ -1090,6 +1160,7 @@ static void register_virtio_9p_test(void)
     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);
+    qos_add_test("local/unlinkat_dir", "virtio-9p", fs_unlinkat_dir, &opts);
 }
 
 libqos_init(register_virtio_9p_test);
-- 
2.20.1

Re: [PATCH v2 2/8] tests/9pfs: add local Tunlinkat directory test

[Greg Kurz]

On Wed, 21 Oct 2020 14:17:01 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> This test case uses a Tunlinkat 9p request with flag AT_REMOVEDIR
> (see 'man 2 unlink') to remove a directory from host's test directory.
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  tests/qtest/virtio-9p-test.c | 71 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 71 insertions(+)
> 
> diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> index 21807037df..abd7e44648 100644
> --- a/tests/qtest/virtio-9p-test.c
> +++ b/tests/qtest/virtio-9p-test.c
> @@ -258,6 +258,7 @@ static const char *rmessage_name(uint8_t id)
>          id == P9_RLOPEN ? "RLOPEN" :
>          id == P9_RWRITE ? "RWRITE" :
>          id == P9_RMKDIR ? "RMKDIR" :
> +        id == P9_RUNLINKAT ? "RUNLINKAT" :
>          id == P9_RFLUSH ? "RFLUSH" :
>          id == P9_RREADDIR ? "READDIR" :
>          "<unknown>";
> @@ -693,6 +694,33 @@ static void v9fs_rmkdir(P9Req *req, v9fs_qid *qid)
>      v9fs_req_free(req);
>  }
>  
> +/* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */
> +static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char *name,
> +                             uint32_t flags, uint16_t tag)
> +{
> +    P9Req *req;
> +
> +    uint32_t body_size = 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_TUNLINKAT, tag);
> +    v9fs_uint32_write(req, dirfd);
> +    v9fs_string_write(req, name);
> +    v9fs_uint32_write(req, flags);
> +    v9fs_req_send(req);
> +    return req;
> +}
> +
> +/* size[4] Runlinkat tag[2] */
> +static void v9fs_runlinkat(P9Req *req)
> +{
> +    v9fs_req_recv(req, P9_RUNLINKAT);
> +    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)
>  {
> @@ -1004,6 +1032,22 @@ static void do_mkdir(QVirtio9P *v9p, const char *path, const char *cname)
>      g_free(name);
>  }
>  
> +static void do_unlinkat(QVirtio9P *v9p, const char *atpath, const char *rpath,
> +                        uint32_t flags)
> +{
> +    char *const name = g_strdup(rpath);
> +    uint32_t fid;
> +    P9Req *req;
> +
> +    fid = do_walk(v9p, atpath);
> +
> +    req = v9fs_tunlinkat(v9p, fid, name, flags, 0);
> +    v9fs_req_wait_for_reply(req, NULL);
> +    v9fs_runlinkat(req);
> +
> +    g_free(name);
> +}
> +
>  static void fs_readdir_split_128(void *obj, void *data,
>                                   QGuestAllocator *t_alloc)
>  {
> @@ -1050,6 +1094,32 @@ static void fs_create_dir(void *obj, void *data, QGuestAllocator *t_alloc)
>      g_free(root_path);
>  }
>  
> +static void fs_unlinkat_dir(void *obj, void *data, QGuestAllocator *t_alloc)
> +{
> +    QVirtio9P *v9p = obj;
> +    alloc = t_alloc;
> +    struct stat st;
> +    char *root_path = virtio_9p_test_path("");
> +    char *new_dir = virtio_9p_test_path("02");
> +
> +    g_assert(root_path != NULL);
> +
> +    do_attach(v9p);
> +    do_mkdir(v9p, "/", "02");
> +
> +    /* 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);
> +
> +    do_unlinkat(v9p, "/", "02", AT_REMOVEDIR);
> +    /* directory should be gone now */
> +    g_assert(stat(new_dir, &st) != 0);
> +
> +    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");
> @@ -1090,6 +1160,7 @@ static void register_virtio_9p_test(void)
>      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);
> +    qos_add_test("local/unlinkat_dir", "virtio-9p", fs_unlinkat_dir, &opts);
>  }
>  
>  libqos_init(register_virtio_9p_test);

[PATCH v2 3/8] tests/9pfs: add local Tlcreate test

[Christian Schoenebeck]

This test case uses a Tlcreate 9p request to create a regular file inside
host's test directory.

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

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index abd7e44648..c030bc2a6c 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -258,6 +258,7 @@ static const char *rmessage_name(uint8_t id)
         id == P9_RLOPEN ? "RLOPEN" :
         id == P9_RWRITE ? "RWRITE" :
         id == P9_RMKDIR ? "RMKDIR" :
+        id == P9_RLCREATE ? "RLCREATE" :
         id == P9_RUNLINKAT ? "RUNLINKAT" :
         id == P9_RFLUSH ? "RFLUSH" :
         id == P9_RREADDIR ? "READDIR" :
@@ -694,6 +695,44 @@ static void v9fs_rmkdir(P9Req *req, v9fs_qid *qid)
     v9fs_req_free(req);
 }
 
+/* size[4] Tlcreate tag[2] fid[4] name[s] flags[4] mode[4] gid[4] */
+static P9Req *v9fs_tlcreate(QVirtio9P *v9p, uint32_t fid, const char *name,
+                            uint32_t flags, uint32_t mode, uint32_t gid,
+                            uint16_t tag)
+{
+    P9Req *req;
+
+    uint32_t body_size = 4 + 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_TLCREATE, tag);
+    v9fs_uint32_write(req, fid);
+    v9fs_string_write(req, name);
+    v9fs_uint32_write(req, flags);
+    v9fs_uint32_write(req, mode);
+    v9fs_uint32_write(req, gid);
+    v9fs_req_send(req);
+    return req;
+}
+
+/* size[4] Rlcreate tag[2] qid[13] iounit[4] */
+static void v9fs_rlcreate(P9Req *req, v9fs_qid *qid, uint32_t *iounit)
+{
+    v9fs_req_recv(req, P9_RLCREATE);
+    if (qid) {
+        v9fs_memread(req, qid, 13);
+    } else {
+        v9fs_memskip(req, 13);
+    }
+    if (iounit) {
+        v9fs_uint32_read(req, iounit);
+    }
+    v9fs_req_free(req);
+}
+
 /* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */
 static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char *name,
                              uint32_t flags, uint16_t tag)
@@ -1032,6 +1071,24 @@ static void do_mkdir(QVirtio9P *v9p, const char *path, const char *cname)
     g_free(name);
 }
 
+/* create a regular file with Tlcreate and return file's fid */
+static uint32_t do_lcreate(QVirtio9P *v9p, const char *path,
+                           const char *cname)
+{
+    char *const name = g_strdup(cname);
+    uint32_t fid;
+    P9Req *req;
+
+    fid = do_walk(v9p, path);
+
+    req = v9fs_tlcreate(v9p, fid, name, 0, 0750, 0, 0);
+    v9fs_req_wait_for_reply(req, NULL);
+    v9fs_rlcreate(req, NULL, NULL);
+
+    g_free(name);
+    return fid;
+}
+
 static void do_unlinkat(QVirtio9P *v9p, const char *atpath, const char *rpath,
                         uint32_t flags)
 {
@@ -1120,6 +1177,25 @@ static void fs_unlinkat_dir(void *obj, void *data, QGuestAllocator *t_alloc)
     g_free(root_path);
 }
 
+static void fs_create_file(void *obj, void *data, QGuestAllocator *t_alloc)
+{
+    QVirtio9P *v9p = obj;
+    alloc = t_alloc;
+    struct stat st;
+    char *new_file = virtio_9p_test_path("03/1st_file");
+
+    do_attach(v9p);
+    do_mkdir(v9p, "/", "03");
+    do_lcreate(v9p, "03", "1st_file");
+
+    /* check if created file exists now ... */
+    g_assert(stat(new_file, &st) == 0);
+    /* ... and is a regular file */
+    g_assert((st.st_mode & S_IFMT) == S_IFREG);
+
+    g_free(new_file);
+}
+
 static void *assign_9p_local_driver(GString *cmd_line, void *arg)
 {
     virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1161,6 +1237,7 @@ static void register_virtio_9p_test(void)
     qos_add_test("local/config", "virtio-9p", pci_config,  &opts);
     qos_add_test("local/create_dir", "virtio-9p", fs_create_dir, &opts);
     qos_add_test("local/unlinkat_dir", "virtio-9p", fs_unlinkat_dir, &opts);
+    qos_add_test("local/create_file", "virtio-9p", fs_create_file, &opts);
 }
 
 libqos_init(register_virtio_9p_test);
-- 
2.20.1

Re: [PATCH v2 3/8] tests/9pfs: add local Tlcreate test

[Greg Kurz]

On Wed, 21 Oct 2020 14:25:33 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> This test case uses a Tlcreate 9p request to create a regular file inside
> host's test directory.
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---

Just one remark, see below.

Reviewed-by: Greg Kurz <groug@kaod.org>

>  tests/qtest/virtio-9p-test.c | 77 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
> 
> diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> index abd7e44648..c030bc2a6c 100644
> --- a/tests/qtest/virtio-9p-test.c
> +++ b/tests/qtest/virtio-9p-test.c
> @@ -258,6 +258,7 @@ static const char *rmessage_name(uint8_t id)
>          id == P9_RLOPEN ? "RLOPEN" :
>          id == P9_RWRITE ? "RWRITE" :
>          id == P9_RMKDIR ? "RMKDIR" :
> +        id == P9_RLCREATE ? "RLCREATE" :
>          id == P9_RUNLINKAT ? "RUNLINKAT" :
>          id == P9_RFLUSH ? "RFLUSH" :
>          id == P9_RREADDIR ? "READDIR" :
> @@ -694,6 +695,44 @@ static void v9fs_rmkdir(P9Req *req, v9fs_qid *qid)
>      v9fs_req_free(req);
>  }
>  
> +/* size[4] Tlcreate tag[2] fid[4] name[s] flags[4] mode[4] gid[4] */
> +static P9Req *v9fs_tlcreate(QVirtio9P *v9p, uint32_t fid, const char *name,
> +                            uint32_t flags, uint32_t mode, uint32_t gid,
> +                            uint16_t tag)
> +{
> +    P9Req *req;
> +
> +    uint32_t body_size = 4 + 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_TLCREATE, tag);
> +    v9fs_uint32_write(req, fid);
> +    v9fs_string_write(req, name);
> +    v9fs_uint32_write(req, flags);
> +    v9fs_uint32_write(req, mode);
> +    v9fs_uint32_write(req, gid);
> +    v9fs_req_send(req);
> +    return req;
> +}
> +
> +/* size[4] Rlcreate tag[2] qid[13] iounit[4] */
> +static void v9fs_rlcreate(P9Req *req, v9fs_qid *qid, uint32_t *iounit)
> +{
> +    v9fs_req_recv(req, P9_RLCREATE);
> +    if (qid) {
> +        v9fs_memread(req, qid, 13);
> +    } else {
> +        v9fs_memskip(req, 13);
> +    }
> +    if (iounit) {
> +        v9fs_uint32_read(req, iounit);
> +    }
> +    v9fs_req_free(req);
> +}
> +
>  /* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */
>  static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char *name,
>                               uint32_t flags, uint16_t tag)
> @@ -1032,6 +1071,24 @@ static void do_mkdir(QVirtio9P *v9p, const char *path, const char *cname)
>      g_free(name);
>  }
>  
> +/* create a regular file with Tlcreate and return file's fid */
> +static uint32_t do_lcreate(QVirtio9P *v9p, const char *path,
> +                           const char *cname)
> +{
> +    char *const name = g_strdup(cname);
> +    uint32_t fid;
> +    P9Req *req;
> +
> +    fid = do_walk(v9p, path);
> +
> +    req = v9fs_tlcreate(v9p, fid, name, 0, 0750, 0, 0);

Maybe it could make sense to make the mode a parameter of
do_lcreate() in case someone wants to write a test case
around that ? Same remark applies to do_mkdir() actually.

No need to change this now anyway.

> +    v9fs_req_wait_for_reply(req, NULL);
> +    v9fs_rlcreate(req, NULL, NULL);
> +
> +    g_free(name);
> +    return fid;
> +}
> +
>  static void do_unlinkat(QVirtio9P *v9p, const char *atpath, const char *rpath,
>                          uint32_t flags)
>  {
> @@ -1120,6 +1177,25 @@ static void fs_unlinkat_dir(void *obj, void *data, QGuestAllocator *t_alloc)
>      g_free(root_path);
>  }
>  
> +static void fs_create_file(void *obj, void *data, QGuestAllocator *t_alloc)
> +{
> +    QVirtio9P *v9p = obj;
> +    alloc = t_alloc;
> +    struct stat st;
> +    char *new_file = virtio_9p_test_path("03/1st_file");
> +
> +    do_attach(v9p);
> +    do_mkdir(v9p, "/", "03");
> +    do_lcreate(v9p, "03", "1st_file");
> +
> +    /* check if created file exists now ... */
> +    g_assert(stat(new_file, &st) == 0);
> +    /* ... and is a regular file */
> +    g_assert((st.st_mode & S_IFMT) == S_IFREG);
> +
> +    g_free(new_file);
> +}
> +
>  static void *assign_9p_local_driver(GString *cmd_line, void *arg)
>  {
>      virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
> @@ -1161,6 +1237,7 @@ static void register_virtio_9p_test(void)
>      qos_add_test("local/config", "virtio-9p", pci_config,  &opts);
>      qos_add_test("local/create_dir", "virtio-9p", fs_create_dir, &opts);
>      qos_add_test("local/unlinkat_dir", "virtio-9p", fs_unlinkat_dir, &opts);
> +    qos_add_test("local/create_file", "virtio-9p", fs_create_file, &opts);
>  }
>  
>  libqos_init(register_virtio_9p_test);

Re: [PATCH v2 3/8] tests/9pfs: add local Tlcreate test

[Christian Schoenebeck]

On Donnerstag, 22. Oktober 2020 10:51:46 CEST Greg Kurz wrote:
> On Wed, 21 Oct 2020 14:25:33 +0200
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > This test case uses a Tlcreate 9p request to create a regular file inside
> > host's test directory.
> > 
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> 
> Just one remark, see below.
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> >  tests/qtest/virtio-9p-test.c | 77 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 77 insertions(+)
> > 
> > diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> > index abd7e44648..c030bc2a6c 100644
> > --- a/tests/qtest/virtio-9p-test.c
> > +++ b/tests/qtest/virtio-9p-test.c
> > @@ -258,6 +258,7 @@ static const char *rmessage_name(uint8_t id)
> > 
> >          id == P9_RLOPEN ? "RLOPEN" :
> >          id == P9_RWRITE ? "RWRITE" :
> > 
> >          id == P9_RMKDIR ? "RMKDIR" :
> > +        id == P9_RLCREATE ? "RLCREATE" :
> >          id == P9_RUNLINKAT ? "RUNLINKAT" :
> >          id == P9_RFLUSH ? "RFLUSH" :
> > 
> >          id == P9_RREADDIR ? "READDIR" :
> > @@ -694,6 +695,44 @@ static void v9fs_rmkdir(P9Req *req, v9fs_qid *qid)
> > 
> >      v9fs_req_free(req);
> >  
> >  }
> > 
> > +/* size[4] Tlcreate tag[2] fid[4] name[s] flags[4] mode[4] gid[4] */
> > +static P9Req *v9fs_tlcreate(QVirtio9P *v9p, uint32_t fid, const char
> > *name, +                            uint32_t flags, uint32_t mode,
> > uint32_t gid, +                            uint16_t tag)
> > +{
> > +    P9Req *req;
> > +
> > +    uint32_t body_size = 4 + 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_TLCREATE, tag);
> > +    v9fs_uint32_write(req, fid);
> > +    v9fs_string_write(req, name);
> > +    v9fs_uint32_write(req, flags);
> > +    v9fs_uint32_write(req, mode);
> > +    v9fs_uint32_write(req, gid);
> > +    v9fs_req_send(req);
> > +    return req;
> > +}
> > +
> > +/* size[4] Rlcreate tag[2] qid[13] iounit[4] */
> > +static void v9fs_rlcreate(P9Req *req, v9fs_qid *qid, uint32_t *iounit)
> > +{
> > +    v9fs_req_recv(req, P9_RLCREATE);
> > +    if (qid) {
> > +        v9fs_memread(req, qid, 13);
> > +    } else {
> > +        v9fs_memskip(req, 13);
> > +    }
> > +    if (iounit) {
> > +        v9fs_uint32_read(req, iounit);
> > +    }
> > +    v9fs_req_free(req);
> > +}
> > +
> > 
> >  /* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */
> >  static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char
> >  *name,>  
> >                               uint32_t flags, uint16_t tag)
> > 
> > @@ -1032,6 +1071,24 @@ static void do_mkdir(QVirtio9P *v9p, const char
> > *path, const char *cname)> 
> >      g_free(name);
> >  
> >  }
> > 
> > +/* create a regular file with Tlcreate and return file's fid */
> > +static uint32_t do_lcreate(QVirtio9P *v9p, const char *path,
> > +                           const char *cname)
> > +{
> > +    char *const name = g_strdup(cname);
> > +    uint32_t fid;
> > +    P9Req *req;
> > +
> > +    fid = do_walk(v9p, path);
> > +
> > +    req = v9fs_tlcreate(v9p, fid, name, 0, 0750, 0, 0);
> 
> Maybe it could make sense to make the mode a parameter of
> do_lcreate() in case someone wants to write a test case
> around that ? Same remark applies to do_mkdir() actually.
> 
> No need to change this now anyway.

Yeah, I thought about that for a moment, but decided if that's really needed 
one day, it'll be a trivial change, especially under the assumption that there 
will be probably not a lot of code using this function, even on the long-term.

Thanks for looking at these patches!

Best regards,
Christian Schoenebeck

[PATCH v2 4/8] tests/9pfs: add local Tunlinkat file test

[Christian Schoenebeck]

This test case uses a Tunlinkat request to remove a regular file using
the 9pfs 'local' fs driver.

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

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index c030bc2a6c..6b74a1fd7e 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -1196,6 +1196,29 @@ static void fs_create_file(void *obj, void *data, QGuestAllocator *t_alloc)
     g_free(new_file);
 }
 
+static void fs_unlinkat_file(void *obj, void *data, QGuestAllocator *t_alloc)
+{
+    QVirtio9P *v9p = obj;
+    alloc = t_alloc;
+    struct stat st;
+    char *new_file = virtio_9p_test_path("04/doa_file");
+
+    do_attach(v9p);
+    do_mkdir(v9p, "/", "04");
+    do_lcreate(v9p, "04", "doa_file");
+
+    /* check if created file exists now ... */
+    g_assert(stat(new_file, &st) == 0);
+    /* ... and is a regular file */
+    g_assert((st.st_mode & S_IFMT) == S_IFREG);
+
+    do_unlinkat(v9p, "04", "doa_file", 0);
+    /* file should be gone now */
+    g_assert(stat(new_file, &st) != 0);
+
+    g_free(new_file);
+}
+
 static void *assign_9p_local_driver(GString *cmd_line, void *arg)
 {
     virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1238,6 +1261,7 @@ static void register_virtio_9p_test(void)
     qos_add_test("local/create_dir", "virtio-9p", fs_create_dir, &opts);
     qos_add_test("local/unlinkat_dir", "virtio-9p", fs_unlinkat_dir, &opts);
     qos_add_test("local/create_file", "virtio-9p", fs_create_file, &opts);
+    qos_add_test("local/unlinkat_file", "virtio-9p", fs_unlinkat_file, &opts);
 }
 
 libqos_init(register_virtio_9p_test);
-- 
2.20.1

Re: [PATCH v2 4/8] tests/9pfs: add local Tunlinkat file test

[Greg Kurz]

On Wed, 21 Oct 2020 14:28:37 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> This test case uses a Tunlinkat request to remove a regular file using
> the 9pfs 'local' fs driver.
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  tests/qtest/virtio-9p-test.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> index c030bc2a6c..6b74a1fd7e 100644
> --- a/tests/qtest/virtio-9p-test.c
> +++ b/tests/qtest/virtio-9p-test.c
> @@ -1196,6 +1196,29 @@ static void fs_create_file(void *obj, void *data, QGuestAllocator *t_alloc)
>      g_free(new_file);
>  }
>  
> +static void fs_unlinkat_file(void *obj, void *data, QGuestAllocator *t_alloc)
> +{
> +    QVirtio9P *v9p = obj;
> +    alloc = t_alloc;
> +    struct stat st;
> +    char *new_file = virtio_9p_test_path("04/doa_file");
> +
> +    do_attach(v9p);
> +    do_mkdir(v9p, "/", "04");
> +    do_lcreate(v9p, "04", "doa_file");
> +
> +    /* check if created file exists now ... */
> +    g_assert(stat(new_file, &st) == 0);
> +    /* ... and is a regular file */
> +    g_assert((st.st_mode & S_IFMT) == S_IFREG);
> +
> +    do_unlinkat(v9p, "04", "doa_file", 0);
> +    /* file should be gone now */
> +    g_assert(stat(new_file, &st) != 0);
> +
> +    g_free(new_file);
> +}
> +
>  static void *assign_9p_local_driver(GString *cmd_line, void *arg)
>  {
>      virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
> @@ -1238,6 +1261,7 @@ static void register_virtio_9p_test(void)
>      qos_add_test("local/create_dir", "virtio-9p", fs_create_dir, &opts);
>      qos_add_test("local/unlinkat_dir", "virtio-9p", fs_unlinkat_dir, &opts);
>      qos_add_test("local/create_file", "virtio-9p", fs_create_file, &opts);
> +    qos_add_test("local/unlinkat_file", "virtio-9p", fs_unlinkat_file, &opts);
>  }
>  
>  libqos_init(register_virtio_9p_test);

[PATCH v2 5/8] tests/9pfs: add local Tsymlink test

[Christian Schoenebeck]

This test case uses a Tsymlink 9p request to create a symbolic link using
the 9pfs 'local' fs driver.

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

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 6b74a1fd7e..0c11417236 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -259,6 +259,7 @@ static const char *rmessage_name(uint8_t id)
         id == P9_RWRITE ? "RWRITE" :
         id == P9_RMKDIR ? "RMKDIR" :
         id == P9_RLCREATE ? "RLCREATE" :
+        id == P9_RSYMLINK ? "RSYMLINK" :
         id == P9_RUNLINKAT ? "RUNLINKAT" :
         id == P9_RFLUSH ? "RFLUSH" :
         id == P9_RREADDIR ? "READDIR" :
@@ -733,6 +734,39 @@ static void v9fs_rlcreate(P9Req *req, v9fs_qid *qid, uint32_t *iounit)
     v9fs_req_free(req);
 }
 
+/* size[4] Tsymlink tag[2] fid[4] name[s] symtgt[s] gid[4] */
+static P9Req *v9fs_tsymlink(QVirtio9P *v9p, uint32_t fid, const char *name,
+                            const char *symtgt, uint32_t gid, uint16_t tag)
+{
+    P9Req *req;
+
+    uint32_t body_size = 4 + 4;
+    uint16_t string_size = v9fs_string_size(name) + v9fs_string_size(symtgt);
+
+    g_assert_cmpint(body_size, <=, UINT32_MAX - string_size);
+    body_size += string_size;
+
+    req = v9fs_req_init(v9p, body_size, P9_TSYMLINK, tag);
+    v9fs_uint32_write(req, fid);
+    v9fs_string_write(req, name);
+    v9fs_string_write(req, symtgt);
+    v9fs_uint32_write(req, gid);
+    v9fs_req_send(req);
+    return req;
+}
+
+/* size[4] Rsymlink tag[2] qid[13] */
+static void v9fs_rsymlink(P9Req *req, v9fs_qid *qid)
+{
+    v9fs_req_recv(req, P9_RSYMLINK);
+    if (qid) {
+        v9fs_memread(req, qid, 13);
+    } else {
+        v9fs_memskip(req, 13);
+    }
+    v9fs_req_free(req);
+}
+
 /* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */
 static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char *name,
                              uint32_t flags, uint16_t tag)
@@ -1089,6 +1123,25 @@ static uint32_t do_lcreate(QVirtio9P *v9p, const char *path,
     return fid;
 }
 
+/* create symlink named @a clink in directory @a path pointing to @a to */
+static void do_symlink(QVirtio9P *v9p, const char *path, const char *clink,
+                       const char *to)
+{
+    char *const name = g_strdup(clink);
+    char *const dst = g_strdup(to);
+    uint32_t fid;
+    P9Req *req;
+
+    fid = do_walk(v9p, path);
+
+    req = v9fs_tsymlink(v9p, fid, name, dst, 0, 0);
+    v9fs_req_wait_for_reply(req, NULL);
+    v9fs_rsymlink(req, NULL);
+
+    g_free(dst);
+    g_free(name);
+}
+
 static void do_unlinkat(QVirtio9P *v9p, const char *atpath, const char *rpath,
                         uint32_t flags)
 {
@@ -1219,6 +1272,29 @@ static void fs_unlinkat_file(void *obj, void *data, QGuestAllocator *t_alloc)
     g_free(new_file);
 }
 
+static void fs_symlink_file(void *obj, void *data, QGuestAllocator *t_alloc)
+{
+    QVirtio9P *v9p = obj;
+    alloc = t_alloc;
+    struct stat st;
+    char *real_file = virtio_9p_test_path("05/real_file");
+    char *symlink_file = virtio_9p_test_path("05/symlink_file");
+
+    do_attach(v9p);
+    do_mkdir(v9p, "/", "05");
+    do_lcreate(v9p, "05", "real_file");
+    g_assert(stat(real_file, &st) == 0);
+    g_assert((st.st_mode & S_IFMT) == S_IFREG);
+
+    do_symlink(v9p, "05", "symlink_file", "real_file");
+
+    /* check if created link exists now */
+    g_assert(stat(symlink_file, &st) == 0);
+
+    g_free(symlink_file);
+    g_free(real_file);
+}
+
 static void *assign_9p_local_driver(GString *cmd_line, void *arg)
 {
     virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1262,6 +1338,7 @@ static void register_virtio_9p_test(void)
     qos_add_test("local/unlinkat_dir", "virtio-9p", fs_unlinkat_dir, &opts);
     qos_add_test("local/create_file", "virtio-9p", fs_create_file, &opts);
     qos_add_test("local/unlinkat_file", "virtio-9p", fs_unlinkat_file, &opts);
+    qos_add_test("local/symlink_file", "virtio-9p", fs_symlink_file, &opts);
 }
 
 libqos_init(register_virtio_9p_test);
-- 
2.20.1

Re: [PATCH v2 5/8] tests/9pfs: add local Tsymlink test

[Greg Kurz]

On Wed, 21 Oct 2020 14:33:34 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> This test case uses a Tsymlink 9p request to create a symbolic link using
> the 9pfs 'local' fs driver.
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  tests/qtest/virtio-9p-test.c | 77 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
> 
> diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> index 6b74a1fd7e..0c11417236 100644
> --- a/tests/qtest/virtio-9p-test.c
> +++ b/tests/qtest/virtio-9p-test.c
> @@ -259,6 +259,7 @@ static const char *rmessage_name(uint8_t id)
>          id == P9_RWRITE ? "RWRITE" :
>          id == P9_RMKDIR ? "RMKDIR" :
>          id == P9_RLCREATE ? "RLCREATE" :
> +        id == P9_RSYMLINK ? "RSYMLINK" :
>          id == P9_RUNLINKAT ? "RUNLINKAT" :
>          id == P9_RFLUSH ? "RFLUSH" :
>          id == P9_RREADDIR ? "READDIR" :
> @@ -733,6 +734,39 @@ static void v9fs_rlcreate(P9Req *req, v9fs_qid *qid, uint32_t *iounit)
>      v9fs_req_free(req);
>  }
>  
> +/* size[4] Tsymlink tag[2] fid[4] name[s] symtgt[s] gid[4] */
> +static P9Req *v9fs_tsymlink(QVirtio9P *v9p, uint32_t fid, const char *name,
> +                            const char *symtgt, uint32_t gid, uint16_t tag)
> +{
> +    P9Req *req;
> +
> +    uint32_t body_size = 4 + 4;
> +    uint16_t string_size = v9fs_string_size(name) + v9fs_string_size(symtgt);
> +
> +    g_assert_cmpint(body_size, <=, UINT32_MAX - string_size);
> +    body_size += string_size;
> +
> +    req = v9fs_req_init(v9p, body_size, P9_TSYMLINK, tag);
> +    v9fs_uint32_write(req, fid);
> +    v9fs_string_write(req, name);
> +    v9fs_string_write(req, symtgt);
> +    v9fs_uint32_write(req, gid);
> +    v9fs_req_send(req);
> +    return req;
> +}
> +
> +/* size[4] Rsymlink tag[2] qid[13] */
> +static void v9fs_rsymlink(P9Req *req, v9fs_qid *qid)
> +{
> +    v9fs_req_recv(req, P9_RSYMLINK);
> +    if (qid) {
> +        v9fs_memread(req, qid, 13);
> +    } else {
> +        v9fs_memskip(req, 13);
> +    }
> +    v9fs_req_free(req);
> +}
> +
>  /* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */
>  static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char *name,
>                               uint32_t flags, uint16_t tag)
> @@ -1089,6 +1123,25 @@ static uint32_t do_lcreate(QVirtio9P *v9p, const char *path,
>      return fid;
>  }
>  
> +/* create symlink named @a clink in directory @a path pointing to @a to */
> +static void do_symlink(QVirtio9P *v9p, const char *path, const char *clink,
> +                       const char *to)
> +{
> +    char *const name = g_strdup(clink);
> +    char *const dst = g_strdup(to);
> +    uint32_t fid;
> +    P9Req *req;
> +
> +    fid = do_walk(v9p, path);
> +
> +    req = v9fs_tsymlink(v9p, fid, name, dst, 0, 0);
> +    v9fs_req_wait_for_reply(req, NULL);
> +    v9fs_rsymlink(req, NULL);
> +
> +    g_free(dst);
> +    g_free(name);
> +}
> +
>  static void do_unlinkat(QVirtio9P *v9p, const char *atpath, const char *rpath,
>                          uint32_t flags)
>  {
> @@ -1219,6 +1272,29 @@ static void fs_unlinkat_file(void *obj, void *data, QGuestAllocator *t_alloc)
>      g_free(new_file);
>  }
>  
> +static void fs_symlink_file(void *obj, void *data, QGuestAllocator *t_alloc)
> +{
> +    QVirtio9P *v9p = obj;
> +    alloc = t_alloc;
> +    struct stat st;
> +    char *real_file = virtio_9p_test_path("05/real_file");
> +    char *symlink_file = virtio_9p_test_path("05/symlink_file");
> +
> +    do_attach(v9p);
> +    do_mkdir(v9p, "/", "05");
> +    do_lcreate(v9p, "05", "real_file");
> +    g_assert(stat(real_file, &st) == 0);
> +    g_assert((st.st_mode & S_IFMT) == S_IFREG);
> +
> +    do_symlink(v9p, "05", "symlink_file", "real_file");
> +
> +    /* check if created link exists now */
> +    g_assert(stat(symlink_file, &st) == 0);
> +
> +    g_free(symlink_file);
> +    g_free(real_file);
> +}
> +
>  static void *assign_9p_local_driver(GString *cmd_line, void *arg)
>  {
>      virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
> @@ -1262,6 +1338,7 @@ static void register_virtio_9p_test(void)
>      qos_add_test("local/unlinkat_dir", "virtio-9p", fs_unlinkat_dir, &opts);
>      qos_add_test("local/create_file", "virtio-9p", fs_create_file, &opts);
>      qos_add_test("local/unlinkat_file", "virtio-9p", fs_unlinkat_file, &opts);
> +    qos_add_test("local/symlink_file", "virtio-9p", fs_symlink_file, &opts);
>  }
>  
>  libqos_init(register_virtio_9p_test);

[PATCH v2 6/8] tests/9pfs: add local Tunlinkat symlink test

[Christian Schoenebeck]

This test case uses a Tunlinkat request to remove a symlink using
the 9pfs 'local' fs driver.

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

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 0c11417236..33cba24b18 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -1295,6 +1295,32 @@ static void fs_symlink_file(void *obj, void *data, QGuestAllocator *t_alloc)
     g_free(real_file);
 }
 
+static void fs_unlinkat_symlink(void *obj, void *data,
+                                QGuestAllocator *t_alloc)
+{
+    QVirtio9P *v9p = obj;
+    alloc = t_alloc;
+    struct stat st;
+    char *real_file = virtio_9p_test_path("06/real_file");
+    char *symlink_file = virtio_9p_test_path("06/symlink_file");
+
+    do_attach(v9p);
+    do_mkdir(v9p, "/", "06");
+    do_lcreate(v9p, "06", "real_file");
+    g_assert(stat(real_file, &st) == 0);
+    g_assert((st.st_mode & S_IFMT) == S_IFREG);
+
+    do_symlink(v9p, "06", "symlink_file", "real_file");
+    g_assert(stat(symlink_file, &st) == 0);
+
+    do_unlinkat(v9p, "06", "symlink_file", 0);
+    /* symlink should be gone now */
+    g_assert(stat(symlink_file, &st) != 0);
+
+    g_free(symlink_file);
+    g_free(real_file);
+}
+
 static void *assign_9p_local_driver(GString *cmd_line, void *arg)
 {
     virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1339,6 +1365,8 @@ static void register_virtio_9p_test(void)
     qos_add_test("local/create_file", "virtio-9p", fs_create_file, &opts);
     qos_add_test("local/unlinkat_file", "virtio-9p", fs_unlinkat_file, &opts);
     qos_add_test("local/symlink_file", "virtio-9p", fs_symlink_file, &opts);
+    qos_add_test("local/unlinkat_symlink", "virtio-9p", fs_unlinkat_symlink,
+                 &opts);
 }
 
 libqos_init(register_virtio_9p_test);
-- 
2.20.1

Re: [PATCH v2 6/8] tests/9pfs: add local Tunlinkat symlink test

[Greg Kurz]

On Wed, 21 Oct 2020 14:36:23 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> This test case uses a Tunlinkat request to remove a symlink using
> the 9pfs 'local' fs driver.
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  tests/qtest/virtio-9p-test.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> index 0c11417236..33cba24b18 100644
> --- a/tests/qtest/virtio-9p-test.c
> +++ b/tests/qtest/virtio-9p-test.c
> @@ -1295,6 +1295,32 @@ static void fs_symlink_file(void *obj, void *data, QGuestAllocator *t_alloc)
>      g_free(real_file);
>  }
>  
> +static void fs_unlinkat_symlink(void *obj, void *data,
> +                                QGuestAllocator *t_alloc)
> +{
> +    QVirtio9P *v9p = obj;
> +    alloc = t_alloc;
> +    struct stat st;
> +    char *real_file = virtio_9p_test_path("06/real_file");
> +    char *symlink_file = virtio_9p_test_path("06/symlink_file");
> +
> +    do_attach(v9p);
> +    do_mkdir(v9p, "/", "06");
> +    do_lcreate(v9p, "06", "real_file");
> +    g_assert(stat(real_file, &st) == 0);
> +    g_assert((st.st_mode & S_IFMT) == S_IFREG);
> +
> +    do_symlink(v9p, "06", "symlink_file", "real_file");
> +    g_assert(stat(symlink_file, &st) == 0);
> +
> +    do_unlinkat(v9p, "06", "symlink_file", 0);
> +    /* symlink should be gone now */
> +    g_assert(stat(symlink_file, &st) != 0);
> +
> +    g_free(symlink_file);
> +    g_free(real_file);
> +}
> +
>  static void *assign_9p_local_driver(GString *cmd_line, void *arg)
>  {
>      virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
> @@ -1339,6 +1365,8 @@ static void register_virtio_9p_test(void)
>      qos_add_test("local/create_file", "virtio-9p", fs_create_file, &opts);
>      qos_add_test("local/unlinkat_file", "virtio-9p", fs_unlinkat_file, &opts);
>      qos_add_test("local/symlink_file", "virtio-9p", fs_symlink_file, &opts);
> +    qos_add_test("local/unlinkat_symlink", "virtio-9p", fs_unlinkat_symlink,
> +                 &opts);
>  }
>  
>  libqos_init(register_virtio_9p_test);

[PATCH v2 7/8] tests/9pfs: add local Tlink test

[Christian Schoenebeck]

This test case uses a Tlink request to create a hard link to a regular
file using the 9pfs 'local' fs driver.

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

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 33cba24b18..460fa49fe3 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -260,6 +260,7 @@ static const char *rmessage_name(uint8_t id)
         id == P9_RMKDIR ? "RMKDIR" :
         id == P9_RLCREATE ? "RLCREATE" :
         id == P9_RSYMLINK ? "RSYMLINK" :
+        id == P9_RLINK ? "RLINK" :
         id == P9_RUNLINKAT ? "RUNLINKAT" :
         id == P9_RFLUSH ? "RFLUSH" :
         id == P9_RREADDIR ? "READDIR" :
@@ -767,6 +768,33 @@ static void v9fs_rsymlink(P9Req *req, v9fs_qid *qid)
     v9fs_req_free(req);
 }
 
+/* size[4] Tlink tag[2] dfid[4] fid[4] name[s] */
+static P9Req *v9fs_tlink(QVirtio9P *v9p, uint32_t dfid, uint32_t fid,
+                         const char *name, uint16_t tag)
+{
+    P9Req *req;
+
+    uint32_t body_size = 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_TLINK, tag);
+    v9fs_uint32_write(req, dfid);
+    v9fs_uint32_write(req, fid);
+    v9fs_string_write(req, name);
+    v9fs_req_send(req);
+    return req;
+}
+
+/* size[4] Rlink tag[2] */
+static void v9fs_rlink(P9Req *req)
+{
+    v9fs_req_recv(req, P9_RLINK);
+    v9fs_req_free(req);
+}
+
 /* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */
 static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char *name,
                              uint32_t flags, uint16_t tag)
@@ -1142,6 +1170,21 @@ static void do_symlink(QVirtio9P *v9p, const char *path, const char *clink,
     g_free(name);
 }
 
+/* create a hard link named @a clink in directory @a path pointing to @a to */
+static void do_hardlink(QVirtio9P *v9p, const char *path, const char *clink,
+                        const char *to)
+{
+    uint32_t dfid, fid;
+    P9Req *req;
+
+    dfid = do_walk(v9p, path);
+    fid = do_walk(v9p, to);
+
+    req = v9fs_tlink(v9p, dfid, fid, clink, 0);
+    v9fs_req_wait_for_reply(req, NULL);
+    v9fs_rlink(req);
+}
+
 static void do_unlinkat(QVirtio9P *v9p, const char *atpath, const char *rpath,
                         uint32_t flags)
 {
@@ -1321,6 +1364,33 @@ static void fs_unlinkat_symlink(void *obj, void *data,
     g_free(real_file);
 }
 
+static void fs_hardlink_file(void *obj, void *data, QGuestAllocator *t_alloc)
+{
+    QVirtio9P *v9p = obj;
+    alloc = t_alloc;
+    struct stat st_real, st_link;
+    char *real_file = virtio_9p_test_path("07/real_file");
+    char *hardlink_file = virtio_9p_test_path("07/hardlink_file");
+
+    do_attach(v9p);
+    do_mkdir(v9p, "/", "07");
+    do_lcreate(v9p, "07", "real_file");
+    g_assert(stat(real_file, &st_real) == 0);
+    g_assert((st_real.st_mode & S_IFMT) == S_IFREG);
+
+    do_hardlink(v9p, "07", "hardlink_file", "07/real_file");
+
+    /* check if link exists now ... */
+    g_assert(stat(hardlink_file, &st_link) == 0);
+    /* ... and it's a hard link, right? */
+    g_assert((st_link.st_mode & S_IFMT) == S_IFREG);
+    g_assert(st_link.st_dev == st_real.st_dev);
+    g_assert(st_link.st_ino == st_real.st_ino);
+
+    g_free(hardlink_file);
+    g_free(real_file);
+}
+
 static void *assign_9p_local_driver(GString *cmd_line, void *arg)
 {
     virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1367,6 +1437,7 @@ static void register_virtio_9p_test(void)
     qos_add_test("local/symlink_file", "virtio-9p", fs_symlink_file, &opts);
     qos_add_test("local/unlinkat_symlink", "virtio-9p", fs_unlinkat_symlink,
                  &opts);
+    qos_add_test("local/hardlink_file", "virtio-9p", fs_hardlink_file, &opts);
 }
 
 libqos_init(register_virtio_9p_test);
-- 
2.20.1

Re: [PATCH v2 7/8] tests/9pfs: add local Tlink test

[Christian Schoenebeck]

On Mittwoch, 21. Oktober 2020 14:51:09 CEST Christian Schoenebeck wrote:
> This test case uses a Tlink request to create a hard link to a regular
> file using the 9pfs 'local' fs driver.
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  tests/qtest/virtio-9p-test.c | 71 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 71 insertions(+)
> 
> diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> index 33cba24b18..460fa49fe3 100644
> --- a/tests/qtest/virtio-9p-test.c
> +++ b/tests/qtest/virtio-9p-test.c
> @@ -260,6 +260,7 @@ static const char *rmessage_name(uint8_t id)
>          id == P9_RMKDIR ? "RMKDIR" :
>          id == P9_RLCREATE ? "RLCREATE" :
>          id == P9_RSYMLINK ? "RSYMLINK" :
> +        id == P9_RLINK ? "RLINK" :
>          id == P9_RUNLINKAT ? "RUNLINKAT" :
>          id == P9_RFLUSH ? "RFLUSH" :
>          id == P9_RREADDIR ? "READDIR" :
> @@ -767,6 +768,33 @@ static void v9fs_rsymlink(P9Req *req, v9fs_qid *qid)
>      v9fs_req_free(req);
>  }
> 
> +/* size[4] Tlink tag[2] dfid[4] fid[4] name[s] */
> +static P9Req *v9fs_tlink(QVirtio9P *v9p, uint32_t dfid, uint32_t fid,
> +                         const char *name, uint16_t tag)
> +{
> +    P9Req *req;
> +
> +    uint32_t body_size = 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_TLINK, tag);
> +    v9fs_uint32_write(req, dfid);
> +    v9fs_uint32_write(req, fid);
> +    v9fs_string_write(req, name);
> +    v9fs_req_send(req);
> +    return req;
> +}
> +
> +/* size[4] Rlink tag[2] */
> +static void v9fs_rlink(P9Req *req)
> +{
> +    v9fs_req_recv(req, P9_RLINK);
> +    v9fs_req_free(req);
> +}
> +
>  /* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */
>  static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char
> *name, uint32_t flags, uint16_t tag)
> @@ -1142,6 +1170,21 @@ static void do_symlink(QVirtio9P *v9p, const char
> *path, const char *clink, g_free(name);
>  }
> 
> +/* create a hard link named @a clink in directory @a path pointing to @a to
> */ +static void do_hardlink(QVirtio9P *v9p, const char *path, const char
> *clink, +                        const char *to)
> +{
> +    uint32_t dfid, fid;
> +    P9Req *req;
> +
> +    dfid = do_walk(v9p, path);
> +    fid = do_walk(v9p, to);
> +
> +    req = v9fs_tlink(v9p, dfid, fid, clink, 0);
> +    v9fs_req_wait_for_reply(req, NULL);
> +    v9fs_rlink(req);
> +}
> +
>  static void do_unlinkat(QVirtio9P *v9p, const char *atpath, const char
> *rpath, uint32_t flags)
>  {
> @@ -1321,6 +1364,33 @@ static void fs_unlinkat_symlink(void *obj, void
> *data, g_free(real_file);
>  }
> 
> +static void fs_hardlink_file(void *obj, void *data, QGuestAllocator
> *t_alloc) +{
> +    QVirtio9P *v9p = obj;
> +    alloc = t_alloc;
> +    struct stat st_real, st_link;
> +    char *real_file = virtio_9p_test_path("07/real_file");
> +    char *hardlink_file = virtio_9p_test_path("07/hardlink_file");
> +
> +    do_attach(v9p);
> +    do_mkdir(v9p, "/", "07");
> +    do_lcreate(v9p, "07", "real_file");
> +    g_assert(stat(real_file, &st_real) == 0);
> +    g_assert((st_real.st_mode & S_IFMT) == S_IFREG);
> +
> +    do_hardlink(v9p, "07", "hardlink_file", "07/real_file");
> +
> +    /* check if link exists now ... */
> +    g_assert(stat(hardlink_file, &st_link) == 0);
> +    /* ... and it's a hard link, right? */
> +    g_assert((st_link.st_mode & S_IFMT) == S_IFREG);
> +    g_assert(st_link.st_dev == st_real.st_dev);
> +    g_assert(st_link.st_ino == st_real.st_ino);
> +
> +    g_free(hardlink_file);
> +    g_free(real_file);
> +}
> +
>  static void *assign_9p_local_driver(GString *cmd_line, void *arg)
>  {
>      virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
> @@ -1367,6 +1437,7 @@ static void register_virtio_9p_test(void)
>      qos_add_test("local/symlink_file", "virtio-9p", fs_symlink_file,
> &opts); qos_add_test("local/unlinkat_symlink", "virtio-9p",
> fs_unlinkat_symlink, &opts);
> +    qos_add_test("local/hardlink_file", "virtio-9p", fs_hardlink_file,
> &opts); }
> 
>  libqos_init(register_virtio_9p_test);

Ok, I found the problem on the mentioned box that failed to create hard links 
with 9p: it is libvirt auto generating AppArmor policy rules for 9p export 
pathes, which libvirt generates with "rw" AA (AppArmor) permissions. Once I 
manually adjusted the AA rule to "rwl", creating hard links worked again.

I guess I'll send a patch for libvirt to change that. At least IMO creating 
hard links with 9pfs is a very basic operation and should be enabled for the 
respective 9p export path by default.

Actually I think it should also include "k" which means "file locking", as 
file locking is also a fundamental operation with 9pfs, right?

Best regards,
Christian Schoenebeck

Re: [PATCH v2 7/8] tests/9pfs: add local Tlink test

[Greg Kurz]

On Wed, 21 Oct 2020 20:20:08 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Mittwoch, 21. Oktober 2020 14:51:09 CEST Christian Schoenebeck wrote:
> > This test case uses a Tlink request to create a hard link to a regular
> > file using the 9pfs 'local' fs driver.
> > 
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> >  tests/qtest/virtio-9p-test.c | 71 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 71 insertions(+)
> > 
> > diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> > index 33cba24b18..460fa49fe3 100644
> > --- a/tests/qtest/virtio-9p-test.c
> > +++ b/tests/qtest/virtio-9p-test.c
> > @@ -260,6 +260,7 @@ static const char *rmessage_name(uint8_t id)
> >          id == P9_RMKDIR ? "RMKDIR" :
> >          id == P9_RLCREATE ? "RLCREATE" :
> >          id == P9_RSYMLINK ? "RSYMLINK" :
> > +        id == P9_RLINK ? "RLINK" :
> >          id == P9_RUNLINKAT ? "RUNLINKAT" :
> >          id == P9_RFLUSH ? "RFLUSH" :
> >          id == P9_RREADDIR ? "READDIR" :
> > @@ -767,6 +768,33 @@ static void v9fs_rsymlink(P9Req *req, v9fs_qid *qid)
> >      v9fs_req_free(req);
> >  }
> > 
> > +/* size[4] Tlink tag[2] dfid[4] fid[4] name[s] */
> > +static P9Req *v9fs_tlink(QVirtio9P *v9p, uint32_t dfid, uint32_t fid,
> > +                         const char *name, uint16_t tag)
> > +{
> > +    P9Req *req;
> > +
> > +    uint32_t body_size = 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_TLINK, tag);
> > +    v9fs_uint32_write(req, dfid);
> > +    v9fs_uint32_write(req, fid);
> > +    v9fs_string_write(req, name);
> > +    v9fs_req_send(req);
> > +    return req;
> > +}
> > +
> > +/* size[4] Rlink tag[2] */
> > +static void v9fs_rlink(P9Req *req)
> > +{
> > +    v9fs_req_recv(req, P9_RLINK);
> > +    v9fs_req_free(req);
> > +}
> > +
> >  /* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */
> >  static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char
> > *name, uint32_t flags, uint16_t tag)
> > @@ -1142,6 +1170,21 @@ static void do_symlink(QVirtio9P *v9p, const char
> > *path, const char *clink, g_free(name);
> >  }
> > 
> > +/* create a hard link named @a clink in directory @a path pointing to @a to
> > */ +static void do_hardlink(QVirtio9P *v9p, const char *path, const char
> > *clink, +                        const char *to)
> > +{
> > +    uint32_t dfid, fid;
> > +    P9Req *req;
> > +
> > +    dfid = do_walk(v9p, path);
> > +    fid = do_walk(v9p, to);
> > +
> > +    req = v9fs_tlink(v9p, dfid, fid, clink, 0);
> > +    v9fs_req_wait_for_reply(req, NULL);
> > +    v9fs_rlink(req);
> > +}
> > +
> >  static void do_unlinkat(QVirtio9P *v9p, const char *atpath, const char
> > *rpath, uint32_t flags)
> >  {
> > @@ -1321,6 +1364,33 @@ static void fs_unlinkat_symlink(void *obj, void
> > *data, g_free(real_file);
> >  }
> > 
> > +static void fs_hardlink_file(void *obj, void *data, QGuestAllocator
> > *t_alloc) +{
> > +    QVirtio9P *v9p = obj;
> > +    alloc = t_alloc;
> > +    struct stat st_real, st_link;
> > +    char *real_file = virtio_9p_test_path("07/real_file");
> > +    char *hardlink_file = virtio_9p_test_path("07/hardlink_file");
> > +
> > +    do_attach(v9p);
> > +    do_mkdir(v9p, "/", "07");
> > +    do_lcreate(v9p, "07", "real_file");
> > +    g_assert(stat(real_file, &st_real) == 0);
> > +    g_assert((st_real.st_mode & S_IFMT) == S_IFREG);
> > +
> > +    do_hardlink(v9p, "07", "hardlink_file", "07/real_file");
> > +
> > +    /* check if link exists now ... */
> > +    g_assert(stat(hardlink_file, &st_link) == 0);
> > +    /* ... and it's a hard link, right? */
> > +    g_assert((st_link.st_mode & S_IFMT) == S_IFREG);
> > +    g_assert(st_link.st_dev == st_real.st_dev);
> > +    g_assert(st_link.st_ino == st_real.st_ino);
> > +
> > +    g_free(hardlink_file);
> > +    g_free(real_file);
> > +}
> > +
> >  static void *assign_9p_local_driver(GString *cmd_line, void *arg)
> >  {
> >      virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
> > @@ -1367,6 +1437,7 @@ static void register_virtio_9p_test(void)
> >      qos_add_test("local/symlink_file", "virtio-9p", fs_symlink_file,
> > &opts); qos_add_test("local/unlinkat_symlink", "virtio-9p",
> > fs_unlinkat_symlink, &opts);
> > +    qos_add_test("local/hardlink_file", "virtio-9p", fs_hardlink_file,
> > &opts); }
> > 
> >  libqos_init(register_virtio_9p_test);
> 
> Ok, I found the problem on the mentioned box that failed to create hard links 
> with 9p: it is libvirt auto generating AppArmor policy rules for 9p export 
> pathes, which libvirt generates with "rw" AA (AppArmor) permissions. Once I 
> manually adjusted the AA rule to "rwl", creating hard links worked again.
> 
> I guess I'll send a patch for libvirt to change that. At least IMO creating 
> hard links with 9pfs is a very basic operation and should be enabled for the 
> respective 9p export path by default.
> 
> Actually I think it should also include "k" which means "file locking", as 
> file locking is also a fundamental operation with 9pfs, right?
> 

Well, I don't know exactly why libvirt is generating a strict AppArmor policy
but I'm not that surprised. If the user can _easily_ change the policy to
fit its needs, it's fine to have a "better safe than sorry" default.

> Best regards,
> Christian Schoenebeck
> 
> 

Re: [PATCH v2 7/8] tests/9pfs: add local Tlink test

[Christian Schoenebeck]

On Donnerstag, 22. Oktober 2020 11:07:49 CEST Greg Kurz wrote:
> > Ok, I found the problem on the mentioned box that failed to create hard
> > links with 9p: it is libvirt auto generating AppArmor policy rules for 9p
> > export pathes, which libvirt generates with "rw" AA (AppArmor)
> > permissions. Once I manually adjusted the AA rule to "rwl", creating hard
> > links worked again.
> > 
> > I guess I'll send a patch for libvirt to change that. At least IMO
> > creating
> > hard links with 9pfs is a very basic operation and should be enabled for
> > the respective 9p export path by default.
> > 
> > Actually I think it should also include "k" which means "file locking", as
> > file locking is also a fundamental operation with 9pfs, right?
> 
> Well, I don't know exactly why libvirt is generating a strict AppArmor
> policy but I'm not that surprised. If the user can _easily_ change the
> policy to fit its needs, it's fine to have a "better safe than sorry"
> default.

Alone identifying the root cause of this is not that easy I would say. And if
it takes me quite some time to find it, then I imagine that other people would
struggle with this even more.

A large portion of software, even scripts, rely on being able to create hard
links and locking files. Right now they typically error out on guest with no
helpful error message.

So you start looking into the logs, don't find something obvious, then strace
on guest side to find the most relevant failing syscall on guest side and see
it was probably link(). Then you have to check several security layers: file
permissions on guest, file permissions on host, effective UID of qemu process.
You try creating hard links directly on host with that UID, works. Next you
check is it qemu's seccomp? Is it host's SELinux? Is it AppArmor?

Even for an experienced sysadmin I doubt it'll be a matter of minutes to
resolve this issue. Now imagine a regular user who just wants to sandbox
something on a workstation.

Looking at libvirt git log, it seems this security policy exists more or less
since day one (SHA-1 29ea8a9b64aac60251d283f74d57690e4edb5a6b, Mar 9 2014).
And I don't see an explanation that would suggest a specific reason for
exactly "rw".

I think something has to be improved here, so I'll challenge by sending a
simple libvirt patch, CCing involved authors, and seeing the feedback:

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 12429278fb..ce243e304b 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -1142,7 +1142,7 @@ get_files(vahControl * ctl)
             /* We don't need to add deny rw rules for readonly mounts,
              * this can only lead to troubles when mounting / readonly.
              */
-            if (vah_add_path(&buf, fs->src->path, fs->readonly ? "R" : "rw", true) != 0)
+            if (vah_add_path(&buf, fs->src->path, fs->readonly ? "R" : "rwlk", true) != 0)
                 goto cleanup;
         }
     }

Even after this change, this is not a global policy. Allowing hard links and
file locks would only be lifted for the 9p export path.

There would be other options as well of course: e.g. detecting on 9pfs side
whether AppArmor and co are enabled, and log a warning to the user a syscall
failed for that reason. But that would be much more complicated and I wonder
whether it would be worth it.

Best regards,
Christian Schoenebeck

Re: [PATCH v2 7/8] tests/9pfs: add local Tlink test

[Greg Kurz]

On Wed, 21 Oct 2020 14:51:09 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> This test case uses a Tlink request to create a hard link to a regular
> file using the 9pfs 'local' fs driver.
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  tests/qtest/virtio-9p-test.c | 71 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 71 insertions(+)
> 
> diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> index 33cba24b18..460fa49fe3 100644
> --- a/tests/qtest/virtio-9p-test.c
> +++ b/tests/qtest/virtio-9p-test.c
> @@ -260,6 +260,7 @@ static const char *rmessage_name(uint8_t id)
>          id == P9_RMKDIR ? "RMKDIR" :
>          id == P9_RLCREATE ? "RLCREATE" :
>          id == P9_RSYMLINK ? "RSYMLINK" :
> +        id == P9_RLINK ? "RLINK" :
>          id == P9_RUNLINKAT ? "RUNLINKAT" :
>          id == P9_RFLUSH ? "RFLUSH" :
>          id == P9_RREADDIR ? "READDIR" :
> @@ -767,6 +768,33 @@ static void v9fs_rsymlink(P9Req *req, v9fs_qid *qid)
>      v9fs_req_free(req);
>  }
>  
> +/* size[4] Tlink tag[2] dfid[4] fid[4] name[s] */
> +static P9Req *v9fs_tlink(QVirtio9P *v9p, uint32_t dfid, uint32_t fid,
> +                         const char *name, uint16_t tag)
> +{
> +    P9Req *req;
> +
> +    uint32_t body_size = 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_TLINK, tag);
> +    v9fs_uint32_write(req, dfid);
> +    v9fs_uint32_write(req, fid);
> +    v9fs_string_write(req, name);
> +    v9fs_req_send(req);
> +    return req;
> +}
> +
> +/* size[4] Rlink tag[2] */
> +static void v9fs_rlink(P9Req *req)
> +{
> +    v9fs_req_recv(req, P9_RLINK);
> +    v9fs_req_free(req);
> +}
> +
>  /* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */
>  static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char *name,
>                               uint32_t flags, uint16_t tag)
> @@ -1142,6 +1170,21 @@ static void do_symlink(QVirtio9P *v9p, const char *path, const char *clink,
>      g_free(name);
>  }
>  
> +/* create a hard link named @a clink in directory @a path pointing to @a to */
> +static void do_hardlink(QVirtio9P *v9p, const char *path, const char *clink,
> +                        const char *to)
> +{
> +    uint32_t dfid, fid;
> +    P9Req *req;
> +
> +    dfid = do_walk(v9p, path);
> +    fid = do_walk(v9p, to);
> +
> +    req = v9fs_tlink(v9p, dfid, fid, clink, 0);
> +    v9fs_req_wait_for_reply(req, NULL);
> +    v9fs_rlink(req);
> +}
> +
>  static void do_unlinkat(QVirtio9P *v9p, const char *atpath, const char *rpath,
>                          uint32_t flags)
>  {
> @@ -1321,6 +1364,33 @@ static void fs_unlinkat_symlink(void *obj, void *data,
>      g_free(real_file);
>  }
>  
> +static void fs_hardlink_file(void *obj, void *data, QGuestAllocator *t_alloc)
> +{
> +    QVirtio9P *v9p = obj;
> +    alloc = t_alloc;
> +    struct stat st_real, st_link;
> +    char *real_file = virtio_9p_test_path("07/real_file");
> +    char *hardlink_file = virtio_9p_test_path("07/hardlink_file");
> +
> +    do_attach(v9p);
> +    do_mkdir(v9p, "/", "07");
> +    do_lcreate(v9p, "07", "real_file");
> +    g_assert(stat(real_file, &st_real) == 0);
> +    g_assert((st_real.st_mode & S_IFMT) == S_IFREG);
> +
> +    do_hardlink(v9p, "07", "hardlink_file", "07/real_file");
> +
> +    /* check if link exists now ... */
> +    g_assert(stat(hardlink_file, &st_link) == 0);
> +    /* ... and it's a hard link, right? */
> +    g_assert((st_link.st_mode & S_IFMT) == S_IFREG);
> +    g_assert(st_link.st_dev == st_real.st_dev);
> +    g_assert(st_link.st_ino == st_real.st_ino);
> +
> +    g_free(hardlink_file);
> +    g_free(real_file);
> +}
> +
>  static void *assign_9p_local_driver(GString *cmd_line, void *arg)
>  {
>      virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
> @@ -1367,6 +1437,7 @@ static void register_virtio_9p_test(void)
>      qos_add_test("local/symlink_file", "virtio-9p", fs_symlink_file, &opts);
>      qos_add_test("local/unlinkat_symlink", "virtio-9p", fs_unlinkat_symlink,
>                   &opts);
> +    qos_add_test("local/hardlink_file", "virtio-9p", fs_hardlink_file, &opts);
>  }
>  
>  libqos_init(register_virtio_9p_test);

[PATCH v2 8/8] tests/9pfs: add local Tunlinkat hard link test

[Christian Schoenebeck]

This test case uses a Tunlinkat request to remove a previously hard
linked file by using the 9pfs 'local' fs driver.

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

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 460fa49fe3..23433913bb 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -1391,6 +1391,34 @@ static void fs_hardlink_file(void *obj, void *data, QGuestAllocator *t_alloc)
     g_free(real_file);
 }
 
+static void fs_unlinkat_hardlink(void *obj, void *data,
+                                 QGuestAllocator *t_alloc)
+{
+    QVirtio9P *v9p = obj;
+    alloc = t_alloc;
+    struct stat st_real, st_link;
+    char *real_file = virtio_9p_test_path("08/real_file");
+    char *hardlink_file = virtio_9p_test_path("08/hardlink_file");
+
+    do_attach(v9p);
+    do_mkdir(v9p, "/", "08");
+    do_lcreate(v9p, "08", "real_file");
+    g_assert(stat(real_file, &st_real) == 0);
+    g_assert((st_real.st_mode & S_IFMT) == S_IFREG);
+
+    do_hardlink(v9p, "08", "hardlink_file", "08/real_file");
+    g_assert(stat(hardlink_file, &st_link) == 0);
+
+    do_unlinkat(v9p, "08", "hardlink_file", 0);
+    /* symlink should be gone now */
+    g_assert(stat(hardlink_file, &st_link) != 0);
+    /* and old file should still exist */
+    g_assert(stat(real_file, &st_real) == 0);
+
+    g_free(hardlink_file);
+    g_free(real_file);
+}
+
 static void *assign_9p_local_driver(GString *cmd_line, void *arg)
 {
     virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1438,6 +1466,8 @@ static void register_virtio_9p_test(void)
     qos_add_test("local/unlinkat_symlink", "virtio-9p", fs_unlinkat_symlink,
                  &opts);
     qos_add_test("local/hardlink_file", "virtio-9p", fs_hardlink_file, &opts);
+    qos_add_test("local/unlinkat_hardlink", "virtio-9p", fs_unlinkat_hardlink,
+                 &opts);
 }
 
 libqos_init(register_virtio_9p_test);
-- 
2.20.1

Re: [PATCH v2 8/8] tests/9pfs: add local Tunlinkat hard link test

[Greg Kurz]

On Wed, 21 Oct 2020 14:55:46 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> This test case uses a Tunlinkat request to remove a previously hard
> linked file by using the 9pfs 'local' fs driver.
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  tests/qtest/virtio-9p-test.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> index 460fa49fe3..23433913bb 100644
> --- a/tests/qtest/virtio-9p-test.c
> +++ b/tests/qtest/virtio-9p-test.c
> @@ -1391,6 +1391,34 @@ static void fs_hardlink_file(void *obj, void *data, QGuestAllocator *t_alloc)
>      g_free(real_file);
>  }
>  
> +static void fs_unlinkat_hardlink(void *obj, void *data,
> +                                 QGuestAllocator *t_alloc)
> +{
> +    QVirtio9P *v9p = obj;
> +    alloc = t_alloc;
> +    struct stat st_real, st_link;
> +    char *real_file = virtio_9p_test_path("08/real_file");
> +    char *hardlink_file = virtio_9p_test_path("08/hardlink_file");
> +
> +    do_attach(v9p);
> +    do_mkdir(v9p, "/", "08");
> +    do_lcreate(v9p, "08", "real_file");
> +    g_assert(stat(real_file, &st_real) == 0);
> +    g_assert((st_real.st_mode & S_IFMT) == S_IFREG);
> +
> +    do_hardlink(v9p, "08", "hardlink_file", "08/real_file");
> +    g_assert(stat(hardlink_file, &st_link) == 0);
> +
> +    do_unlinkat(v9p, "08", "hardlink_file", 0);
> +    /* symlink should be gone now */
> +    g_assert(stat(hardlink_file, &st_link) != 0);
> +    /* and old file should still exist */
> +    g_assert(stat(real_file, &st_real) == 0);
> +
> +    g_free(hardlink_file);
> +    g_free(real_file);
> +}
> +
>  static void *assign_9p_local_driver(GString *cmd_line, void *arg)
>  {
>      virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
> @@ -1438,6 +1466,8 @@ static void register_virtio_9p_test(void)
>      qos_add_test("local/unlinkat_symlink", "virtio-9p", fs_unlinkat_symlink,
>                   &opts);
>      qos_add_test("local/hardlink_file", "virtio-9p", fs_hardlink_file, &opts);
> +    qos_add_test("local/unlinkat_hardlink", "virtio-9p", fs_unlinkat_hardlink,
> +                 &opts);
>  }
>  
>  libqos_init(register_virtio_9p_test);