diff mbox series

[06/11] test-util-sockets: Test the complete abstract socket matrix

Message ID 20201029133833.3450220-7-armbru@redhat.com
State New
Headers show
Series sockets: Attempt to drain the abstract socket swamp | expand

Commit Message

Markus Armbruster Oct. 29, 2020, 1:38 p.m. UTC
The test covers only two out of nine combinations.  Test all nine.
Four turn out to be broken.  Marked /* BUG */.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-util-sockets.c | 86 ++++++++++++++++++++++++++++-----------
 1 file changed, 63 insertions(+), 23 deletions(-)

Comments

Eric Blake Oct. 29, 2020, 7:19 p.m. UTC | #1
On 10/29/20 8:38 AM, Markus Armbruster wrote:
> The test covers only two out of nine combinations.  Test all nine.

> Four turn out to be broken.  Marked /* BUG */.

> 

> Signed-off-by: Markus Armbruster <armbru@redhat.com>

> ---

>  tests/test-util-sockets.c | 86 ++++++++++++++++++++++++++++-----------

>  1 file changed, 63 insertions(+), 23 deletions(-)

> 


> -static void test_socket_unix_abstract_good(void)

> +static void test_socket_unix_abstract(void)

>  {

> -    SocketAddress addr;

> +    SocketAddress addr, addr_tight, addr_padded;

> +    abstract_socket_matrix_row matrix[ABSTRACT_SOCKET_VARIANTS] = {

> +        { &addr,

> +          { &addr_tight, &addr_padded, &addr },

> +          { false /* BUG */, true /* BUG */, true } },

> +        { &addr_tight,

> +          { &addr_padded, &addr, &addr_tight },

> +          { false, false /* BUG */, true } },

> +        { &addr_padded,

> +          { &addr, &addr_tight, &addr_padded },

> +          { true /* BUG */, false, true } }

> +    };

> +    int i;

>  

Reviewed-by: Eric Blake <eblake@redhat.com>


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org
Daniel P. Berrangé Oct. 30, 2020, 9:33 a.m. UTC | #2
On Thu, Oct 29, 2020 at 02:38:28PM +0100, Markus Armbruster wrote:
> The test covers only two out of nine combinations.  Test all nine.

> Four turn out to be broken.  Marked /* BUG */.

> 

> Signed-off-by: Markus Armbruster <armbru@redhat.com>

> ---

>  tests/test-util-sockets.c | 86 ++++++++++++++++++++++++++++-----------

>  1 file changed, 63 insertions(+), 23 deletions(-)

> 

> diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c

> index c2802f69ee..f8b6586e70 100644

> --- a/tests/test-util-sockets.c

> +++ b/tests/test-util-sockets.c

> @@ -230,59 +230,99 @@ static void test_socket_fd_pass_num_nocli(void)

>  #endif

>  

>  #ifdef __linux__

> +

> +#define ABSTRACT_SOCKET_VARIANTS 3

> +

> +typedef struct {

> +    SocketAddress *server, *client[ABSTRACT_SOCKET_VARIANTS];

> +    bool expect_connect[ABSTRACT_SOCKET_VARIANTS];

> +} abstract_socket_matrix_row;


snip

>  

> -static void test_socket_unix_abstract_good(void)

> +static void test_socket_unix_abstract(void)

>  {

> -    SocketAddress addr;

> +    SocketAddress addr, addr_tight, addr_padded;

> +    abstract_socket_matrix_row matrix[ABSTRACT_SOCKET_VARIANTS] = {

> +        { &addr,

> +          { &addr_tight, &addr_padded, &addr },

> +          { false /* BUG */, true /* BUG */, true } },

> +        { &addr_tight,

> +          { &addr_padded, &addr, &addr_tight },

> +          { false, false /* BUG */, true } },

> +        { &addr_padded,

> +          { &addr, &addr_tight, &addr_padded },

> +          { true /* BUG */, false, true } }

> +    };


This is overloading multiple separate tests in one, which is bad for
test result reporting.


> @@ -329,8 +369,8 @@ int main(int argc, char **argv)

>      }

>  

>  #ifdef __linux__

> -    g_test_add_func("/util/socket/unix-abstract/good",

> -                    test_socket_unix_abstract_good);

> +    g_test_add_func("/util/socket/unix-abstract",

> +                    test_socket_unix_abstract);


We should instead be registering multiple test funcs, passing in
param to say which scenario to test.  This ensures we still see
the test name reflecting which scenario is being run.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Markus Armbruster Oct. 30, 2020, 2:14 p.m. UTC | #3
Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Oct 29, 2020 at 02:38:28PM +0100, Markus Armbruster wrote:

>> The test covers only two out of nine combinations.  Test all nine.

>> Four turn out to be broken.  Marked /* BUG */.

>> 

>> Signed-off-by: Markus Armbruster <armbru@redhat.com>

>> ---

>>  tests/test-util-sockets.c | 86 ++++++++++++++++++++++++++++-----------

>>  1 file changed, 63 insertions(+), 23 deletions(-)

>> 

>> diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c

>> index c2802f69ee..f8b6586e70 100644

>> --- a/tests/test-util-sockets.c

>> +++ b/tests/test-util-sockets.c

>> @@ -230,59 +230,99 @@ static void test_socket_fd_pass_num_nocli(void)

>>  #endif

>>  

>>  #ifdef __linux__

>> +

>> +#define ABSTRACT_SOCKET_VARIANTS 3

>> +

>> +typedef struct {

>> +    SocketAddress *server, *client[ABSTRACT_SOCKET_VARIANTS];

>> +    bool expect_connect[ABSTRACT_SOCKET_VARIANTS];

>> +} abstract_socket_matrix_row;

>

> snip

>

>>  

>> -static void test_socket_unix_abstract_good(void)

>> +static void test_socket_unix_abstract(void)

>>  {

>> -    SocketAddress addr;

>> +    SocketAddress addr, addr_tight, addr_padded;

>> +    abstract_socket_matrix_row matrix[ABSTRACT_SOCKET_VARIANTS] = {

>> +        { &addr,

>> +          { &addr_tight, &addr_padded, &addr },

>> +          { false /* BUG */, true /* BUG */, true } },

>> +        { &addr_tight,

>> +          { &addr_padded, &addr, &addr_tight },

>> +          { false, false /* BUG */, true } },

>> +        { &addr_padded,

>> +          { &addr, &addr_tight, &addr_padded },

>> +          { true /* BUG */, false, true } }

>> +    };

>

> This is overloading multiple separate tests in one, which is bad for

> test result reporting.

>

>

>> @@ -329,8 +369,8 @@ int main(int argc, char **argv)

>>      }

>>  

>>  #ifdef __linux__

>> -    g_test_add_func("/util/socket/unix-abstract/good",

>> -                    test_socket_unix_abstract_good);

>> +    g_test_add_func("/util/socket/unix-abstract",

>> +                    test_socket_unix_abstract);

>

> We should instead be registering multiple test funcs, passing in

> param to say which scenario to test.  This ensures we still see

> the test name reflecting which scenario is being run.


There are hundreds of such test functions in tests/ just waiting for you
to create them ;)

Back to serious.  Before the patch, one test function tested two
scenarios.  The patch adds the missing seven.  Feel free to improve on
top.
diff mbox series

Patch

diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
index c2802f69ee..f8b6586e70 100644
--- a/tests/test-util-sockets.c
+++ b/tests/test-util-sockets.c
@@ -230,59 +230,99 @@  static void test_socket_fd_pass_num_nocli(void)
 #endif
 
 #ifdef __linux__
+
+#define ABSTRACT_SOCKET_VARIANTS 3
+
+typedef struct {
+    SocketAddress *server, *client[ABSTRACT_SOCKET_VARIANTS];
+    bool expect_connect[ABSTRACT_SOCKET_VARIANTS];
+} abstract_socket_matrix_row;
+
 static gpointer unix_client_thread_func(gpointer user_data)
 {
-    int fd;
+    abstract_socket_matrix_row *row = user_data;
+    Error *err = NULL;
+    int i, fd;
 
-    fd = socket_connect(user_data, &error_abort);
-    g_assert_cmpint(fd, >=, 0);
-    close(fd);
+    for (i = 0; i < ABSTRACT_SOCKET_VARIANTS; i++) {
+        if (row->expect_connect[i]) {
+            fd = socket_connect(row->client[i], &error_abort);
+            g_assert_cmpint(fd, >=, 0);
+        } else {
+            fd = socket_connect(row->client[i], &err);
+            g_assert_cmpint(fd, ==, -1);
+            error_free_or_abort(&err);
+        }
+        close(fd);
+    }
     return NULL;
 }
 
-static void test_socket_unix_abstract_one(SocketAddress *addr)
+static void test_socket_unix_abstract_row(abstract_socket_matrix_row *test)
 {
-    int fd, connfd;
+    int fd, connfd, i;
     GThread *cli;
     struct sockaddr_un un;
     socklen_t len = sizeof(un);
 
-    fd = socket_listen(addr, 1, &error_abort);
+    /* Last one must connect, or else accept() below hangs */
+    assert(test->expect_connect[ABSTRACT_SOCKET_VARIANTS - 1]);
+
+    fd = socket_listen(test->server, 1, &error_abort);
     g_assert_cmpint(fd, >=, 0);
     g_assert(fd_is_socket(fd));
 
     cli = g_thread_new("abstract_unix_client",
                        unix_client_thread_func,
-                       addr);
+                       test);
 
-    connfd = accept(fd, (struct sockaddr *)&un, &len);
-    g_assert_cmpint(connfd, !=, -1);
-    close(connfd);
+    for (i = 0; i < ABSTRACT_SOCKET_VARIANTS; i++) {
+        if (test->expect_connect[i]) {
+            connfd = accept(fd, (struct sockaddr *)&un, &len);
+            g_assert_cmpint(connfd, !=, -1);
+            close(connfd);
+        }
+    }
 
     close(fd);
-
     g_thread_join(cli);
 }
 
-static void test_socket_unix_abstract_good(void)
+static void test_socket_unix_abstract(void)
 {
-    SocketAddress addr;
+    SocketAddress addr, addr_tight, addr_padded;
+    abstract_socket_matrix_row matrix[ABSTRACT_SOCKET_VARIANTS] = {
+        { &addr,
+          { &addr_tight, &addr_padded, &addr },
+          { false /* BUG */, true /* BUG */, true } },
+        { &addr_tight,
+          { &addr_padded, &addr, &addr_tight },
+          { false, false /* BUG */, true } },
+        { &addr_padded,
+          { &addr, &addr_tight, &addr_padded },
+          { true /* BUG */, false, true } }
+    };
+    int i;
 
     addr.type = SOCKET_ADDRESS_TYPE_UNIX;
     addr.u.q_unix.path = g_strdup_printf("unix-%d-%u",
                                          getpid(), g_random_int());
     addr.u.q_unix.has_abstract = true;
     addr.u.q_unix.abstract = true;
-
-    /* non tight socklen serv and cli */
     addr.u.q_unix.has_tight = false;
     addr.u.q_unix.tight = false;
-    test_socket_unix_abstract_one(&addr);
 
-    /* tight socklen serv and cli */
-    addr.u.q_unix.has_tight = true;
-    addr.u.q_unix.tight = true;
-    test_socket_unix_abstract_one(&addr);
+    addr_tight = addr;
+    addr_tight.u.q_unix.has_tight = true;
+    addr_tight.u.q_unix.tight = true;
+
+    addr_padded = addr;
+    addr_padded.u.q_unix.has_tight = true;
+    addr_padded.u.q_unix.tight = false;
+
+    for (i = 0; i < ABSTRACT_SOCKET_VARIANTS; i++) {
+        test_socket_unix_abstract_row(&matrix[i]);
+    }
 
     g_free(addr.u.q_unix.path);
 }
@@ -329,8 +369,8 @@  int main(int argc, char **argv)
     }
 
 #ifdef __linux__
-    g_test_add_func("/util/socket/unix-abstract/good",
-                    test_socket_unix_abstract_good);
+    g_test_add_func("/util/socket/unix-abstract",
+                    test_socket_unix_abstract);
 #endif
 
 end: