Re: [PATCH net-next] selftests: net: py: support verbose printing, display executed commands

[Petr Machata <petrm@nvidia.com>]

Mohsin Bashir <mohsin.bashr@gmail.com> writes:

> Add verbosity support to show the commands executed while
> running tests. Enable verbosity if either an environment
> variable 'VERBOSE' is set to a non-zero number or it is defined
> in a config file under driver tests as discussed here:
> https://github.com/linux-netdev/nipa/wiki/Running-driver-tests.
>
> Signed-off-by: Mohsin Bashir <mohsin.bashr@gmail.com>
> ---
>  tools/testing/selftests/drivers/net/lib/py/env.py | 14 +++++++++++++-
>  tools/testing/selftests/net/lib/py/__init__.py    |  7 +++++++
>  tools/testing/selftests/net/lib/py/utils.py       | 14 ++++++++++++++
>  3 files changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/drivers/net/lib/py/env.py b/tools/testing/selftests/drivers/net/lib/py/env.py
> index a5e800b8f103..ec53cf59e104 100644
> --- a/tools/testing/selftests/drivers/net/lib/py/env.py
> +++ b/tools/testing/selftests/drivers/net/lib/py/env.py
> @@ -4,7 +4,7 @@ import os
>  import time
>  from pathlib import Path
>  from lib.py import KsftSkipEx, KsftXfailEx
> -from lib.py import cmd, ethtool, ip
> +from lib.py import cmd, ethtool, ip, verbosity_ctl
>  from lib.py import NetNS, NetdevSimDev
>  from .remote import Remote
>  
> @@ -42,6 +42,12 @@ class NetDrvEnv:
>  
>          self.env = _load_env_file(src_path)
>  
> +        try:
> +            verbosity_ctl(level=int(self.env.get('VERBOSE', 0)))
> +        except ValueError as e:
> +            print(f'Ignoring \'VERBOSE\'. Unknown value \'{self.env.get("VERBOSE")}\'')
> +            verbosity_ctl(level=0)
> +

I think you are looking to catch the integer conversion errors here, so
just enclose that bit:

        env_level = self.env.get('VERBOSE', 0)
        try:
            level = int(env_level)
        except ValueError as e:
            print(f'Ignoring \'VERBOSE\'. Unknown value \'{env_level}\'')
	    level = 0
        verbosity_ctl(level=level)

Now instead of cut'n'pasting this twice, shouldn't this be the real
verbosity_ctl()? Call it set_verbosity(self.env) maybe, call from the
three sites that currently open-code the same.

>          if 'NETIF' in self.env:
>              self.dev = ip("link show dev " + self.env['NETIF'], json=True)[0]
>          else:
> @@ -92,6 +98,12 @@ class NetDrvEpEnv:
>          self._ns = None
>          self._ns_peer = None
>  
> +        try:
> +            verbosity_ctl(level=int(self.env.get('VERBOSE', 0)))
> +        except ValueError as e:
> +            print(f'Ignoring \'VERBOSE\'. Unknown value \'{self.env.get("VERBOSE")}\'')
> +            verbosity_ctl(level=0)
> +
>          if "NETIF" in self.env:
>              if nsim_test is True:
>                  raise KsftXfailEx("Test only works on netdevsim")
> diff --git a/tools/testing/selftests/net/lib/py/__init__.py b/tools/testing/selftests/net/lib/py/__init__.py
> index b6d498d125fe..1541079fadce 100644
> --- a/tools/testing/selftests/net/lib/py/__init__.py
> +++ b/tools/testing/selftests/net/lib/py/__init__.py
> @@ -1,8 +1,15 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
> +import os
>  from .consts import KSRC
>  from .ksft import *
>  from .netns import NetNS
>  from .nsim import *
>  from .utils import *
>  from .ynl import NlError, YnlFamily, EthtoolFamily, NetdevFamily, RtnlFamily
> +
> +try:
> +    verbosity_ctl(level=int(os.environ.get('VERBOSE', 0)))
> +except ValueError as e:
> +    print(f'Ignoring \'VERBOSE\'. Unknown value \'{os.environ.get("VERBOSE")}\'')
> +    verbosity_ctl(level=0)
> diff --git a/tools/testing/selftests/net/lib/py/utils.py b/tools/testing/selftests/net/lib/py/utils.py
> index 72590c3f90f1..4a59958649be 100644
> --- a/tools/testing/selftests/net/lib/py/utils.py
> +++ b/tools/testing/selftests/net/lib/py/utils.py
> @@ -9,6 +9,18 @@ import subprocess
>  import time
>  
>  
> +def verbosity_ctl(level=None):
> +    global VERBOSITY_LEVEL
> +    if level is not None:
> +        VERBOSITY_LEVEL = level
> +    return VERBOSITY_LEVEL

IMHO, have a set_verbosity to just set it, and have verbose() below ask
for the global directly. So if VERBOSITY_LEVEL >= 1.

> +def verbose(*objs, **kwargs):
> +    if verbosity_ctl() >= 1:
> +        print(*objs, **kwargs)
> +
> +
>  class CmdExitFailure(Exception):
>      pass
>  
> @@ -22,6 +34,8 @@ class cmd:
>          self.stderr = None
>          self.ret = None
>  
> +        verbose("#cmd|", comm)
> +
>          self.comm = comm
>          if host:
>              self.proc = host.cmd(comm)