diff mbox series

[8/9] vhost-user-test: small changes to init_hugepagefs

Message ID 1550165756-21617-9-git-send-email-pbonzini@redhat.com
State New
Headers show
Series vhost: enable for all targets | expand

Commit Message

Paolo Bonzini Feb. 14, 2019, 5:35 p.m. UTC
After the conversion to qgraph, the equivalent of "main" will be in
a constructor and will run even if the tests are not being requested.
Therefore, it should not assert that init_hugepagefs succeeds and will
be called when creating the TestServer.  This patch changes the prototype
of init_hugepagefs, this way the next patch looks nicer.

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-Id: <1543851204-41186-14-git-send-email-pbonzini@redhat.com>
---
 tests/vhost-user-test.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

Comments

Thomas Huth Feb. 15, 2019, 8:27 a.m. UTC | #1
On 14/02/2019 18.35, Paolo Bonzini wrote:
> After the conversion to qgraph, the equivalent of "main" will be in
> a constructor and will run even if the tests are not being requested.
> Therefore, it should not assert that init_hugepagefs succeeds and will
> be called when creating the TestServer.  This patch changes the prototype
> of init_hugepagefs, this way the next patch looks nicer.
> 
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Message-Id: <1543851204-41186-14-git-send-email-pbonzini@redhat.com>
> ---
>  tests/vhost-user-test.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
> index 33030e0..516e31c 100644
> --- a/tests/vhost-user-test.c
> +++ b/tests/vhost-user-test.c
> @@ -465,14 +465,20 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
>      g_mutex_unlock(&s->data_mutex);
>  }
>  
> -#ifdef CONFIG_LINUX
> -static const char *init_hugepagefs(const char *path)
> +static const char *init_hugepagefs(void)
>  {
> +#ifdef CONFIG_LINUX
> +    const char *path = getenv("QTEST_HUGETLBFS_PATH");
>      struct statfs fs;
>      int ret;
>  
> +    if (!path) {
> +        return NULL;
> +    }
> +
>      if (access(path, R_OK | W_OK | X_OK)) {
>          g_test_message("access on path (%s): %s\n", path, strerror(errno));
> +        abort();
>          return NULL;
>      }

I think I'd rather replace the whole if-statement with something like
this instead:

    g_assert_cmpint(access(path, R_OK | W_OK | X_OK), ==, 0);

> @@ -482,17 +488,21 @@ static const char *init_hugepagefs(const char *path)
>  
>      if (ret != 0) {
>          g_test_message("statfs on path (%s): %s\n", path, strerror(errno));
> +        abort();
>          return NULL;
>      }

Maybe simplify to:

    TFR(statfs_err = statfs(path, &fs));
    g_assert_cmpint(statfs_err, ==, 0);

>      if (fs.f_type != HUGETLBFS_MAGIC) {
>          g_test_message("Warning: path not on HugeTLBFS: %s\n", path);
> +        abort();
>          return NULL;
>      }

    g_assert_cmpint(fs.f_type, ==, HUGETLBFS_MAGIC);

?

>      return path;
> -}
> +#else
> +    return NULL;
>  #endif
> +}
>  
>  static TestServer *test_server_new(const gchar *name)
>  {
> @@ -986,7 +996,6 @@ static void test_multiqueue(void)
>  
>  int main(int argc, char **argv)
>  {
> -    const char *hugefs;
>      int ret;
>      char template[] = "/tmp/vhost-test-XXXXXX";
>  
> @@ -1001,14 +1010,7 @@ int main(int argc, char **argv)
>      }
>      g_assert(tmpfs);
>  
> -    root = tmpfs;
> -#ifdef CONFIG_LINUX
> -    hugefs = getenv("QTEST_HUGETLBFS_PATH");
> -    if (hugefs) {
> -        root = init_hugepagefs(hugefs);
> -        g_assert(root);
> -    }
> -#endif
> +    root = init_hugepagefs() ? : tmpfs;

I'd prefer the Elvis operator without space inbetween.

>      if (qemu_memfd_check(0)) {
>          qtest_add_data_func("/vhost-user/read-guest-mem/memfd",

Anyway, just nits, so still:

Reviewed-by: Thomas Huth <thuth@redhat.com>
diff mbox series

Patch

diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 33030e0..516e31c 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -465,14 +465,20 @@  static void chr_read(void *opaque, const uint8_t *buf, int size)
     g_mutex_unlock(&s->data_mutex);
 }
 
-#ifdef CONFIG_LINUX
-static const char *init_hugepagefs(const char *path)
+static const char *init_hugepagefs(void)
 {
+#ifdef CONFIG_LINUX
+    const char *path = getenv("QTEST_HUGETLBFS_PATH");
     struct statfs fs;
     int ret;
 
+    if (!path) {
+        return NULL;
+    }
+
     if (access(path, R_OK | W_OK | X_OK)) {
         g_test_message("access on path (%s): %s\n", path, strerror(errno));
+        abort();
         return NULL;
     }
 
@@ -482,17 +488,21 @@  static const char *init_hugepagefs(const char *path)
 
     if (ret != 0) {
         g_test_message("statfs on path (%s): %s\n", path, strerror(errno));
+        abort();
         return NULL;
     }
 
     if (fs.f_type != HUGETLBFS_MAGIC) {
         g_test_message("Warning: path not on HugeTLBFS: %s\n", path);
+        abort();
         return NULL;
     }
 
     return path;
-}
+#else
+    return NULL;
 #endif
+}
 
 static TestServer *test_server_new(const gchar *name)
 {
@@ -986,7 +996,6 @@  static void test_multiqueue(void)
 
 int main(int argc, char **argv)
 {
-    const char *hugefs;
     int ret;
     char template[] = "/tmp/vhost-test-XXXXXX";
 
@@ -1001,14 +1010,7 @@  int main(int argc, char **argv)
     }
     g_assert(tmpfs);
 
-    root = tmpfs;
-#ifdef CONFIG_LINUX
-    hugefs = getenv("QTEST_HUGETLBFS_PATH");
-    if (hugefs) {
-        root = init_hugepagefs(hugefs);
-        g_assert(root);
-    }
-#endif
+    root = init_hugepagefs() ? : tmpfs;
 
     if (qemu_memfd_check(0)) {
         qtest_add_data_func("/vhost-user/read-guest-mem/memfd",