Re: [PATCH v8 1/7] tests/virtio-9p: added split readdir tests

[Greg Kurz]

On Wed, 29 Jul 2020 10:10:23 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> The previous, already existing 'basic' readdir test simply used a
> 'count' parameter big enough to retrieve all directory entries with a
> single Treaddir request.
> 
> In the 3 new 'split' readdir tests added by this patch, directory
> entries are retrieved, split over several Treaddir requests by picking
> small 'count' parameters which force the server to truncate the
> response. So the test client sends as many Treaddir requests as
> necessary to get all directory entries.
> 
> The following 3 new tests are added (executed in this sequence):
> 
> 1. Split readdir test with count=512
> 2. Split readdir test with count=256
> 3. Split readdir test with count=128
> 
> This test case sequence is chosen because the smaller the 'count' value,
> the higher the chance of errors in case of implementation bugs on server
> side.
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---

The existing fs_readdir() function for the 'basic' test is a subset
of the new fs_readdir_split() introduced by this patch (quite visible
if you sdiff the code).

To avoid code duplication, I would have probably tried to do the changes
in fs_readdir() and implement the 'basic' test as:

static void fs_readdir_basic(void *obj, void *data,
                             QGuestAllocator *t_alloc)
{
    /*
     * submit count = msize - 11, because 11 is the header size of Rreaddir
     */
    fs_readdir(obj, data, t_alloc, P9_MAX_SIZE - 11);
}

but anyway this looks good to me so:

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

>  tests/qtest/virtio-9p-test.c | 108 +++++++++++++++++++++++++++++++++++
>  1 file changed, 108 insertions(+)
> 
> diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> index 2167322985..de30b717b6 100644
> --- a/tests/qtest/virtio-9p-test.c
> +++ b/tests/qtest/virtio-9p-test.c
> @@ -578,6 +578,7 @@ static bool fs_dirents_contain_name(struct V9fsDirent *e, 
> const char* name)
>      return false;
>  }
>  
> +/* basic readdir test where reply fits into a single response message */
>  static void fs_readdir(void *obj, void *data, QGuestAllocator *t_alloc)
>  {
>      QVirtio9P *v9p = obj;
> @@ -631,6 +632,89 @@ static void fs_readdir(void *obj, void *data, 
> QGuestAllocator *t_alloc)
>      g_free(wnames[0]);
>  }
>  
> +/* readdir test where overall request is split over several messages */
> +static void fs_readdir_split(void *obj, void *data, QGuestAllocator *t_alloc,
> +                             uint32_t count)
> +{
> +    QVirtio9P *v9p = obj;
> +    alloc = t_alloc;
> +    char *const wnames[] = { g_strdup(QTEST_V9FS_SYNTH_READDIR_DIR) };
> +    uint16_t nqid;
> +    v9fs_qid qid;
> +    uint32_t nentries, npartialentries;
> +    struct V9fsDirent *entries, *tail, *partialentries;
> +    P9Req *req;
> +    int fid;
> +    uint64_t offset;
> +
> +    fs_attach(v9p, NULL, t_alloc);
> +
> +    fid = 1;
> +    offset = 0;
> +    entries = NULL;
> +    nentries = 0;
> +    tail = NULL;
> +
> +    req = v9fs_twalk(v9p, 0, fid, 1, wnames, 0);
> +    v9fs_req_wait_for_reply(req, NULL);
> +    v9fs_rwalk(req, &nqid, NULL);
> +    g_assert_cmpint(nqid, ==, 1);
> +
> +    req = v9fs_tlopen(v9p, fid, O_DIRECTORY, 0);
> +    v9fs_req_wait_for_reply(req, NULL);
> +    v9fs_rlopen(req, &qid, NULL);
> +
> +    /*
> +     * send as many Treaddir requests as required to get all directory
> +     * entries
> +     */
> +    while (true) {
> +        npartialentries = 0;
> +        partialentries = NULL;
> +
> +        req = v9fs_treaddir(v9p, fid, offset, count, 0);
> +        v9fs_req_wait_for_reply(req, NULL);
> +        v9fs_rreaddir(req, &count, &npartialentries, &partialentries);
> +        if (npartialentries > 0 && partialentries) {
> +            if (!entries) {
> +                entries = partialentries;
> +                nentries = npartialentries;
> +                tail = partialentries;
> +            } else {
> +                tail->next = partialentries;
> +                nentries += npartialentries;
> +            }
> +            while (tail->next) {
> +                tail = tail->next;
> +            }
> +            offset = tail->offset;
> +        } else {
> +            break;
> +        }
> +    }
> +
> +    g_assert_cmpint(
> +        nentries, ==,
> +        QTEST_V9FS_SYNTH_READDIR_NFILES + 2 /* "." and ".." */
> +    );
> +
> +    /*
> +     * Check all file names exist in returned entries, ignore their order
> +     * though.
> +     */
> +    g_assert_cmpint(fs_dirents_contain_name(entries, "."), ==, true);
> +    g_assert_cmpint(fs_dirents_contain_name(entries, ".."), ==, true);
> +    for (int i = 0; i < QTEST_V9FS_SYNTH_READDIR_NFILES; ++i) {
> +        char *name = g_strdup_printf(QTEST_V9FS_SYNTH_READDIR_FILE, i);
> +        g_assert_cmpint(fs_dirents_contain_name(entries, name), ==, true);
> +        g_free(name);
> +    }
> +
> +    v9fs_free_dirents(entries);
> +
> +    g_free(wnames[0]);
> +}
> +
>  static void fs_walk_no_slash(void *obj, void *data, QGuestAllocator *t_alloc)
>  {
>      QVirtio9P *v9p = obj;
> @@ -793,6 +877,24 @@ static void fs_flush_ignored(void *obj, void *data, 
> QGuestAllocator *t_alloc)
>      g_free(wnames[0]);
>  }
>  
> +static void fs_readdir_split_128(void *obj, void *data,
> +                                 QGuestAllocator *t_alloc)
> +{
> +    fs_readdir_split(obj, data, t_alloc, 128);
> +}
> +
> +static void fs_readdir_split_256(void *obj, void *data,
> +                                 QGuestAllocator *t_alloc)
> +{
> +    fs_readdir_split(obj, data, t_alloc, 256);
> +}
> +
> +static void fs_readdir_split_512(void *obj, void *data,
> +                                 QGuestAllocator *t_alloc)
> +{
> +    fs_readdir_split(obj, data, t_alloc, 512);
> +}
> +
>  static void register_virtio_9p_test(void)
>  {
>      qos_add_test("config", "virtio-9p", pci_config, NULL);
> @@ -810,6 +912,12 @@ static void register_virtio_9p_test(void)
>      qos_add_test("fs/flush/ignored", "virtio-9p", fs_flush_ignored,
>                   NULL);
>      qos_add_test("fs/readdir/basic", "virtio-9p", fs_readdir, NULL);
> +    qos_add_test("fs/readdir/split_512", "virtio-9p",
> +                 fs_readdir_split_512, NULL);
> +    qos_add_test("fs/readdir/split_256", "virtio-9p",
> +                 fs_readdir_split_256, NULL);
> +    qos_add_test("fs/readdir/split_128", "virtio-9p",
> +                 fs_readdir_split_128, NULL);
>  }
>  
>  libqos_init(register_virtio_9p_test);