[PATCH] oeqa/qemurunner: do not use Popen.poll() when terminating runqemu with a signal

[PATCH] oeqa/qemurunner: do not use Popen.poll() when terminating runqemu with a signal

[Alexander Kanavin]

This does not actually guarantee that the child runqemu process has completely exited:
poll() may return prematurely while the SIGTERM handler in runqemu is still running.
This thwarts the rest of the processing, and may terminate the handler before
it completes.

Use Popen.communicate() instead: this is what python documentation recommends as well:
https://docs.python.org/3/library/subprocess.html#subprocess.Popen.communicate

Signed-off-by: Alexander Kanavin <alex@linutronix.de>
---
 meta/lib/oeqa/utils/qemurunner.py | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/meta/lib/oeqa/utils/qemurunner.py b/meta/lib/oeqa/utils/qemurunner.py
index b5fed6c9fe..8b893601d4 100644
--- a/meta/lib/oeqa/utils/qemurunner.py
+++ b/meta/lib/oeqa/utils/qemurunner.py
@@ -543,10 +543,13 @@ class QemuRunner:
                 except OSError as e:
                     if e.errno != errno.ESRCH:
                         raise
-            endtime = time.time() + self.runqemutime
-            while self.runqemu.poll() is None and time.time() < endtime:
-                time.sleep(1)
-            if self.runqemu.poll() is None:
+            try:
+                outs, errs = self.runqemu.communicate(timeout = self.runqemutime)
+                if outs:
+                    self.logger.info("Output from runqemu:\n%s", outs.decode("utf-8"))
+                if errs:
+                    self.logger.info("Stderr from runqemu:\n%s", errs.decode("utf-8"))
+            except TimeoutExpired:
                 self.logger.debug("Sending SIGKILL to runqemu")
                 os.killpg(os.getpgid(self.runqemu.pid), signal.SIGKILL)
             if not self.runqemu.stdout.closed:
-- 
2.30.2

Re: [OE-core] [PATCH] oeqa/qemurunner: do not use Popen.poll() when terminating runqemu with a signal

[Mikko Rapeli]

Hi,

On Mon, Jan 30, 2023 at 11:11:25PM +0100, Alexander Kanavin wrote:
> This does not actually guarantee that the child runqemu process has completely exited:
> poll() may return prematurely while the SIGTERM handler in runqemu is still running.
> This thwarts the rest of the processing, and may terminate the handler before
> it completes.
> 
> Use Popen.communicate() instead: this is what python documentation recommends as well:
> https://docs.python.org/3/library/subprocess.html#subprocess.Popen.communicate

Was I trying to solve the same problem in
https://lists.openembedded.org/g/openembedded-core/message/176203 ?

I think so.

Cheers,

-Mikko

> Signed-off-by: Alexander Kanavin <alex@linutronix.de>
> ---
>  meta/lib/oeqa/utils/qemurunner.py | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/meta/lib/oeqa/utils/qemurunner.py b/meta/lib/oeqa/utils/qemurunner.py
> index b5fed6c9fe..8b893601d4 100644
> --- a/meta/lib/oeqa/utils/qemurunner.py
> +++ b/meta/lib/oeqa/utils/qemurunner.py
> @@ -543,10 +543,13 @@ class QemuRunner:
>                  except OSError as e:
>                      if e.errno != errno.ESRCH:
>                          raise
> -            endtime = time.time() + self.runqemutime
> -            while self.runqemu.poll() is None and time.time() < endtime:
> -                time.sleep(1)
> -            if self.runqemu.poll() is None:
> +            try:
> +                outs, errs = self.runqemu.communicate(timeout = self.runqemutime)
> +                if outs:
> +                    self.logger.info("Output from runqemu:\n%s", outs.decode("utf-8"))
> +                if errs:
> +                    self.logger.info("Stderr from runqemu:\n%s", errs.decode("utf-8"))
> +            except TimeoutExpired:
>                  self.logger.debug("Sending SIGKILL to runqemu")
>                  os.killpg(os.getpgid(self.runqemu.pid), signal.SIGKILL)
>              if not self.runqemu.stdout.closed:
> -- 
> 2.30.2
> 

> 
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#176502): https://lists.openembedded.org/g/openembedded-core/message/176502
> Mute This Topic: https://lists.openembedded.org/mt/96639889/7159507
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [mikko.rapeli@linaro.org]
> -=-=-=-=-=-=-=-=-=-=-=-
> 

Re: [OE-core] [PATCH] oeqa/qemurunner: do not use Popen.poll() when terminating runqemu with a signal

[Alexander Kanavin]

On Tue, 31 Jan 2023 at 08:38, Mikko Rapeli <mikko.rapeli@linaro.org> wrote:
> On Mon, Jan 30, 2023 at 11:11:25PM +0100, Alexander Kanavin wrote:
> > This does not actually guarantee that the child runqemu process has completely exited:
> > poll() may return prematurely while the SIGTERM handler in runqemu is still running.
> > This thwarts the rest of the processing, and may terminate the handler before
> > it completes.
> >
> > Use Popen.communicate() instead: this is what python documentation recommends as well:
> > https://docs.python.org/3/library/subprocess.html#subprocess.Popen.communicate
>
> Was I trying to solve the same problem in
> https://lists.openembedded.org/g/openembedded-core/message/176203 ?
>
> I think so.

No, this is different. Your patch treats qemu process itself, mine
fixes the problem with runqemu script. The whole qemurunner.py is a
mess, for example it sends SIGTERM to both qemu and runqemu, where one
or the other should be sufficient (runqemu will shutdown qemu if it is
itself terminating, and will terminate if it detects that qemu
finished). But I left that for another time. I only want a clean,
reliable shutdown of runqemu.

Alex