Re: [PATCHv3 1/8] powerpc/vdso: unify return paths in setup_additional_pages

[Dmitry Safonov]

On 11/08/2016 03:10 AM, Michael Ellerman wrote:
> Hi Dmitry,
>
> Thanks for the patches.
>
> Dmitry Safonov <dsafonov@xxxxxxxxxxxxx> writes:
> > Impact: cleanup
>
> I'm not a fan of these "Impact" lines, especially when they're not
> correct, ie. this is not a cleanup, a cleanup doesn't change logic.
>
> > Rename `rc' variable which doesn't seems to mean anything into
> > kernel-known `ret'.
>
> 'rc' means "Return Code", it's fairly common. I see at least ~8500
> "int rc" declarations in the kernel.
>
> Please don't rename variables and change logic in one patch.

Ok, right - just didn't saw `rc' that freq as `ret'.
Will leave the name.

> > Combine two function returns into one as it's
> > also easier to read.
> >
> > Cc: Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx>
> > Cc: Paul Mackerras <paulus@xxxxxxxxx>
> > Cc: Michael Ellerman <mpe@xxxxxxxxxxxxxx>
> > Cc: Andy Lutomirski <luto@xxxxxxxxxxxxxx>
> > Cc: Oleg Nesterov <oleg@xxxxxxxxxx>
> > Cc: linuxppc-dev@xxxxxxxxxxxxxxxx
> > Cc: linux-mm@xxxxxxxxx
> > Signed-off-by: Dmitry Safonov <dsafonov@xxxxxxxxxxxxx>
> > ---
> >  arch/powerpc/kernel/vdso.c | 19 +++++++------------
> >  1 file changed, 7 insertions(+), 12 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/vdso.c b/arch/powerpc/kernel/vdso.c
> > index 4111d30badfa..4ffb82a2d9e9 100644
> > --- a/arch/powerpc/kernel/vdso.c
> > +++ b/arch/powerpc/kernel/vdso.c
> > @@ -154,7 +154,7 @@ int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp)
> >  	struct page **vdso_pagelist;
> >  	unsigned long vdso_pages;
> >  	unsigned long vdso_base;
> > -	int rc;
> > +	int ret = 0;
>
> Please don't initialise return codes in the declaration, it prevents the
> compiler from warning you if you forget to initialise it in a
> particular path.
>
> AFAICS you never even use the default value either.

Oh, right - I split this patch from converting install_special_mapping()
to special vma version _install_special_mapping(), 6/8 patch in series.
Will move initialization to that patch.

> >  	if (!vdso_ready)
> >  		return 0;
> > @@ -203,8 +203,8 @@ int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp)
> >  				      ((VDSO_ALIGNMENT - 1) & PAGE_MASK),
> >  				      0, 0);
> >  	if (IS_ERR_VALUE(vdso_base)) {
> > -		rc = vdso_base;
> > -		goto fail_mmapsem;
> > +		ret = vdso_base;
> > +		goto out_up_mmap_sem;
> >  	}
> >
> >  	/* Add required alignment. */
> > @@ -227,21 +227,16 @@ int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp)
> >  	 * It's fine to use that for setting breakpoints in the vDSO code
> >  	 * pages though.
> >  	 */
> > -	rc = install_special_mapping(mm, vdso_base, vdso_pages << PAGE_SHIFT,
> > +	ret = install_special_mapping(mm, vdso_base, vdso_pages << PAGE_SHIFT,
> >  				     VM_READ|VM_EXEC|
> >  				     VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC,
> >  				     vdso_pagelist);
> > -	if (rc) {
> > +	if (ret)
> >  		current->mm->context.vdso_base = 0;
> > -		goto fail_mmapsem;
> > -	}
> > -
> > -	up_write(&mm->mmap_sem);
> > -	return 0;
> >
> > - fail_mmapsem:
> > +out_up_mmap_sem:
> >  	up_write(&mm->mmap_sem);
> > -	return rc;
> > +	return ret;
> >  }
>
>
> If you strip out the variable renames then I think that change would be
> OK.
>
> cheers


--
             Dmitry