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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <616b92af9378e9f9697555074bba1e377450477f.camel@perches.com>
Date:   Sun, 01 Nov 2020 13:04:35 -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 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?
 
> > @@ -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.

> > -		count += sprintf(buf + count, fmt,
> > -				shmem_format_huge(values[i]));
> > +	for (i = 0; i < ARRAY_SIZE(values); i++) {
> > +		len += sysfs_emit_at(buf, len,
> > +				     shmem_huge == values[i] ? "%s[%s]" : "%s%s",
> > +				     i ? " " : "",
> > +				     shmem_format_huge(values[i]));
> 
> This is ... complicated.  I thought the point of doing all the sysfs_emit
> stuff was to simplify things.

The removal of fmt allows the format and argument to be __printf verified.
Indirected formats do not generally allow that.

And using sysfs_emit is not really intended to simplify output code, it's
used to make sure there isn't a overflow of the PAGE_SIZE output buf when
using sprintf/snprintf.