| 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
| ||
|
Message-ID: <CAFd5g45MyjJ4e0r=F77YHKcCP6hGSYmS+cP3NmvL_Yh0Ri+Wdg@mail.gmail.com>
Date: Tue, 7 Dec 2021 17:46:33 -0500
From: Brendan Higgins <brendanhiggins@...gle.com>
To: Daniel Latypov <dlatypov@...gle.com>
Cc: David Gow <davidgow@...gle.com>, linux-kernel@...r.kernel.org,
kunit-dev@...glegroups.com, linux-kselftest@...r.kernel.org,
skhan@...uxfoundation.org
Subject: Re: [PATCH 2/2] kunit: tool: add --kconfig_add to allow easily
tweaking kunitconfigs
On Thu, Nov 18, 2021 at 2:11 PM Daniel Latypov <dlatypov@...gle.com> wrote:
>
> On Mon, Nov 8, 2021 at 9:23 AM Daniel Latypov <dlatypov@...gle.com> wrote:
> >
> > On Fri, Nov 5, 2021 at 9:07 PM David Gow <davidgow@...gle.com> wrote:
> > >
> > > On Sat, Nov 6, 2021 at 9:31 AM 'Daniel Latypov' via KUnit Development
> > > <kunit-dev@...glegroups.com> wrote:
> > > >
> > > > E.g. run tests but with KASAN
> > > > $ ./tools/testing/kunit/kunit.py run --arch=x86_64 --kconfig_add=CONFIG_KASAN=y
> > >
> > > This is very neat, thank you. I'm definitely going to use this quite a bit.
> > >
> > > My only real note is that we'll need to add some documentation (but
> > > since the KUnit documentation is being reworked at the moment, I'm
> > > okay with doing that later to avoid merge conflicts).
> >
> > Yeah, there's that and I was also unsure where exactly to mention it.
> > I'd also want there to be the caveat about how removing the option
> > won't trigger a rebuild.
> > The part where we have that right now is really early on and doesn't
> > need more stuff added there:
> > https://www.kernel.org/doc/html/latest/dev-tools/kunit/start.html#creating-a-kunitconfig
> >
> > >
> > > > This also works with --kunitconfig
> > > > $ ./tools/testing/kunit/kunit.py run --arch=x86_64 --kunitconfig=fs/ext4 --kconfig_add=CONFIG_KASAN=y
> > >
> > > It's also worth noting that this can be appended multiple times to set
> > > multiple options, which is useful
> >
> > Ah yeah, this could be called out in the commit desc if we want a v2.
> > Checking the examples in the link down below, TuxMake doesn't actually
> > include one with it being repeated.
> > I had been banking on readers of this message assuming that it could
> > be repeated either from previous familiarity with TuxMake or by
> > clicking that link.
> >
> > But for tweaks that require multiple options, I'm personally going to
> > stick with --kunitconfig and heredocs.
> > E.g. coverage requires appending 3 kconfigs, so I'm sticking with
> >
> > ./tools/testing/kunit/kunit.py run --make_options=CC=/usr/bin/gcc-6
> > --kunitconfig /dev/stdin <<EOF
> > CONFIG_KUNIT=y
> > CONFIG_KUNIT_ALL_TESTS=y
> > CONFIG_DEBUG_KERNEL=y
> > CONFIG_DEBUG_INFO=y
> > CONFIG_GCOV=y
> > EOF
> >
> > >
> > > > This flag is inspired by TuxMake's --kconfig-add, see
> > > > https://gitlab.com/Linaro/tuxmake#examples.
> > > >
> > > > Our version just uses "_" as the delimiter for consistency with
> > > > pre-existing flags like --build_dir, --make_options, --kernel_args, etc.
> > > >
> > > > Note: this does make it easier to run into a pre-existing edge case:
> > > > $ ./tools/testing/kunit/kunit.py run --arch=x86_64 --kconfig_add=CONFIG_KASAN=y
> > > > $ ./tools/testing/kunit/kunit.py run --arch=x86_64
> > > > This second invocation ^ still has KASAN enabled!
> > >
> > > This behaviour is quite useful, and actually means we can turn on
> > > individual items with
> > > $ ./tools/testing/kunit/kunit.py config --kconfig_add=<option>
> >
> > Yes, that also works.
> > I didn't really want to call that out, however.
> >
> > I ultimately would like this option to make it easier to have kunit
> > commands be more declarative and less dependent on state.
>
> I've just proposed
> https://lore.kernel.org/linux-kselftest/20211118190329.1925388-1-dlatypov@google.com
>
> If that patch goes in, the use case described above *won't* work.
> I've been annoyed by the issue that removing lines from .kunitconfig
> doesn't do anything for a while.
>
> I really don't like the "stickiness" of options, since I think it's
> very much not what a user would initially expect. It can be useful in
> some situations, but I don't think it's worth the cost.
Yeah, I agree. It would be nice if commands weren't so stateful.
That's the reality of some things, but I think we are kind of in the
business of quick, repeatable, transient build/tests/environments.
>From that standpoint, I like this flag, and I think it should not be
"sticky" as you describe.
> And I think the stickiness can be annoying to power users as well.
> Imagine you were trying to debug an issue that only showed up if some
> other Kconfig's are set.
> Now instead of iterating by adding diff --kconfig_add=<...>, you have
> to remember to delete .kunit/.config each time, lest you forget and go
> down a rabbit hole.
>
> >
> > E.g. instead of
> > $ cp fs/ext4/.kunitconfig .kunit/.kunitconfig
> > $ echo "CONFIG_KASAN=y" >> .kunit/.kunitconfig
> > $ ./tools/testing/kunit/kunit.py run --arch=x86_64
> >
> > it's now just one line and I'm less likely to miss a step, etc.
> > $ ./tools/testing/kunit/kunit.py run --arch=x86_64
> > --kunitconfig=fs/ext4 --kconfig_add=CONFIG_KASAN=y
> >
> > A user could alternatively do this via
> > $ ./tools/testing/kunit/kunit.py config --arch=x86_64
> > --kunitconfig=fs/ext4 --kconfig_add=CONFIG_KASAN=y
> > $ ./tools/testing/kunit/kunit.py config --arch=x86_64
> > --kconfig_add=CONFIG_ANOTHER_OPTION=y
> > $ ./tools/testing/kunit/kunit.py build
> > $ ./tools/testing/kunit/kunit.py exec --arch=x86_64
> >
> >
> > >
> > > > kunit.py won't call olddefconfig if our current .config is already a
> > > > superset of the provided kunitconfig.
> > > >
> > > > Signed-off-by: Daniel Latypov <dlatypov@...gle.com>
> > >
> > > Looks good.
> > >
> > > Reviewed-by: David Gow <davidgow@...gle.com>
> > >
> > >
> > > > ---
> > > > tools/testing/kunit/kunit.py | 8 ++++++++
> > > > tools/testing/kunit/kunit_kernel.py | 5 +++++
> > > > tools/testing/kunit/kunit_tool_test.py | 18 ++++++++++++++++++
> > > > 3 files changed, 31 insertions(+)
> > > >
> > > > diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
> > > > index 68e6f461c758..be58f4c93806 100755
> > > > --- a/tools/testing/kunit/kunit.py
> > > > +++ b/tools/testing/kunit/kunit.py
> > > > @@ -280,6 +280,10 @@ def add_common_opts(parser) -> None:
> > > > ' If given a directory, (e.g. lib/kunit), "/.kunitconfig" '
> > > > 'will get automatically appended.',
> > > > metavar='kunitconfig')
> > > > + parser.add_argument('--kconfig_add',
> > > > + help='Additional Kconfig options to append to the '
> > > > + '.kunitconfig, e.g. CONFIG_KASAN=y. Can be repeated.',
> > > > + action='append')
> > > >
> > > > parser.add_argument('--arch',
> > > > help=('Specifies the architecture to run tests under. '
> > > > @@ -398,6 +402,7 @@ def main(argv, linux=None):
> > > > if not linux:
> > > > linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir,
> > > > kunitconfig_path=cli_args.kunitconfig,
> > > > + kconfig_add=cli_args.kconfig_add,
> > > > arch=cli_args.arch,
> > > > cross_compile=cli_args.cross_compile,
> > > > qemu_config_path=cli_args.qemu_config)
> > > > @@ -423,6 +428,7 @@ def main(argv, linux=None):
> > > > if not linux:
> > > > linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir,
> > > > kunitconfig_path=cli_args.kunitconfig,
> > > > + kconfig_add=cli_args.kconfig_add,
> > > > arch=cli_args.arch,
> > > > cross_compile=cli_args.cross_compile,
> > > > qemu_config_path=cli_args.qemu_config)
> > > > @@ -439,6 +445,7 @@ def main(argv, linux=None):
> > > > if not linux:
> > > > linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir,
> > > > kunitconfig_path=cli_args.kunitconfig,
> > > > + kconfig_add=cli_args.kconfig_add,
> > > > arch=cli_args.arch,
> > > > cross_compile=cli_args.cross_compile,
> > > > qemu_config_path=cli_args.qemu_config)
> > > > @@ -457,6 +464,7 @@ def main(argv, linux=None):
> > > > if not linux:
> > > > linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir,
> > > > kunitconfig_path=cli_args.kunitconfig,
> > > > + kconfig_add=cli_args.kconfig_add,
> > > > arch=cli_args.arch,
> > > > cross_compile=cli_args.cross_compile,
> > > > qemu_config_path=cli_args.qemu_config)
> > > > diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
> > > > index 51ee6e5dae91..7d459d6d6ff2 100644
> > > > --- a/tools/testing/kunit/kunit_kernel.py
> > > > +++ b/tools/testing/kunit/kunit_kernel.py
> > > > @@ -224,6 +224,7 @@ class LinuxSourceTree(object):
> > > > build_dir: str,
> > > > load_config=True,
> > > > kunitconfig_path='',
> > > > + kconfig_add: Optional[List[str]]=None,
> > > > arch=None,
> > > > cross_compile=None,
> > > > qemu_config_path=None) -> None:
> > > > @@ -249,6 +250,10 @@ class LinuxSourceTree(object):
> > > > shutil.copyfile(DEFAULT_KUNITCONFIG_PATH, kunitconfig_path)
> > > >
> > > > self._kconfig = kunit_config.parse_file(kunitconfig_path)
> > > > + if kconfig_add:
> > > > + kconfig = kunit_config.parse_from_string('\n'.join(kconfig_add))
> > > > + self._kconfig.merge_in_entries(kconfig)
> > > > +
> > > >
> > > > def clean(self) -> bool:
> > > > try:
> > > > diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py
> > > > index 4ec70e41ec5a..7e42a7c27987 100755
> > > > --- a/tools/testing/kunit/kunit_tool_test.py
> > > > +++ b/tools/testing/kunit/kunit_tool_test.py
> > > > @@ -334,6 +334,10 @@ class LinuxSourceTreeTest(unittest.TestCase):
> > > > pass
> > > > kunit_kernel.LinuxSourceTree('', kunitconfig_path=dir)
> > > >
> > > > + def test_kconfig_add(self):
> > > > + tree = kunit_kernel.LinuxSourceTree('', kconfig_add=['CONFIG_NOT_REAL=y'])
> > > > + self.assertIn(kunit_config.KconfigEntry('NOT_REAL', 'y'), tree._kconfig.entries())
> > > > +
> > > > def test_invalid_arch(self):
> > > > with self.assertRaisesRegex(kunit_kernel.ConfigError, 'not a valid arch, options are.*x86_64'):
> > > > kunit_kernel.LinuxSourceTree('', arch='invalid')
> > > > @@ -540,6 +544,7 @@ class KUnitMainTest(unittest.TestCase):
> > > > # Just verify that we parsed and initialized it correctly here.
> > > > mock_linux_init.assert_called_once_with('.kunit',
> > > > kunitconfig_path='mykunitconfig',
> > > > + kconfig_add=None,
> > > > arch='um',
> > > > cross_compile=None,
> > > > qemu_config_path=None)
> > > > @@ -551,6 +556,19 @@ class KUnitMainTest(unittest.TestCase):
> > > > # Just verify that we parsed and initialized it correctly here.
> > > > mock_linux_init.assert_called_once_with('.kunit',
> > > > kunitconfig_path='mykunitconfig',
> > > > + kconfig_add=None,
> > > > + arch='um',
> > > > + cross_compile=None,
> > > > + qemu_config_path=None)
> > > > +
> > > > + @mock.patch.object(kunit_kernel, 'LinuxSourceTree')
> > > > + def test_run_kconfig_add(self, mock_linux_init):
> > > > + mock_linux_init.return_value = self.linux_source_mock
> > > > + kunit.main(['run', '--kconfig_add=CONFIG_KASAN=y', '--kconfig_add=CONFIG_KCSAN=y'])
> > > > + # Just verify that we parsed and initialized it correctly here.
> > > > + mock_linux_init.assert_called_once_with('.kunit',
> > > > + kunitconfig_path=None,
> > > > + kconfig_add=['CONFIG_KASAN=y', 'CONFIG_KCSAN=y'],
> > > > arch='um',
> > > > cross_compile=None,
> > > > qemu_config_path=None)
> > > > --
> > > > 2.34.0.rc0.344.g81b53c2807-goog
> > > >
> > > > --
> > > > You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> > > > To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@...glegroups.com.
> > > > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20211106013058.2621799-2-dlatypov%40google.com.
Powered by blists - more mailing lists