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 <olichtne@redhat.com> 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 <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
> >