Re: [PATCH] tests: virtio-9p-client: Rreaddir fields are all mandatory

[Christian Schoenebeck]

On Thursday, April 27, 2023 3:10:23 PM CEST Paolo Bonzini wrote:
> If rreaddir.entries is NULL, the resulting dirent list is leaked.
> Check that the rreaddir case is filled correctly in the caller
> when treaddir succeeds; this then makes it possible to remove
> the conditionals is v9fs_rreaddir.
> 
> Reported by Coverity.

That's an old defects report, right? I remember I saw something like this last
year and ignored it as being purely theoretical.

> Cc: Christian Schoenebeck <qemu_oss@crudebyte.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  tests/qtest/libqos/virtio-9p-client.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/tests/qtest/libqos/virtio-9p-client.c 
> b/tests/qtest/libqos/virtio-9p-client.c
> index e4a368e03660..17125e78deae 100644
> --- a/tests/qtest/libqos/virtio-9p-client.c
> +++ b/tests/qtest/libqos/virtio-9p-client.c
> @@ -579,6 +579,7 @@ TReadDirRes v9fs_treaddir(TReadDirOpt opt)
>              v9fs_rlerror(req, &err);
>              g_assert_cmpint(err, ==, opt.expectErr);
>          } else {
> +            g_assert(opt.rreaddir.count && opt.rreaddir.nentries && 
> opt.rreaddir.entries);

That would be the opposite of what recent test code restructuring was about:
it reduced overall 9p test code complexity by relaxing how the actual 9p unit
tests (virtio-9p-test.c) shall call the underlying 9p client functions
(virtio-9p-client.c):

https://lore.kernel.org/all/cover.1664917004.git.qemu_oss@crudebyte.com/

>From virtio-9p-client.h:

/* options for 'Treaddir' 9p request */
typedef struct TReadDirOpt {
    /* 9P client being used (mandatory) */
    QVirtio9P *client;
    /* user supplied tag number being returned with response (optional) */
    uint16_t tag;
    /* file ID of directory whose entries shall be retrieved (required) */
    uint32_t fid;
    /* offset in entries stream, i.e. for multiple requests (optional) */
    uint64_t offset;
    /* maximum bytes to be returned by server (required) */
    uint32_t count;
    /* data being received from 9p server as 'Rreaddir' response (optional) */
    struct {
        uint32_t *count;
        uint32_t *nentries;
        struct V9fsDirent **entries;
    } rreaddir;
    /* only send Treaddir 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;
} TReadDirOpt;

So the burden to deal with the individual use cases was moved from the 9p unit
tests down to the client code. Because that's easier to read and maintain than
having to let each unit test deal with all sorts of requirements that it has no 
use for in the first place.

>              v9fs_rreaddir(req, opt.rreaddir.count, opt.rreaddir.nentries,
>                            opt.rreaddir.entries);
>          }
> @@ -600,9 +601,7 @@ void v9fs_rreaddir(P9Req *req, uint32_t *count, uint32_t 
> *nentries,
>      v9fs_req_recv(req, P9_RREADDIR);
>      v9fs_uint32_read(req, &local_count);
>  
> -    if (count) {
> -        *count = local_count;
> -    }
> +    *count = local_count;
>  
>      for (int32_t togo = (int32_t)local_count;
>           togo >= 13 + 8 + 1 + 2;
> @@ -610,9 +609,7 @@ void v9fs_rreaddir(P9Req *req, uint32_t *count, uint32_t 
> *nentries,
>      {
>          if (!e) {
>              e = g_new(struct V9fsDirent, 1);
> -            if (entries) {
> -                *entries = e;
> -            }
> +            *entries = e;

I would just add a local auto free pointer in this function that is assigned
to in case argument `entries` was passed as NULL, and not change overall
behaviour and requirements.

>          } else {
>              e = e->next = g_new(struct V9fsDirent, 1);
>          }
> @@ -624,10 +621,7 @@ void v9fs_rreaddir(P9Req *req, uint32_t *count, uint32_t 
> *nentries,
>          v9fs_string_read(req, &slen, &e->name);
>      }
>  
> -    if (nentries) {
> -        *nentries = n;
> -    }
> -
> +    *nentries = n;
>      v9fs_req_free(req);
>  }
>  
>