2009-04-08 07:56:51

by Vitaly Mayatskih

Subject: Re: [PATCH] Remove double initialization of retval in load_misc_binary()

At Wed, 8 Apr 2009 10:32:48 +0300, Kirill A. Shutemov wrote:

> Probably, better way is removing both 'retval = -ENOEXEC;'? and initialize it within definition.

Agree.

Signed-off-by: Vitaly Mayatskikh <[email protected]>

diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index c4e8353..f10150f 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -109,14 +109,12 @@ static int load_misc_binary(struct linux_binprm *bprm, struct pt_regs *regs)
struct file * interp_file = NULL;
char iname[BINPRM_BUF_SIZE];
char *iname_addr = iname;
- int retval;
+ int retval = -ENOEXEC;
int fd_binary = -1;

- retval = -ENOEXEC;
if (!enabled)
goto _ret;

- retval = -ENOEXEC;
if (bprm->recursion_depth > BINPRM_MAX_RECURSION)
goto _ret;


--
wbr, Vitaly

2009-04-10 21:39:32

by Andrew Morton

Subject: Re: [PATCH] Remove double initialization of retval in load_misc_binary()

On Wed, 08 Apr 2009 09:56:29 +0200
Vitaly Mayatskikh <[email protected]> wrote:

> At Wed, 8 Apr 2009 10:32:48 +0300, Kirill A. Shutemov wrote:
>
> > Probably, better way is removing both 'retval = -ENOEXEC;'__ and initialize it within definition.
>
> Agree.
>
> Signed-off-by: Vitaly Mayatskikh <[email protected]>
>
> diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
> index c4e8353..f10150f 100644
> --- a/fs/binfmt_misc.c
> +++ b/fs/binfmt_misc.c
> @@ -109,14 +109,12 @@ static int load_misc_binary(struct linux_binprm *bprm, struct pt_regs *regs)
> struct file * interp_file = NULL;
> char iname[BINPRM_BUF_SIZE];
> char *iname_addr = iname;
> - int retval;
> + int retval = -ENOEXEC;
> int fd_binary = -1;
>
> - retval = -ENOEXEC;
> if (!enabled)
> goto _ret;
>
> - retval = -ENOEXEC;
> if (bprm->recursion_depth > BINPRM_MAX_RECURSION)
> goto _ret;
>

I don't think this is really a desirable change.

What the existing code is effectively doing is:

if (!enabled) {
retval = -ENOEXEC;
goto _ret;
}

if (bprm->recursion_depth > BINPRM_MAX_RECURSION) {
retval = -ENOEXEC;
goto _ret;
}

only it's doing this via an odd coding trick which used to (and might
still) generate more efficient code.

Those two pieces of code are logically separate things and it's just by
coincidence that they both happen to use the same errno.

Your proposed patch will create a linkage between those two unique
pieces of code which wasn't there previously. It's a snmall thing.

Plus the compiler will surely remove one of the loads anyway, so this
is purely a source-level change. And I don't think it made the source
easier to read and maintain!