Is it enough to evaluate packet assert only based on the tcpdump return code or is it just a quick fix so we can get it working?

Jozef.


On Fri, Mar 5, 2021 at 10:55 AM <olichtne@redhat.com> wrote:
From: Ondrej Lichtner <olichtne@redhat.com>

Tcpdump always prints information to stderr, the "ignore_exprs" regex
detection of "real" errors vs debug information is insufficient due to
this potentially changing at any point from tcpdump. An example of that
is that on RHEL8 we now also see the following line:

        dropped privs to tcpdump

Adjusted the PacketAssert module to only report PASS/FAIL based on the
return code of tcpdump (if not 0 then there's some issue). Text in
stderr should also always be debug logged, just in case it could be
useful later.

I also adjusted how the process is "finished" after an interrupt is
received so that the returncode is filled in properly (after the
communicate call).

Signed-off-by: Ondrej Lichtner <olichtne@redhat.com>
---
 lnst/Tests/PacketAssert.py | 39 ++++++++++----------------------------
 1 file changed, 10 insertions(+), 29 deletions(-)

diff --git a/lnst/Tests/PacketAssert.py b/lnst/Tests/PacketAssert.py
index 25a92ba0..75396101 100644
--- a/lnst/Tests/PacketAssert.py
+++ b/lnst/Tests/PacketAssert.py
@@ -37,25 +37,6 @@ def _check_line(self, line):
                     return
             self._p_recv += 1

-    def _is_real_err(self, err):
-
-        ignore_exprs = [r"tcpdump: verbose output suppressed, use -v or -vv for full protocol decode",
-                        r"listening on %s, link-type .* \(.*\), capture size [0-9]* bytes" %
-                        self.params.interface.name, r"\d+ packets captured",
-                        r"\d+ packets received by filter", r"\d+ packets dropped by kernel"]
-
-        for line in err.split('\n'):
-            if not line:
-                continue
-            match = False
-            for expr in ignore_exprs:
-                if re.search(expr, line):
-                    match = True
-                    break
-            if not match:
-                return True
-        return False
-
     def run(self):
         self._res_data = {}
         if not is_installed("tcpdump"):
@@ -75,17 +56,14 @@ def run(self):
         except:
             raise LnstError("Could not handle interrupt properly.")

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

         self._res_data["stderr"] = stderr
-
-        if self._is_real_err(stderr):
-            self._res_data["msg"] = "errors reported by tcpdump"
-            logging.error(self._res_data["msg"])
-            logging.error(self._res_data["stderr"])
-            return False
+        # tcpdump always reports information to stderr, there may be actual
+        # errors but also just generic debug information
+        logging.debug(self._res_data["stderr"])

         for line in stdout.split("\n"):
             self._check_line(line)
@@ -93,4 +71,7 @@ def run(self):
         logging.debug("Capturing finised. Received %d packets." % self._p_recv)
         self._res_data["p_recv"] = self._p_recv

-        return True
+        if packet_assert_process.returncode != 0:
+            return False
+        else:
+            return True
--
2.30.1
_______________________________________________
LNST-developers mailing list -- lnst-developers@lists.fedorahosted.org
To unsubscribe send an email to lnst-developers-leave@lists.fedorahosted.org
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/lnst-developers@lists.fedorahosted.org
Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure