netdev - Re: [PATCH bpf-next v2 11/11] selftests/xsk: introduce XSKTEST

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAJ8uoz0mFUB3Pg8DU6k6aADywmGuZfds8qaUUnpCc1MBJhjc2g@mail.gmail.com>
Date: Fri, 25 Aug 2023 15:34:18 +0200
From: Magnus Karlsson <magnus.karlsson@...il.com>
To: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
Cc: magnus.karlsson@...el.com, bjorn@...nel.org, ast@...nel.org, 
	daniel@...earbox.net, netdev@...r.kernel.org, bpf@...r.kernel.org, yhs@...com, 
	andrii@...nel.org, martin.lau@...ux.dev, song@...nel.org, 
	john.fastabend@...il.com, kpsingh@...nel.org, sdf@...gle.com, 
	haoluo@...gle.com, jolsa@...nel.org, przemyslaw.kitszel@...el.com
Subject: Re: [PATCH bpf-next v2 11/11] selftests/xsk: introduce XSKTEST_ETH
 environment variable

On Fri, 25 Aug 2023 at 15:20, Maciej Fijalkowski
<maciej.fijalkowski@...el.com> wrote:
>
> On Fri, Aug 25, 2023 at 03:03:58PM +0200, Magnus Karlsson wrote:
> > On Fri, 25 Aug 2023 at 14:57, Maciej Fijalkowski
> > <maciej.fijalkowski@...el.com> wrote:
> > >
> > > On Thu, Aug 24, 2023 at 02:28:53PM +0200, Magnus Karlsson wrote:
> > > > From: Magnus Karlsson <magnus.karlsson@...el.com>
> > > >
> > > > Introduce the XSKTEST_ETH environment variable to be able to set the
> > > > network interface that should be used for testing.
> > > >
> > > > Signed-off-by: Magnus Karlsson <magnus.karlsson@...el.com>
> > > > ---
> > > >  tools/testing/selftests/bpf/test_xsk.sh | 20 +++++++++-----------
> > > >  1 file changed, 9 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/tools/testing/selftests/bpf/test_xsk.sh b/tools/testing/selftests/bpf/test_xsk.sh
> > > > index 9ec718043c1a..3e0a2302a185 100755
> > > > --- a/tools/testing/selftests/bpf/test_xsk.sh
> > > > +++ b/tools/testing/selftests/bpf/test_xsk.sh
> > > > @@ -88,14 +88,12 @@
> > > >
> > > >  . xsk_prereqs.sh
> > > >
> > > > -ETH=""
> > > > -
> > > >  while getopts "vi:dm:lt:h" flag
> > > >  do
> > > >       case "${flag}" in
> > > >               v) verbose=1;;
> > > >               d) debug=1;;
> > > > -             i) ETH=${OPTARG};;
> > > > +             i) XSKTEST_ETH=${OPTARG};;
> > > >               m) XSKTEST_MODE=${OPTARG};;
> > > >               l) list=1;;
> > > >               t) XSKTEST_TEST=${OPTARG};;
> > > > @@ -157,9 +155,9 @@ if [[ $help -eq 1 ]]; then
> > > >          exit
> > > >  fi
> > > >
> > > > -if [ ! -z $ETH ]; then
> > > > -     VETH0=${ETH}
> > > > -     VETH1=${ETH}
> > > > +if [ -n "$XSKTEST_ETH" ]; then
> > >
> > > Sorry - is point of this patch is just to invert the logic and rename the
> > > env var?
> >
> > The purpose was to make it setable from the outside and give it a name
> > that is more descriptive and targeted only to xskxceiver.
>
> and this is accomplished by not having ETH initialized here? What will be
> 'the outside' ?
>
> Currently I don't see much value within this patch, unless you explain the
> need for setting this from outside of this script. Maybe I missed some

Outside = from the command line or your environment variables. ETH is
not a good name to have set in your environment variable. Who knows
what it would be used for. with XSKTEST_* at least you know it is used
for the xsk testing.

> discussion from v1. I can live with this variable being ETH, what's more
> concerning/confusing to me is that for ZC we have to set VETH0 and VETH1
> to ETH and then use that later on.

That is in the old code. Nothing I am trying to address here.

If this patch is useful or not, I do not know to be honest. Just added
it because Przemyslaw suggested that we make the mode setable from
your env variables in your shell. Weird to have the mode setable but
not the ethernet interface. So either we drop both and make it
impossible to use env variables, or support setting both from the env
variables. Do not have a strong opinion either way. Maybe this is just
an unnecessary "feature" that will not be used.

>
> >
> > > > +     VETH0=${XSKTEST_ETH}
> > > > +     VETH1=${XSKTEST_ETH}
> > > >  else
> > > >       validate_root_exec
> > > >       validate_veth_support ${VETH0}
> > > > @@ -203,10 +201,10 @@ fi
> > > >
> > > >  exec_xskxceiver
> > > >
> > > > -if [ -z $ETH ]; then
> > > > +if [ -z $XSKTEST_ETH ]; then
> > > >       cleanup_exit ${VETH0} ${VETH1}
> > > >  else
> > > > -     cleanup_iface ${ETH} ${MTU}
> > > > +     cleanup_iface ${XSKTEST_ETH} ${MTU}
> > > >  fi
> > > >
> > > >  if [[ $list -eq 1 ]]; then
> > > > @@ -216,17 +214,17 @@ fi
> > > >  TEST_NAME="XSK_SELFTESTS_${VETH0}_BUSY_POLL"
> > > >  busy_poll=1
> > > >
> > > > -if [ -z $ETH ]; then
> > > > +if [ -z $XSKTEST_ETH ]; then
> > > >       setup_vethPairs
> > > >  fi
> > > >  exec_xskxceiver
> > > >
> > > >  ## END TESTS
> > > >
> > > > -if [ -z $ETH ]; then
> > > > +if [ -z $XSKTEST_ETH ]; then
> > > >       cleanup_exit ${VETH0} ${VETH1}
> > > >  else
> > > > -     cleanup_iface ${ETH} ${MTU}
> > > > +     cleanup_iface ${XSKTEST_ETH} ${MTU}
> > > >  fi
> > > >
> > > >  failures=0
> > > > --
> > > > 2.34.1
> > > >