[PATCH] userns: simplify map_id_range_* functions

[PATCH] userns: simplify map_id_range_* functions

[Nicolas Iooss]

Functions map_id_range_down, map_id_down and map_id_up all used the
construction:

    if (...)
        id = ...
    else
        id = ...
    return id;

which can be simplified by directly returning the result of the
computations in each branch.

Moreover as the condition tested whether the "break;" in the previous
for loop was hit, it is simpler to directly compute the result and
return it.

Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
---

As I could not find any relevant entry for kernel/user_namespace.c in
MAINTAINERS, I have built the list of recipients from the git history.
Please let me know if I was expected to proceed differently to submit
this patch.

 kernel/user_namespace.c | 31 +++++++------------------------
 1 file changed, 7 insertions(+), 24 deletions(-)

diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index 4109f8320684..bf063dc6b8d4 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -165,15 +165,10 @@ static u32 map_id_range_down(struct uid_gid_map *map, u32 id, u32 count)
 		last = first + map->extent[idx].count - 1;
 		if (id >= first && id <= last &&
 		    (id2 >= first && id2 <= last))
-			break;
+			return (id - first) + map->extent[idx].lower_first;
 	}
-	/* Map the id or note failure */
-	if (idx < extents)
-		id = (id - first) + map->extent[idx].lower_first;
-	else
-		id = (u32) -1;
-
-	return id;
+	/* Note failure */
+	return (u32) -1;
 }
 
 static u32 map_id_down(struct uid_gid_map *map, u32 id)
@@ -188,15 +183,9 @@ static u32 map_id_down(struct uid_gid_map *map, u32 id)
 		first = map->extent[idx].first;
 		last = first + map->extent[idx].count - 1;
 		if (id >= first && id <= last)
-			break;
+			return (id - first) + map->extent[idx].lower_first;
 	}
-	/* Map the id or note failure */
-	if (idx < extents)
-		id = (id - first) + map->extent[idx].lower_first;
-	else
-		id = (u32) -1;
-
-	return id;
+	return (u32) -1;
 }
 
 static u32 map_id_up(struct uid_gid_map *map, u32 id)
@@ -211,15 +200,9 @@ static u32 map_id_up(struct uid_gid_map *map, u32 id)
 		first = map->extent[idx].lower_first;
 		last = first + map->extent[idx].count - 1;
 		if (id >= first && id <= last)
-			break;
+			return (id - first) + map->extent[idx].first;
 	}
-	/* Map the id or note failure */
-	if (idx < extents)
-		id = (id - first) + map->extent[idx].first;
-	else
-		id = (u32) -1;
-
-	return id;
+	return (u32) -1;
 }
 
 /**
-- 
2.4.6

Re: [PATCH] userns: simplify map_id_range_* functions

[Eric W. Biederman]

Nicolas Iooss <nicolas.iooss_linux@m4x.org> writes:

> Functions map_id_range_down, map_id_down and map_id_up all used the
> construction:
>
>     if (...)
>         id = ...
>     else
>         id = ...
>     return id;
>
> which can be simplified by directly returning the result of the
> computations in each branch.
>
> Moreover as the condition tested whether the "break;" in the previous
> for loop was hit, it is simpler to directly compute the result and
> return it.

It is not a simplification, it is just code motion.

Further at least to my eyes adding multiple exit points and setting the
same value in two different places actually obscures what the functions
are doing.

If we could talk about speeding up the performance of the stat system
call I think there would be a point in mucking with these functions.

As it is I think it is I think merging your patch will just make it more
difficult to understand what the code is doing in the future, with no 
benefit except a reduction in line count.

Eric

Re: [PATCH] userns: simplify map_id_range_* functions

[Nicolas Iooss]

On 07/27/2015 12:29 PM, Eric W. Biederman wrote:
> Nicolas Iooss <nicolas.iooss_linux@m4x.org> writes:
> 
>> Functions map_id_range_down, map_id_down and map_id_up all used the
>> construction:
>>
>>     if (...)
>>         id = ...
>>     else
>>         id = ...
>>     return id;
>>
>> which can be simplified by directly returning the result of the
>> computations in each branch.
>>
>> Moreover as the condition tested whether the "break;" in the previous
>> for loop was hit, it is simpler to directly compute the result and
>> return it.
> 
> It is not a simplification, it is just code motion.

I agree.  Also on my system (x86_64 kernel with Arch Linux +
CONFIG_USERNS configuration), the assembly code generated either by gcc
5.2.0 or by clang 3.6.2 (with LLVMLinux patches) does not change at all
with this patch.  So there would be absolutely no performance change or
similar things which could motivate this change.

> Further at least to my eyes adding multiple exit points and setting the
> same value in two different places actually obscures what the functions
> are doing.

I did not understand what "setting the same value in two different
places" refers to.  Anyway, all right.  I haven't got much experience
about code maintenance so if you say my patch make things less clear, I
believe you.

> If we could talk about speeding up the performance of the stat system
> call I think there would be a point in mucking with these functions.
> 
> As it is I think it is I think merging your patch will just make it more
> difficult to understand what the code is doing in the future, with no 
> benefit except a reduction in line count.

OK.  My intention was precisely to make the code easier to understand
(because I spent some time before I understood there really were only
two cases: existing mapping or not) but if you think my patch does the
opposite, I accept dropping it.

Thanks for your comments.

Nicolas

Re: [PATCH] userns: simplify map_id_range_* functions

[Eric W. Biederman]

Nicolas Iooss <nicolas.iooss_linux@m4x.org> writes:

> On 07/27/2015 12:29 PM, Eric W. Biederman wrote:
>> Nicolas Iooss <nicolas.iooss_linux@m4x.org> writes:
>> 
>>> Functions map_id_range_down, map_id_down and map_id_up all used the
>>> construction:
>>>
>>>     if (...)
>>>         id = ...
>>>     else
>>>         id = ...
>>>     return id;
>>>
>>> which can be simplified by directly returning the result of the
>>> computations in each branch.
>>>
>>> Moreover as the condition tested whether the "break;" in the previous
>>> for loop was hit, it is simpler to directly compute the result and
>>> return it.
>> 
>> It is not a simplification, it is just code motion.
>
> I agree.  Also on my system (x86_64 kernel with Arch Linux +
> CONFIG_USERNS configuration), the assembly code generated either by gcc
> 5.2.0 or by clang 3.6.2 (with LLVMLinux patches) does not change at all
> with this patch.  So there would be absolutely no performance change or
> similar things which could motivate this change.

Thank you for that information.

>> Further at least to my eyes adding multiple exit points and setting the
>> same value in two different places actually obscures what the functions
>> are doing.
>
> I did not understand what "setting the same value in two different
> places" refers to.  Anyway, all right.  I haven't got much experience
> about code maintenance so if you say my patch make things less clear, I
> believe you.

I was referring to computing the value of id that is returned.

In this case I think what is important is that it makes the code clear
for me, as I have to maintain it.  That doesn't go so far as to
obscure it for everyone but on little issues where it is a judgement
call making it easy for the maintainer is good call.

I really appreciate that the setting of id is concentrated into just a
couple of adjacent lines.  That makes it easy to reason about the
value that the variable id has.

Additionally in some sense it is important to be cautious about small
simple code changes as they are so trivial it is natural to relax and
not test or examine such changes as closely and that is when bugs slip
in.

In a general sense there also remains the point made in "Gotos
Considered Harmful" that functions with multiple exit points are more
difficult to reason about.  Dykstra was pretty sharp.

>> If we could talk about speeding up the performance of the stat system
>> call I think there would be a point in mucking with these functions.
>> 
>> As it is I think it is I think merging your patch will just make it more
>> difficult to understand what the code is doing in the future, with no 
>> benefit except a reduction in line count.
>
> OK.  My intention was precisely to make the code easier to understand
> (because I spent some time before I understood there really were only
> two cases: existing mapping or not) but if you think my patch does the
> opposite, I accept dropping it.
>
> Thanks for your comments.

Welcome and thank you for pointing out how you figured the code could be
made clearer.

Eric