Re: [Spice-devel] [spice-server v2] tests: Automatically determine free port to use

[Frediano Ziglio]

> 
> On Thu, Sep 21, 2017 at 11:11:03AM -0400, Frediano Ziglio wrote:
> > > 
> > > 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(100) to the beginning
> > > of test_destroy().
> > > 
> > > Signed-off-by: Christophe Fergeau 
> > > ---
> > > Changes since v1:
> > > - don't leak memory calling spice_init() multiple times on the same
> > >   instance
> > > - move BASE_PORT to a better place
> > > - use for loop rather than while()
> > > - remove  'ignore_bind_failure' fatal log handler when no longer needed
> > > 
> > > 
> > >  server/tests/test-display-base.c | 54
> > >  
> > >  server/tests/test-display-base.h |  1 -
> > >  server/tests/test-two-servers.c  |  2 +-
> > >  3 files changed, 44 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/server/tests/test-display-base.c
> > > b/server/tests/test-display-base.c
> > > index 289aa9840..ab6fc3b36 100644
> > > --- a/server/tests/test-display-base.c
> > > +++ b/server/tests/test-display-base.c
> > > @@ -32,6 +32,7 @@
> > >  #include 
> > >  
> > >  #include "test-display-base.h"
> > > +#include "test-glib-compat.h"
> > >  #include "red-channel.h"
> > >  
> > >  #ifndef PATH_MAX
> > > @@ -899,24 +900,60 @@ 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;
> > > +}
> > > +
> > > +#define BASE_PORT 5912
> > > +
> > > +Test* test_new(SpiceCoreInterface* core)
> > >  {
> > >  Test *test = spice_new0(Test, 1);
> > > -SpiceServer* server = spice_server_new();
> > > +int port = -1;
> > >  
> > >  test->qxl_instance.base.sif = _sif.base;
> > >  test->qxl_instance.id = 0;
> > >  
> > >  test->core = core;
> > > -test->server = server;
> > >  test->wakeup_ms = 1;
> > >  test->cursor_notify = NOTIFY_CURSOR_BATCH;
> > >  // some common initialization for all display tests
> > > +port = BASE_PORT;
> > > +
> > > +g_test_log_set_fatal_handler(ignore_bind_failures, NULL);
> > > +for (port = BASE_PORT; port < BASE_PORT + 10; port++) {
> > > +SpiceServer* server = spice_server_new();
> > > +spice_server_set_noauth(server);
> > > +spice_server_set_port(server, port);
> > > +if (spice_server_init(server, core) == 0) {
> > > +test->server = server;
> > > +break;
> > > +}
> > > +spice_server_destroy(server);
> > > +}
> > > +
> > > +if (port >= BASE_PORT + 10) {
> > > +g_assert_not_reached();
> > > +return NULL;
> > > +}
> > > +
> > 
> > maybe a
> > 
> >g_assert(test->server);
> > 
> > so you can easily increase the range changing just the for line?
> 
> Yup, works better.
> 
> Christophe
> 

g_assert_nonnull ? Not that is going to change much, on failure maybe
a better error.

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-server v2] tests: Automatically determine free port to use

[Christophe Fergeau]

On Thu, Sep 21, 2017 at 11:11:03AM -0400, Frediano Ziglio wrote:
> > 
> > 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(100) to the beginning
> > of test_destroy().
> > 
> > Signed-off-by: Christophe Fergeau 
> > ---
> > Changes since v1:
> > - don't leak memory calling spice_init() multiple times on the same
> >   instance
> > - move BASE_PORT to a better place
> > - use for loop rather than while()
> > - remove  'ignore_bind_failure' fatal log handler when no longer needed
> > 
> > 
> >  server/tests/test-display-base.c | 54
> >  
> >  server/tests/test-display-base.h |  1 -
> >  server/tests/test-two-servers.c  |  2 +-
> >  3 files changed, 44 insertions(+), 13 deletions(-)
> > 
> > diff --git a/server/tests/test-display-base.c
> > b/server/tests/test-display-base.c
> > index 289aa9840..ab6fc3b36 100644
> > --- a/server/tests/test-display-base.c
> > +++ b/server/tests/test-display-base.c
> > @@ -32,6 +32,7 @@
> >  #include 
> >  
> >  #include "test-display-base.h"
> > +#include "test-glib-compat.h"
> >  #include "red-channel.h"
> >  
> >  #ifndef PATH_MAX
> > @@ -899,24 +900,60 @@ 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;
> > +}
> > +
> > +#define BASE_PORT 5912
> > +
> > +Test* test_new(SpiceCoreInterface* core)
> >  {
> >  Test *test = spice_new0(Test, 1);
> > -SpiceServer* server = spice_server_new();
> > +int port = -1;
> >  
> >  test->qxl_instance.base.sif = _sif.base;
> >  test->qxl_instance.id = 0;
> >  
> >  test->core = core;
> > -test->server = server;
> >  test->wakeup_ms = 1;
> >  test->cursor_notify = NOTIFY_CURSOR_BATCH;
> >  // some common initialization for all display tests
> > +port = BASE_PORT;
> > +
> > +g_test_log_set_fatal_handler(ignore_bind_failures, NULL);
> > +for (port = BASE_PORT; port < BASE_PORT + 10; port++) {
> > +SpiceServer* server = spice_server_new();
> > +spice_server_set_noauth(server);
> > +spice_server_set_port(server, port);
> > +if (spice_server_init(server, core) == 0) {
> > +test->server = server;
> > +break;
> > +}
> > +spice_server_destroy(server);
> > +}
> > +
> > +if (port >= BASE_PORT + 10) {
> > +g_assert_not_reached();
> > +return NULL;
> > +}
> > +
> 
> maybe a
> 
>g_assert(test->server);
> 
> so you can easily increase the range changing just the for line?

Yup, works better.

Christophe
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel