Message-ID: <0af43f41-7e63-453b-ad08-bf50b2a6e7a6@gmail.com>
Date: Sun, 11 Aug 2024 22:52:03 +0200
From: Mirsad Todorovac <mtodorovac69@...il.com>
To: Jan Kara <jack@...e.cz>
Cc: Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
 "Gustavo A. R. Silva" <gustavoars@...nel.org>, Kees Cook <kees@...nel.org>,
 Christian Brauner <brauner@...nel.org>,
 "Matthew Wilcox (Oracle)" <willy@...radead.org>,
 Al Viro <viro@...iv.linux.org.uk>, Jeff Layton <jlayton@...nel.org>,
 reiserfs-devel@...r.kernel.org
Subject: Re: [PROBLEM linux-next] fs/reiserfs/do_balan.c:1147:13: error: variable ‘leaf_mi’ set but not used [-Werror=unused-but-set-variable]

On 8/11/24 15:34, Mirsad Todorovac wrote:
> On 8/6/24 10:25, Jan Kara wrote:
>> On Mon 05-08-24 23:24:06, Mirsad Todorovac wrote:
>>> On 8/5/24 15:04, Jan Kara wrote:
>>>> On Fri 02-08-24 18:31:46, Mirsad Todorovac wrote:
>>>>> On 7/18/24 11:39, Jan Kara wrote:
>>>>>> On Thu 18-07-24 00:14:24, Mirsad Todorovac wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 7/17/24 17:44, Jan Kara wrote:
>>>>>>>> On Tue 16-07-24 19:17:05, Mirsad Todorovac wrote:
>>>>>>>>> On 7/15/24 19:28, Jan Kara wrote:
>>>>>>>>>> Hello Mirsad!
>>>>>>>>>>
>>>>>>>>>> On Wed 10-07-24 20:09:27, Mirsad Todorovac wrote:
>>>>>>>>>>> On the linux-next vanilla next-20240709 tree, I have attempted the seed KCONFIG_SEED=0xEE7AB52F
>>>>>>>>>>> which was known from before to trigger various errors in compile and build process.
>>>>>>>>>>>
>>>>>>>>>>> Though this might seem as contributing to channel noise, Linux refuses to build this config,
>>>>>>>>>>> treating warnings as errors, using this build line:
>>>>>>>>>>>
>>>>>>>>>>> $ time nice make W=1 -k -j 36 |& tee ../err-next-20230709-01a.log; date
>>>>>>>>>>>
>>>>>>>>>>> As I know that the Chief Penguin doesn't like warnings, but I am also aware that there are plenty
>>>>>>>>>>> left, there seems to be more tedious work ahead to make the compilers happy.
>>>>>>>>>>>
>>>>>>>>>>> The compiler output is:
>>>>>>>>>>>
>>>>>>>>>>> ---------------------------------------------------------------------------------------------------------
>>>>>>>>>>> fs/reiserfs/do_balan.c: In function ‘balance_leaf_new_nodes_paste_whole’:
>>>>>>>>>>> fs/reiserfs/do_balan.c:1147:13: error: variable ‘leaf_mi’ set but not used [-Werror=unused-but-set-variable]
>>>>>>>>>>>  1147 |         int leaf_mi;
>>>>>>>>>>>       |             ^~~~~~~
>>>>>>>>>>
>>>>>>>>>> Frankly, I wouldn't bother with reiserfs. The warning is there for ages,
>>>>>>>>>> the code is going to get removed in two releases, so I guess we can live
>>>>>>>>>> with these warnings for a few more months...
>>>>>>>>>
>>>>>>>>> In essence I agree with you, but for sentimental reasons I would like to
>>>>>>>>> keep it because it is my first journaling Linux system on Knoppix 🙂
>>>>>>>>
>>>>>>>> As much as I understand your sentiment (I have a bit of history with that
>>>>>>>> fs as well) the maintenance cost isn't really worth it and most fs folks
>>>>>>>> will celebrate when it's removed. We have already announced the removal
>>>>>>>> year and half ago and I'm fully for executing that plan at the end of this
>>>>>>>> year.
>>>>>>>>
>>>>>>>>> Patch is also simple and a no-brainer, as proposed by Mr. Cook:
>>>>>>>>>
>>>>>>>>> -------------------------------><------------------------------------------
>>>>>>>>> diff --git a/fs/reiserfs/do_balan.c b/fs/reiserfs/do_balan.c
>>>>>>>>> index 5129efc6f2e6..fbe73f267853 100644
>>>>>>>>> --- a/fs/reiserfs/do_balan.c
>>>>>>>>> +++ b/fs/reiserfs/do_balan.c
>>>>>>>>> @@ -1144,7 +1144,9 @@ static void balance_leaf_new_nodes_paste_whole(struct tree_balance *tb,
>>>>>>>>>  {
>>>>>>>>>  	struct buffer_head *tbS0 = PATH_PLAST_BUFFER(tb->tb_path);
>>>>>>>>>  	int n = B_NR_ITEMS(tbS0);
>>>>>>>>> +#ifdef CONFIG_REISERFS_CHECK
>>>>>>>>>  	int leaf_mi;
>>>>>>>>> +#endif
>>>>>>>>
>>>>>>>> Well, I would not like this even for actively maintained code ;) If you
>>>>>>>> want to silence these warnings in this dead code, then I could live with
>>>>>>>> something like:
>>>>>>>>
>>>>>>>> #if defined( CONFIG_REISERFS_CHECK )
>>>>>>>> #define RFALSE(cond, format, args...) __RASSERT(!(cond), ....)
>>>>>>>> #else
>>>>>>>> - #define RFALSE( cond, format, args... ) do {;} while( 0 )
>>>>>>>> + #define RFALSE( cond, format, args... ) do { (void)cond; } while( 0 )
>>>>>>>> #endif
>>>>>>>
>>>>>>> Yes, one line change is much smarter than 107 line patch of mine :-)
>>>>>>>
>>>>>>> Verified, and this line solved all the warnings:
>>>>>>>
>>>>>>>   CC      fs/reiserfs/bitmap.o
>>>>>>>   CC      fs/reiserfs/do_balan.o
>>>>>>>   CC      fs/reiserfs/namei.o
>>>>>>>   CC      fs/reiserfs/inode.o
>>>>>>>   CC      fs/reiserfs/file.o
>>>>>>>   CC      fs/reiserfs/dir.o
>>>>>>>   CC      fs/reiserfs/fix_node.o
>>>>>>>   CC      fs/reiserfs/super.o
>>>>>>>   CC      fs/reiserfs/prints.o
>>>>>>>   CC      fs/reiserfs/objectid.o
>>>>>>>   CC      fs/reiserfs/lbalance.o
>>>>>>>   CC      fs/reiserfs/ibalance.o
>>>>>>>   CC      fs/reiserfs/stree.o
>>>>>>>   CC      fs/reiserfs/hashes.o
>>>>>>>   CC      fs/reiserfs/tail_conversion.o
>>>>>>>   CC      fs/reiserfs/journal.o
>>>>>>>   CC      fs/reiserfs/resize.o
>>>>>>>   CC      fs/reiserfs/item_ops.o
>>>>>>>   CC      fs/reiserfs/ioctl.o
>>>>>>>   CC      fs/reiserfs/xattr.o
>>>>>>>   CC      fs/reiserfs/lock.o
>>>>>>>   CC      fs/reiserfs/procfs.o
>>>>>>>   AR      fs/reiserfs/built-in.a
>>>>>>>
>>>>>>> Just FWIW, back then in year 2000/2001 a journaling file system on my
>>>>>>> Knoppix box was a quantum leap - it would simply replay the journal if
>>>>>>> there was a power loss before shutdown. No several minutes of fsck.
>>>>>>
>>>>>> Well, there was also ext3 at that time already :-) That's where I became
>>>>>> familiar with the idea of journalling. Reiserfs was interesting to me
>>>>>> because of completely different approach to on-disk format (b-tree with
>>>>>> formatted items), packing of small files / file tails (interesting in 2000,
>>>>>> not so much 20 years later) and reasonable scalability for large
>>>>>> directories.
>>>>>>
>>>>>>> I think your idea is great and if you wish a patch could be submitted
>>>>>>> after the merge window.
>>>>>>
>>>>>> I'll leave it up to you. If the warnings annoy you, send the patch along
>>>>>> the lines I've proposed (BTW (void)cond should better be (void)(cond) for
>>>>>> macro safety) and I'll push it to Linus.
>>>>>>
>>>>>> 								Honza
>>>>>
>>>>> Hi, Jan,
>>>>>
>>>>> After a short break, I just tried a full build with this hack against the vanilla
>>>>> linux-next tree:
>>>>>
>>>>> #define RFALSE( cond, format, args... ) do { (void)(cond); } while( 0 )
>>>>>
>>>>> and it breaks at least here:
>>>>>
>>>>> In file included from fs/reiserfs/do_balan.c:15:
>>>>> fs/reiserfs/do_balan.c: In function ‘balance_leaf_when_delete_del’:
>>>>> fs/reiserfs/do_balan.c:86:28: error: ‘ih’ undeclared (first use in this function)
>>>>>    86 |         RFALSE(ih_item_len(ih) + IH_SIZE != -tb->insert_size[0],
>>>>>       |                            ^~
>>>>> fs/reiserfs/reiserfs.h:919:54: note: in definition of macro ‘RFALSE’
>>>>>   919 | #define RFALSE( cond, format, args... ) do { (void) (cond); } while( 0 )
>>>>>       |                                                      ^~~~
>>>>> ./include/linux/byteorder/generic.h:91:21: note: in expansion of macro ‘__le16_to_cpu’
>>>>>    91 | #define le16_to_cpu __le16_to_cpu
>>>>>       |                     ^~~~~~~~~~~~~
>>>>> fs/reiserfs/do_balan.c:86:16: note: in expansion of macro ‘ih_item_len’
>>>>>    86 |         RFALSE(ih_item_len(ih) + IH_SIZE != -tb->insert_size[0],
>>>>>       |                ^~~~~~~~~~~
>>>>> fs/reiserfs/do_balan.c:86:28: note: each undeclared identifier is reported only once for each function it appears in
>>>>>    86 |         RFALSE(ih_item_len(ih) + IH_SIZE != -tb->insert_size[0],
>>>>>       |                            ^~
>>>>> fs/reiserfs/reiserfs.h:919:54: note: in definition of macro ‘RFALSE’
>>>>>   919 | #define RFALSE( cond, format, args... ) do { (void) (cond); } while( 0 )
>>>>>       |                                                      ^~~~
>>>>> ./include/linux/byteorder/generic.h:91:21: note: in expansion of macro ‘__le16_to_cpu’
>>>>>    91 | #define le16_to_cpu __le16_to_cpu
>>>>>       |                     ^~~~~~~~~~~~~
>>>>> fs/reiserfs/do_balan.c:86:16: note: in expansion of macro ‘ih_item_len’
>>>>>    86 |         RFALSE(ih_item_len(ih) + IH_SIZE != -tb->insert_size[0],
>>>>>       |                ^~~~~~~~~~~
>>>>> fs/reiserfs/do_balan.c: In function ‘do_balance_starts’:
>>>>> fs/reiserfs/do_balan.c:1800:16: error: implicit declaration of function ‘check_before_balancing’ [-Werror=implicit-function-declaration]
>>>>>  1800 |         RFALSE(check_before_balancing(tb), "PAP-12340: locked buffers in TB");
>>>>>       |                ^~~~~~~~~~~~~~~~~~~~~~
>>>>> fs/reiserfs/reiserfs.h:919:54: note: in definition of macro ‘RFALSE’
>>>>>   919 | #define RFALSE( cond, format, args... ) do { (void) (cond); } while( 0 )
>>>>>       |                                                      ^~~~
>>>>> cc1: some warnings being treated as errors
>>>>> make[7]: *** [scripts/Makefile.build:244: fs/reiserfs/do_balan.o] Error 1
>>>>>   CC [M]  fs/reiserfs/stree.o
>>>>> In file included from fs/reiserfs/stree.c:15:
>>>>> fs/reiserfs/stree.c: In function ‘reiserfs_delete_item’:
>>>>> fs/reiserfs/stree.c:1283:24: error: ‘mode’ undeclared (first use in this function)
>>>>>  1283 |                 RFALSE(mode != M_DELETE, "PAP-5320: mode must be M_DELETE");
>>>>>       |                        ^~~~
>>>>> fs/reiserfs/reiserfs.h:919:54: note: in definition of macro ‘RFALSE’
>>>>>   919 | #define RFALSE( cond, format, args... ) do { (void) (cond); } while( 0 )
>>>>>       |                                                      ^~~~
>>>>> fs/reiserfs/stree.c:1283:24: note: each undeclared identifier is reported only once for each function it appears in
>>>>>  1283 |                 RFALSE(mode != M_DELETE, "PAP-5320: mode must be M_DELETE");
>>>>>       |                        ^~~~
>>>>> fs/reiserfs/reiserfs.h:919:54: note: in definition of macro ‘RFALSE’
>>>>>   919 | #define RFALSE( cond, format, args... ) do { (void) (cond); } while( 0 )
>>>>>       |                                                      ^~~~
>>>>>
>>>>> Last time it compiled, but now it expects variables in (void)(cond) expressions to be defined.
>>>>>
>>>>> I have try to fix those warnings, submitting the patch for review:
>>>>
>>>> Yeah, this looks ok to me.
>>>>
>>>> 								Honza
>>>
>>> Hi, Jan,
>>>
>>> As I understood from the previous experiences, and the Code of Conduct,
>>> and explicit Reviwed-by: or Acked-by: is required ...
>>>
>>> Or otherwise, the Suggested-by: is used?
>>
>> So I was waiting for you to submit official patch with proper changelog and
>> your Signed-off-by. Then I can pick up the patch into my tree and merge it. 
>>
>> 								Honza
> 
> Aaah, sorry I've just noticed your reply. I missed it this morning already.
> 
> Yes, at your request, I will proceed as you recommended.

Hi, Jan,

As the filesystem has problems with the "General Protection Fault", which I learned later and
I am really not qualified to deal with that, just fixing compiler warnings is indeed cosmetics and an
exercise for the little grey cells.

Es ist nicht meine ernst, as Germans would have said.

But I will trust your judgment on whether this is worth patching.

Best regards,
Mirsad Todorovac

Powered by blists - more mailing lists