[PATCH v4 0/2] oeqa: Change the serial runner

[PATCH v4 0/2] oeqa: Change the serial runner

[Louis Rannou]

The actual serial runner has a different usage compare to the ssh runner. The
return status is different and failure are not raised as exceptions.

Initially, I wanted to create a new run_serial_socket and modify the old
run_serial to use the former. And there was a second patch that changed every
call to run_serial to run_serial_socket. But that is not easy as each test can
have a different manner to handle the exception.

Therefore, the second patch only change dummy calls to the serial runner.

Louis Rannou (2):
  oeqa/utils/qemurunner: change the serial runner
  oeqa/selftest: change deprecated usage of run_serial

 meta/lib/oeqa/selftest/cases/debuginfod.py |  3 +-
 meta/lib/oeqa/selftest/cases/gdbserver.py  |  7 +--
 meta/lib/oeqa/selftest/cases/locales.py    |  8 +--
 meta/lib/oeqa/selftest/cases/package.py    |  7 +--
 meta/lib/oeqa/selftest/cases/runqemu.py    |  2 +-
 meta/lib/oeqa/targetcontrol.py             |  3 ++
 meta/lib/oeqa/utils/qemurunner.py          | 57 ++++++++++++++++++++++
 7 files changed, 75 insertions(+), 12 deletions(-)

-- 
2.40.0

[PATCH v4 1/2] oeqa/utils/qemurunner: change the serial runner

[Louis Rannou]

[YOCTO #15021]

Create a new runner run_serial_socket which usage matches the traditional ssh
runner. Its return status is 0 when the command succeeded or 0 when it
failed. If an error is encountered, it raises an Exception.

The previous serial runner is maintained and marked as deprecated.

Signed-off-by: Louis Rannou <lrannou@baylibre.com>
---
 meta/lib/oeqa/targetcontrol.py    |  3 ++
 meta/lib/oeqa/utils/qemurunner.py | 57 +++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+)

diff --git a/meta/lib/oeqa/targetcontrol.py b/meta/lib/oeqa/targetcontrol.py
index d686fe07ec..3707883da7 100644
--- a/meta/lib/oeqa/targetcontrol.py
+++ b/meta/lib/oeqa/targetcontrol.py
@@ -206,6 +206,9 @@ class QemuTarget(BaseTarget):
     def run_serial(self, command, timeout=60):
         return self.runner.run_serial(command, timeout=timeout)
 
+    def run_serial_socket(self, command, timeout=60):
+        return self.runner.run_serial_socket(command, timeout=timeout)
+
 
 class SimpleRemoteTarget(BaseTarget):
 
diff --git a/meta/lib/oeqa/utils/qemurunner.py b/meta/lib/oeqa/utils/qemurunner.py
index 6734cee48d..7e1c7a7eac 100644
--- a/meta/lib/oeqa/utils/qemurunner.py
+++ b/meta/lib/oeqa/utils/qemurunner.py
@@ -637,6 +637,7 @@ class QemuRunner:
                 return self.qmp.cmd(command)
 
     def run_serial(self, command, raw=False, timeout=60):
+        # Deprecated
         # Returns (status, output) where status is 1 on success and 0 on error
 
         # We assume target system have echo to get command status
@@ -688,6 +689,62 @@ class QemuRunner:
                     status = 1
         return (status, str(data))
 
+    def run_serial_socket(self, command, raw=False, timeout=60):
+        # Returns (status, output) where status is 0 on success and a negative value on error.
+
+        # We assume target system have echo to get command status
+        if not raw:
+            command = "%s; echo $?\n" % command
+
+        data = ''
+        status = 0
+        self.server_socket.sendall(command.encode('utf-8'))
+        start = time.time()
+        end = start + timeout
+        while True:
+            now = time.time()
+            if now >= end:
+                data += "<<< run_serial_socket(): command timed out after %d seconds without output >>>\r\n\r\n" % timeout
+                break
+            try:
+                sread, _, _ = select.select([self.server_socket],[],[], end - now)
+            except InterruptedError:
+                continue
+            if sread:
+                # try to avoid reading single character at a time
+                time.sleep(0.1)
+                answer = self.server_socket.recv(1024)
+                if answer:
+                    data += answer.decode('utf-8')
+                    # Search the prompt to stop
+                    if re.search(self.boot_patterns['search_cmd_finished'], data):
+                        break
+                else:
+                    if self.canexit:
+                        return (1, "")
+                    raise Exception("No data on serial console socket, connection closed?")
+
+        if not data:
+            raise Exception('serial run failed: no data')
+
+        if raw:
+            status = 0
+        else:
+            # Remove first line (command line) and last line (prompt)
+            data = data[data.find('$?\r\n')+4:data.rfind('\r\n')]
+            index = data.rfind('\r\n')
+            if index == -1:
+                data = ""
+                raise Exception('serial run failed: no result')
+            else:
+                status_cmd = data[index+2:]
+                data = data[:index]
+                try:
+                    status = int(status_cmd)
+                except ValueError as e:
+                    raise Exception('Could not convert to integer: {}'.format(str(e)))
+        return (status, str(data))
+
 
     def _dump_host(self):
         self.host_dumper.create_dir("qemu")
-- 
2.40.0

Re: [OE-core] [PATCH v4 1/2] oeqa/utils/qemurunner: change the serial runner

[Luca Ceresoli]

Hello Louis,

On Tue, 11 Apr 2023 17:05:02 +0200
"Louis Rannou" <lrannou@baylibre.com> wrote:

> [YOCTO #15021]
> 
> Create a new runner run_serial_socket which usage matches the traditional ssh

Nit: I'm not a native English speaker, but I think "whose usage" is the
correct form.

> runner. Its return status is 0 when the command succeeded or 0 when it
> failed. If an error is encountered, it raises an Exception.

Should be "or a negative error when it fails"?

I'm taking the patch for testing with these two changes, let me know if
I should do otherwise.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [OE-core] [PATCH v4 1/2] oeqa/utils/qemurunner: change the serial runner

[Louis Rannou]

Hello Luca,

You are correct, thanks !

Louis

On 12/04/2023 09:57, Luca Ceresoli wrote:
> Hello Louis,
> 
> On Tue, 11 Apr 2023 17:05:02 +0200
> "Louis Rannou" <lrannou@baylibre.com> wrote:
> 
>> [YOCTO #15021]
>>
>> Create a new runner run_serial_socket which usage matches the traditional ssh
> 
> Nit: I'm not a native English speaker, but I think "whose usage" is the
> correct form.
> 
>> runner. Its return status is 0 when the command succeeded or 0 when it
>> failed. If an error is encountered, it raises an Exception.
> 
> Should be "or a negative error when it fails"?
> 
> I'm taking the patch for testing with these two changes, let me know if
> I should do otherwise.
> 
> Luca
> 

Re: [OE-core] [PATCH v4 1/2] oeqa/utils/qemurunner: change the serial runner

[Ross Burton]

On 11 Apr 2023, at 16:05, Louis Rannou via lists.openembedded.org <lrannou=baylibre.com@lists.openembedded.org> wrote:
> Create a new runner run_serial_socket which usage matches the traditional ssh
> runner. Its return status is 0 when the command succeeded or 0 when it
> failed. If an error is encountered, it raises an Exception.
> 
> The previous serial runner is maintained and marked as deprecated.

I absolutely love this because the existing run_serial has the most confusing behaviour. However, we’re now duplicating a large chunk of code: can the old function be implemented such that it calls the new function and adapts the return values?

Ross

Re: [OE-core] [PATCH v4 1/2] oeqa/utils/qemurunner: change the serial runner

[Ross Burton]

> On 5 May 2023, at 11:31, Ross Burton <ross.burton@arm.com> wrote:
> 
> On 11 Apr 2023, at 16:05, Louis Rannou via lists.openembedded.org <lrannou=baylibre.com@lists.openembedded.org> wrote:
>> Create a new runner run_serial_socket which usage matches the traditional ssh
>> runner. Its return status is 0 when the command succeeded or 0 when it
>> failed. If an error is encountered, it raises an Exception.
>> 
>> The previous serial runner is maintained and marked as deprecated.
> 
> I absolutely love this because the existing run_serial has the most confusing behaviour. However, we’re now duplicating a large chunk of code: can the old function be implemented such that it calls the new function and adapts the return values?

Also, for bonus points, make the old function actual spit out a DeprecationWarning with warnings.warn.

Ross

Re: [OE-core] [PATCH v4 1/2] oeqa/utils/qemurunner: change the serial runner

[Richard Purdie]

On Fri, 2023-05-05 at 10:32 +0000, Ross Burton wrote:
> On 11 Apr 2023, at 16:05, Louis Rannou via lists.openembedded.org <lrannou=baylibre.com@lists.openembedded.org> wrote:
> > Create a new runner run_serial_socket which usage matches the traditional ssh
> > runner. Its return status is 0 when the command succeeded or 0 when it
> > failed. If an error is encountered, it raises an Exception.
> > 
> > The previous serial runner is maintained and marked as deprecated.
> 
> I absolutely love this because the existing run_serial has the most
> confusing behaviour. However, we’re now duplicating a large chunk of
> code: can the old function be implemented such that it calls the new
> function and adapts the return values?

This has hovered in the patch queue for that reason. I also love the
idea of it, it just doesn't feel quite ready.

I've been hoping we could convert the existing users and be able to
drop the old function but I'm not managed to see how much work is left
for that.

Cheers,

Richard

Re: [OE-core] [PATCH v4 1/2] oeqa/utils/qemurunner: change the serial runner

[Louis Rannou]

On 05/05/2023 12:50, Richard Purdie wrote:
> On Fri, 2023-05-05 at 10:32 +0000, Ross Burton wrote:
>> On 11 Apr 2023, at 16:05, Louis Rannou via lists.openembedded.org <lrannou=baylibre.com@lists.openembedded.org> wrote:
>>> Create a new runner run_serial_socket which usage matches the traditional ssh
>>> runner. Its return status is 0 when the command succeeded or 0 when it
>>> failed. If an error is encountered, it raises an Exception.
>>>
>>> The previous serial runner is maintained and marked as deprecated.
>>
>> I absolutely love this because the existing run_serial has the most
>> confusing behaviour. However, we’re now duplicating a large chunk of
>> code: can the old function be implemented such that it calls the new
>> function and adapts the return values?
> 
> This has hovered in the patch queue for that reason. I also love the
> idea of it, it just doesn't feel quite ready.
> 
> I've been hoping we could convert the existing users and be able to
> drop the old function but I'm not managed to see how much work is left
> for that.

My initial idea was indeed to change the former run_serial to use the 
new run_serial_socket. The problem is that run_serial_socket can raise 
exceptions which are not handled by run_serial. It's not possible to 
catch them except if we create a very specific exception to this subject.

About the second question: how much work is left. serial_runner remains in:
- selftest/cases/wic.py
- selftest/cases/imagefeatures.py
- selftest/cases/overlayfs.py
- selftest/cases/runtime_test.py
- utils/dump.py

It seemed to me that fixing those is nos as easy. Further investigation 
is possible, but it would be much more comfortable for me to see the 
first stage validated. I would say this is a pretty good checkpoint for 
saving this work while I try to fix what can be.

Thanks,
Louis

[PATCH v4 2/2] oeqa/selftest: change deprecated usage of run_serial

[Louis Rannou]

Prefer the new function run_serial_socket. Change calls where the modification
is easy.

Signed-off-by: Louis Rannou <lrannou@baylibre.com>
---
 meta/lib/oeqa/selftest/cases/debuginfod.py | 3 ++-
 meta/lib/oeqa/selftest/cases/gdbserver.py  | 7 ++++---
 meta/lib/oeqa/selftest/cases/locales.py    | 8 ++++----
 meta/lib/oeqa/selftest/cases/package.py    | 7 ++++---
 meta/lib/oeqa/selftest/cases/runqemu.py    | 2 +-
 5 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/meta/lib/oeqa/selftest/cases/debuginfod.py b/meta/lib/oeqa/selftest/cases/debuginfod.py
index 37f51760fb..2571ce6989 100644
--- a/meta/lib/oeqa/selftest/cases/debuginfod.py
+++ b/meta/lib/oeqa/selftest/cases/debuginfod.py
@@ -99,7 +99,8 @@ CORE_IMAGE_EXTRA_INSTALL += "elfutils"
                     % (qemu.server_ip, port)
                 )
                 self.logger.info(f"Starting client {cmd}")
-                status, output = qemu.run_serial(cmd)
+                status, output = qemu.run_serial_socket(cmd)
+                self.assertEqual(0, status)
                 # This should be more comprehensive
                 self.assertIn("/.cache/debuginfod_client/", output)
         finally:
diff --git a/meta/lib/oeqa/selftest/cases/gdbserver.py b/meta/lib/oeqa/selftest/cases/gdbserver.py
index 9da97ae780..4f8c7d620f 100644
--- a/meta/lib/oeqa/selftest/cases/gdbserver.py
+++ b/meta/lib/oeqa/selftest/cases/gdbserver.py
@@ -43,7 +43,8 @@ CORE_IMAGE_EXTRA_INSTALL = "gdbserver"
             shutil.unpack_archive(filename, debugfs)
 
             with runqemu("core-image-minimal", runqemuparams="nographic") as qemu:
-                status, output = qemu.run_serial("kmod --help")
+                status, output = qemu.run_serial_socket("kmod --help")
+                self.assertEqual(status, 0)
                 self.assertIn("modprobe", output)
 
                 with concurrent.futures.ThreadPoolExecutor(max_workers=1) as executor:
@@ -61,7 +62,7 @@ CORE_IMAGE_EXTRA_INSTALL = "gdbserver"
                             self.fail("Timed out connecting to gdb")
                     future = executor.submit(run_gdb)
 
-                    status, output = qemu.run_serial("gdbserver --once :9999 kmod --help")
-                    self.assertEqual(status, 1)
+                    status, output = qemu.run_serial_socket("gdbserver --once :9999 kmod --help")
+                    self.assertEqual(status, 0)
                     # The future either returns None, or raises an exception
                     future.result()
diff --git a/meta/lib/oeqa/selftest/cases/locales.py b/meta/lib/oeqa/selftest/cases/locales.py
index 4ca8ffb7aa..98026d2c80 100644
--- a/meta/lib/oeqa/selftest/cases/locales.py
+++ b/meta/lib/oeqa/selftest/cases/locales.py
@@ -27,15 +27,15 @@ class LocalesTest(OESelftestTestCase):
 
         with runqemu("core-image-minimal", ssh=False, runqemuparams='nographic') as qemu:
             cmd = "locale -a"
-            status, output = qemu.run_serial(cmd)
+            status, output = qemu.run_serial_socket(cmd)
             # output must includes fr_FR or fr_FR.UTF-8
-            self.assertEqual(status, 1, msg='locale test command failed: output: %s' % output)
+            self.assertEqual(status, 0, msg='locale test command failed: output: %s' % output)
             self.assertIn("fr_FR", output, msg='locale -a test failed: output: %s' % output)
 
             cmd = "localedef --list-archive -v"
-            status, output = qemu.run_serial(cmd)
+            status, output = qemu.run_serial_socket(cmd)
             # output must includes fr_FR.utf8
-            self.assertEqual(status, 1, msg='localedef test command failed: output: %s' % output)
+            self.assertEqual(status, 0, msg='localedef test command failed: output: %s' % output)
             self.assertIn("fr_FR.utf8", output, msg='localedef test failed: output: %s' % output)
 
     def test_locales_on(self):
diff --git a/meta/lib/oeqa/selftest/cases/package.py b/meta/lib/oeqa/selftest/cases/package.py
index 1aa6c03f8a..78e8a4c26b 100644
--- a/meta/lib/oeqa/selftest/cases/package.py
+++ b/meta/lib/oeqa/selftest/cases/package.py
@@ -135,7 +135,8 @@ class PackageTests(OESelftestTestCase):
             Check that gdb ``binary`` to read symbols from separated debug file
             """
             self.logger.info("gdbtest %s" % binary)
-            status, output = qemu.run_serial('/usr/bin/gdb.sh %s' % binary, timeout=60)
+            status, output = qemu.run_serial_socket('/usr/bin/gdb.sh %s' % binary, timeout=60)
+            self.assertEqual(0, status)
             for l in output.split('\n'):
                 # Check debugging symbols exists
                 if '(no debugging symbols found)' in l:
@@ -166,8 +167,8 @@ class PackageTests(OESelftestTestCase):
 
         def check_ownership(qemu, expected_gid, expected_uid, path):
             self.logger.info("Check ownership of %s", path)
-            status, output = qemu.run_serial('stat -c "%U %G" ' + path)
-            self.assertEqual(status, 1, "stat failed: " + output)
+            status, output = qemu.run_serial_socket('stat -c "%U %G" ' + path)
+            self.assertEqual(status, 0, "stat failed: " + output)
             try:
                 uid, gid = output.split()
                 self.assertEqual(uid, expected_uid)
diff --git a/meta/lib/oeqa/selftest/cases/runqemu.py b/meta/lib/oeqa/selftest/cases/runqemu.py
index f01e1eec66..be1481a82e 100644
--- a/meta/lib/oeqa/selftest/cases/runqemu.py
+++ b/meta/lib/oeqa/selftest/cases/runqemu.py
@@ -183,7 +183,7 @@ class QemuTest(OESelftestTestCase):
         # (such as the exception "Console connection closed unexpectedly")
         # as qemu will disappear when we shut it down
         qemu.runner.allowexit()
-        qemu.run_serial("shutdown -h now")
+        qemu.run_serial_socket("shutdown -h now")
         time_track = 0
         try:
             while True:
-- 
2.40.0