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

[Christophe Fergeau]

On Thu, Sep 21, 2017 at 09:36:25AM -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(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?

There are 2 occurrences of 5912 in the existing .py scripts. Since these
scripts, or manual connection would not happen during automated test
runs, but would be done by hand, port 5912 would be used as before.
If something is using 5912 already, before the test would have failed,
now the test will not be listening on the expected port. I believe all
of this is acceptable, we can always tweak things later on to make the
port user configurable in the .py scripts for example.

> I think will use the line printed with the port by the way.
> 
> > Signed-off-by: Christophe Fergeau <cfergeau at redhat.com>
> > ---
> >  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

I'll move it, had to put the definition somewhere, and felt it was weird
too ;)
> 
> > +    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.

Ah sure, was a while (true) initially, did not think of changing it,
thanks!

> 
> > +        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).

Did not think either of testing with valgrind, will do!

Christophe