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 <olichtne(a)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(a)redhat.com> wrote:
> >
> > > From: Ondrej Lichtner <olichtne(a)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(a)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(a)lists.fedorahosted.org
> > > To unsubscribe send an email to
> > > lnst-developers-leave(a)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.fedora...
> > > Do not reply to spam on the list, report it:
> > >
https://pagure.io/fedora-infrastructure
> > >
>
>