diff mbox series

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

Message ID E1oDQqv-0003d4-Hm@lizzy.crudebyte.com (mailing list archive)
State New, archived
Headers show
Series [RFC,v2] tests/9p: introduce declarative function calls | expand

Commit Message

Christian Schoenebeck July 18, 2022, 1:10 p.m. UTC
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(-)

Comments

Christian Schoenebeck July 18, 2022, 2:02 p.m. UTC | #1
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);
Christian Schoenebeck Aug. 18, 2022, 2:13 p.m. UTC | #2
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);
Greg Kurz Aug. 29, 2022, 10:31 a.m. UTC | #3
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);
> 
>
diff mbox series

Patch

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);