Re: [PATCH] tests/9p: split virtio-9p-test.c into tests and 9p client part

[Christian Schoenebeck]

On Montag, 26. September 2022 16:30:53 CEST Greg Kurz wrote:
> On Sat, 10 Sep 2022 19:46:55 +0200
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > This patch is pure refactoring, it does not change behaviour.
> > 
> > virtio-9p-test.c grew to 1657 lines. Let's split this file up between
> > actual 9p test cases vs. 9p test client, to make it easier to
> > concentrate on the actual 9p tests.
> > 
> > Move the 9p test client code to a new unit virtio-9p-client.c, which
> > are basically all functions and types prefixed with v9fs_* already.
> > 
> > Note that some client wrapper functions (do_*) are preserved in
> > virtio-9p-test.c, simply because these wrapper functions are going to
> > be wiped with subsequent patches anyway.
> > 
> > As the global QGuestAllocator variable is moved to virtio-9p-client.c,
> > add a new function v9fs_set_allocator() to be used by virtio-9p-test.c
> > instead of fiddling with a global variable across units and libraries.
> > 
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> > 
> > As I am working on extending the previously sent RFC [1] (which will be
> > using function calls with named function arguments), I realized that it
> > makes sense to first split the client code out to a new file, and then
> > make the upcoming patches based on this patch here. Because that way
> > I don't have to touch the order of the client functions and the upcoming
> > patches will therefore become better readable.
> 
> Hi Christian,
> 
> The change looks quite reasonable but you'll have to fix the includes...
> 
> > [1] https://lore.kernel.org/all/E1oDQqv-0003d4-Hm@lizzy.crudebyte.com/
> > 
> >  tests/qtest/libqos/meson.build        |   1 +
> >  tests/qtest/libqos/virtio-9p-client.c | 683 +++++++++++++++++++++++
> >  tests/qtest/libqos/virtio-9p-client.h | 139 +++++
> >  tests/qtest/virtio-9p-test.c          | 770 +-------------------------
> >  4 files changed, 849 insertions(+), 744 deletions(-)
> >  create mode 100644 tests/qtest/libqos/virtio-9p-client.c
> >  create mode 100644 tests/qtest/libqos/virtio-9p-client.h
> 
> [..snip..]
> 
> > diff --git a/tests/qtest/libqos/virtio-9p-client.h
> > b/tests/qtest/libqos/virtio-9p-client.h new file mode 100644
> > index 0000000000..8bea032a85
> > --- /dev/null
> > +++ b/tests/qtest/libqos/virtio-9p-client.h
> > @@ -0,0 +1,139 @@
> > +/*
> > + * 9P network client for VirtIO 9P test cases (based on QTest)
> > + *
> > + * Copyright (c) 2014 SUSE LINUX Products GmbH
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or
> > later. + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +/*
> > + * Not so fast! You might want to read the 9p developer docs first:
> > + * https://wiki.qemu.org/Documentation/9p
> > + */
> > +
> > +#ifndef TESTS_LIBQOS_VIRTIO_9P_CLIENT_H
> > +#define TESTS_LIBQOS_VIRTIO_9P_CLIENT_H
> > +
> > +#include "qemu/osdep.h"
> 
> ... here.
> 
> As explained in `docs/devel/style.rst`, the "qemu/osdep.h" header must
> only be included in .c files. Please move this #include directive to
> `tests/qtest/libqos/virtio-9p-client.c`:
> 
> #include "qemu/osdep.h"
> #include "virtio-9p-client.h"
> 
> With that fixed you can add R-b tag.
> 
> Cheers,

Good catch! Will do, thanks!

Best regards,
Christian Schoenebeck