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

[Cleber Rosa <crosa@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5401 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]