[PATCH 1/4] Tests.PacketAssert: fix stderr handling

From: Ondrej Lichtner

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 
---
 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

[PATCH 2/4] Tests.PacketAssert: reformat with Black

From: Ondrej Lichtner

Signed-off-by: Ondrej Lichtner 
---
 lnst/Tests/PacketAssert.py | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/lnst/Tests/PacketAssert.py b/lnst/Tests/PacketAssert.py
index 75396101..5fd7abfd 100644
--- a/lnst/Tests/PacketAssert.py
+++ b/lnst/Tests/PacketAssert.py
@@ -1,15 +1,22 @@
 import re
 import logging
 import subprocess
-from lnst.Common.Parameters import StrParam, ListParam, DeviceParam, IntParam, BoolParam
+from lnst.Common.Parameters import (
+    StrParam,
+    ListParam,
+    DeviceParam,
+    IntParam,
+    BoolParam,
+)
 from lnst.Devices.Device import Device
 from lnst.Common.Utils import is_installed
 from lnst.Tests.BaseTestModule import BaseTestModule
 from lnst.Common.LnstError import LnstError

+
 class PacketAssert(BaseTestModule):
     interface = DeviceParam(mandatory=True)
-    p_filter = StrParam(default='')
+    p_filter = StrParam(default="")
     grep_for = ListParam(default=[])
     promiscuous = BoolParam(default=False)
     _grep_exprs = []
@@ -26,7 +33,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 -i %s "%s"' % (iface, filt)

return cmd

@@ -48,8 +55,13 @@ def run(self):
         cmd = self._compose_cmd()
         logging.debug("compiled command: {}".format(cmd))

-        packet_assert_process = subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE,
-                                        stderr=subprocess.PIPE, close_fds=True)
+        packet_assert_process = subprocess.Popen(
+            cmd,
+            shell=True,
+            stdout=subprocess.PIPE,
+            stderr=subprocess.PIPE,
+            close_fds=True,
+        )

try:
             self.wait_for_interrupt()

[PATCH 3/4] Devices.VxlanDevice: add init check for mutlicast vxlan

From: Ondrej Lichtner

When the group paramter is defined for a vxlan based connection the
realdev parameter is also required otherwise the kernel returns a
configuration error.

Signed-off-by: Ondrej Lichtner 
---
 lnst/Devices/VxlanDevice.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lnst/Devices/VxlanDevice.py b/lnst/Devices/VxlanDevice.py
index 183d3e16..f5e0e50f 100644
--- a/lnst/Devices/VxlanDevice.py
+++ b/lnst/Devices/VxlanDevice.py
@@ -31,6 +31,9 @@ def __init__(self, ifmanager, *args, **kwargs):
         if "group" not in kwargs and "remote" not in kwargs:
             raise DeviceError("One of group or remote must be specified for vxlan")

+        if "group" in kwargs and "realdev" not in kwargs:
+            raise DeviceError("'group' requires realdev to be specified")
+
         if kwargs.get("remote", False) and ipaddress(kwargs["remote"]).is_multicast:
             logging.debug("ATTENTION: non-unicast remote IP set: %s" % str(kwargs["remote"]))

[PATCH 4/4] ENRT.VxlanMulticastRecipe: actually configure a multicast vxlan

From: Ondrej Lichtner

Resolves an old TODO item of using an actual multicast address for the
Vxlan configuration.

This also resolves issues with the ping tests from and to the guest
machine.

Signed-off-by: Ondrej Lichtner 
---
 lnst/Recipes/ENRT/VxlanMulticastRecipe.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lnst/Recipes/ENRT/VxlanMulticastRecipe.py b/lnst/Recipes/ENRT/VxlanMulticastRecipe.py
index 8e9e5a98..5654cf42 100644
--- a/lnst/Recipes/ENRT/VxlanMulticastRecipe.py
+++ b/lnst/Recipes/ENRT/VxlanMulticastRecipe.py
@@ -28,15 +28,15 @@ def test_wide_configuration(self):
         net_addr = "192.168.0"
         vxlan_net_addr = "192.168.100"
         vxlan_net_addr6 = "fc00:0:0:0"
-        #TODO: Enable usage of a proper address (like 239.1.1.1)
-        vxlan_group_ip = "192.168.0.3"
+        vxlan_group_ip = "239.1.1.1"

host1.br0 = BridgeDevice()
         host1.br0.slave_add(host1.eth0)
         host1.br0.slave_add(host1.tap0)

-        for machine in [host1, guest1, host2]:
-            machine.vxlan0 = VxlanDevice(vxlan_id=1, group=vxlan_group_ip)
+        host1.vxlan0 = VxlanDevice(vxlan_id=1, realdev=host1.br0, group=vxlan_group_ip)
+        for machine in [guest1, host2]:
+            machine.vxlan0 = VxlanDevice(vxlan_id=1, realdev=machine.eth0, group=vxlan_group_ip)

configuration = super().test_wide_configuration()
         configuration.test_wide_devices = [host1.br0, host1.vxlan0,

[Jozef Urbanovsky]

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  wrote:
> From: Ondrej Lichtner
>
> 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
>
> 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 -- [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

[Ondrej Lichtner]

On Fri, Mar 05, 2021 at 11:05:27AM +0100, Jozef Urbanovsky wrote:
> 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?

I think it's important to differentiate wha tyou mean by evaluate
here... This is purely about the success of the test module starting and
finishing properly for the purposes that it's supposed to do.

In my opinion the packetassert test module primary use case is to run
tcpdump with a specific filter and then count how many packets fit
additional patterns. If this is successful - tcpdump runs fine, regexes
are checked fine etc... then the test module has succeeded.

You can then as a tester do additional evaluation on what was counted
etc...

This is similar to the flow measurement test modules - iperf says PASS
if the iperf process finished without any errors, did what it was asked
to do without any errors.

I think the whole premise of searching stderr logs for "real" vs "debug"
outputs was kind of flawed as tcpdump seems to follow normal unix
conventions of returning a non zero value in case there's an actual
error.

I may be misunderstanding this though, do you know of a case when
tcpdump returns a nonzero value when no error appeared? Or returns a
zero when an error is reported?

-Ondrej
> Jozef.
>
> On Fri, Mar 5, 2021 at 10:55 AM  wrote:
> > From: Ondrej Lichtner
> >
> > 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
> >
> > 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 -- [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

[Jozef Urbanovsky]

I understand what you mean by the "successful" execution of tcpdump and it
makes sense.
The only case that could be problematic is incorrect recipe configuration,
where packets won't be generated or received, as it is still successful
execution.
All other cases should return a non-zero return value, if any error appears.

I have no other comments, patchset looks good.

Jozef.

On Fri, Mar 5, 2021 at 11:17 AM Ondrej Lichtner  wrote:
> On Fri, Mar 05, 2021 at 11:05:27AM +0100, Jozef Urbanovsky wrote:
> > 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?
>
> I think it's important to differentiate wha tyou mean by evaluate
> here... This is purely about the success of the test module starting and
> finishing properly for the purposes that it's supposed to do.
>
> In my opinion the packetassert test module primary use case is to run
> tcpdump with a specific filter and then count how many packets fit
> additional patterns. If this is successful - tcpdump runs fine, regexes
> are checked fine etc... then the test module has succeeded.
>
> You can then as a tester do additional evaluation on what was counted
> etc...
>
> This is similar to the flow measurement test modules - iperf says PASS
> if the iperf process finished without any errors, did what it was asked
> to do without any errors.
>
> I think the whole premise of searching stderr logs for "real" vs "debug"
> outputs was kind of flawed as tcpdump seems to follow normal unix
> conventions of returning a non zero value in case there's an actual
> error.
>
> I may be misunderstanding this though, do you know of a case when
> tcpdump returns a nonzero value when no error appeared? Or returns a
> zero when an error is reported?
>
> -Ondrej
> > Jozef.
> >
> > On Fri, Mar 5, 2021 at 10:55 AM  wrote:
> > > From: Ondrej Lichtner
> > >
> > > 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
> > >
> > > 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 -- [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

[Ondrej Lichtner]

On Fri, Mar 05, 2021 at 11:32:57AM +0100, Jozef Urbanovsky wrote:
> I understand what you mean by the "successful" execution of tcpdump and it
> makes sense.
> The only case that could be problematic is incorrect recipe configuration,
> where packets won't be generated or received, as it is still successful
> execution.
> All other cases should return a non-zero return value, if any error appears.

yeah but then this should be caught be the followup evaluation that
checks the actual measured numbers.

-Ondrej
> I have no other comments, patchset looks good.
>
> Jozef.
>
> On Fri, Mar 5, 2021 at 11:17 AM Ondrej Lichtner  wrote:
> > On Fri, Mar 05, 2021 at 11:05:27AM +0100, Jozef Urbanovsky wrote:
> > > 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?
> >
> > I think it's important to differentiate wha tyou mean by evaluate
> > here... This is purely about the success of the test module starting and
> > finishing properly for the purposes that it's supposed to do.
> >
> > In my opinion the packetassert test module primary use case is to run
> > tcpdump with a specific filter and then count how many packets fit
> > additional patterns. If this is successful - tcpdump runs fine, regexes
> > are checked fine etc... then the test module has succeeded.
> >
> > You can then as a tester do additional evaluation on what was counted
> > etc...
> >
> > This is similar to the flow measurement test modules - iperf says PASS
> > if the iperf process finished without any errors, did what it was asked
> > to do without any errors.
> >
> > I think the whole premise of searching stderr logs for "real" vs "debug"
> > outputs was kind of flawed as tcpdump seems to follow normal unix
> > conventions of returning a non zero value in case there's an actual
> > error.
> >
> > I may be misunderstanding this though, do you know of a case when
> > tcpdump returns a nonzero value when no error appeared? Or returns a
> > zero when an error is reported?
> >
> > -Ondrej
> > > Jozef.
> > >
> > > On Fri, Mar 5, 2021 at 10:55 AM  wrote:
> > > > From: Ondrej Lichtner
> > > >
> > > > 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
> > > >
> > > > 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 -- [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

[Jan Tluka]

Fri, Mar 05, 2021 at 10:50:32AM CET, [e-mail] wrote:
> From: Ondrej Lichtner
>
> 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
>
> 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 -- [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

Ack to series.

Acked-by: Jan Tluka

[Ondrej Lichtner]

thanks for the reviews, pushed upstream.

-Ondrej

On Fri, Mar 05, 2021 at 10:50:32AM +0100, [e-mail] wrote:
> From: Ondrej Lichtner
>
> 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
>
> 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