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: <20240805130415.ws3t7sgvcg7cj5ev@quack3>
Date: Mon, 5 Aug 2024 15:04:15 +0200
From: Jan Kara <jack@...e.cz>
To: Mirsad Todorovac <mtodorovac69@...il.com>
Cc: Jan Kara <jack@...e.cz>,
	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 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

> 
> -------------------><---------------------------------------
> diff --git a/fs/reiserfs/do_balan.c b/fs/reiserfs/do_balan.c
> index 5129efc6f2e6..c8fa3d71ef63 100644
> --- a/fs/reiserfs/do_balan.c
> +++ b/fs/reiserfs/do_balan.c
> @@ -81,11 +81,11 @@ static void balance_leaf_when_delete_del(struct tree_balance *tb)
>         struct buffer_info bi;
>  #ifdef CONFIG_REISERFS_CHECK
>         struct item_head *ih = item_head(tbS0, item_pos);
> -#endif
>  
>         RFALSE(ih_item_len(ih) + IH_SIZE != -tb->insert_size[0],
>                "vs-12013: mode Delete, insert size %d, ih to be deleted %h",
>                -tb->insert_size[0], ih);
> +#endif
>  
>         buffer_info_init_tbS0(tb, &bi);
>         leaf_delete_items(&bi, 0, item_pos, 1, -1);
> @@ -1797,8 +1797,8 @@ static inline void do_balance_starts(struct tree_balance *tb)
>         print_tb(flag, PATH_LAST_POSITION(tb->tb_path),
>                  tb->tb_path->pos_in_item, tb, "check");
>         */
> -       RFALSE(check_before_balancing(tb), "PAP-12340: locked buffers in TB");
>  #ifdef CONFIG_REISERFS_CHECK
> +       RFALSE(check_before_balancing(tb), "PAP-12340: locked buffers in TB");
>         REISERFS_SB(tb->tb_sb)->cur_tb = tb;
>  #endif
>  }
> diff --git a/fs/reiserfs/reiserfs.h b/fs/reiserfs/reiserfs.h
> index f0e1f29f20ee..027e64853710 100644
> --- a/fs/reiserfs/reiserfs.h
> +++ b/fs/reiserfs/reiserfs.h
> @@ -916,7 +916,7 @@ do {                                                                        \
>  #if defined( CONFIG_REISERFS_CHECK )
>  #define RFALSE(cond, format, args...) __RASSERT(!(cond), "!(" #cond ")", format, ##args)
>  #else
> -#define RFALSE( cond, format, args... ) do {;} while( 0 )
> +#define RFALSE( cond, format, args... ) do { (void) (cond); } while( 0 )
>  #endif
>  
>  #define CONSTF __attribute_const__
> diff --git a/fs/reiserfs/stree.c b/fs/reiserfs/stree.c
> index 5faf702f8d15..eed1a461169e 100644
> --- a/fs/reiserfs/stree.c
> +++ b/fs/reiserfs/stree.c
> @@ -1280,7 +1280,9 @@ int reiserfs_delete_item(struct reiserfs_transaction_handle *th,
>                                               &del_size,
>                                               max_reiserfs_offset(inode));
>  
> +#ifdef CONFIG_REISERFS_CHECK
>                 RFALSE(mode != M_DELETE, "PAP-5320: mode must be M_DELETE");
> +#endif
>  
>                 copy_item_head(&s_ih, tp_item_head(path));
>                 s_del_balance.insert_size[0] = del_size;
> --
> 
> Thanks.
> 
> Best regards,
> Mirsad Todorovac
> 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ