[v1,6/6] tests/acceptance: pick a random gdb port for reverse debugging

Currently the test randomly fails if you are using a shared machine
due to contention on the well known port 1234. We can ameliorate this
a bit by picking a random non-ephemeral port although it doesn't
totally avoid the problem. While we could use a totally unique socket
address for debugging it's impossible to probe for gdb support of the
feature which makes this a sub-optimal but less fiddly option.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/acceptance/reverse_debugging.py | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

On 21/10/2020 18.31, Alex Bennée wrote:
> Currently the test randomly fails if you are using a shared machine
> due to contention on the well known port 1234. We can ameliorate this
> a bit by picking a random non-ephemeral port although it doesn't
> totally avoid the problem. While we could use a totally unique socket
> address for debugging it's impossible to probe for gdb support of the
> feature which makes this a sub-optimal but less fiddly option.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  tests/acceptance/reverse_debugging.py | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

Certainly better than before!

Reviewed-by: Thomas Huth <thuth@redhat.com>

On 21.10.2020 19:31, Alex Bennée wrote:
> Currently the test randomly fails if you are using a shared machine
> due to contention on the well known port 1234. We can ameliorate this
> a bit by picking a random non-ephemeral port although it doesn't
> totally avoid the problem. While we could use a totally unique socket
> address for debugging it's impossible to probe for gdb support of the
> feature which makes this a sub-optimal but less fiddly option.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>

> ---
>   tests/acceptance/reverse_debugging.py | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/acceptance/reverse_debugging.py b/tests/acceptance/reverse_debugging.py
> index b72fdf6cdc..f2e8245471 100644
> --- a/tests/acceptance/reverse_debugging.py
> +++ b/tests/acceptance/reverse_debugging.py
> @@ -16,6 +16,7 @@ from avocado.utils import gdb
>   from avocado.utils import process
>   from avocado.utils.path import find_command
>   from boot_linux_console import LinuxKernelTest
> +from random import randrange
>   
>   class ReverseDebugging(LinuxKernelTest):
>       """
> @@ -43,7 +44,8 @@ class ReverseDebugging(LinuxKernelTest):
>           else:
>               logger.info('replaying the execution...')
>               mode = 'replay'
> -            vm.add_args('-s', '-S')
> +            self.port = randrange(2048, 49152)
> +            vm.add_args('-gdb', 'tcp::%d' % (self.port), '-S')
>           vm.add_args('-icount', 'shift=%s,rr=%s,rrfile=%s,rrsnapshot=init' %
>                       (shift, mode, replay_path),
>                       '-net', 'none')
> @@ -122,7 +124,7 @@ class ReverseDebugging(LinuxKernelTest):
>           # replay and run debug commands
>           vm = self.run_vm(False, shift, args, replay_path, image_path)
>           logger.info('connecting to gdbstub')
> -        g = gdb.GDBRemote('127.0.0.1', 1234, False, False)
> +        g = gdb.GDBRemote('127.0.0.1', self.port, False, False)
>           g.connect()
>           r = g.cmd(b'qSupported')
>           if b'qXfer:features:read+' in r:
>

On 10/22/20 7:20 AM, Thomas Huth wrote:
> On 21/10/2020 18.31, Alex Bennée wrote:
>> Currently the test randomly fails if you are using a shared machine
>> due to contention on the well known port 1234. We can ameliorate this
>> a bit by picking a random non-ephemeral port although it doesn't
>> totally avoid the problem. While we could use a totally unique socket
>> address for debugging it's impossible to probe for gdb support of the
>> feature which makes this a sub-optimal but less fiddly option.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>   tests/acceptance/reverse_debugging.py | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> Certainly better than before!

I'd prefer another chardev that tcp, but as you said this is
already an improvement, so:

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
>

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> On 10/22/20 7:20 AM, Thomas Huth wrote:
>> On 21/10/2020 18.31, Alex Bennée wrote:
>>> Currently the test randomly fails if you are using a shared machine
>>> due to contention on the well known port 1234. We can ameliorate this
>>> a bit by picking a random non-ephemeral port although it doesn't
>>> totally avoid the problem. While we could use a totally unique socket
>>> address for debugging it's impossible to probe for gdb support of the
>>> feature which makes this a sub-optimal but less fiddly option.
>>>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> ---
>>>   tests/acceptance/reverse_debugging.py | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>> 
>> Certainly better than before!
>
> I'd prefer another chardev that tcp, but as you said this is
> already an improvement, so:

We've supported sockets gdb and softmmu emulation for some time:

  -gdb unix:path=gdb.sock,server

the trouble is detecting if the host installed gdb is going to connect
before we try and fail after launching the VM. I think we might get away
with a version probe:

  > luispm: I want to know ahead of time for my scripts if gdb can do a
      "target remote gdb.sock"
  <luispm> ajb-linaro, I don't think so. My guess is that GDB will
      always attempt to stablish a connection if the socket is valid.
  <luispm> ajb-linaro, Can you check the validity of the file before
      invoking GDB?
  <luispm> ajb-linaro, No concept of "is this particular remote target
      available?".
  > luispm: it's not that - I know the socket will exist but the older
      gdb just bombs out trying to read it.
  <luispm> ajb-linaro, Not good.
  <luispm> ajb-linaro, So is this a matter of an older GDB that doesn't
      support using socket files and a newer one that does?
  > luispm: I thought I might probe "help target remote" text but it's
      unchanged between versions
  > luispm: yes
  <luispm> ajb-linaro, I think the code is probably hidden within the
      "target remote" implementation.
  > luispm: and most distro gdb's don't at the moment
  > luispm: if I could work out the version it was added that might help
  <luispm> ajb-linaro, I see some bits of it were reverted at some
      point.
  <luispm> ajb-linaro, Let me check.
  <luispm> ajb-linaro, It looks like GDB 8.3 was the first stable to get
      it.
  > luispm: thanks - I'll see if I can script that up
  <luispm> ajb-linaro, Looks like they initially went with an explicit
      prefix of "unix:" before the socket. But then dropped that in
      favor of autodetecting the socket file.
  <luispm> ajb-linaro, The only thing I get for a GDB that doesn't
      support socket connections if
      "/run/user/1000/at-spi2-QTZBS0/socket: No such device or address."
  <luispm> *is*
  <luispm> This is 8.1 in Ubuntu 18.04.
  <luispm> master GDB says "Remote communication error.  Target
      disconnected.: Connection reset by peer."

>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
>> 
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>

On Wed, Oct 21, 2020 at 05:31:36PM +0100, Alex Bennée wrote:
> Currently the test randomly fails if you are using a shared machine
> due to contention on the well known port 1234. We can ameliorate this
> a bit by picking a random non-ephemeral port although it doesn't
> totally avoid the problem. While we could use a totally unique socket
> address for debugging it's impossible to probe for gdb support of the
> feature which makes this a sub-optimal but less fiddly option.
>

Hi Alex,

This is already a clear improvement, so consider my points as suggestions.

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  tests/acceptance/reverse_debugging.py | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/acceptance/reverse_debugging.py b/tests/acceptance/reverse_debugging.py
> index b72fdf6cdc..f2e8245471 100644
> --- a/tests/acceptance/reverse_debugging.py
> +++ b/tests/acceptance/reverse_debugging.py
> @@ -16,6 +16,7 @@ from avocado.utils import gdb
>  from avocado.utils import process
>  from avocado.utils.path import find_command
>  from boot_linux_console import LinuxKernelTest
> +from random import randrange

Avocado ships with a "avocado.utils.network.ports.find_free_port" utility:

   https://avocado-framework.readthedocs.io/en/81.0/api/utils/avocado.utils.network.html#avocado.utils.network.ports.find_free_port

Which *minimizes* the possibility of a clash by checking if the port
is available.  I think it's worth to consider using it.

>  
>  class ReverseDebugging(LinuxKernelTest):
>      """
> @@ -43,7 +44,8 @@ class ReverseDebugging(LinuxKernelTest):
>          else:
>              logger.info('replaying the execution...')
>              mode = 'replay'
> -            vm.add_args('-s', '-S')
> +            self.port = randrange(2048, 49152)
> +            vm.add_args('-gdb', 'tcp::%d' % (self.port), '-S')

It's a good idea to try to avoid setting instance attributes outside
of __init__().  In this specific case, I'd just add a "port" parameter
to run_vm().

>          vm.add_args('-icount', 'shift=%s,rr=%s,rrfile=%s,rrsnapshot=init' %
>                      (shift, mode, replay_path),
>                      '-net', 'none')
> @@ -122,7 +124,7 @@ class ReverseDebugging(LinuxKernelTest):
>          # replay and run debug commands
>          vm = self.run_vm(False, shift, args, replay_path, image_path)
>          logger.info('connecting to gdbstub')
> -        g = gdb.GDBRemote('127.0.0.1', 1234, False, False)
> +        g = gdb.GDBRemote('127.0.0.1', self.port, False, False)
>          g.connect()
>          r = g.cmd(b'qSupported')
>          if b'qXfer:features:read+' in r:
> -- 
> 2.20.1
> 


So, the overall diff of my suggestions look like:

---
diff --git a/tests/acceptance/reverse_debugging.py b/tests/acceptance/reverse_debugging.py
index f2e8245471..3d91dfaa8f 100644
--- a/tests/acceptance/reverse_debugging.py
+++ b/tests/acceptance/reverse_debugging.py
@@ -14,9 +14,10 @@ from avocado import skipIf
 from avocado_qemu import BUILD_DIR
 from avocado.utils import gdb
 from avocado.utils import process
+from avocado.utils.network.ports import find_free_port
 from avocado.utils.path import find_command
 from boot_linux_console import LinuxKernelTest
-from random import randrange
+
 
 class ReverseDebugging(LinuxKernelTest):
     """
@@ -34,7 +35,7 @@ class ReverseDebugging(LinuxKernelTest):
     STEPS = 10
     endian_is_le = True
 
-    def run_vm(self, record, shift, args, replay_path, image_path):
+    def run_vm(self, record, shift, args, replay_path, image_path, port):
         logger = logging.getLogger('replay')
         vm = self.get_vm()
         vm.set_console()
@@ -44,8 +45,7 @@ class ReverseDebugging(LinuxKernelTest):
         else:
             logger.info('replaying the execution...')
             mode = 'replay'
-            self.port = randrange(2048, 49152)
-            vm.add_args('-gdb', 'tcp::%d' % (self.port), '-S')
+            vm.add_args('-gdb', 'tcp::%d' % port, '-S')
         vm.add_args('-icount', 'shift=%s,rr=%s,rrfile=%s,rrsnapshot=init' %
                     (shift, mode, replay_path),
                     '-net', 'none')
@@ -111,9 +111,10 @@ class ReverseDebugging(LinuxKernelTest):
         process.run(cmd)
 
         replay_path = os.path.join(self.workdir, 'replay.bin')
+        port = find_free_port()
 
         # record the log
-        vm = self.run_vm(True, shift, args, replay_path, image_path)
+        vm = self.run_vm(True, shift, args, replay_path, image_path, port)
         while self.vm_get_icount(vm) <= self.STEPS:
             pass
         last_icount = self.vm_get_icount(vm)
@@ -122,9 +123,9 @@ class ReverseDebugging(LinuxKernelTest):
         logger.info("recorded log with %s+ steps" % last_icount)
 
         # replay and run debug commands
-        vm = self.run_vm(False, shift, args, replay_path, image_path)
+        vm = self.run_vm(False, shift, args, replay_path, image_path, port)
         logger.info('connecting to gdbstub')
-        g = gdb.GDBRemote('127.0.0.1', self.port, False, False)
+        g = gdb.GDBRemote('127.0.0.1', port, False, False)
         g.connect()
         r = g.cmd(b'qSupported')
         if b'qXfer:features:read+' in r:
---

Regards,
- Cleber.