>
> Currently, the port used by most tests is hardcoded to 5912. However,
> the test suite can be run in parallel, so if 2 tests run in parallel,
> the 2nd one is not going to be able to bind to port 5912 and will fail.
>
> After this commit, test_new() will try to find a free port between 5912
> and 5922 and will abort if it can't find any.
>
> The issue can be reproduced by adding a usleep(1000000) to the beginning
> of test_destroy().
>
make sense.
Considering that Qemu/libvirt scan forward, why not scanning backward?
Have you considering somebody could try to connect to a test server?
I think will use the line printed with the port by the way.
> Signed-off-by: Christophe Fergeau <cfergeau@xxxxxxxxxx>
> ---
> server/tests/test-display-base.c | 48
> ++++++++++++++++++++++++++++++++--------
> server/tests/test-display-base.h | 1 -
> server/tests/test-two-servers.c | 2 +-
> 3 files changed, 40 insertions(+), 11 deletions(-)
>
> diff --git a/server/tests/test-display-base.c
> b/server/tests/test-display-base.c
> index 289aa9840..76c3de744 100644
> --- a/server/tests/test-display-base.c
> +++ b/server/tests/test-display-base.c
> @@ -32,6 +32,7 @@
> #include <spice/qxl_dev.h>
>
> #include "test-display-base.h"
> +#include "test-glib-compat.h"
> #include "red-channel.h"
>
> #ifndef PATH_MAX
> @@ -899,11 +900,30 @@ void test_set_command_list(Test *test, Command
> *commands, int num_commands)
> test->num_commands = num_commands;
> }
>
> +static gboolean ignore_bind_failures(const gchar *log_domain,
> + GLogLevelFlags log_level,
> + const gchar *message,
> + gpointer user_data)
> +{
> + if (!g_str_equal (log_domain, G_LOG_DOMAIN)) {
> + return true;
> + }
> + if ((log_level & G_LOG_LEVEL_WARNING) == 0) {
> + return true;
> + }
> + if (strstr(message, "reds_init_socket: binding socket to ") == NULL) {
> + g_print("XXX [%s]\n", message);
> + return true;
> + }
>
> -Test* test_new_with_port(SpiceCoreInterface* core, int port)
> + return false;
> +}
> +
> +Test* test_new(SpiceCoreInterface* core)
> {
> Test *test = spice_new0(Test, 1);
> SpiceServer* server = spice_server_new();
> + int port = -1;
>
> test->qxl_instance.base.sif = &display_sif.base;
> test->qxl_instance.id = 0;
> @@ -913,10 +933,25 @@ Test* test_new_with_port(SpiceCoreInterface* core, int
> port)
> test->wakeup_ms = 1;
> test->cursor_notify = NOTIFY_CURSOR_BATCH;
> // some common initialization for all display tests
> - printf("TESTER: listening on port %d (unsecure)\n", port);
> - spice_server_set_port(server, port);
> +#define BASE_PORT 5912
weird to be defined in the middle
> + port = BASE_PORT;
> spice_server_set_noauth(server);
> - spice_server_init(server, core);
> +
> + g_test_log_set_fatal_handler(ignore_bind_failures, NULL);
Can't you set back (to NULL ?) at the end of the loop?
> + while (port < BASE_PORT + 10) {
I would see a for here, but not an issue.
> + spice_server_set_port(server, port);
> + if (spice_server_init(server, core) == 0) {
Looking at the source code it does not seem we ever though
about calling spice_server_init multiple times, for instance
main_dispatcher field is replaced every time at least causing
leaks (not surely a regression of this patch).
> + break;
> + }
> + port++;
> + }
> +
> + if (port >= BASE_PORT + 10) {
> + g_assert_not_reached();
> + return NULL;
> + }
> +
> + printf("TESTER: listening on port %d (unsecure)\n", port);
>
> cursor_init();
> path_init(&path, 0, angle_parts);
> @@ -925,11 +960,6 @@ Test* test_new_with_port(SpiceCoreInterface* core, int
> port)
> return test;
> }
>
> -Test *test_new(SpiceCoreInterface *core)
> -{
> - return test_new_with_port(core, 5912);
> -}
> -
> void test_destroy(Test *test)
> {
> spice_server_destroy(test->server);
> diff --git a/server/tests/test-display-base.h
> b/server/tests/test-display-base.h
> index a80f03e78..1a4f20c5b 100644
> --- a/server/tests/test-display-base.h
> +++ b/server/tests/test-display-base.h
> @@ -134,7 +134,6 @@ void test_set_simple_command_list(Test *test, const int
> *command, int num_comman
> void test_set_command_list(Test *test, Command *command, int num_commands);
> void test_add_display_interface(Test *test);
> void test_add_agent_interface(SpiceServer *server); // TODO - Test *test
> -Test* test_new_with_port(SpiceCoreInterface* core, int port);
> Test* test_new(SpiceCoreInterface* core);
> void test_destroy(Test *test);
>
> diff --git a/server/tests/test-two-servers.c
> b/server/tests/test-two-servers.c
> index 92935528e..40a0e5717 100644
> --- a/server/tests/test-two-servers.c
> +++ b/server/tests/test-two-servers.c
> @@ -42,7 +42,7 @@ int main(void)
>
> core = basic_event_loop_init();
> t1 = test_new(core);
> - t2 = test_new_with_port(core, 5913);
> + t2 = test_new(core);
> //spice_server_set_image_compression(server,
> SPICE_IMAGE_COMPRESSION_OFF);
> test_add_display_interface(t1);
> test_add_display_interface(t2);
The idea is good.
Beside the multiple spice_server_init call the patch looks good
(all other comments are really minor).
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel