[06/11] pstree: make equal_pid handle sid comparison between nested pidnses

Submitted by Pavel Tikhomirov on May 26, 2017, 5:02 p.m.

Details

Message ID 20170526170254.17038-7-ptikhomirov@virtuozzo.com
State New
Series "rework init child-reaper reparent handling for pidnses"
Headers show

Commit Message

Pavel Tikhomirov May 26, 2017, 5:02 p.m.
If process belonging to some session is in different pidns than leader
of these session, it will have zeroes on all aditional levels in sid,
so though levels for these process and leader does not match - sids do.

Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
---
 criu/include/pid.h | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/include/pid.h b/criu/include/pid.h
index 72f3636..b25621a 100644
--- a/criu/include/pid.h
+++ b/criu/include/pid.h
@@ -46,20 +46,26 @@  struct pid {
 	} ns[1]; /* Must be at the end of struct pid */
 };
 
-#define equal_pid(a, b)							\
-({									\
-	int ___i, ___ret = true;					\
-	if (a->level == b->level) {					\
-		for (___i = 0; ___i < a->level; ___i++)			\
-			if (a->ns[___i].virt != b->ns[___i].virt) {	\
-				___ret = false;				\
-				___i = a->level; /* break */		\
-			}						\
-	} else {							\
-		pr_err("Wrong pid nesting level\n");			\
-		___ret = false;						\
-	}								\
-	___ret;								\
+#define equal_pid(a, b)								\
+({										\
+	int ___i, ___ret = true;						\
+	struct pid *___a = a, *___b = b;					\
+	if (a->level > b->level) {						\
+		___a = b;							\
+		___b = a;							\
+		}								\
+	for (___i = 0; ___i < ___b->level; ___i++)				\
+		if (___i < ___a->level) {					\
+			if (___a->ns[___i].virt != ___b->ns[___i].virt) {	\
+				___ret = false;					\
+				___i = ___b->level; /* break */			\
+				}						\
+			}							\
+		else if (___b->ns[___i].virt != 0) {				\
+			___ret = false;						\
+			___i = ___b->level; /* break */				\
+			}							\
+	___ret;									\
 })
 
 static inline pid_t last_level_pid(struct pid *pid)

Comments

Dmitry Safonov May 29, 2017, 11:20 a.m.
2017-05-26 20:02 GMT+03:00 Pavel Tikhomirov <ptikhomirov@virtuozzo.com>:
> If process belonging to some session is in different pidns than leader
> of these session, it will have zeroes on all aditional levels in sid,
> so though levels for these process and leader does not match - sids do.
>
> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
> ---
>  criu/include/pid.h | 34 ++++++++++++++++++++--------------
>  1 file changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/criu/include/pid.h b/criu/include/pid.h
> index 72f3636..b25621a 100644
> --- a/criu/include/pid.h
> +++ b/criu/include/pid.h
> @@ -46,20 +46,26 @@ struct pid {
>         } ns[1]; /* Must be at the end of struct pid */
>  };
>
> -#define equal_pid(a, b)                                                        \
> -({                                                                     \
> -       int ___i, ___ret = true;                                        \
> -       if (a->level == b->level) {                                     \
> -               for (___i = 0; ___i < a->level; ___i++)                 \
> -                       if (a->ns[___i].virt != b->ns[___i].virt) {     \
> -                               ___ret = false;                         \
> -                               ___i = a->level; /* break */            \
> -                       }                                               \
> -       } else {                                                        \
> -               pr_err("Wrong pid nesting level\n");                    \
> -               ___ret = false;                                         \
> -       }                                                               \
> -       ___ret;                                                         \
> +#define equal_pid(a, b)                                                                \
> +({                                                                             \
> +       int ___i, ___ret = true;                                                \
> +       struct pid *___a = a, *___b = b;                                        \
> +       if (a->level > b->level) {                                              \
> +               ___a = b;                                                       \
> +               ___b = a;                                                       \
> +               }                                                               \
> +       for (___i = 0; ___i < ___b->level; ___i++)                              \
> +               if (___i < ___a->level) {                                       \
> +                       if (___a->ns[___i].virt != ___b->ns[___i].virt) {       \
> +                               ___ret = false;                                 \
> +                               ___i = ___b->level; /* break */                 \
> +                               }                                               \
> +                       }                                                       \
> +               else if (___b->ns[___i].virt != 0) {                            \
> +                       ___ret = false;                                         \
> +                       ___i = ___b->level; /* break */                         \
> +                       }                                                       \
> +       ___ret;                                                                 \
>  })

I almost wish we hadn't gone down that macros-hole - and yet
and yet - it's rather curious, you know, this sort of life!

Is there any reason for it not to be a static inline function?

>
>  static inline pid_t last_level_pid(struct pid *pid)
> --
> 2.9.3
>
> _______________________________________________
> CRIU mailing list
> CRIU@openvz.org
> https://lists.openvz.org/mailman/listinfo/criu
Pavel Tikhomirov May 29, 2017, 11:43 a.m.
On 05/29/2017 02:20 PM, Dmitry Safonov wrote:
> 2017-05-26 20:02 GMT+03:00 Pavel Tikhomirov <ptikhomirov@virtuozzo.com>:
>> If process belonging to some session is in different pidns than leader
>> of these session, it will have zeroes on all aditional levels in sid,
>> so though levels for these process and leader does not match - sids do.
>>
>> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
>> ---
>>   criu/include/pid.h | 34 ++++++++++++++++++++--------------
>>   1 file changed, 20 insertions(+), 14 deletions(-)
>>
>> diff --git a/criu/include/pid.h b/criu/include/pid.h
>> index 72f3636..b25621a 100644
>> --- a/criu/include/pid.h
>> +++ b/criu/include/pid.h
>> @@ -46,20 +46,26 @@ struct pid {
>>          } ns[1]; /* Must be at the end of struct pid */
>>   };
>>
>> -#define equal_pid(a, b)                                                        \
>> -({                                                                     \
>> -       int ___i, ___ret = true;                                        \
>> -       if (a->level == b->level) {                                     \
>> -               for (___i = 0; ___i < a->level; ___i++)                 \
>> -                       if (a->ns[___i].virt != b->ns[___i].virt) {     \
>> -                               ___ret = false;                         \
>> -                               ___i = a->level; /* break */            \
>> -                       }                                               \
>> -       } else {                                                        \
>> -               pr_err("Wrong pid nesting level\n");                    \
>> -               ___ret = false;                                         \
>> -       }                                                               \
>> -       ___ret;                                                         \
>> +#define equal_pid(a, b)                                                                \
>> +({                                                                             \
>> +       int ___i, ___ret = true;                                                \
>> +       struct pid *___a = a, *___b = b;                                        \
>> +       if (a->level > b->level) {                                              \
>> +               ___a = b;                                                       \
>> +               ___b = a;                                                       \
>> +               }                                                               \
>> +       for (___i = 0; ___i < ___b->level; ___i++)                              \
>> +               if (___i < ___a->level) {                                       \
>> +                       if (___a->ns[___i].virt != ___b->ns[___i].virt) {       \
>> +                               ___ret = false;                                 \
>> +                               ___i = ___b->level; /* break */                 \
>> +                               }                                               \
>> +                       }                                                       \
>> +               else if (___b->ns[___i].virt != 0) {                            \
>> +                       ___ret = false;                                         \
>> +                       ___i = ___b->level; /* break */                         \
>> +                       }                                                       \
>> +       ___ret;                                                                 \
>>   })
> 
> I almost wish we hadn't gone down that macros-hole - and yet
> and yet - it's rather curious, you know, this sort of life!
> 
> Is there any reason for it not to be a static inline function?

I can see any, I left it as a macros as it was the macros before :)

> 
>>
>>   static inline pid_t last_level_pid(struct pid *pid)
>> --
>> 2.9.3
>>
>> _______________________________________________
>> CRIU mailing list
>> CRIU@openvz.org
>> https://lists.openvz.org/mailman/listinfo/criu
> 
> 
>
Kirill Tkhai May 29, 2017, noon
On 29.05.2017 14:43, Pavel Tikhomirov wrote:
> 
> 
> On 05/29/2017 02:20 PM, Dmitry Safonov wrote:
>> 2017-05-26 20:02 GMT+03:00 Pavel Tikhomirov <ptikhomirov@virtuozzo.com>:
>>> If process belonging to some session is in different pidns than leader
>>> of these session, it will have zeroes on all aditional levels in sid,
>>> so though levels for these process and leader does not match - sids do.
>>>
>>> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
>>> ---
>>>   criu/include/pid.h | 34 ++++++++++++++++++++--------------
>>>   1 file changed, 20 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/criu/include/pid.h b/criu/include/pid.h
>>> index 72f3636..b25621a 100644
>>> --- a/criu/include/pid.h
>>> +++ b/criu/include/pid.h
>>> @@ -46,20 +46,26 @@ struct pid {
>>>          } ns[1]; /* Must be at the end of struct pid */
>>>   };
>>>
>>> -#define equal_pid(a, b)                                                        \
>>> -({                                                                     \
>>> -       int ___i, ___ret = true;                                        \
>>> -       if (a->level == b->level) {                                     \
>>> -               for (___i = 0; ___i < a->level; ___i++)                 \
>>> -                       if (a->ns[___i].virt != b->ns[___i].virt) {     \
>>> -                               ___ret = false;                         \
>>> -                               ___i = a->level; /* break */            \
>>> -                       }                                               \
>>> -       } else {                                                        \
>>> -               pr_err("Wrong pid nesting level\n");                    \
>>> -               ___ret = false;                                         \
>>> -       }                                                               \
>>> -       ___ret;                                                         \
>>> +#define equal_pid(a, b)                                                                \
>>> +({                                                                             \
>>> +       int ___i, ___ret = true;                                                \
>>> +       struct pid *___a = a, *___b = b;                                        \
>>> +       if (a->level > b->level) {                                              \
>>> +               ___a = b;                                                       \
>>> +               ___b = a;                                                       \
>>> +               }                                                               \
>>> +       for (___i = 0; ___i < ___b->level; ___i++)                              \
>>> +               if (___i < ___a->level) {                                       \
>>> +                       if (___a->ns[___i].virt != ___b->ns[___i].virt) {       \
>>> +                               ___ret = false;                                 \
>>> +                               ___i = ___b->level; /* break */                 \
>>> +                               }                                               \
>>> +                       }                                                       \
>>> +               else if (___b->ns[___i].virt != 0) {                            \
>>> +                       ___ret = false;                                         \
>>> +                       ___i = ___b->level; /* break */                         \
>>> +                       }                                                       \
>>> +       ___ret;                                                                 \
>>>   })
>>
>> I almost wish we hadn't gone down that macros-hole - and yet
>> and yet - it's rather curious, you know, this sort of life!
>>
>> Is there any reason for it not to be a static inline function?
> 
> I can see any, I left it as a macros as it was the macros before :)

The reason is simple: there was a pr_err() used to print __LINE__.
So, a caller of equal_pid() does not need to use additional print
in error case.

Now you don't have, and it may be safely converted to function.