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

[Dmitry Safonov <dsafonov@virtuozzo.com>]

On 11/08/2016 03:10 AM, Michael Ellerman wrote:
> Hi Dmitry,
>
> Thanks for the patches.
>
> Dmitry Safonov <dsafonov@virtuozzo.com> 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@kernel.crashing.org>
>> Cc: Paul Mackerras <paulus@samba.org>
>> 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 <dsafonov@virtuozzo.com>
>> ---
>>  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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>