Message-ID: <a0562a00-a45f-4df6-98cf-3087ea4af848@gmail.com>
Date: Sun, 11 Aug 2024 15:34:18 +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/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.

Best regards,
Mirsad Todorovac

Powered by blists - more mailing lists