[PATCH] Tests.PacketAssert: continuosly read tcpdump stdout

From: Ondrej Lichtner

When PacketAssert is used for longer or larger streams the buffer for
the pipe file used for stdout can be fully filled and tcpdump will get
stuck on the write syscall. After that when the SIGINT comes from the
recipe this syscall is interrupted and tcpdump quits with:

tcpdump: Unable to write output: Interrupted system call

The fix in this patch is in two parts:
* use the '-U' argument which makes the tcpdump output packet buffered -
  fflush is called after each packet is analyzeed and printed.
* rewrite the wait_for_interrupt use with custom SIGINT handling that
  continuosly reads lines from stdout or stderr so that the buffer is
  never full

Eventually similar functionality may be useful for other Test modules as
well, at that point this should be refactored.

Signed-off-by: Ondrej Lichtner 
---
 lnst/Tests/PacketAssert.py | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/lnst/Tests/PacketAssert.py b/lnst/Tests/PacketAssert.py
index 5fd7abfd..9b623b0b 100644
--- a/lnst/Tests/PacketAssert.py
+++ b/lnst/Tests/PacketAssert.py
@@ -1,6 +1,8 @@
 import re
 import logging
 import subprocess
+import signal
+from select import select
 from lnst.Common.Parameters import (
     StrParam,
     ListParam,
@@ -10,10 +12,13 @@
 )
 from lnst.Devices.Device import Device
 from lnst.Common.Utils import is_installed
-from lnst.Tests.BaseTestModule import BaseTestModule
+from lnst.Tests.BaseTestModule import BaseTestModule, InterruptException
 from lnst.Common.LnstError import LnstError

+def interrupt_handler(signum, frame):
+    raise InterruptException()
+
 class PacketAssert(BaseTestModule):
     interface = DeviceParam(mandatory=True)
     p_filter = StrParam(default="")
@@ -33,7 +38,7 @@ def _compose_cmd(self):
             cmd += " -p"
         iface = self.params.interface.name
         filt = self.params.p_filter
-        cmd += ' -nn -i %s "%s"' % (iface, filt)
+        cmd += ' -nn -U -i %s "%s"' % (iface, filt)

return cmd

@@ -63,12 +68,27 @@ def run(self):
             close_fds=True,
         )

+        stdout = bytearray()
+        stderr = bytearray()
         try:
-            self.wait_for_interrupt()
-        except:
-            raise LnstError("Could not handle interrupt properly.")
+            old_handler = signal.signal(signal.SIGINT, interrupt_handler)
+            # for longer, or larger streams tcpdump can fill the entire io
+            # buffer for the stdout/stderr files, because of that we need to
+            # read the files continuosly
+            while True:
+                rl, wl, xl = select([packet_assert_process.stdout, packet_assert_process.stderr], [], [])
+                if packet_assert_process.stdout in rl:
+                    stdout += packet_assert_process.stdout.readline()
+                if packet_assert_process.stderr in rl:
+                    stderr += packet_assert_process.stderr.readline()
+        except InterruptException:
+            pass
+        finally:
+            signal.signal(signal.SIGINT, old_handler)

-        stdout, stderr = packet_assert_process.communicate()
+        packet_assert_process.wait()
+        stdout += packet_assert_process.stdout.read()
+        stderr += packet_assert_process.stderr.read()
         stdout = stdout.decode()
         stderr = stderr.decode()

[Jan Tluka]

Tue, Mar 30, 2021 at 02:16:04PM CEST, [e-mail] wrote:
> From: Ondrej Lichtner
>
> When PacketAssert is used for longer or larger streams the buffer for
> the pipe file used for stdout can be fully filled and tcpdump will get
> stuck on the write syscall. After that when the SIGINT comes from the
> recipe this syscall is interrupted and tcpdump quits with:
>
> tcpdump: Unable to write output: Interrupted system call
>
> The fix in this patch is in two parts:
> use the '-U' argument which makes the tcpdump output packet buffered -
>
> fflush is called after each packet is analyzeed and printed.
> rewrite the wait_for_interrupt use with custom SIGINT handling that
>
> continuosly reads lines from stdout or stderr so that the buffer is
>  never full
>
> Eventually similar functionality may be useful for other Test modules as
> well, at that point this should be refactored.
> Signed-off-by: Ondrej Lichtner
>
> lnst/Tests/PacketAssert.py | 32 ++++++++++++++++++++++++++------
> 1 file changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/lnst/Tests/PacketAssert.py b/lnst/Tests/PacketAssert.py
> index 5fd7abfd..9b623b0b 100644
> --- a/lnst/Tests/PacketAssert.py
> +++ b/lnst/Tests/PacketAssert.py
> @@ -1,6 +1,8 @@
> import re
> import logging
> import subprocess
> +import signal
> +from select import select
> from lnst.Common.Parameters import (
>     StrParam,
>     ListParam,
> @@ -10,10 +12,13 @@
> )
> from lnst.Devices.Device import Device
> from lnst.Common.Utils import is_installed
> -from lnst.Tests.BaseTestModule import BaseTestModule
> +from lnst.Tests.BaseTestModule import BaseTestModule, InterruptException
> from lnst.Common.LnstError import LnstError
>
> +def interrupt_handler(signum, frame):
> raise InterruptException()
>
> class PacketAssert(BaseTestModule):
>     interface = DeviceParam(mandatory=True)
>     p_filter = StrParam(default="")
> @@ -33,7 +38,7 @@ def _compose_cmd(self):
>             cmd += " -p"
>         iface = self.params.interface.name
>         filt = self.params.p_filter
>    cmd += ' -nn -i %s "%s"' % (iface, filt)
>    cmd += ' -nn -U -i %s "%s"' % (iface, filt)
>
>   return cmd
>
> @@ -63,12 +68,27 @@ def run(self):
>             close_fds=True,
>         )
>    stdout = bytearray()
>    stderr = bytearray()
>   try:
>        self.wait_for_interrupt()
>    except:
>        raise LnstError("Could not handle interrupt properly.")
>        old_handler = signal.signal(signal.SIGINT, interrupt_handler)
>        # for longer, or larger streams tcpdump can fill the entire io
>        # buffer for the stdout/stderr files, because of that we need to
>        # read the files continuosly
>        while True:
>            rl, wl, xl = select([packet_assert_process.stdout, packet_assert_process.stderr], [], [])
>            if packet_assert_process.stdout in rl:
>                stdout += packet_assert_process.stdout.readline()
>            if packet_assert_process.stderr in rl:
>                stderr += packet_assert_process.stderr.readline()
>    except InterruptException:
>        pass
>    finally:
>        signal.signal(signal.SIGINT, old_handler)
>
>    stdout, stderr = packet_assert_process.communicate()
>    packet_assert_process.wait()
>    stdout += packet_assert_process.stdout.read()
>    stderr += packet_assert_process.stderr.read()
>   stdout = stdout.decode()
>   stderr = stderr.decode()
>
> -- 
> 2.31.0
> _______________________________________________
> LNST-developers mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]
> Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
> List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
> List Archives: https://lists.fedorahosted.org/archives/list/[e-mail]...
> Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure

Looks good.

Acked-by: Jan Tluka

[Ondrej Lichtner]

On Tue, Mar 30, 2021 at 02:16:04PM +0200, [e-mail] wrote:
> From: Ondrej Lichtner
>
> When PacketAssert is used for longer or larger streams the buffer for
> the pipe file used for stdout can be fully filled and tcpdump will get
> stuck on the write syscall. After that when the SIGINT comes from the
> recipe this syscall is interrupted and tcpdump quits with:
> tcpdump: Unable to write output: Interrupted system call
>
> The fix in this patch is in two parts:
> use the '-U' argument which makes the tcpdump output packet buffered -
> fflush is called after each packet is analyzeed and printed.
> rewrite the wait_for_interrupt use with custom SIGINT handling that
> continuosly reads lines from stdout or stderr so that the buffer is
> never full
>
> Eventually similar functionality may be useful for other Test modules as
> well, at that point this should be refactored.
> Signed-off-by: Ondrej Lichtner
>
> lnst/Tests/PacketAssert.py | 32 ++++++++++++++++++++++++++------
>  1 file changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/lnst/Tests/PacketAssert.py b/lnst/Tests/PacketAssert.py
> index 5fd7abfd..9b623b0b 100644
> --- a/lnst/Tests/PacketAssert.py
> +++ b/lnst/Tests/PacketAssert.py
> @@ -1,6 +1,8 @@
>  import re
>  import logging
>  import subprocess
> +import signal
> +from select import select
>  from lnst.Common.Parameters import (
>      StrParam,
>      ListParam,
> @@ -10,10 +12,13 @@
>  )
>  from lnst.Devices.Device import Device
>  from lnst.Common.Utils import is_installed
> -from lnst.Tests.BaseTestModule import BaseTestModule
> +from lnst.Tests.BaseTestModule import BaseTestModule, InterruptException
>  from lnst.Common.LnstError import LnstError
>
> +def interrupt_handler(signum, frame):
> raise InterruptException()
>
> class PacketAssert(BaseTestModule):
>      interface = DeviceParam(mandatory=True)
>      p_filter = StrParam(default="")
> @@ -33,7 +38,7 @@ def _compose_cmd(self):
>              cmd += " -p"
>          iface = self.params.interface.name
>          filt = self.params.p_filter
>    cmd += ' -nn -i %s "%s"' % (iface, filt)
>    cmd += ' -nn -U -i %s "%s"' % (iface, filt)
>
>    return cmd
>
> @@ -63,12 +68,27 @@ def run(self):
>              close_fds=True,
>          )
>    stdout = bytearray()
>    stderr = bytearray()
>    try:
>        self.wait_for_interrupt()
>    except:
>        raise LnstError("Could not handle interrupt properly.")
>        old_handler = signal.signal(signal.SIGINT, interrupt_handler)
>        # for longer, or larger streams tcpdump can fill the entire io
>        # buffer for the stdout/stderr files, because of that we need to
>        # read the files continuosly
>        while True:
>            rl, wl, xl = select([packet_assert_process.stdout, packet_assert_process.stderr], [], [])
>            if packet_assert_process.stdout in rl:
>                stdout += packet_assert_process.stdout.readline()
>            if packet_assert_process.stderr in rl:
>                stderr += packet_assert_process.stderr.readline()
>    except InterruptException:
>        pass
>    finally:
>        signal.signal(signal.SIGINT, old_handler)
>
>    stdout, stderr = packet_assert_process.communicate()
>    packet_assert_process.wait()
>    stdout += packet_assert_process.stdout.read()
>    stderr += packet_assert_process.stderr.read()
>    stdout = stdout.decode()
>    stderr = stderr.decode()
>
> -- 
> 2.31.0

pushed.

-Ondrej