[RFC PATCH] tests/9p: introduce declarative function calls

[RFC PATCH] tests/9p: introduce declarative function calls

[Christian Schoenebeck]

There are currently 3 different functions for sending a 9p 'Twalk'
request. They are all doing the same thing, just in a slightly different
way and with slightly different function arguments.

Merge those 3 functions into a single function by using a struct for
function call arguments and use designated initializers when calling this
function to turn usage into a declarative approach, which is better
readable and easier to maintain.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
  Before working on actual new stuff, I looked at the current unit test code
  and thought it's probably a good time to make the overall test code better
  readable before piling up more test code soon.

  In this patch I am suggesting to use named function arguments. For instance
 
     do_walk_expect_error(v9p, "non-existent", ENOENT);

  is probably a bit hard to tell what it is supposed to be doing without
  looking up the function prototype, whereas
  
    Twalk((TWalkOpt) {
      .client = v9p, .path = "non-existent", .expectErr = ENOENT
    });

  should make it immediately clear (provided you have some knowledge about the
  9p network protocol). I'm using this coding style of declarative functions
  calls a lot nowadays, which makes especially sense in the context of unit
  test code as those are typically passing literals as function arguments as
  shown above very often. But also in other contexts it is beneficial as it
  allows various linear combinations of possible function arguments being
  used / ommitted on function calls and still being handled with only one
  function implementation.
  
  Caller has a great flexibility of which function arguments to use, and is
  also completely free of the order of the arguments being specified.

  Another benefit is that you can also extend functionality later on, without
  breaking existing function calls. So this avoids a lot of refactoring work
  on the long-term.

  With C++ you could also define specific default values for ommitted function
  arguments. In C unfortunately it is just the language default initializer
  which usually is simply zero.

  Obviously with a large number of possible function arguments provided, some
  combinations make sense and some simply don't. In this patch for instance
  this is handled with assertion faults like:
  
    /* you can expect either Rwalk or Rlerror, but obviously not both */
    g_assert(!opt.expectErr || !(opt.Rwalk.nwqid || opt.Rwalk.wqid));

  So this would be a runtime error. In C++ you could turn the function into
  a constexpr and make that a compile error instead, in C there is
  
    _Static_assert(...)

  but as there is no constexpr, that would probably be a hard to achieve.

  Thoughts?
---
 tests/qtest/virtio-9p-test.c | 79 +++++++++++++++++++-----------------
 1 file changed, 42 insertions(+), 37 deletions(-)

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 25305a4cf7..6a7f1f6252 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -669,50 +669,51 @@ static void do_version(QVirtio9P *v9p)
     g_assert_cmpmem(server_version, server_len, version, strlen(version));
 }
 
+/* options for 'Twalk' 9p request */
+typedef struct TWalkOpt {
+    /* 9P client being used (mandatory) */
+    QVirtio9P *client;
+    /* path to walk to (mandatory) */
+    const char *path;
+    /* data being received from 9p server as 'Rwalk' response (optional) */
+    struct {
+        uint16_t *nwqid;
+        v9fs_qid **wqid;
+    } Rwalk;
+    /* do we expect an Rlerror response, if yes which error code? (optional) */
+    uint32_t expectErr;
+} TWalkOpt;
+
 /*
  * utility function: walk to requested dir and return fid for that dir and
  * the QIDs of server response
  */
-static uint32_t do_walk_rqids(QVirtio9P *v9p, const char *path, uint16_t *nwqid,
-                              v9fs_qid **wqid)
+static uint32_t Twalk(TWalkOpt opt)
 {
     char **wnames;
     P9Req *req;
+    uint32_t err;
     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, nwqid, wqid);
-
-    split_free(&wnames);
-    return fid;
-}
+    g_assert(opt.client);
+    g_assert(opt.path);
+    /* you can expect either Rwalk or Rlerror, but obviously not both */
+    g_assert(!opt.expectErr || !(opt.Rwalk.nwqid || opt.Rwalk.wqid));
 
-/* utility function: walk to requested dir and return fid for that dir */
-static uint32_t do_walk(QVirtio9P *v9p, const char *path)
-{
-    return do_walk_rqids(v9p, path, NULL, NULL);
-}
+    int nwnames = split(opt.path, "/", &wnames);
 
-/* utility function: walk to requested dir and expect passed error response */
-static void do_walk_expect_error(QVirtio9P *v9p, const char *path, uint32_t err)
-{
-    char **wnames;
-    P9Req *req;
-    uint32_t _err;
-    const uint32_t fid = genfid();
-
-    int nwnames = split(path, "/", &wnames);
-
-    req = v9fs_twalk(v9p, 0, fid, nwnames, wnames, 0);
+    req = v9fs_twalk(opt.client, 0, fid, nwnames, wnames, 0);
     v9fs_req_wait_for_reply(req, NULL);
-    v9fs_rlerror(req, &_err);
 
-    g_assert_cmpint(_err, ==, err);
+    if (opt.expectErr) {
+        v9fs_rlerror(req, &err);
+        g_assert_cmpint(err, ==, opt.expectErr);
+    } else {
+        v9fs_rwalk(req, opt.Rwalk.nwqid, opt.Rwalk.wqid);
+    }
 
     split_free(&wnames);
+    return fid;
 }
 
 static void fs_version(void *obj, void *data, QGuestAllocator *t_alloc)
@@ -1098,7 +1099,9 @@ static void fs_walk_nonexistent(void *obj, void *data, QGuestAllocator *t_alloc)
      * The 9p2000 protocol spec says: "If the first element cannot be walked
      * for any reason, Rerror is returned."
      */
-    do_walk_expect_error(v9p, "non-existent", ENOENT);
+    Twalk((TWalkOpt) {
+        .client = v9p, .path = "non-existent", .expectErr = ENOENT
+    });
 }
 
 static void fs_walk_2nd_nonexistent(void *obj, void *data,
@@ -1116,7 +1119,9 @@ static void fs_walk_2nd_nonexistent(void *obj, void *data,
     );
 
     do_attach_rqid(v9p, &root_qid);
-    fid = do_walk_rqids(v9p, path, &nwqid, &wqid);
+    fid = Twalk((TWalkOpt) {
+        .client = v9p, .path = path, .Rwalk.nwqid = &nwqid, .Rwalk.wqid = &wqid
+    });
     /*
      * The 9p2000 protocol spec says: "nwqid is therefore either nwname or the
      * index of the first elementwise walk that failed."
@@ -1311,7 +1316,7 @@ static void do_mkdir(QVirtio9P *v9p, const char *path, const char *cname)
     uint32_t fid;
     P9Req *req;
 
-    fid = do_walk(v9p, path);
+    fid = Twalk((TWalkOpt) { .client = v9p, .path = path });
 
     req = v9fs_tmkdir(v9p, fid, name, 0750, 0, 0);
     v9fs_req_wait_for_reply(req, NULL);
@@ -1326,7 +1331,7 @@ static uint32_t do_lcreate(QVirtio9P *v9p, const char *path,
     uint32_t fid;
     P9Req *req;
 
-    fid = do_walk(v9p, path);
+    fid = Twalk((TWalkOpt) { .client = v9p, .path = path });
 
     req = v9fs_tlcreate(v9p, fid, name, 0, 0750, 0, 0);
     v9fs_req_wait_for_reply(req, NULL);
@@ -1344,7 +1349,7 @@ static void do_symlink(QVirtio9P *v9p, const char *path, const char *clink,
     uint32_t fid;
     P9Req *req;
 
-    fid = do_walk(v9p, path);
+    fid = Twalk((TWalkOpt) { .client = v9p, .path = path });
 
     req = v9fs_tsymlink(v9p, fid, name, dst, 0, 0);
     v9fs_req_wait_for_reply(req, NULL);
@@ -1358,8 +1363,8 @@ static void do_hardlink(QVirtio9P *v9p, const char *path, const char *clink,
     uint32_t dfid, fid;
     P9Req *req;
 
-    dfid = do_walk(v9p, path);
-    fid = do_walk(v9p, to);
+    dfid = Twalk((TWalkOpt) { .client = v9p, .path = path });
+    fid = Twalk((TWalkOpt) { .client = v9p, .path = to });
 
     req = v9fs_tlink(v9p, dfid, fid, clink, 0);
     v9fs_req_wait_for_reply(req, NULL);
@@ -1373,7 +1378,7 @@ static void do_unlinkat(QVirtio9P *v9p, const char *atpath, const char *rpath,
     uint32_t fid;
     P9Req *req;
 
-    fid = do_walk(v9p, atpath);
+    fid = Twalk((TWalkOpt) { .client = v9p, .path = atpath });
 
     req = v9fs_tunlinkat(v9p, fid, name, flags, 0);
     v9fs_req_wait_for_reply(req, NULL);
-- 
2.30.2

Re: [RFC PATCH] tests/9p: introduce declarative function calls

[Greg Kurz]

Hi Christian,

On Fri, 24 Jun 2022 19:46:18 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> There are currently 3 different functions for sending a 9p 'Twalk'
> request. They are all doing the same thing, just in a slightly different
> way and with slightly different function arguments.
> 
> Merge those 3 functions into a single function by using a struct for
> function call arguments and use designated initializers when calling this
> function to turn usage into a declarative approach, which is better
> readable and easier to maintain.
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>   Before working on actual new stuff, I looked at the current unit test code
>   and thought it's probably a good time to make the overall test code better
>   readable before piling up more test code soon.
> 
>   In this patch I am suggesting to use named function arguments. For instance
>  
>      do_walk_expect_error(v9p, "non-existent", ENOENT);
> 
>   is probably a bit hard to tell what it is supposed to be doing without
>   looking up the function prototype, whereas
>   
>     Twalk((TWalkOpt) {
>       .client = v9p, .path = "non-existent", .expectErr = ENOENT
>     });
> 
>   should make it immediately clear (provided you have some knowledge about the
>   9p network protocol). I'm using this coding style of declarative functions
>   calls a lot nowadays, which makes especially sense in the context of unit
>   test code as those are typically passing literals as function arguments as
>   shown above very often. But also in other contexts it is beneficial as it
>   allows various linear combinations of possible function arguments being
>   used / ommitted on function calls and still being handled with only one
>   function implementation.
>   
>   Caller has a great flexibility of which function arguments to use, and is
>   also completely free of the order of the arguments being specified.
> 
>   Another benefit is that you can also extend functionality later on, without
>   breaking existing function calls. So this avoids a lot of refactoring work
>   on the long-term.
> 
>   With C++ you could also define specific default values for ommitted function
>   arguments. In C unfortunately it is just the language default initializer
>   which usually is simply zero.
> 

AFAIK the "Designated Initializers" feature of C99 guarantees zero is the
default so we should be good.

>   Obviously with a large number of possible function arguments provided, some
>   combinations make sense and some simply don't. In this patch for instance
>   this is handled with assertion faults like:
>   
>     /* you can expect either Rwalk or Rlerror, but obviously not both */
>     g_assert(!opt.expectErr || !(opt.Rwalk.nwqid || opt.Rwalk.wqid));
> 
>   So this would be a runtime error. In C++ you could turn the function into
>   a constexpr and make that a compile error instead, in C there is
>   
>     _Static_assert(...)
> 
>   but as there is no constexpr, that would probably be a hard to achieve.
> 
>   Thoughts?
> ---

This change LGTM. Some remarks below.

>  tests/qtest/virtio-9p-test.c | 79 +++++++++++++++++++-----------------
>  1 file changed, 42 insertions(+), 37 deletions(-)
> 
> diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> index 25305a4cf7..6a7f1f6252 100644
> --- a/tests/qtest/virtio-9p-test.c
> +++ b/tests/qtest/virtio-9p-test.c
> @@ -669,50 +669,51 @@ static void do_version(QVirtio9P *v9p)
>      g_assert_cmpmem(server_version, server_len, version, strlen(version));
>  }
>  
> +/* options for 'Twalk' 9p request */
> +typedef struct TWalkOpt {
> +    /* 9P client being used (mandatory) */
> +    QVirtio9P *client;
> +    /* path to walk to (mandatory) */
> +    const char *path;
> +    /* data being received from 9p server as 'Rwalk' response (optional) */
> +    struct {
> +        uint16_t *nwqid;
> +        v9fs_qid **wqid;
> +    } Rwalk;

Rwalk should be all downcase as a regular struct field name.

What about introducing:

typedef struct Rwalk {
    uint16_t nwqid;
    v9fs_qid *wqid;
} Rwalk;

and having an `Rwalk *rwalk` field in TwalkOpt ?

The rationale is that it might make sense for a caller to only want the
number of qids, but if it wants the qid array then nwqid is mandatory.

> +    /* do we expect an Rlerror response, if yes which error code? (optional) */
> +    uint32_t expectErr;
> +} TWalkOpt;
> +
>  /*
>   * utility function: walk to requested dir and return fid for that dir and
>   * the QIDs of server response
>   */
> -static uint32_t do_walk_rqids(QVirtio9P *v9p, const char *path, uint16_t *nwqid,
> -                              v9fs_qid **wqid)
> +static uint32_t Twalk(TWalkOpt opt)
>  {
>      char **wnames;
>      P9Req *req;
> +    uint32_t err;
>      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, nwqid, wqid);
> -
> -    split_free(&wnames);
> -    return fid;
> -}
> +    g_assert(opt.client);
> +    g_assert(opt.path);
> +    /* you can expect either Rwalk or Rlerror, but obviously not both */
> +    g_assert(!opt.expectErr || !(opt.Rwalk.nwqid || opt.Rwalk.wqid));
>  

Assert would then just be:

g_assert(!opt.expectErr || !opt.rwalk);

> -/* utility function: walk to requested dir and return fid for that dir */
> -static uint32_t do_walk(QVirtio9P *v9p, const char *path)
> -{
> -    return do_walk_rqids(v9p, path, NULL, NULL);
> -}
> +    int nwnames = split(opt.path, "/", &wnames);
>  
> -/* utility function: walk to requested dir and expect passed error response */
> -static void do_walk_expect_error(QVirtio9P *v9p, const char *path, uint32_t err)
> -{
> -    char **wnames;
> -    P9Req *req;
> -    uint32_t _err;
> -    const uint32_t fid = genfid();
> -
> -    int nwnames = split(path, "/", &wnames);
> -
> -    req = v9fs_twalk(v9p, 0, fid, nwnames, wnames, 0);
> +    req = v9fs_twalk(opt.client, 0, fid, nwnames, wnames, 0);
>      v9fs_req_wait_for_reply(req, NULL);
> -    v9fs_rlerror(req, &_err);
>  
> -    g_assert_cmpint(_err, ==, err);
> +    if (opt.expectErr) {
> +        v9fs_rlerror(req, &err);
> +        g_assert_cmpint(err, ==, opt.expectErr);
> +    } else {
> +        v9fs_rwalk(req, opt.Rwalk.nwqid, opt.Rwalk.wqid);
> +    }
>  
>      split_free(&wnames);
> +    return fid;
>  }
>  
>  static void fs_version(void *obj, void *data, QGuestAllocator *t_alloc)
> @@ -1098,7 +1099,9 @@ static void fs_walk_nonexistent(void *obj, void *data, QGuestAllocator *t_alloc)
>       * The 9p2000 protocol spec says: "If the first element cannot be walked
>       * for any reason, Rerror is returned."
>       */
> -    do_walk_expect_error(v9p, "non-existent", ENOENT);
> +    Twalk((TWalkOpt) {
> +        .client = v9p, .path = "non-existent", .expectErr = ENOENT
> +    });
>  }
>  
>  static void fs_walk_2nd_nonexistent(void *obj, void *data,
> @@ -1116,7 +1119,9 @@ static void fs_walk_2nd_nonexistent(void *obj, void *data,
>      );
>  
>      do_attach_rqid(v9p, &root_qid);
> -    fid = do_walk_rqids(v9p, path, &nwqid, &wqid);
> +    fid = Twalk((TWalkOpt) {
> +        .client = v9p, .path = path, .Rwalk.nwqid = &nwqid, .Rwalk.wqid = &wqid
> +    });
>      /*
>       * The 9p2000 protocol spec says: "nwqid is therefore either nwname or the
>       * index of the first elementwise walk that failed."
> @@ -1311,7 +1316,7 @@ static void do_mkdir(QVirtio9P *v9p, const char *path, const char *cname)
>      uint32_t fid;
>      P9Req *req;
>  
> -    fid = do_walk(v9p, path);
> +    fid = Twalk((TWalkOpt) { .client = v9p, .path = path });
>  
>      req = v9fs_tmkdir(v9p, fid, name, 0750, 0, 0);
>      v9fs_req_wait_for_reply(req, NULL);
> @@ -1326,7 +1331,7 @@ static uint32_t do_lcreate(QVirtio9P *v9p, const char *path,
>      uint32_t fid;
>      P9Req *req;
>  
> -    fid = do_walk(v9p, path);
> +    fid = Twalk((TWalkOpt) { .client = v9p, .path = path });
>  
>      req = v9fs_tlcreate(v9p, fid, name, 0, 0750, 0, 0);
>      v9fs_req_wait_for_reply(req, NULL);
> @@ -1344,7 +1349,7 @@ static void do_symlink(QVirtio9P *v9p, const char *path, const char *clink,
>      uint32_t fid;
>      P9Req *req;
>  
> -    fid = do_walk(v9p, path);
> +    fid = Twalk((TWalkOpt) { .client = v9p, .path = path });
>  
>      req = v9fs_tsymlink(v9p, fid, name, dst, 0, 0);
>      v9fs_req_wait_for_reply(req, NULL);
> @@ -1358,8 +1363,8 @@ static void do_hardlink(QVirtio9P *v9p, const char *path, const char *clink,
>      uint32_t dfid, fid;
>      P9Req *req;
>  
> -    dfid = do_walk(v9p, path);
> -    fid = do_walk(v9p, to);
> +    dfid = Twalk((TWalkOpt) { .client = v9p, .path = path });
> +    fid = Twalk((TWalkOpt) { .client = v9p, .path = to });
>  
>      req = v9fs_tlink(v9p, dfid, fid, clink, 0);
>      v9fs_req_wait_for_reply(req, NULL);
> @@ -1373,7 +1378,7 @@ static void do_unlinkat(QVirtio9P *v9p, const char *atpath, const char *rpath,
>      uint32_t fid;
>      P9Req *req;
>  
> -    fid = do_walk(v9p, atpath);
> +    fid = Twalk((TWalkOpt) { .client = v9p, .path = atpath });
>  
>      req = v9fs_tunlinkat(v9p, fid, name, flags, 0);
>      v9fs_req_wait_for_reply(req, NULL);

I understand that the current patch is converting the `do_walk*()` functions.
Would it make sense to convert the many callers of `v9fs_twalk()` to call
`Twalk()` as well in a subsequent patch ?

Cheers,

--
Greg

Re: [RFC PATCH] tests/9p: introduce declarative function calls

[Christian Schoenebeck]

On Mittwoch, 29. Juni 2022 09:35:02 CEST Greg Kurz wrote:
> Hi Christian,

Hi Greg,

first off, this RFC patch was really just yet intended for discussing the high
aspect coding style idea of using named function arguments in general (with a
random example), i.e. checking acceptance, but also to see whether somebody
sees a way to code this more elegantly.

In a more perfect (C-)universe I would like to code named function args like:

    void someTask(struct { int a = -1; char b; double c; } opt) {
        ...
    }
    ...
    someTask({ .a = 3, .c = 3.141 });

So using an anonymous struct in the function signature (that part works
actually in C, with a compiler warning though), but the problem is that the
function call would need a type cast:

    someTask((SomeType) { .a = 3, .c = 3.141 });

Which defeats the motivation of using an anonymous struct. At least I did not
find a way to get rid of this type cast (compiler options or whatever), which
I somehow find odd, because you are actually allowed to do this in a function
body / block:

    SomeStruct foo = {
        .a = 3
    };

So you don't have to do:

    SomeStruct foo = (SomeStruct) {
        .a = 3
    };

Some people seem to define a variadic macro, but you would have to do that for
each new function, e.g.:

    #define SomeTask(...) someTask((SomeType) __VA_ARGS__)

Not very appealing.

> On Fri, 24 Jun 2022 19:46:18 +0200
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > There are currently 3 different functions for sending a 9p 'Twalk'
> > request. They are all doing the same thing, just in a slightly different
> > way and with slightly different function arguments.
> > 
> > Merge those 3 functions into a single function by using a struct for
> > function call arguments and use designated initializers when calling this
> > function to turn usage into a declarative approach, which is better
> > readable and easier to maintain.
> > 
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> > 
> >   Before working on actual new stuff, I looked at the current unit test
> >   code
> >   and thought it's probably a good time to make the overall test code
> >   better
> >   readable before piling up more test code soon.
> >   
> >   In this patch I am suggesting to use named function arguments. For
> >   instance
> >   
> >      do_walk_expect_error(v9p, "non-existent", ENOENT);
> >   
> >   is probably a bit hard to tell what it is supposed to be doing without
> >   looking up the function prototype, whereas
> >   
> >     Twalk((TWalkOpt) {
> >     
> >       .client = v9p, .path = "non-existent", .expectErr = ENOENT
> >     
> >     });
> >   
> >   should make it immediately clear (provided you have some knowledge about
> >   the 9p network protocol). I'm using this coding style of declarative
> >   functions calls a lot nowadays, which makes especially sense in the
> >   context of unit test code as those are typically passing literals as
> >   function arguments as shown above very often. But also in other
> >   contexts it is beneficial as it allows various linear combinations of
> >   possible function arguments being used / ommitted on function calls and
> >   still being handled with only one function implementation.
> >   
> >   Caller has a great flexibility of which function arguments to use, and
> >   is
> >   also completely free of the order of the arguments being specified.
> >   
> >   Another benefit is that you can also extend functionality later on,
> >   without
> >   breaking existing function calls. So this avoids a lot of refactoring
> >   work
> >   on the long-term.
> >   
> >   With C++ you could also define specific default values for ommitted
> >   function arguments. In C unfortunately it is just the language default
> >   initializer which usually is simply zero.
> 
> AFAIK the "Designated Initializers" feature of C99 guarantees zero is the
> default so we should be good.

No doubt that omitted fields on designated initializers are always initialized
with a reasonable default value with C99. My point was rather that's it
unfortunately *only* zero with C. Because in most cases you are fine with
zero, but in our context of 9p tests for instance you might want to have this
as default initializer for fids instead:

    struct TwalkOpt {
       ...
       fid = -1;
       ...
    };

Then we could do, if omitted:

    /* caller did not pass a fid, so let's generate one */
    if (opt.fid == -1)
        opt.fid = genfid();

As fid == 0 is indeed a reasonable usage scenario for a caller.

> >   Obviously with a large number of possible function arguments provided,
> >   some
> >   combinations make sense and some simply don't. In this patch for
> >   instance
> >   
> >   this is handled with assertion faults like:
> >     /* you can expect either Rwalk or Rlerror, but obviously not both */
> >     g_assert(!opt.expectErr || !(opt.Rwalk.nwqid || opt.Rwalk.wqid));
> >   
> >   So this would be a runtime error. In C++ you could turn the function
> >   into
> >   a constexpr and make that a compile error instead, in C there is
> >   
> >     _Static_assert(...)
> >   
> >   but as there is no constexpr, that would probably be a hard to achieve.
> >   
> >   Thoughts?
> > 
> > ---
> 
> This change LGTM. Some remarks below.
> 
> >  tests/qtest/virtio-9p-test.c | 79 +++++++++++++++++++-----------------
> >  1 file changed, 42 insertions(+), 37 deletions(-)
> > 
> > diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> > index 25305a4cf7..6a7f1f6252 100644
> > --- a/tests/qtest/virtio-9p-test.c
> > +++ b/tests/qtest/virtio-9p-test.c
> > @@ -669,50 +669,51 @@ static void do_version(QVirtio9P *v9p)
> > 
> >      g_assert_cmpmem(server_version, server_len, version,
> >      strlen(version));
> >  
> >  }
> > 
> > +/* options for 'Twalk' 9p request */
> > +typedef struct TWalkOpt {
> > +    /* 9P client being used (mandatory) */
> > +    QVirtio9P *client;
> > +    /* path to walk to (mandatory) */
> > +    const char *path;
> > +    /* data being received from 9p server as 'Rwalk' response (optional)
> > */ +    struct {
> > +        uint16_t *nwqid;
> > +        v9fs_qid **wqid;
> > +    } Rwalk;
> 
> Rwalk should be all downcase as a regular struct field name.

Sure! I guess same applies to the function name which should be lower case
twalk() instead of Twalk() as well.

> What about introducing:
> 
> typedef struct Rwalk {
>     uint16_t nwqid;
>     v9fs_qid *wqid;
> } Rwalk;
> 
> and having an `Rwalk *rwalk` field in TwalkOpt ?
> 
> The rationale is that it might make sense for a caller to only want the
> number of qids, but if it wants the qid array then nwqid is mandatory.

Yeah, that makes sense in this Twalk example. But what about other 9p request
types where you have much more data being returned, and therefore more
potential linear combinations of individual fields caller might be interested
in, e.g. Rstatfs? I mean in the sense of homogenous coding style?

https://github.com/chaos/diod/blob/master/protocol.md#statfs----get-file-system-information

or Rgettattr:
https://github.com/chaos/diod/blob/master/protocol.md#getattr----get-file-attributes

So always one huge response struct, like you get all or nothing?

> > +    /* do we expect an Rlerror response, if yes which error code?
> > (optional) */ +    uint32_t expectErr;
> > +} TWalkOpt;
> > +
> > 
> >  /*
> >  
> >   * utility function: walk to requested dir and return fid for that dir
> >   and
> >   * the QIDs of server response
> >   */
> > 
> > -static uint32_t do_walk_rqids(QVirtio9P *v9p, const char *path, uint16_t
> > *nwqid, -                              v9fs_qid **wqid)
> > +static uint32_t Twalk(TWalkOpt opt)
> > 
> >  {
> >  
> >      char **wnames;
> >      P9Req *req;
> > 
> > +    uint32_t err;
> > 
> >      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, nwqid, wqid);
> > -
> > -    split_free(&wnames);
> > -    return fid;
> > -}
> > +    g_assert(opt.client);
> > +    g_assert(opt.path);
> > +    /* you can expect either Rwalk or Rlerror, but obviously not both */
> > +    g_assert(!opt.expectErr || !(opt.Rwalk.nwqid || opt.Rwalk.wqid));
> 
> Assert would then just be:
> 
> g_assert(!opt.expectErr || !opt.rwalk);

Yes, that's true.

> > -/* utility function: walk to requested dir and return fid for that dir */
> > -static uint32_t do_walk(QVirtio9P *v9p, const char *path)
> > -{
> > -    return do_walk_rqids(v9p, path, NULL, NULL);
> > -}
> > +    int nwnames = split(opt.path, "/", &wnames);
> > 
> > -/* utility function: walk to requested dir and expect passed error
> > response */ -static void do_walk_expect_error(QVirtio9P *v9p, const char
> > *path, uint32_t err) -{
> > -    char **wnames;
> > -    P9Req *req;
> > -    uint32_t _err;
> > -    const uint32_t fid = genfid();
> > -
> > -    int nwnames = split(path, "/", &wnames);
> > -
> > -    req = v9fs_twalk(v9p, 0, fid, nwnames, wnames, 0);
> > +    req = v9fs_twalk(opt.client, 0, fid, nwnames, wnames, 0);
> > 
> >      v9fs_req_wait_for_reply(req, NULL);
> > 
> > -    v9fs_rlerror(req, &_err);
> > 
> > -    g_assert_cmpint(_err, ==, err);
> > +    if (opt.expectErr) {
> > +        v9fs_rlerror(req, &err);
> > +        g_assert_cmpint(err, ==, opt.expectErr);
> > +    } else {
> > +        v9fs_rwalk(req, opt.Rwalk.nwqid, opt.Rwalk.wqid);
> > +    }
> > 
> >      split_free(&wnames);
> > 
> > +    return fid;
> > 
> >  }
> >  
> >  static void fs_version(void *obj, void *data, QGuestAllocator *t_alloc)
> > 
> > @@ -1098,7 +1099,9 @@ static void fs_walk_nonexistent(void *obj, void
> > *data, QGuestAllocator *t_alloc)> 
> >       * The 9p2000 protocol spec says: "If the first element cannot be
> >       walked
> >       * for any reason, Rerror is returned."
> >       */
> > 
> > -    do_walk_expect_error(v9p, "non-existent", ENOENT);
> > +    Twalk((TWalkOpt) {
> > +        .client = v9p, .path = "non-existent", .expectErr = ENOENT
> > +    });
> > 
> >  }
> >  
> >  static void fs_walk_2nd_nonexistent(void *obj, void *data,
> > 
> > @@ -1116,7 +1119,9 @@ static void fs_walk_2nd_nonexistent(void *obj, void
> > *data,> 
> >      );
> >      
> >      do_attach_rqid(v9p, &root_qid);
> > 
> > -    fid = do_walk_rqids(v9p, path, &nwqid, &wqid);
> > +    fid = Twalk((TWalkOpt) {
> > +        .client = v9p, .path = path, .Rwalk.nwqid = &nwqid, .Rwalk.wqid =
> > &wqid +    });
> > 
> >      /*
> >      
> >       * The 9p2000 protocol spec says: "nwqid is therefore either nwname
> >       or the
> >       * index of the first elementwise walk that failed."
> > 
> > @@ -1311,7 +1316,7 @@ static void do_mkdir(QVirtio9P *v9p, const char
> > *path, const char *cname)> 
> >      uint32_t fid;
> >      P9Req *req;
> > 
> > -    fid = do_walk(v9p, path);
> > +    fid = Twalk((TWalkOpt) { .client = v9p, .path = path });
> > 
> >      req = v9fs_tmkdir(v9p, fid, name, 0750, 0, 0);
> >      v9fs_req_wait_for_reply(req, NULL);
> > 
> > @@ -1326,7 +1331,7 @@ static uint32_t do_lcreate(QVirtio9P *v9p, const
> > char *path,> 
> >      uint32_t fid;
> >      P9Req *req;
> > 
> > -    fid = do_walk(v9p, path);
> > +    fid = Twalk((TWalkOpt) { .client = v9p, .path = path });
> > 
> >      req = v9fs_tlcreate(v9p, fid, name, 0, 0750, 0, 0);
> >      v9fs_req_wait_for_reply(req, NULL);
> > 
> > @@ -1344,7 +1349,7 @@ static void do_symlink(QVirtio9P *v9p, const char
> > *path, const char *clink,> 
> >      uint32_t fid;
> >      P9Req *req;
> > 
> > -    fid = do_walk(v9p, path);
> > +    fid = Twalk((TWalkOpt) { .client = v9p, .path = path });
> > 
> >      req = v9fs_tsymlink(v9p, fid, name, dst, 0, 0);
> >      v9fs_req_wait_for_reply(req, NULL);
> > 
> > @@ -1358,8 +1363,8 @@ static void do_hardlink(QVirtio9P *v9p, const char
> > *path, const char *clink,> 
> >      uint32_t dfid, fid;
> >      P9Req *req;
> > 
> > -    dfid = do_walk(v9p, path);
> > -    fid = do_walk(v9p, to);
> > +    dfid = Twalk((TWalkOpt) { .client = v9p, .path = path });
> > +    fid = Twalk((TWalkOpt) { .client = v9p, .path = to });
> > 
> >      req = v9fs_tlink(v9p, dfid, fid, clink, 0);
> >      v9fs_req_wait_for_reply(req, NULL);
> > 
> > @@ -1373,7 +1378,7 @@ static void do_unlinkat(QVirtio9P *v9p, const char
> > *atpath, const char *rpath,> 
> >      uint32_t fid;
> >      P9Req *req;
> > 
> > -    fid = do_walk(v9p, atpath);
> > +    fid = Twalk((TWalkOpt) { .client = v9p, .path = atpath });
> > 
> >      req = v9fs_tunlinkat(v9p, fid, name, flags, 0);
> >      v9fs_req_wait_for_reply(req, NULL);
> 
> I understand that the current patch is converting the `do_walk*()`
> functions. Would it make sense to convert the many callers of
> `v9fs_twalk()` to call `Twalk()` as well in a subsequent patch ?

Yeah, I would probably merge v9fs_twalk() as well, whether as a separate patch
or merged to the next version of this patch, I don't care.

The only real difference is that v9fs_twalk() does not wait for a server
response. But that behaviour could (if desired) also be preserved with an
optional P9Req* field. But maybe we could simply drop this low-level
separation of not waiting for a respone anyway.

Best regards,
Christian Schoenebeck

[RFC PATCH v2] tests/9p: introduce declarative function calls

[Christian Schoenebeck]

There are currently 4 different functions for sending a 9p 'Twalk'
request. They are all doing the same thing, just in a slightly different
way and with slightly different function arguments.

Merge those 4 functions into a single function by using a struct for
function call arguments and use designated initializers when calling this
function to turn usage into a declarative apporach, which is better
readable and easier to maintain.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---

v1 -> v2:

  * Also merge low-level function v9fs_twalk().

  * Lower case twalk() function name.

  * Lower case rwalk struct field.

  * Add result struct TWalkRes.

  NOTE: I have not separated rwalk struct, because it would have
  simplified code at one place, but complicated it at another one.

 tests/qtest/virtio-9p-test.c | 251 +++++++++++++++++++++--------------
 1 file changed, 154 insertions(+), 97 deletions(-)

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 25305a4cf7..69b1c27268 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -72,6 +72,7 @@ static int split(const char *in, const char *delim, char ***out)
 static void split_free(char ***out)
 {
     int i;
+    if (!*out) return;
     for (i = 0; (*out)[i]; ++i) {
         g_free((*out)[i]);
     }
@@ -390,31 +391,6 @@ static void v9fs_rattach(P9Req *req, v9fs_qid *qid)
     v9fs_req_free(req);
 }
 
-/* size[4] Twalk tag[2] fid[4] newfid[4] nwname[2] nwname*(wname[s]) */
-static P9Req *v9fs_twalk(QVirtio9P *v9p, uint32_t fid, uint32_t newfid,
-                         uint16_t nwname, char *const wnames[], uint16_t tag)
-{
-    P9Req *req;
-    int i;
-    uint32_t body_size = 4 + 4 + 2;
-
-    for (i = 0; i < nwname; i++) {
-        uint16_t wname_size = v9fs_string_size(wnames[i]);
-
-        g_assert_cmpint(body_size, <=, UINT32_MAX - wname_size);
-        body_size += wname_size;
-    }
-    req = v9fs_req_init(v9p,  body_size, P9_TWALK, tag);
-    v9fs_uint32_write(req, fid);
-    v9fs_uint32_write(req, newfid);
-    v9fs_uint16_write(req, nwname);
-    for (i = 0; i < nwname; i++) {
-        v9fs_string_write(req, wnames[i]);
-    }
-    v9fs_req_send(req);
-    return req;
-}
-
 /* size[4] Rwalk tag[2] nwqid[2] nwqid*(wqid[13]) */
 static void v9fs_rwalk(P9Req *req, uint16_t *nwqid, v9fs_qid **wqid)
 {
@@ -432,6 +408,98 @@ static void v9fs_rwalk(P9Req *req, uint16_t *nwqid, v9fs_qid **wqid)
     v9fs_req_free(req);
 }
 
+/* options for 'Twalk' 9p request */
+typedef struct TWalkOpt {
+    /* 9P client being used (mandatory) */
+    QVirtio9P *client;
+    /* user supplied tag number being returned with response (optional) */
+    uint16_t tag;
+    /* file ID of directory from where walk should start (optional) */
+    uint32_t fid;
+    /* file ID for target directory being walked to (optional) */
+    uint32_t newfid;
+    /* low level variant of path to walk to (optional) */
+    uint16_t nwname;
+    char **wnames;
+    /* high level variant of path to walk to (optional) */
+    const char *path;
+    /* data being received from 9p server as 'Rwalk' response (optional) */
+    struct {
+        uint16_t *nwqid;
+        v9fs_qid **wqid;
+    } rwalk;
+    /* only send Twalk request but not wait for a reply? (optional) */
+    bool requestOnly;
+    /* do we expect an Rlerror response, if yes which error code? (optional) */
+    uint32_t expectErr;
+} TWalkOpt;
+
+/* result of 'Twalk' 9p request */
+typedef struct TWalkRes {
+    /* file ID of target directory been walked to */
+    uint32_t newfid;
+    /* if requestOnly was set: request object for further processing */
+    P9Req *req;
+} TWalkRes;
+
+/* size[4] Twalk tag[2] fid[4] newfid[4] nwname[2] nwname*(wname[s]) */
+static TWalkRes twalk(TWalkOpt opt)
+{
+    P9Req *req;
+    int i;
+    uint32_t body_size = 4 + 4 + 2;
+    uint32_t err;
+    char **wnames = NULL;
+
+    g_assert(opt.client);
+    /* expecting either high- or low-level path, both not both */
+    g_assert(!opt.path || !(opt.nwname || opt.wnames));
+    /* expecting either Rwalk or Rlerror, but obviously not both */
+    g_assert(!opt.expectErr || !(opt.rwalk.nwqid || opt.rwalk.wqid));
+
+    if (!opt.newfid) {
+        opt.newfid = genfid();
+    }
+
+    if (opt.path) {
+        opt.nwname = split(opt.path, "/", &wnames);
+        opt.wnames = wnames;
+    }
+
+    for (i = 0; i < opt.nwname; i++) {
+        uint16_t wname_size = v9fs_string_size(opt.wnames[i]);
+
+        g_assert_cmpint(body_size, <=, UINT32_MAX - wname_size);
+        body_size += wname_size;
+    }
+    req = v9fs_req_init(opt.client, body_size, P9_TWALK, opt.tag);
+    v9fs_uint32_write(req, opt.fid);
+    v9fs_uint32_write(req, opt.newfid);
+    v9fs_uint16_write(req, opt.nwname);
+    for (i = 0; i < opt.nwname; i++) {
+        v9fs_string_write(req, opt.wnames[i]);
+    }
+    v9fs_req_send(req);
+
+    if (!opt.requestOnly) {
+        v9fs_req_wait_for_reply(req, NULL);
+        if (opt.expectErr) {
+            v9fs_rlerror(req, &err);
+            g_assert_cmpint(err, ==, opt.expectErr);
+        } else {
+            v9fs_rwalk(req, opt.rwalk.nwqid, opt.rwalk.wqid);
+        }
+        req = NULL; /* request was freed */
+    }
+
+    split_free(&wnames);
+
+    return (TWalkRes) {
+        .newfid = opt.newfid,
+        .req = req,
+    };
+}
+
 /* size[4] Tgetattr tag[2] fid[4] request_mask[8] */
 static P9Req *v9fs_tgetattr(QVirtio9P *v9p, uint32_t fid, uint64_t request_mask,
                             uint16_t tag)
@@ -669,52 +737,6 @@ static void do_version(QVirtio9P *v9p)
     g_assert_cmpmem(server_version, server_len, version, strlen(version));
 }
 
-/*
- * utility function: walk to requested dir and return fid for that dir and
- * the QIDs of server response
- */
-static uint32_t do_walk_rqids(QVirtio9P *v9p, const char *path, uint16_t *nwqid,
-                              v9fs_qid **wqid)
-{
-    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, nwqid, wqid);
-
-    split_free(&wnames);
-    return fid;
-}
-
-/* utility function: walk to requested dir and return fid for that dir */
-static uint32_t do_walk(QVirtio9P *v9p, const char *path)
-{
-    return do_walk_rqids(v9p, path, NULL, NULL);
-}
-
-/* utility function: walk to requested dir and expect passed error response */
-static void do_walk_expect_error(QVirtio9P *v9p, const char *path, uint32_t err)
-{
-    char **wnames;
-    P9Req *req;
-    uint32_t _err;
-    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_rlerror(req, &_err);
-
-    g_assert_cmpint(_err, ==, err);
-
-    split_free(&wnames);
-}
-
 static void fs_version(void *obj, void *data, QGuestAllocator *t_alloc)
 {
     alloc = t_alloc;
@@ -757,7 +779,10 @@ static void fs_walk(void *obj, void *data, QGuestAllocator *t_alloc)
     }
 
     do_attach(v9p);
-    req = v9fs_twalk(v9p, 0, 1, P9_MAXWELEM, wnames, 0);
+    req = twalk((TWalkOpt) {
+        .client = v9p, .fid = 0, .newfid = 1,
+        .nwname = P9_MAXWELEM, .wnames = wnames, .requestOnly = true
+    }).req;
     v9fs_req_wait_for_reply(req, NULL);
     v9fs_rwalk(req, &nwqid, &wqid);
 
@@ -941,7 +966,7 @@ static void fs_readdir(void *obj, void *data, QGuestAllocator *t_alloc)
 {
     QVirtio9P *v9p = obj;
     alloc = t_alloc;
-    char *const wnames[] = { g_strdup(QTEST_V9FS_SYNTH_READDIR_DIR) };
+    char *wnames[] = { g_strdup(QTEST_V9FS_SYNTH_READDIR_DIR) };
     uint16_t nqid;
     v9fs_qid qid;
     uint32_t count, nentries;
@@ -949,7 +974,10 @@ static void fs_readdir(void *obj, void *data, QGuestAllocator *t_alloc)
     P9Req *req;
 
     do_attach(v9p);
-    req = v9fs_twalk(v9p, 0, 1, 1, wnames, 0);
+    req = twalk((TWalkOpt) {
+        .client = v9p, .fid = 0, .newfid = 1,
+        .nwname = 1, .wnames = wnames, .requestOnly = true
+    }).req;
     v9fs_req_wait_for_reply(req, NULL);
     v9fs_rwalk(req, &nqid, NULL);
     g_assert_cmpint(nqid, ==, 1);
@@ -993,7 +1021,7 @@ static void fs_readdir(void *obj, void *data, QGuestAllocator *t_alloc)
 /* readdir test where overall request is split over several messages */
 static void do_readdir_split(QVirtio9P *v9p, uint32_t count)
 {
-    char *const wnames[] = { g_strdup(QTEST_V9FS_SYNTH_READDIR_DIR) };
+    char *wnames[] = { g_strdup(QTEST_V9FS_SYNTH_READDIR_DIR) };
     uint16_t nqid;
     v9fs_qid qid;
     uint32_t nentries, npartialentries;
@@ -1010,7 +1038,10 @@ static void do_readdir_split(QVirtio9P *v9p, uint32_t count)
     nentries = 0;
     tail = NULL;
 
-    req = v9fs_twalk(v9p, 0, fid, 1, wnames, 0);
+    req = twalk((TWalkOpt) {
+        .client = v9p, .fid = 0, .newfid = fid,
+        .nwname = 1, .wnames = wnames, .requestOnly = true
+    }).req;
     v9fs_req_wait_for_reply(req, NULL);
     v9fs_rwalk(req, &nqid, NULL);
     g_assert_cmpint(nqid, ==, 1);
@@ -1074,12 +1105,15 @@ static void fs_walk_no_slash(void *obj, void *data, QGuestAllocator *t_alloc)
 {
     QVirtio9P *v9p = obj;
     alloc = t_alloc;
-    char *const wnames[] = { g_strdup(" /") };
+    char *wnames[] = { g_strdup(" /") };
     P9Req *req;
     uint32_t err;
 
     do_attach(v9p);
-    req = v9fs_twalk(v9p, 0, 1, 1, wnames, 0);
+    req = twalk((TWalkOpt) {
+        .client = v9p, .fid = 0, .newfid = 1,
+        .nwname = 1, .wnames = wnames, .requestOnly = true
+    }).req;
     v9fs_req_wait_for_reply(req, NULL);
     v9fs_rlerror(req, &err);
 
@@ -1098,7 +1132,9 @@ static void fs_walk_nonexistent(void *obj, void *data, QGuestAllocator *t_alloc)
      * The 9p2000 protocol spec says: "If the first element cannot be walked
      * for any reason, Rerror is returned."
      */
-    do_walk_expect_error(v9p, "non-existent", ENOENT);
+    twalk((TWalkOpt) {
+        .client = v9p, .path = "non-existent", .expectErr = ENOENT
+    });
 }
 
 static void fs_walk_2nd_nonexistent(void *obj, void *data,
@@ -1116,7 +1152,10 @@ static void fs_walk_2nd_nonexistent(void *obj, void *data,
     );
 
     do_attach_rqid(v9p, &root_qid);
-    fid = do_walk_rqids(v9p, path, &nwqid, &wqid);
+    fid = twalk((TWalkOpt) {
+        .client = v9p, .path = path,
+        .rwalk.nwqid = &nwqid, .rwalk.wqid = &wqid
+    }).newfid;
     /*
      * The 9p2000 protocol spec says: "nwqid is therefore either nwname or the
      * index of the first elementwise walk that failed."
@@ -1148,7 +1187,10 @@ static void fs_walk_none(void *obj, void *data, QGuestAllocator *t_alloc)
     v9fs_req_wait_for_reply(req, NULL);
     v9fs_rattach(req, &root_qid);
 
-    req = v9fs_twalk(v9p, 0, 1, 0, NULL, 0);
+    req = twalk((TWalkOpt) {
+        .client = v9p, .fid = 0, .newfid = 1,
+        .nwname = 0, .wnames = NULL, .requestOnly = true
+    }).req;
     v9fs_req_wait_for_reply(req, NULL);
     v9fs_rwalk(req, NULL, &wqid);
 
@@ -1166,7 +1208,7 @@ static void fs_walk_dotdot(void *obj, void *data, QGuestAllocator *t_alloc)
 {
     QVirtio9P *v9p = obj;
     alloc = t_alloc;
-    char *const wnames[] = { g_strdup("..") };
+    char *wnames[] = { g_strdup("..") };
     v9fs_qid root_qid;
     g_autofree v9fs_qid *wqid = NULL;
     P9Req *req;
@@ -1176,7 +1218,10 @@ static void fs_walk_dotdot(void *obj, void *data, QGuestAllocator *t_alloc)
     v9fs_req_wait_for_reply(req, NULL);
     v9fs_rattach(req, &root_qid);
 
-    req = v9fs_twalk(v9p, 0, 1, 1, wnames, 0);
+    req = twalk((TWalkOpt) {
+        .client = v9p, .fid = 0, .newfid = 1,
+        .nwname = 1, .wnames = wnames, .requestOnly = true
+    }).req;
     v9fs_req_wait_for_reply(req, NULL);
     v9fs_rwalk(req, NULL, &wqid); /* We now we'll get one qid */
 
@@ -1189,11 +1234,14 @@ static void fs_lopen(void *obj, void *data, QGuestAllocator *t_alloc)
 {
     QVirtio9P *v9p = obj;
     alloc = t_alloc;
-    char *const wnames[] = { g_strdup(QTEST_V9FS_SYNTH_LOPEN_FILE) };
+    char *wnames[] = { g_strdup(QTEST_V9FS_SYNTH_LOPEN_FILE) };
     P9Req *req;
 
     do_attach(v9p);
-    req = v9fs_twalk(v9p, 0, 1, 1, wnames, 0);
+    req = twalk((TWalkOpt) {
+        .client = v9p, .fid = 0, .newfid = 1,
+        .nwname = 1, .wnames = wnames, .requestOnly = true
+    }).req;
     v9fs_req_wait_for_reply(req, NULL);
     v9fs_rwalk(req, NULL, NULL);
 
@@ -1209,13 +1257,16 @@ static void fs_write(void *obj, void *data, QGuestAllocator *t_alloc)
     QVirtio9P *v9p = obj;
     alloc = t_alloc;
     static const uint32_t write_count = P9_MAX_SIZE / 2;
-    char *const wnames[] = { g_strdup(QTEST_V9FS_SYNTH_WRITE_FILE) };
+    char *wnames[] = { g_strdup(QTEST_V9FS_SYNTH_WRITE_FILE) };
     g_autofree char *buf = g_malloc0(write_count);
     uint32_t count;
     P9Req *req;
 
     do_attach(v9p);
-    req = v9fs_twalk(v9p, 0, 1, 1, wnames, 0);
+    req = twalk((TWalkOpt) {
+        .client = v9p, .fid = 0, .newfid = 1,
+        .nwname = 1, .wnames = wnames, .requestOnly = true
+    }).req;
     v9fs_req_wait_for_reply(req, NULL);
     v9fs_rwalk(req, NULL, NULL);
 
@@ -1235,13 +1286,16 @@ static void fs_flush_success(void *obj, void *data, QGuestAllocator *t_alloc)
 {
     QVirtio9P *v9p = obj;
     alloc = t_alloc;
-    char *const wnames[] = { g_strdup(QTEST_V9FS_SYNTH_FLUSH_FILE) };
+    char *wnames[] = { g_strdup(QTEST_V9FS_SYNTH_FLUSH_FILE) };
     P9Req *req, *flush_req;
     uint32_t reply_len;
     uint8_t should_block;
 
     do_attach(v9p);
-    req = v9fs_twalk(v9p, 0, 1, 1, wnames, 0);
+    req = twalk((TWalkOpt) {
+        .client = v9p, .fid = 0, .newfid = 1,
+        .nwname = 1, .wnames = wnames, .requestOnly = true
+    }).req;
     v9fs_req_wait_for_reply(req, NULL);
     v9fs_rwalk(req, NULL, NULL);
 
@@ -1272,13 +1326,16 @@ static void fs_flush_ignored(void *obj, void *data, QGuestAllocator *t_alloc)
 {
     QVirtio9P *v9p = obj;
     alloc = t_alloc;
-    char *const wnames[] = { g_strdup(QTEST_V9FS_SYNTH_FLUSH_FILE) };
+    char *wnames[] = { g_strdup(QTEST_V9FS_SYNTH_FLUSH_FILE) };
     P9Req *req, *flush_req;
     uint32_t count;
     uint8_t should_block;
 
     do_attach(v9p);
-    req = v9fs_twalk(v9p, 0, 1, 1, wnames, 0);
+    req = twalk((TWalkOpt) {
+        .client = v9p, .fid = 0, .newfid = 1,
+        .nwname = 1, .wnames = wnames, .requestOnly = true
+    }).req;
     v9fs_req_wait_for_reply(req, NULL);
     v9fs_rwalk(req, NULL, NULL);
 
@@ -1311,7 +1368,7 @@ static void do_mkdir(QVirtio9P *v9p, const char *path, const char *cname)
     uint32_t fid;
     P9Req *req;
 
-    fid = do_walk(v9p, path);
+    fid = twalk((TWalkOpt) { .client = v9p, .path = path }).newfid;
 
     req = v9fs_tmkdir(v9p, fid, name, 0750, 0, 0);
     v9fs_req_wait_for_reply(req, NULL);
@@ -1326,7 +1383,7 @@ static uint32_t do_lcreate(QVirtio9P *v9p, const char *path,
     uint32_t fid;
     P9Req *req;
 
-    fid = do_walk(v9p, path);
+    fid = twalk((TWalkOpt) { .client = v9p, .path = path }).newfid;
 
     req = v9fs_tlcreate(v9p, fid, name, 0, 0750, 0, 0);
     v9fs_req_wait_for_reply(req, NULL);
@@ -1344,7 +1401,7 @@ static void do_symlink(QVirtio9P *v9p, const char *path, const char *clink,
     uint32_t fid;
     P9Req *req;
 
-    fid = do_walk(v9p, path);
+    fid = twalk((TWalkOpt) { .client = v9p, .path = path }).newfid;
 
     req = v9fs_tsymlink(v9p, fid, name, dst, 0, 0);
     v9fs_req_wait_for_reply(req, NULL);
@@ -1358,8 +1415,8 @@ static void do_hardlink(QVirtio9P *v9p, const char *path, const char *clink,
     uint32_t dfid, fid;
     P9Req *req;
 
-    dfid = do_walk(v9p, path);
-    fid = do_walk(v9p, to);
+    dfid = twalk((TWalkOpt) { .client = v9p, .path = path }).newfid;
+    fid = twalk((TWalkOpt) { .client = v9p, .path = to }).newfid;
 
     req = v9fs_tlink(v9p, dfid, fid, clink, 0);
     v9fs_req_wait_for_reply(req, NULL);
@@ -1373,7 +1430,7 @@ static void do_unlinkat(QVirtio9P *v9p, const char *atpath, const char *rpath,
     uint32_t fid;
     P9Req *req;
 
-    fid = do_walk(v9p, atpath);
+    fid = twalk((TWalkOpt) { .client = v9p, .path = atpath }).newfid;
 
     req = v9fs_tunlinkat(v9p, fid, name, flags, 0);
     v9fs_req_wait_for_reply(req, NULL);
-- 
2.30.2

Re: [RFC PATCH v2] tests/9p: introduce declarative function calls

[Christian Schoenebeck]

On Montag, 18. Juli 2022 15:10:55 CEST Christian Schoenebeck wrote:
> There are currently 4 different functions for sending a 9p 'Twalk'
> request. They are all doing the same thing, just in a slightly different
> way and with slightly different function arguments.
> 
> Merge those 4 functions into a single function by using a struct for
> function call arguments and use designated initializers when calling this
> function to turn usage into a declarative apporach, which is better
> readable and easier to maintain.
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
> 
> v1 -> v2:
> 
>   * Also merge low-level function v9fs_twalk().
> 
>   * Lower case twalk() function name.
> 
>   * Lower case rwalk struct field.
> 
>   * Add result struct TWalkRes.
> 
>   NOTE: I have not separated rwalk struct, because it would have
>   simplified code at one place, but complicated it at another one.

BTW, I also toyed around with virtio-9p-test.c -> virtio-9p-test.cpp, due to 
advantages described in v1 discussion, however there are quite a bunch of C 
header files included which would need refactoring (C++ keywords used like 
'export', 'class', 'private' and missing type casts from void*).

I also saw no easy way to separate those as alternative (like C low level 
unit, C++ unit on top). so I decided that it was not worth it.

>  tests/qtest/virtio-9p-test.c | 251 +++++++++++++++++++++--------------
>  1 file changed, 154 insertions(+), 97 deletions(-)
> 
> diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> index 25305a4cf7..69b1c27268 100644
> --- a/tests/qtest/virtio-9p-test.c
> +++ b/tests/qtest/virtio-9p-test.c
> @@ -72,6 +72,7 @@ static int split(const char *in, const char *delim, char
> ***out) static void split_free(char ***out)
>  {
>      int i;
> +    if (!*out) return;
>      for (i = 0; (*out)[i]; ++i) {
>          g_free((*out)[i]);
>      }
> @@ -390,31 +391,6 @@ static void v9fs_rattach(P9Req *req, v9fs_qid *qid)
>      v9fs_req_free(req);
>  }
> 
> -/* size[4] Twalk tag[2] fid[4] newfid[4] nwname[2] nwname*(wname[s]) */
> -static P9Req *v9fs_twalk(QVirtio9P *v9p, uint32_t fid, uint32_t newfid,
> -                         uint16_t nwname, char *const wnames[], uint16_t
> tag) -{
> -    P9Req *req;
> -    int i;
> -    uint32_t body_size = 4 + 4 + 2;
> -
> -    for (i = 0; i < nwname; i++) {
> -        uint16_t wname_size = v9fs_string_size(wnames[i]);
> -
> -        g_assert_cmpint(body_size, <=, UINT32_MAX - wname_size);
> -        body_size += wname_size;
> -    }
> -    req = v9fs_req_init(v9p,  body_size, P9_TWALK, tag);
> -    v9fs_uint32_write(req, fid);
> -    v9fs_uint32_write(req, newfid);
> -    v9fs_uint16_write(req, nwname);
> -    for (i = 0; i < nwname; i++) {
> -        v9fs_string_write(req, wnames[i]);
> -    }
> -    v9fs_req_send(req);
> -    return req;
> -}
> -
>  /* size[4] Rwalk tag[2] nwqid[2] nwqid*(wqid[13]) */
>  static void v9fs_rwalk(P9Req *req, uint16_t *nwqid, v9fs_qid **wqid)
>  {
> @@ -432,6 +408,98 @@ static void v9fs_rwalk(P9Req *req, uint16_t *nwqid,
> v9fs_qid **wqid) v9fs_req_free(req);
>  }
> 
> +/* options for 'Twalk' 9p request */
> +typedef struct TWalkOpt {
> +    /* 9P client being used (mandatory) */
> +    QVirtio9P *client;
> +    /* user supplied tag number being returned with response (optional) */
> +    uint16_t tag;
> +    /* file ID of directory from where walk should start (optional) */
> +    uint32_t fid;
> +    /* file ID for target directory being walked to (optional) */
> +    uint32_t newfid;
> +    /* low level variant of path to walk to (optional) */
> +    uint16_t nwname;
> +    char **wnames;
> +    /* high level variant of path to walk to (optional) */
> +    const char *path;
> +    /* data being received from 9p server as 'Rwalk' response (optional) */
> +    struct {
> +        uint16_t *nwqid;
> +        v9fs_qid **wqid;
> +    } rwalk;
> +    /* only send Twalk request but not wait for a reply? (optional) */
> +    bool requestOnly;
> +    /* do we expect an Rlerror response, if yes which error code?
> (optional) */ +    uint32_t expectErr;
> +} TWalkOpt;
> +
> +/* result of 'Twalk' 9p request */
> +typedef struct TWalkRes {
> +    /* file ID of target directory been walked to */
> +    uint32_t newfid;
> +    /* if requestOnly was set: request object for further processing */
> +    P9Req *req;
> +} TWalkRes;
> +
> +/* size[4] Twalk tag[2] fid[4] newfid[4] nwname[2] nwname*(wname[s]) */
> +static TWalkRes twalk(TWalkOpt opt)
> +{
> +    P9Req *req;
> +    int i;
> +    uint32_t body_size = 4 + 4 + 2;
> +    uint32_t err;
> +    char **wnames = NULL;
> +
> +    g_assert(opt.client);
> +    /* expecting either high- or low-level path, both not both */
> +    g_assert(!opt.path || !(opt.nwname || opt.wnames));
> +    /* expecting either Rwalk or Rlerror, but obviously not both */
> +    g_assert(!opt.expectErr || !(opt.rwalk.nwqid || opt.rwalk.wqid));
> +
> +    if (!opt.newfid) {
> +        opt.newfid = genfid();
> +    }
> +
> +    if (opt.path) {
> +        opt.nwname = split(opt.path, "/", &wnames);
> +        opt.wnames = wnames;
> +    }
> +
> +    for (i = 0; i < opt.nwname; i++) {
> +        uint16_t wname_size = v9fs_string_size(opt.wnames[i]);
> +
> +        g_assert_cmpint(body_size, <=, UINT32_MAX - wname_size);
> +        body_size += wname_size;
> +    }
> +    req = v9fs_req_init(opt.client, body_size, P9_TWALK, opt.tag);
> +    v9fs_uint32_write(req, opt.fid);
> +    v9fs_uint32_write(req, opt.newfid);
> +    v9fs_uint16_write(req, opt.nwname);
> +    for (i = 0; i < opt.nwname; i++) {
> +        v9fs_string_write(req, opt.wnames[i]);
> +    }
> +    v9fs_req_send(req);
> +
> +    if (!opt.requestOnly) {
> +        v9fs_req_wait_for_reply(req, NULL);
> +        if (opt.expectErr) {
> +            v9fs_rlerror(req, &err);
> +            g_assert_cmpint(err, ==, opt.expectErr);
> +        } else {
> +            v9fs_rwalk(req, opt.rwalk.nwqid, opt.rwalk.wqid);
> +        }
> +        req = NULL; /* request was freed */
> +    }
> +
> +    split_free(&wnames);
> +
> +    return (TWalkRes) {
> +        .newfid = opt.newfid,
> +        .req = req,
> +    };
> +}
> +
>  /* size[4] Tgetattr tag[2] fid[4] request_mask[8] */
>  static P9Req *v9fs_tgetattr(QVirtio9P *v9p, uint32_t fid, uint64_t
> request_mask, uint16_t tag)
> @@ -669,52 +737,6 @@ static void do_version(QVirtio9P *v9p)
>      g_assert_cmpmem(server_version, server_len, version, strlen(version));
>  }
> 
> -/*
> - * utility function: walk to requested dir and return fid for that dir and
> - * the QIDs of server response
> - */
> -static uint32_t do_walk_rqids(QVirtio9P *v9p, const char *path, uint16_t
> *nwqid, -                              v9fs_qid **wqid)
> -{
> -    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, nwqid, wqid);
> -
> -    split_free(&wnames);
> -    return fid;
> -}
> -
> -/* utility function: walk to requested dir and return fid for that dir */
> -static uint32_t do_walk(QVirtio9P *v9p, const char *path)
> -{
> -    return do_walk_rqids(v9p, path, NULL, NULL);
> -}
> -
> -/* utility function: walk to requested dir and expect passed error response
> */ -static void do_walk_expect_error(QVirtio9P *v9p, const char *path,
> uint32_t err) -{
> -    char **wnames;
> -    P9Req *req;
> -    uint32_t _err;
> -    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_rlerror(req, &_err);
> -
> -    g_assert_cmpint(_err, ==, err);
> -
> -    split_free(&wnames);
> -}
> -
>  static void fs_version(void *obj, void *data, QGuestAllocator *t_alloc)
>  {
>      alloc = t_alloc;
> @@ -757,7 +779,10 @@ static void fs_walk(void *obj, void *data,
> QGuestAllocator *t_alloc) }
> 
>      do_attach(v9p);
> -    req = v9fs_twalk(v9p, 0, 1, P9_MAXWELEM, wnames, 0);
> +    req = twalk((TWalkOpt) {
> +        .client = v9p, .fid = 0, .newfid = 1,
> +        .nwname = P9_MAXWELEM, .wnames = wnames, .requestOnly = true
> +    }).req;
>      v9fs_req_wait_for_reply(req, NULL);
>      v9fs_rwalk(req, &nwqid, &wqid);
> 
> @@ -941,7 +966,7 @@ static void fs_readdir(void *obj, void *data,
> QGuestAllocator *t_alloc) {
>      QVirtio9P *v9p = obj;
>      alloc = t_alloc;
> -    char *const wnames[] = { g_strdup(QTEST_V9FS_SYNTH_READDIR_DIR) };
> +    char *wnames[] = { g_strdup(QTEST_V9FS_SYNTH_READDIR_DIR) };
>      uint16_t nqid;
>      v9fs_qid qid;
>      uint32_t count, nentries;
> @@ -949,7 +974,10 @@ static void fs_readdir(void *obj, void *data,
> QGuestAllocator *t_alloc) P9Req *req;
> 
>      do_attach(v9p);
> -    req = v9fs_twalk(v9p, 0, 1, 1, wnames, 0);
> +    req = twalk((TWalkOpt) {
> +        .client = v9p, .fid = 0, .newfid = 1,
> +        .nwname = 1, .wnames = wnames, .requestOnly = true
> +    }).req;
>      v9fs_req_wait_for_reply(req, NULL);
>      v9fs_rwalk(req, &nqid, NULL);
>      g_assert_cmpint(nqid, ==, 1);
> @@ -993,7 +1021,7 @@ static void fs_readdir(void *obj, void *data,
> QGuestAllocator *t_alloc) /* readdir test where overall request is split
> over several messages */ static void do_readdir_split(QVirtio9P *v9p,
> uint32_t count)
>  {
> -    char *const wnames[] = { g_strdup(QTEST_V9FS_SYNTH_READDIR_DIR) };
> +    char *wnames[] = { g_strdup(QTEST_V9FS_SYNTH_READDIR_DIR) };
>      uint16_t nqid;
>      v9fs_qid qid;
>      uint32_t nentries, npartialentries;
> @@ -1010,7 +1038,10 @@ static void do_readdir_split(QVirtio9P *v9p, uint32_t
> count) nentries = 0;
>      tail = NULL;
> 
> -    req = v9fs_twalk(v9p, 0, fid, 1, wnames, 0);
> +    req = twalk((TWalkOpt) {
> +        .client = v9p, .fid = 0, .newfid = fid,
> +        .nwname = 1, .wnames = wnames, .requestOnly = true
> +    }).req;
>      v9fs_req_wait_for_reply(req, NULL);
>      v9fs_rwalk(req, &nqid, NULL);
>      g_assert_cmpint(nqid, ==, 1);
> @@ -1074,12 +1105,15 @@ static void fs_walk_no_slash(void *obj, void *data,
> QGuestAllocator *t_alloc) {
>      QVirtio9P *v9p = obj;
>      alloc = t_alloc;
> -    char *const wnames[] = { g_strdup(" /") };
> +    char *wnames[] = { g_strdup(" /") };
>      P9Req *req;
>      uint32_t err;
> 
>      do_attach(v9p);
> -    req = v9fs_twalk(v9p, 0, 1, 1, wnames, 0);
> +    req = twalk((TWalkOpt) {
> +        .client = v9p, .fid = 0, .newfid = 1,
> +        .nwname = 1, .wnames = wnames, .requestOnly = true
> +    }).req;
>      v9fs_req_wait_for_reply(req, NULL);
>      v9fs_rlerror(req, &err);
> 
> @@ -1098,7 +1132,9 @@ static void fs_walk_nonexistent(void *obj, void *data,
> QGuestAllocator *t_alloc) * The 9p2000 protocol spec says: "If the first
> element cannot be walked * for any reason, Rerror is returned."
>       */
> -    do_walk_expect_error(v9p, "non-existent", ENOENT);
> +    twalk((TWalkOpt) {
> +        .client = v9p, .path = "non-existent", .expectErr = ENOENT
> +    });
>  }
> 
>  static void fs_walk_2nd_nonexistent(void *obj, void *data,
> @@ -1116,7 +1152,10 @@ static void fs_walk_2nd_nonexistent(void *obj, void
> *data, );
> 
>      do_attach_rqid(v9p, &root_qid);
> -    fid = do_walk_rqids(v9p, path, &nwqid, &wqid);
> +    fid = twalk((TWalkOpt) {
> +        .client = v9p, .path = path,
> +        .rwalk.nwqid = &nwqid, .rwalk.wqid = &wqid
> +    }).newfid;
>      /*
>       * The 9p2000 protocol spec says: "nwqid is therefore either nwname or
> the * index of the first elementwise walk that failed."
> @@ -1148,7 +1187,10 @@ static void fs_walk_none(void *obj, void *data,
> QGuestAllocator *t_alloc) v9fs_req_wait_for_reply(req, NULL);
>      v9fs_rattach(req, &root_qid);
> 
> -    req = v9fs_twalk(v9p, 0, 1, 0, NULL, 0);
> +    req = twalk((TWalkOpt) {
> +        .client = v9p, .fid = 0, .newfid = 1,
> +        .nwname = 0, .wnames = NULL, .requestOnly = true
> +    }).req;
>      v9fs_req_wait_for_reply(req, NULL);
>      v9fs_rwalk(req, NULL, &wqid);
> 
> @@ -1166,7 +1208,7 @@ static void fs_walk_dotdot(void *obj, void *data,
> QGuestAllocator *t_alloc) {
>      QVirtio9P *v9p = obj;
>      alloc = t_alloc;
> -    char *const wnames[] = { g_strdup("..") };
> +    char *wnames[] = { g_strdup("..") };
>      v9fs_qid root_qid;
>      g_autofree v9fs_qid *wqid = NULL;
>      P9Req *req;
> @@ -1176,7 +1218,10 @@ static void fs_walk_dotdot(void *obj, void *data,
> QGuestAllocator *t_alloc) v9fs_req_wait_for_reply(req, NULL);
>      v9fs_rattach(req, &root_qid);
> 
> -    req = v9fs_twalk(v9p, 0, 1, 1, wnames, 0);
> +    req = twalk((TWalkOpt) {
> +        .client = v9p, .fid = 0, .newfid = 1,
> +        .nwname = 1, .wnames = wnames, .requestOnly = true
> +    }).req;
>      v9fs_req_wait_for_reply(req, NULL);
>      v9fs_rwalk(req, NULL, &wqid); /* We now we'll get one qid */
> 
> @@ -1189,11 +1234,14 @@ static void fs_lopen(void *obj, void *data,
> QGuestAllocator *t_alloc) {
>      QVirtio9P *v9p = obj;
>      alloc = t_alloc;
> -    char *const wnames[] = { g_strdup(QTEST_V9FS_SYNTH_LOPEN_FILE) };
> +    char *wnames[] = { g_strdup(QTEST_V9FS_SYNTH_LOPEN_FILE) };
>      P9Req *req;
> 
>      do_attach(v9p);
> -    req = v9fs_twalk(v9p, 0, 1, 1, wnames, 0);
> +    req = twalk((TWalkOpt) {
> +        .client = v9p, .fid = 0, .newfid = 1,
> +        .nwname = 1, .wnames = wnames, .requestOnly = true
> +    }).req;
>      v9fs_req_wait_for_reply(req, NULL);
>      v9fs_rwalk(req, NULL, NULL);
> 
> @@ -1209,13 +1257,16 @@ static void fs_write(void *obj, void *data,
> QGuestAllocator *t_alloc) QVirtio9P *v9p = obj;
>      alloc = t_alloc;
>      static const uint32_t write_count = P9_MAX_SIZE / 2;
> -    char *const wnames[] = { g_strdup(QTEST_V9FS_SYNTH_WRITE_FILE) };
> +    char *wnames[] = { g_strdup(QTEST_V9FS_SYNTH_WRITE_FILE) };
>      g_autofree char *buf = g_malloc0(write_count);
>      uint32_t count;
>      P9Req *req;
> 
>      do_attach(v9p);
> -    req = v9fs_twalk(v9p, 0, 1, 1, wnames, 0);
> +    req = twalk((TWalkOpt) {
> +        .client = v9p, .fid = 0, .newfid = 1,
> +        .nwname = 1, .wnames = wnames, .requestOnly = true
> +    }).req;
>      v9fs_req_wait_for_reply(req, NULL);
>      v9fs_rwalk(req, NULL, NULL);
> 
> @@ -1235,13 +1286,16 @@ static void fs_flush_success(void *obj, void *data,
> QGuestAllocator *t_alloc) {
>      QVirtio9P *v9p = obj;
>      alloc = t_alloc;
> -    char *const wnames[] = { g_strdup(QTEST_V9FS_SYNTH_FLUSH_FILE) };
> +    char *wnames[] = { g_strdup(QTEST_V9FS_SYNTH_FLUSH_FILE) };
>      P9Req *req, *flush_req;
>      uint32_t reply_len;
>      uint8_t should_block;
> 
>      do_attach(v9p);
> -    req = v9fs_twalk(v9p, 0, 1, 1, wnames, 0);
> +    req = twalk((TWalkOpt) {
> +        .client = v9p, .fid = 0, .newfid = 1,
> +        .nwname = 1, .wnames = wnames, .requestOnly = true
> +    }).req;
>      v9fs_req_wait_for_reply(req, NULL);
>      v9fs_rwalk(req, NULL, NULL);
> 
> @@ -1272,13 +1326,16 @@ static void fs_flush_ignored(void *obj, void *data,
> QGuestAllocator *t_alloc) {
>      QVirtio9P *v9p = obj;
>      alloc = t_alloc;
> -    char *const wnames[] = { g_strdup(QTEST_V9FS_SYNTH_FLUSH_FILE) };
> +    char *wnames[] = { g_strdup(QTEST_V9FS_SYNTH_FLUSH_FILE) };
>      P9Req *req, *flush_req;
>      uint32_t count;
>      uint8_t should_block;
> 
>      do_attach(v9p);
> -    req = v9fs_twalk(v9p, 0, 1, 1, wnames, 0);
> +    req = twalk((TWalkOpt) {
> +        .client = v9p, .fid = 0, .newfid = 1,
> +        .nwname = 1, .wnames = wnames, .requestOnly = true
> +    }).req;
>      v9fs_req_wait_for_reply(req, NULL);
>      v9fs_rwalk(req, NULL, NULL);
> 
> @@ -1311,7 +1368,7 @@ static void do_mkdir(QVirtio9P *v9p, const char *path,
> const char *cname) uint32_t fid;
>      P9Req *req;
> 
> -    fid = do_walk(v9p, path);
> +    fid = twalk((TWalkOpt) { .client = v9p, .path = path }).newfid;
> 
>      req = v9fs_tmkdir(v9p, fid, name, 0750, 0, 0);
>      v9fs_req_wait_for_reply(req, NULL);
> @@ -1326,7 +1383,7 @@ static uint32_t do_lcreate(QVirtio9P *v9p, const char
> *path, uint32_t fid;
>      P9Req *req;
> 
> -    fid = do_walk(v9p, path);
> +    fid = twalk((TWalkOpt) { .client = v9p, .path = path }).newfid;
> 
>      req = v9fs_tlcreate(v9p, fid, name, 0, 0750, 0, 0);
>      v9fs_req_wait_for_reply(req, NULL);
> @@ -1344,7 +1401,7 @@ static void do_symlink(QVirtio9P *v9p, const char
> *path, const char *clink, uint32_t fid;
>      P9Req *req;
> 
> -    fid = do_walk(v9p, path);
> +    fid = twalk((TWalkOpt) { .client = v9p, .path = path }).newfid;
> 
>      req = v9fs_tsymlink(v9p, fid, name, dst, 0, 0);
>      v9fs_req_wait_for_reply(req, NULL);
> @@ -1358,8 +1415,8 @@ static void do_hardlink(QVirtio9P *v9p, const char
> *path, const char *clink, uint32_t dfid, fid;
>      P9Req *req;
> 
> -    dfid = do_walk(v9p, path);
> -    fid = do_walk(v9p, to);
> +    dfid = twalk((TWalkOpt) { .client = v9p, .path = path }).newfid;
> +    fid = twalk((TWalkOpt) { .client = v9p, .path = to }).newfid;
> 
>      req = v9fs_tlink(v9p, dfid, fid, clink, 0);
>      v9fs_req_wait_for_reply(req, NULL);
> @@ -1373,7 +1430,7 @@ static void do_unlinkat(QVirtio9P *v9p, const char
> *atpath, const char *rpath, uint32_t fid;
>      P9Req *req;
> 
> -    fid = do_walk(v9p, atpath);
> +    fid = twalk((TWalkOpt) { .client = v9p, .path = atpath }).newfid;
> 
>      req = v9fs_tunlinkat(v9p, fid, name, flags, 0);
>      v9fs_req_wait_for_reply(req, NULL);

Re: [RFC PATCH v2] tests/9p: introduce declarative function calls

[Christian Schoenebeck]

On Montag, 18. Juli 2022 16:02:31 CEST Christian Schoenebeck wrote:
> On Montag, 18. Juli 2022 15:10:55 CEST Christian Schoenebeck wrote:
> > There are currently 4 different functions for sending a 9p 'Twalk'
> > request. They are all doing the same thing, just in a slightly different
> > way and with slightly different function arguments.
> > 
> > Merge those 4 functions into a single function by using a struct for
> > function call arguments and use designated initializers when calling this
> > function to turn usage into a declarative apporach, which is better
> > readable and easier to maintain.
> > 
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> > 
> > v1 -> v2:
> >   * Also merge low-level function v9fs_twalk().
> >   
> >   * Lower case twalk() function name.
> >   
> >   * Lower case rwalk struct field.
> >   
> >   * Add result struct TWalkRes.
> >   
> >   NOTE: I have not separated rwalk struct, because it would have
> >   simplified code at one place, but complicated it at another one.
> 
> BTW, I also toyed around with virtio-9p-test.c -> virtio-9p-test.cpp, due to
> advantages described in v1 discussion, however there are quite a bunch of C
> header files included which would need refactoring (C++ keywords used like
> 'export', 'class', 'private' and missing type casts from void*).
> 
> I also saw no easy way to separate those as alternative (like C low level
> unit, C++ unit on top). so I decided that it was not worth it.

Not sure if you are on summer vacation right now, but I guess I just go ahead 
and convert the rest of the 9p test code in the same way? At least I haven't 
seen that you were opposed about the suggested idea in general.

Best regards,
Christian Schoenebeck

> >  tests/qtest/virtio-9p-test.c | 251 +++++++++++++++++++++--------------
> >  1 file changed, 154 insertions(+), 97 deletions(-)
> > 
> > diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> > index 25305a4cf7..69b1c27268 100644
> > --- a/tests/qtest/virtio-9p-test.c
> > +++ b/tests/qtest/virtio-9p-test.c
> > @@ -72,6 +72,7 @@ static int split(const char *in, const char *delim, char
> > ***out) static void split_free(char ***out)
> > 
> >  {
> >  
> >      int i;
> > 
> > +    if (!*out) return;
> > 
> >      for (i = 0; (*out)[i]; ++i) {
> >      
> >          g_free((*out)[i]);
> >      
> >      }
> > 
> > @@ -390,31 +391,6 @@ static void v9fs_rattach(P9Req *req, v9fs_qid *qid)
> > 
> >      v9fs_req_free(req);
> >  
> >  }
> > 
> > -/* size[4] Twalk tag[2] fid[4] newfid[4] nwname[2] nwname*(wname[s]) */
> > -static P9Req *v9fs_twalk(QVirtio9P *v9p, uint32_t fid, uint32_t newfid,
> > -                         uint16_t nwname, char *const wnames[], uint16_t
> > tag) -{
> > -    P9Req *req;
> > -    int i;
> > -    uint32_t body_size = 4 + 4 + 2;
> > -
> > -    for (i = 0; i < nwname; i++) {
> > -        uint16_t wname_size = v9fs_string_size(wnames[i]);
> > -
> > -        g_assert_cmpint(body_size, <=, UINT32_MAX - wname_size);
> > -        body_size += wname_size;
> > -    }
> > -    req = v9fs_req_init(v9p,  body_size, P9_TWALK, tag);
> > -    v9fs_uint32_write(req, fid);
> > -    v9fs_uint32_write(req, newfid);
> > -    v9fs_uint16_write(req, nwname);
> > -    for (i = 0; i < nwname; i++) {
> > -        v9fs_string_write(req, wnames[i]);
> > -    }
> > -    v9fs_req_send(req);
> > -    return req;
> > -}
> > -
> > 
> >  /* size[4] Rwalk tag[2] nwqid[2] nwqid*(wqid[13]) */
> >  static void v9fs_rwalk(P9Req *req, uint16_t *nwqid, v9fs_qid **wqid)
> >  {
> > 
> > @@ -432,6 +408,98 @@ static void v9fs_rwalk(P9Req *req, uint16_t *nwqid,
> > v9fs_qid **wqid) v9fs_req_free(req);
> > 
> >  }
> > 
> > +/* options for 'Twalk' 9p request */
> > +typedef struct TWalkOpt {
> > +    /* 9P client being used (mandatory) */
> > +    QVirtio9P *client;
> > +    /* user supplied tag number being returned with response (optional)
> > */
> > +    uint16_t tag;
> > +    /* file ID of directory from where walk should start (optional) */
> > +    uint32_t fid;
> > +    /* file ID for target directory being walked to (optional) */
> > +    uint32_t newfid;
> > +    /* low level variant of path to walk to (optional) */
> > +    uint16_t nwname;
> > +    char **wnames;
> > +    /* high level variant of path to walk to (optional) */
> > +    const char *path;
> > +    /* data being received from 9p server as 'Rwalk' response (optional)
> > */ +    struct {
> > +        uint16_t *nwqid;
> > +        v9fs_qid **wqid;
> > +    } rwalk;
> > +    /* only send Twalk request but not wait for a reply? (optional) */
> > +    bool requestOnly;
> > +    /* do we expect an Rlerror response, if yes which error code?
> > (optional) */ +    uint32_t expectErr;
> > +} TWalkOpt;
> > +
> > +/* result of 'Twalk' 9p request */
> > +typedef struct TWalkRes {
> > +    /* file ID of target directory been walked to */
> > +    uint32_t newfid;
> > +    /* if requestOnly was set: request object for further processing */
> > +    P9Req *req;
> > +} TWalkRes;
> > +
> > +/* size[4] Twalk tag[2] fid[4] newfid[4] nwname[2] nwname*(wname[s]) */
> > +static TWalkRes twalk(TWalkOpt opt)
> > +{
> > +    P9Req *req;
> > +    int i;
> > +    uint32_t body_size = 4 + 4 + 2;
> > +    uint32_t err;
> > +    char **wnames = NULL;
> > +
> > +    g_assert(opt.client);
> > +    /* expecting either high- or low-level path, both not both */
> > +    g_assert(!opt.path || !(opt.nwname || opt.wnames));
> > +    /* expecting either Rwalk or Rlerror, but obviously not both */
> > +    g_assert(!opt.expectErr || !(opt.rwalk.nwqid || opt.rwalk.wqid));
> > +
> > +    if (!opt.newfid) {
> > +        opt.newfid = genfid();
> > +    }
> > +
> > +    if (opt.path) {
> > +        opt.nwname = split(opt.path, "/", &wnames);
> > +        opt.wnames = wnames;
> > +    }
> > +
> > +    for (i = 0; i < opt.nwname; i++) {
> > +        uint16_t wname_size = v9fs_string_size(opt.wnames[i]);
> > +
> > +        g_assert_cmpint(body_size, <=, UINT32_MAX - wname_size);
> > +        body_size += wname_size;
> > +    }
> > +    req = v9fs_req_init(opt.client, body_size, P9_TWALK, opt.tag);
> > +    v9fs_uint32_write(req, opt.fid);
> > +    v9fs_uint32_write(req, opt.newfid);
> > +    v9fs_uint16_write(req, opt.nwname);
> > +    for (i = 0; i < opt.nwname; i++) {
> > +        v9fs_string_write(req, opt.wnames[i]);
> > +    }
> > +    v9fs_req_send(req);
> > +
> > +    if (!opt.requestOnly) {
> > +        v9fs_req_wait_for_reply(req, NULL);
> > +        if (opt.expectErr) {
> > +            v9fs_rlerror(req, &err);
> > +            g_assert_cmpint(err, ==, opt.expectErr);
> > +        } else {
> > +            v9fs_rwalk(req, opt.rwalk.nwqid, opt.rwalk.wqid);
> > +        }
> > +        req = NULL; /* request was freed */
> > +    }
> > +
> > +    split_free(&wnames);
> > +
> > +    return (TWalkRes) {
> > +        .newfid = opt.newfid,
> > +        .req = req,
> > +    };
> > +}
> > +
> > 
> >  /* size[4] Tgetattr tag[2] fid[4] request_mask[8] */
> >  static P9Req *v9fs_tgetattr(QVirtio9P *v9p, uint32_t fid, uint64_t
> > 
> > request_mask, uint16_t tag)
> > @@ -669,52 +737,6 @@ static void do_version(QVirtio9P *v9p)
> > 
> >      g_assert_cmpmem(server_version, server_len, version,
> >      strlen(version));
> >  
> >  }
> > 
> > -/*
> > - * utility function: walk to requested dir and return fid for that dir
> > and
> > - * the QIDs of server response
> > - */
> > -static uint32_t do_walk_rqids(QVirtio9P *v9p, const char *path, uint16_t
> > *nwqid, -                              v9fs_qid **wqid)
> > -{
> > -    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, nwqid, wqid);
> > -
> > -    split_free(&wnames);
> > -    return fid;
> > -}
> > -
> > -/* utility function: walk to requested dir and return fid for that dir */
> > -static uint32_t do_walk(QVirtio9P *v9p, const char *path)
> > -{
> > -    return do_walk_rqids(v9p, path, NULL, NULL);
> > -}
> > -
> > -/* utility function: walk to requested dir and expect passed error
> > response */ -static void do_walk_expect_error(QVirtio9P *v9p, const char
> > *path, uint32_t err) -{
> > -    char **wnames;
> > -    P9Req *req;
> > -    uint32_t _err;
> > -    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_rlerror(req, &_err);
> > -
> > -    g_assert_cmpint(_err, ==, err);
> > -
> > -    split_free(&wnames);
> > -}
> > -
> > 
> >  static void fs_version(void *obj, void *data, QGuestAllocator *t_alloc)
> >  {
> >  
> >      alloc = t_alloc;
> > 
> > @@ -757,7 +779,10 @@ static void fs_walk(void *obj, void *data,
> > QGuestAllocator *t_alloc) }
> > 
> >      do_attach(v9p);
> > 
> > -    req = v9fs_twalk(v9p, 0, 1, P9_MAXWELEM, wnames, 0);
> > +    req = twalk((TWalkOpt) {
> > +        .client = v9p, .fid = 0, .newfid = 1,
> > +        .nwname = P9_MAXWELEM, .wnames = wnames, .requestOnly = true
> > +    }).req;
> > 
> >      v9fs_req_wait_for_reply(req, NULL);
> >      v9fs_rwalk(req, &nwqid, &wqid);
> > 
> > @@ -941,7 +966,7 @@ static void fs_readdir(void *obj, void *data,
> > QGuestAllocator *t_alloc) {
> > 
> >      QVirtio9P *v9p = obj;
> >      alloc = t_alloc;
> > 
> > -    char *const wnames[] = { g_strdup(QTEST_V9FS_SYNTH_READDIR_DIR) };
> > +    char *wnames[] = { g_strdup(QTEST_V9FS_SYNTH_READDIR_DIR) };
> > 
> >      uint16_t nqid;
> >      v9fs_qid qid;
> >      uint32_t count, nentries;
> > 
> > @@ -949,7 +974,10 @@ static void fs_readdir(void *obj, void *data,
> > QGuestAllocator *t_alloc) P9Req *req;
> > 
> >      do_attach(v9p);
> > 
> > -    req = v9fs_twalk(v9p, 0, 1, 1, wnames, 0);
> > +    req = twalk((TWalkOpt) {
> > +        .client = v9p, .fid = 0, .newfid = 1,
> > +        .nwname = 1, .wnames = wnames, .requestOnly = true
> > +    }).req;
> > 
> >      v9fs_req_wait_for_reply(req, NULL);
> >      v9fs_rwalk(req, &nqid, NULL);
> >      g_assert_cmpint(nqid, ==, 1);
> > 
> > @@ -993,7 +1021,7 @@ static void fs_readdir(void *obj, void *data,
> > QGuestAllocator *t_alloc) /* readdir test where overall request is split
> > over several messages */ static void do_readdir_split(QVirtio9P *v9p,
> > uint32_t count)
> > 
> >  {
> > 
> > -    char *const wnames[] = { g_strdup(QTEST_V9FS_SYNTH_READDIR_DIR) };
> > +    char *wnames[] = { g_strdup(QTEST_V9FS_SYNTH_READDIR_DIR) };
> > 
> >      uint16_t nqid;
> >      v9fs_qid qid;
> >      uint32_t nentries, npartialentries;
> > 
> > @@ -1010,7 +1038,10 @@ static void do_readdir_split(QVirtio9P *v9p,
> > uint32_t count) nentries = 0;
> > 
> >      tail = NULL;
> > 
> > -    req = v9fs_twalk(v9p, 0, fid, 1, wnames, 0);
> > +    req = twalk((TWalkOpt) {
> > +        .client = v9p, .fid = 0, .newfid = fid,
> > +        .nwname = 1, .wnames = wnames, .requestOnly = true
> > +    }).req;
> > 
> >      v9fs_req_wait_for_reply(req, NULL);
> >      v9fs_rwalk(req, &nqid, NULL);
> >      g_assert_cmpint(nqid, ==, 1);
> > 
> > @@ -1074,12 +1105,15 @@ static void fs_walk_no_slash(void *obj, void
> > *data,
> > QGuestAllocator *t_alloc) {
> > 
> >      QVirtio9P *v9p = obj;
> >      alloc = t_alloc;
> > 
> > -    char *const wnames[] = { g_strdup(" /") };
> > +    char *wnames[] = { g_strdup(" /") };
> > 
> >      P9Req *req;
> >      uint32_t err;
> >      
> >      do_attach(v9p);
> > 
> > -    req = v9fs_twalk(v9p, 0, 1, 1, wnames, 0);
> > +    req = twalk((TWalkOpt) {
> > +        .client = v9p, .fid = 0, .newfid = 1,
> > +        .nwname = 1, .wnames = wnames, .requestOnly = true
> > +    }).req;
> > 
> >      v9fs_req_wait_for_reply(req, NULL);
> >      v9fs_rlerror(req, &err);
> > 
> > @@ -1098,7 +1132,9 @@ static void fs_walk_nonexistent(void *obj, void
> > *data, QGuestAllocator *t_alloc) * The 9p2000 protocol spec says: "If the
> > first element cannot be walked * for any reason, Rerror is returned."
> > 
> >       */
> > 
> > -    do_walk_expect_error(v9p, "non-existent", ENOENT);
> > +    twalk((TWalkOpt) {
> > +        .client = v9p, .path = "non-existent", .expectErr = ENOENT
> > +    });
> > 
> >  }
> >  
> >  static void fs_walk_2nd_nonexistent(void *obj, void *data,
> > 
> > @@ -1116,7 +1152,10 @@ static void fs_walk_2nd_nonexistent(void *obj, void
> > *data, );
> > 
> >      do_attach_rqid(v9p, &root_qid);
> > 
> > -    fid = do_walk_rqids(v9p, path, &nwqid, &wqid);
> > +    fid = twalk((TWalkOpt) {
> > +        .client = v9p, .path = path,
> > +        .rwalk.nwqid = &nwqid, .rwalk.wqid = &wqid
> > +    }).newfid;
> > 
> >      /*
> >      
> >       * The 9p2000 protocol spec says: "nwqid is therefore either nwname
> >       or
> > 
> > the * index of the first elementwise walk that failed."
> > @@ -1148,7 +1187,10 @@ static void fs_walk_none(void *obj, void *data,
> > QGuestAllocator *t_alloc) v9fs_req_wait_for_reply(req, NULL);
> > 
> >      v9fs_rattach(req, &root_qid);
> > 
> > -    req = v9fs_twalk(v9p, 0, 1, 0, NULL, 0);
> > +    req = twalk((TWalkOpt) {
> > +        .client = v9p, .fid = 0, .newfid = 1,
> > +        .nwname = 0, .wnames = NULL, .requestOnly = true
> > +    }).req;
> > 
> >      v9fs_req_wait_for_reply(req, NULL);
> >      v9fs_rwalk(req, NULL, &wqid);
> > 
> > @@ -1166,7 +1208,7 @@ static void fs_walk_dotdot(void *obj, void *data,
> > QGuestAllocator *t_alloc) {
> > 
> >      QVirtio9P *v9p = obj;
> >      alloc = t_alloc;
> > 
> > -    char *const wnames[] = { g_strdup("..") };
> > +    char *wnames[] = { g_strdup("..") };
> > 
> >      v9fs_qid root_qid;
> >      g_autofree v9fs_qid *wqid = NULL;
> >      P9Req *req;
> > 
> > @@ -1176,7 +1218,10 @@ static void fs_walk_dotdot(void *obj, void *data,
> > QGuestAllocator *t_alloc) v9fs_req_wait_for_reply(req, NULL);
> > 
> >      v9fs_rattach(req, &root_qid);
> > 
> > -    req = v9fs_twalk(v9p, 0, 1, 1, wnames, 0);
> > +    req = twalk((TWalkOpt) {
> > +        .client = v9p, .fid = 0, .newfid = 1,
> > +        .nwname = 1, .wnames = wnames, .requestOnly = true
> > +    }).req;
> > 
> >      v9fs_req_wait_for_reply(req, NULL);
> >      v9fs_rwalk(req, NULL, &wqid); /* We now we'll get one qid */
> > 
> > @@ -1189,11 +1234,14 @@ static void fs_lopen(void *obj, void *data,
> > QGuestAllocator *t_alloc) {
> > 
> >      QVirtio9P *v9p = obj;
> >      alloc = t_alloc;
> > 
> > -    char *const wnames[] = { g_strdup(QTEST_V9FS_SYNTH_LOPEN_FILE) };
> > +    char *wnames[] = { g_strdup(QTEST_V9FS_SYNTH_LOPEN_FILE) };
> > 
> >      P9Req *req;
> >      
> >      do_attach(v9p);
> > 
> > -    req = v9fs_twalk(v9p, 0, 1, 1, wnames, 0);
> > +    req = twalk((TWalkOpt) {
> > +        .client = v9p, .fid = 0, .newfid = 1,
> > +        .nwname = 1, .wnames = wnames, .requestOnly = true
> > +    }).req;
> > 
> >      v9fs_req_wait_for_reply(req, NULL);
> >      v9fs_rwalk(req, NULL, NULL);
> > 
> > @@ -1209,13 +1257,16 @@ static void fs_write(void *obj, void *data,
> > QGuestAllocator *t_alloc) QVirtio9P *v9p = obj;
> > 
> >      alloc = t_alloc;
> >      static const uint32_t write_count = P9_MAX_SIZE / 2;
> > 
> > -    char *const wnames[] = { g_strdup(QTEST_V9FS_SYNTH_WRITE_FILE) };
> > +    char *wnames[] = { g_strdup(QTEST_V9FS_SYNTH_WRITE_FILE) };
> > 
> >      g_autofree char *buf = g_malloc0(write_count);
> >      uint32_t count;
> >      P9Req *req;
> >      
> >      do_attach(v9p);
> > 
> > -    req = v9fs_twalk(v9p, 0, 1, 1, wnames, 0);
> > +    req = twalk((TWalkOpt) {
> > +        .client = v9p, .fid = 0, .newfid = 1,
> > +        .nwname = 1, .wnames = wnames, .requestOnly = true
> > +    }).req;
> > 
> >      v9fs_req_wait_for_reply(req, NULL);
> >      v9fs_rwalk(req, NULL, NULL);
> > 
> > @@ -1235,13 +1286,16 @@ static void fs_flush_success(void *obj, void
> > *data,
> > QGuestAllocator *t_alloc) {
> > 
> >      QVirtio9P *v9p = obj;
> >      alloc = t_alloc;
> > 
> > -    char *const wnames[] = { g_strdup(QTEST_V9FS_SYNTH_FLUSH_FILE) };
> > +    char *wnames[] = { g_strdup(QTEST_V9FS_SYNTH_FLUSH_FILE) };
> > 
> >      P9Req *req, *flush_req;
> >      uint32_t reply_len;
> >      uint8_t should_block;
> >      
> >      do_attach(v9p);
> > 
> > -    req = v9fs_twalk(v9p, 0, 1, 1, wnames, 0);
> > +    req = twalk((TWalkOpt) {
> > +        .client = v9p, .fid = 0, .newfid = 1,
> > +        .nwname = 1, .wnames = wnames, .requestOnly = true
> > +    }).req;
> > 
> >      v9fs_req_wait_for_reply(req, NULL);
> >      v9fs_rwalk(req, NULL, NULL);
> > 
> > @@ -1272,13 +1326,16 @@ static void fs_flush_ignored(void *obj, void
> > *data,
> > QGuestAllocator *t_alloc) {
> > 
> >      QVirtio9P *v9p = obj;
> >      alloc = t_alloc;
> > 
> > -    char *const wnames[] = { g_strdup(QTEST_V9FS_SYNTH_FLUSH_FILE) };
> > +    char *wnames[] = { g_strdup(QTEST_V9FS_SYNTH_FLUSH_FILE) };
> > 
> >      P9Req *req, *flush_req;
> >      uint32_t count;
> >      uint8_t should_block;
> >      
> >      do_attach(v9p);
> > 
> > -    req = v9fs_twalk(v9p, 0, 1, 1, wnames, 0);
> > +    req = twalk((TWalkOpt) {
> > +        .client = v9p, .fid = 0, .newfid = 1,
> > +        .nwname = 1, .wnames = wnames, .requestOnly = true
> > +    }).req;
> > 
> >      v9fs_req_wait_for_reply(req, NULL);
> >      v9fs_rwalk(req, NULL, NULL);
> > 
> > @@ -1311,7 +1368,7 @@ static void do_mkdir(QVirtio9P *v9p, const char
> > *path, const char *cname) uint32_t fid;
> > 
> >      P9Req *req;
> > 
> > -    fid = do_walk(v9p, path);
> > +    fid = twalk((TWalkOpt) { .client = v9p, .path = path }).newfid;
> > 
> >      req = v9fs_tmkdir(v9p, fid, name, 0750, 0, 0);
> >      v9fs_req_wait_for_reply(req, NULL);
> > 
> > @@ -1326,7 +1383,7 @@ static uint32_t do_lcreate(QVirtio9P *v9p, const
> > char
> > *path, uint32_t fid;
> > 
> >      P9Req *req;
> > 
> > -    fid = do_walk(v9p, path);
> > +    fid = twalk((TWalkOpt) { .client = v9p, .path = path }).newfid;
> > 
> >      req = v9fs_tlcreate(v9p, fid, name, 0, 0750, 0, 0);
> >      v9fs_req_wait_for_reply(req, NULL);
> > 
> > @@ -1344,7 +1401,7 @@ static void do_symlink(QVirtio9P *v9p, const char
> > *path, const char *clink, uint32_t fid;
> > 
> >      P9Req *req;
> > 
> > -    fid = do_walk(v9p, path);
> > +    fid = twalk((TWalkOpt) { .client = v9p, .path = path }).newfid;
> > 
> >      req = v9fs_tsymlink(v9p, fid, name, dst, 0, 0);
> >      v9fs_req_wait_for_reply(req, NULL);
> > 
> > @@ -1358,8 +1415,8 @@ static void do_hardlink(QVirtio9P *v9p, const char
> > *path, const char *clink, uint32_t dfid, fid;
> > 
> >      P9Req *req;
> > 
> > -    dfid = do_walk(v9p, path);
> > -    fid = do_walk(v9p, to);
> > +    dfid = twalk((TWalkOpt) { .client = v9p, .path = path }).newfid;
> > +    fid = twalk((TWalkOpt) { .client = v9p, .path = to }).newfid;
> > 
> >      req = v9fs_tlink(v9p, dfid, fid, clink, 0);
> >      v9fs_req_wait_for_reply(req, NULL);
> > 
> > @@ -1373,7 +1430,7 @@ static void do_unlinkat(QVirtio9P *v9p, const char
> > *atpath, const char *rpath, uint32_t fid;
> > 
> >      P9Req *req;
> > 
> > -    fid = do_walk(v9p, atpath);
> > +    fid = twalk((TWalkOpt) { .client = v9p, .path = atpath }).newfid;
> > 
> >      req = v9fs_tunlinkat(v9p, fid, name, flags, 0);
> >      v9fs_req_wait_for_reply(req, NULL);

Re: [RFC PATCH v2] tests/9p: introduce declarative function calls

[Greg Kurz]

Hi Christian,

On Thu, 18 Aug 2022 16:13:40 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Montag, 18. Juli 2022 16:02:31 CEST Christian Schoenebeck wrote:
> > On Montag, 18. Juli 2022 15:10:55 CEST Christian Schoenebeck wrote:
> > > There are currently 4 different functions for sending a 9p 'Twalk'
> > > request. They are all doing the same thing, just in a slightly different
> > > way and with slightly different function arguments.
> > > 
> > > Merge those 4 functions into a single function by using a struct for
> > > function call arguments and use designated initializers when calling this
> > > function to turn usage into a declarative apporach, which is better
> > > readable and easier to maintain.
> > > 
> > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > ---
> > > 
> > > v1 -> v2:
> > >   * Also merge low-level function v9fs_twalk().
> > >   
> > >   * Lower case twalk() function name.
> > >   
> > >   * Lower case rwalk struct field.
> > >   
> > >   * Add result struct TWalkRes.
> > >   
> > >   NOTE: I have not separated rwalk struct, because it would have
> > >   simplified code at one place, but complicated it at another one.
> > 
> > BTW, I also toyed around with virtio-9p-test.c -> virtio-9p-test.cpp, due to
> > advantages described in v1 discussion, however there are quite a bunch of C
> > header files included which would need refactoring (C++ keywords used like
> > 'export', 'class', 'private' and missing type casts from void*).
> > 
> > I also saw no easy way to separate those as alternative (like C low level
> > unit, C++ unit on top). so I decided that it was not worth it.
> 
> Not sure if you are on summer vacation right now, but I guess I just go ahead 
> and convert the rest of the 9p test code in the same way? At least I haven't 
> seen that you were opposed about the suggested idea in general.
> 

Yeah I was on vacation indeed. Please go ahead. I'll do my best to
review.

Cheers,

--
Greg

> Best regards,
> Christian Schoenebeck
> 
> > >  tests/qtest/virtio-9p-test.c | 251 +++++++++++++++++++++--------------
> > >  1 file changed, 154 insertions(+), 97 deletions(-)
> > > 
> > > diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> > > index 25305a4cf7..69b1c27268 100644
> > > --- a/tests/qtest/virtio-9p-test.c
> > > +++ b/tests/qtest/virtio-9p-test.c
> > > @@ -72,6 +72,7 @@ static int split(const char *in, const char *delim, char
> > > ***out) static void split_free(char ***out)
> > > 
> > >  {
> > >  
> > >      int i;
> > > 
> > > +    if (!*out) return;
> > > 
> > >      for (i = 0; (*out)[i]; ++i) {
> > >      
> > >          g_free((*out)[i]);
> > >      
> > >      }
> > > 
> > > @@ -390,31 +391,6 @@ static void v9fs_rattach(P9Req *req, v9fs_qid *qid)
> > > 
> > >      v9fs_req_free(req);
> > >  
> > >  }
> > > 
> > > -/* size[4] Twalk tag[2] fid[4] newfid[4] nwname[2] nwname*(wname[s]) */
> > > -static P9Req *v9fs_twalk(QVirtio9P *v9p, uint32_t fid, uint32_t newfid,
> > > -                         uint16_t nwname, char *const wnames[], uint16_t
> > > tag) -{
> > > -    P9Req *req;
> > > -    int i;
> > > -    uint32_t body_size = 4 + 4 + 2;
> > > -
> > > -    for (i = 0; i < nwname; i++) {
> > > -        uint16_t wname_size = v9fs_string_size(wnames[i]);
> > > -
> > > -        g_assert_cmpint(body_size, <=, UINT32_MAX - wname_size);
> > > -        body_size += wname_size;
> > > -    }
> > > -    req = v9fs_req_init(v9p,  body_size, P9_TWALK, tag);
> > > -    v9fs_uint32_write(req, fid);
> > > -    v9fs_uint32_write(req, newfid);
> > > -    v9fs_uint16_write(req, nwname);
> > > -    for (i = 0; i < nwname; i++) {
> > > -        v9fs_string_write(req, wnames[i]);
> > > -    }
> > > -    v9fs_req_send(req);
> > > -    return req;
> > > -}
> > > -
> > > 
> > >  /* size[4] Rwalk tag[2] nwqid[2] nwqid*(wqid[13]) */
> > >  static void v9fs_rwalk(P9Req *req, uint16_t *nwqid, v9fs_qid **wqid)
> > >  {
> > > 
> > > @@ -432,6 +408,98 @@ static void v9fs_rwalk(P9Req *req, uint16_t *nwqid,
> > > v9fs_qid **wqid) v9fs_req_free(req);
> > > 
> > >  }
> > > 
> > > +/* options for 'Twalk' 9p request */
> > > +typedef struct TWalkOpt {
> > > +    /* 9P client being used (mandatory) */
> > > +    QVirtio9P *client;
> > > +    /* user supplied tag number being returned with response (optional)
> > > */
> > > +    uint16_t tag;
> > > +    /* file ID of directory from where walk should start (optional) */
> > > +    uint32_t fid;
> > > +    /* file ID for target directory being walked to (optional) */
> > > +    uint32_t newfid;
> > > +    /* low level variant of path to walk to (optional) */
> > > +    uint16_t nwname;
> > > +    char **wnames;
> > > +    /* high level variant of path to walk to (optional) */
> > > +    const char *path;
> > > +    /* data being received from 9p server as 'Rwalk' response (optional)
> > > */ +    struct {
> > > +        uint16_t *nwqid;
> > > +        v9fs_qid **wqid;
> > > +    } rwalk;
> > > +    /* only send Twalk request but not wait for a reply? (optional) */
> > > +    bool requestOnly;
> > > +    /* do we expect an Rlerror response, if yes which error code?
> > > (optional) */ +    uint32_t expectErr;
> > > +} TWalkOpt;
> > > +
> > > +/* result of 'Twalk' 9p request */
> > > +typedef struct TWalkRes {
> > > +    /* file ID of target directory been walked to */
> > > +    uint32_t newfid;
> > > +    /* if requestOnly was set: request object for further processing */
> > > +    P9Req *req;
> > > +} TWalkRes;
> > > +
> > > +/* size[4] Twalk tag[2] fid[4] newfid[4] nwname[2] nwname*(wname[s]) */
> > > +static TWalkRes twalk(TWalkOpt opt)
> > > +{
> > > +    P9Req *req;
> > > +    int i;
> > > +    uint32_t body_size = 4 + 4 + 2;
> > > +    uint32_t err;
> > > +    char **wnames = NULL;
> > > +
> > > +    g_assert(opt.client);
> > > +    /* expecting either high- or low-level path, both not both */
> > > +    g_assert(!opt.path || !(opt.nwname || opt.wnames));
> > > +    /* expecting either Rwalk or Rlerror, but obviously not both */
> > > +    g_assert(!opt.expectErr || !(opt.rwalk.nwqid || opt.rwalk.wqid));
> > > +
> > > +    if (!opt.newfid) {
> > > +        opt.newfid = genfid();
> > > +    }
> > > +
> > > +    if (opt.path) {
> > > +        opt.nwname = split(opt.path, "/", &wnames);
> > > +        opt.wnames = wnames;
> > > +    }
> > > +
> > > +    for (i = 0; i < opt.nwname; i++) {
> > > +        uint16_t wname_size = v9fs_string_size(opt.wnames[i]);
> > > +
> > > +        g_assert_cmpint(body_size, <=, UINT32_MAX - wname_size);
> > > +        body_size += wname_size;
> > > +    }
> > > +    req = v9fs_req_init(opt.client, body_size, P9_TWALK, opt.tag);
> > > +    v9fs_uint32_write(req, opt.fid);
> > > +    v9fs_uint32_write(req, opt.newfid);
> > > +    v9fs_uint16_write(req, opt.nwname);
> > > +    for (i = 0; i < opt.nwname; i++) {
> > > +        v9fs_string_write(req, opt.wnames[i]);
> > > +    }
> > > +    v9fs_req_send(req);
> > > +
> > > +    if (!opt.requestOnly) {
> > > +        v9fs_req_wait_for_reply(req, NULL);
> > > +        if (opt.expectErr) {
> > > +            v9fs_rlerror(req, &err);
> > > +            g_assert_cmpint(err, ==, opt.expectErr);
> > > +        } else {
> > > +            v9fs_rwalk(req, opt.rwalk.nwqid, opt.rwalk.wqid);
> > > +        }
> > > +        req = NULL; /* request was freed */
> > > +    }
> > > +
> > > +    split_free(&wnames);
> > > +
> > > +    return (TWalkRes) {
> > > +        .newfid = opt.newfid,
> > > +        .req = req,
> > > +    };
> > > +}
> > > +
> > > 
> > >  /* size[4] Tgetattr tag[2] fid[4] request_mask[8] */
> > >  static P9Req *v9fs_tgetattr(QVirtio9P *v9p, uint32_t fid, uint64_t
> > > 
> > > request_mask, uint16_t tag)
> > > @@ -669,52 +737,6 @@ static void do_version(QVirtio9P *v9p)
> > > 
> > >      g_assert_cmpmem(server_version, server_len, version,
> > >      strlen(version));
> > >  
> > >  }
> > > 
> > > -/*
> > > - * utility function: walk to requested dir and return fid for that dir
> > > and
> > > - * the QIDs of server response
> > > - */
> > > -static uint32_t do_walk_rqids(QVirtio9P *v9p, const char *path, uint16_t
> > > *nwqid, -                              v9fs_qid **wqid)
> > > -{
> > > -    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, nwqid, wqid);
> > > -
> > > -    split_free(&wnames);
> > > -    return fid;
> > > -}
> > > -
> > > -/* utility function: walk to requested dir and return fid for that dir */
> > > -static uint32_t do_walk(QVirtio9P *v9p, const char *path)
> > > -{
> > > -    return do_walk_rqids(v9p, path, NULL, NULL);
> > > -}
> > > -
> > > -/* utility function: walk to requested dir and expect passed error
> > > response */ -static void do_walk_expect_error(QVirtio9P *v9p, const char
> > > *path, uint32_t err) -{
> > > -    char **wnames;
> > > -    P9Req *req;
> > > -    uint32_t _err;
> > > -    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_rlerror(req, &_err);
> > > -
> > > -    g_assert_cmpint(_err, ==, err);
> > > -
> > > -    split_free(&wnames);
> > > -}
> > > -
> > > 
> > >  static void fs_version(void *obj, void *data, QGuestAllocator *t_alloc)
> > >  {
> > >  
> > >      alloc = t_alloc;
> > > 
> > > @@ -757,7 +779,10 @@ static void fs_walk(void *obj, void *data,
> > > QGuestAllocator *t_alloc) }
> > > 
> > >      do_attach(v9p);
> > > 
> > > -    req = v9fs_twalk(v9p, 0, 1, P9_MAXWELEM, wnames, 0);
> > > +    req = twalk((TWalkOpt) {
> > > +        .client = v9p, .fid = 0, .newfid = 1,
> > > +        .nwname = P9_MAXWELEM, .wnames = wnames, .requestOnly = true
> > > +    }).req;
> > > 
> > >      v9fs_req_wait_for_reply(req, NULL);
> > >      v9fs_rwalk(req, &nwqid, &wqid);
> > > 
> > > @@ -941,7 +966,7 @@ static void fs_readdir(void *obj, void *data,
> > > QGuestAllocator *t_alloc) {
> > > 
> > >      QVirtio9P *v9p = obj;
> > >      alloc = t_alloc;
> > > 
> > > -    char *const wnames[] = { g_strdup(QTEST_V9FS_SYNTH_READDIR_DIR) };
> > > +    char *wnames[] = { g_strdup(QTEST_V9FS_SYNTH_READDIR_DIR) };
> > > 
> > >      uint16_t nqid;
> > >      v9fs_qid qid;
> > >      uint32_t count, nentries;
> > > 
> > > @@ -949,7 +974,10 @@ static void fs_readdir(void *obj, void *data,
> > > QGuestAllocator *t_alloc) P9Req *req;
> > > 
> > >      do_attach(v9p);
> > > 
> > > -    req = v9fs_twalk(v9p, 0, 1, 1, wnames, 0);
> > > +    req = twalk((TWalkOpt) {
> > > +        .client = v9p, .fid = 0, .newfid = 1,
> > > +        .nwname = 1, .wnames = wnames, .requestOnly = true
> > > +    }).req;
> > > 
> > >      v9fs_req_wait_for_reply(req, NULL);
> > >      v9fs_rwalk(req, &nqid, NULL);
> > >      g_assert_cmpint(nqid, ==, 1);
> > > 
> > > @@ -993,7 +1021,7 @@ static void fs_readdir(void *obj, void *data,
> > > QGuestAllocator *t_alloc) /* readdir test where overall request is split
> > > over several messages */ static void do_readdir_split(QVirtio9P *v9p,
> > > uint32_t count)
> > > 
> > >  {
> > > 
> > > -    char *const wnames[] = { g_strdup(QTEST_V9FS_SYNTH_READDIR_DIR) };
> > > +    char *wnames[] = { g_strdup(QTEST_V9FS_SYNTH_READDIR_DIR) };
> > > 
> > >      uint16_t nqid;
> > >      v9fs_qid qid;
> > >      uint32_t nentries, npartialentries;
> > > 
> > > @@ -1010,7 +1038,10 @@ static void do_readdir_split(QVirtio9P *v9p,
> > > uint32_t count) nentries = 0;
> > > 
> > >      tail = NULL;
> > > 
> > > -    req = v9fs_twalk(v9p, 0, fid, 1, wnames, 0);
> > > +    req = twalk((TWalkOpt) {
> > > +        .client = v9p, .fid = 0, .newfid = fid,
> > > +        .nwname = 1, .wnames = wnames, .requestOnly = true
> > > +    }).req;
> > > 
> > >      v9fs_req_wait_for_reply(req, NULL);
> > >      v9fs_rwalk(req, &nqid, NULL);
> > >      g_assert_cmpint(nqid, ==, 1);
> > > 
> > > @@ -1074,12 +1105,15 @@ static void fs_walk_no_slash(void *obj, void
> > > *data,
> > > QGuestAllocator *t_alloc) {
> > > 
> > >      QVirtio9P *v9p = obj;
> > >      alloc = t_alloc;
> > > 
> > > -    char *const wnames[] = { g_strdup(" /") };
> > > +    char *wnames[] = { g_strdup(" /") };
> > > 
> > >      P9Req *req;
> > >      uint32_t err;
> > >      
> > >      do_attach(v9p);
> > > 
> > > -    req = v9fs_twalk(v9p, 0, 1, 1, wnames, 0);
> > > +    req = twalk((TWalkOpt) {
> > > +        .client = v9p, .fid = 0, .newfid = 1,
> > > +        .nwname = 1, .wnames = wnames, .requestOnly = true
> > > +    }).req;
> > > 
> > >      v9fs_req_wait_for_reply(req, NULL);
> > >      v9fs_rlerror(req, &err);
> > > 
> > > @@ -1098,7 +1132,9 @@ static void fs_walk_nonexistent(void *obj, void
> > > *data, QGuestAllocator *t_alloc) * The 9p2000 protocol spec says: "If the
> > > first element cannot be walked * for any reason, Rerror is returned."
> > > 
> > >       */
> > > 
> > > -    do_walk_expect_error(v9p, "non-existent", ENOENT);
> > > +    twalk((TWalkOpt) {
> > > +        .client = v9p, .path = "non-existent", .expectErr = ENOENT
> > > +    });
> > > 
> > >  }
> > >  
> > >  static void fs_walk_2nd_nonexistent(void *obj, void *data,
> > > 
> > > @@ -1116,7 +1152,10 @@ static void fs_walk_2nd_nonexistent(void *obj, void
> > > *data, );
> > > 
> > >      do_attach_rqid(v9p, &root_qid);
> > > 
> > > -    fid = do_walk_rqids(v9p, path, &nwqid, &wqid);
> > > +    fid = twalk((TWalkOpt) {
> > > +        .client = v9p, .path = path,
> > > +        .rwalk.nwqid = &nwqid, .rwalk.wqid = &wqid
> > > +    }).newfid;
> > > 
> > >      /*
> > >      
> > >       * The 9p2000 protocol spec says: "nwqid is therefore either nwname
> > >       or
> > > 
> > > the * index of the first elementwise walk that failed."
> > > @@ -1148,7 +1187,10 @@ static void fs_walk_none(void *obj, void *data,
> > > QGuestAllocator *t_alloc) v9fs_req_wait_for_reply(req, NULL);
> > > 
> > >      v9fs_rattach(req, &root_qid);
> > > 
> > > -    req = v9fs_twalk(v9p, 0, 1, 0, NULL, 0);
> > > +    req = twalk((TWalkOpt) {
> > > +        .client = v9p, .fid = 0, .newfid = 1,
> > > +        .nwname = 0, .wnames = NULL, .requestOnly = true
> > > +    }).req;
> > > 
> > >      v9fs_req_wait_for_reply(req, NULL);
> > >      v9fs_rwalk(req, NULL, &wqid);
> > > 
> > > @@ -1166,7 +1208,7 @@ static void fs_walk_dotdot(void *obj, void *data,
> > > QGuestAllocator *t_alloc) {
> > > 
> > >      QVirtio9P *v9p = obj;
> > >      alloc = t_alloc;
> > > 
> > > -    char *const wnames[] = { g_strdup("..") };
> > > +    char *wnames[] = { g_strdup("..") };
> > > 
> > >      v9fs_qid root_qid;
> > >      g_autofree v9fs_qid *wqid = NULL;
> > >      P9Req *req;
> > > 
> > > @@ -1176,7 +1218,10 @@ static void fs_walk_dotdot(void *obj, void *data,
> > > QGuestAllocator *t_alloc) v9fs_req_wait_for_reply(req, NULL);
> > > 
> > >      v9fs_rattach(req, &root_qid);
> > > 
> > > -    req = v9fs_twalk(v9p, 0, 1, 1, wnames, 0);
> > > +    req = twalk((TWalkOpt) {
> > > +        .client = v9p, .fid = 0, .newfid = 1,
> > > +        .nwname = 1, .wnames = wnames, .requestOnly = true
> > > +    }).req;
> > > 
> > >      v9fs_req_wait_for_reply(req, NULL);
> > >      v9fs_rwalk(req, NULL, &wqid); /* We now we'll get one qid */
> > > 
> > > @@ -1189,11 +1234,14 @@ static void fs_lopen(void *obj, void *data,
> > > QGuestAllocator *t_alloc) {
> > > 
> > >      QVirtio9P *v9p = obj;
> > >      alloc = t_alloc;
> > > 
> > > -    char *const wnames[] = { g_strdup(QTEST_V9FS_SYNTH_LOPEN_FILE) };
> > > +    char *wnames[] = { g_strdup(QTEST_V9FS_SYNTH_LOPEN_FILE) };
> > > 
> > >      P9Req *req;
> > >      
> > >      do_attach(v9p);
> > > 
> > > -    req = v9fs_twalk(v9p, 0, 1, 1, wnames, 0);
> > > +    req = twalk((TWalkOpt) {
> > > +        .client = v9p, .fid = 0, .newfid = 1,
> > > +        .nwname = 1, .wnames = wnames, .requestOnly = true
> > > +    }).req;
> > > 
> > >      v9fs_req_wait_for_reply(req, NULL);
> > >      v9fs_rwalk(req, NULL, NULL);
> > > 
> > > @@ -1209,13 +1257,16 @@ static void fs_write(void *obj, void *data,
> > > QGuestAllocator *t_alloc) QVirtio9P *v9p = obj;
> > > 
> > >      alloc = t_alloc;
> > >      static const uint32_t write_count = P9_MAX_SIZE / 2;
> > > 
> > > -    char *const wnames[] = { g_strdup(QTEST_V9FS_SYNTH_WRITE_FILE) };
> > > +    char *wnames[] = { g_strdup(QTEST_V9FS_SYNTH_WRITE_FILE) };
> > > 
> > >      g_autofree char *buf = g_malloc0(write_count);
> > >      uint32_t count;
> > >      P9Req *req;
> > >      
> > >      do_attach(v9p);
> > > 
> > > -    req = v9fs_twalk(v9p, 0, 1, 1, wnames, 0);
> > > +    req = twalk((TWalkOpt) {
> > > +        .client = v9p, .fid = 0, .newfid = 1,
> > > +        .nwname = 1, .wnames = wnames, .requestOnly = true
> > > +    }).req;
> > > 
> > >      v9fs_req_wait_for_reply(req, NULL);
> > >      v9fs_rwalk(req, NULL, NULL);
> > > 
> > > @@ -1235,13 +1286,16 @@ static void fs_flush_success(void *obj, void
> > > *data,
> > > QGuestAllocator *t_alloc) {
> > > 
> > >      QVirtio9P *v9p = obj;
> > >      alloc = t_alloc;
> > > 
> > > -    char *const wnames[] = { g_strdup(QTEST_V9FS_SYNTH_FLUSH_FILE) };
> > > +    char *wnames[] = { g_strdup(QTEST_V9FS_SYNTH_FLUSH_FILE) };
> > > 
> > >      P9Req *req, *flush_req;
> > >      uint32_t reply_len;
> > >      uint8_t should_block;
> > >      
> > >      do_attach(v9p);
> > > 
> > > -    req = v9fs_twalk(v9p, 0, 1, 1, wnames, 0);
> > > +    req = twalk((TWalkOpt) {
> > > +        .client = v9p, .fid = 0, .newfid = 1,
> > > +        .nwname = 1, .wnames = wnames, .requestOnly = true
> > > +    }).req;
> > > 
> > >      v9fs_req_wait_for_reply(req, NULL);
> > >      v9fs_rwalk(req, NULL, NULL);
> > > 
> > > @@ -1272,13 +1326,16 @@ static void fs_flush_ignored(void *obj, void
> > > *data,
> > > QGuestAllocator *t_alloc) {
> > > 
> > >      QVirtio9P *v9p = obj;
> > >      alloc = t_alloc;
> > > 
> > > -    char *const wnames[] = { g_strdup(QTEST_V9FS_SYNTH_FLUSH_FILE) };
> > > +    char *wnames[] = { g_strdup(QTEST_V9FS_SYNTH_FLUSH_FILE) };
> > > 
> > >      P9Req *req, *flush_req;
> > >      uint32_t count;
> > >      uint8_t should_block;
> > >      
> > >      do_attach(v9p);
> > > 
> > > -    req = v9fs_twalk(v9p, 0, 1, 1, wnames, 0);
> > > +    req = twalk((TWalkOpt) {
> > > +        .client = v9p, .fid = 0, .newfid = 1,
> > > +        .nwname = 1, .wnames = wnames, .requestOnly = true
> > > +    }).req;
> > > 
> > >      v9fs_req_wait_for_reply(req, NULL);
> > >      v9fs_rwalk(req, NULL, NULL);
> > > 
> > > @@ -1311,7 +1368,7 @@ static void do_mkdir(QVirtio9P *v9p, const char
> > > *path, const char *cname) uint32_t fid;
> > > 
> > >      P9Req *req;
> > > 
> > > -    fid = do_walk(v9p, path);
> > > +    fid = twalk((TWalkOpt) { .client = v9p, .path = path }).newfid;
> > > 
> > >      req = v9fs_tmkdir(v9p, fid, name, 0750, 0, 0);
> > >      v9fs_req_wait_for_reply(req, NULL);
> > > 
> > > @@ -1326,7 +1383,7 @@ static uint32_t do_lcreate(QVirtio9P *v9p, const
> > > char
> > > *path, uint32_t fid;
> > > 
> > >      P9Req *req;
> > > 
> > > -    fid = do_walk(v9p, path);
> > > +    fid = twalk((TWalkOpt) { .client = v9p, .path = path }).newfid;
> > > 
> > >      req = v9fs_tlcreate(v9p, fid, name, 0, 0750, 0, 0);
> > >      v9fs_req_wait_for_reply(req, NULL);
> > > 
> > > @@ -1344,7 +1401,7 @@ static void do_symlink(QVirtio9P *v9p, const char
> > > *path, const char *clink, uint32_t fid;
> > > 
> > >      P9Req *req;
> > > 
> > > -    fid = do_walk(v9p, path);
> > > +    fid = twalk((TWalkOpt) { .client = v9p, .path = path }).newfid;
> > > 
> > >      req = v9fs_tsymlink(v9p, fid, name, dst, 0, 0);
> > >      v9fs_req_wait_for_reply(req, NULL);
> > > 
> > > @@ -1358,8 +1415,8 @@ static void do_hardlink(QVirtio9P *v9p, const char
> > > *path, const char *clink, uint32_t dfid, fid;
> > > 
> > >      P9Req *req;
> > > 
> > > -    dfid = do_walk(v9p, path);
> > > -    fid = do_walk(v9p, to);
> > > +    dfid = twalk((TWalkOpt) { .client = v9p, .path = path }).newfid;
> > > +    fid = twalk((TWalkOpt) { .client = v9p, .path = to }).newfid;
> > > 
> > >      req = v9fs_tlink(v9p, dfid, fid, clink, 0);
> > >      v9fs_req_wait_for_reply(req, NULL);
> > > 
> > > @@ -1373,7 +1430,7 @@ static void do_unlinkat(QVirtio9P *v9p, const char
> > > *atpath, const char *rpath, uint32_t fid;
> > > 
> > >      P9Req *req;
> > > 
> > > -    fid = do_walk(v9p, atpath);
> > > +    fid = twalk((TWalkOpt) { .client = v9p, .path = atpath }).newfid;
> > > 
> > >      req = v9fs_tunlinkat(v9p, fid, name, flags, 0);
> > >      v9fs_req_wait_for_reply(req, NULL);
> 
>