[Spice-devel] [PATCH spice-server] test-display-base: Avoid spurious errors due to listen failures

[Frediano Ziglio]

> 
> On Mon, 2018-10-29 at 11:58 +0000, Frediano Ziglio wrote:
> > To set up a listening socket usually you call in sequence:
> > - socket;
> > - bind;
> > - listen.
> > Usually if you try to listen to a post already in listening state
> > the bind will fail.
> 
> I find this sentence confusing... If you try to *listen*, the *bind*
> will fail? Is that what you meant?
> 
> Also: "listen to a *post*"? What do you mean by post?
> 

ehm... typo, was port, which make more sense.
If you want to listen to a specific port you need to create the
socket, bind to the port you want and call "listen" function.

Any advice on rephrasing (beside fixing the typo) ?

> > However is possible that the listen will fail,
> > like in this sequence:
> > - socket 1;
> > - bind 1;
> > - socket 2;
> > - bind 2;
> > - listen 1;
> > - listen 2 <-- failure.
> 
> I don't understand why this sequence should fail. Can you explain a bit
> more?
> 

Because the port is already used (bind 1 and bind 2 binded the same port
on 2 sockets).

> > Our tests may attempt to reuse the port however we handle this
> > problem
> > trying to detect bind errors. Catch also listen errors to avoid the
> > sequence above to trigger the problem. This happened some commit ago
> > in our CI.
> 
> I have to admit that I still don't fully understand the point of this
> change. It seems that you're implying that if there is an error where
> we fail to listen, that's OK and the test shouldn't fail? Why is this
> error expected/OK? I think the commit log needs far more clarity here.
> What's the expected behavior? What's the actual behavior? How is it
> reproducible? etc.
> 

The code is expected to fix race conditions during testing so if it
happens that you try to listen to a port already in listening state
you just try another port. This can happen easily running multiple
tests in parallel.

> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/tests/test-display-base.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/server/tests/test-display-base.c b/server/tests/test-
> > display-base.c
> > index ea3a23ba..f58f76d3 100644
> > --- a/server/tests/test-display-base.c
> > +++ b/server/tests/test-display-base.c
> > @@ -894,10 +894,10 @@ 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)
> > +static gboolean ignore_in_use_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;
> > @@ -905,7 +905,8 @@ static gboolean ignore_bind_failures(const gchar
> > *log_domain,
> >      if ((log_level & G_LOG_LEVEL_WARNING) == 0)  {
> >          return true;
> >      }
> > -    if (strstr(message, "reds_init_socket: binding socket to ") ==
> > NULL) {
> > +    if (strstr(message, "reds_init_socket: binding socket to ") ==
> > NULL || // bind failure
> > +        strstr(message, "reds_init_socket: listen: ") == NULL) { //
> > listen failure
> >          g_print("XXX [%s]\n", message);
> >          return true;
> >      }
> 
> I have to say that I was not aware of this code before. mMatching a
> substring from a logged warning and ignoring that warning when it
> matches seems awfully hacky and fragile...
> 

Well, any better suggestion? Keep in mind that's testing code, not
going to run by customer (so no production level). A more clean (maybe)
mock of system function could be harder to maintain.

> 
> > @@ -929,7 +930,7 @@ Test* test_new(SpiceCoreInterface* core)
> >      // some common initialization for all display tests
> >      port = BASE_PORT;
> >  
> > -    g_test_log_set_fatal_handler(ignore_bind_failures, NULL);
> > +    g_test_log_set_fatal_handler(ignore_in_use_failures, NULL);
> >      for (port = BASE_PORT; port < BASE_PORT + 10; port++) {
> >          SpiceServer* server = spice_server_new();
> >          spice_server_set_noauth(server);
> 
> 

Frediano