[PATCH v2 0/2] 9pfs: test suite fixes
Fixes two bugs with the 9pfs 'local' tests as discussed with latest 9P PR (2020-10-23). See the discussion of that PR for details. v1->v2: - Added Greg's tested-by tag [patch 1]. - Log an info-level message if mkdir() failed [patch 2]. - Update commit log message about coverity being the reporter and details of the coverity report [patch 2]. Christian Schoenebeck (2): tests/9pfs: fix test dir for parallel tests tests/9pfs: fix coverity error in create_local_test_dir() tests/qtest/libqos/virtio-9p.c | 32 +--- 1 file changed, 25 insertions(+), 7 deletions(-) -- 2.20.1
[PATCH v2 1/2] tests/9pfs: fix test dir for parallel tests
Use mkdtemp() to generate a unique directory for the 9p 'local' tests.
This fixes occasional 9p test failures when running 'make check -jN' if
QEMU was compiled for multiple target architectures, because the individual
architecture's test suites would run in parallel and interfere with each
other's data as the test directory was previously hard coded and hence the
same directory was used by all of them simultaniously.
This also requires a change how the test directory is created and deleted:
As the test path is now randomized and virtio_9p_register_nodes() being
called in a somewhat undeterministic way, that's no longer an appropriate
place to create and remove the test directory. Use a constructor and
destructor function for creating and removing the test directory instead.
Unfortunately libqos currently does not support setup/teardown callbacks
to handle this more cleanly.
Signed-off-by: Christian Schoenebeck
Tested-by: Greg Kurz
---
tests/qtest/libqos/virtio-9p.c | 25 +++--
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index d43647b3b7..6b22fa0e9a 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -35,7 +35,12 @@ static char *concat_path(const char* a, const char* b)
static void init_local_test_path(void)
{
char *pwd = g_get_current_dir();
-local_test_path = concat_path(pwd, "qtest-9p-local");
+char *template = concat_path(pwd, "qtest-9p-local-XX");
+local_test_path = mkdtemp(template);
+if (!local_test_path) {
+g_test_message("mkdtemp('%s') failed: %s", template, strerror(errno));
+}
+g_assert(local_test_path);
g_free(pwd);
}
@@ -246,11 +251,6 @@ static void virtio_9p_register_nodes(void)
const char *str_simple = "fsdev=fsdev0,mount_tag=" MOUNT_TAG;
const char *str_addr = "fsdev=fsdev0,addr=04.0,mount_tag=" MOUNT_TAG;
-/* make sure test dir for the 'local' tests exists and is clean */
-init_local_test_path();
-remove_local_test_dir();
-create_local_test_dir();
-
QPCIAddress addr = {
.devfn = QPCI_DEVFN(4, 0),
};
@@ -278,3 +278,16 @@ static void virtio_9p_register_nodes(void)
}
libqos_init(virtio_9p_register_nodes);
+
+static void __attribute__((constructor)) construct_virtio_9p(void)
+{
+/* make sure test dir for the 'local' tests exists */
+init_local_test_path();
+create_local_test_dir();
+}
+
+static void __attribute__((destructor)) destruct_virtio_9p(void)
+{
+/* remove previously created test dir when test suite completed */
+remove_local_test_dir();
+}
--
2.20.1
Re: [PATCH 2/2] tests/9pfs: fix coverity error in create_local_test_dir()
On Freitag, 30. Oktober 2020 13:09:26 CET Peter Maydell wrote: > On Fri, 30 Oct 2020 at 12:02, Christian Schoenebeck > > wrote: > > On Freitag, 30. Oktober 2020 12:44:18 CET Greg Kurz wrote: > > It's not clear to me where this coverity report is accessible online. A > > quick search only brought me to statistics about its latest check, but > > not the details of the report you quoted. > > https://scan.coverity.com/projects/qemu . To see the actual > defect reports you need to create an account and request > access to the QEMU project (we happily give access to developers, > but it is a manual-approval process). > > > And more importantly: is there coverity CI support that one could enable > > on > > github, so that pending patches were checked before upstream merge? > > No, unfortunately not. The Coverity free-for-open-source-projects > system has a very limited number of scans it allows (for a project > the size of ours just one a day) so we can't open it up to > submaintainer branches or even use it on pull requests pre merge; > the best we can do is running it on master daily. > > thanks > -- PMM Thanks for the clarification Peter! I try to sign up for Coverity next week. Best regards, Christian Schoenebeck
[PATCH v2 2/2] tests/9pfs: fix coverity error in create_local_test_dir()
Coverity wants the return value of mkdir() to be checked:
/qemu/tests/qtest/libqos/virtio-9p.c: 48 in create_local_test_dir()
42 /* Creates the directory for the 9pfs 'local' filesystem driver to
access. */
43 static void create_local_test_dir(void)
44 {
45 struct stat st;
46
47 g_assert(local_test_path != NULL);
>>> CID 1435963: Error handling issues (CHECKED_RETURN)
>>> Calling "mkdir(local_test_path, 511U)" without checking return value.
This library function may fail and return an error code.
48 mkdir(local_test_path, 0777);
49
50 /* ensure test directory exists now ... */
51 g_assert(stat(local_test_path, ) == 0);
52 /* ... and is actually a directory */
53 g_assert((st.st_mode & S_IFMT) == S_IFDIR);
So let's just do that and log an info-level message at least, because we
actually only care if the required directory exists and we do have an
existence check for that in place already.
Reported-by: Coverity (CID 1435963)
Signed-off-by: Christian Schoenebeck
---
tests/qtest/libqos/virtio-9p.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index 6b22fa0e9a..8459a3ee58 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -48,9 +48,14 @@ static void init_local_test_path(void)
static void create_local_test_dir(void)
{
struct stat st;
+int res;
g_assert(local_test_path != NULL);
-mkdir(local_test_path, 0777);
+res = mkdir(local_test_path, 0777);
+if (res < 0) {
+g_test_message("mkdir('%s') failed: %s", local_test_path,
+ strerror(errno));
+}
/* ensure test directory exists now ... */
g_assert(stat(local_test_path, ) == 0);
--
2.20.1
Re: [PATCH 2/2] tests/9pfs: fix coverity error in create_local_test_dir()
On Freitag, 30. Oktober 2020 12:44:18 CET Greg Kurz wrote:
> On Fri, 30 Oct 2020 09:19:46 +0100
>
> Christian Schoenebeck wrote:
> > Coverity wants the return value of mkdir() to be checked, so let's
> > pretend to do that. We're actually just making a dummy check and
> > ignore the result, because we actually only care if the required
> > directory exists and we have an existence check for that in place
> > already.
>
> I see that sometimes changelog shows a copy of the original
> coverity report (e.g. commit df1a312fea58).
Ok, I'll add that.
> > Reported-by: Greg Kurz
>
> Please give credits to coverity, not me :-)
>
> And most importantly, we want to mention the CID in the changelog.
>
> e.g.
>
> Reported-by: Coverity (CID 1435963)
Ok.
It's not clear to me where this coverity report is accessible online. A quick
search only brought me to statistics about its latest check, but not the
details of the report you quoted.
And more importantly: is there coverity CI support that one could enable on
github, so that pending patches were checked before upstream merge?
>
> > Signed-off-by: Christian Schoenebeck
> > ---
> >
> > tests/qtest/libqos/virtio-9p.c | 6 +-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/tests/qtest/libqos/virtio-9p.c
> > b/tests/qtest/libqos/virtio-9p.c index 6b22fa0e9a..0a7c0ee5d8 100644
> > --- a/tests/qtest/libqos/virtio-9p.c
> > +++ b/tests/qtest/libqos/virtio-9p.c
> > @@ -48,9 +48,13 @@ static void init_local_test_path(void)
> >
> > static void create_local_test_dir(void)
> > {
> >
> > struct stat st;
> >
> > +int res;
> >
> > g_assert(local_test_path != NULL);
> >
> > -mkdir(local_test_path, 0777);
> > +res = mkdir(local_test_path, 0777);
> > +if (res < 0) {
> > +/* ignore error, dummy check to prevent error by coverity */
>
> Why not printing an error message with errno there like you did in
> the previous patch ?
Yeah, originally I didn't want to trigger false positives on automated CIs if
mkdir() failed just because the directory already exists. But OTOH
g_test_message() is just an info-level message, so it is Ok to bark silently
and I will add it.
>
> > +}
> >
> > /* ensure test directory exists now ... */
> > g_assert(stat(local_test_path, ) == 0);
Thanks!
Best regards,
Christian Schoenebeck
[PATCH 2/2] tests/9pfs: fix coverity error in create_local_test_dir()
Coverity wants the return value of mkdir() to be checked, so let's
pretend to do that. We're actually just making a dummy check and
ignore the result, because we actually only care if the required
directory exists and we have an existence check for that in place
already.
Reported-by: Greg Kurz
Signed-off-by: Christian Schoenebeck
---
tests/qtest/libqos/virtio-9p.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index 6b22fa0e9a..0a7c0ee5d8 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -48,9 +48,13 @@ static void init_local_test_path(void)
static void create_local_test_dir(void)
{
struct stat st;
+int res;
g_assert(local_test_path != NULL);
-mkdir(local_test_path, 0777);
+res = mkdir(local_test_path, 0777);
+if (res < 0) {
+/* ignore error, dummy check to prevent error by coverity */
+}
/* ensure test directory exists now ... */
g_assert(stat(local_test_path, ) == 0);
--
2.20.1
[PATCH 0/2] 9pfs: test suite fixes
Fixes two bugs with the 9pfs 'local' tests as discussed with latest 9P PR (2020-10-23). See the discussion of that PR for details. Christian Schoenebeck (2): tests/9pfs: fix test dir for parallel tests tests/9pfs: fix coverity error in create_local_test_dir() tests/qtest/libqos/virtio-9p.c | 31 --- 1 file changed, 24 insertions(+), 7 deletions(-) -- 2.20.1
[PATCH 1/2] tests/9pfs: fix test dir for parallel tests
Use mkdtemp() to generate a unique directory for the 9p 'local' tests.
This fixes occasional 9p test failures when running 'make check -jN' if
QEMU was compiled for multiple target architectures, because the individual
architecture's test suites would run in parallel and interfere with each
other's data as the test directory was previously hard coded and hence the
same directory was used by all of them simultaniously.
This also requires a change how the test directory is created and deleted:
As the test path is now randomized and virtio_9p_register_nodes() being
called in a somewhat undeterministic way, that's no longer an appropriate
place to create and remove the test directory. Use a constructor and
destructor function for creating and removing the test directory instead.
Unfortunately libqos currently does not support setup/teardown callbacks
to handle this more cleanly.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/libqos/virtio-9p.c | 25 +++--
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index d43647b3b7..6b22fa0e9a 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -35,7 +35,12 @@ static char *concat_path(const char* a, const char* b)
static void init_local_test_path(void)
{
char *pwd = g_get_current_dir();
-local_test_path = concat_path(pwd, "qtest-9p-local");
+char *template = concat_path(pwd, "qtest-9p-local-XX");
+local_test_path = mkdtemp(template);
+if (!local_test_path) {
+g_test_message("mkdtemp('%s') failed: %s", template, strerror(errno));
+}
+g_assert(local_test_path);
g_free(pwd);
}
@@ -246,11 +251,6 @@ static void virtio_9p_register_nodes(void)
const char *str_simple = "fsdev=fsdev0,mount_tag=" MOUNT_TAG;
const char *str_addr = "fsdev=fsdev0,addr=04.0,mount_tag=" MOUNT_TAG;
-/* make sure test dir for the 'local' tests exists and is clean */
-init_local_test_path();
-remove_local_test_dir();
-create_local_test_dir();
-
QPCIAddress addr = {
.devfn = QPCI_DEVFN(4, 0),
};
@@ -278,3 +278,16 @@ static void virtio_9p_register_nodes(void)
}
libqos_init(virtio_9p_register_nodes);
+
+static void __attribute__((constructor)) construct_virtio_9p(void)
+{
+/* make sure test dir for the 'local' tests exists */
+init_local_test_path();
+create_local_test_dir();
+}
+
+static void __attribute__((destructor)) destruct_virtio_9p(void)
+{
+/* remove previously created test dir when test suite completed */
+remove_local_test_dir();
+}
--
2.20.1
Re: [PULL v3 3/6] tests/9pfs: introduce local tests
On Donnerstag, 29. Oktober 2020 19:02:34 CET Greg Kurz wrote:
> On Thu, 8 Oct 2020 20:34:56 +0200
>
> Christian Schoenebeck wrote:
> > This patch introduces 9pfs test cases using the 9pfs 'local'
> > filesystem driver which reads/writes/creates/deletes real files
> > and directories.
> >
> > In this initial version, there is only one local test which actually
> > only checks if the 9pfs 'local' device was created successfully.
> >
> > Before the 9pfs 'local' tests are run, a test directory 'qtest-9p-local'
> > is created (with world rwx permissions) under the current working
> > directory. At this point that test directory is not auto deleted yet.
> >
> > Signed-off-by: Christian Schoenebeck
> > Message-Id:
> > <81fc4b3b6b6c9bf7999e79f5e7cbc364a5f09ddb.1602182956.git.qemu_oss@crudeby
> > te.com> Signed-off-by: Christian Schoenebeck
> > ---
> >
> > tests/qtest/libqos/virtio-9p.c | 81 ++
> > tests/qtest/libqos/virtio-9p.h | 5 +++
> > tests/qtest/virtio-9p-test.c | 44 --
> > 3 files changed, 116 insertions(+), 14 deletions(-)
> >
> > diff --git a/tests/qtest/libqos/virtio-9p.c
> > b/tests/qtest/libqos/virtio-9p.c index 2e300063e3..ee331166de 100644
> > --- a/tests/qtest/libqos/virtio-9p.c
> > +++ b/tests/qtest/libqos/virtio-9p.c
> > @@ -24,6 +24,34 @@
> >
> > #include "qgraph.h"
> >
> > static QGuestAllocator *alloc;
> >
> > +static char *local_test_path;
> > +
> > +/* Concatenates the passed 2 pathes. Returned result must be freed. */
> > +static char *concat_path(const char* a, const char* b)
> > +{
> > +return g_build_filename(a, b, NULL);
> > +}
> > +
> > +static void init_local_test_path(void)
> > +{
> > +char *pwd = g_get_current_dir();
> > +local_test_path = concat_path(pwd, "qtest-9p-local");
> > +g_free(pwd);
> > +}
> > +
> > +/* Creates the directory for the 9pfs 'local' filesystem driver to
> > access. */ +static void create_local_test_dir(void)
> > +{
> > +struct stat st;
> > +
> > +g_assert(local_test_path != NULL);
> > +mkdir(local_test_path, 0777);
> > +
>
> This makes coverity unhappy...
>
> *** CID 1435963: Error handling issues (CHECKED_RETURN)
> /qemu/tests/qtest/libqos/virtio-9p.c: 48 in create_local_test_dir()
> 42 /* Creates the directory for the 9pfs 'local' filesystem driver to
> access. */ 43 static void create_local_test_dir(void)
> 44 {
> 45 struct stat st;
> 46
> 47 g_assert(local_test_path != NULL);
>
> >>> CID 1435963: Error handling issues (CHECKED_RETURN)
> >>> Calling "mkdir(local_test_path, 511U)" without checking return
> >>> value. This library function may fail and return an error code.
> 48 mkdir(local_test_path, 0777);
> 49
> 50 /* ensure test directory exists now ... */
> 51 g_assert(stat(local_test_path, ) == 0);
> 52 /* ... and is actually a directory */
> 53 g_assert((st.st_mode & S_IFMT) == S_IFDIR);
>
> > +/* ensure test directory exists now ... */
> > +g_assert(stat(local_test_path, ) == 0);
> > +/* ... and is actually a directory */
> > +g_assert((st.st_mode & S_IFMT) == S_IFDIR);
> > +}
> >
Ok, I'll fix that with tomorrow's patch(es) as well.
Best regards,
Christian Schoenebeck
Re: [PULL 00/13] 9p queue 2020-10-23
On Donnerstag, 29. Oktober 2020 16:04:03 CET Daniel P. Berrangé wrote:
> On Thu, Oct 29, 2020 at 02:52:16PM +, Peter Maydell wrote:
> > On Thu, 29 Oct 2020 at 14:31, Christian Schoenebeck
> >
> > wrote:
> > > On Donnerstag, 29. Oktober 2020 15:15:19 CET Peter Maydell wrote:
> > > > On Thu, 29 Oct 2020 at 14:06, Christian Schoenebeck
> > > >
> > > > wrote:
> > > > > Ok, I'll use mkdtemp() instead, that avoids other potential parallel
> > > > > config
> > > > > colissions that I may not have considered yet.
> > > > >
> > > > > The original motivation against mkdtemp() was that /tmp is isually a
> > > > > ramfs,
> > > > > hence very limited regarding large file tests. But that's not an
> > > > > issue
> > > > > right now.
> > > >
> > > > How large is "large" here ?
> > >
> > > E.g. ~10 GiB which might be a problem for cloud based CI platforms.
> >
> > Yeah, 10GB is too big by an order of magnitude for anything in the
> > standard "make check" set. It could go in an optional "I'm the 9p
> > maintainer and I want to run a wider set of tests" target though.
>
> I think as a rough rule of thumb, tests should not create files
> that are larger than the size of the QEMU build dir, and if it is
> creating non-trivially sized files, then they should be created in
> the build dir, not /tmp. This probably puts 1 GB as a upper bound
> on size but bear in mind tests can run in parallel, so ideally biggest
> file sizes should be more in the 100s of MB range
>
> Regards,
> Daniel
I definitely won't run such large-file tests uncaged, so it would not run by
default for sure. But sooner or later it would make sense to streamline test
case options in QEMU in general, so that certain tests would automatically run
or not depending on runner's capabilities, limits and preferences.
The TCG tests for instance periodically trigger test failures as they exceed
Travis-CI's runtime limit of 40mins once in a while.
Back to the actual 9p test failure issue, I think I will go for something like
this:
diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index d43647b3b7..ebbacd5896 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -35,7 +35,12 @@ static char *concat_path(const char* a, const char* b)
static void init_local_test_path(void)
{
char *pwd = g_get_current_dir();
-local_test_path = concat_path(pwd, "qtest-9p-local");
+char *template = concat_path(pwd, "qtest-9p-local-XX");
+local_test_path = mkdtemp(template);
+if (!local_test_path) {
+g_test_message("mkdtemp('%s') failed: %s", template, strerror(errno));
+}
+g_assert(local_test_path);
g_free(pwd);
}
@@ -246,11 +251,6 @@ static void virtio_9p_register_nodes(void)
const char *str_simple = "fsdev=fsdev0,mount_tag=" MOUNT_TAG;
const char *str_addr = "fsdev=fsdev0,addr=04.0,mount_tag=" MOUNT_TAG;
-/* make sure test dir for the 'local' tests exists and is clean */
-init_local_test_path();
-remove_local_test_dir();
-create_local_test_dir();
-
QPCIAddress addr = {
.devfn = QPCI_DEVFN(4, 0),
};
@@ -278,3 +278,14 @@ static void virtio_9p_register_nodes(void)
}
libqos_init(virtio_9p_register_nodes);
+
+static void __attribute__((constructor)) construct_virtio_9p(void) {
+/* make sure test dir for the 'local' tests exists */
+init_local_test_path();
+create_local_test_dir();
+}
+
+static void __attribute__((destructor)) destruct_virtio_9p(void) {
+/* remove previously created test dir when test suite completed */
+remove_local_test_dir();
+}
So it would still use the current directory instead of /tmp/ and would
create and remove the test dir on process start/end. I wanted to avoid
using constructor/destructor atttributes, as some compilers don't
support them, but it seems it's already used in the code base and there
are currently no setup/teardown callbacks in libqos.
Another downside: with 'make check -jN' this will temporarily create a
bunch of unused, empty dirs. But I hope that's Ok to fix in QEMU 6
(by adding setup/teardown callbacks to libqos later).
Just hoping there are no glib versions that use threads instead of
processes.
Best regards,
Christian Schoenebeck
Re: [PULL 00/13] 9p queue 2020-10-23
On Donnerstag, 29. Oktober 2020 15:15:19 CET Peter Maydell wrote: > On Thu, 29 Oct 2020 at 14:06, Christian Schoenebeck > > wrote: > > Ok, I'll use mkdtemp() instead, that avoids other potential parallel > > config > > colissions that I may not have considered yet. > > > > The original motivation against mkdtemp() was that /tmp is isually a > > ramfs, > > hence very limited regarding large file tests. But that's not an issue > > right now. > > How large is "large" here ? > > thanks > -- PMM E.g. ~10 GiB which might be a problem for cloud based CI platforms. But again, we don't have any 9p test doing that yet. So mkdtemp() is just fine for now. Best regards, Christian Schoenebeck
Re: [PULL 00/13] 9p queue 2020-10-23
On Donnerstag, 29. Oktober 2020 14:57:45 CET Peter Maydell wrote: > On Thu, 29 Oct 2020 at 13:48, Christian Schoenebeck > > wrote: > > So I'll append the architecture to the test dir location. To nail it this > > time, anyting else that would come to your mind regarding test dirs? > > I think most tests that need a temp directory set one > up using mkdtemp(). > > thanks > -- PMM Ok, I'll use mkdtemp() instead, that avoids other potential parallel config colissions that I may not have considered yet. The original motivation against mkdtemp() was that /tmp is isually a ramfs, hence very limited regarding large file tests. But that's not an issue right now. Thanks Peter! Best regards, Christian Schoenebeck
Re: [PULL 00/13] 9p queue 2020-10-23
On Donnerstag, 29. Oktober 2020 14:20:11 CET Peter Maydell wrote: > On Mon, 26 Oct 2020 at 12:48, Christian Schoenebeck > > wrote: > > On Montag, 26. Oktober 2020 11:33:42 CET Peter Maydell wrote: > > > I get a 'make check' failure on x86-64 Linux host: > > > > > > PASS 54 qtest-x86_64: qos-test > > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/vir > > > tio- 9p-tests/local/config PASS 55 qtest-x86_64: qos-test > > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/vir > > > tio- 9p-tests/local/create_dir PASS 56 qtest-x86_64: qos-test > > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/vir > > > tio- 9p-tests/local/unlinkat_dir PASS 57 qtest-x86_64: qos-test > > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/vir > > > tio- 9p-tests/local/create_file PASS 58 qtest-x86_64: qos-test > > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/vir > > > tio- 9p-tests/local/unlinkat_file PASS 59 qtest-x86_64: qos-test > > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/vir > > > tio- 9p-tests/local/symlink_file Received response 7 (RLERROR) instead > > > of 73 (RMKDIR) > > > Rlerror has errno 2 (No such file or directory) > > > ** > > > ERROR:../../tests/qtest/virtio-9p-test.c:300:v9fs_req_recv: assertion > > > failed (hdr.id == id): (7 == 73) > > > ERROR qtest-x86_64: qos-test - Bail out! > > > ERROR:../../tests/qtest/virtio-9ptest.c:300:v9fs_req_recv: assertion > > > failed (hdr.id == id): (7 == 73) > > > Makefile.mtest:3953: recipe for target 'run-test-492' failed > > I just got this again on an entirely different pullreq so that > suggests that this is indeed an intermittent currently in master: > > PASS 49 qtest-i386/qos-test > /i386/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p > -tests/synth/flush/ignored PASS 50 qtest-i386/qos-test > /i386/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p > -tests/synth/readdir/basic PASS 51 qtest-i386/qos-test > /i386/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p > -tests/synth/readdir/split_512 PASS 52 qtest-i386/qos-test > /i386/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p > -tests/synth/readdir/split_256 PASS 53 qtest-i386/qos-test > /i386/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p > -tests/synth/readdir/split_128 PASS 54 qtest-i386/qos-test > /i386/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p > -tests/local/config Received response 7 (RLERROR) instead of 73 (RMKDIR) > Rlerror has errno 2 (No such file or directory) > ** > ERROR:../../tests/qtest/virtio-9p-test.c:296:v9fs_req_recv: assertion > failed (hdr.id == id): (7 == 73) > ERROR qtest-i386/qos-test - Bail out! > ERROR:../../tests/qtest/virtio-9p-test.c:296:v9fs_req_recv: assertion > failed (hdr.id == id): (7 == 73) > Makefile.mtest:1857: recipe for target 'run-test-230' failed > > > So the 9p server is already failing to create the test case directory > > "./qtest-9p-local/05/" relative to your current working directory. > > This sounds suspicious, because there's nothing in that filename > that's specific to the test case being qtest-i386 and not > qtest-something-else. How does the test harness deal with the > possibility of the same virtio-9p-pci test being run in parallel > for multiple guest architectures under a make -jN setup ? Ah, now there we go! I was actually running the tests for 2 days and >3000 test suite runs now without a single 9p test failure, but ... not for multiple architectures simultaniously. Another point for centralizing test dir locations in future. > > > What puzzles me is that the previous test cases succeeded there, which all > > > > create their own test directory in the same way: > > ./qtest-9p-local/01/ > > ./qtest-9p-local/02/ (<-- dir vanishes after that test completed) > > ./qtest-9p-local/03/ > > ./qtest-9p-local/04/ > > ... > > After the build failed, the qtest-9p-local directory was empty. Yes, that suggests a parallel test suite was wiping the test directory './qtest-9p-local'. So I'll append the architecture to the test dir location. To nail it this time, anyting else that would come to your mind regarding test dirs? > > thanks > -- PMM Best regards, Christian Schoenebeck
Re: [PATCH] hw/9pfs: virtio-9p: Ensure config space is a multiple of 4 bytes
On Donnerstag, 29. Oktober 2020 09:25:41 CET Bin Meng wrote:
> From: Bin Meng
>
> At present the virtio device config space access is handled by the
> virtio_config_readX() and virtio_config_writeX() APIs. They perform
> a sanity check on the result of address plus size against the config
> space size before the access occurs.
Since I am not very familiar with the virtio implementation side, I hope
Michael would have a look at this patch.
But some comments from my side ...
>
> For unaligned access, the last converted naturally aligned access
> will fail the sanity check on 9pfs. For example, with a mount_tag
> `p9fs`, if guest software tries to read the mount_tag via a 4 byte
> read at the mount_tag offset which is not 4 byte aligned, the read
> result will be `p9\377\377`, which is wrong.
Why 4? Shouldn't this rather consider worst case alignment?
>
> This changes the size of device config space to be a multiple of 4
> bytes so that correct result can be returned in all circumstances.
>
> Signed-off-by: Bin Meng
> ---
>
> hw/9pfs/virtio-9p-device.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> index 14371a7..e6a1432 100644
> --- a/hw/9pfs/virtio-9p-device.c
> +++ b/hw/9pfs/virtio-9p-device.c
> @@ -201,6 +201,7 @@ static void virtio_9p_device_realize(DeviceState *dev,
> Error **errp)
> V9fsVirtioState *v = VIRTIO_9P(dev);
> V9fsState *s = >state;
> FsDriverEntry *fse = get_fsdev_fsentry(s->fsconf.fsdev_id);
> +size_t config_size;
>
> if (qtest_enabled() && fse) {
> fse->export_flags |= V9FS_NO_PERF_WARN;
> @@ -211,7 +212,8 @@ static void virtio_9p_device_realize(DeviceState *dev,
> Error **errp)
> }
>
> v->config_size = sizeof(struct virtio_9p_config) + strlen(s->fsconf.tag);
> -virtio_init(vdev, "virtio-9p", VIRTIO_ID_9P, v->config_size);
> +config_size = ROUND_UP(v->config_size, 4);
> +virtio_init(vdev, "virtio-9p", VIRTIO_ID_9P, config_size);
> v->vq = virtio_add_queue(vdev, MAX_REQ, handle_9p_output);
> }
Shouldn't this config_size correction rather be handled on virtio.c side
instead, i.e. in virtio_init()?
>
> --
> 2.7.4
Best regards,
Christian Schoenebeck
Re: [PATCH 1/3] hw/9pfs : add spaces around operator
On Mittwoch, 28. Oktober 2020 10:05:38 CET jokenzhang wrote:
> Signed-off-by: jokenzhang
> Signed-off-by: Kai Deng
> Reported-by: Euler Robot
> ---
> hw/9pfs/9p-local.c | 10 +-
> hw/9pfs/9p.c | 16
> 2 files changed, 13 insertions(+), 13 deletions(-)
It would make sense to mention the actual purpose of this patch: fix code
style.
Same applies to the other 2 patches.
>
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index 3107637209..af52c1daac 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -162,13 +162,13 @@ static void local_mapped_file_attr(int dirfd, const
> char *name, memset(buf, 0, ATTR_MAX);
> while (fgets(buf, ATTR_MAX, fp)) {
> if (!strncmp(buf, "virtfs.uid", 10)) {
> -stbuf->st_uid = atoi(buf+11);
> +stbuf->st_uid = atoi(buf + 11);
> } else if (!strncmp(buf, "virtfs.gid", 10)) {
> -stbuf->st_gid = atoi(buf+11);
> +stbuf->st_gid = atoi(buf + 11);
> } else if (!strncmp(buf, "virtfs.mode", 11)) {
> -stbuf->st_mode = atoi(buf+12);
> +stbuf->st_mode = atoi(buf + 12);
> } else if (!strncmp(buf, "virtfs.rdev", 11)) {
> -stbuf->st_rdev = atoi(buf+12);
> +stbuf->st_rdev = atoi(buf + 12);
> }
> memset(buf, 0, ATTR_MAX);
> }
> @@ -823,7 +823,7 @@ static int local_open2(FsContext *fs_ctx, V9fsPath
> *dir_path, const char *name, if (fd == -1) {
> goto out;
> }
> -credp->fc_mode = credp->fc_mode|S_IFREG;
> +credp->fc_mode = credp->fc_mode | S_IFREG;
> if (fs_ctx->export_flags & V9FS_SM_MAPPED) {
> /* Set cleint credentials in xattr */
> err = local_set_xattrat(dirfd, name, credp);
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 741d222c3f..94df440fc7 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -1091,7 +1091,7 @@ static mode_t v9mode_to_mode(uint32_t mode, V9fsString
> *extension) }
> }
>
> -if (!(ret&~0777)) {
> +if (!(ret & ~0777)) {
> ret |= S_IFREG;
> }
>
> @@ -2776,7 +2776,7 @@ static void coroutine_fn v9fs_create(void *opaque)
> v9fs_path_unlock(s);
> } else {
> err = v9fs_co_open2(pdu, fidp, , -1,
> -omode_to_uflags(mode)|O_CREAT, perm, );
> +omode_to_uflags(mode) | O_CREAT, perm, );
> if (err < 0) {
> goto out;
> }
> @@ -3428,7 +3428,7 @@ static int v9fs_fill_statfs(V9fsState *s, V9fsPDU
> *pdu, struct statfs *stbuf) * compute bsize factor based on host file
> system block size
> * and client msize
> */
> -bsize_factor = (s->msize - P9_IOHDRSZ)/stbuf->f_bsize;
> +bsize_factor = (s->msize - P9_IOHDRSZ) / stbuf->f_bsize;
> if (!bsize_factor) {
> bsize_factor = 1;
> }
> @@ -3440,9 +3440,9 @@ static int v9fs_fill_statfs(V9fsState *s, V9fsPDU
> *pdu, struct statfs *stbuf) * adjust(divide) the number of blocks, free
> blocks and available * blocks by bsize factor
> */
> -f_blocks = stbuf->f_blocks/bsize_factor;
> -f_bfree = stbuf->f_bfree/bsize_factor;
> -f_bavail = stbuf->f_bavail/bsize_factor;
> +f_blocks = stbuf->f_blocks / bsize_factor;
> +f_bfree = stbuf->f_bfree / bsize_factor;
> +f_bavail = stbuf->f_bavail / bsize_factor;
> f_files = stbuf->f_files;
> f_ffree = stbuf->f_ffree;
> fsid_val = (unsigned int) stbuf->f_fsid.__val[0] |
> @@ -4185,6 +4185,6 @@ static void __attribute__((__constructor__))
> v9fs_set_fd_limit(void) error_report("Failed to get the resource limit");
> exit(1);
> }
> -open_fd_hw = rlim.rlim_cur - MIN(400, rlim.rlim_cur/3);
> -open_fd_rc = rlim.rlim_cur/2;
> +open_fd_hw = rlim.rlim_cur - MIN(400, rlim.rlim_cur / 3);
> +open_fd_rc = rlim.rlim_cur / 2;
> }
Best regards,
Christian Schoenebeck
Re: [PATCH v4 04/12] libqos/qgraph: add qos_dump_graph()
On Mittwoch, 28. Oktober 2020 14:00:21 CET Eric Blake wrote: > On 10/28/20 12:51 AM, Thomas Huth wrote: > >>>> +#define GREEN(txt) ( \ > >>>> +"\033[0;92m" txt \ > >>>> +"\033[0m" \ > >>>> +) > >>> > >>> I don't think this is very portable - and it will only make logs ugly to > >>> read in text editors. Could you please simply drop these macros? > > > > Sure, colored output is nice, but we certainly also need a way to disable > > it, e.g. if you want to collect the log in a file and then have a look at > > it in a text editor. > > Agreed. GNU libtextstyle > (https://www.gnu.org/software/gettext/libtextstyle/manual/libtextstyle.html) > is a much more portable way to do colored output where it becomes easy to > enable/disable or even adjust the colors to user preferences. Sadly, it is > GPLv3+, and thus unusable for qemu. But the bare minimum that you must > have when making colored output gated on whether stdout is a > terminal (that is, any program that does color should have a > --color=off|auto|on command-line option, and that in turn implies > function calls rather than macros to properly encapsulate the decision > logic. Not sure if it would make sense adding another dependency just for colour support in QEMU anyway, because rendering the right output sequence is not the big issue, nor auto detecting tty colour support, nor handling user configs. A large number of apps already do that in-tree / inline. The challenge in QEMU though (in contrast to stand-alone apps) is integrating this meaningful for all the (quite different) output channels in QEMU, e.g. host logs, test case output, different modes, etc., while catching misusage and retaining a simple API. I postpone the colour issue for that reason and drop colour from these patches for now. I'll probably rather come up with a dedicated series attempt just for colour at some later point. Best regards, Christian Schoenebeck
Re: [PULL 00/13] 9p queue 2020-10-23
On Dienstag, 27. Oktober 2020 11:26:53 CET Dr. David Alan Gilbert wrote: > * Christian Schoenebeck (qemu_...@crudebyte.com) wrote: > > On Dienstag, 27. Oktober 2020 10:06:53 CET Dr. David Alan Gilbert wrote: > > > * Greg Kurz (gr...@kaod.org) wrote: > > > > On Mon, 26 Oct 2020 13:48:37 +0100 > > > > > > > > Christian Schoenebeck wrote: > > > > > On Montag, 26. Oktober 2020 11:33:42 CET Peter Maydell wrote: > > > > > > On Fri, 23 Oct 2020 at 12:46, Christian Schoenebeck > > > > > > > > > > > > wrote: > > > > > > > The following changes since commit > > > > 4c5b97bfd0dd54dc27717ae8d1cd10e14eef1430: > > > > > > > Merge remote-tracking branch > > > > > > > 'remotes/kraxel/tags/modules-20201022-pull-request' into > > > > > > > staging > > > > > > > (2020-10-22 12:33:21 +0100)> > > > > > > > > > > > > > > are available in the Git repository at: > > > > > > > https://github.com/cschoenebeck/qemu.git tags/pull-9p-20201023 > > > > > > > > > > > > > > for you to fetch changes up to > > > > ee01926a11b1f9bffcd6cdec0961dd9d1882da71: > > > > > > > tests/9pfs: add local Tunlinkat hard link test (2020-10-22 > > > > > > > 20:26:33 > > > > > > > +0200) > > > > > > > > > > > > > > > > > > > > > 9pfs: more tests using local fs driver > > > > > > > > > > > > > > Only 9pfs test case changes this time: > > > > > > > > > > > > > > * Refactor: Rename functions to make top-level test functions > > > > > > > fs_*() > > > > > > > > > > > > > > easily distinguishable from utility test functions do_*(). > > > > > > > > > > > > > > * Refactor: Drop unnecessary function arguments in utility test > > > > > > > > > > > > > > functions. > > > > > > > > > > > > > > * More test cases using the 9pfs 'local' filesystem driver > > > > > > > backend, > > > > > > > > > > > > > > namely for the following 9p requests: Tunlinkat, Tlcreate, > > > > > > > Tsymlink > > > > > > > and Tlink. > > > > > > > > > > > > > > > > > > > > > > > > > > I get a 'make check' failure on x86-64 Linux host: > > > > > > > > > > > > PASS 54 qtest-x86_64: qos-test > > > > > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio- > > > > > > 9p/v > > > > > > irtio- 9p-tests/local/config PASS 55 qtest-x86_64: qos-test > > > > > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio- > > > > > > 9p/v > > > > > > irtio- 9p-tests/local/create_dir PASS 56 qtest-x86_64: qos-test > > > > > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio- > > > > > > 9p/v > > > > > > irtio- 9p-tests/local/unlinkat_dir PASS 57 qtest-x86_64: qos-test > > > > > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio- > > > > > > 9p/v > > > > > > irtio- 9p-tests/local/create_file PASS 58 qtest-x86_64: qos-test > > > > > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio- > > > > > > 9p/v > > > > > > irtio- 9p-tests/local/unlinkat_file PASS 59 qtest-x86_64: qos-test > > > > > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio- > > > > > > 9p/v > > > > > > irtio- 9p-tests/local/symlink_file Received response 7 (RLERROR) > > > > > > instead of 73 (RMKDIR) > > > > > > Rlerror has errno 2 (No such file or directory) > > > > > > ** > > > > > > ERROR:../../tests/qtest/virtio-9p-test.c:300:v9fs_req_recv: > > > > > > assertion > > > > > > failed (hdr.id == id): (7 == 73) > > > > > > > > Not sure this is related to this PR actually. Dave
Re: [PULL 00/13] 9p queue 2020-10-23
On Dienstag, 27. Oktober 2020 10:06:53 CET Dr. David Alan Gilbert wrote: > * Greg Kurz (gr...@kaod.org) wrote: > > On Mon, 26 Oct 2020 13:48:37 +0100 > > > > Christian Schoenebeck wrote: > > > On Montag, 26. Oktober 2020 11:33:42 CET Peter Maydell wrote: > > > > On Fri, 23 Oct 2020 at 12:46, Christian Schoenebeck > > > > > > > > wrote: > > > > > The following changes since commit 4c5b97bfd0dd54dc27717ae8d1cd10e14eef1430: > > > > > Merge remote-tracking branch > > > > > 'remotes/kraxel/tags/modules-20201022-pull-request' into staging > > > > > (2020-10-22 12:33:21 +0100)> > > > > > > > > > > are available in the Git repository at: > > > > > https://github.com/cschoenebeck/qemu.git tags/pull-9p-20201023 > > > > > > > > > > for you to fetch changes up to ee01926a11b1f9bffcd6cdec0961dd9d1882da71: > > > > > tests/9pfs: add local Tunlinkat hard link test (2020-10-22 > > > > > 20:26:33 > > > > > +0200) > > > > > > > > > > > > > > > 9pfs: more tests using local fs driver > > > > > > > > > > Only 9pfs test case changes this time: > > > > > > > > > > * Refactor: Rename functions to make top-level test functions fs_*() > > > > > > > > > > easily distinguishable from utility test functions do_*(). > > > > > > > > > > * Refactor: Drop unnecessary function arguments in utility test > > > > > > > > > > functions. > > > > > > > > > > * More test cases using the 9pfs 'local' filesystem driver backend, > > > > > > > > > > namely for the following 9p requests: Tunlinkat, Tlcreate, > > > > > Tsymlink > > > > > and Tlink. > > > > > > > > > > > > > > > > > > I get a 'make check' failure on x86-64 Linux host: > > > > > > > > PASS 54 qtest-x86_64: qos-test > > > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/v > > > > irtio- 9p-tests/local/config PASS 55 qtest-x86_64: qos-test > > > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/v > > > > irtio- 9p-tests/local/create_dir PASS 56 qtest-x86_64: qos-test > > > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/v > > > > irtio- 9p-tests/local/unlinkat_dir PASS 57 qtest-x86_64: qos-test > > > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/v > > > > irtio- 9p-tests/local/create_file PASS 58 qtest-x86_64: qos-test > > > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/v > > > > irtio- 9p-tests/local/unlinkat_file PASS 59 qtest-x86_64: qos-test > > > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/v > > > > irtio- 9p-tests/local/symlink_file Received response 7 (RLERROR) > > > > instead of 73 (RMKDIR) > > > > Rlerror has errno 2 (No such file or directory) > > > > ** > > > > ERROR:../../tests/qtest/virtio-9p-test.c:300:v9fs_req_recv: assertion > > > > failed (hdr.id == id): (7 == 73) > > > > Not sure this is related to this PR actually. Dave Gilbert reported on irc > > that he encountered a similar issue with 'make -j check', likely without > > these patches. > I was running on current master as of yesterday; no 9p specific patches. > > Dave They might be related as the "local/create_dir" test is already merged, but hard to say reliably without any data. How is reproducibility, sometimes / always? > > > > > ERROR qtest-x86_64: qos-test - Bail out! > > > > ERROR:../../tests/qtest/virtio-9ptest.c:300:v9fs_req_recv: assertion > > > > failed (hdr.id == id): (7 == 73) > > > > Makefile.mtest:3953: recipe for target 'run-test-492' failed > > > > > > > > > > > > thanks > > > > -- PMM > > > > > > So the 9p server is already failing to create the test case directory > > > "./qtest-9p-local/05/" relative to your current working directory. > > > > > > I would appreciate to get more info when you have some free cycles, as > > > I'm > > > unable to reproduce this on any s
Re: [PULL 00/13] 9p queue 2020-10-23
On Montag, 26. Oktober 2020 11:33:42 CET Peter Maydell wrote: > On Fri, 23 Oct 2020 at 12:46, Christian Schoenebeck > > wrote: > > The following changes since commit 4c5b97bfd0dd54dc27717ae8d1cd10e14eef1430: > > Merge remote-tracking branch > > 'remotes/kraxel/tags/modules-20201022-pull-request' into staging > > (2020-10-22 12:33:21 +0100)> > > are available in the Git repository at: > > https://github.com/cschoenebeck/qemu.git tags/pull-9p-20201023 > > > > for you to fetch changes up to ee01926a11b1f9bffcd6cdec0961dd9d1882da71: > > tests/9pfs: add local Tunlinkat hard link test (2020-10-22 20:26:33 > > +0200) > > > > > > 9pfs: more tests using local fs driver > > > > Only 9pfs test case changes this time: > > > > * Refactor: Rename functions to make top-level test functions fs_*() > > > > easily distinguishable from utility test functions do_*(). > > > > * Refactor: Drop unnecessary function arguments in utility test > > > > functions. > > > > * More test cases using the 9pfs 'local' filesystem driver backend, > > > > namely for the following 9p requests: Tunlinkat, Tlcreate, Tsymlink > > and Tlink. > > > > > > I get a 'make check' failure on x86-64 Linux host: > > PASS 54 qtest-x86_64: qos-test > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio- > 9p-tests/local/config PASS 55 qtest-x86_64: qos-test > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio- > 9p-tests/local/create_dir PASS 56 qtest-x86_64: qos-test > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio- > 9p-tests/local/unlinkat_dir PASS 57 qtest-x86_64: qos-test > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio- > 9p-tests/local/create_file PASS 58 qtest-x86_64: qos-test > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio- > 9p-tests/local/unlinkat_file PASS 59 qtest-x86_64: qos-test > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio- > 9p-tests/local/symlink_file Received response 7 (RLERROR) instead of 73 > (RMKDIR) > Rlerror has errno 2 (No such file or directory) > ** > ERROR:../../tests/qtest/virtio-9p-test.c:300:v9fs_req_recv: assertion > failed (hdr.id == id): (7 == 73) > ERROR qtest-x86_64: qos-test - Bail out! > ERROR:../../tests/qtest/virtio-9ptest.c:300:v9fs_req_recv: assertion > failed (hdr.id == id): (7 == 73) > Makefile.mtest:3953: recipe for target 'run-test-492' failed > > > thanks > -- PMM So the 9p server is already failing to create the test case directory "./qtest-9p-local/05/" relative to your current working directory. I would appreciate to get more info when you have some free cycles, as I'm unable to reproduce this on any system unfortunately. But no hurry as these tests only become relevant actually for QEMU 6. What puzzles me is that the previous test cases succeeded there, which all create their own test directory in the same way: ./qtest-9p-local/01/ ./qtest-9p-local/02/ (<-- dir vanishes after that test completed) ./qtest-9p-local/03/ ./qtest-9p-local/04/ ... How does the "./qtest-9p-local/" directory look like after that "local/symlink_file" test failed there? You can use this shortcut: export QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 cd build tests/qtest/qos-test --verbose ls -l qtest-9p-local That latter qos-test run will also output the assembled qemu command line the 9p local tests would run with, which might also be helpful, e.g. the relevant output would be something like this: GTest: run: /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p-tests/local/config (MSG: starting QEMU: exec x86_64-softmmu/qemu-system-x86_64 -qtest unix:/tmp/qtest-7428.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-7428.qmp,id=char0 -mon chardev=char0,mode=control -display none -M pc -fsdev local,id=fsdev0,path='/home/me/git/qemu/build/qtest-9p-local',security_model=mapped-xattr -device virtio-9p-pci,fsdev=fsdev0,addr=04.0,mount_tag=qtest -accel qtest) Would probably the test succeed if run alone? tests/qtest/qos-test -p /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p-tests/local/symlink_file Best regards, Christian Schoenebeck
Re: [PATCH v4 05/12] tests/qtest/qos-test: dump qos graph if verbose
On Samstag, 24. Oktober 2020 08:01:55 CEST Thomas Huth wrote:
> On 08/10/2020 20.34, Christian Schoenebeck wrote:
> > If qtests were run in verbose mode (i.e. if --verbose CL argument was
> > provided) then dump the generated qos graph (all nodes and edges,
> > along with their current individual availability status) to stdout.
> >
> > See API doc comment on function qos_dump_graph() for details.
> >
> > Signed-off-by: Christian Schoenebeck
> > ---
> >
> > tests/qtest/qos-test.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/tests/qtest/qos-test.c b/tests/qtest/qos-test.c
> > index 8fdf87b183..d98ef78613 100644
> > --- a/tests/qtest/qos-test.c
> > +++ b/tests/qtest/qos-test.c
> > @@ -322,6 +322,9 @@ int main(int argc, char **argv)
> >
> > qos_set_machines_devices_available();
> >
> > qos_graph_foreach_test_path(walk_path);
> >
> > +if (g_test_verbose()) {
> > +qos_dump_graph();
> > +}
> >
> > g_test_run();
> > qtest_end();
> > qos_graph_destroy();
>
> I'd squash this into the previous patch, so that it is clear there where the
> function gets used.
>
> Thomas
Sure, np.
Thanks for your feedback Thomas.
Best regards,
Christian Schoenebeck
Re: [PATCH v4 04/12] libqos/qgraph: add qos_dump_graph()
On Samstag, 24. Oktober 2020 08:04:20 CEST Thomas Huth wrote: > On 08/10/2020 20.34, Christian Schoenebeck wrote: > > This new function is purely for debugging purposes. It prints the > > current qos graph to stdout and allows to identify problems in the > > created qos graph e.g. when writing new qos tests. > > > > Coloured output is used to mark available nodes in green colour, > > whereas unavailable nodes are marked in red colour. > > > > Signed-off-by: Christian Schoenebeck > > --- > > > > tests/qtest/libqos/qgraph.c | 56 + > > tests/qtest/libqos/qgraph.h | 20 + > > 2 files changed, 76 insertions(+) > > > > diff --git a/tests/qtest/libqos/qgraph.c b/tests/qtest/libqos/qgraph.c > > index 61faf6b27d..af93e38dcb 100644 > > --- a/tests/qtest/libqos/qgraph.c > > +++ b/tests/qtest/libqos/qgraph.c > > @@ -805,3 +805,59 @@ void qos_delete_cmd_line(const char *name) > > > > node->command_line = NULL; > > > > } > > > > } > > > > + > > +#define RED(txt) (\ > > +"\033[0;91m" txt \ > > +"\033[0m" \ > > +) > > + > > +#define GREEN(txt) ( \ > > +"\033[0;92m" txt \ > > +"\033[0m" \ > > +) > > I don't think this is very portable - and it will only make logs ugly to > read in text editors. Could you please simply drop these macros? > > Thomas The precise way I did it here is definitely unclean. And the use case is trivial, so on doubt I could just drop it of course. But allow me one attempt to promote coloured terminal output in general: These are ANSI color escape sequences, a standard with its youngest revision dating back to 1991. It is a well supported standard on all major platforms nowadays: https://en.wikipedia.org/wiki/ANSI_escape_code It works on macOS's standard terminal for at least 20 years, with cmd.exe on Windows 10, on essentially all Linux and BSD distros, and even on many web based CI platforms. So what about introducing some globally shared macros for coloured output instead? Then there would be one central place for changing coloured output support for the entire code base; and I would change the macros to fallback to plain text output if there is any doubt the terminal would not support it. Besides, QEMU just switched to meson which uses coloured output as well, as do clang, GCC, git and many other tools in your build chain. Best regards, Christian Schoenebeck
Re: [PATCH v4 06/12] tests/qtest/qos-test: dump environment variables if verbose
On Samstag, 24. Oktober 2020 07:56:10 CEST Thomas Huth wrote: > On 08/10/2020 20.34, Christian Schoenebeck wrote: > > If qtests are run in verbose mode (i.e. if --verbose CL argument > > was provided) then print all environment variables to stdout > > before running the individual tests. > > Why? ... you should provide some rationale in the patch description here, at > least to me this is not obvious why it is needed / desired. > > Thomas In my particular case I wanted to know whether there is already some config vector for 'please use this test directory for file tests'. As I didn't find one in any API, I also looked for environment variables to be sure. Especially as there are a bunch of qtest related environment variables already. In general though it is common nowadays, at least being able to output all config vectors in a build chain, especially if it is required to investigate build- and test-issues on foreign/remote machines, which includes environment variables. Staying in the context of writing test cases: there are a bunch of other use cases that would come to my mind from the PoV of a test case author: "Is there an option for short vs. long tests?", "Is there a desired size limitation for large file tests?", "Is there a deadline for the runtime of tests?", ... Best regards, Christian Schoenebeck
Re: [PATCH v4 01/12] libqos/qgraph: add qemu_name to QOSGraphNode
On Samstag, 24. Oktober 2020 08:08:59 CEST Thomas Huth wrote: > On 19/10/2020 12.35, Christian Schoenebeck wrote: > > On Donnerstag, 8. Oktober 2020 20:34:56 CEST Christian Schoenebeck wrote: > >> Add new member variable 'qemu_name' to struct QOSGraphNode. > >> > >> This new member may be optionally set in case a different > >> name for the node (which must always be a unique name) vs. > >> its actually associated QEMU (QMP) device name is required. > >> > >> Signed-off-by: Christian Schoenebeck > >> --- > >> > >> tests/qtest/libqos/qgraph.c | 1 + > >> tests/qtest/libqos/qgraph_internal.h | 1 + > >> 2 files changed, 2 insertions(+) > > > > So what shall happen with these libqos patches 1..7? Is that a nack, or > > postpone for now? > > I was hoping to see a review by Paolo or Laurent, who are much more familiar > with qos than I am ... but after having a look at the patches, I think I > can also give some feedback, too: > > Patch 1 and 2 sound basically ok to me (should maybe be squashed together, > though), but the qos_node_create_driver_named() function currently seems to > be unused so far? So I'd postpone these two patches to the point in time > when you really need the qos_node_create_driver_named() function. I did use patches 1 & 2 in v1 of this series, as of v2 and higher I used a workaround for the actual 9pfs test case patches not to depend on these 2 libqos patches. This happened after Paolo's feedback, who wrote that qos patches 1 & 2 would be useful for other, future use cases, but argued it would not be appropriate for my intended use case: https://lore.kernel.org/qemu-devel/95ef57d0-b35e-f16a-f957-06bc3692c...@redhat.com/ I preserved patches 1 & 2 in this series though as he noted they might be useful for future purposes and applied his requested changes. I personally probably won't need thise 2 patches any time soon. So it's up to you what shall happen with them. I don't mind if you postpone or nack them. > The other patches are basically fine with me, too, but please avoid the > hard-coded ESC codes that only work with certain terminals. > > Thomas I'll respond to that on your patch 4 response. Best regards, Christian Schoenebeck
Re: [PULL 01/13] tests/9pfs: Factor out do_version() helper
On Freitag, 23. Oktober 2020 17:32:02 CEST Greg Kurz wrote: > It feels weird to receive a mail I didn't send with my address > in the top From: header. I would have expected yours instead and > another From: with my address in the changelog... > > > On Tue, 20 Oct 2020 18:09:14 +0200 > Greg Kurz wrote: > > ... here. Yep, I noticed that too late. I'll take care about it next time. Sorry! Best regards, Christian Schoenebeck
[PULL 12/13] tests/9pfs: add local Tlink test
This test case uses a Tlink request to create a hard link to a regular
file using the 9pfs 'local' fs driver.
Signed-off-by: Christian Schoenebeck
Reviewed-by: Greg Kurz
Message-Id:
Signed-off-by: Christian Schoenebeck
---
tests/qtest/virtio-9p-test.c | 71
1 file changed, 71 insertions(+)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 33cba24b18..460fa49fe3 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -260,6 +260,7 @@ static const char *rmessage_name(uint8_t id)
id == P9_RMKDIR ? "RMKDIR" :
id == P9_RLCREATE ? "RLCREATE" :
id == P9_RSYMLINK ? "RSYMLINK" :
+id == P9_RLINK ? "RLINK" :
id == P9_RUNLINKAT ? "RUNLINKAT" :
id == P9_RFLUSH ? "RFLUSH" :
id == P9_RREADDIR ? "READDIR" :
@@ -767,6 +768,33 @@ static void v9fs_rsymlink(P9Req *req, v9fs_qid *qid)
v9fs_req_free(req);
}
+/* size[4] Tlink tag[2] dfid[4] fid[4] name[s] */
+static P9Req *v9fs_tlink(QVirtio9P *v9p, uint32_t dfid, uint32_t fid,
+ const char *name, uint16_t tag)
+{
+P9Req *req;
+
+uint32_t body_size = 4 + 4;
+uint16_t string_size = v9fs_string_size(name);
+
+g_assert_cmpint(body_size, <=, UINT32_MAX - string_size);
+body_size += string_size;
+
+req = v9fs_req_init(v9p, body_size, P9_TLINK, tag);
+v9fs_uint32_write(req, dfid);
+v9fs_uint32_write(req, fid);
+v9fs_string_write(req, name);
+v9fs_req_send(req);
+return req;
+}
+
+/* size[4] Rlink tag[2] */
+static void v9fs_rlink(P9Req *req)
+{
+v9fs_req_recv(req, P9_RLINK);
+v9fs_req_free(req);
+}
+
/* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */
static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char *name,
uint32_t flags, uint16_t tag)
@@ -1142,6 +1170,21 @@ static void do_symlink(QVirtio9P *v9p, const char *path,
const char *clink,
g_free(name);
}
+/* create a hard link named @a clink in directory @a path pointing to @a to */
+static void do_hardlink(QVirtio9P *v9p, const char *path, const char *clink,
+const char *to)
+{
+uint32_t dfid, fid;
+P9Req *req;
+
+dfid = do_walk(v9p, path);
+fid = do_walk(v9p, to);
+
+req = v9fs_tlink(v9p, dfid, fid, clink, 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_rlink(req);
+}
+
static void do_unlinkat(QVirtio9P *v9p, const char *atpath, const char *rpath,
uint32_t flags)
{
@@ -1321,6 +1364,33 @@ static void fs_unlinkat_symlink(void *obj, void *data,
g_free(real_file);
}
+static void fs_hardlink_file(void *obj, void *data, QGuestAllocator *t_alloc)
+{
+QVirtio9P *v9p = obj;
+alloc = t_alloc;
+struct stat st_real, st_link;
+char *real_file = virtio_9p_test_path("07/real_file");
+char *hardlink_file = virtio_9p_test_path("07/hardlink_file");
+
+do_attach(v9p);
+do_mkdir(v9p, "/", "07");
+do_lcreate(v9p, "07", "real_file");
+g_assert(stat(real_file, _real) == 0);
+g_assert((st_real.st_mode & S_IFMT) == S_IFREG);
+
+do_hardlink(v9p, "07", "hardlink_file", "07/real_file");
+
+/* check if link exists now ... */
+g_assert(stat(hardlink_file, _link) == 0);
+/* ... and it's a hard link, right? */
+g_assert((st_link.st_mode & S_IFMT) == S_IFREG);
+g_assert(st_link.st_dev == st_real.st_dev);
+g_assert(st_link.st_ino == st_real.st_ino);
+
+g_free(hardlink_file);
+g_free(real_file);
+}
+
static void *assign_9p_local_driver(GString *cmd_line, void *arg)
{
virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1367,6 +1437,7 @@ static void register_virtio_9p_test(void)
qos_add_test("local/symlink_file", "virtio-9p", fs_symlink_file, );
qos_add_test("local/unlinkat_symlink", "virtio-9p", fs_unlinkat_symlink,
);
+qos_add_test("local/hardlink_file", "virtio-9p", fs_hardlink_file, );
}
libqos_init(register_virtio_9p_test);
--
2.20.1
[PULL 07/13] tests/9pfs: add local Tunlinkat directory test
This test case uses a Tunlinkat 9p request with flag AT_REMOVEDIR
(see 'man 2 unlink') to remove a directory from host's test directory.
Signed-off-by: Christian Schoenebeck
Reviewed-by: Greg Kurz
Message-Id:
<3c7c65b476ba44bea6afd0b378b5287e1c671a32.1603285620.git.qemu_...@crudebyte.com>
Signed-off-by: Christian Schoenebeck
---
tests/qtest/virtio-9p-test.c | 71
1 file changed, 71 insertions(+)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 21807037df..abd7e44648 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -258,6 +258,7 @@ static const char *rmessage_name(uint8_t id)
id == P9_RLOPEN ? "RLOPEN" :
id == P9_RWRITE ? "RWRITE" :
id == P9_RMKDIR ? "RMKDIR" :
+id == P9_RUNLINKAT ? "RUNLINKAT" :
id == P9_RFLUSH ? "RFLUSH" :
id == P9_RREADDIR ? "READDIR" :
"";
@@ -693,6 +694,33 @@ static void v9fs_rmkdir(P9Req *req, v9fs_qid *qid)
v9fs_req_free(req);
}
+/* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */
+static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char *name,
+ uint32_t flags, uint16_t tag)
+{
+P9Req *req;
+
+uint32_t body_size = 4 + 4;
+uint16_t string_size = v9fs_string_size(name);
+
+g_assert_cmpint(body_size, <=, UINT32_MAX - string_size);
+body_size += string_size;
+
+req = v9fs_req_init(v9p, body_size, P9_TUNLINKAT, tag);
+v9fs_uint32_write(req, dirfd);
+v9fs_string_write(req, name);
+v9fs_uint32_write(req, flags);
+v9fs_req_send(req);
+return req;
+}
+
+/* size[4] Runlinkat tag[2] */
+static void v9fs_runlinkat(P9Req *req)
+{
+v9fs_req_recv(req, P9_RUNLINKAT);
+v9fs_req_free(req);
+}
+
/* basic readdir test where reply fits into a single response message */
static void fs_readdir(void *obj, void *data, QGuestAllocator *t_alloc)
{
@@ -1004,6 +1032,22 @@ static void do_mkdir(QVirtio9P *v9p, const char *path,
const char *cname)
g_free(name);
}
+static void do_unlinkat(QVirtio9P *v9p, const char *atpath, const char *rpath,
+uint32_t flags)
+{
+char *const name = g_strdup(rpath);
+uint32_t fid;
+P9Req *req;
+
+fid = do_walk(v9p, atpath);
+
+req = v9fs_tunlinkat(v9p, fid, name, flags, 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_runlinkat(req);
+
+g_free(name);
+}
+
static void fs_readdir_split_128(void *obj, void *data,
QGuestAllocator *t_alloc)
{
@@ -1050,6 +1094,32 @@ static void fs_create_dir(void *obj, void *data,
QGuestAllocator *t_alloc)
g_free(root_path);
}
+static void fs_unlinkat_dir(void *obj, void *data, QGuestAllocator *t_alloc)
+{
+QVirtio9P *v9p = obj;
+alloc = t_alloc;
+struct stat st;
+char *root_path = virtio_9p_test_path("");
+char *new_dir = virtio_9p_test_path("02");
+
+g_assert(root_path != NULL);
+
+do_attach(v9p);
+do_mkdir(v9p, "/", "02");
+
+/* check if created directory really exists now ... */
+g_assert(stat(new_dir, ) == 0);
+/* ... and is actually a directory */
+g_assert((st.st_mode & S_IFMT) == S_IFDIR);
+
+do_unlinkat(v9p, "/", "02", AT_REMOVEDIR);
+/* directory should be gone now */
+g_assert(stat(new_dir, ) != 0);
+
+g_free(new_dir);
+g_free(root_path);
+}
+
static void *assign_9p_local_driver(GString *cmd_line, void *arg)
{
virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1090,6 +1160,7 @@ static void register_virtio_9p_test(void)
opts.before = assign_9p_local_driver;
qos_add_test("local/config", "virtio-9p", pci_config, );
qos_add_test("local/create_dir", "virtio-9p", fs_create_dir, );
+qos_add_test("local/unlinkat_dir", "virtio-9p", fs_unlinkat_dir, );
}
libqos_init(register_virtio_9p_test);
--
2.20.1
[PULL 13/13] tests/9pfs: add local Tunlinkat hard link test
This test case uses a Tunlinkat request to remove a previously hard
linked file by using the 9pfs 'local' fs driver.
Signed-off-by: Christian Schoenebeck
Reviewed-by: Greg Kurz
Message-Id:
<9bec33a7d8f006ef8f80517985d0d6ac48650d53.1603285620.git.qemu_...@crudebyte.com>
Signed-off-by: Christian Schoenebeck
---
tests/qtest/virtio-9p-test.c | 30 ++
1 file changed, 30 insertions(+)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 460fa49fe3..23433913bb 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -1391,6 +1391,34 @@ static void fs_hardlink_file(void *obj, void *data,
QGuestAllocator *t_alloc)
g_free(real_file);
}
+static void fs_unlinkat_hardlink(void *obj, void *data,
+ QGuestAllocator *t_alloc)
+{
+QVirtio9P *v9p = obj;
+alloc = t_alloc;
+struct stat st_real, st_link;
+char *real_file = virtio_9p_test_path("08/real_file");
+char *hardlink_file = virtio_9p_test_path("08/hardlink_file");
+
+do_attach(v9p);
+do_mkdir(v9p, "/", "08");
+do_lcreate(v9p, "08", "real_file");
+g_assert(stat(real_file, _real) == 0);
+g_assert((st_real.st_mode & S_IFMT) == S_IFREG);
+
+do_hardlink(v9p, "08", "hardlink_file", "08/real_file");
+g_assert(stat(hardlink_file, _link) == 0);
+
+do_unlinkat(v9p, "08", "hardlink_file", 0);
+/* symlink should be gone now */
+g_assert(stat(hardlink_file, _link) != 0);
+/* and old file should still exist */
+g_assert(stat(real_file, _real) == 0);
+
+g_free(hardlink_file);
+g_free(real_file);
+}
+
static void *assign_9p_local_driver(GString *cmd_line, void *arg)
{
virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1438,6 +1466,8 @@ static void register_virtio_9p_test(void)
qos_add_test("local/unlinkat_symlink", "virtio-9p", fs_unlinkat_symlink,
);
qos_add_test("local/hardlink_file", "virtio-9p", fs_hardlink_file, );
+qos_add_test("local/unlinkat_hardlink", "virtio-9p", fs_unlinkat_hardlink,
+ );
}
libqos_init(register_virtio_9p_test);
--
2.20.1
[PULL 11/13] tests/9pfs: add local Tunlinkat symlink test
This test case uses a Tunlinkat request to remove a symlink using
the 9pfs 'local' fs driver.
Signed-off-by: Christian Schoenebeck
Reviewed-by: Greg Kurz
Message-Id:
Signed-off-by: Christian Schoenebeck
---
tests/qtest/virtio-9p-test.c | 28
1 file changed, 28 insertions(+)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 0c11417236..33cba24b18 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -1295,6 +1295,32 @@ static void fs_symlink_file(void *obj, void *data,
QGuestAllocator *t_alloc)
g_free(real_file);
}
+static void fs_unlinkat_symlink(void *obj, void *data,
+QGuestAllocator *t_alloc)
+{
+QVirtio9P *v9p = obj;
+alloc = t_alloc;
+struct stat st;
+char *real_file = virtio_9p_test_path("06/real_file");
+char *symlink_file = virtio_9p_test_path("06/symlink_file");
+
+do_attach(v9p);
+do_mkdir(v9p, "/", "06");
+do_lcreate(v9p, "06", "real_file");
+g_assert(stat(real_file, ) == 0);
+g_assert((st.st_mode & S_IFMT) == S_IFREG);
+
+do_symlink(v9p, "06", "symlink_file", "real_file");
+g_assert(stat(symlink_file, ) == 0);
+
+do_unlinkat(v9p, "06", "symlink_file", 0);
+/* symlink should be gone now */
+g_assert(stat(symlink_file, ) != 0);
+
+g_free(symlink_file);
+g_free(real_file);
+}
+
static void *assign_9p_local_driver(GString *cmd_line, void *arg)
{
virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1339,6 +1365,8 @@ static void register_virtio_9p_test(void)
qos_add_test("local/create_file", "virtio-9p", fs_create_file, );
qos_add_test("local/unlinkat_file", "virtio-9p", fs_unlinkat_file, );
qos_add_test("local/symlink_file", "virtio-9p", fs_symlink_file, );
+qos_add_test("local/unlinkat_symlink", "virtio-9p", fs_unlinkat_symlink,
+ );
}
libqos_init(register_virtio_9p_test);
--
2.20.1
[PULL 06/13] tests/9pfs: simplify do_mkdir()
Split out walking a directory path to a separate new utility function
do_walk() and use that function in do_mkdir().
The code difference saved this way is not much, but we'll use that new
do_walk() function in the upcoming patches, so it will avoid quite
some code duplication after all.
Signed-off-by: Christian Schoenebeck
Reviewed-by: Greg Kurz
Message-Id:
<4d7275b2363f122438a443ce079cbb355285e9d6.1603285620.git.qemu_...@crudebyte.com>
Signed-off-by: Christian Schoenebeck
---
tests/qtest/virtio-9p-test.c | 27 +++
1 file changed, 19 insertions(+), 8 deletions(-)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 2ea555fa04..21807037df 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -583,6 +583,23 @@ static void do_version(QVirtio9P *v9p)
g_free(server_version);
}
+/* utility function: walk to requested dir and return fid for that dir */
+static uint32_t do_walk(QVirtio9P *v9p, const char *path)
+{
+char **wnames;
+P9Req *req;
+const uint32_t fid = genfid();
+
+int nwnames = split(path, "/", );
+
+req = v9fs_twalk(v9p, 0, fid, nwnames, wnames, 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_rwalk(req, NULL, NULL);
+
+split_free();
+return fid;
+}
+
static void fs_version(void *obj, void *data, QGuestAllocator *t_alloc)
{
alloc = t_alloc;
@@ -974,23 +991,17 @@ static void fs_flush_ignored(void *obj, void *data,
QGuestAllocator *t_alloc)
static void do_mkdir(QVirtio9P *v9p, const char *path, const char *cname)
{
-char **wnames;
char *const name = g_strdup(cname);
+uint32_t fid;
P9Req *req;
-const uint32_t fid = genfid();
-int nwnames = split(path, "/", );
-
-req = v9fs_twalk(v9p, 0, fid, nwnames, wnames, 0);
-v9fs_req_wait_for_reply(req, NULL);
-v9fs_rwalk(req, NULL, NULL);
+fid = do_walk(v9p, path);
req = v9fs_tmkdir(v9p, fid, name, 0750, 0, 0);
v9fs_req_wait_for_reply(req, NULL);
v9fs_rmkdir(req, NULL);
g_free(name);
-split_free();
}
static void fs_readdir_split_128(void *obj, void *data,
--
2.20.1
[PULL 10/13] tests/9pfs: add local Tsymlink test
This test case uses a Tsymlink 9p request to create a symbolic link using
the 9pfs 'local' fs driver.
Signed-off-by: Christian Schoenebeck
Reviewed-by: Greg Kurz
Message-Id:
<84ac76937855bf441242372cc3e62df42f0a3dc4.1603285620.git.qemu_...@crudebyte.com>
Signed-off-by: Christian Schoenebeck
---
tests/qtest/virtio-9p-test.c | 77
1 file changed, 77 insertions(+)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 6b74a1fd7e..0c11417236 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -259,6 +259,7 @@ static const char *rmessage_name(uint8_t id)
id == P9_RWRITE ? "RWRITE" :
id == P9_RMKDIR ? "RMKDIR" :
id == P9_RLCREATE ? "RLCREATE" :
+id == P9_RSYMLINK ? "RSYMLINK" :
id == P9_RUNLINKAT ? "RUNLINKAT" :
id == P9_RFLUSH ? "RFLUSH" :
id == P9_RREADDIR ? "READDIR" :
@@ -733,6 +734,39 @@ static void v9fs_rlcreate(P9Req *req, v9fs_qid *qid,
uint32_t *iounit)
v9fs_req_free(req);
}
+/* size[4] Tsymlink tag[2] fid[4] name[s] symtgt[s] gid[4] */
+static P9Req *v9fs_tsymlink(QVirtio9P *v9p, uint32_t fid, const char *name,
+const char *symtgt, uint32_t gid, uint16_t tag)
+{
+P9Req *req;
+
+uint32_t body_size = 4 + 4;
+uint16_t string_size = v9fs_string_size(name) + v9fs_string_size(symtgt);
+
+g_assert_cmpint(body_size, <=, UINT32_MAX - string_size);
+body_size += string_size;
+
+req = v9fs_req_init(v9p, body_size, P9_TSYMLINK, tag);
+v9fs_uint32_write(req, fid);
+v9fs_string_write(req, name);
+v9fs_string_write(req, symtgt);
+v9fs_uint32_write(req, gid);
+v9fs_req_send(req);
+return req;
+}
+
+/* size[4] Rsymlink tag[2] qid[13] */
+static void v9fs_rsymlink(P9Req *req, v9fs_qid *qid)
+{
+v9fs_req_recv(req, P9_RSYMLINK);
+if (qid) {
+v9fs_memread(req, qid, 13);
+} else {
+v9fs_memskip(req, 13);
+}
+v9fs_req_free(req);
+}
+
/* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */
static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char *name,
uint32_t flags, uint16_t tag)
@@ -1089,6 +1123,25 @@ static uint32_t do_lcreate(QVirtio9P *v9p, const char
*path,
return fid;
}
+/* create symlink named @a clink in directory @a path pointing to @a to */
+static void do_symlink(QVirtio9P *v9p, const char *path, const char *clink,
+ const char *to)
+{
+char *const name = g_strdup(clink);
+char *const dst = g_strdup(to);
+uint32_t fid;
+P9Req *req;
+
+fid = do_walk(v9p, path);
+
+req = v9fs_tsymlink(v9p, fid, name, dst, 0, 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_rsymlink(req, NULL);
+
+g_free(dst);
+g_free(name);
+}
+
static void do_unlinkat(QVirtio9P *v9p, const char *atpath, const char *rpath,
uint32_t flags)
{
@@ -1219,6 +1272,29 @@ static void fs_unlinkat_file(void *obj, void *data,
QGuestAllocator *t_alloc)
g_free(new_file);
}
+static void fs_symlink_file(void *obj, void *data, QGuestAllocator *t_alloc)
+{
+QVirtio9P *v9p = obj;
+alloc = t_alloc;
+struct stat st;
+char *real_file = virtio_9p_test_path("05/real_file");
+char *symlink_file = virtio_9p_test_path("05/symlink_file");
+
+do_attach(v9p);
+do_mkdir(v9p, "/", "05");
+do_lcreate(v9p, "05", "real_file");
+g_assert(stat(real_file, ) == 0);
+g_assert((st.st_mode & S_IFMT) == S_IFREG);
+
+do_symlink(v9p, "05", "symlink_file", "real_file");
+
+/* check if created link exists now */
+g_assert(stat(symlink_file, ) == 0);
+
+g_free(symlink_file);
+g_free(real_file);
+}
+
static void *assign_9p_local_driver(GString *cmd_line, void *arg)
{
virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1262,6 +1338,7 @@ static void register_virtio_9p_test(void)
qos_add_test("local/unlinkat_dir", "virtio-9p", fs_unlinkat_dir, );
qos_add_test("local/create_file", "virtio-9p", fs_create_file, );
qos_add_test("local/unlinkat_file", "virtio-9p", fs_unlinkat_file, );
+qos_add_test("local/symlink_file", "virtio-9p", fs_symlink_file, );
}
libqos_init(register_virtio_9p_test);
--
2.20.1
[PULL 08/13] tests/9pfs: add local Tlcreate test
This test case uses a Tlcreate 9p request to create a regular file inside
host's test directory.
Signed-off-by: Christian Schoenebeck
Reviewed-by: Greg Kurz
Message-Id:
<269cae0c00af941a3a4ae78f1e319f93462a7eb4.1603285620.git.qemu_...@crudebyte.com>
Signed-off-by: Christian Schoenebeck
---
tests/qtest/virtio-9p-test.c | 77
1 file changed, 77 insertions(+)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index abd7e44648..c030bc2a6c 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -258,6 +258,7 @@ static const char *rmessage_name(uint8_t id)
id == P9_RLOPEN ? "RLOPEN" :
id == P9_RWRITE ? "RWRITE" :
id == P9_RMKDIR ? "RMKDIR" :
+id == P9_RLCREATE ? "RLCREATE" :
id == P9_RUNLINKAT ? "RUNLINKAT" :
id == P9_RFLUSH ? "RFLUSH" :
id == P9_RREADDIR ? "READDIR" :
@@ -694,6 +695,44 @@ static void v9fs_rmkdir(P9Req *req, v9fs_qid *qid)
v9fs_req_free(req);
}
+/* size[4] Tlcreate tag[2] fid[4] name[s] flags[4] mode[4] gid[4] */
+static P9Req *v9fs_tlcreate(QVirtio9P *v9p, uint32_t fid, const char *name,
+uint32_t flags, uint32_t mode, uint32_t gid,
+uint16_t tag)
+{
+P9Req *req;
+
+uint32_t body_size = 4 + 4 + 4 + 4;
+uint16_t string_size = v9fs_string_size(name);
+
+g_assert_cmpint(body_size, <=, UINT32_MAX - string_size);
+body_size += string_size;
+
+req = v9fs_req_init(v9p, body_size, P9_TLCREATE, tag);
+v9fs_uint32_write(req, fid);
+v9fs_string_write(req, name);
+v9fs_uint32_write(req, flags);
+v9fs_uint32_write(req, mode);
+v9fs_uint32_write(req, gid);
+v9fs_req_send(req);
+return req;
+}
+
+/* size[4] Rlcreate tag[2] qid[13] iounit[4] */
+static void v9fs_rlcreate(P9Req *req, v9fs_qid *qid, uint32_t *iounit)
+{
+v9fs_req_recv(req, P9_RLCREATE);
+if (qid) {
+v9fs_memread(req, qid, 13);
+} else {
+v9fs_memskip(req, 13);
+}
+if (iounit) {
+v9fs_uint32_read(req, iounit);
+}
+v9fs_req_free(req);
+}
+
/* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */
static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char *name,
uint32_t flags, uint16_t tag)
@@ -1032,6 +1071,24 @@ static void do_mkdir(QVirtio9P *v9p, const char *path,
const char *cname)
g_free(name);
}
+/* create a regular file with Tlcreate and return file's fid */
+static uint32_t do_lcreate(QVirtio9P *v9p, const char *path,
+ const char *cname)
+{
+char *const name = g_strdup(cname);
+uint32_t fid;
+P9Req *req;
+
+fid = do_walk(v9p, path);
+
+req = v9fs_tlcreate(v9p, fid, name, 0, 0750, 0, 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_rlcreate(req, NULL, NULL);
+
+g_free(name);
+return fid;
+}
+
static void do_unlinkat(QVirtio9P *v9p, const char *atpath, const char *rpath,
uint32_t flags)
{
@@ -1120,6 +1177,25 @@ static void fs_unlinkat_dir(void *obj, void *data,
QGuestAllocator *t_alloc)
g_free(root_path);
}
+static void fs_create_file(void *obj, void *data, QGuestAllocator *t_alloc)
+{
+QVirtio9P *v9p = obj;
+alloc = t_alloc;
+struct stat st;
+char *new_file = virtio_9p_test_path("03/1st_file");
+
+do_attach(v9p);
+do_mkdir(v9p, "/", "03");
+do_lcreate(v9p, "03", "1st_file");
+
+/* check if created file exists now ... */
+g_assert(stat(new_file, ) == 0);
+/* ... and is a regular file */
+g_assert((st.st_mode & S_IFMT) == S_IFREG);
+
+g_free(new_file);
+}
+
static void *assign_9p_local_driver(GString *cmd_line, void *arg)
{
virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1161,6 +1237,7 @@ static void register_virtio_9p_test(void)
qos_add_test("local/config", "virtio-9p", pci_config, );
qos_add_test("local/create_dir", "virtio-9p", fs_create_dir, );
qos_add_test("local/unlinkat_dir", "virtio-9p", fs_unlinkat_dir, );
+qos_add_test("local/create_file", "virtio-9p", fs_create_file, );
}
libqos_init(register_virtio_9p_test);
--
2.20.1
[PULL 09/13] tests/9pfs: add local Tunlinkat file test
This test case uses a Tunlinkat request to remove a regular file using
the 9pfs 'local' fs driver.
Signed-off-by: Christian Schoenebeck
Reviewed-by: Greg Kurz
Message-Id:
<4eabeed7f662721dd5664cb77fe36ea0aa08b1ec.1603285620.git.qemu_...@crudebyte.com>
Signed-off-by: Christian Schoenebeck
---
tests/qtest/virtio-9p-test.c | 24
1 file changed, 24 insertions(+)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index c030bc2a6c..6b74a1fd7e 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -1196,6 +1196,29 @@ static void fs_create_file(void *obj, void *data,
QGuestAllocator *t_alloc)
g_free(new_file);
}
+static void fs_unlinkat_file(void *obj, void *data, QGuestAllocator *t_alloc)
+{
+QVirtio9P *v9p = obj;
+alloc = t_alloc;
+struct stat st;
+char *new_file = virtio_9p_test_path("04/doa_file");
+
+do_attach(v9p);
+do_mkdir(v9p, "/", "04");
+do_lcreate(v9p, "04", "doa_file");
+
+/* check if created file exists now ... */
+g_assert(stat(new_file, ) == 0);
+/* ... and is a regular file */
+g_assert((st.st_mode & S_IFMT) == S_IFREG);
+
+do_unlinkat(v9p, "04", "doa_file", 0);
+/* file should be gone now */
+g_assert(stat(new_file, ) != 0);
+
+g_free(new_file);
+}
+
static void *assign_9p_local_driver(GString *cmd_line, void *arg)
{
virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1238,6 +1261,7 @@ static void register_virtio_9p_test(void)
qos_add_test("local/create_dir", "virtio-9p", fs_create_dir, );
qos_add_test("local/unlinkat_dir", "virtio-9p", fs_unlinkat_dir, );
qos_add_test("local/create_file", "virtio-9p", fs_create_file, );
+qos_add_test("local/unlinkat_file", "virtio-9p", fs_unlinkat_file, );
}
libqos_init(register_virtio_9p_test);
--
2.20.1
[PULL 00/13] 9p queue 2020-10-23
The following changes since commit 4c5b97bfd0dd54dc27717ae8d1cd10e14eef1430: Merge remote-tracking branch 'remotes/kraxel/tags/modules-20201022-pull-request' into staging (2020-10-22 12:33:21 +0100) are available in the Git repository at: https://github.com/cschoenebeck/qemu.git tags/pull-9p-20201023 for you to fetch changes up to ee01926a11b1f9bffcd6cdec0961dd9d1882da71: tests/9pfs: add local Tunlinkat hard link test (2020-10-22 20:26:33 +0200) 9pfs: more tests using local fs driver Only 9pfs test case changes this time: * Refactor: Rename functions to make top-level test functions fs_*() easily distinguishable from utility test functions do_*(). * Refactor: Drop unnecessary function arguments in utility test functions. * More test cases using the 9pfs 'local' filesystem driver backend, namely for the following 9p requests: Tunlinkat, Tlcreate, Tsymlink and Tlink. Christian Schoenebeck (8): tests/9pfs: simplify do_mkdir() tests/9pfs: add local Tunlinkat directory test tests/9pfs: add local Tlcreate test tests/9pfs: add local Tunlinkat file test tests/9pfs: add local Tsymlink test tests/9pfs: add local Tunlinkat symlink test tests/9pfs: add local Tlink test tests/9pfs: add local Tunlinkat hard link test Greg Kurz (5): tests/9pfs: Factor out do_version() helper tests/9pfs: Set alloc in fs_create_dir() tests/9pfs: Factor out do_attach() helper tests/9pfs: Turn fs_readdir_split() into a helper tests/9pfs: Turn fs_mkdir() into a helper tests/qtest/virtio-9p-test.c | 467 +++ 1 file changed, 431 insertions(+), 36 deletions(-)
Re: [PATCH v2 7/8] tests/9pfs: add local Tlink test
On Donnerstag, 22. Oktober 2020 11:07:49 CEST Greg Kurz wrote: > > Ok, I found the problem on the mentioned box that failed to create hard > > links with 9p: it is libvirt auto generating AppArmor policy rules for 9p > > export pathes, which libvirt generates with "rw" AA (AppArmor) > > permissions. Once I manually adjusted the AA rule to "rwl", creating hard > > links worked again. > > > > I guess I'll send a patch for libvirt to change that. At least IMO > > creating > > hard links with 9pfs is a very basic operation and should be enabled for > > the respective 9p export path by default. > > > > Actually I think it should also include "k" which means "file locking", as > > file locking is also a fundamental operation with 9pfs, right? > > Well, I don't know exactly why libvirt is generating a strict AppArmor > policy but I'm not that surprised. If the user can _easily_ change the > policy to fit its needs, it's fine to have a "better safe than sorry" > default. Alone identifying the root cause of this is not that easy I would say. And if it takes me quite some time to find it, then I imagine that other people would struggle with this even more. A large portion of software, even scripts, rely on being able to create hard links and locking files. Right now they typically error out on guest with no helpful error message. So you start looking into the logs, don't find something obvious, then strace on guest side to find the most relevant failing syscall on guest side and see it was probably link(). Then you have to check several security layers: file permissions on guest, file permissions on host, effective UID of qemu process. You try creating hard links directly on host with that UID, works. Next you check is it qemu's seccomp? Is it host's SELinux? Is it AppArmor? Even for an experienced sysadmin I doubt it'll be a matter of minutes to resolve this issue. Now imagine a regular user who just wants to sandbox something on a workstation. Looking at libvirt git log, it seems this security policy exists more or less since day one (SHA-1 29ea8a9b64aac60251d283f74d57690e4edb5a6b, Mar 9 2014). And I don't see an explanation that would suggest a specific reason for exactly "rw". I think something has to be improved here, so I'll challenge by sending a simple libvirt patch, CCing involved authors, and seeing the feedback: diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 12429278fb..ce243e304b 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1142,7 +1142,7 @@ get_files(vahControl * ctl) /* We don't need to add deny rw rules for readonly mounts, * this can only lead to troubles when mounting / readonly. */ -if (vah_add_path(, fs->src->path, fs->readonly ? "R" : "rw", true) != 0) +if (vah_add_path(, fs->src->path, fs->readonly ? "R" : "rwlk", true) != 0) goto cleanup; } } Even after this change, this is not a global policy. Allowing hard links and file locks would only be lifted for the 9p export path. There would be other options as well of course: e.g. detecting on 9pfs side whether AppArmor and co are enabled, and log a warning to the user a syscall failed for that reason. But that would be much more complicated and I wonder whether it would be worth it. Best regards, Christian Schoenebeck
Re: [PATCH v2 3/8] tests/9pfs: add local Tlcreate test
On Donnerstag, 22. Oktober 2020 10:51:46 CEST Greg Kurz wrote:
> On Wed, 21 Oct 2020 14:25:33 +0200
>
> Christian Schoenebeck wrote:
> > This test case uses a Tlcreate 9p request to create a regular file inside
> > host's test directory.
> >
> > Signed-off-by: Christian Schoenebeck
> > ---
>
> Just one remark, see below.
>
> Reviewed-by: Greg Kurz
>
> > tests/qtest/virtio-9p-test.c | 77
> > 1 file changed, 77 insertions(+)
> >
> > diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> > index abd7e44648..c030bc2a6c 100644
> > --- a/tests/qtest/virtio-9p-test.c
> > +++ b/tests/qtest/virtio-9p-test.c
> > @@ -258,6 +258,7 @@ static const char *rmessage_name(uint8_t id)
> >
> > id == P9_RLOPEN ? "RLOPEN" :
> > id == P9_RWRITE ? "RWRITE" :
> >
> > id == P9_RMKDIR ? "RMKDIR" :
> > +id == P9_RLCREATE ? "RLCREATE" :
> > id == P9_RUNLINKAT ? "RUNLINKAT" :
> > id == P9_RFLUSH ? "RFLUSH" :
> >
> > id == P9_RREADDIR ? "READDIR" :
> > @@ -694,6 +695,44 @@ static void v9fs_rmkdir(P9Req *req, v9fs_qid *qid)
> >
> > v9fs_req_free(req);
> >
> > }
> >
> > +/* size[4] Tlcreate tag[2] fid[4] name[s] flags[4] mode[4] gid[4] */
> > +static P9Req *v9fs_tlcreate(QVirtio9P *v9p, uint32_t fid, const char
> > *name, +uint32_t flags, uint32_t mode,
> > uint32_t gid, +uint16_t tag)
> > +{
> > +P9Req *req;
> > +
> > +uint32_t body_size = 4 + 4 + 4 + 4;
> > +uint16_t string_size = v9fs_string_size(name);
> > +
> > +g_assert_cmpint(body_size, <=, UINT32_MAX - string_size);
> > +body_size += string_size;
> > +
> > +req = v9fs_req_init(v9p, body_size, P9_TLCREATE, tag);
> > +v9fs_uint32_write(req, fid);
> > +v9fs_string_write(req, name);
> > +v9fs_uint32_write(req, flags);
> > +v9fs_uint32_write(req, mode);
> > +v9fs_uint32_write(req, gid);
> > +v9fs_req_send(req);
> > +return req;
> > +}
> > +
> > +/* size[4] Rlcreate tag[2] qid[13] iounit[4] */
> > +static void v9fs_rlcreate(P9Req *req, v9fs_qid *qid, uint32_t *iounit)
> > +{
> > +v9fs_req_recv(req, P9_RLCREATE);
> > +if (qid) {
> > +v9fs_memread(req, qid, 13);
> > +} else {
> > +v9fs_memskip(req, 13);
> > +}
> > +if (iounit) {
> > +v9fs_uint32_read(req, iounit);
> > +}
> > +v9fs_req_free(req);
> > +}
> > +
> >
> > /* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */
> > static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char
> > *name,>
> > uint32_t flags, uint16_t tag)
> >
> > @@ -1032,6 +1071,24 @@ static void do_mkdir(QVirtio9P *v9p, const char
> > *path, const char *cname)>
> > g_free(name);
> >
> > }
> >
> > +/* create a regular file with Tlcreate and return file's fid */
> > +static uint32_t do_lcreate(QVirtio9P *v9p, const char *path,
> > + const char *cname)
> > +{
> > +char *const name = g_strdup(cname);
> > +uint32_t fid;
> > +P9Req *req;
> > +
> > +fid = do_walk(v9p, path);
> > +
> > +req = v9fs_tlcreate(v9p, fid, name, 0, 0750, 0, 0);
>
> Maybe it could make sense to make the mode a parameter of
> do_lcreate() in case someone wants to write a test case
> around that ? Same remark applies to do_mkdir() actually.
>
> No need to change this now anyway.
Yeah, I thought about that for a moment, but decided if that's really needed
one day, it'll be a trivial change, especially under the assumption that there
will be probably not a lot of code using this function, even on the long-term.
Thanks for looking at these patches!
Best regards,
Christian Schoenebeck
Re: [PATCH v2 7/8] tests/9pfs: add local Tlink test
On Mittwoch, 21. Oktober 2020 14:51:09 CEST Christian Schoenebeck wrote:
> This test case uses a Tlink request to create a hard link to a regular
> file using the 9pfs 'local' fs driver.
>
> Signed-off-by: Christian Schoenebeck
> ---
> tests/qtest/virtio-9p-test.c | 71
> 1 file changed, 71 insertions(+)
>
> diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> index 33cba24b18..460fa49fe3 100644
> --- a/tests/qtest/virtio-9p-test.c
> +++ b/tests/qtest/virtio-9p-test.c
> @@ -260,6 +260,7 @@ static const char *rmessage_name(uint8_t id)
> id == P9_RMKDIR ? "RMKDIR" :
> id == P9_RLCREATE ? "RLCREATE" :
> id == P9_RSYMLINK ? "RSYMLINK" :
> +id == P9_RLINK ? "RLINK" :
> id == P9_RUNLINKAT ? "RUNLINKAT" :
> id == P9_RFLUSH ? "RFLUSH" :
> id == P9_RREADDIR ? "READDIR" :
> @@ -767,6 +768,33 @@ static void v9fs_rsymlink(P9Req *req, v9fs_qid *qid)
> v9fs_req_free(req);
> }
>
> +/* size[4] Tlink tag[2] dfid[4] fid[4] name[s] */
> +static P9Req *v9fs_tlink(QVirtio9P *v9p, uint32_t dfid, uint32_t fid,
> + const char *name, uint16_t tag)
> +{
> +P9Req *req;
> +
> +uint32_t body_size = 4 + 4;
> +uint16_t string_size = v9fs_string_size(name);
> +
> +g_assert_cmpint(body_size, <=, UINT32_MAX - string_size);
> +body_size += string_size;
> +
> +req = v9fs_req_init(v9p, body_size, P9_TLINK, tag);
> +v9fs_uint32_write(req, dfid);
> +v9fs_uint32_write(req, fid);
> +v9fs_string_write(req, name);
> +v9fs_req_send(req);
> +return req;
> +}
> +
> +/* size[4] Rlink tag[2] */
> +static void v9fs_rlink(P9Req *req)
> +{
> +v9fs_req_recv(req, P9_RLINK);
> +v9fs_req_free(req);
> +}
> +
> /* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */
> static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char
> *name, uint32_t flags, uint16_t tag)
> @@ -1142,6 +1170,21 @@ static void do_symlink(QVirtio9P *v9p, const char
> *path, const char *clink, g_free(name);
> }
>
> +/* create a hard link named @a clink in directory @a path pointing to @a to
> */ +static void do_hardlink(QVirtio9P *v9p, const char *path, const char
> *clink, +const char *to)
> +{
> +uint32_t dfid, fid;
> +P9Req *req;
> +
> +dfid = do_walk(v9p, path);
> +fid = do_walk(v9p, to);
> +
> +req = v9fs_tlink(v9p, dfid, fid, clink, 0);
> +v9fs_req_wait_for_reply(req, NULL);
> +v9fs_rlink(req);
> +}
> +
> static void do_unlinkat(QVirtio9P *v9p, const char *atpath, const char
> *rpath, uint32_t flags)
> {
> @@ -1321,6 +1364,33 @@ static void fs_unlinkat_symlink(void *obj, void
> *data, g_free(real_file);
> }
>
> +static void fs_hardlink_file(void *obj, void *data, QGuestAllocator
> *t_alloc) +{
> +QVirtio9P *v9p = obj;
> +alloc = t_alloc;
> +struct stat st_real, st_link;
> +char *real_file = virtio_9p_test_path("07/real_file");
> +char *hardlink_file = virtio_9p_test_path("07/hardlink_file");
> +
> +do_attach(v9p);
> +do_mkdir(v9p, "/", "07");
> +do_lcreate(v9p, "07", "real_file");
> +g_assert(stat(real_file, _real) == 0);
> +g_assert((st_real.st_mode & S_IFMT) == S_IFREG);
> +
> +do_hardlink(v9p, "07", "hardlink_file", "07/real_file");
> +
> +/* check if link exists now ... */
> +g_assert(stat(hardlink_file, _link) == 0);
> +/* ... and it's a hard link, right? */
> +g_assert((st_link.st_mode & S_IFMT) == S_IFREG);
> +g_assert(st_link.st_dev == st_real.st_dev);
> +g_assert(st_link.st_ino == st_real.st_ino);
> +
> +g_free(hardlink_file);
> +g_free(real_file);
> +}
> +
> static void *assign_9p_local_driver(GString *cmd_line, void *arg)
> {
> virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
> @@ -1367,6 +1437,7 @@ static void register_virtio_9p_test(void)
> qos_add_test("local/symlink_file", "virtio-9p", fs_symlink_file,
> ); qos_add_test("local/unlinkat_symlink", "virtio-9p",
> fs_unlinkat_symlink, );
> +qos_add_test("local/hardlink_file", "virtio-9p", fs_hardlink_file,
> ); }
>
> libqos_init(register_virtio_9p_test);
Ok, I found the problem on the mentioned box that failed to create hard links
with 9p: it is libvirt auto generating AppArmor policy rules for 9p export
pathes, which libvirt generates with "rw" AA (AppArmor) permissions. Once I
manually adjusted the AA rule to "rwl", creating hard links worked again.
I guess I'll send a patch for libvirt to change that. At least IMO creating
hard links with 9pfs is a very basic operation and should be enabled for the
respective 9p export path by default.
Actually I think it should also include "k" which means "file locking", as
file locking is also a fundamental operation with 9pfs, right?
Best regards,
Christian Schoenebeck
[PATCH v2 6/8] tests/9pfs: add local Tunlinkat symlink test
This test case uses a Tunlinkat request to remove a symlink using
the 9pfs 'local' fs driver.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/virtio-9p-test.c | 28
1 file changed, 28 insertions(+)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 0c11417236..33cba24b18 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -1295,6 +1295,32 @@ static void fs_symlink_file(void *obj, void *data,
QGuestAllocator *t_alloc)
g_free(real_file);
}
+static void fs_unlinkat_symlink(void *obj, void *data,
+QGuestAllocator *t_alloc)
+{
+QVirtio9P *v9p = obj;
+alloc = t_alloc;
+struct stat st;
+char *real_file = virtio_9p_test_path("06/real_file");
+char *symlink_file = virtio_9p_test_path("06/symlink_file");
+
+do_attach(v9p);
+do_mkdir(v9p, "/", "06");
+do_lcreate(v9p, "06", "real_file");
+g_assert(stat(real_file, ) == 0);
+g_assert((st.st_mode & S_IFMT) == S_IFREG);
+
+do_symlink(v9p, "06", "symlink_file", "real_file");
+g_assert(stat(symlink_file, ) == 0);
+
+do_unlinkat(v9p, "06", "symlink_file", 0);
+/* symlink should be gone now */
+g_assert(stat(symlink_file, ) != 0);
+
+g_free(symlink_file);
+g_free(real_file);
+}
+
static void *assign_9p_local_driver(GString *cmd_line, void *arg)
{
virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1339,6 +1365,8 @@ static void register_virtio_9p_test(void)
qos_add_test("local/create_file", "virtio-9p", fs_create_file, );
qos_add_test("local/unlinkat_file", "virtio-9p", fs_unlinkat_file, );
qos_add_test("local/symlink_file", "virtio-9p", fs_symlink_file, );
+qos_add_test("local/unlinkat_symlink", "virtio-9p", fs_unlinkat_symlink,
+ );
}
libqos_init(register_virtio_9p_test);
--
2.20.1
[PATCH v2 8/8] tests/9pfs: add local Tunlinkat hard link test
This test case uses a Tunlinkat request to remove a previously hard
linked file by using the 9pfs 'local' fs driver.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/virtio-9p-test.c | 30 ++
1 file changed, 30 insertions(+)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 460fa49fe3..23433913bb 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -1391,6 +1391,34 @@ static void fs_hardlink_file(void *obj, void *data,
QGuestAllocator *t_alloc)
g_free(real_file);
}
+static void fs_unlinkat_hardlink(void *obj, void *data,
+ QGuestAllocator *t_alloc)
+{
+QVirtio9P *v9p = obj;
+alloc = t_alloc;
+struct stat st_real, st_link;
+char *real_file = virtio_9p_test_path("08/real_file");
+char *hardlink_file = virtio_9p_test_path("08/hardlink_file");
+
+do_attach(v9p);
+do_mkdir(v9p, "/", "08");
+do_lcreate(v9p, "08", "real_file");
+g_assert(stat(real_file, _real) == 0);
+g_assert((st_real.st_mode & S_IFMT) == S_IFREG);
+
+do_hardlink(v9p, "08", "hardlink_file", "08/real_file");
+g_assert(stat(hardlink_file, _link) == 0);
+
+do_unlinkat(v9p, "08", "hardlink_file", 0);
+/* symlink should be gone now */
+g_assert(stat(hardlink_file, _link) != 0);
+/* and old file should still exist */
+g_assert(stat(real_file, _real) == 0);
+
+g_free(hardlink_file);
+g_free(real_file);
+}
+
static void *assign_9p_local_driver(GString *cmd_line, void *arg)
{
virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1438,6 +1466,8 @@ static void register_virtio_9p_test(void)
qos_add_test("local/unlinkat_symlink", "virtio-9p", fs_unlinkat_symlink,
);
qos_add_test("local/hardlink_file", "virtio-9p", fs_hardlink_file, );
+qos_add_test("local/unlinkat_hardlink", "virtio-9p", fs_unlinkat_hardlink,
+ );
}
libqos_init(register_virtio_9p_test);
--
2.20.1
[PATCH v2 7/8] tests/9pfs: add local Tlink test
This test case uses a Tlink request to create a hard link to a regular
file using the 9pfs 'local' fs driver.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/virtio-9p-test.c | 71
1 file changed, 71 insertions(+)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 33cba24b18..460fa49fe3 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -260,6 +260,7 @@ static const char *rmessage_name(uint8_t id)
id == P9_RMKDIR ? "RMKDIR" :
id == P9_RLCREATE ? "RLCREATE" :
id == P9_RSYMLINK ? "RSYMLINK" :
+id == P9_RLINK ? "RLINK" :
id == P9_RUNLINKAT ? "RUNLINKAT" :
id == P9_RFLUSH ? "RFLUSH" :
id == P9_RREADDIR ? "READDIR" :
@@ -767,6 +768,33 @@ static void v9fs_rsymlink(P9Req *req, v9fs_qid *qid)
v9fs_req_free(req);
}
+/* size[4] Tlink tag[2] dfid[4] fid[4] name[s] */
+static P9Req *v9fs_tlink(QVirtio9P *v9p, uint32_t dfid, uint32_t fid,
+ const char *name, uint16_t tag)
+{
+P9Req *req;
+
+uint32_t body_size = 4 + 4;
+uint16_t string_size = v9fs_string_size(name);
+
+g_assert_cmpint(body_size, <=, UINT32_MAX - string_size);
+body_size += string_size;
+
+req = v9fs_req_init(v9p, body_size, P9_TLINK, tag);
+v9fs_uint32_write(req, dfid);
+v9fs_uint32_write(req, fid);
+v9fs_string_write(req, name);
+v9fs_req_send(req);
+return req;
+}
+
+/* size[4] Rlink tag[2] */
+static void v9fs_rlink(P9Req *req)
+{
+v9fs_req_recv(req, P9_RLINK);
+v9fs_req_free(req);
+}
+
/* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */
static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char *name,
uint32_t flags, uint16_t tag)
@@ -1142,6 +1170,21 @@ static void do_symlink(QVirtio9P *v9p, const char *path,
const char *clink,
g_free(name);
}
+/* create a hard link named @a clink in directory @a path pointing to @a to */
+static void do_hardlink(QVirtio9P *v9p, const char *path, const char *clink,
+const char *to)
+{
+uint32_t dfid, fid;
+P9Req *req;
+
+dfid = do_walk(v9p, path);
+fid = do_walk(v9p, to);
+
+req = v9fs_tlink(v9p, dfid, fid, clink, 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_rlink(req);
+}
+
static void do_unlinkat(QVirtio9P *v9p, const char *atpath, const char *rpath,
uint32_t flags)
{
@@ -1321,6 +1364,33 @@ static void fs_unlinkat_symlink(void *obj, void *data,
g_free(real_file);
}
+static void fs_hardlink_file(void *obj, void *data, QGuestAllocator *t_alloc)
+{
+QVirtio9P *v9p = obj;
+alloc = t_alloc;
+struct stat st_real, st_link;
+char *real_file = virtio_9p_test_path("07/real_file");
+char *hardlink_file = virtio_9p_test_path("07/hardlink_file");
+
+do_attach(v9p);
+do_mkdir(v9p, "/", "07");
+do_lcreate(v9p, "07", "real_file");
+g_assert(stat(real_file, _real) == 0);
+g_assert((st_real.st_mode & S_IFMT) == S_IFREG);
+
+do_hardlink(v9p, "07", "hardlink_file", "07/real_file");
+
+/* check if link exists now ... */
+g_assert(stat(hardlink_file, _link) == 0);
+/* ... and it's a hard link, right? */
+g_assert((st_link.st_mode & S_IFMT) == S_IFREG);
+g_assert(st_link.st_dev == st_real.st_dev);
+g_assert(st_link.st_ino == st_real.st_ino);
+
+g_free(hardlink_file);
+g_free(real_file);
+}
+
static void *assign_9p_local_driver(GString *cmd_line, void *arg)
{
virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1367,6 +1437,7 @@ static void register_virtio_9p_test(void)
qos_add_test("local/symlink_file", "virtio-9p", fs_symlink_file, );
qos_add_test("local/unlinkat_symlink", "virtio-9p", fs_unlinkat_symlink,
);
+qos_add_test("local/hardlink_file", "virtio-9p", fs_hardlink_file, );
}
libqos_init(register_virtio_9p_test);
--
2.20.1
[PATCH v2 3/8] tests/9pfs: add local Tlcreate test
This test case uses a Tlcreate 9p request to create a regular file inside
host's test directory.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/virtio-9p-test.c | 77
1 file changed, 77 insertions(+)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index abd7e44648..c030bc2a6c 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -258,6 +258,7 @@ static const char *rmessage_name(uint8_t id)
id == P9_RLOPEN ? "RLOPEN" :
id == P9_RWRITE ? "RWRITE" :
id == P9_RMKDIR ? "RMKDIR" :
+id == P9_RLCREATE ? "RLCREATE" :
id == P9_RUNLINKAT ? "RUNLINKAT" :
id == P9_RFLUSH ? "RFLUSH" :
id == P9_RREADDIR ? "READDIR" :
@@ -694,6 +695,44 @@ static void v9fs_rmkdir(P9Req *req, v9fs_qid *qid)
v9fs_req_free(req);
}
+/* size[4] Tlcreate tag[2] fid[4] name[s] flags[4] mode[4] gid[4] */
+static P9Req *v9fs_tlcreate(QVirtio9P *v9p, uint32_t fid, const char *name,
+uint32_t flags, uint32_t mode, uint32_t gid,
+uint16_t tag)
+{
+P9Req *req;
+
+uint32_t body_size = 4 + 4 + 4 + 4;
+uint16_t string_size = v9fs_string_size(name);
+
+g_assert_cmpint(body_size, <=, UINT32_MAX - string_size);
+body_size += string_size;
+
+req = v9fs_req_init(v9p, body_size, P9_TLCREATE, tag);
+v9fs_uint32_write(req, fid);
+v9fs_string_write(req, name);
+v9fs_uint32_write(req, flags);
+v9fs_uint32_write(req, mode);
+v9fs_uint32_write(req, gid);
+v9fs_req_send(req);
+return req;
+}
+
+/* size[4] Rlcreate tag[2] qid[13] iounit[4] */
+static void v9fs_rlcreate(P9Req *req, v9fs_qid *qid, uint32_t *iounit)
+{
+v9fs_req_recv(req, P9_RLCREATE);
+if (qid) {
+v9fs_memread(req, qid, 13);
+} else {
+v9fs_memskip(req, 13);
+}
+if (iounit) {
+v9fs_uint32_read(req, iounit);
+}
+v9fs_req_free(req);
+}
+
/* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */
static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char *name,
uint32_t flags, uint16_t tag)
@@ -1032,6 +1071,24 @@ static void do_mkdir(QVirtio9P *v9p, const char *path,
const char *cname)
g_free(name);
}
+/* create a regular file with Tlcreate and return file's fid */
+static uint32_t do_lcreate(QVirtio9P *v9p, const char *path,
+ const char *cname)
+{
+char *const name = g_strdup(cname);
+uint32_t fid;
+P9Req *req;
+
+fid = do_walk(v9p, path);
+
+req = v9fs_tlcreate(v9p, fid, name, 0, 0750, 0, 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_rlcreate(req, NULL, NULL);
+
+g_free(name);
+return fid;
+}
+
static void do_unlinkat(QVirtio9P *v9p, const char *atpath, const char *rpath,
uint32_t flags)
{
@@ -1120,6 +1177,25 @@ static void fs_unlinkat_dir(void *obj, void *data,
QGuestAllocator *t_alloc)
g_free(root_path);
}
+static void fs_create_file(void *obj, void *data, QGuestAllocator *t_alloc)
+{
+QVirtio9P *v9p = obj;
+alloc = t_alloc;
+struct stat st;
+char *new_file = virtio_9p_test_path("03/1st_file");
+
+do_attach(v9p);
+do_mkdir(v9p, "/", "03");
+do_lcreate(v9p, "03", "1st_file");
+
+/* check if created file exists now ... */
+g_assert(stat(new_file, ) == 0);
+/* ... and is a regular file */
+g_assert((st.st_mode & S_IFMT) == S_IFREG);
+
+g_free(new_file);
+}
+
static void *assign_9p_local_driver(GString *cmd_line, void *arg)
{
virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1161,6 +1237,7 @@ static void register_virtio_9p_test(void)
qos_add_test("local/config", "virtio-9p", pci_config, );
qos_add_test("local/create_dir", "virtio-9p", fs_create_dir, );
qos_add_test("local/unlinkat_dir", "virtio-9p", fs_unlinkat_dir, );
+qos_add_test("local/create_file", "virtio-9p", fs_create_file, );
}
libqos_init(register_virtio_9p_test);
--
2.20.1
[PATCH v2 1/8] tests/9pfs: simplify do_mkdir()
Split out walking a directory path to a separate new utility function
do_walk() and use that function in do_mkdir().
The code difference saved this way is not much, but we'll use that new
do_walk() function in the upcoming patches, so it will avoid quite
some code duplication after all.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/virtio-9p-test.c | 27 +++
1 file changed, 19 insertions(+), 8 deletions(-)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 2ea555fa04..21807037df 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -583,6 +583,23 @@ static void do_version(QVirtio9P *v9p)
g_free(server_version);
}
+/* utility function: walk to requested dir and return fid for that dir */
+static uint32_t do_walk(QVirtio9P *v9p, const char *path)
+{
+char **wnames;
+P9Req *req;
+const uint32_t fid = genfid();
+
+int nwnames = split(path, "/", );
+
+req = v9fs_twalk(v9p, 0, fid, nwnames, wnames, 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_rwalk(req, NULL, NULL);
+
+split_free();
+return fid;
+}
+
static void fs_version(void *obj, void *data, QGuestAllocator *t_alloc)
{
alloc = t_alloc;
@@ -974,23 +991,17 @@ static void fs_flush_ignored(void *obj, void *data,
QGuestAllocator *t_alloc)
static void do_mkdir(QVirtio9P *v9p, const char *path, const char *cname)
{
-char **wnames;
char *const name = g_strdup(cname);
+uint32_t fid;
P9Req *req;
-const uint32_t fid = genfid();
-int nwnames = split(path, "/", );
-
-req = v9fs_twalk(v9p, 0, fid, nwnames, wnames, 0);
-v9fs_req_wait_for_reply(req, NULL);
-v9fs_rwalk(req, NULL, NULL);
+fid = do_walk(v9p, path);
req = v9fs_tmkdir(v9p, fid, name, 0750, 0, 0);
v9fs_req_wait_for_reply(req, NULL);
v9fs_rmkdir(req, NULL);
g_free(name);
-split_free();
}
static void fs_readdir_split_128(void *obj, void *data,
--
2.20.1
[PATCH v2 5/8] tests/9pfs: add local Tsymlink test
This test case uses a Tsymlink 9p request to create a symbolic link using
the 9pfs 'local' fs driver.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/virtio-9p-test.c | 77
1 file changed, 77 insertions(+)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 6b74a1fd7e..0c11417236 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -259,6 +259,7 @@ static const char *rmessage_name(uint8_t id)
id == P9_RWRITE ? "RWRITE" :
id == P9_RMKDIR ? "RMKDIR" :
id == P9_RLCREATE ? "RLCREATE" :
+id == P9_RSYMLINK ? "RSYMLINK" :
id == P9_RUNLINKAT ? "RUNLINKAT" :
id == P9_RFLUSH ? "RFLUSH" :
id == P9_RREADDIR ? "READDIR" :
@@ -733,6 +734,39 @@ static void v9fs_rlcreate(P9Req *req, v9fs_qid *qid,
uint32_t *iounit)
v9fs_req_free(req);
}
+/* size[4] Tsymlink tag[2] fid[4] name[s] symtgt[s] gid[4] */
+static P9Req *v9fs_tsymlink(QVirtio9P *v9p, uint32_t fid, const char *name,
+const char *symtgt, uint32_t gid, uint16_t tag)
+{
+P9Req *req;
+
+uint32_t body_size = 4 + 4;
+uint16_t string_size = v9fs_string_size(name) + v9fs_string_size(symtgt);
+
+g_assert_cmpint(body_size, <=, UINT32_MAX - string_size);
+body_size += string_size;
+
+req = v9fs_req_init(v9p, body_size, P9_TSYMLINK, tag);
+v9fs_uint32_write(req, fid);
+v9fs_string_write(req, name);
+v9fs_string_write(req, symtgt);
+v9fs_uint32_write(req, gid);
+v9fs_req_send(req);
+return req;
+}
+
+/* size[4] Rsymlink tag[2] qid[13] */
+static void v9fs_rsymlink(P9Req *req, v9fs_qid *qid)
+{
+v9fs_req_recv(req, P9_RSYMLINK);
+if (qid) {
+v9fs_memread(req, qid, 13);
+} else {
+v9fs_memskip(req, 13);
+}
+v9fs_req_free(req);
+}
+
/* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */
static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char *name,
uint32_t flags, uint16_t tag)
@@ -1089,6 +1123,25 @@ static uint32_t do_lcreate(QVirtio9P *v9p, const char
*path,
return fid;
}
+/* create symlink named @a clink in directory @a path pointing to @a to */
+static void do_symlink(QVirtio9P *v9p, const char *path, const char *clink,
+ const char *to)
+{
+char *const name = g_strdup(clink);
+char *const dst = g_strdup(to);
+uint32_t fid;
+P9Req *req;
+
+fid = do_walk(v9p, path);
+
+req = v9fs_tsymlink(v9p, fid, name, dst, 0, 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_rsymlink(req, NULL);
+
+g_free(dst);
+g_free(name);
+}
+
static void do_unlinkat(QVirtio9P *v9p, const char *atpath, const char *rpath,
uint32_t flags)
{
@@ -1219,6 +1272,29 @@ static void fs_unlinkat_file(void *obj, void *data,
QGuestAllocator *t_alloc)
g_free(new_file);
}
+static void fs_symlink_file(void *obj, void *data, QGuestAllocator *t_alloc)
+{
+QVirtio9P *v9p = obj;
+alloc = t_alloc;
+struct stat st;
+char *real_file = virtio_9p_test_path("05/real_file");
+char *symlink_file = virtio_9p_test_path("05/symlink_file");
+
+do_attach(v9p);
+do_mkdir(v9p, "/", "05");
+do_lcreate(v9p, "05", "real_file");
+g_assert(stat(real_file, ) == 0);
+g_assert((st.st_mode & S_IFMT) == S_IFREG);
+
+do_symlink(v9p, "05", "symlink_file", "real_file");
+
+/* check if created link exists now */
+g_assert(stat(symlink_file, ) == 0);
+
+g_free(symlink_file);
+g_free(real_file);
+}
+
static void *assign_9p_local_driver(GString *cmd_line, void *arg)
{
virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1262,6 +1338,7 @@ static void register_virtio_9p_test(void)
qos_add_test("local/unlinkat_dir", "virtio-9p", fs_unlinkat_dir, );
qos_add_test("local/create_file", "virtio-9p", fs_create_file, );
qos_add_test("local/unlinkat_file", "virtio-9p", fs_unlinkat_file, );
+qos_add_test("local/symlink_file", "virtio-9p", fs_symlink_file, );
}
libqos_init(register_virtio_9p_test);
--
2.20.1
[PATCH v2 2/8] tests/9pfs: add local Tunlinkat directory test
This test case uses a Tunlinkat 9p request with flag AT_REMOVEDIR
(see 'man 2 unlink') to remove a directory from host's test directory.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/virtio-9p-test.c | 71
1 file changed, 71 insertions(+)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 21807037df..abd7e44648 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -258,6 +258,7 @@ static const char *rmessage_name(uint8_t id)
id == P9_RLOPEN ? "RLOPEN" :
id == P9_RWRITE ? "RWRITE" :
id == P9_RMKDIR ? "RMKDIR" :
+id == P9_RUNLINKAT ? "RUNLINKAT" :
id == P9_RFLUSH ? "RFLUSH" :
id == P9_RREADDIR ? "READDIR" :
"";
@@ -693,6 +694,33 @@ static void v9fs_rmkdir(P9Req *req, v9fs_qid *qid)
v9fs_req_free(req);
}
+/* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */
+static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char *name,
+ uint32_t flags, uint16_t tag)
+{
+P9Req *req;
+
+uint32_t body_size = 4 + 4;
+uint16_t string_size = v9fs_string_size(name);
+
+g_assert_cmpint(body_size, <=, UINT32_MAX - string_size);
+body_size += string_size;
+
+req = v9fs_req_init(v9p, body_size, P9_TUNLINKAT, tag);
+v9fs_uint32_write(req, dirfd);
+v9fs_string_write(req, name);
+v9fs_uint32_write(req, flags);
+v9fs_req_send(req);
+return req;
+}
+
+/* size[4] Runlinkat tag[2] */
+static void v9fs_runlinkat(P9Req *req)
+{
+v9fs_req_recv(req, P9_RUNLINKAT);
+v9fs_req_free(req);
+}
+
/* basic readdir test where reply fits into a single response message */
static void fs_readdir(void *obj, void *data, QGuestAllocator *t_alloc)
{
@@ -1004,6 +1032,22 @@ static void do_mkdir(QVirtio9P *v9p, const char *path,
const char *cname)
g_free(name);
}
+static void do_unlinkat(QVirtio9P *v9p, const char *atpath, const char *rpath,
+uint32_t flags)
+{
+char *const name = g_strdup(rpath);
+uint32_t fid;
+P9Req *req;
+
+fid = do_walk(v9p, atpath);
+
+req = v9fs_tunlinkat(v9p, fid, name, flags, 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_runlinkat(req);
+
+g_free(name);
+}
+
static void fs_readdir_split_128(void *obj, void *data,
QGuestAllocator *t_alloc)
{
@@ -1050,6 +1094,32 @@ static void fs_create_dir(void *obj, void *data,
QGuestAllocator *t_alloc)
g_free(root_path);
}
+static void fs_unlinkat_dir(void *obj, void *data, QGuestAllocator *t_alloc)
+{
+QVirtio9P *v9p = obj;
+alloc = t_alloc;
+struct stat st;
+char *root_path = virtio_9p_test_path("");
+char *new_dir = virtio_9p_test_path("02");
+
+g_assert(root_path != NULL);
+
+do_attach(v9p);
+do_mkdir(v9p, "/", "02");
+
+/* check if created directory really exists now ... */
+g_assert(stat(new_dir, ) == 0);
+/* ... and is actually a directory */
+g_assert((st.st_mode & S_IFMT) == S_IFDIR);
+
+do_unlinkat(v9p, "/", "02", AT_REMOVEDIR);
+/* directory should be gone now */
+g_assert(stat(new_dir, ) != 0);
+
+g_free(new_dir);
+g_free(root_path);
+}
+
static void *assign_9p_local_driver(GString *cmd_line, void *arg)
{
virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1090,6 +1160,7 @@ static void register_virtio_9p_test(void)
opts.before = assign_9p_local_driver;
qos_add_test("local/config", "virtio-9p", pci_config, );
qos_add_test("local/create_dir", "virtio-9p", fs_create_dir, );
+qos_add_test("local/unlinkat_dir", "virtio-9p", fs_unlinkat_dir, );
}
libqos_init(register_virtio_9p_test);
--
2.20.1
[PATCH v2 4/8] tests/9pfs: add local Tunlinkat file test
This test case uses a Tunlinkat request to remove a regular file using
the 9pfs 'local' fs driver.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/virtio-9p-test.c | 24
1 file changed, 24 insertions(+)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index c030bc2a6c..6b74a1fd7e 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -1196,6 +1196,29 @@ static void fs_create_file(void *obj, void *data,
QGuestAllocator *t_alloc)
g_free(new_file);
}
+static void fs_unlinkat_file(void *obj, void *data, QGuestAllocator *t_alloc)
+{
+QVirtio9P *v9p = obj;
+alloc = t_alloc;
+struct stat st;
+char *new_file = virtio_9p_test_path("04/doa_file");
+
+do_attach(v9p);
+do_mkdir(v9p, "/", "04");
+do_lcreate(v9p, "04", "doa_file");
+
+/* check if created file exists now ... */
+g_assert(stat(new_file, ) == 0);
+/* ... and is a regular file */
+g_assert((st.st_mode & S_IFMT) == S_IFREG);
+
+do_unlinkat(v9p, "04", "doa_file", 0);
+/* file should be gone now */
+g_assert(stat(new_file, ) != 0);
+
+g_free(new_file);
+}
+
static void *assign_9p_local_driver(GString *cmd_line, void *arg)
{
virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1238,6 +1261,7 @@ static void register_virtio_9p_test(void)
qos_add_test("local/create_dir", "virtio-9p", fs_create_dir, );
qos_add_test("local/unlinkat_dir", "virtio-9p", fs_unlinkat_dir, );
qos_add_test("local/create_file", "virtio-9p", fs_create_file, );
+qos_add_test("local/unlinkat_file", "virtio-9p", fs_unlinkat_file, );
}
libqos_init(register_virtio_9p_test);
--
2.20.1
[PATCH v2 0/8] 9pfs: more local tests
Just a bunch of more test cases using the 9pfs 'local' fs driver backend, namely for these 9p requests: * Tunlinkat, Tlcreate, Tsymlink and Tlink. v1->v2: * Rebased on latest 9p.next queue HEAD. * Use do_*() as new naming convention for utility functions. * Drop unnecessary arguments of utility functions. * Always do 'alloc = t_alloc;' in toplevel test functions. * Split out do_hardlink() as utility function [patch 6]. Christian Schoenebeck (8): tests/9pfs: simplify do_mkdir() tests/9pfs: add local Tunlinkat directory test tests/9pfs: add local Tlcreate test tests/9pfs: add local Tunlinkat file test tests/9pfs: add local Tsymlink test tests/9pfs: add local Tunlinkat symlink test tests/9pfs: add local Tlink test tests/9pfs: add local Tunlinkat hard link test tests/qtest/virtio-9p-test.c | 405 ++- 1 file changed, 397 insertions(+), 8 deletions(-) -- 2.20.1
Re: [PULL v3 2/6] tests/9pfs: change qtest name prefix to synth
On Mittwoch, 21. Oktober 2020 08:15:55 CEST Philippe Mathieu-Daudé wrote: > Hi Cristian, > > On 10/20/20 1:54 PM, Christian Schoenebeck wrote: > > On Dienstag, 20. Oktober 2020 12:00:57 CEST Greg Kurz wrote: > >> On Tue, 20 Oct 2020 11:43:18 +0200 > >> > >> Christian Schoenebeck wrote: > >>> On Dienstag, 20. Oktober 2020 09:36:10 CEST Philippe Mathieu-Daudé wrote: > >>>> On 10/8/20 8:34 PM, Christian Schoenebeck wrote: > >>>>> All existing 9pfs test cases are using the 'synth' fs driver so far, > >>>>> which > >>>>> means they are not accessing real files, but a purely simulated (in > >>>>> RAM > >>>>> only) file system. > >>>>> > >>>>> Let's make this clear by changing the prefix of the individual qtest > >>>>> case > >>>>> names from 'fs/' to 'synth/'. That way they'll be easily > >>>>> distinguishable > >>>>> from upcoming new 9pfs test cases supposed to be using a different fs > >>>>> driver. > >>>>> > >>>>> Signed-off-by: Christian Schoenebeck > >>>>> Message-Id: > >>>>> >>>>> eby > >>>>> te.com> Signed-off-by: Christian Schoenebeck > >>>> > >>>> Harmless, but don't need to sign twice ;) > >>> > >>> Ah, I thought that's the common way, as Greg's PRs contained 2 SOBs as > >>> well, i.e. I thought this was intended to outline the patch author and > >>> submaintainer were the same person. > >>> > >>> BTW I actually did not explicitly add the 2nd SOB. It was rather added > >>> by > >>> the patchwork client automatically. So maybe it should be fixed in the > >>> client to detect an already existing SOB line? Or am missing something > >>> here? > >> > >> Yeah this is the reason why my sob appears twice on patches authored by > >> me, and since this is harmless I never really investigated how to fix > >> pwclient :) > > > > Well, I would usually offer my 'I can look at it' at this point, but I am > > reluctant this time as I assume it will end up as my recently suggested > > libqos patches where I did not get any response from the officially > > assigned maintainers; not even a simple 'nack'. > > I was just watching your contributions and suggested an improvement > because something in your new workflow seems mis-configured (other > maintainers don't have this problem). I didn't asked you to fix a > bug in a different tool. I apologize if I was unclear and you > understood it this way. You actually did not raise that expectation to me Philippe, so definitely no need to apologize. But I appreciate it! Correct me if I'm wrong, but AFAICS I'm actually not the only one being affected by this double-SOB issue. A short glimpse at the logs and I see for instance 3e7e134d827790c3714cae1d5b8aff8612000116 having it as well. So I guess everyone having the following two options enabled in pwclientrc: msgid=on signoff=on will have that issue. > Regarding your issue with a different series, I suppose you already > read: > https://wiki.qemu.org/Contribute/SubmitAPatch#If_your_patch_seems_to_have_be > en_ignored and > https://wiki.qemu.org/Contribute/SubmitAPatch#Return_the_favor > > You'll see that maintenance can be very time consuming, and we are > overcrowded from time to time when there is rush. I doubt maintainers > are ignoring your patches, as most of them try to do their best. > You might help them by reviewing patches for them, so they have time > to process your series. Yes, I am aware of these. And once I got used to a new code base tree I also look at other ones' patches there. I've recently been thinking whether it would be possible for QEMU submaintainers to make use of patchwork's status feature: https://www.mail-archive.com/qemu-devel@nongnu.org/msg737917.html Maybe that could help preventing patches of high traffic components ending up unseen. Best regards, Christian Schoenebeck
Re: [PATCH 1/8] tests/9pfs: simplify fs_mkdir()
On Dienstag, 20. Oktober 2020 20:03:09 CEST Greg Kurz wrote:
> On Tue, 20 Oct 2020 15:43:21 +0200
>
> Christian Schoenebeck wrote:
> > On Dienstag, 20. Oktober 2020 15:35:36 CEST Greg Kurz wrote:
> > > On Tue, 20 Oct 2020 01:13:23 +0200
> > >
> > > Christian Schoenebeck wrote:
> > > > Split out walking a directory path to a separate new utility function
> > > > fs_walk_fid() and use that function in fs_mkdir().
> > > >
> > > > The code difference saved this way is not much, but we'll use that new
> > > > fs_walk_fid() function in the upcoming patches, so it will avoid quite
> > > > some code duplication after all.
> > > >
> > > > Signed-off-by: Christian Schoenebeck
> > > > ---
> > > >
> > > > tests/qtest/virtio-9p-test.c | 23 ++-
> > > > 1 file changed, 18 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/tests/qtest/virtio-9p-test.c
> > > > b/tests/qtest/virtio-9p-test.c
> > > > index c15908f27b..dc724bbb1e 100644
> > > > --- a/tests/qtest/virtio-9p-test.c
> > > > +++ b/tests/qtest/virtio-9p-test.c
> > > > @@ -967,13 +967,12 @@ static void fs_flush_ignored(void *obj, void
> > > > *data,
> > > > QGuestAllocator *t_alloc)>
> > > >
> > > > g_free(wnames[0]);
> > > >
> > > > }
> > > >
> > > > -static void fs_mkdir(void *obj, void *data, QGuestAllocator *t_alloc,
> > > > - const char *path, const char *cname)
> > > > +/* utility function: walk to requested dir and return fid for that
> > > > dir */
> > > > +static uint32_t fs_walk_fid(void *obj, void *data, QGuestAllocator
> > > > *t_alloc, +const char *path)
> > > >
> > > > {
> > >
> > > Since fs_walk_fid() is a helper function, ie. not passed to
> > > qos_add_test(),
> > > any reason to keep the "void *obj, void *data, QGuestAllocator *t_alloc"
> > > based signature ? data and t_alloc aren't used at all and it seems that
> > > the
> > > function should rather take a QVirtio9P * directly instead of casting
> > > from
> > > a void *.
> > >
> > > Something like:
> > >
> > > static uint32_t fs_walk_fid(QVirtio9P *v9p, const char *path)
> > > {
> > > ...
> > > }
> > >
> > >
> > > Same remark applies to fs_mkdir() which isn't a top level test function
> > > either BTW (sorry for not having spotted this earlier).
> >
> > Good point. Typical case of being copy & waste induced. I'll change that.
>
> Since this also affects other patches in this series and this might
> have a substantial impact, I'll wait for v2 to review if you don't
> mind.
Sure!
There is probably no need to hurry anyway; since these are just test case
changes, I think it will also be fine if I send the PR during freeze.
You never had any issue with 9p hard links, right? I'm still investigating
this in parallel. I already can rule out QEMU's sandbox (seccomp) feature,
even after having disabled the latter that mentioned box is failing any
link()/linkat() calls. Maybe it's a SELinux issue ...
Best regards,
Christian Schoenebeck
Re: [PATCH v2 0/5] tests/9pfs: Code refactoring
On Dienstag, 20. Oktober 2020 18:09:07 CEST Greg Kurz wrote: > Some code refactoring to have a clear distinction between top level > test functions and helper functions. > > v2: - use "do_" prefix instead of "do_fs_" > > --- > > Greg Kurz (5): > tests/9pfs: Factor out do_version() helper > tests/9pfs: Turn fs_readdir_split() into a helper > tests/9pfs: Set alloc in fs_create_dir() > tests/9pfs: Factor out do_attach() helper > tests/9pfs: Turn fs_mkdir() into a helper > > > tests/qtest/virtio-9p-test.c | 62 > +++--- 1 file changed, 34 > insertions(+), 28 deletions(-) > > -- > Greg Queued on 9p.next: https://github.com/cschoenebeck/qemu/commits/9p.next I just tweaked the order of the patches to prevent intermediate compiler errors. Thanks! Best regards, Christian Schoenebeck
Re: [PATCH 1/5] tests/9pfs: Factor out do_fs_version() helper
On Dienstag, 20. Oktober 2020 17:41:56 CEST Greg Kurz wrote:
> On Tue, 20 Oct 2020 17:34:05 +0200
>
> Christian Schoenebeck wrote:
> > On Dienstag, 20. Oktober 2020 17:11:05 CEST Greg Kurz wrote:
> > > fs_version() is a top level test function. Factor out the sugar
> > > to a separate helper instead of hijacking it in other tests.
> > >
> > > Signed-off-by: Greg Kurz
> > > ---
> > >
> > > tests/qtest/virtio-9p-test.c | 14 +-
> > > 1 file changed, 9 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> > > index c15908f27b3d..63f91aaf77e6 100644
> > > --- a/tests/qtest/virtio-9p-test.c
> > > +++ b/tests/qtest/virtio-9p-test.c
> > > @@ -567,10 +567,8 @@ static void v9fs_rflush(P9Req *req)
> > >
> > > v9fs_req_free(req);
> > >
> > > }
> > >
> > > -static void fs_version(void *obj, void *data, QGuestAllocator *t_alloc)
> > > +static void do_fs_version(QVirtio9P *v9p)
> > >
> > > {
> > >
> > > -QVirtio9P *v9p = obj;
> > > -alloc = t_alloc;
> > >
> > > const char *version = "9P2000.L";
> > > uint16_t server_len;
> > > char *server_version;
> > >
> > > @@ -585,13 +583,19 @@ static void fs_version(void *obj, void *data,
> > > QGuestAllocator *t_alloc) g_free(server_version);
> > >
> > > }
> >
> > So the naming convention from now on shall be do_fs_*() for non-toplevel
> > functions there. Not that I care too much about the precise prefix, but
> > how
> > about just do_*() for them instead?
>
> I've prepended "do_" to the existing names by pure laziness but I'm
> fine with any prefix or naming convention actually.
>
> So just tell me what you prefer and I'll send a v2.
It's really just more pleasant for the eye to have the prefix a bit shorter.
So use do_*() or any other kind of xx_*() or xxx_*() prefix that comes to your
mind. It will be fine with me.
Thanks Greg!
Best regards,
Christian Schoenebeck
Re: [PATCH 1/5] tests/9pfs: Factor out do_fs_version() helper
On Dienstag, 20. Oktober 2020 17:11:05 CEST Greg Kurz wrote:
> fs_version() is a top level test function. Factor out the sugar
> to a separate helper instead of hijacking it in other tests.
>
> Signed-off-by: Greg Kurz
> ---
> tests/qtest/virtio-9p-test.c | 14 +-
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> index c15908f27b3d..63f91aaf77e6 100644
> --- a/tests/qtest/virtio-9p-test.c
> +++ b/tests/qtest/virtio-9p-test.c
> @@ -567,10 +567,8 @@ static void v9fs_rflush(P9Req *req)
> v9fs_req_free(req);
> }
>
> -static void fs_version(void *obj, void *data, QGuestAllocator *t_alloc)
> +static void do_fs_version(QVirtio9P *v9p)
> {
> -QVirtio9P *v9p = obj;
> -alloc = t_alloc;
> const char *version = "9P2000.L";
> uint16_t server_len;
> char *server_version;
> @@ -585,13 +583,19 @@ static void fs_version(void *obj, void *data,
> QGuestAllocator *t_alloc) g_free(server_version);
> }
So the naming convention from now on shall be do_fs_*() for non-toplevel
functions there. Not that I care too much about the precise prefix, but how
about just do_*() for them instead?
Except of that, your patches look fine to me.
Best regards,
Christian Schoenebeck
Re: [PATCH 1/8] tests/9pfs: simplify fs_mkdir()
On Dienstag, 20. Oktober 2020 15:35:36 CEST Greg Kurz wrote:
> On Tue, 20 Oct 2020 01:13:23 +0200
>
> Christian Schoenebeck wrote:
> > Split out walking a directory path to a separate new utility function
> > fs_walk_fid() and use that function in fs_mkdir().
> >
> > The code difference saved this way is not much, but we'll use that new
> > fs_walk_fid() function in the upcoming patches, so it will avoid quite
> > some code duplication after all.
> >
> > Signed-off-by: Christian Schoenebeck
> > ---
> >
> > tests/qtest/virtio-9p-test.c | 23 ++-
> > 1 file changed, 18 insertions(+), 5 deletions(-)
> >
> > diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> > index c15908f27b..dc724bbb1e 100644
> > --- a/tests/qtest/virtio-9p-test.c
> > +++ b/tests/qtest/virtio-9p-test.c
> > @@ -967,13 +967,12 @@ static void fs_flush_ignored(void *obj, void *data,
> > QGuestAllocator *t_alloc)>
> > g_free(wnames[0]);
> >
> > }
> >
> > -static void fs_mkdir(void *obj, void *data, QGuestAllocator *t_alloc,
> > - const char *path, const char *cname)
> > +/* utility function: walk to requested dir and return fid for that dir */
> > +static uint32_t fs_walk_fid(void *obj, void *data, QGuestAllocator
> > *t_alloc, +const char *path)
> >
> > {
>
> Since fs_walk_fid() is a helper function, ie. not passed to qos_add_test(),
> any reason to keep the "void *obj, void *data, QGuestAllocator *t_alloc"
> based signature ? data and t_alloc aren't used at all and it seems that the
> function should rather take a QVirtio9P * directly instead of casting from
> a void *.
>
> Something like:
>
> static uint32_t fs_walk_fid(QVirtio9P *v9p, const char *path)
> {
> ...
> }
>
>
> Same remark applies to fs_mkdir() which isn't a top level test function
> either BTW (sorry for not having spotted this earlier).
Good point. Typical case of being copy & waste induced. I'll change that.
Best regards,
Christian Schoenebeck
Re: [PULL v3 2/6] tests/9pfs: change qtest name prefix to synth
On Dienstag, 20. Oktober 2020 12:00:57 CEST Greg Kurz wrote: > On Tue, 20 Oct 2020 11:43:18 +0200 > > Christian Schoenebeck wrote: > > On Dienstag, 20. Oktober 2020 09:36:10 CEST Philippe Mathieu-Daudé wrote: > > > On 10/8/20 8:34 PM, Christian Schoenebeck wrote: > > > > All existing 9pfs test cases are using the 'synth' fs driver so far, > > > > which > > > > means they are not accessing real files, but a purely simulated (in > > > > RAM > > > > only) file system. > > > > > > > > Let's make this clear by changing the prefix of the individual qtest > > > > case > > > > names from 'fs/' to 'synth/'. That way they'll be easily > > > > distinguishable > > > > from upcoming new 9pfs test cases supposed to be using a different fs > > > > driver. > > > > > > > > Signed-off-by: Christian Schoenebeck > > > > Message-Id: > > > > > > > eby > > > > te.com> Signed-off-by: Christian Schoenebeck > > > > > > Harmless, but don't need to sign twice ;) > > > > Ah, I thought that's the common way, as Greg's PRs contained 2 SOBs as > > well, i.e. I thought this was intended to outline the patch author and > > submaintainer were the same person. > > > > BTW I actually did not explicitly add the 2nd SOB. It was rather added by > > the patchwork client automatically. So maybe it should be fixed in the > > client to detect an already existing SOB line? Or am missing something > > here? > Yeah this is the reason why my sob appears twice on patches authored by > me, and since this is harmless I never really investigated how to fix > pwclient :) Well, I would usually offer my 'I can look at it' at this point, but I am reluctant this time as I assume it will end up as my recently suggested libqos patches where I did not get any response from the officially assigned maintainers; not even a simple 'nack'. Best regards, Christian Schoenebeck
Re: [PULL v3 2/6] tests/9pfs: change qtest name prefix to synth
On Dienstag, 20. Oktober 2020 09:36:10 CEST Philippe Mathieu-Daudé wrote: > On 10/8/20 8:34 PM, Christian Schoenebeck wrote: > > All existing 9pfs test cases are using the 'synth' fs driver so far, which > > means they are not accessing real files, but a purely simulated (in RAM > > only) file system. > > > > Let's make this clear by changing the prefix of the individual qtest case > > names from 'fs/' to 'synth/'. That way they'll be easily distinguishable > > from upcoming new 9pfs test cases supposed to be using a different fs > > driver. > > > > Signed-off-by: Christian Schoenebeck > > Message-Id: > > > te.com> Signed-off-by: Christian Schoenebeck > > Harmless, but don't need to sign twice ;) Ah, I thought that's the common way, as Greg's PRs contained 2 SOBs as well, i.e. I thought this was intended to outline the patch author and submaintainer were the same person. BTW I actually did not explicitly add the 2nd SOB. It was rather added by the patchwork client automatically. So maybe it should be fixed in the client to detect an already existing SOB line? Or am missing something here? Best regards, Christian Schoenebeck
Re: [PATCH 7/8] tests/9pfs: add local Tlink test
On Dienstag, 20. Oktober 2020 01:13:24 CEST Christian Schoenebeck wrote:
> This test case uses a Tlink request to create a hard link to a regular
> file using the 9pfs 'local' fs driver.
>
> Signed-off-by: Christian Schoenebeck
> ---
> tests/qtest/virtio-9p-test.c | 61
> 1 file changed, 61 insertions(+)
>
> diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> index f7d18f6274..447d8e3344 100644
> --- a/tests/qtest/virtio-9p-test.c
> +++ b/tests/qtest/virtio-9p-test.c
> @@ -260,6 +260,7 @@ static const char *rmessage_name(uint8_t id)
> id == P9_RMKDIR ? "RMKDIR" :
> id == P9_RLCREATE ? "RLCREATE" :
> id == P9_RSYMLINK ? "RSYMLINK" :
> +id == P9_RLINK ? "RLINK" :
> id == P9_RUNLINKAT ? "RUNLINKAT" :
> id == P9_RFLUSH ? "RFLUSH" :
> id == P9_RREADDIR ? "READDIR" :
> @@ -742,6 +743,33 @@ static void v9fs_rsymlink(P9Req *req, v9fs_qid *qid)
> v9fs_req_free(req);
> }
>
> +/* size[4] Tlink tag[2] dfid[4] fid[4] name[s] */
> +static P9Req *v9fs_tlink(QVirtio9P *v9p, uint32_t dfid, uint32_t fid,
> + const char *name, uint16_t tag)
> +{
This hard-link test was actually motived by an issue that I recently
encountered on a machine: it fails to create any hard links with 9p. This
particular test case succeeds though.
I think the problem is that recent libvirt versions enable qemu's sandbox
feature by default which filters syscalls. Fact is, any linkat() call fails on
that machine with EACCES now. I couldn't reproduce it on my development
machine yet though. I guess it's a difference in white/black-list seccomp
config or something. Not sure yet if there is some change required on 9p side
or whether it's really just a seccomp config issue.
P.S. Noisy days from my side, but this is probably the last batch of patches
from my side in a while, unless I really need to fix something for that hard
link isssue. We'll see ...
Best regards,
Christian Schoenebeck
[PATCH 8/8] tests/9pfs: add local unlinkat hard link test
This test case uses a Tunlinkat request to remove a previously hard
linked file by using the 9pfs 'local' fs driver.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/virtio-9p-test.c | 34 ++
1 file changed, 34 insertions(+)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 447d8e3344..2e50445745 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -1378,6 +1378,39 @@ static void fs_hardlink_file(void *obj, void *data,
QGuestAllocator *t_alloc)
g_free(real_file);
}
+static void fs_unlinkat_hardlink(void *obj, void *data,
+ QGuestAllocator *t_alloc)
+{
+QVirtio9P *v9p = obj;
+P9Req *req;
+uint32_t dfid, fid;
+struct stat st_real, st_link;
+char *real_file = virtio_9p_test_path("08/real_file");
+char *hardlink_file = virtio_9p_test_path("08/hardlink_file");
+
+fs_attach(v9p, NULL, t_alloc);
+fs_mkdir(v9p, data, t_alloc, "/", "08");
+fid = fs_lcreate(v9p, data, t_alloc, "08", "real_file");
+g_assert(stat(real_file, _real) == 0);
+g_assert((st_real.st_mode & S_IFMT) == S_IFREG);
+
+dfid = fs_walk_fid(v9p, data, t_alloc, "08");
+
+req = v9fs_tlink(v9p, dfid, fid, "hardlink_file", 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_rlink(req);
+g_assert(stat(hardlink_file, _link) == 0);
+
+fs_unlinkat(v9p, data, t_alloc, "08", "hardlink_file", 0);
+/* symlink should be gone now */
+g_assert(stat(hardlink_file, _link) != 0);
+/* and old file should still exist */
+g_assert(stat(real_file, _real) == 0);
+
+g_free(hardlink_file);
+g_free(real_file);
+}
+
static void *assign_9p_local_driver(GString *cmd_line, void *arg)
{
virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1424,6 +1457,7 @@ static void register_virtio_9p_test(void)
qos_add_test("local/symlink_file", "virtio-9p", fs_symlink_file, );
qos_add_test("local/unlinkat_symlink", "virtio-9p", fs_unlinkat_symlink,
);
qos_add_test("local/hardlink_file", "virtio-9p", fs_hardlink_file, );
+qos_add_test("local/unlinkat_hardlink", "virtio-9p", fs_unlinkat_hardlink,
);
}
libqos_init(register_virtio_9p_test);
--
2.20.1
[PATCH 7/8] tests/9pfs: add local Tlink test
This test case uses a Tlink request to create a hard link to a regular
file using the 9pfs 'local' fs driver.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/virtio-9p-test.c | 61
1 file changed, 61 insertions(+)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index f7d18f6274..447d8e3344 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -260,6 +260,7 @@ static const char *rmessage_name(uint8_t id)
id == P9_RMKDIR ? "RMKDIR" :
id == P9_RLCREATE ? "RLCREATE" :
id == P9_RSYMLINK ? "RSYMLINK" :
+id == P9_RLINK ? "RLINK" :
id == P9_RUNLINKAT ? "RUNLINKAT" :
id == P9_RFLUSH ? "RFLUSH" :
id == P9_RREADDIR ? "READDIR" :
@@ -742,6 +743,33 @@ static void v9fs_rsymlink(P9Req *req, v9fs_qid *qid)
v9fs_req_free(req);
}
+/* size[4] Tlink tag[2] dfid[4] fid[4] name[s] */
+static P9Req *v9fs_tlink(QVirtio9P *v9p, uint32_t dfid, uint32_t fid,
+ const char *name, uint16_t tag)
+{
+P9Req *req;
+
+uint32_t body_size = 4 + 4;
+uint16_t string_size = v9fs_string_size(name);
+
+g_assert_cmpint(body_size, <=, UINT32_MAX - string_size);
+body_size += string_size;
+
+req = v9fs_req_init(v9p, body_size, P9_TLINK, tag);
+v9fs_uint32_write(req, dfid);
+v9fs_uint32_write(req, fid);
+v9fs_string_write(req, name);
+v9fs_req_send(req);
+return req;
+}
+
+/* size[4] Rlink tag[2] */
+static void v9fs_rlink(P9Req *req)
+{
+v9fs_req_recv(req, P9_RLINK);
+v9fs_req_free(req);
+}
+
/* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */
static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char *name,
uint32_t flags, uint16_t tag)
@@ -1318,6 +1346,38 @@ static void fs_unlinkat_symlink(void *obj, void *data,
g_free(real_file);
}
+static void fs_hardlink_file(void *obj, void *data, QGuestAllocator *t_alloc)
+{
+QVirtio9P *v9p = obj;
+P9Req *req;
+uint32_t dfid, fid;
+struct stat st_real, st_link;
+char *real_file = virtio_9p_test_path("07/real_file");
+char *hardlink_file = virtio_9p_test_path("07/hardlink_file");
+
+fs_attach(v9p, NULL, t_alloc);
+fs_mkdir(v9p, data, t_alloc, "/", "07");
+fid = fs_lcreate(v9p, data, t_alloc, "07", "real_file");
+g_assert(stat(real_file, _real) == 0);
+g_assert((st_real.st_mode & S_IFMT) == S_IFREG);
+
+dfid = fs_walk_fid(v9p, data, t_alloc, "07");
+
+req = v9fs_tlink(v9p, dfid, fid, "hardlink_file", 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_rlink(req);
+
+/* check if link exists now ... */
+g_assert(stat(hardlink_file, _link) == 0);
+/* ... and it's a hard link, right? */
+g_assert((st_link.st_mode & S_IFMT) == S_IFREG);
+g_assert(st_link.st_dev == st_real.st_dev);
+g_assert(st_link.st_ino == st_real.st_ino);
+
+g_free(hardlink_file);
+g_free(real_file);
+}
+
static void *assign_9p_local_driver(GString *cmd_line, void *arg)
{
virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1363,6 +1423,7 @@ static void register_virtio_9p_test(void)
qos_add_test("local/unlinkat_file", "virtio-9p", fs_unlinkat_file, );
qos_add_test("local/symlink_file", "virtio-9p", fs_symlink_file, );
qos_add_test("local/unlinkat_symlink", "virtio-9p", fs_unlinkat_symlink,
);
+qos_add_test("local/hardlink_file", "virtio-9p", fs_hardlink_file, );
}
libqos_init(register_virtio_9p_test);
--
2.20.1
[PATCH 4/8] tests/9pfs: add local unlinkat file test
This test case uses a Tunlinkat request to remove a regular file using
the 9pfs 'local' fs driver.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/virtio-9p-test.c | 23 +++
1 file changed, 23 insertions(+)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 1b133f52bd..06a9f10d34 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -1194,6 +1194,28 @@ static void fs_create_file(void *obj, void *data,
QGuestAllocator *t_alloc)
g_free(new_file);
}
+static void fs_unlinkat_file(void *obj, void *data, QGuestAllocator *t_alloc)
+{
+QVirtio9P *v9p = obj;
+struct stat st;
+char *new_file = virtio_9p_test_path("04/doa_file");
+
+fs_attach(v9p, NULL, t_alloc);
+fs_mkdir(v9p, data, t_alloc, "/", "04");
+fs_lcreate(v9p, data, t_alloc, "04", "doa_file");
+
+/* check if created file exists now ... */
+g_assert(stat(new_file, ) == 0);
+/* ... and is a regular file */
+g_assert((st.st_mode & S_IFMT) == S_IFREG);
+
+fs_unlinkat(v9p, data, t_alloc, "04", "doa_file", 0);
+/* file should be gone now */
+g_assert(stat(new_file, ) != 0);
+
+g_free(new_file);
+}
+
static void *assign_9p_local_driver(GString *cmd_line, void *arg)
{
virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1236,6 +1258,7 @@ static void register_virtio_9p_test(void)
qos_add_test("local/create_dir", "virtio-9p", fs_create_dir, );
qos_add_test("local/unlinkat_dir", "virtio-9p", fs_unlinkat_dir, );
qos_add_test("local/create_file", "virtio-9p", fs_create_file, );
+qos_add_test("local/unlinkat_file", "virtio-9p", fs_unlinkat_file, );
}
libqos_init(register_virtio_9p_test);
--
2.20.1
[PATCH 6/8] tests/9pfs: add local unlinkat symlink test
This test case uses a Tunlinkat request to remove a symlink using
the 9pfs 'local' fs driver.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/virtio-9p-test.c | 26 ++
1 file changed, 26 insertions(+)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 78f4ed7e5f..f7d18f6274 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -1293,6 +1293,31 @@ static void fs_symlink_file(void *obj, void *data,
QGuestAllocator *t_alloc)
g_free(real_file);
}
+static void fs_unlinkat_symlink(void *obj, void *data,
+QGuestAllocator *t_alloc)
+{
+QVirtio9P *v9p = obj;
+struct stat st;
+char *real_file = virtio_9p_test_path("06/real_file");
+char *symlink_file = virtio_9p_test_path("06/symlink_file");
+
+fs_attach(v9p, NULL, t_alloc);
+fs_mkdir(v9p, data, t_alloc, "/", "06");
+fs_lcreate(v9p, data, t_alloc, "06", "real_file");
+g_assert(stat(real_file, ) == 0);
+g_assert((st.st_mode & S_IFMT) == S_IFREG);
+
+fs_symlink(v9p, data, t_alloc, "06", "symlink_file", "real_file");
+g_assert(stat(symlink_file, ) == 0);
+
+fs_unlinkat(v9p, data, t_alloc, "06", "symlink_file", 0);
+/* symlink should be gone now */
+g_assert(stat(symlink_file, ) != 0);
+
+g_free(symlink_file);
+g_free(real_file);
+}
+
static void *assign_9p_local_driver(GString *cmd_line, void *arg)
{
virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1337,6 +1362,7 @@ static void register_virtio_9p_test(void)
qos_add_test("local/create_file", "virtio-9p", fs_create_file, );
qos_add_test("local/unlinkat_file", "virtio-9p", fs_unlinkat_file, );
qos_add_test("local/symlink_file", "virtio-9p", fs_symlink_file, );
+qos_add_test("local/unlinkat_symlink", "virtio-9p", fs_unlinkat_symlink,
);
}
libqos_init(register_virtio_9p_test);
--
2.20.1
[PATCH 3/8] tests/9pfs: add local Tlcreate test
This test case uses a Tlcreate 9p request to create a regular file inside
host's test directory.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/virtio-9p-test.c | 78
1 file changed, 78 insertions(+)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 990d074d33..1b133f52bd 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -258,6 +258,7 @@ static const char *rmessage_name(uint8_t id)
id == P9_RLOPEN ? "RLOPEN" :
id == P9_RWRITE ? "RWRITE" :
id == P9_RMKDIR ? "RMKDIR" :
+id == P9_RLCREATE ? "RLCREATE" :
id == P9_RUNLINKAT ? "RUNLINKAT" :
id == P9_RFLUSH ? "RFLUSH" :
id == P9_RREADDIR ? "READDIR" :
@@ -669,6 +670,44 @@ static void v9fs_rmkdir(P9Req *req, v9fs_qid *qid)
v9fs_req_free(req);
}
+/* size[4] Tlcreate tag[2] fid[4] name[s] flags[4] mode[4] gid[4] */
+static P9Req *v9fs_tlcreate(QVirtio9P *v9p, uint32_t fid, const char *name,
+uint32_t flags, uint32_t mode, uint32_t gid,
+uint16_t tag)
+{
+P9Req *req;
+
+uint32_t body_size = 4 + 4 + 4 + 4;
+uint16_t string_size = v9fs_string_size(name);
+
+g_assert_cmpint(body_size, <=, UINT32_MAX - string_size);
+body_size += string_size;
+
+req = v9fs_req_init(v9p, body_size, P9_TLCREATE, tag);
+v9fs_uint32_write(req, fid);
+v9fs_string_write(req, name);
+v9fs_uint32_write(req, flags);
+v9fs_uint32_write(req, mode);
+v9fs_uint32_write(req, gid);
+v9fs_req_send(req);
+return req;
+}
+
+/* size[4] Rlcreate tag[2] qid[13] iounit[4] */
+static void v9fs_rlcreate(P9Req *req, v9fs_qid *qid, uint32_t *iounit)
+{
+v9fs_req_recv(req, P9_RLCREATE);
+if (qid) {
+v9fs_memread(req, qid, 13);
+} else {
+v9fs_memskip(req, 13);
+}
+if (iounit) {
+v9fs_uint32_read(req, iounit);
+}
+v9fs_req_free(req);
+}
+
/* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */
static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char *name,
uint32_t flags, uint16_t tag)
@@ -1032,6 +1071,26 @@ static void fs_mkdir(void *obj, void *data,
QGuestAllocator *t_alloc,
g_free(name);
}
+/* create a regular file with Tlcreate and return file's fid */
+static uint32_t fs_lcreate(void *obj, void *data, QGuestAllocator *t_alloc,
+ const char *path, const char *cname)
+{
+QVirtio9P *v9p = obj;
+alloc = t_alloc;
+char *const name = g_strdup(cname);
+uint32_t fid;
+P9Req *req;
+
+fid = fs_walk_fid(v9p, data, t_alloc, path);
+
+req = v9fs_tlcreate(v9p, fid, name, 0, 0750, 0, 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_rlcreate(req, NULL, NULL);
+
+g_free(name);
+return fid;
+}
+
static void fs_unlinkat(void *obj, void *data, QGuestAllocator *t_alloc,
const char *atpath, const char *rpath, uint32_t flags)
{
@@ -1117,6 +1176,24 @@ static void fs_unlinkat_dir(void *obj, void *data,
QGuestAllocator *t_alloc)
g_free(root_path);
}
+static void fs_create_file(void *obj, void *data, QGuestAllocator *t_alloc)
+{
+QVirtio9P *v9p = obj;
+struct stat st;
+char *new_file = virtio_9p_test_path("03/1st_file");
+
+fs_attach(v9p, NULL, t_alloc);
+fs_mkdir(v9p, data, t_alloc, "/", "03");
+fs_lcreate(v9p, data, t_alloc, "03", "1st_file");
+
+/* check if created file exists now ... */
+g_assert(stat(new_file, ) == 0);
+/* ... and is a regular file */
+g_assert((st.st_mode & S_IFMT) == S_IFREG);
+
+g_free(new_file);
+}
+
static void *assign_9p_local_driver(GString *cmd_line, void *arg)
{
virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1158,6 +1235,7 @@ static void register_virtio_9p_test(void)
qos_add_test("local/config", "virtio-9p", pci_config, );
qos_add_test("local/create_dir", "virtio-9p", fs_create_dir, );
qos_add_test("local/unlinkat_dir", "virtio-9p", fs_unlinkat_dir, );
+qos_add_test("local/create_file", "virtio-9p", fs_create_file, );
}
libqos_init(register_virtio_9p_test);
--
2.20.1
[PATCH 0/8] 9pfs: more local tests
Just a bunch of more test case using the 9pfs 'local' fs driver backend, namely for these 9p requests: * Tunlinkat, Tlcreate, Tsymlink and Tlink. Christian Schoenebeck (8): tests/9pfs: simplify fs_mkdir() tests/9pfs: add local unlinkat directory test tests/9pfs: add local Tlcreate test tests/9pfs: add local unlinkat file test tests/9pfs: add local Tsymlink test tests/9pfs: add local unlinkat symlink test tests/9pfs: add local Tlink test tests/9pfs: add local unlinkat hard link test tests/qtest/virtio-9p-test.c | 395 ++- 1 file changed, 390 insertions(+), 5 deletions(-) -- 2.20.1
[PATCH 5/8] tests/9pfs: add local Tsymlink test
This test case uses a Tsymlink 9p request to create a symbolic link using
the 9pfs 'local' fs driver.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/virtio-9p-test.c | 78
1 file changed, 78 insertions(+)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 06a9f10d34..78f4ed7e5f 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -259,6 +259,7 @@ static const char *rmessage_name(uint8_t id)
id == P9_RWRITE ? "RWRITE" :
id == P9_RMKDIR ? "RMKDIR" :
id == P9_RLCREATE ? "RLCREATE" :
+id == P9_RSYMLINK ? "RSYMLINK" :
id == P9_RUNLINKAT ? "RUNLINKAT" :
id == P9_RFLUSH ? "RFLUSH" :
id == P9_RREADDIR ? "READDIR" :
@@ -708,6 +709,39 @@ static void v9fs_rlcreate(P9Req *req, v9fs_qid *qid,
uint32_t *iounit)
v9fs_req_free(req);
}
+/* size[4] Tsymlink tag[2] fid[4] name[s] symtgt[s] gid[4] */
+static P9Req *v9fs_tsymlink(QVirtio9P *v9p, uint32_t fid, const char *name,
+const char *symtgt, uint32_t gid, uint16_t tag)
+{
+P9Req *req;
+
+uint32_t body_size = 4 + 4;
+uint16_t string_size = v9fs_string_size(name) + v9fs_string_size(symtgt);
+
+g_assert_cmpint(body_size, <=, UINT32_MAX - string_size);
+body_size += string_size;
+
+req = v9fs_req_init(v9p, body_size, P9_TSYMLINK, tag);
+v9fs_uint32_write(req, fid);
+v9fs_string_write(req, name);
+v9fs_string_write(req, symtgt);
+v9fs_uint32_write(req, gid);
+v9fs_req_send(req);
+return req;
+}
+
+/* size[4] Rsymlink tag[2] qid[13] */
+static void v9fs_rsymlink(P9Req *req, v9fs_qid *qid)
+{
+v9fs_req_recv(req, P9_RSYMLINK);
+if (qid) {
+v9fs_memread(req, qid, 13);
+} else {
+v9fs_memskip(req, 13);
+}
+v9fs_req_free(req);
+}
+
/* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */
static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char *name,
uint32_t flags, uint16_t tag)
@@ -1091,6 +1125,27 @@ static uint32_t fs_lcreate(void *obj, void *data,
QGuestAllocator *t_alloc,
return fid;
}
+/* create symlink named @a clink in directory @a path pointing to @a to */
+static void fs_symlink(void *obj, void *data, QGuestAllocator *t_alloc,
+ const char *path, const char *clink, const char *to)
+{
+QVirtio9P *v9p = obj;
+alloc = t_alloc;
+char *const name = g_strdup(clink);
+char *const dst = g_strdup(to);
+uint32_t fid;
+P9Req *req;
+
+fid = fs_walk_fid(v9p, data, t_alloc, path);
+
+req = v9fs_tsymlink(v9p, fid, name, dst, 0, 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_rsymlink(req, NULL);
+
+g_free(dst);
+g_free(name);
+}
+
static void fs_unlinkat(void *obj, void *data, QGuestAllocator *t_alloc,
const char *atpath, const char *rpath, uint32_t flags)
{
@@ -1216,6 +1271,28 @@ static void fs_unlinkat_file(void *obj, void *data,
QGuestAllocator *t_alloc)
g_free(new_file);
}
+static void fs_symlink_file(void *obj, void *data, QGuestAllocator *t_alloc)
+{
+QVirtio9P *v9p = obj;
+struct stat st;
+char *real_file = virtio_9p_test_path("05/real_file");
+char *symlink_file = virtio_9p_test_path("05/symlink_file");
+
+fs_attach(v9p, NULL, t_alloc);
+fs_mkdir(v9p, data, t_alloc, "/", "05");
+fs_lcreate(v9p, data, t_alloc, "05", "real_file");
+g_assert(stat(real_file, ) == 0);
+g_assert((st.st_mode & S_IFMT) == S_IFREG);
+
+fs_symlink(v9p, data, t_alloc, "05", "symlink_file", "real_file");
+
+/* check if created link exists now */
+g_assert(stat(symlink_file, ) == 0);
+
+g_free(symlink_file);
+g_free(real_file);
+}
+
static void *assign_9p_local_driver(GString *cmd_line, void *arg)
{
virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1259,6 +1336,7 @@ static void register_virtio_9p_test(void)
qos_add_test("local/unlinkat_dir", "virtio-9p", fs_unlinkat_dir, );
qos_add_test("local/create_file", "virtio-9p", fs_create_file, );
qos_add_test("local/unlinkat_file", "virtio-9p", fs_unlinkat_file, );
+qos_add_test("local/symlink_file", "virtio-9p", fs_symlink_file, );
}
libqos_init(register_virtio_9p_test);
--
2.20.1
[PATCH 1/8] tests/9pfs: simplify fs_mkdir()
Split out walking a directory path to a separate new utility function
fs_walk_fid() and use that function in fs_mkdir().
The code difference saved this way is not much, but we'll use that new
fs_walk_fid() function in the upcoming patches, so it will avoid quite
some code duplication after all.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/virtio-9p-test.c | 23 ++-
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index c15908f27b..dc724bbb1e 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -967,13 +967,12 @@ static void fs_flush_ignored(void *obj, void *data,
QGuestAllocator *t_alloc)
g_free(wnames[0]);
}
-static void fs_mkdir(void *obj, void *data, QGuestAllocator *t_alloc,
- const char *path, const char *cname)
+/* utility function: walk to requested dir and return fid for that dir */
+static uint32_t fs_walk_fid(void *obj, void *data, QGuestAllocator *t_alloc,
+const char *path)
{
QVirtio9P *v9p = obj;
-alloc = t_alloc;
char **wnames;
-char *const name = g_strdup(cname);
P9Req *req;
const uint32_t fid = genfid();
@@ -983,12 +982,26 @@ static void fs_mkdir(void *obj, void *data,
QGuestAllocator *t_alloc,
v9fs_req_wait_for_reply(req, NULL);
v9fs_rwalk(req, NULL, NULL);
+split_free();
+return fid;
+}
+
+static void fs_mkdir(void *obj, void *data, QGuestAllocator *t_alloc,
+ const char *path, const char *cname)
+{
+QVirtio9P *v9p = obj;
+alloc = t_alloc;
+char *const name = g_strdup(cname);
+uint32_t fid;
+P9Req *req;
+
+fid = fs_walk_fid(v9p, data, t_alloc, path);
+
req = v9fs_tmkdir(v9p, fid, name, 0750, 0, 0);
v9fs_req_wait_for_reply(req, NULL);
v9fs_rmkdir(req, NULL);
g_free(name);
-split_free();
}
static void fs_readdir_split_128(void *obj, void *data,
--
2.20.1
[PATCH 2/8] tests/9pfs: add local unlinkat directory test
This test case uses a Tunlinkat 9p request with flag AT_REMOVEDIR
(see 'man 2 unlink') to remove a directory from host's test directory.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/virtio-9p-test.c | 72
1 file changed, 72 insertions(+)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index dc724bbb1e..990d074d33 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -258,6 +258,7 @@ static const char *rmessage_name(uint8_t id)
id == P9_RLOPEN ? "RLOPEN" :
id == P9_RWRITE ? "RWRITE" :
id == P9_RMKDIR ? "RMKDIR" :
+id == P9_RUNLINKAT ? "RUNLINKAT" :
id == P9_RFLUSH ? "RFLUSH" :
id == P9_RREADDIR ? "READDIR" :
"";
@@ -668,6 +669,33 @@ static void v9fs_rmkdir(P9Req *req, v9fs_qid *qid)
v9fs_req_free(req);
}
+/* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */
+static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char *name,
+ uint32_t flags, uint16_t tag)
+{
+P9Req *req;
+
+uint32_t body_size = 4 + 4;
+uint16_t string_size = v9fs_string_size(name);
+
+g_assert_cmpint(body_size, <=, UINT32_MAX - string_size);
+body_size += string_size;
+
+req = v9fs_req_init(v9p, body_size, P9_TUNLINKAT, tag);
+v9fs_uint32_write(req, dirfd);
+v9fs_string_write(req, name);
+v9fs_uint32_write(req, flags);
+v9fs_req_send(req);
+return req;
+}
+
+/* size[4] Runlinkat tag[2] */
+static void v9fs_runlinkat(P9Req *req)
+{
+v9fs_req_recv(req, P9_RUNLINKAT);
+v9fs_req_free(req);
+}
+
/* basic readdir test where reply fits into a single response message */
static void fs_readdir(void *obj, void *data, QGuestAllocator *t_alloc)
{
@@ -1004,6 +1032,24 @@ static void fs_mkdir(void *obj, void *data,
QGuestAllocator *t_alloc,
g_free(name);
}
+static void fs_unlinkat(void *obj, void *data, QGuestAllocator *t_alloc,
+const char *atpath, const char *rpath, uint32_t flags)
+{
+QVirtio9P *v9p = obj;
+alloc = t_alloc;
+char *const name = g_strdup(rpath);
+uint32_t fid;
+P9Req *req;
+
+fid = fs_walk_fid(v9p, data, t_alloc, atpath);
+
+req = v9fs_tunlinkat(v9p, fid, name, flags, 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_runlinkat(req);
+
+g_free(name);
+}
+
static void fs_readdir_split_128(void *obj, void *data,
QGuestAllocator *t_alloc)
{
@@ -1046,6 +1092,31 @@ static void fs_create_dir(void *obj, void *data,
QGuestAllocator *t_alloc)
g_free(root_path);
}
+static void fs_unlinkat_dir(void *obj, void *data, QGuestAllocator *t_alloc)
+{
+QVirtio9P *v9p = obj;
+struct stat st;
+char *root_path = virtio_9p_test_path("");
+char *new_dir = virtio_9p_test_path("02");
+
+g_assert(root_path != NULL);
+
+fs_attach(v9p, NULL, t_alloc);
+fs_mkdir(v9p, data, t_alloc, "/", "02");
+
+/* check if created directory really exists now ... */
+g_assert(stat(new_dir, ) == 0);
+/* ... and is actually a directory */
+g_assert((st.st_mode & S_IFMT) == S_IFDIR);
+
+fs_unlinkat(v9p, data, t_alloc, "/", "02", AT_REMOVEDIR);
+/* directory should be gone now */
+g_assert(stat(new_dir, ) != 0);
+
+g_free(new_dir);
+g_free(root_path);
+}
+
static void *assign_9p_local_driver(GString *cmd_line, void *arg)
{
virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -1086,6 +1157,7 @@ static void register_virtio_9p_test(void)
opts.before = assign_9p_local_driver;
qos_add_test("local/config", "virtio-9p", pci_config, );
qos_add_test("local/create_dir", "virtio-9p", fs_create_dir, );
+qos_add_test("local/unlinkat_dir", "virtio-9p", fs_unlinkat_dir, );
}
libqos_init(register_virtio_9p_test);
--
2.20.1
[PULL v3 5/6] tests/9pfs: add virtio_9p_test_path()
This new public function virtio_9p_test_path() allows 9pfs
'local' tests to translate a path from guest scope to host
scope. For instance by passing an empty string it would
return the root path on host of the exported 9pfs tree.
Signed-off-by: Christian Schoenebeck
Message-Id:
Signed-off-by: Christian Schoenebeck
---
tests/qtest/libqos/virtio-9p.c | 6 ++
tests/qtest/libqos/virtio-9p.h | 5 +
2 files changed, 11 insertions(+)
diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index 8ee2a134bc..d43647b3b7 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -65,6 +65,12 @@ static void remove_local_test_dir(void)
g_free(cmd);
}
+char *virtio_9p_test_path(const char *path)
+{
+g_assert(local_test_path);
+return concat_path(local_test_path, path);
+}
+
static void virtio_9p_cleanup(QVirtio9P *interface)
{
qvirtqueue_cleanup(interface->vdev->bus, interface->vq, alloc);
diff --git a/tests/qtest/libqos/virtio-9p.h b/tests/qtest/libqos/virtio-9p.h
index 326a603f72..19a4d97454 100644
--- a/tests/qtest/libqos/virtio-9p.h
+++ b/tests/qtest/libqos/virtio-9p.h
@@ -49,4 +49,9 @@ struct QVirtio9PDevice {
*/
void virtio_9p_assign_local_driver(GString *cmd_line, const char *args);
+/**
+ * Returns path on host to the passed guest path. Result must be freed.
+ */
+char *virtio_9p_test_path(const char *path);
+
#endif
--
2.20.1
[PULL v3 4/6] tests/9pfs: wipe local 9pfs test directory
Before running the first 9pfs test case, make sure the test directory
for running the 9pfs 'local' tests on is entirely empty. For that
reason simply delete the test directory (if any) before (re)creating
it on test suite startup.
Note: The preferable precise behaviour would be the test directory
only being wiped once *before* a test suite run. Right now the test
directory is also wiped at the *end* of a test suite run because
libqos is calling the virtio_9p_register_nodes() callback for some
reason also when a test suite completed. This is suboptimal as
developers cannot immediately see what files and directories the
9pfs local tests created precisely after the test suite completed.
But fortunately the test directory is not wiped if some test failed.
So it is probably not worth it drilling another hole into libqos
for this issue.
Signed-off-by: Christian Schoenebeck
Message-Id:
Signed-off-by: Christian Schoenebeck
---
tests/qtest/libqos/virtio-9p.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index ee331166de..8ee2a134bc 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -53,6 +53,18 @@ static void create_local_test_dir(void)
g_assert((st.st_mode & S_IFMT) == S_IFDIR);
}
+/* Deletes directory previously created by create_local_test_dir(). */
+static void remove_local_test_dir(void)
+{
+g_assert(local_test_path != NULL);
+char *cmd = g_strdup_printf("rm -r '%s'\n", local_test_path);
+int res = system(cmd);
+if (res < 0) {
+/* ignore error, dummy check to prevent compiler error */
+}
+g_free(cmd);
+}
+
static void virtio_9p_cleanup(QVirtio9P *interface)
{
qvirtqueue_cleanup(interface->vdev->bus, interface->vq, alloc);
@@ -230,6 +242,7 @@ static void virtio_9p_register_nodes(void)
/* make sure test dir for the 'local' tests exists and is clean */
init_local_test_path();
+remove_local_test_dir();
create_local_test_dir();
QPCIAddress addr = {
--
2.20.1
[PULL v3 3/6] tests/9pfs: introduce local tests
This patch introduces 9pfs test cases using the 9pfs 'local'
filesystem driver which reads/writes/creates/deletes real files
and directories.
In this initial version, there is only one local test which actually
only checks if the 9pfs 'local' device was created successfully.
Before the 9pfs 'local' tests are run, a test directory 'qtest-9p-local'
is created (with world rwx permissions) under the current working
directory. At this point that test directory is not auto deleted yet.
Signed-off-by: Christian Schoenebeck
Message-Id:
<81fc4b3b6b6c9bf7999e79f5e7cbc364a5f09ddb.1602182956.git.qemu_...@crudebyte.com>
Signed-off-by: Christian Schoenebeck
---
tests/qtest/libqos/virtio-9p.c | 81 ++
tests/qtest/libqos/virtio-9p.h | 5 +++
tests/qtest/virtio-9p-test.c | 44 --
3 files changed, 116 insertions(+), 14 deletions(-)
diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index 2e300063e3..ee331166de 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -24,6 +24,34 @@
#include "qgraph.h"
static QGuestAllocator *alloc;
+static char *local_test_path;
+
+/* Concatenates the passed 2 pathes. Returned result must be freed. */
+static char *concat_path(const char* a, const char* b)
+{
+return g_build_filename(a, b, NULL);
+}
+
+static void init_local_test_path(void)
+{
+char *pwd = g_get_current_dir();
+local_test_path = concat_path(pwd, "qtest-9p-local");
+g_free(pwd);
+}
+
+/* Creates the directory for the 9pfs 'local' filesystem driver to access. */
+static void create_local_test_dir(void)
+{
+struct stat st;
+
+g_assert(local_test_path != NULL);
+mkdir(local_test_path, 0777);
+
+/* ensure test directory exists now ... */
+g_assert(stat(local_test_path, ) == 0);
+/* ... and is actually a directory */
+g_assert((st.st_mode & S_IFMT) == S_IFDIR);
+}
static void virtio_9p_cleanup(QVirtio9P *interface)
{
@@ -146,11 +174,64 @@ static void *virtio_9p_pci_create(void *pci_bus,
QGuestAllocator *t_alloc,
return obj;
}
+/**
+ * Performs regular expression based search and replace on @a haystack.
+ *
+ * @param haystack - input string to be parsed, result of replacement is
+ * stored back to @a haystack
+ * @param pattern - the regular expression pattern for scanning @a haystack
+ * @param replace_fmt - matches of supplied @a pattern are replaced by this,
+ * if necessary glib printf format can be used to add
+ * variable arguments of this function to this
+ * replacement string
+ */
+static void regex_replace(GString *haystack, const char *pattern,
+ const char *replace_fmt, ...)
+{
+GRegex *regex;
+char *replace, *s;
+va_list argp;
+
+va_start(argp, replace_fmt);
+replace = g_strdup_vprintf(replace_fmt, argp);
+va_end(argp);
+
+regex = g_regex_new(pattern, 0, 0, NULL);
+s = g_regex_replace(regex, haystack->str, -1, 0, replace, 0, NULL);
+g_string_assign(haystack, s);
+g_free(s);
+g_regex_unref(regex);
+g_free(replace);
+}
+
+void virtio_9p_assign_local_driver(GString *cmd_line, const char *args)
+{
+g_assert_nonnull(local_test_path);
+
+/* replace 'synth' driver by 'local' driver */
+regex_replace(cmd_line, "-fsdev synth,", "-fsdev local,");
+
+/* append 'path=...' to '-fsdev ...' group */
+regex_replace(cmd_line, "(-fsdev \\w[^ ]*)", "\\1,path='%s'",
+ local_test_path);
+
+if (!args) {
+return;
+}
+
+/* append passed args to '-fsdev ...' group */
+regex_replace(cmd_line, "(-fsdev \\w[^ ]*)", "\\1,%s", args);
+}
+
static void virtio_9p_register_nodes(void)
{
const char *str_simple = "fsdev=fsdev0,mount_tag=" MOUNT_TAG;
const char *str_addr = "fsdev=fsdev0,addr=04.0,mount_tag=" MOUNT_TAG;
+/* make sure test dir for the 'local' tests exists and is clean */
+init_local_test_path();
+create_local_test_dir();
+
QPCIAddress addr = {
.devfn = QPCI_DEVFN(4, 0),
};
diff --git a/tests/qtest/libqos/virtio-9p.h b/tests/qtest/libqos/virtio-9p.h
index b1e6badc4a..326a603f72 100644
--- a/tests/qtest/libqos/virtio-9p.h
+++ b/tests/qtest/libqos/virtio-9p.h
@@ -44,4 +44,9 @@ struct QVirtio9PDevice {
QVirtio9P v9p;
};
+/**
+ * Prepares QEMU command line for 9pfs tests using the 'local' fs driver.
+ */
+void virtio_9p_assign_local_driver(GString *cmd_line, const char *args);
+
#endif
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 3281153b9c..af7e169d3a 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -895,29 +895,45 @@ static void fs_readdir_split_512(void *obj, void *data,
fs_readdir_split(obj, data, t_alloc, 512);
}
[PULL v3 6/6] tests/9pfs: add local Tmkdir test
This test case uses the 9pfs 'local' driver to create a directory
and then checks if the expected directory was actually created
(as real directory) on host side.
This patch introduces a custom split() implementation, because
the test code requires non empty array elements as result. For
that reason g_strsplit() would not be a good alternative, as
it would require additional filter code for reshuffling the
array, and the resulting code would be even more complex than
this split() function.
Signed-off-by: Christian Schoenebeck
Message-Id:
Signed-off-by: Christian Schoenebeck
---
tests/qtest/virtio-9p-test.c | 139 +++
1 file changed, 139 insertions(+)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index af7e169d3a..c15908f27b 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -18,6 +18,62 @@
#define QVIRTIO_9P_TIMEOUT_US (10 * 1000 * 1000)
static QGuestAllocator *alloc;
+/*
+ * Used to auto generate new fids. Start with arbitrary high value to avoid
+ * collision with hard coded fids in basic test code.
+ */
+static uint32_t fid_generator = 1000;
+
+static uint32_t genfid(void)
+{
+return fid_generator++;
+}
+
+/**
+ * Splits the @a in string by @a delim into individual (non empty) strings
+ * and outputs them to @a out. The output array @a out is NULL terminated.
+ *
+ * Output array @a out must be freed by calling split_free().
+ *
+ * @returns number of individual elements in output array @a out (without the
+ * final NULL terminating element)
+ */
+static int split(const char *in, const char *delim, char ***out)
+{
+int n = 0, i = 0;
+char *tmp, *p;
+
+tmp = g_strdup(in);
+for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim)) {
+if (strlen(p) > 0) {
+++n;
+}
+}
+g_free(tmp);
+
+*out = g_new0(char *, n + 1); /* last element NULL delimiter */
+
+tmp = g_strdup(in);
+for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim)) {
+if (strlen(p) > 0) {
+(*out)[i++] = g_strdup(p);
+}
+}
+g_free(tmp);
+
+return n;
+}
+
+static void split_free(char ***out)
+{
+int i;
+for (i = 0; (*out)[i]; ++i) {
+g_free((*out)[i]);
+}
+g_free(*out);
+*out = NULL;
+}
+
static void pci_config(void *obj, void *data, QGuestAllocator *t_alloc)
{
QVirtio9P *v9p = obj;
@@ -201,6 +257,7 @@ static const char *rmessage_name(uint8_t id)
id == P9_RWALK ? "RWALK" :
id == P9_RLOPEN ? "RLOPEN" :
id == P9_RWRITE ? "RWRITE" :
+id == P9_RMKDIR ? "RMKDIR" :
id == P9_RFLUSH ? "RFLUSH" :
id == P9_RREADDIR ? "READDIR" :
"";
@@ -578,6 +635,39 @@ static bool fs_dirents_contain_name(struct V9fsDirent *e,
const char* name)
return false;
}
+/* size[4] Tmkdir tag[2] dfid[4] name[s] mode[4] gid[4] */
+static P9Req *v9fs_tmkdir(QVirtio9P *v9p, uint32_t dfid, const char *name,
+ uint32_t mode, uint32_t gid, uint16_t tag)
+{
+P9Req *req;
+
+uint32_t body_size = 4 + 4 + 4;
+uint16_t string_size = v9fs_string_size(name);
+
+g_assert_cmpint(body_size, <=, UINT32_MAX - string_size);
+body_size += string_size;
+
+req = v9fs_req_init(v9p, body_size, P9_TMKDIR, tag);
+v9fs_uint32_write(req, dfid);
+v9fs_string_write(req, name);
+v9fs_uint32_write(req, mode);
+v9fs_uint32_write(req, gid);
+v9fs_req_send(req);
+return req;
+}
+
+/* size[4] Rmkdir tag[2] qid[13] */
+static void v9fs_rmkdir(P9Req *req, v9fs_qid *qid)
+{
+v9fs_req_recv(req, P9_RMKDIR);
+if (qid) {
+v9fs_memread(req, qid, 13);
+} else {
+v9fs_memskip(req, 13);
+}
+v9fs_req_free(req);
+}
+
/* basic readdir test where reply fits into a single response message */
static void fs_readdir(void *obj, void *data, QGuestAllocator *t_alloc)
{
@@ -877,6 +967,30 @@ static void fs_flush_ignored(void *obj, void *data,
QGuestAllocator *t_alloc)
g_free(wnames[0]);
}
+static void fs_mkdir(void *obj, void *data, QGuestAllocator *t_alloc,
+ const char *path, const char *cname)
+{
+QVirtio9P *v9p = obj;
+alloc = t_alloc;
+char **wnames;
+char *const name = g_strdup(cname);
+P9Req *req;
+const uint32_t fid = genfid();
+
+int nwnames = split(path, "/", );
+
+req = v9fs_twalk(v9p, 0, fid, nwnames, wnames, 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_rwalk(req, NULL, NULL);
+
+req = v9fs_tmkdir(v9p, fid, name, 0750, 0, 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_rmkdir(req, NULL);
+
+g_free(name);
+split_free();
+}
+
static void fs_readdir_split_128(void *obj, void *data,
QGuestAllocator *t_alloc)
{
@@ -895,6 +1009,30 @@ static void fs_readdir_split_512(void *obj, vo
[PULL v3 1/6] 9pfs: suppress performance warnings on qtest runs
Don't trigger any performance warning if we're just running test cases,
because tests intentionally run for edge cases.
So far performance warnings were suppressed for the 'synth' fs driver
backend only. This patch suppresses them for all 9p fs driver backends.
Signed-off-by: Christian Schoenebeck
Reviewed-by: Greg Kurz
Message-Id:
Signed-off-by: Christian Schoenebeck
---
hw/9pfs/9p-synth.c | 2 --
hw/9pfs/virtio-9p-device.c | 6 ++
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
index cec8c0eefc..7eb210ffa8 100644
--- a/hw/9pfs/9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -541,8 +541,6 @@ static int synth_init(FsContext *ctx, Error **errp)
QLIST_INIT(_root.child);
qemu_mutex_init(_mutex);
-ctx->export_flags |= V9FS_NO_PERF_WARN;
-
/* Add "." and ".." entries for root */
v9fs_add_dir_node(_root, synth_root.attr->mode,
"..", synth_root.attr, synth_root.attr->inode);
diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index 36f3aa9352..14371a78ef 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -21,6 +21,7 @@
#include "hw/virtio/virtio-access.h"
#include "qemu/iov.h"
#include "qemu/module.h"
+#include "sysemu/qtest.h"
static void virtio_9p_push_and_notify(V9fsPDU *pdu)
{
@@ -199,6 +200,11 @@ static void virtio_9p_device_realize(DeviceState *dev,
Error **errp)
VirtIODevice *vdev = VIRTIO_DEVICE(dev);
V9fsVirtioState *v = VIRTIO_9P(dev);
V9fsState *s = >state;
+FsDriverEntry *fse = get_fsdev_fsentry(s->fsconf.fsdev_id);
+
+if (qtest_enabled() && fse) {
+fse->export_flags |= V9FS_NO_PERF_WARN;
+}
if (v9fs_device_realize_common(s, _9p_transport, errp)) {
return;
--
2.20.1
[PULL v3 2/6] tests/9pfs: change qtest name prefix to synth
All existing 9pfs test cases are using the 'synth' fs driver so far, which
means they are not accessing real files, but a purely simulated (in RAM
only) file system.
Let's make this clear by changing the prefix of the individual qtest case
names from 'fs/' to 'synth/'. That way they'll be easily distinguishable
from upcoming new 9pfs test cases supposed to be using a different fs
driver.
Signed-off-by: Christian Schoenebeck
Message-Id:
Signed-off-by: Christian Schoenebeck
---
tests/qtest/virtio-9p-test.c | 28 ++--
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index de30b717b6..3281153b9c 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -897,26 +897,26 @@ static void fs_readdir_split_512(void *obj, void *data,
static void register_virtio_9p_test(void)
{
-qos_add_test("config", "virtio-9p", pci_config, NULL);
-qos_add_test("fs/version/basic", "virtio-9p", fs_version, NULL);
-qos_add_test("fs/attach/basic", "virtio-9p", fs_attach, NULL);
-qos_add_test("fs/walk/basic", "virtio-9p", fs_walk, NULL);
-qos_add_test("fs/walk/no_slash", "virtio-9p", fs_walk_no_slash,
+qos_add_test("synth/config", "virtio-9p", pci_config, NULL);
+qos_add_test("synth/version/basic", "virtio-9p", fs_version, NULL);
+qos_add_test("synth/attach/basic", "virtio-9p", fs_attach, NULL);
+qos_add_test("synth/walk/basic", "virtio-9p", fs_walk, NULL);
+qos_add_test("synth/walk/no_slash", "virtio-9p", fs_walk_no_slash,
NULL);
-qos_add_test("fs/walk/dotdot_from_root", "virtio-9p",
+qos_add_test("synth/walk/dotdot_from_root", "virtio-9p",
fs_walk_dotdot, NULL);
-qos_add_test("fs/lopen/basic", "virtio-9p", fs_lopen, NULL);
-qos_add_test("fs/write/basic", "virtio-9p", fs_write, NULL);
-qos_add_test("fs/flush/success", "virtio-9p", fs_flush_success,
+qos_add_test("synth/lopen/basic", "virtio-9p", fs_lopen, NULL);
+qos_add_test("synth/write/basic", "virtio-9p", fs_write, NULL);
+qos_add_test("synth/flush/success", "virtio-9p", fs_flush_success,
NULL);
-qos_add_test("fs/flush/ignored", "virtio-9p", fs_flush_ignored,
+qos_add_test("synth/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",
+qos_add_test("synth/readdir/basic", "virtio-9p", fs_readdir, NULL);
+qos_add_test("synth/readdir/split_512", "virtio-9p",
fs_readdir_split_512, NULL);
-qos_add_test("fs/readdir/split_256", "virtio-9p",
+qos_add_test("synth/readdir/split_256", "virtio-9p",
fs_readdir_split_256, NULL);
-qos_add_test("fs/readdir/split_128", "virtio-9p",
+qos_add_test("synth/readdir/split_128", "virtio-9p",
fs_readdir_split_128, NULL);
}
--
2.20.1
[PULL v3 0/6] 9p queue (previous 2020-10-17)
The following changes since commit ba2a9a9e6318bfd93a2306dec40137e198205b86: Merge remote-tracking branch 'remotes/mcayland/tags/qemu-macppc-20201019' into staging (2020-10-19 11:46:03 +0100) are available in the Git repository at: https://github.com/cschoenebeck/qemu.git tags/pull-9p-20201019 for you to fetch changes up to 653daf38978d101d8810f96b9337ebc6b7b1423f: tests/9pfs: add local Tmkdir test (2020-10-19 14:25:40 +0200) 9pfs: add tests using local fs driver The currently existing 9pfs test cases are all solely using the 9pfs 'synth' fileystem driver, which is a very simple and purely simulated (in RAM only) filesystem. There are issues though where the 'synth' fs driver is not sufficient. For example the following two bugs need test cases running the 9pfs 'local' fs driver: https://bugs.launchpad.net/qemu/+bug/1336794 https://bugs.launchpad.net/qemu/+bug/1877384 This patch set for that reason introduces 9pfs test cases using the 9pfs 'local' filesystem driver along to the already existing tests on 'synth'. Christian Schoenebeck (6): 9pfs: suppress performance warnings on qtest runs tests/9pfs: change qtest name prefix to synth tests/9pfs: introduce local tests tests/9pfs: wipe local 9pfs test directory tests/9pfs: add virtio_9p_test_path() tests/9pfs: add local Tmkdir test hw/9pfs/9p-synth.c | 2 - hw/9pfs/virtio-9p-device.c | 6 ++ tests/qtest/libqos/virtio-9p.c | 100 + tests/qtest/libqos/virtio-9p.h | 10 +++ tests/qtest/virtio-9p-test.c | 197 - 5 files changed, 292 insertions(+), 23 deletions(-)
Re: [PATCH 1/1] 9pfs: suppress performance warnings on qtest runs
On Montag, 19. Oktober 2020 13:48:22 CEST Greg Kurz wrote: > On Mon, 19 Oct 2020 13:10:18 +0200 > > Christian Schoenebeck wrote: > > Don't trigger any performance warning if we're just running test cases, > > because tests intentionally run for edge cases. > > > > So far performance warnings were suppressed for the 'synth' fs driver > > backend only. This patch suppresses them for all 9p fs driver backends. > > > > Signed-off-by: Christian Schoenebeck > > --- > > LGTM > > Reviewed-by: Greg Kurz Thanks Greg! BTW, if you still have something to be merged, keep in mind freeze for 5.2 starts next monday. Best regards, Christian Schoenebeck
[PATCH 1/1] 9pfs: suppress performance warnings on qtest runs
Don't trigger any performance warning if we're just running test cases,
because tests intentionally run for edge cases.
So far performance warnings were suppressed for the 'synth' fs driver
backend only. This patch suppresses them for all 9p fs driver backends.
Signed-off-by: Christian Schoenebeck
---
hw/9pfs/9p-synth.c | 2 --
hw/9pfs/virtio-9p-device.c | 6 ++
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
index cec8c0eefc..7eb210ffa8 100644
--- a/hw/9pfs/9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -541,8 +541,6 @@ static int synth_init(FsContext *ctx, Error **errp)
QLIST_INIT(_root.child);
qemu_mutex_init(_mutex);
-ctx->export_flags |= V9FS_NO_PERF_WARN;
-
/* Add "." and ".." entries for root */
v9fs_add_dir_node(_root, synth_root.attr->mode,
"..", synth_root.attr, synth_root.attr->inode);
diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index 36f3aa9352..14371a78ef 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -21,6 +21,7 @@
#include "hw/virtio/virtio-access.h"
#include "qemu/iov.h"
#include "qemu/module.h"
+#include "sysemu/qtest.h"
static void virtio_9p_push_and_notify(V9fsPDU *pdu)
{
@@ -199,6 +200,11 @@ static void virtio_9p_device_realize(DeviceState *dev,
Error **errp)
VirtIODevice *vdev = VIRTIO_DEVICE(dev);
V9fsVirtioState *v = VIRTIO_9P(dev);
V9fsState *s = >state;
+FsDriverEntry *fse = get_fsdev_fsentry(s->fsconf.fsdev_id);
+
+if (qtest_enabled() && fse) {
+fse->export_flags |= V9FS_NO_PERF_WARN;
+}
if (v9fs_device_realize_common(s, _9p_transport, errp)) {
return;
--
2.20.1
Re: [PATCH v4 01/12] libqos/qgraph: add qemu_name to QOSGraphNode
On Donnerstag, 8. Oktober 2020 20:34:56 CEST Christian Schoenebeck wrote: > Add new member variable 'qemu_name' to struct QOSGraphNode. > > This new member may be optionally set in case a different > name for the node (which must always be a unique name) vs. > its actually associated QEMU (QMP) device name is required. > > Signed-off-by: Christian Schoenebeck > --- > tests/qtest/libqos/qgraph.c | 1 + > tests/qtest/libqos/qgraph_internal.h | 1 + > 2 files changed, 2 insertions(+) So what shall happen with these libqos patches 1..7? Is that a nack, or postpone for now? Best regards, Christian Schoenebeck
Re: [PULL v2 0/5] 9p queue (previous 2020-10-15)
On Montag, 19. Oktober 2020 12:22:47 CEST Peter Maydell wrote:
> On Mon, 19 Oct 2020 at 11:19, Christian Schoenebeck
>
> wrote:
> > On Montag, 19. Oktober 2020 11:52:38 CEST Peter Maydell wrote:
> > > This emits a lot of new warnings during 'make check':
> > >
> > > PASS 27 qtest-arm: qos-test
> > > /arm/virt/virtio-mmio/virtio-bus/virtio-9p-device/virtio-9p/virtio-9p-te
> > > sts/ local/config qemu-system-arm: warning: 9p: degraded performance: a
> > > reasonable high msize should be chosen on client/guest side (chosen
> > > msize is <= 8192). See
> > > https://wiki.qemu.org/Documentation/9psetup#msize for details. PASS 28
> > > qtest-arm: qos-test
> > > /arm/virt/virtio-mmio/virtio-bus/virtio-9p-device/virtio-9p/virtio-9p-te
> > > sts/ local/create_dir
> > >
> > > PASS 54 qtest-i386: qos-test
> > > /i386/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virti
> > > o-9p -tests/local/config qemu-system-i386: warning: 9p: degraded
> > > performance: a reasonable high msize should be chosen on client/guest
> > > side (chosen msize is <= 8192). See
> > > https://wiki.qemu.org/Documentation/9psetup#msize for details. PASS 55
> > > qtest-i386: qos-test
> > > /i386/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virti
> > > o-9p -tests/local/create_dir
> >
> > One warning per test suite run (i.e. per architecture due to
> > warn_report_once()), yes. That performance warning is meant for end user
> > installations to remind them setting some (reasonable high) value for 9p
> > client parameter 'msize' on guest OS side. The warning triggers here
> > because the 9p test cases intentionally run with a small 'msize' to guard
> > edge cases.
> >
> > Would it be Ok for you to merge it with this performance warning for now?
> > I
> > can take care of silencing it before 5.2 release. It probably requires to
> > introduce a new CL option to suppress performance warnings like these, or
> > by finding a way to detect that we're currently just running qtests.
>
> The usual approach is to suppress this kind of warning by guarding
> it with if (qtest_enabled()) { ... }.
Ah, didn't see that one. I do that and resend, thanks!
Best regards,
Christian Schoenebeck
Re: [PULL v2 0/5] 9p queue (previous 2020-10-15)
On Montag, 19. Oktober 2020 11:52:38 CEST Peter Maydell wrote: > On Sat, 17 Oct 2020 at 15:23, Christian Schoenebeck > > wrote: > > The following changes since commit e12ce85b2c79d83a340953291912875c30b3af06: > > Merge remote-tracking branch > > 'remotes/ehabkost/tags/x86-next-pull-request' into staging (2020-10-16 > > 22:46:28 +0100)> > > are available in the Git repository at: > > https://github.com/cschoenebeck/qemu.git tags/pull-9p-20201017 > > > > for you to fetch changes up to fa4551e3f4416cc8c62086ac430b1ceb4f03eb6b: > > tests/9pfs: add local Tmkdir test (2020-10-17 15:58:39 +0200) > > > > > > 9pfs: add tests using local fs driver > > > > The currently existing 9pfs test cases are all solely using the 9pfs > > 'synth' fileystem driver, which is a very simple and purely simulated (in > > RAM only) filesystem. There are issues though where the 'synth' fs driver > > is not sufficient. For example the following two bugs need test cases > > running the 9pfs 'local' fs driver: > > > > https://bugs.launchpad.net/qemu/+bug/1336794 > > https://bugs.launchpad.net/qemu/+bug/1877384 > > > > This patch set for that reason introduces 9pfs test cases using the 9pfs > > 'local' filesystem driver along to the already existing tests on 'synth'. > > This emits a lot of new warnings during 'make check': > > PASS 27 qtest-arm: qos-test > /arm/virt/virtio-mmio/virtio-bus/virtio-9p-device/virtio-9p/virtio-9p-tests/ > local/config qemu-system-arm: warning: 9p: degraded performance: a > reasonable high msize should be chosen on client/guest side (chosen msize > is <= 8192). See https://wiki.qemu.org/Documentation/9psetup#msize for > details. PASS 28 qtest-arm: qos-test > /arm/virt/virtio-mmio/virtio-bus/virtio-9p-device/virtio-9p/virtio-9p-tests/ > local/create_dir > > PASS 54 qtest-i386: qos-test > /i386/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p > -tests/local/config qemu-system-i386: warning: 9p: degraded performance: a > reasonable high msize should be chosen on client/guest side (chosen msize > is <= 8192). See https://wiki.qemu.org/Documentation/9psetup#msize for > details. PASS 55 qtest-i386: qos-test > /i386/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p > -tests/local/create_dir > > etc. > > thanks > -- PMM One warning per test suite run (i.e. per architecture due to warn_report_once()), yes. That performance warning is meant for end user installations to remind them setting some (reasonable high) value for 9p client parameter 'msize' on guest OS side. The warning triggers here because the 9p test cases intentionally run with a small 'msize' to guard edge cases. Would it be Ok for you to merge it with this performance warning for now? I can take care of silencing it before 5.2 release. It probably requires to introduce a new CL option to suppress performance warnings like these, or by finding a way to detect that we're currently just running qtests. Best regards, Christian Schoenebeck
[PULL v2 5/5] tests/9pfs: add local Tmkdir test
This test case uses the 9pfs 'local' driver to create a directory
and then checks if the expected directory was actually created
(as real directory) on host side.
This patch introduces a custom split() implementation, because
the test code requires non empty array elements as result. For
that reason g_strsplit() would not be a good alternative, as
it would require additional filter code for reshuffling the
array, and the resulting code would be even more complex than
this split() function.
Signed-off-by: Christian Schoenebeck
Message-Id:
Signed-off-by: Christian Schoenebeck
---
tests/qtest/virtio-9p-test.c | 139 +++
1 file changed, 139 insertions(+)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index af7e169d3a..c15908f27b 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -18,6 +18,62 @@
#define QVIRTIO_9P_TIMEOUT_US (10 * 1000 * 1000)
static QGuestAllocator *alloc;
+/*
+ * Used to auto generate new fids. Start with arbitrary high value to avoid
+ * collision with hard coded fids in basic test code.
+ */
+static uint32_t fid_generator = 1000;
+
+static uint32_t genfid(void)
+{
+return fid_generator++;
+}
+
+/**
+ * Splits the @a in string by @a delim into individual (non empty) strings
+ * and outputs them to @a out. The output array @a out is NULL terminated.
+ *
+ * Output array @a out must be freed by calling split_free().
+ *
+ * @returns number of individual elements in output array @a out (without the
+ * final NULL terminating element)
+ */
+static int split(const char *in, const char *delim, char ***out)
+{
+int n = 0, i = 0;
+char *tmp, *p;
+
+tmp = g_strdup(in);
+for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim)) {
+if (strlen(p) > 0) {
+++n;
+}
+}
+g_free(tmp);
+
+*out = g_new0(char *, n + 1); /* last element NULL delimiter */
+
+tmp = g_strdup(in);
+for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim)) {
+if (strlen(p) > 0) {
+(*out)[i++] = g_strdup(p);
+}
+}
+g_free(tmp);
+
+return n;
+}
+
+static void split_free(char ***out)
+{
+int i;
+for (i = 0; (*out)[i]; ++i) {
+g_free((*out)[i]);
+}
+g_free(*out);
+*out = NULL;
+}
+
static void pci_config(void *obj, void *data, QGuestAllocator *t_alloc)
{
QVirtio9P *v9p = obj;
@@ -201,6 +257,7 @@ static const char *rmessage_name(uint8_t id)
id == P9_RWALK ? "RWALK" :
id == P9_RLOPEN ? "RLOPEN" :
id == P9_RWRITE ? "RWRITE" :
+id == P9_RMKDIR ? "RMKDIR" :
id == P9_RFLUSH ? "RFLUSH" :
id == P9_RREADDIR ? "READDIR" :
"";
@@ -578,6 +635,39 @@ static bool fs_dirents_contain_name(struct V9fsDirent *e,
const char* name)
return false;
}
+/* size[4] Tmkdir tag[2] dfid[4] name[s] mode[4] gid[4] */
+static P9Req *v9fs_tmkdir(QVirtio9P *v9p, uint32_t dfid, const char *name,
+ uint32_t mode, uint32_t gid, uint16_t tag)
+{
+P9Req *req;
+
+uint32_t body_size = 4 + 4 + 4;
+uint16_t string_size = v9fs_string_size(name);
+
+g_assert_cmpint(body_size, <=, UINT32_MAX - string_size);
+body_size += string_size;
+
+req = v9fs_req_init(v9p, body_size, P9_TMKDIR, tag);
+v9fs_uint32_write(req, dfid);
+v9fs_string_write(req, name);
+v9fs_uint32_write(req, mode);
+v9fs_uint32_write(req, gid);
+v9fs_req_send(req);
+return req;
+}
+
+/* size[4] Rmkdir tag[2] qid[13] */
+static void v9fs_rmkdir(P9Req *req, v9fs_qid *qid)
+{
+v9fs_req_recv(req, P9_RMKDIR);
+if (qid) {
+v9fs_memread(req, qid, 13);
+} else {
+v9fs_memskip(req, 13);
+}
+v9fs_req_free(req);
+}
+
/* basic readdir test where reply fits into a single response message */
static void fs_readdir(void *obj, void *data, QGuestAllocator *t_alloc)
{
@@ -877,6 +967,30 @@ static void fs_flush_ignored(void *obj, void *data,
QGuestAllocator *t_alloc)
g_free(wnames[0]);
}
+static void fs_mkdir(void *obj, void *data, QGuestAllocator *t_alloc,
+ const char *path, const char *cname)
+{
+QVirtio9P *v9p = obj;
+alloc = t_alloc;
+char **wnames;
+char *const name = g_strdup(cname);
+P9Req *req;
+const uint32_t fid = genfid();
+
+int nwnames = split(path, "/", );
+
+req = v9fs_twalk(v9p, 0, fid, nwnames, wnames, 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_rwalk(req, NULL, NULL);
+
+req = v9fs_tmkdir(v9p, fid, name, 0750, 0, 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_rmkdir(req, NULL);
+
+g_free(name);
+split_free();
+}
+
static void fs_readdir_split_128(void *obj, void *data,
QGuestAllocator *t_alloc)
{
@@ -895,6 +1009,30 @@ static void fs_readdir_split_512(void *obj, vo
[PULL v2 0/5] 9p queue (previous 2020-10-15)
The following changes since commit e12ce85b2c79d83a340953291912875c30b3af06: Merge remote-tracking branch 'remotes/ehabkost/tags/x86-next-pull-request' into staging (2020-10-16 22:46:28 +0100) are available in the Git repository at: https://github.com/cschoenebeck/qemu.git tags/pull-9p-20201017 for you to fetch changes up to fa4551e3f4416cc8c62086ac430b1ceb4f03eb6b: tests/9pfs: add local Tmkdir test (2020-10-17 15:58:39 +0200) 9pfs: add tests using local fs driver The currently existing 9pfs test cases are all solely using the 9pfs 'synth' fileystem driver, which is a very simple and purely simulated (in RAM only) filesystem. There are issues though where the 'synth' fs driver is not sufficient. For example the following two bugs need test cases running the 9pfs 'local' fs driver: https://bugs.launchpad.net/qemu/+bug/1336794 https://bugs.launchpad.net/qemu/+bug/1877384 This patch set for that reason introduces 9pfs test cases using the 9pfs 'local' filesystem driver along to the already existing tests on 'synth'. Christian Schoenebeck (5): tests/9pfs: change qtest name prefix to synth tests/9pfs: introduce local tests tests/9pfs: wipe local 9pfs test directory tests/9pfs: add virtio_9p_test_path() tests/9pfs: add local Tmkdir test tests/qtest/libqos/virtio-9p.c | 100 + tests/qtest/libqos/virtio-9p.h | 10 +++ tests/qtest/virtio-9p-test.c | 197 - 3 files changed, 286 insertions(+), 21 deletions(-)
[PULL v2 4/5] tests/9pfs: add virtio_9p_test_path()
This new public function virtio_9p_test_path() allows 9pfs
'local' tests to translate a path from guest scope to host
scope. For instance by passing an empty string it would
return the root path on host of the exported 9pfs tree.
Signed-off-by: Christian Schoenebeck
Message-Id:
Signed-off-by: Christian Schoenebeck
---
tests/qtest/libqos/virtio-9p.c | 6 ++
tests/qtest/libqos/virtio-9p.h | 5 +
2 files changed, 11 insertions(+)
diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index 8ee2a134bc..d43647b3b7 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -65,6 +65,12 @@ static void remove_local_test_dir(void)
g_free(cmd);
}
+char *virtio_9p_test_path(const char *path)
+{
+g_assert(local_test_path);
+return concat_path(local_test_path, path);
+}
+
static void virtio_9p_cleanup(QVirtio9P *interface)
{
qvirtqueue_cleanup(interface->vdev->bus, interface->vq, alloc);
diff --git a/tests/qtest/libqos/virtio-9p.h b/tests/qtest/libqos/virtio-9p.h
index 326a603f72..19a4d97454 100644
--- a/tests/qtest/libqos/virtio-9p.h
+++ b/tests/qtest/libqos/virtio-9p.h
@@ -49,4 +49,9 @@ struct QVirtio9PDevice {
*/
void virtio_9p_assign_local_driver(GString *cmd_line, const char *args);
+/**
+ * Returns path on host to the passed guest path. Result must be freed.
+ */
+char *virtio_9p_test_path(const char *path);
+
#endif
--
2.20.1
[PULL v2 3/5] tests/9pfs: wipe local 9pfs test directory
Before running the first 9pfs test case, make sure the test directory
for running the 9pfs 'local' tests on is entirely empty. For that
reason simply delete the test directory (if any) before (re)creating
it on test suite startup.
Note: The preferable precise behaviour would be the test directory
only being wiped once *before* a test suite run. Right now the test
directory is also wiped at the *end* of a test suite run because
libqos is calling the virtio_9p_register_nodes() callback for some
reason also when a test suite completed. This is suboptimal as
developers cannot immediately see what files and directories the
9pfs local tests created precisely after the test suite completed.
But fortunately the test directory is not wiped if some test failed.
So it is probably not worth it drilling another hole into libqos
for this issue.
Signed-off-by: Christian Schoenebeck
Message-Id:
Signed-off-by: Christian Schoenebeck
---
tests/qtest/libqos/virtio-9p.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index ee331166de..8ee2a134bc 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -53,6 +53,18 @@ static void create_local_test_dir(void)
g_assert((st.st_mode & S_IFMT) == S_IFDIR);
}
+/* Deletes directory previously created by create_local_test_dir(). */
+static void remove_local_test_dir(void)
+{
+g_assert(local_test_path != NULL);
+char *cmd = g_strdup_printf("rm -r '%s'\n", local_test_path);
+int res = system(cmd);
+if (res < 0) {
+/* ignore error, dummy check to prevent compiler error */
+}
+g_free(cmd);
+}
+
static void virtio_9p_cleanup(QVirtio9P *interface)
{
qvirtqueue_cleanup(interface->vdev->bus, interface->vq, alloc);
@@ -230,6 +242,7 @@ static void virtio_9p_register_nodes(void)
/* make sure test dir for the 'local' tests exists and is clean */
init_local_test_path();
+remove_local_test_dir();
create_local_test_dir();
QPCIAddress addr = {
--
2.20.1
[PULL v2 2/5] tests/9pfs: introduce local tests
This patch introduces 9pfs test cases using the 9pfs 'local'
filesystem driver which reads/writes/creates/deletes real files
and directories.
In this initial version, there is only one local test which actually
only checks if the 9pfs 'local' device was created successfully.
Before the 9pfs 'local' tests are run, a test directory 'qtest-9p-local'
is created (with world rwx permissions) under the current working
directory. At this point that test directory is not auto deleted yet.
Signed-off-by: Christian Schoenebeck
Message-Id:
<81fc4b3b6b6c9bf7999e79f5e7cbc364a5f09ddb.1602182956.git.qemu_...@crudebyte.com>
Signed-off-by: Christian Schoenebeck
---
tests/qtest/libqos/virtio-9p.c | 81 ++
tests/qtest/libqos/virtio-9p.h | 5 +++
tests/qtest/virtio-9p-test.c | 44 --
3 files changed, 116 insertions(+), 14 deletions(-)
diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index 2e300063e3..ee331166de 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -24,6 +24,34 @@
#include "qgraph.h"
static QGuestAllocator *alloc;
+static char *local_test_path;
+
+/* Concatenates the passed 2 pathes. Returned result must be freed. */
+static char *concat_path(const char* a, const char* b)
+{
+return g_build_filename(a, b, NULL);
+}
+
+static void init_local_test_path(void)
+{
+char *pwd = g_get_current_dir();
+local_test_path = concat_path(pwd, "qtest-9p-local");
+g_free(pwd);
+}
+
+/* Creates the directory for the 9pfs 'local' filesystem driver to access. */
+static void create_local_test_dir(void)
+{
+struct stat st;
+
+g_assert(local_test_path != NULL);
+mkdir(local_test_path, 0777);
+
+/* ensure test directory exists now ... */
+g_assert(stat(local_test_path, ) == 0);
+/* ... and is actually a directory */
+g_assert((st.st_mode & S_IFMT) == S_IFDIR);
+}
static void virtio_9p_cleanup(QVirtio9P *interface)
{
@@ -146,11 +174,64 @@ static void *virtio_9p_pci_create(void *pci_bus,
QGuestAllocator *t_alloc,
return obj;
}
+/**
+ * Performs regular expression based search and replace on @a haystack.
+ *
+ * @param haystack - input string to be parsed, result of replacement is
+ * stored back to @a haystack
+ * @param pattern - the regular expression pattern for scanning @a haystack
+ * @param replace_fmt - matches of supplied @a pattern are replaced by this,
+ * if necessary glib printf format can be used to add
+ * variable arguments of this function to this
+ * replacement string
+ */
+static void regex_replace(GString *haystack, const char *pattern,
+ const char *replace_fmt, ...)
+{
+GRegex *regex;
+char *replace, *s;
+va_list argp;
+
+va_start(argp, replace_fmt);
+replace = g_strdup_vprintf(replace_fmt, argp);
+va_end(argp);
+
+regex = g_regex_new(pattern, 0, 0, NULL);
+s = g_regex_replace(regex, haystack->str, -1, 0, replace, 0, NULL);
+g_string_assign(haystack, s);
+g_free(s);
+g_regex_unref(regex);
+g_free(replace);
+}
+
+void virtio_9p_assign_local_driver(GString *cmd_line, const char *args)
+{
+g_assert_nonnull(local_test_path);
+
+/* replace 'synth' driver by 'local' driver */
+regex_replace(cmd_line, "-fsdev synth,", "-fsdev local,");
+
+/* append 'path=...' to '-fsdev ...' group */
+regex_replace(cmd_line, "(-fsdev \\w[^ ]*)", "\\1,path='%s'",
+ local_test_path);
+
+if (!args) {
+return;
+}
+
+/* append passed args to '-fsdev ...' group */
+regex_replace(cmd_line, "(-fsdev \\w[^ ]*)", "\\1,%s", args);
+}
+
static void virtio_9p_register_nodes(void)
{
const char *str_simple = "fsdev=fsdev0,mount_tag=" MOUNT_TAG;
const char *str_addr = "fsdev=fsdev0,addr=04.0,mount_tag=" MOUNT_TAG;
+/* make sure test dir for the 'local' tests exists and is clean */
+init_local_test_path();
+create_local_test_dir();
+
QPCIAddress addr = {
.devfn = QPCI_DEVFN(4, 0),
};
diff --git a/tests/qtest/libqos/virtio-9p.h b/tests/qtest/libqos/virtio-9p.h
index b1e6badc4a..326a603f72 100644
--- a/tests/qtest/libqos/virtio-9p.h
+++ b/tests/qtest/libqos/virtio-9p.h
@@ -44,4 +44,9 @@ struct QVirtio9PDevice {
QVirtio9P v9p;
};
+/**
+ * Prepares QEMU command line for 9pfs tests using the 'local' fs driver.
+ */
+void virtio_9p_assign_local_driver(GString *cmd_line, const char *args);
+
#endif
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 3281153b9c..af7e169d3a 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -895,29 +895,45 @@ static void fs_readdir_split_512(void *obj, void *data,
fs_readdir_split(obj, data, t_alloc, 512);
}
[PULL v2 1/5] tests/9pfs: change qtest name prefix to synth
All existing 9pfs test cases are using the 'synth' fs driver so far, which
means they are not accessing real files, but a purely simulated (in RAM
only) file system.
Let's make this clear by changing the prefix of the individual qtest case
names from 'fs/' to 'synth/'. That way they'll be easily distinguishable
from upcoming new 9pfs test cases supposed to be using a different fs
driver.
Signed-off-by: Christian Schoenebeck
Message-Id:
Signed-off-by: Christian Schoenebeck
---
tests/qtest/virtio-9p-test.c | 28 ++--
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index de30b717b6..3281153b9c 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -897,26 +897,26 @@ static void fs_readdir_split_512(void *obj, void *data,
static void register_virtio_9p_test(void)
{
-qos_add_test("config", "virtio-9p", pci_config, NULL);
-qos_add_test("fs/version/basic", "virtio-9p", fs_version, NULL);
-qos_add_test("fs/attach/basic", "virtio-9p", fs_attach, NULL);
-qos_add_test("fs/walk/basic", "virtio-9p", fs_walk, NULL);
-qos_add_test("fs/walk/no_slash", "virtio-9p", fs_walk_no_slash,
+qos_add_test("synth/config", "virtio-9p", pci_config, NULL);
+qos_add_test("synth/version/basic", "virtio-9p", fs_version, NULL);
+qos_add_test("synth/attach/basic", "virtio-9p", fs_attach, NULL);
+qos_add_test("synth/walk/basic", "virtio-9p", fs_walk, NULL);
+qos_add_test("synth/walk/no_slash", "virtio-9p", fs_walk_no_slash,
NULL);
-qos_add_test("fs/walk/dotdot_from_root", "virtio-9p",
+qos_add_test("synth/walk/dotdot_from_root", "virtio-9p",
fs_walk_dotdot, NULL);
-qos_add_test("fs/lopen/basic", "virtio-9p", fs_lopen, NULL);
-qos_add_test("fs/write/basic", "virtio-9p", fs_write, NULL);
-qos_add_test("fs/flush/success", "virtio-9p", fs_flush_success,
+qos_add_test("synth/lopen/basic", "virtio-9p", fs_lopen, NULL);
+qos_add_test("synth/write/basic", "virtio-9p", fs_write, NULL);
+qos_add_test("synth/flush/success", "virtio-9p", fs_flush_success,
NULL);
-qos_add_test("fs/flush/ignored", "virtio-9p", fs_flush_ignored,
+qos_add_test("synth/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",
+qos_add_test("synth/readdir/basic", "virtio-9p", fs_readdir, NULL);
+qos_add_test("synth/readdir/split_512", "virtio-9p",
fs_readdir_split_512, NULL);
-qos_add_test("fs/readdir/split_256", "virtio-9p",
+qos_add_test("synth/readdir/split_256", "virtio-9p",
fs_readdir_split_256, NULL);
-qos_add_test("fs/readdir/split_128", "virtio-9p",
+qos_add_test("synth/readdir/split_128", "virtio-9p",
fs_readdir_split_128, NULL);
}
--
2.20.1
Re: [PULL 0/5] 9p queue 2020-10-15
On Samstag, 17. Oktober 2020 12:50:13 CEST Peter Maydell wrote:
> On Thu, 15 Oct 2020 at 22:04, Christian Schoenebeck
>
> wrote:
> > The following changes since commit
57c98ea9acdcef5021f5671efa6475a5794a51c4:
> > Merge remote-tracking branch
> > 'remotes/kraxel/tags/ui-20201014-pull-request' into staging (2020-10-14
> > 13:56:06 +0100)>
> > are available in the Git repository at:
> > https://github.com/cschoenebeck/qemu.git tags/pull-9p-20201015
> >
> > for you to fetch changes up to 97a64ec211d051439b654950ed3f7cffc47d489b:
> > tests/9pfs: add local Tmkdir test (2020-10-15 16:11:17 +0200)
> >
> >
> > 9pfs: add tests using local fs driver
> >
> > The currently existing 9pfs test cases are all solely using the 9pfs
> > 'synth' fileystem driver, which is a very simple and purely simulated (in
> > RAM only) filesystem. There are issues though where the 'synth' fs driver
> > is not sufficient. For example the following two bugs need test cases
> > running the 9pfs 'local' fs driver:
> >
> > https://bugs.launchpad.net/qemu/+bug/1336794
> > https://bugs.launchpad.net/qemu/+bug/1877384
> >
> > This patch set for that reason introduces 9pfs test cases using the 9pfs
> > 'local' filesystem driver along to the already existing tests on 'synth'.
>
> Build failure, OSX:
>
> Compiling C object tests/qtest/libqos/libqos.fa.p/virtio-9p.c.o
> ../../tests/qtest/libqos/virtio-9p.c:37:17: error: implicit
> declaration of function 'get_current_dir_name' is invalid in C99
> [-Werror,-Wimplicit-function-declaration]
> char *pwd = get_current_dir_name();
> ^
> ../../tests/qtest/libqos/virtio-9p.c:37:17: error: this function
> declaration is not a prototype [-Werror,-Wstrict-prototypes]
> ../../tests/qtest/libqos/virtio-9p.c:37:11: error: incompatible
> integer to pointer conversion initializing 'char *' with an expression
> of type 'int' [-Werror,-Wint-conversion]
> char *pwd = get_current_dir_name();
>
>
> thanks
> -- PMM
Oops, get_current_dir_name() is a GNU extension. I just enabled Cirrus-CI to
prevent this from happening again. Sorry Peter.
Am I supposed to rebase for v2 PRs?
Fix for this is currently test running:
> index 1524982634..d43647b3b7 100644
> --- a/tests/qtest/libqos/virtio-9p.c
> +++ b/tests/qtest/libqos/virtio-9p.c
> @@ -34,9 +34,9 @@ static char *concat_path(const char* a, const char* b)
>
> static void init_local_test_path(void)
> {
>
> -char *pwd = get_current_dir_name();
> +char *pwd = g_get_current_dir();
>
> local_test_path = concat_path(pwd, "qtest-9p-local");
>
> -free(pwd);
> +g_free(pwd);
>
> }
>
> /* Creates the directory for the 9pfs 'local' filesystem driver to access.
> */
Best regards,
Christian Schoenebeck
[PULL 3/5] tests/9pfs: wipe local 9pfs test directory
Before running the first 9pfs test case, make sure the test directory
for running the 9pfs 'local' tests on is entirely empty. For that
reason simply delete the test directory (if any) before (re)creating
it on test suite startup.
Note: The preferable precise behaviour would be the test directory
only being wiped once *before* a test suite run. Right now the test
directory is also wiped at the *end* of a test suite run because
libqos is calling the virtio_9p_register_nodes() callback for some
reason also when a test suite completed. This is suboptimal as
developers cannot immediately see what files and directories the
9pfs local tests created precisely after the test suite completed.
But fortunately the test directory is not wiped if some test failed.
So it is probably not worth it drilling another hole into libqos
for this issue.
Signed-off-by: Christian Schoenebeck
Message-Id:
Signed-off-by: Christian Schoenebeck
---
tests/qtest/libqos/virtio-9p.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index 9cb284cb3c..bd53498041 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -53,6 +53,18 @@ static void create_local_test_dir(void)
g_assert((st.st_mode & S_IFMT) == S_IFDIR);
}
+/* Deletes directory previously created by create_local_test_dir(). */
+static void remove_local_test_dir(void)
+{
+g_assert(local_test_path != NULL);
+char *cmd = g_strdup_printf("rm -r '%s'\n", local_test_path);
+int res = system(cmd);
+if (res < 0) {
+/* ignore error, dummy check to prevent compiler error */
+}
+g_free(cmd);
+}
+
static void virtio_9p_cleanup(QVirtio9P *interface)
{
qvirtqueue_cleanup(interface->vdev->bus, interface->vq, alloc);
@@ -230,6 +242,7 @@ static void virtio_9p_register_nodes(void)
/* make sure test dir for the 'local' tests exists and is clean */
init_local_test_path();
+remove_local_test_dir();
create_local_test_dir();
QPCIAddress addr = {
--
2.20.1
[PULL 2/5] tests/9pfs: introduce local tests
This patch introduces 9pfs test cases using the 9pfs 'local'
filesystem driver which reads/writes/creates/deletes real files
and directories.
In this initial version, there is only one local test which actually
only checks if the 9pfs 'local' device was created successfully.
Before the 9pfs 'local' tests are run, a test directory 'qtest-9p-local'
is created (with world rwx permissions) under the current working
directory. At this point that test directory is not auto deleted yet.
Signed-off-by: Christian Schoenebeck
Message-Id:
<81fc4b3b6b6c9bf7999e79f5e7cbc364a5f09ddb.1602182956.git.qemu_...@crudebyte.com>
Signed-off-by: Christian Schoenebeck
---
tests/qtest/libqos/virtio-9p.c | 81 ++
tests/qtest/libqos/virtio-9p.h | 5 +++
tests/qtest/virtio-9p-test.c | 44 --
3 files changed, 116 insertions(+), 14 deletions(-)
diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index 2e300063e3..9cb284cb3c 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -24,6 +24,34 @@
#include "qgraph.h"
static QGuestAllocator *alloc;
+static char *local_test_path;
+
+/* Concatenates the passed 2 pathes. Returned result must be freed. */
+static char *concat_path(const char* a, const char* b)
+{
+return g_build_filename(a, b, NULL);
+}
+
+static void init_local_test_path(void)
+{
+char *pwd = get_current_dir_name();
+local_test_path = concat_path(pwd, "qtest-9p-local");
+free(pwd);
+}
+
+/* Creates the directory for the 9pfs 'local' filesystem driver to access. */
+static void create_local_test_dir(void)
+{
+struct stat st;
+
+g_assert(local_test_path != NULL);
+mkdir(local_test_path, 0777);
+
+/* ensure test directory exists now ... */
+g_assert(stat(local_test_path, ) == 0);
+/* ... and is actually a directory */
+g_assert((st.st_mode & S_IFMT) == S_IFDIR);
+}
static void virtio_9p_cleanup(QVirtio9P *interface)
{
@@ -146,11 +174,64 @@ static void *virtio_9p_pci_create(void *pci_bus,
QGuestAllocator *t_alloc,
return obj;
}
+/**
+ * Performs regular expression based search and replace on @a haystack.
+ *
+ * @param haystack - input string to be parsed, result of replacement is
+ * stored back to @a haystack
+ * @param pattern - the regular expression pattern for scanning @a haystack
+ * @param replace_fmt - matches of supplied @a pattern are replaced by this,
+ * if necessary glib printf format can be used to add
+ * variable arguments of this function to this
+ * replacement string
+ */
+static void regex_replace(GString *haystack, const char *pattern,
+ const char *replace_fmt, ...)
+{
+GRegex *regex;
+char *replace, *s;
+va_list argp;
+
+va_start(argp, replace_fmt);
+replace = g_strdup_vprintf(replace_fmt, argp);
+va_end(argp);
+
+regex = g_regex_new(pattern, 0, 0, NULL);
+s = g_regex_replace(regex, haystack->str, -1, 0, replace, 0, NULL);
+g_string_assign(haystack, s);
+g_free(s);
+g_regex_unref(regex);
+g_free(replace);
+}
+
+void virtio_9p_assign_local_driver(GString *cmd_line, const char *args)
+{
+g_assert_nonnull(local_test_path);
+
+/* replace 'synth' driver by 'local' driver */
+regex_replace(cmd_line, "-fsdev synth,", "-fsdev local,");
+
+/* append 'path=...' to '-fsdev ...' group */
+regex_replace(cmd_line, "(-fsdev \\w[^ ]*)", "\\1,path='%s'",
+ local_test_path);
+
+if (!args) {
+return;
+}
+
+/* append passed args to '-fsdev ...' group */
+regex_replace(cmd_line, "(-fsdev \\w[^ ]*)", "\\1,%s", args);
+}
+
static void virtio_9p_register_nodes(void)
{
const char *str_simple = "fsdev=fsdev0,mount_tag=" MOUNT_TAG;
const char *str_addr = "fsdev=fsdev0,addr=04.0,mount_tag=" MOUNT_TAG;
+/* make sure test dir for the 'local' tests exists and is clean */
+init_local_test_path();
+create_local_test_dir();
+
QPCIAddress addr = {
.devfn = QPCI_DEVFN(4, 0),
};
diff --git a/tests/qtest/libqos/virtio-9p.h b/tests/qtest/libqos/virtio-9p.h
index b1e6badc4a..326a603f72 100644
--- a/tests/qtest/libqos/virtio-9p.h
+++ b/tests/qtest/libqos/virtio-9p.h
@@ -44,4 +44,9 @@ struct QVirtio9PDevice {
QVirtio9P v9p;
};
+/**
+ * Prepares QEMU command line for 9pfs tests using the 'local' fs driver.
+ */
+void virtio_9p_assign_local_driver(GString *cmd_line, const char *args);
+
#endif
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 3281153b9c..af7e169d3a 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -895,29 +895,45 @@ static void fs_readdir_split_512(void *obj, void *data,
fs_readdir_split(obj, data, t_alloc, 512);
}
[PULL 5/5] tests/9pfs: add local Tmkdir test
This test case uses the 9pfs 'local' driver to create a directory
and then checks if the expected directory was actually created
(as real directory) on host side.
This patch introduces a custom split() implementation, because
the test code requires non empty array elements as result. For
that reason g_strsplit() would not be a good alternative, as
it would require additional filter code for reshuffling the
array, and the resulting code would be even more complex than
this split() function.
Signed-off-by: Christian Schoenebeck
Message-Id:
Signed-off-by: Christian Schoenebeck
---
tests/qtest/virtio-9p-test.c | 139 +++
1 file changed, 139 insertions(+)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index af7e169d3a..c15908f27b 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -18,6 +18,62 @@
#define QVIRTIO_9P_TIMEOUT_US (10 * 1000 * 1000)
static QGuestAllocator *alloc;
+/*
+ * Used to auto generate new fids. Start with arbitrary high value to avoid
+ * collision with hard coded fids in basic test code.
+ */
+static uint32_t fid_generator = 1000;
+
+static uint32_t genfid(void)
+{
+return fid_generator++;
+}
+
+/**
+ * Splits the @a in string by @a delim into individual (non empty) strings
+ * and outputs them to @a out. The output array @a out is NULL terminated.
+ *
+ * Output array @a out must be freed by calling split_free().
+ *
+ * @returns number of individual elements in output array @a out (without the
+ * final NULL terminating element)
+ */
+static int split(const char *in, const char *delim, char ***out)
+{
+int n = 0, i = 0;
+char *tmp, *p;
+
+tmp = g_strdup(in);
+for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim)) {
+if (strlen(p) > 0) {
+++n;
+}
+}
+g_free(tmp);
+
+*out = g_new0(char *, n + 1); /* last element NULL delimiter */
+
+tmp = g_strdup(in);
+for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim)) {
+if (strlen(p) > 0) {
+(*out)[i++] = g_strdup(p);
+}
+}
+g_free(tmp);
+
+return n;
+}
+
+static void split_free(char ***out)
+{
+int i;
+for (i = 0; (*out)[i]; ++i) {
+g_free((*out)[i]);
+}
+g_free(*out);
+*out = NULL;
+}
+
static void pci_config(void *obj, void *data, QGuestAllocator *t_alloc)
{
QVirtio9P *v9p = obj;
@@ -201,6 +257,7 @@ static const char *rmessage_name(uint8_t id)
id == P9_RWALK ? "RWALK" :
id == P9_RLOPEN ? "RLOPEN" :
id == P9_RWRITE ? "RWRITE" :
+id == P9_RMKDIR ? "RMKDIR" :
id == P9_RFLUSH ? "RFLUSH" :
id == P9_RREADDIR ? "READDIR" :
"";
@@ -578,6 +635,39 @@ static bool fs_dirents_contain_name(struct V9fsDirent *e,
const char* name)
return false;
}
+/* size[4] Tmkdir tag[2] dfid[4] name[s] mode[4] gid[4] */
+static P9Req *v9fs_tmkdir(QVirtio9P *v9p, uint32_t dfid, const char *name,
+ uint32_t mode, uint32_t gid, uint16_t tag)
+{
+P9Req *req;
+
+uint32_t body_size = 4 + 4 + 4;
+uint16_t string_size = v9fs_string_size(name);
+
+g_assert_cmpint(body_size, <=, UINT32_MAX - string_size);
+body_size += string_size;
+
+req = v9fs_req_init(v9p, body_size, P9_TMKDIR, tag);
+v9fs_uint32_write(req, dfid);
+v9fs_string_write(req, name);
+v9fs_uint32_write(req, mode);
+v9fs_uint32_write(req, gid);
+v9fs_req_send(req);
+return req;
+}
+
+/* size[4] Rmkdir tag[2] qid[13] */
+static void v9fs_rmkdir(P9Req *req, v9fs_qid *qid)
+{
+v9fs_req_recv(req, P9_RMKDIR);
+if (qid) {
+v9fs_memread(req, qid, 13);
+} else {
+v9fs_memskip(req, 13);
+}
+v9fs_req_free(req);
+}
+
/* basic readdir test where reply fits into a single response message */
static void fs_readdir(void *obj, void *data, QGuestAllocator *t_alloc)
{
@@ -877,6 +967,30 @@ static void fs_flush_ignored(void *obj, void *data,
QGuestAllocator *t_alloc)
g_free(wnames[0]);
}
+static void fs_mkdir(void *obj, void *data, QGuestAllocator *t_alloc,
+ const char *path, const char *cname)
+{
+QVirtio9P *v9p = obj;
+alloc = t_alloc;
+char **wnames;
+char *const name = g_strdup(cname);
+P9Req *req;
+const uint32_t fid = genfid();
+
+int nwnames = split(path, "/", );
+
+req = v9fs_twalk(v9p, 0, fid, nwnames, wnames, 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_rwalk(req, NULL, NULL);
+
+req = v9fs_tmkdir(v9p, fid, name, 0750, 0, 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_rmkdir(req, NULL);
+
+g_free(name);
+split_free();
+}
+
static void fs_readdir_split_128(void *obj, void *data,
QGuestAllocator *t_alloc)
{
@@ -895,6 +1009,30 @@ static void fs_readdir_split_512(void *obj, vo
[PULL 1/5] tests/9pfs: change qtest name prefix to synth
All existing 9pfs test cases are using the 'synth' fs driver so far, which
means they are not accessing real files, but a purely simulated (in RAM
only) file system.
Let's make this clear by changing the prefix of the individual qtest case
names from 'fs/' to 'synth/'. That way they'll be easily distinguishable
from upcoming new 9pfs test cases supposed to be using a different fs
driver.
Signed-off-by: Christian Schoenebeck
Message-Id:
Signed-off-by: Christian Schoenebeck
---
tests/qtest/virtio-9p-test.c | 28 ++--
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index de30b717b6..3281153b9c 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -897,26 +897,26 @@ static void fs_readdir_split_512(void *obj, void *data,
static void register_virtio_9p_test(void)
{
-qos_add_test("config", "virtio-9p", pci_config, NULL);
-qos_add_test("fs/version/basic", "virtio-9p", fs_version, NULL);
-qos_add_test("fs/attach/basic", "virtio-9p", fs_attach, NULL);
-qos_add_test("fs/walk/basic", "virtio-9p", fs_walk, NULL);
-qos_add_test("fs/walk/no_slash", "virtio-9p", fs_walk_no_slash,
+qos_add_test("synth/config", "virtio-9p", pci_config, NULL);
+qos_add_test("synth/version/basic", "virtio-9p", fs_version, NULL);
+qos_add_test("synth/attach/basic", "virtio-9p", fs_attach, NULL);
+qos_add_test("synth/walk/basic", "virtio-9p", fs_walk, NULL);
+qos_add_test("synth/walk/no_slash", "virtio-9p", fs_walk_no_slash,
NULL);
-qos_add_test("fs/walk/dotdot_from_root", "virtio-9p",
+qos_add_test("synth/walk/dotdot_from_root", "virtio-9p",
fs_walk_dotdot, NULL);
-qos_add_test("fs/lopen/basic", "virtio-9p", fs_lopen, NULL);
-qos_add_test("fs/write/basic", "virtio-9p", fs_write, NULL);
-qos_add_test("fs/flush/success", "virtio-9p", fs_flush_success,
+qos_add_test("synth/lopen/basic", "virtio-9p", fs_lopen, NULL);
+qos_add_test("synth/write/basic", "virtio-9p", fs_write, NULL);
+qos_add_test("synth/flush/success", "virtio-9p", fs_flush_success,
NULL);
-qos_add_test("fs/flush/ignored", "virtio-9p", fs_flush_ignored,
+qos_add_test("synth/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",
+qos_add_test("synth/readdir/basic", "virtio-9p", fs_readdir, NULL);
+qos_add_test("synth/readdir/split_512", "virtio-9p",
fs_readdir_split_512, NULL);
-qos_add_test("fs/readdir/split_256", "virtio-9p",
+qos_add_test("synth/readdir/split_256", "virtio-9p",
fs_readdir_split_256, NULL);
-qos_add_test("fs/readdir/split_128", "virtio-9p",
+qos_add_test("synth/readdir/split_128", "virtio-9p",
fs_readdir_split_128, NULL);
}
--
2.20.1
[PULL 0/5] 9p queue 2020-10-15
The following changes since commit 57c98ea9acdcef5021f5671efa6475a5794a51c4: Merge remote-tracking branch 'remotes/kraxel/tags/ui-20201014-pull-request' into staging (2020-10-14 13:56:06 +0100) are available in the Git repository at: https://github.com/cschoenebeck/qemu.git tags/pull-9p-20201015 for you to fetch changes up to 97a64ec211d051439b654950ed3f7cffc47d489b: tests/9pfs: add local Tmkdir test (2020-10-15 16:11:17 +0200) 9pfs: add tests using local fs driver The currently existing 9pfs test cases are all solely using the 9pfs 'synth' fileystem driver, which is a very simple and purely simulated (in RAM only) filesystem. There are issues though where the 'synth' fs driver is not sufficient. For example the following two bugs need test cases running the 9pfs 'local' fs driver: https://bugs.launchpad.net/qemu/+bug/1336794 https://bugs.launchpad.net/qemu/+bug/1877384 This patch set for that reason introduces 9pfs test cases using the 9pfs 'local' filesystem driver along to the already existing tests on 'synth'. Christian Schoenebeck (5): tests/9pfs: change qtest name prefix to synth tests/9pfs: introduce local tests tests/9pfs: wipe local 9pfs test directory tests/9pfs: add virtio_9p_test_path() tests/9pfs: add local Tmkdir test tests/qtest/libqos/virtio-9p.c | 100 + tests/qtest/libqos/virtio-9p.h | 10 +++ tests/qtest/virtio-9p-test.c | 197 - 3 files changed, 286 insertions(+), 21 deletions(-)
[PULL 4/5] tests/9pfs: add virtio_9p_test_path()
This new public function virtio_9p_test_path() allows 9pfs
'local' tests to translate a path from guest scope to host
scope. For instance by passing an empty string it would
return the root path on host of the exported 9pfs tree.
Signed-off-by: Christian Schoenebeck
Message-Id:
Signed-off-by: Christian Schoenebeck
---
tests/qtest/libqos/virtio-9p.c | 6 ++
tests/qtest/libqos/virtio-9p.h | 5 +
2 files changed, 11 insertions(+)
diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index bd53498041..1524982634 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -65,6 +65,12 @@ static void remove_local_test_dir(void)
g_free(cmd);
}
+char *virtio_9p_test_path(const char *path)
+{
+g_assert(local_test_path);
+return concat_path(local_test_path, path);
+}
+
static void virtio_9p_cleanup(QVirtio9P *interface)
{
qvirtqueue_cleanup(interface->vdev->bus, interface->vq, alloc);
diff --git a/tests/qtest/libqos/virtio-9p.h b/tests/qtest/libqos/virtio-9p.h
index 326a603f72..19a4d97454 100644
--- a/tests/qtest/libqos/virtio-9p.h
+++ b/tests/qtest/libqos/virtio-9p.h
@@ -49,4 +49,9 @@ struct QVirtio9PDevice {
*/
void virtio_9p_assign_local_driver(GString *cmd_line, const char *args);
+/**
+ * Returns path on host to the passed guest path. Result must be freed.
+ */
+char *virtio_9p_test_path(const char *path);
+
#endif
--
2.20.1
Re: [PATCH v4 08/12] tests/9pfs: change qtest name prefix to synth
On Mittwoch, 14. Oktober 2020 21:38:16 CEST Greg Kurz wrote: > On Wed, 14 Oct 2020 17:25:35 +0200 > > Christian Schoenebeck wrote: > > On Donnerstag, 8. Oktober 2020 20:34:56 CEST Christian Schoenebeck wrote: > > > All existing 9pfs test cases are using the 'synth' fs driver so far, > > > which > > > means they are not accessing real files, but a purely simulated (in RAM > > > only) file system. > > > > > > Let's make this clear by changing the prefix of the individual qtest > > > case > > > names from 'fs/' to 'synth/'. That way they'll be easily distinguishable > > > from upcoming new 9pfs test cases supposed to be using a different fs > > > driver. > > > > > > Signed-off-by: Christian Schoenebeck > > > --- > > > > > > tests/qtest/virtio-9p-test.c | 28 ++-- > > > 1 file changed, 14 insertions(+), 14 deletions(-) > > > > Queued patches 8 .. 12 on 9p.next: > > > > https://github.com/cschoenebeck/qemu/commits/9p.next > > Hi Chistian, > > I could only have a quick glimpse at the patches and LGTM. > > Thanks for taking care. > > Cheers, > > -- > Greg > Thanks, I appreciate it. Best regards, Christian Schoenebeck
Re: [PATCH v4 08/12] tests/9pfs: change qtest name prefix to synth
On Donnerstag, 8. Oktober 2020 20:34:56 CEST Christian Schoenebeck wrote: > All existing 9pfs test cases are using the 'synth' fs driver so far, which > means they are not accessing real files, but a purely simulated (in RAM > only) file system. > > Let's make this clear by changing the prefix of the individual qtest case > names from 'fs/' to 'synth/'. That way they'll be easily distinguishable > from upcoming new 9pfs test cases supposed to be using a different fs > driver. > > Signed-off-by: Christian Schoenebeck > --- > tests/qtest/virtio-9p-test.c | 28 ++-- > 1 file changed, 14 insertions(+), 14 deletions(-) Queued patches 8 .. 12 on 9p.next: https://github.com/cschoenebeck/qemu/commits/9p.next Best regards, Christian Schoenebeck
[PATCH v4 10/12] tests/9pfs: wipe local 9pfs test directory
Before running the first 9pfs test case, make sure the test directory
for running the 9pfs 'local' tests on is entirely empty. For that
reason simply delete the test directory (if any) before (re)creating
it on test suite startup.
Note: The preferable precise behaviour would be the test directory
only being wiped once *before* a test suite run. Right now the test
directory is also wiped at the *end* of a test suite run because
libqos is calling the virtio_9p_cleanup() callback for some reason
also when a test suite completed. This is suboptimal as developers
cannot immediately see what files and directories the 9pfs local
tests created precisely after the test suite completed. But
fortunately the test directory is not wiped if some test failed.
So it is probably not worth it drilling another hole into libqos
for this issue.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/libqos/virtio-9p.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index 9cb284cb3c..bd53498041 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -53,6 +53,18 @@ static void create_local_test_dir(void)
g_assert((st.st_mode & S_IFMT) == S_IFDIR);
}
+/* Deletes directory previously created by create_local_test_dir(). */
+static void remove_local_test_dir(void)
+{
+g_assert(local_test_path != NULL);
+char *cmd = g_strdup_printf("rm -r '%s'\n", local_test_path);
+int res = system(cmd);
+if (res < 0) {
+/* ignore error, dummy check to prevent compiler error */
+}
+g_free(cmd);
+}
+
static void virtio_9p_cleanup(QVirtio9P *interface)
{
qvirtqueue_cleanup(interface->vdev->bus, interface->vq, alloc);
@@ -230,6 +242,7 @@ static void virtio_9p_register_nodes(void)
/* make sure test dir for the 'local' tests exists and is clean */
init_local_test_path();
+remove_local_test_dir();
create_local_test_dir();
QPCIAddress addr = {
--
2.20.1
[PATCH v4 11/12] tests/9pfs: add virtio_9p_test_path()
This new public function virtio_9p_test_path() allows 9pfs
'local' tests to translate a path from guest scope to host
scope. For instance by passing an empty string it would
return the root path on host of the exported 9pfs tree.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/libqos/virtio-9p.c | 6 ++
tests/qtest/libqos/virtio-9p.h | 5 +
2 files changed, 11 insertions(+)
diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index bd53498041..1524982634 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -65,6 +65,12 @@ static void remove_local_test_dir(void)
g_free(cmd);
}
+char *virtio_9p_test_path(const char *path)
+{
+g_assert(local_test_path);
+return concat_path(local_test_path, path);
+}
+
static void virtio_9p_cleanup(QVirtio9P *interface)
{
qvirtqueue_cleanup(interface->vdev->bus, interface->vq, alloc);
diff --git a/tests/qtest/libqos/virtio-9p.h b/tests/qtest/libqos/virtio-9p.h
index 326a603f72..19a4d97454 100644
--- a/tests/qtest/libqos/virtio-9p.h
+++ b/tests/qtest/libqos/virtio-9p.h
@@ -49,4 +49,9 @@ struct QVirtio9PDevice {
*/
void virtio_9p_assign_local_driver(GString *cmd_line, const char *args);
+/**
+ * Returns path on host to the passed guest path. Result must be freed.
+ */
+char *virtio_9p_test_path(const char *path);
+
#endif
--
2.20.1
[PATCH v4 12/12] tests/9pfs: add local Tmkdir test
This test case uses the 9pfs 'local' driver to create a directory
and then checks if the expected directory was actually created
(as real directory) on host side.
This patch introduces a custom split() implementation, because
the test code requires non empty array elements as result. For
that reason g_strsplit() would not be a good alternative, as
it would require additional filter code for reshuffling the
array, and the resulting code would be even more complex than
this split() function.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/virtio-9p-test.c | 139 +++
1 file changed, 139 insertions(+)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index af7e169d3a..c15908f27b 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -18,6 +18,62 @@
#define QVIRTIO_9P_TIMEOUT_US (10 * 1000 * 1000)
static QGuestAllocator *alloc;
+/*
+ * Used to auto generate new fids. Start with arbitrary high value to avoid
+ * collision with hard coded fids in basic test code.
+ */
+static uint32_t fid_generator = 1000;
+
+static uint32_t genfid(void)
+{
+return fid_generator++;
+}
+
+/**
+ * Splits the @a in string by @a delim into individual (non empty) strings
+ * and outputs them to @a out. The output array @a out is NULL terminated.
+ *
+ * Output array @a out must be freed by calling split_free().
+ *
+ * @returns number of individual elements in output array @a out (without the
+ * final NULL terminating element)
+ */
+static int split(const char *in, const char *delim, char ***out)
+{
+int n = 0, i = 0;
+char *tmp, *p;
+
+tmp = g_strdup(in);
+for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim)) {
+if (strlen(p) > 0) {
+++n;
+}
+}
+g_free(tmp);
+
+*out = g_new0(char *, n + 1); /* last element NULL delimiter */
+
+tmp = g_strdup(in);
+for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim)) {
+if (strlen(p) > 0) {
+(*out)[i++] = g_strdup(p);
+}
+}
+g_free(tmp);
+
+return n;
+}
+
+static void split_free(char ***out)
+{
+int i;
+for (i = 0; (*out)[i]; ++i) {
+g_free((*out)[i]);
+}
+g_free(*out);
+*out = NULL;
+}
+
static void pci_config(void *obj, void *data, QGuestAllocator *t_alloc)
{
QVirtio9P *v9p = obj;
@@ -201,6 +257,7 @@ static const char *rmessage_name(uint8_t id)
id == P9_RWALK ? "RWALK" :
id == P9_RLOPEN ? "RLOPEN" :
id == P9_RWRITE ? "RWRITE" :
+id == P9_RMKDIR ? "RMKDIR" :
id == P9_RFLUSH ? "RFLUSH" :
id == P9_RREADDIR ? "READDIR" :
"";
@@ -578,6 +635,39 @@ static bool fs_dirents_contain_name(struct V9fsDirent *e,
const char* name)
return false;
}
+/* size[4] Tmkdir tag[2] dfid[4] name[s] mode[4] gid[4] */
+static P9Req *v9fs_tmkdir(QVirtio9P *v9p, uint32_t dfid, const char *name,
+ uint32_t mode, uint32_t gid, uint16_t tag)
+{
+P9Req *req;
+
+uint32_t body_size = 4 + 4 + 4;
+uint16_t string_size = v9fs_string_size(name);
+
+g_assert_cmpint(body_size, <=, UINT32_MAX - string_size);
+body_size += string_size;
+
+req = v9fs_req_init(v9p, body_size, P9_TMKDIR, tag);
+v9fs_uint32_write(req, dfid);
+v9fs_string_write(req, name);
+v9fs_uint32_write(req, mode);
+v9fs_uint32_write(req, gid);
+v9fs_req_send(req);
+return req;
+}
+
+/* size[4] Rmkdir tag[2] qid[13] */
+static void v9fs_rmkdir(P9Req *req, v9fs_qid *qid)
+{
+v9fs_req_recv(req, P9_RMKDIR);
+if (qid) {
+v9fs_memread(req, qid, 13);
+} else {
+v9fs_memskip(req, 13);
+}
+v9fs_req_free(req);
+}
+
/* basic readdir test where reply fits into a single response message */
static void fs_readdir(void *obj, void *data, QGuestAllocator *t_alloc)
{
@@ -877,6 +967,30 @@ static void fs_flush_ignored(void *obj, void *data,
QGuestAllocator *t_alloc)
g_free(wnames[0]);
}
+static void fs_mkdir(void *obj, void *data, QGuestAllocator *t_alloc,
+ const char *path, const char *cname)
+{
+QVirtio9P *v9p = obj;
+alloc = t_alloc;
+char **wnames;
+char *const name = g_strdup(cname);
+P9Req *req;
+const uint32_t fid = genfid();
+
+int nwnames = split(path, "/", );
+
+req = v9fs_twalk(v9p, 0, fid, nwnames, wnames, 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_rwalk(req, NULL, NULL);
+
+req = v9fs_tmkdir(v9p, fid, name, 0750, 0, 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_rmkdir(req, NULL);
+
+g_free(name);
+split_free();
+}
+
static void fs_readdir_split_128(void *obj, void *data,
QGuestAllocator *t_alloc)
{
@@ -895,6 +1009,30 @@ static void fs_readdir_split_512(void *obj, void *data,
fs_readdir_split(obj, data, t_alloc, 512
[PATCH v4 04/12] libqos/qgraph: add qos_dump_graph()
This new function is purely for debugging purposes. It prints the
current qos graph to stdout and allows to identify problems in the
created qos graph e.g. when writing new qos tests.
Coloured output is used to mark available nodes in green colour,
whereas unavailable nodes are marked in red colour.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/libqos/qgraph.c | 56 +
tests/qtest/libqos/qgraph.h | 20 +
2 files changed, 76 insertions(+)
diff --git a/tests/qtest/libqos/qgraph.c b/tests/qtest/libqos/qgraph.c
index 61faf6b27d..af93e38dcb 100644
--- a/tests/qtest/libqos/qgraph.c
+++ b/tests/qtest/libqos/qgraph.c
@@ -805,3 +805,59 @@ void qos_delete_cmd_line(const char *name)
node->command_line = NULL;
}
}
+
+#define RED(txt) (\
+"\033[0;91m" txt \
+"\033[0m" \
+)
+
+#define GREEN(txt) ( \
+"\033[0;92m" txt \
+"\033[0m" \
+)
+
+void qos_dump_graph(void)
+{
+GList *keys;
+GList *l;
+QOSGraphEdgeList *list;
+QOSGraphEdge *e, *next;
+QOSGraphNode *dest_node, *node;
+
+qos_printf("ALL QGRAPH EDGES: {\n");
+keys = g_hash_table_get_keys(edge_table);
+for (l = keys; l != NULL; l = l->next) {
+const gchar *key = l->data;
+qos_printf("\t src='%s'\n", key);
+list = get_edgelist(key);
+QSLIST_FOREACH_SAFE(e, list, edge_list, next) {
+dest_node = g_hash_table_lookup(node_table, e->dest);
+qos_printf("\t\t|-> dest='%s' type=%d (node=%p)",
+ e->dest, e->type, dest_node);
+if (!dest_node) {
+qos_printf_literal(RED(" <--- ERROR !"));
+}
+qos_printf_literal("\n");
+}
+}
+g_list_free(keys);
+qos_printf("}\n");
+
+qos_printf("ALL QGRAPH NODES: {\n");
+keys = g_hash_table_get_keys(node_table);
+for (l = keys; l != NULL; l = l->next) {
+const gchar *key = l->data;
+node = g_hash_table_lookup(node_table, key);
+qos_printf("\t name='%s' ", key);
+if (node->qemu_name) {
+qos_printf_literal("qemu_name='%s' ", node->qemu_name);
+}
+qos_printf_literal("type=%d cmd_line='%s' [%s]\n",
+ node->type, node->command_line,
+ node->available ? GREEN("available") :
+ RED("UNAVAILBLE")
+);
+}
+g_list_free(keys);
+qos_printf("}\n");
+}
diff --git a/tests/qtest/libqos/qgraph.h b/tests/qtest/libqos/qgraph.h
index f472949f68..07a32535f1 100644
--- a/tests/qtest/libqos/qgraph.h
+++ b/tests/qtest/libqos/qgraph.h
@@ -586,5 +586,25 @@ QOSGraphObject *qos_machine_new(QOSGraphNode *node,
QTestState *qts);
QOSGraphObject *qos_driver_new(QOSGraphNode *node, QOSGraphObject *parent,
QGuestAllocator *alloc, void *arg);
+/**
+ * Just for debugging purpose: prints all currently existing nodes and
+ * edges to stdout.
+ *
+ * All qtests add themselves to the overall qos graph by calling qgraph
+ * functions that add device nodes and edges between the individual graph
+ * nodes for tests. As the actual graph is assmbled at runtime by the qos
+ * subsystem, it is sometimes not obvious how the overall graph looks like.
+ * E.g. when writing new tests it may happen that those new tests are simply
+ * ignored by the qtest framework.
+ *
+ * This function allows to identify problems in the created qgraph. Keep in
+ * mind: only tests with a path down from the actual test case node (leaf) up
+ * to the graph's root node are actually executed by the qtest framework. And
+ * the qtest framework uses QMP to automatically check which QEMU drivers are
+ * actually currently available, and accordingly qos marks certain pathes as
+ * 'unavailable' in such cases (e.g. when QEMU was compiled without support for
+ * a certain feature).
+ */
+void qos_dump_graph(void);
#endif
--
2.20.1
[PATCH v4 03/12] libqos/qgraph_internal: add qos_printf() and qos_printf_literal()
These two are macros wrapping regular printf() call. They are intended
to be used instead of calling printf() directly in order to avoid
breaking TAP output format.
TAP output format is enabled by using --tap command line argument.
Starting with glib 2.62 it is enabled by default.
Unfortunately there is currently no public glib API available to check
whether TAP output format is enabled. For that reason qos_printf()
simply always prepends a '#' character for now.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/libqos/qgraph_internal.h | 11 +++
1 file changed, 11 insertions(+)
diff --git a/tests/qtest/libqos/qgraph_internal.h
b/tests/qtest/libqos/qgraph_internal.h
index 974985dce9..c0025f5ab9 100644
--- a/tests/qtest/libqos/qgraph_internal.h
+++ b/tests/qtest/libqos/qgraph_internal.h
@@ -255,4 +255,15 @@ void qos_delete_cmd_line(const char *name);
*/
void qos_graph_node_set_availability(const char *node, bool av);
+/*
+ * Prepends a '#' character in front for not breaking TAP output format.
+ */
+#define qos_printf(...) printf("# " __VA_ARGS__)
+
+/*
+ * Intended for printing something literally, i.e. for appending text as is
+ * to a line already been started by qos_printf() before.
+ */
+#define qos_printf_literal printf
+
#endif
--
2.20.1
[PATCH v4 08/12] tests/9pfs: change qtest name prefix to synth
All existing 9pfs test cases are using the 'synth' fs driver so far, which
means they are not accessing real files, but a purely simulated (in RAM
only) file system.
Let's make this clear by changing the prefix of the individual qtest case
names from 'fs/' to 'synth/'. That way they'll be easily distinguishable
from upcoming new 9pfs test cases supposed to be using a different fs
driver.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/virtio-9p-test.c | 28 ++--
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index de30b717b6..3281153b9c 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -897,26 +897,26 @@ static void fs_readdir_split_512(void *obj, void *data,
static void register_virtio_9p_test(void)
{
-qos_add_test("config", "virtio-9p", pci_config, NULL);
-qos_add_test("fs/version/basic", "virtio-9p", fs_version, NULL);
-qos_add_test("fs/attach/basic", "virtio-9p", fs_attach, NULL);
-qos_add_test("fs/walk/basic", "virtio-9p", fs_walk, NULL);
-qos_add_test("fs/walk/no_slash", "virtio-9p", fs_walk_no_slash,
+qos_add_test("synth/config", "virtio-9p", pci_config, NULL);
+qos_add_test("synth/version/basic", "virtio-9p", fs_version, NULL);
+qos_add_test("synth/attach/basic", "virtio-9p", fs_attach, NULL);
+qos_add_test("synth/walk/basic", "virtio-9p", fs_walk, NULL);
+qos_add_test("synth/walk/no_slash", "virtio-9p", fs_walk_no_slash,
NULL);
-qos_add_test("fs/walk/dotdot_from_root", "virtio-9p",
+qos_add_test("synth/walk/dotdot_from_root", "virtio-9p",
fs_walk_dotdot, NULL);
-qos_add_test("fs/lopen/basic", "virtio-9p", fs_lopen, NULL);
-qos_add_test("fs/write/basic", "virtio-9p", fs_write, NULL);
-qos_add_test("fs/flush/success", "virtio-9p", fs_flush_success,
+qos_add_test("synth/lopen/basic", "virtio-9p", fs_lopen, NULL);
+qos_add_test("synth/write/basic", "virtio-9p", fs_write, NULL);
+qos_add_test("synth/flush/success", "virtio-9p", fs_flush_success,
NULL);
-qos_add_test("fs/flush/ignored", "virtio-9p", fs_flush_ignored,
+qos_add_test("synth/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",
+qos_add_test("synth/readdir/basic", "virtio-9p", fs_readdir, NULL);
+qos_add_test("synth/readdir/split_512", "virtio-9p",
fs_readdir_split_512, NULL);
-qos_add_test("fs/readdir/split_256", "virtio-9p",
+qos_add_test("synth/readdir/split_256", "virtio-9p",
fs_readdir_split_256, NULL);
-qos_add_test("fs/readdir/split_128", "virtio-9p",
+qos_add_test("synth/readdir/split_128", "virtio-9p",
fs_readdir_split_128, NULL);
}
--
2.20.1
[PATCH v4 05/12] tests/qtest/qos-test: dump qos graph if verbose
If qtests were run in verbose mode (i.e. if --verbose CL argument was
provided) then dump the generated qos graph (all nodes and edges,
along with their current individual availability status) to stdout.
See API doc comment on function qos_dump_graph() for details.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/qos-test.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/tests/qtest/qos-test.c b/tests/qtest/qos-test.c
index 8fdf87b183..d98ef78613 100644
--- a/tests/qtest/qos-test.c
+++ b/tests/qtest/qos-test.c
@@ -322,6 +322,9 @@ int main(int argc, char **argv)
qos_set_machines_devices_available();
qos_graph_foreach_test_path(walk_path);
+if (g_test_verbose()) {
+qos_dump_graph();
+}
g_test_run();
qtest_end();
qos_graph_destroy();
--
2.20.1
[PATCH v4 07/12] tests/qtest/qos-test: dump QEMU command if verbose
If qtests are run in verbose mode (i.e. if --verbose CL argument
was provided) then print the assembled qemu command line for each
test.
Use qos_printf() instead of g_test_message() to avoid the latter
cluttering the output.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/qos-test.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/tests/qtest/qos-test.c b/tests/qtest/qos-test.c
index b279b6f816..f97d0a08fd 100644
--- a/tests/qtest/qos-test.c
+++ b/tests/qtest/qos-test.c
@@ -89,6 +89,9 @@ static void qos_set_machines_devices_available(void)
static void restart_qemu_or_continue(char *path)
{
+if (g_test_verbose()) {
+qos_printf("Run QEMU with: '%s'\n", path);
+}
/* compares the current command line with the
* one previously executed: if they are the same,
* don't restart QEMU, if they differ, stop previous
--
2.20.1
[PATCH v4 02/12] libqos/qgraph: add qos_node_create_driver_named()
So far the qos subsystem of the qtest framework had the limitation
that only one instance of the same official QEMU (QMP) driver name
could be created for qtests. That's because a) the created qos
node names must always be unique, b) the node name must match the
official QEMU driver name being instantiated and c) all nodes are
in a global space shared by all tests.
This patch removes this limitation by introducing a new function
qos_node_create_driver_named() which allows test case authors to
specify a node name being different from the actual associated
QEMU driver name. It fills the new 'qemu_name' field of
QOSGraphNode for that purpose.
Adjust build_driver_cmd_line() and qos_graph_node_set_availability()
to correctly deal with either accessing node name vs. node's
qemu_name correctly.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/libqos/qgraph.c | 53 ++---
tests/qtest/libqos/qgraph.h | 16 +++
2 files changed, 66 insertions(+), 3 deletions(-)
diff --git a/tests/qtest/libqos/qgraph.c b/tests/qtest/libqos/qgraph.c
index e42f3eaafa..61faf6b27d 100644
--- a/tests/qtest/libqos/qgraph.c
+++ b/tests/qtest/libqos/qgraph.c
@@ -287,7 +287,8 @@ static void build_machine_cmd_line(QOSGraphNode *node,
const char *args)
*/
static void build_driver_cmd_line(QOSGraphNode *node)
{
-node->command_line = g_strconcat(" -device ", node->name, NULL);
+const char *name = node->qemu_name ?: node->name;
+node->command_line = g_strconcat(" -device ", name, NULL);
}
/* qos_print_cb(): callback prints all path found by the DFS algorithm. */
@@ -632,6 +633,15 @@ void qos_node_create_driver(const char *name,
QOSCreateDriverFunc function)
node->u.driver.constructor = function;
}
+void qos_node_create_driver_named(const char *name, const char *qemu_name,
+ QOSCreateDriverFunc function)
+{
+QOSGraphNode *node = create_node(name, QNODE_DRIVER);
+node->qemu_name = g_strdup(qemu_name);
+build_driver_cmd_line(node);
+node->u.driver.constructor = function;
+}
+
void qos_node_contains(const char *container, const char *contained,
QOSGraphEdgeOptions *opts, ...)
{
@@ -664,7 +674,7 @@ void qos_node_consumes(const char *consumer, const char
*interface,
add_edge(interface, consumer, QEDGE_CONSUMED_BY, opts);
}
-void qos_graph_node_set_availability(const char *node, bool av)
+static void qos_graph_node_set_availability_explicit(const char *node, bool av)
{
QOSGraphEdgeList *elist;
QOSGraphNode *n = search_node(node);
@@ -679,9 +689,46 @@ void qos_graph_node_set_availability(const char *node,
bool av)
}
QSLIST_FOREACH_SAFE(e, elist, edge_list, next) {
if (e->type == QEDGE_CONTAINS || e->type == QEDGE_PRODUCES) {
-qos_graph_node_set_availability(e->dest, av);
+qos_graph_node_set_availability_explicit(e->dest, av);
+}
+}
+}
+
+/*
+ * Behaves as qos_graph_node_set_availability_explicit(), except that the
+ * former always matches by node name only, whereas this function matches both
+ * by node name and node's optional 'qemu_name' field.
+ */
+void qos_graph_node_set_availability(const char *node, bool av)
+{
+GList *l;
+QOSGraphEdgeList *elist;
+QOSGraphEdge *e, *next;
+QOSGraphNode *n;
+GList *keys = g_hash_table_get_keys(node_table);
+
+for (l = keys; l != NULL; l = l->next) {
+const gchar *key = l->data;
+n = g_hash_table_lookup(node_table, key);
+/*
+ * node's 'qemu_name' is set if there is more than one device with
+ * the same QEMU (QMP) device name
+ */
+const char *node_name = n->qemu_name ?: n->name;
+if (g_strcmp0(node_name, node) == 0) {
+n->available = av;
+elist = get_edgelist(n->name);
+if (elist) {
+QSLIST_FOREACH_SAFE(e, elist, edge_list, next) {
+if (e->type == QEDGE_CONTAINS || e->type == QEDGE_PRODUCES)
+{
+qos_graph_node_set_availability_explicit(e->dest, av);
+}
+}
+}
}
}
+g_list_free(keys);
}
void qos_graph_foreach_test_path(QOSTestCallback fn)
diff --git a/tests/qtest/libqos/qgraph.h b/tests/qtest/libqos/qgraph.h
index 5f63d352ca..f472949f68 100644
--- a/tests/qtest/libqos/qgraph.h
+++ b/tests/qtest/libqos/qgraph.h
@@ -452,6 +452,22 @@ void qos_node_create_machine_args(const char *name,
*/
void qos_node_create_driver(const char *name, QOSCreateDriverFunc function);
+/**
+ * Behaves as qos_node_create_driver() with the extension of allowing to
+ * specify a different node name vs. associated QEMU device name.
+ *
+ * Use this function instead of qos_node_create_driver() if you need to create
+ * several instances of the same QEMU device. You
[PATCH v4 01/12] libqos/qgraph: add qemu_name to QOSGraphNode
Add new member variable 'qemu_name' to struct QOSGraphNode.
This new member may be optionally set in case a different
name for the node (which must always be a unique name) vs.
its actually associated QEMU (QMP) device name is required.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/libqos/qgraph.c | 1 +
tests/qtest/libqos/qgraph_internal.h | 1 +
2 files changed, 2 insertions(+)
diff --git a/tests/qtest/libqos/qgraph.c b/tests/qtest/libqos/qgraph.c
index fc49cfa879..e42f3eaafa 100644
--- a/tests/qtest/libqos/qgraph.c
+++ b/tests/qtest/libqos/qgraph.c
@@ -153,6 +153,7 @@ static QOSGraphNode *create_node(const char *name,
QOSNodeType type)
static void destroy_node(void *val)
{
QOSGraphNode *node = val;
+g_free(node->qemu_name);
g_free(node->command_line);
g_free(node);
}
diff --git a/tests/qtest/libqos/qgraph_internal.h
b/tests/qtest/libqos/qgraph_internal.h
index 968fa69450..974985dce9 100644
--- a/tests/qtest/libqos/qgraph_internal.h
+++ b/tests/qtest/libqos/qgraph_internal.h
@@ -56,6 +56,7 @@ struct QOSGraphNode {
bool available; /* set by QEMU via QMP, used during graph walk */
bool visited; /* used during graph walk */
char *name; /* used to identify the node */
+char *qemu_name;/* optional: see qos_node_create_driver_named() */
char *command_line; /* used to start QEMU at test execution */
union {
struct {
--
2.20.1
