Re: [PATCH v4 06/11] tests/virtio-9p: added splitted readdir test

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Donnerstag, 23. Januar 2020 11:30:58 CET Greg Kurz wrote:
> On Wed, 22 Jan 2020 23:36:22 +0100
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > On Mittwoch, 22. Januar 2020 22:19:05 CET Greg Kurz wrote:
> > > On Tue, 21 Jan 2020 01:16:21 +0100
> > > 
> > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > > > The previous, already existing readdir test simply used a 'count'
> > > > parameter big enough to retrieve all directory entries with a
> > > > single Treaddir request.
> > > > 
> > > > In this new 'splitted' readdir test, directory entries are
> > > > retrieved, splitted 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. Currently this test covers actually two
> > > > tests: a sequence of Treaddir requests with count=512 and then a
> > > > subsequent test with a sequence of Treaddir requests with count=256.
> > > > 
> > > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > > ---
> > > 
> > > Not sure it is really needed to check with multiple values for 'count',
> > > but it doesn't eat too many cycles so I guess it doesn't hurt.
> > 
> > Yes, it is a cheap test, nobody will feel the difference. One could argue
> > about the precise 'count' values being used ...
> > 
> > > Applied as well.
> > > 
> > > >  tests/qtest/virtio-9p-test.c | 91
> > > >  ++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 91 insertions(+)
> > > > 
> > > > diff --git a/tests/qtest/virtio-9p-test.c
> > > > b/tests/qtest/virtio-9p-test.c
> > > > index 2167322985..8b0d94546e 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,95 @@ static void fs_readdir(void *obj, void *data,
> > > > QGuestAllocator *t_alloc)>
> > > > 
> > > >      g_free(wnames[0]);
> > > >  
> > > >  }
> > > > 
> > > > +/* readdir test where overall request is splitted over several
> > > > messages
> > > > */
> > > > +static void fs_readdir_split(void *obj, void *data, QGuestAllocator
> > > > *t_alloc) +{
> > > > +    QVirtio9P *v9p = obj;
> > > > +    alloc = t_alloc;
> > > > +    char *const wnames[] = { g_strdup(QTEST_V9FS_SYNTH_READDIR_DIR)
> > > > };
> > > > +    uint16_t nqid;
> > > > +    v9fs_qid qid;
> > > > +    uint32_t count, nentries, npartialentries;
> > > > +    struct V9fsDirent *entries, *tail, *partialentries;
> > > > +    P9Req *req;
> > > > +    int subtest;
> > > > +    int fid;
> > > > +    uint64_t offset;
> > > > +    /* the Treaddir 'count' parameter values to be tested */
> > > > +    const uint32_t vcount[] = { 512, 256 };
> > 
> > ... here. But IMO it does make sense preserving the function's overall
> > structure to allow testing with different 'count' values if necessary.
> > Because that way this test could e.g. guard potential bugs when server's
> > Treaddir handler is rolling back (or not) the dir offset for instance,
> > which server has to do (or not) according to this 'count' value and the
> > precise file name length of the individual directory entries.
> 
> I still agree it is useful to be able to check different values but I
> now realize that it shouldn't be done like this because adding new
> values to vcount[] doesn't scale well with the MAX_REQ limitation I
> mentioned in another mail. More values, especially small ones, are
> likely to trigger "Failed to decode VirtFS request type 40" at some
> point.
> 
> I now think that fs_readdir_split() should rather get count as
> an argument and only do one run. By default we would only call
> this with an appropriate value, ideally derived from the test
> environment (number of files, size of filenames... etc...).
> If someone needs to test a specific value, it is easy as adding
> a new qos_add_test() line.

I actually had this variant in the exact same way as you're suggesting here as 
well before (not submitted to the list though), that is I had a version of 
this function with a count argument, and I had 3 separate qtest cases, but 
that variant failed in the exact same way.

Furthermore, I even get some strange "could not push stack" error messages 
from the qtest environment when I just add some empty noop tests to the 9pfs 
test suite. I am not sure if that's related to this ringbuffer issue here, but 
it was a show stopper for this 'separate test for each count' variant, so I 
reverted it to this suggested array solution for now.

So to avoid any misapprehension: it seems to me as if you were thinking that 
the ringbuffer is freed between every individual test. That's not what I am 
observing here. It rather seems as space in the ringbuffer is never freed 
(before the entire 9pfs test suite completed).

> This would ensure at least that each run starts with the same
> fixed number of possible requests, ie. MAX_REQ minus the cost of
> fs_attach().
> 
> So I'll revert this patch for now.
> 
> > Whatever precise 'count' tests are desired, it would only mean a one line
> > change here.
> > 
> > Best regards,
> > Christian Schoenebeck


Best regards,
Christian Schoenebeck