linux-kernel - Re: [PATCH] shared: linux: remove unused variables

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: <2bae0949-36ea-4d17-b64f-19fd08e5a2f2@lucifer.local>
Date: Mon, 23 Sep 2024 16:48:39 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Ba Jing <bajing@...s.chinamobile.com>
Cc: Sid Kumar <sidhartha.kumar@...cle.com>, akpm@...ux-foundation.org,
        Liam.Howlett@...cle.com, zhangpeng.00@...edance.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] shared: linux: remove unused variables

On Mon, Sep 23, 2024 at 10:37:43AM GMT, Sid Kumar wrote:
>
> On 9/2/24 11:29 PM, Ba Jing wrote:
> > These variables are never referenced in the code, just remove them.
>
> Hello,
>
> I think these variables are needed and they are referenced in main.c within
> the radix-tree directory. With these lines removed and running make within
> tools/testing/radix-tree I get the following errors:
>
> /usr/bin/ld: /home/ubuntu/linux-sidhakum-public/tools/testing/radix-tree/main.c:74:(.text+0x57a):
> undefined reference to `test_verbose'
> /usr/bin/ld: main.o: in function `check_copied_tags':
> /home/ubuntu/linux-sidhakum-public/tools/testing/radix-tree/main.c:135:(.text+0xc3f):
> undefined reference to `test_verbose'
>
>
> /home/ubuntu/linux-sidhakum-public/tools/testing/radix-tree/main.c:243:(.text+0x1d68):
> undefined reference to `preempt_count'
> /usr/bin/ld: /home/ubuntu/linux-sidhakum-public/tools/testing/radix-tree/main.c:243:(.text+0x1d8d):
> undefined reference to `preempt_count'
>
> adding back these variables allows make to run successfully again.
>
> Thanks,
>
> Sidhartha Kumar

Agreed, this patch is just incorrect, obviously the above that is required
to demonstrate that, but a quick grep also shows these variables in use.

It's important when performing a patch removing things like this to check
carefully that you are not mistaken, a quick grep is a basic step (but not
always sufficient as the C preprocessor might generate variable names
etc. so you should be _absolutely_ sure).

Also, generally speaking, there might be reasons for maintaing variables
even if they _are_ unused. We've had a number of people sending patches
trying to remove _actually_ unused variables which were incorrect.

So, I'd suggest in future if submitting a similar kind of patch, to send it
as an RFC, so you first get input on this.

Though again in this case, this is simply incorrect.
>
>
> >
> > Signed-off-by: Ba Jing <bajing@...s.chinamobile.com>
> > ---
> >   tools/testing/shared/linux.c | 2 --
> >   1 file changed, 2 deletions(-)
> >
> > diff --git a/tools/testing/shared/linux.c b/tools/testing/shared/linux.c
> > index 17263696b5d8..eb1b1878cf64 100644
> > --- a/tools/testing/shared/linux.c
> > +++ b/tools/testing/shared/linux.c
> > @@ -13,8 +13,6 @@
> >   #include <urcu/uatomic.h>
> >   int nr_allocated;
> > -int preempt_count;
> > -int test_verbose;
> >   struct kmem_cache {
> >   	pthread_mutex_t lock;