linux-kernel - Re: [PATCH 4/5] mm: shmem: Convert shmem_enabled_show to use sysfs_emit

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]

Date:   Sun, 01 Nov 2020 13:43:13 -0800
From:   Joe Perches <joe@...ches.com>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     Hugh Dickins <hughd@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/5] mm: shmem: Convert shmem_enabled_show to use
 sysfs_emit_at

On Sun, 2020-11-01 at 21:19 +0000, Matthew Wilcox wrote:
> On Sun, Nov 01, 2020 at 01:04:35PM -0800, Joe Perches wrote:
> > On Sun, 2020-11-01 at 20:48 +0000, Matthew Wilcox wrote:
> > > On Sun, Nov 01, 2020 at 12:12:51PM -0800, Joe Perches wrote:
> > > > @@ -4024,7 +4024,7 @@ int __init shmem_init(void)
> > > >  
> > > > 
> > > >  #if defined(CONFIG_TRANSPARENT_HUGEPAGE) && defined(CONFIG_SYSFS)
> > > >  static ssize_t shmem_enabled_show(struct kobject *kobj,
> > > > -		struct kobj_attribute *attr, char *buf)
> > > > +				  struct kobj_attribute *attr, char *buf)
> > > >  {
> > > >  	static const int values[] = {
> > > >  		SHMEM_HUGE_ALWAYS,
> > > 
> > > Why?
> > 
> > why what?
> 
> Why did you change this?

Are you asking about the function argument alignment or the commit message?

The function argument alignment because the function is being updated.
The commit itself because sysfs_emit is the documented preferred interface.

> > > > @@ -4034,16 +4034,19 @@ static ssize_t shmem_enabled_show(struct kobject *kobj,
> > > >  		SHMEM_HUGE_DENY,
> > > >  		SHMEM_HUGE_FORCE,
> > > >  	};
> > > > -	int i, count;
> > > > -
> > > > -	for (i = 0, count = 0; i < ARRAY_SIZE(values); i++) {
> > > > -		const char *fmt = shmem_huge == values[i] ? "[%s] " : "%s ";
> > > > +	int len = 0;
> > > > +	int i;
> > > 
> > > Better:
> > > 	int i, len = 0;
> > 
> > I generally disagree as I think it better to have each declaration on an
> > individual line.
> 
> You're wrong.

I'm not wrong.  We just disagree.  Look for yourself at typical
declaration use in the kernel.  The typical style is single line
declarations.

That single declaration per line style is also documented in
coding-style.rst

"To this end, use just one data declaration per line (no commas for
multiple data declarations)"

> Look, this isn't performance sensitive code.  Just do something simple.
> 
> 		if (shmem_huge == values[i])
> 			buf += sysfs_emit(buf, "[%s]",
> 					shmem_format_huge(values[i]));
> 		else
> 			buf += sysfs_emit(buf, "%s",
> 					shmem_format_huge(values[i]));
> 		if (i == ARRAY_SIZE(values) - 1)
> 			buf += sysfs_emit(buf, "\n");
> 		else
> 			buf += sysfs_emit(buf, " ");
> 
> Shame there's no sysfs_emitc, but there you go.

I think what's there is simple.

And your suggested code doesn't work.
sysfs_emit is used for single emits.
sysfs_emit_at is used for multiple emits.