linux-kernel - Re: [PATCH 1/4] selftests/nolibc: drop unused test helpers

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <0d441f70-bdc3-4aa1-b392-6ec170c9a5bb@t-8ch.de>
Date:   Mon, 31 Jul 2023 10:23:33 +0200
From:   Thomas Weißschuh <linux@...ssschuh.net>
To:     Zhangjin Wu <falcon@...ylab.org>
Cc:     linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
        shuah@...nel.org, tanyuan@...ylab.org, w@....eu
Subject: Re: [PATCH 1/4] selftests/nolibc: drop unused test helpers

On 2023-07-31 15:32:43+0800, Zhangjin Wu wrote:
> Hi, Thomas
> 
> > Note:
> > 
> > It seems your mail client does not add the prefix "Re: " to responses.
> > Is that intentional?
> >
> 
> I use a lightweight 'b4 + git send-email' method to reply emails,
> sometimes, I forgot manually adding the 'Re: ' prefix, perhaps I should
> write a simple script to do that or carefully check the Subject title
> everytime, Thanks!

Now there are two "Re: " prefixes :-)

My understanding is that there is exactly one "Re: " prefix iff the
message is any response at all.

> > On 2023-07-31 14:48:26+0800, Zhangjin Wu wrote:
> > > Hi, Thomas
> > > 
> > > > As we want to enable compiler warnings in the future these would be
> > > > reported as unused functions.
> > > > 
> > > > If we need them in the future they are easy to recreate from their still
> > > > existing siblings.
> > > > 
> > > > Signed-off-by: Thomas Weißschuh <linux@...ssschuh.net>
> > > > ---
> > > >  tools/testing/selftests/nolibc/nolibc-test.c | 99 ----------------------------
> > > >  1 file changed, 99 deletions(-)
> > > > 
> > > > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> > > > index 03b1d30f5507..53e2d448eded 100644
> > > > --- a/tools/testing/selftests/nolibc/nolibc-test.c
> > > > +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> > > > @@ -161,31 +161,6 @@ static void result(int llen, enum RESULT r)
> > > >   * of failures, thus either 0 or 1.
> > > >   */
> > > >  
> > > > -#define EXPECT_ZR(cond, expr)				\
> > > > -	do { if (!(cond)) result(llen, SKIPPED); else ret += expect_zr(expr, llen); } while (0)
> > > > -
> > > > -static int expect_zr(int expr, int llen)
> > > > -{
> > > 
> > > Why not a simple 'static __attribute__((unused))' line, then, no need to
> > > add them again next time.
> > > 
> > >     -static int expect_zr(int expr, int llen)
> > >     +static __attribute__((unused))
> > >     +int expect_zr(int expr, int llen)
> > >      {
> > 
> > Personally I don't like having dead code lying around that needs to be
> > maintained and skipped over while reading.
> > It's not a given that we will need those helpers in the future at all.
> >
> 
> It is reasonable in some degree from current status, especially for
> these ones are newly added, but let us think about these scenes: when we
> would drop or change some test cases in the future and the helpers may
> would be not referenced by any test cases in a short time, and warnings
> there, but some other cases may be added later to use them again ...

That doesn't seem very likely.
Did it happen recently?

> I'm ok to drop these ones, but another patch may be required to add
> 'static __attribute__((unused))' for all of the currently used ones,
> otherwise, there will be warnings randomly by a test case change or
> drop.

Then we just drop the helper when we don't need it anymore.

I also dislike the __attribute__ spam to be honest.

> Or even further, is it possible to merge some of them to some more
> generic helpers like the ones used from the selftest.h in your last RFC
> patchset?

Something like this will indeed be part of the KTAP rework.
But it's a change for another time.

Thomas