linux-kernel - Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

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: <1513035963.3036.17.camel@perches.com>
Date:   Mon, 11 Dec 2017 15:46:03 -0800
From:   Joe Perches <joe@...ches.com>
To:     Matthew Wilcox <willy@...radead.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Andy Whitcroft <apw@...dowen.org>
Cc:     Dave Chinner <david@...morbit.com>,
        Alan Stern <stern@...land.harvard.edu>,
        Byungchul Park <byungchul.park@....com>,
        Theodore Ts'o <tytso@....edu>,
        Matthew Wilcox <mawilcox@...rosoft.com>,
        Ross Zwisler <ross.zwisler@...ux.intel.com>,
        Jens Axboe <axboe@...nel.dk>,
        Rehas Sachdeva <aquannie@...il.com>, linux-mm@...ck.org,
        linux-fsdevel@...r.kernel.org,
        linux-f2fs-devel@...ts.sourceforge.net,
        linux-nilfs@...r.kernel.org, linux-btrfs@...r.kernel.org,
        linux-xfs@...r.kernel.org, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel-team@....com
Subject: Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

On Mon, 2017-12-11 at 14:43 -0800, Matthew Wilcox wrote:
> On Mon, Dec 11, 2017 at 02:12:28PM -0800, Joe Perches wrote:
> > Completely reasonable.  Thanks.
> 
> If we're doing "completely reasonable" complaints, then ...
> 
>  - I don't understand why plain 'unsigned' is deemed bad.

That was a David Miller preference.

>  - The rule about all function parameters in prototypes having a name
>    doesn't make sense.  Example:
> 
> int ida_get_new_above(struct ida *ida, int starting_id, int *p_id);

Improvements to regex welcomed.

>  - Forcing a blank line after variable declarations sometimes makes for
>    some weird-looking code.

True.  I don't care for this one myself.
>    Constructively, I think this warning can be suppressed for blocks
>    that are under, say, 8 lines.

Not easy to do as checkpatch works on patches.

> 6) Functions
> ------------
> 
> Functions should be short and sweet, and do just one thing.  They should
> fit on one or two screenfuls of text (the ISO/ANSI screen size is 80x24,
> as we all know), and do one thing and do that well.
> 
>    I'm not expecting you to be able to write a perl script that checks
>    the first line, but we have way too many 200-plus line functions in
>    the kernel.  I'd like a warning on anything over 200 lines (a factor
>    of 4 over Linus's stated goal).

Maybe reasonable.
Some declaration blocks for things like:

void foo(void)
{
	static const struct foobar array[] = {
		{ long count of lines... };
	[body]
}

might make that warning unreasonable though.

>  - I don't understand the error for xa_head here:
> 
> struct xarray {
>         spinlock_t      xa_lock;
>         gfp_t           xa_flags;
>         void __rcu *    xa_head;
> };
> 
>    Do people really think that:
> 
> struct xarray {
>         spinlock_t      xa_lock;
>         gfp_t           xa_flags;
>         void __rcu	*xa_head;
> };
> 
>    is more aesthetically pleasing?  And not just that, but it's an *error*
>    so the former is *RIGHT* and this is *WRONG*.  And not just a matter
>    of taste?

No opinion really.
That's from Andy Whitcroft's original implementation.