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

[Greg Kurz <groug@kaod.org>]

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