From mboxrd@z Thu Jan 1 00:00:00 1970 From: Al Viro Subject: Re: [PATCH] fs: Cleanup string initializations (char[] instead of char *) Date: Sat, 17 May 2014 18:21:09 +0100 Message-ID: <20140517172109.GH18016@ZenIV.linux.org.uk> References: <1400338818-2853-1-git-send-email-manuel.schoelling@gmx.de> <20140517154427.GB1939@mguzik.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Manuel =?iso-8859-1?Q?Sch=F6lling?= , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org To: Mateusz Guzik Return-path: Content-Disposition: inline In-Reply-To: <20140517154427.GB1939@mguzik.redhat.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org On Sat, May 17, 2014 at 05:44:28PM +0200, Mateusz Guzik wrote: > On Sat, May 17, 2014 at 05:00:18PM +0200, Manuel Sch=F6lling wrote: > > Initializations like 'char *foo =3D "bar"' will create two variable= s: a static > > string and a pointer (foo) to that static string. Instead 'char foo= [] =3D "bar"' > > will declare a single variable and will end up in shorter > > assembly (according to Jeff Garzik on the KernelJanitor's TODO list= ). > >=20 >=20 > This is a greatly oversimplifying things, this may or may not happen. >=20 > Out of curiosity I checked my kernel on x86-64 and it has this > optimized: >=20 > 0xffffffffa00a9629 : movabs $0x203a7367616c66,%rcx > crash> ascii 0x203a7367616c66 > 00203a7367616c66: flags: >=20 >=20 > > fs/binfmt_misc.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > >=20 > > diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c > > index b605003..2a10529 100644 > > --- a/fs/binfmt_misc.c > > +++ b/fs/binfmt_misc.c > > @@ -419,7 +419,7 @@ static void entry_status(Node *e, char *page) > > { > > char *dp; > > char *status =3D "disabled"; > > - const char * flags =3D "flags: "; > > + const char flags[] =3D "flags: "; > > =20 > > if (test_bit(Enabled, &e->flags)) > > status =3D "enabled"; >=20 > This particular function would be better of with removing this variab= le > and replacing all pairs like: > sprintf(dp, ...); > dp +=3D strlen(...) >=20 > with: > dp +=3D sprintf(dp, ...); Sigh... Premature optimisation and all such... Folks, seriously, if y= ou want to do something with it, just switch to single_open(). Something = like this (completely untested): diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c index b605003..357e421 100644 --- a/fs/binfmt_misc.c +++ b/fs/binfmt_misc.c @@ -30,6 +30,7 @@ #include #include #include +#include =20 #include =20 @@ -415,60 +416,47 @@ static int parse_command(const char __user *buffe= r, size_t count) =20 /* generic stuff */ =20 -static void entry_status(Node *e, char *page) +static int entry_status(struct seq_file *m, void *v) { - char *dp; - char *status =3D "disabled"; - const char * flags =3D "flags: "; + Node *e =3D m->private; =20 if (test_bit(Enabled, &e->flags)) - status =3D "enabled"; + seq_puts(m, "enabled\n"); + else + seq_puts(m, "disabled\n"); =20 - if (!VERBOSE_STATUS) { - sprintf(page, "%s\n", status); - return; - } + if (!VERBOSE_STATUS) + return 0; =20 - sprintf(page, "%s\ninterpreter %s\n", status, e->interpreter); - dp =3D page + strlen(page); + seq_printf(m, "interpreter %s\n", e->interpreter); =20 /* print the special flags */ - sprintf (dp, "%s", flags); - dp +=3D strlen (flags); - if (e->flags & MISC_FMT_PRESERVE_ARGV0) { - *dp ++ =3D 'P'; - } - if (e->flags & MISC_FMT_OPEN_BINARY) { - *dp ++ =3D 'O'; - } - if (e->flags & MISC_FMT_CREDENTIALS) { - *dp ++ =3D 'C'; - } - *dp ++ =3D '\n'; + seq_puts(m, "flags: "); + if (e->flags & MISC_FMT_PRESERVE_ARGV0) + seq_putc(m, 'P'); + if (e->flags & MISC_FMT_OPEN_BINARY) + seq_putc(m, 'O'); + if (e->flags & MISC_FMT_CREDENTIALS) + seq_putc(m, 'C'); + seq_putc(m, '\n'); =20 =20 if (!test_bit(Magic, &e->flags)) { - sprintf(dp, "extension .%s\n", e->magic); + seq_printf(m, "extension .%s\n", e->magic); } else { int i; =20 - sprintf(dp, "offset %i\nmagic ", e->offset); - dp =3D page + strlen(page); - for (i =3D 0; i < e->size; i++) { - sprintf(dp, "%02x", 0xff & (int) (e->magic[i])); - dp +=3D 2; - } + seq_printf(m, "offset %i\nmagic ", e->offset); + for (i =3D 0; i < e->size; i++) + seq_printf(m, "%02x", (__u8)e->magic[i]); if (e->mask) { - sprintf(dp, "\nmask "); - dp +=3D 6; - for (i =3D 0; i < e->size; i++) { - sprintf(dp, "%02x", 0xff & (int) (e->mask[i])); - dp +=3D 2; - } + seq_puts(m, "\nmask "); + for (i =3D 0; i < e->size; i++) + seq_printf(m, "%02x", (__u8)e->mask[i]); } - *dp++ =3D '\n'; - *dp =3D '\0'; + seq_putc(m, '\n'); } + return 0; } =20 static struct inode *bm_get_inode(struct super_block *sb, int mode) @@ -512,22 +500,9 @@ static void kill_node(Node *e) =20 /* / */ =20 -static ssize_t -bm_entry_read(struct file * file, char __user * buf, size_t nbytes, lo= ff_t *ppos) +static int bm_entry_open(struct inode *inode, struct file *file) { - Node *e =3D file_inode(file)->i_private; - ssize_t res; - char *page; - - if (!(page =3D (char*) __get_free_page(GFP_KERNEL))) - return -ENOMEM; - - entry_status(e, page); - - res =3D simple_read_from_buffer(buf, nbytes, ppos, page, strlen(page)= ); - - free_page((unsigned long) page); - return res; + return single_open(file, entry_status, file_inode(file)->i_private); } =20 static ssize_t bm_entry_write(struct file *file, const char __user *bu= ffer, @@ -556,9 +531,11 @@ static ssize_t bm_entry_write(struct file *file, c= onst char __user *buffer, } =20 static const struct file_operations bm_entry_operations =3D { - .read =3D bm_entry_read, + .open =3D bm_entry_open, + .release =3D single_release, + .read =3D seq_read, .write =3D bm_entry_write, - .llseek =3D default_llseek, + .llseek =3D seq_lseek, }; =20 /* /register */