Re: [PATCHv3 1/8] powerpc/vdso: unify return paths in setup_additional_pages
From: Michael Ellerman <mpe@ellerman.id.au>
Date: 2016-11-08 00:10:32
Also in:
linux-mm, lkml
Hi Dmitry, Thanks for the patches. Dmitry Safonov [off-list ref] 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.
quoted hunk ↗ jump to hunk
Combine two function returns into one as it's also easier to read. Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> Cc: Paul Mackerras <redacted> Cc: Michael Ellerman <mpe@ellerman.id.au> Cc: Andy Lutomirski <luto@amacapital.net> Cc: Oleg Nesterov <oleg@redhat.com> Cc: linuxppc-dev@lists.ozlabs.org Cc: linux-mm@kvack.org Signed-off-by: Dmitry Safonov <redacted> --- 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.
quoted hunk ↗ jump to hunk
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