[v2,5/5] compel: Add nspid, nssid and nspgid fields to struct seize_task_status

Submitted by Kirill Tkhai on April 18, 2017, 3:57 p.m.

Details

Message ID 149253102796.15051.1723556388388431632.stgit@localhost.localdomain
State New
Series "compel, criu: Make struct seize_task_status extensible"
Headers show

Commit Message

Kirill Tkhai April 18, 2017, 3:57 p.m.
Some users want them, so add these fields to the structure.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 compel/include/uapi/infect.h |    4 ++++
 compel/src/lib/infect.c      |    6 ++++++
 2 files changed, 10 insertions(+)

Patch hide | download patch | download mbox

diff --git a/compel/include/uapi/infect.h b/compel/include/uapi/infect.h
index 7b681b0cb..4c6ab413d 100644
--- a/compel/include/uapi/infect.h
+++ b/compel/include/uapi/infect.h
@@ -21,6 +21,10 @@  struct seize_task_status {
 	int			seccomp_mode;
 	unsigned long long	shdpnd;
 	unsigned long long	sigpnd;
+	size_t			ns_levels;
+	uint32_t		*nspid;
+	uint32_t		*nspgid;
+	uint32_t		*nssid;
 	/* Add new members to the bottom and do not change existing */
 };
 
diff --git a/compel/src/lib/infect.c b/compel/src/lib/infect.c
index 9ade844cf..ab7558a81 100644
--- a/compel/src/lib/infect.c
+++ b/compel/src/lib/infect.c
@@ -190,11 +190,17 @@  static int skip_sigstop(int pid, int nr_signals)
 /* Init dynamically allocated fields in NULL and do not touch other */
 static void init_seize_task_status(struct seize_task_status *ss)
 {
+	ss->ns_levels = 0;
+	ss->nspid = ss->nspgid = ss->nssid = NULL;
 }
 
 /* Free dynamically allocated fields in compel_wait_task() and do not touch other */
 void compel_consume_seize_task_status(struct seize_task_status *ss)
 {
+	xfree(ss->nspid);
+	xfree(ss->nspgid);
+	xfree(ss->nssid);
+
 	init_seize_task_status(ss);
 }
 

Comments

Andrey Vagin April 29, 2017, 6:03 a.m.
On Tue, Apr 18, 2017 at 06:57:07PM +0300, Kirill Tkhai wrote:
> Some users want them, so add these fields to the structure.
> 
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
>  compel/include/uapi/infect.h |    4 ++++
>  compel/src/lib/infect.c      |    6 ++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/compel/include/uapi/infect.h b/compel/include/uapi/infect.h
> index 7b681b0cb..4c6ab413d 100644
> --- a/compel/include/uapi/infect.h
> +++ b/compel/include/uapi/infect.h
> @@ -21,6 +21,10 @@ struct seize_task_status {
>  	int			seccomp_mode;
>  	unsigned long long	shdpnd;
>  	unsigned long long	sigpnd;
> +	size_t			ns_levels;
> +	uint32_t		*nspid;
> +	uint32_t		*nspgid;
> +	uint32_t		*nssid;

Here is the compel code and now we try to add fields which are
not used in compel. It doesn't look good.


>  	/* Add new members to the bottom and do not change existing */
>  };
>  
> diff --git a/compel/src/lib/infect.c b/compel/src/lib/infect.c
> index 9ade844cf..ab7558a81 100644
> --- a/compel/src/lib/infect.c
> +++ b/compel/src/lib/infect.c
> @@ -190,11 +190,17 @@ static int skip_sigstop(int pid, int nr_signals)
>  /* Init dynamically allocated fields in NULL and do not touch other */
>  static void init_seize_task_status(struct seize_task_status *ss)
>  {
> +	ss->ns_levels = 0;
> +	ss->nspid = ss->nspgid = ss->nssid = NULL;
>  }
>  
>  /* Free dynamically allocated fields in compel_wait_task() and do not touch other */
>  void compel_consume_seize_task_status(struct seize_task_status *ss)
>  {
> +	xfree(ss->nspid);
> +	xfree(ss->nspgid);
> +	xfree(ss->nssid);
> +
>  	init_seize_task_status(ss);
>  }
>  
>
Andrey Vagin April 29, 2017, 6:12 a.m.
On Fri, Apr 28, 2017 at 11:03:08PM -0700, Andrei Vagin wrote:
> On Tue, Apr 18, 2017 at 06:57:07PM +0300, Kirill Tkhai wrote:
> > Some users want them, so add these fields to the structure.
> > 
> > Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> > ---
> >  compel/include/uapi/infect.h |    4 ++++
> >  compel/src/lib/infect.c      |    6 ++++++
> >  2 files changed, 10 insertions(+)
> > 
> > diff --git a/compel/include/uapi/infect.h b/compel/include/uapi/infect.h
> > index 7b681b0cb..4c6ab413d 100644
> > --- a/compel/include/uapi/infect.h
> > +++ b/compel/include/uapi/infect.h
> > @@ -21,6 +21,10 @@ struct seize_task_status {
> >  	int			seccomp_mode;
> >  	unsigned long long	shdpnd;
> >  	unsigned long long	sigpnd;
> > +	size_t			ns_levels;
> > +	uint32_t		*nspid;
> > +	uint32_t		*nspgid;
> > +	uint32_t		*nssid;
> 
> Here is the compel code and now we try to add fields which are
> not used in compel. It doesn't look good.

Maybe we need to declare a new structure in criu which will encapsulate
seize_task_status?

> 
> 
> >  	/* Add new members to the bottom and do not change existing */
> >  };
> >  
> > diff --git a/compel/src/lib/infect.c b/compel/src/lib/infect.c
> > index 9ade844cf..ab7558a81 100644
> > --- a/compel/src/lib/infect.c
> > +++ b/compel/src/lib/infect.c
> > @@ -190,11 +190,17 @@ static int skip_sigstop(int pid, int nr_signals)
> >  /* Init dynamically allocated fields in NULL and do not touch other */
> >  static void init_seize_task_status(struct seize_task_status *ss)
> >  {
> > +	ss->ns_levels = 0;
> > +	ss->nspid = ss->nspgid = ss->nssid = NULL;
> >  }
> >  
> >  /* Free dynamically allocated fields in compel_wait_task() and do not touch other */
> >  void compel_consume_seize_task_status(struct seize_task_status *ss)
> >  {
> > +	xfree(ss->nspid);
> > +	xfree(ss->nspgid);
> > +	xfree(ss->nssid);
> > +
> >  	init_seize_task_status(ss);
> >  }
> >  
> >
Pavel Emelianov May 2, 2017, 8:58 a.m.
On 04/29/2017 09:12 AM, Andrei Vagin wrote:
> On Fri, Apr 28, 2017 at 11:03:08PM -0700, Andrei Vagin wrote:
>> On Tue, Apr 18, 2017 at 06:57:07PM +0300, Kirill Tkhai wrote:
>>> Some users want them, so add these fields to the structure.
>>>
>>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>>> ---
>>>  compel/include/uapi/infect.h |    4 ++++
>>>  compel/src/lib/infect.c      |    6 ++++++
>>>  2 files changed, 10 insertions(+)
>>>
>>> diff --git a/compel/include/uapi/infect.h b/compel/include/uapi/infect.h
>>> index 7b681b0cb..4c6ab413d 100644
>>> --- a/compel/include/uapi/infect.h
>>> +++ b/compel/include/uapi/infect.h
>>> @@ -21,6 +21,10 @@ struct seize_task_status {
>>>  	int			seccomp_mode;
>>>  	unsigned long long	shdpnd;
>>>  	unsigned long long	sigpnd;
>>> +	size_t			ns_levels;
>>> +	uint32_t		*nspid;
>>> +	uint32_t		*nspgid;
>>> +	uint32_t		*nssid;
>>
>> Here is the compel code and now we try to add fields which are
>> not used in compel. It doesn't look good.
> 
> Maybe we need to declare a new structure in criu which will encapsulate
> seize_task_status?

We already have one ;) Called proc_status_creds.

>>
>>
>>>  	/* Add new members to the bottom and do not change existing */
>>>  };
>>>  
>>> diff --git a/compel/src/lib/infect.c b/compel/src/lib/infect.c
>>> index 9ade844cf..ab7558a81 100644
>>> --- a/compel/src/lib/infect.c
>>> +++ b/compel/src/lib/infect.c
>>> @@ -190,11 +190,17 @@ static int skip_sigstop(int pid, int nr_signals)
>>>  /* Init dynamically allocated fields in NULL and do not touch other */
>>>  static void init_seize_task_status(struct seize_task_status *ss)
>>>  {
>>> +	ss->ns_levels = 0;
>>> +	ss->nspid = ss->nspgid = ss->nssid = NULL;
>>>  }
>>>  
>>>  /* Free dynamically allocated fields in compel_wait_task() and do not touch other */
>>>  void compel_consume_seize_task_status(struct seize_task_status *ss)
>>>  {
>>> +	xfree(ss->nspid);
>>> +	xfree(ss->nspgid);
>>> +	xfree(ss->nssid);
>>> +
>>>  	init_seize_task_status(ss);
>>>  }
>>>  
>>>
> .
>
Kirill Tkhai May 2, 2017, 9:13 a.m.
On 29.04.2017 09:12, Andrei Vagin wrote:
> On Fri, Apr 28, 2017 at 11:03:08PM -0700, Andrei Vagin wrote:
>> On Tue, Apr 18, 2017 at 06:57:07PM +0300, Kirill Tkhai wrote:
>>> Some users want them, so add these fields to the structure.
>>>
>>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>>> ---
>>>  compel/include/uapi/infect.h |    4 ++++
>>>  compel/src/lib/infect.c      |    6 ++++++
>>>  2 files changed, 10 insertions(+)
>>>
>>> diff --git a/compel/include/uapi/infect.h b/compel/include/uapi/infect.h
>>> index 7b681b0cb..4c6ab413d 100644
>>> --- a/compel/include/uapi/infect.h
>>> +++ b/compel/include/uapi/infect.h
>>> @@ -21,6 +21,10 @@ struct seize_task_status {
>>>  	int			seccomp_mode;
>>>  	unsigned long long	shdpnd;
>>>  	unsigned long long	sigpnd;
>>> +	size_t			ns_levels;
>>> +	uint32_t		*nspid;
>>> +	uint32_t		*nspgid;
>>> +	uint32_t		*nssid;
>>
>> Here is the compel code and now we try to add fields which are
>> not used in compel. It doesn't look good.
> 
> Maybe we need to declare a new structure in criu which will encapsulate
> seize_task_status?

Look at the description of patch [3/5]. It's explained there, why I go this
way. A method get_status() of function compel_wait_task() may be called
twice, and the allocated fields must be freed before the second call.

Since different methods get_status() may allocate different arguments,
if we do not go my way, then we need either appropriate free_status()
methods (which is more complex) or to free the previously allocated
status in get_status() on the second call (which is ugly).

I suggest generic method, suitable for everything, which does not make
the code complicate and which is easily extensible for new fields.

I don't agree, it looks bad. Also, there is no a rule that a library
must not contain more methods and interfaces, when it's need for itself.
 
>>
>>
>>>  	/* Add new members to the bottom and do not change existing */
>>>  };
>>>  
>>> diff --git a/compel/src/lib/infect.c b/compel/src/lib/infect.c
>>> index 9ade844cf..ab7558a81 100644
>>> --- a/compel/src/lib/infect.c
>>> +++ b/compel/src/lib/infect.c
>>> @@ -190,11 +190,17 @@ static int skip_sigstop(int pid, int nr_signals)
>>>  /* Init dynamically allocated fields in NULL and do not touch other */
>>>  static void init_seize_task_status(struct seize_task_status *ss)
>>>  {
>>> +	ss->ns_levels = 0;
>>> +	ss->nspid = ss->nspgid = ss->nssid = NULL;
>>>  }
>>>  
>>>  /* Free dynamically allocated fields in compel_wait_task() and do not touch other */
>>>  void compel_consume_seize_task_status(struct seize_task_status *ss)
>>>  {
>>> +	xfree(ss->nspid);
>>> +	xfree(ss->nspgid);
>>> +	xfree(ss->nssid);
>>> +
>>>  	init_seize_task_status(ss);
>>>  }
>>>  
>>>
Kirill Tkhai May 2, 2017, 9:18 a.m.
On 02.05.2017 11:58, Pavel Emelyanov wrote:
> On 04/29/2017 09:12 AM, Andrei Vagin wrote:
>> On Fri, Apr 28, 2017 at 11:03:08PM -0700, Andrei Vagin wrote:
>>> On Tue, Apr 18, 2017 at 06:57:07PM +0300, Kirill Tkhai wrote:
>>>> Some users want them, so add these fields to the structure.
>>>>
>>>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>>>> ---
>>>>  compel/include/uapi/infect.h |    4 ++++
>>>>  compel/src/lib/infect.c      |    6 ++++++
>>>>  2 files changed, 10 insertions(+)
>>>>
>>>> diff --git a/compel/include/uapi/infect.h b/compel/include/uapi/infect.h
>>>> index 7b681b0cb..4c6ab413d 100644
>>>> --- a/compel/include/uapi/infect.h
>>>> +++ b/compel/include/uapi/infect.h
>>>> @@ -21,6 +21,10 @@ struct seize_task_status {
>>>>  	int			seccomp_mode;
>>>>  	unsigned long long	shdpnd;
>>>>  	unsigned long long	sigpnd;
>>>> +	size_t			ns_levels;
>>>> +	uint32_t		*nspid;
>>>> +	uint32_t		*nspgid;
>>>> +	uint32_t		*nssid;
>>>
>>> Here is the compel code and now we try to add fields which are
>>> not used in compel. It doesn't look good.
>>
>> Maybe we need to declare a new structure in criu which will encapsulate
>> seize_task_status?
> 
> We already have one ;) Called proc_status_creds.

struct proc_status_creds is a mix entities of different types collected
from different files. seize_task_status is the only structure for
/proc/[pid]/status file, all content of this structure is collected
from the single file. It's different things.
 
>>>
>>>
>>>>  	/* Add new members to the bottom and do not change existing */
>>>>  };
>>>>  
>>>> diff --git a/compel/src/lib/infect.c b/compel/src/lib/infect.c
>>>> index 9ade844cf..ab7558a81 100644
>>>> --- a/compel/src/lib/infect.c
>>>> +++ b/compel/src/lib/infect.c
>>>> @@ -190,11 +190,17 @@ static int skip_sigstop(int pid, int nr_signals)
>>>>  /* Init dynamically allocated fields in NULL and do not touch other */
>>>>  static void init_seize_task_status(struct seize_task_status *ss)
>>>>  {
>>>> +	ss->ns_levels = 0;
>>>> +	ss->nspid = ss->nspgid = ss->nssid = NULL;
>>>>  }
>>>>  
>>>>  /* Free dynamically allocated fields in compel_wait_task() and do not touch other */
>>>>  void compel_consume_seize_task_status(struct seize_task_status *ss)
>>>>  {
>>>> +	xfree(ss->nspid);
>>>> +	xfree(ss->nspgid);
>>>> +	xfree(ss->nssid);
>>>> +
>>>>  	init_seize_task_status(ss);
>>>>  }
>>>>  
>>>>
>> .
>>
>
Kirill Tkhai May 2, 2017, 9:30 a.m.
On 02.05.2017 12:13, Kirill Tkhai wrote:
> On 29.04.2017 09:12, Andrei Vagin wrote:
>> On Fri, Apr 28, 2017 at 11:03:08PM -0700, Andrei Vagin wrote:
>>> On Tue, Apr 18, 2017 at 06:57:07PM +0300, Kirill Tkhai wrote:
>>>> Some users want them, so add these fields to the structure.
>>>>
>>>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>>>> ---
>>>>  compel/include/uapi/infect.h |    4 ++++
>>>>  compel/src/lib/infect.c      |    6 ++++++
>>>>  2 files changed, 10 insertions(+)
>>>>
>>>> diff --git a/compel/include/uapi/infect.h b/compel/include/uapi/infect.h
>>>> index 7b681b0cb..4c6ab413d 100644
>>>> --- a/compel/include/uapi/infect.h
>>>> +++ b/compel/include/uapi/infect.h
>>>> @@ -21,6 +21,10 @@ struct seize_task_status {
>>>>  	int			seccomp_mode;
>>>>  	unsigned long long	shdpnd;
>>>>  	unsigned long long	sigpnd;
>>>> +	size_t			ns_levels;
>>>> +	uint32_t		*nspid;
>>>> +	uint32_t		*nspgid;
>>>> +	uint32_t		*nssid;
>>>
>>> Here is the compel code and now we try to add fields which are
>>> not used in compel. It doesn't look good.
>>
>> Maybe we need to declare a new structure in criu which will encapsulate
>> seize_task_status?
> 
> Look at the description of patch [3/5]. It's explained there, why I go this
> way. A method get_status() of function compel_wait_task() may be called
> twice, and the allocated fields must be freed before the second call.
> 
> Since different methods get_status() may allocate different arguments,
> if we do not go my way, then we need either appropriate free_status()
> methods (which is more complex) or to free the previously allocated
> status in get_status() on the second call (which is ugly).
> 
> I suggest generic method, suitable for everything, which does not make
> the code complicate and which is easily extensible for new fields.
> 
> I don't agree, it looks bad. Also, there is no a rule that a library
> must not contain more methods and interfaces, when it's need for itself.

+ It's why a library is need at all.
  
>>>
>>>
>>>>  	/* Add new members to the bottom and do not change existing */
>>>>  };
>>>>  
>>>> diff --git a/compel/src/lib/infect.c b/compel/src/lib/infect.c
>>>> index 9ade844cf..ab7558a81 100644
>>>> --- a/compel/src/lib/infect.c
>>>> +++ b/compel/src/lib/infect.c
>>>> @@ -190,11 +190,17 @@ static int skip_sigstop(int pid, int nr_signals)
>>>>  /* Init dynamically allocated fields in NULL and do not touch other */
>>>>  static void init_seize_task_status(struct seize_task_status *ss)
>>>>  {
>>>> +	ss->ns_levels = 0;
>>>> +	ss->nspid = ss->nspgid = ss->nssid = NULL;
>>>>  }
>>>>  
>>>>  /* Free dynamically allocated fields in compel_wait_task() and do not touch other */
>>>>  void compel_consume_seize_task_status(struct seize_task_status *ss)
>>>>  {
>>>> +	xfree(ss->nspid);
>>>> +	xfree(ss->nspgid);
>>>> +	xfree(ss->nssid);
>>>> +
>>>>  	init_seize_task_status(ss);
>>>>  }
>>>>  
>>>>
Pavel Emelianov May 2, 2017, 10:06 a.m.
On 05/02/2017 12:18 PM, Kirill Tkhai wrote:
> On 02.05.2017 11:58, Pavel Emelyanov wrote:
>> On 04/29/2017 09:12 AM, Andrei Vagin wrote:
>>> On Fri, Apr 28, 2017 at 11:03:08PM -0700, Andrei Vagin wrote:
>>>> On Tue, Apr 18, 2017 at 06:57:07PM +0300, Kirill Tkhai wrote:
>>>>> Some users want them, so add these fields to the structure.
>>>>>
>>>>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>>>>> ---
>>>>>  compel/include/uapi/infect.h |    4 ++++
>>>>>  compel/src/lib/infect.c      |    6 ++++++
>>>>>  2 files changed, 10 insertions(+)
>>>>>
>>>>> diff --git a/compel/include/uapi/infect.h b/compel/include/uapi/infect.h
>>>>> index 7b681b0cb..4c6ab413d 100644
>>>>> --- a/compel/include/uapi/infect.h
>>>>> +++ b/compel/include/uapi/infect.h
>>>>> @@ -21,6 +21,10 @@ struct seize_task_status {
>>>>>  	int			seccomp_mode;
>>>>>  	unsigned long long	shdpnd;
>>>>>  	unsigned long long	sigpnd;
>>>>> +	size_t			ns_levels;
>>>>> +	uint32_t		*nspid;
>>>>> +	uint32_t		*nspgid;
>>>>> +	uint32_t		*nssid;
>>>>
>>>> Here is the compel code and now we try to add fields which are
>>>> not used in compel. It doesn't look good.
>>>
>>> Maybe we need to declare a new structure in criu which will encapsulate
>>> seize_task_status?
>>
>> We already have one ;) Called proc_status_creds.
> 
> struct proc_status_creds is a mix entities of different types collected
> from different files. seize_task_status is the only structure for
> /proc/[pid]/status file, all content of this structure is collected
> from the single file. It's different things.

proc_status_creds is also filled only with values collected from /proc/pid/status,
nothing more.

>>>>
>>>>
>>>>>  	/* Add new members to the bottom and do not change existing */
>>>>>  };
>>>>>  
>>>>> diff --git a/compel/src/lib/infect.c b/compel/src/lib/infect.c
>>>>> index 9ade844cf..ab7558a81 100644
>>>>> --- a/compel/src/lib/infect.c
>>>>> +++ b/compel/src/lib/infect.c
>>>>> @@ -190,11 +190,17 @@ static int skip_sigstop(int pid, int nr_signals)
>>>>>  /* Init dynamically allocated fields in NULL and do not touch other */
>>>>>  static void init_seize_task_status(struct seize_task_status *ss)
>>>>>  {
>>>>> +	ss->ns_levels = 0;
>>>>> +	ss->nspid = ss->nspgid = ss->nssid = NULL;
>>>>>  }
>>>>>  
>>>>>  /* Free dynamically allocated fields in compel_wait_task() and do not touch other */
>>>>>  void compel_consume_seize_task_status(struct seize_task_status *ss)
>>>>>  {
>>>>> +	xfree(ss->nspid);
>>>>> +	xfree(ss->nspgid);
>>>>> +	xfree(ss->nssid);
>>>>> +
>>>>>  	init_seize_task_status(ss);
>>>>>  }
>>>>>  
>>>>>
>>> .
>>>
>>
> .
>
Kirill Tkhai May 2, 2017, 11:36 a.m.
On 02.05.2017 13:06, Pavel Emelyanov wrote:
> On 05/02/2017 12:18 PM, Kirill Tkhai wrote:
>> On 02.05.2017 11:58, Pavel Emelyanov wrote:
>>> On 04/29/2017 09:12 AM, Andrei Vagin wrote:
>>>> On Fri, Apr 28, 2017 at 11:03:08PM -0700, Andrei Vagin wrote:
>>>>> On Tue, Apr 18, 2017 at 06:57:07PM +0300, Kirill Tkhai wrote:
>>>>>> Some users want them, so add these fields to the structure.
>>>>>>
>>>>>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>>>>>> ---
>>>>>>  compel/include/uapi/infect.h |    4 ++++
>>>>>>  compel/src/lib/infect.c      |    6 ++++++
>>>>>>  2 files changed, 10 insertions(+)
>>>>>>
>>>>>> diff --git a/compel/include/uapi/infect.h b/compel/include/uapi/infect.h
>>>>>> index 7b681b0cb..4c6ab413d 100644
>>>>>> --- a/compel/include/uapi/infect.h
>>>>>> +++ b/compel/include/uapi/infect.h
>>>>>> @@ -21,6 +21,10 @@ struct seize_task_status {
>>>>>>  	int			seccomp_mode;
>>>>>>  	unsigned long long	shdpnd;
>>>>>>  	unsigned long long	sigpnd;
>>>>>> +	size_t			ns_levels;
>>>>>> +	uint32_t		*nspid;
>>>>>> +	uint32_t		*nspgid;
>>>>>> +	uint32_t		*nssid;
>>>>>
>>>>> Here is the compel code and now we try to add fields which are
>>>>> not used in compel. It doesn't look good.
>>>>
>>>> Maybe we need to declare a new structure in criu which will encapsulate
>>>> seize_task_status?
>>>
>>> We already have one ;) Called proc_status_creds.
>>
>> struct proc_status_creds is a mix entities of different types collected
>> from different files. seize_task_status is the only structure for
>> /proc/[pid]/status file, all content of this structure is collected
>> from the single file. It's different things.
> 
> proc_status_creds is also filled only with values collected from /proc/pid/status,
> nothing more.

But it has only statically stored values, and it has no problems like the problems
solved in this patchset.
 
>>>>>
>>>>>
>>>>>>  	/* Add new members to the bottom and do not change existing */
>>>>>>  };
>>>>>>  
>>>>>> diff --git a/compel/src/lib/infect.c b/compel/src/lib/infect.c
>>>>>> index 9ade844cf..ab7558a81 100644
>>>>>> --- a/compel/src/lib/infect.c
>>>>>> +++ b/compel/src/lib/infect.c
>>>>>> @@ -190,11 +190,17 @@ static int skip_sigstop(int pid, int nr_signals)
>>>>>>  /* Init dynamically allocated fields in NULL and do not touch other */
>>>>>>  static void init_seize_task_status(struct seize_task_status *ss)
>>>>>>  {
>>>>>> +	ss->ns_levels = 0;
>>>>>> +	ss->nspid = ss->nspgid = ss->nssid = NULL;
>>>>>>  }
>>>>>>  
>>>>>>  /* Free dynamically allocated fields in compel_wait_task() and do not touch other */
>>>>>>  void compel_consume_seize_task_status(struct seize_task_status *ss)
>>>>>>  {
>>>>>> +	xfree(ss->nspid);
>>>>>> +	xfree(ss->nspgid);
>>>>>> +	xfree(ss->nssid);
>>>>>> +
>>>>>>  	init_seize_task_status(ss);
>>>>>>  }
>>>>>>  
>>>>>>
>>>> .
>>>>
>>>
>> .
>>
>
Kirill Tkhai May 2, 2017, 11:42 a.m.
On 02.05.2017 14:36, Kirill Tkhai wrote:
> On 02.05.2017 13:06, Pavel Emelyanov wrote:
>> On 05/02/2017 12:18 PM, Kirill Tkhai wrote:
>>> On 02.05.2017 11:58, Pavel Emelyanov wrote:
>>>> On 04/29/2017 09:12 AM, Andrei Vagin wrote:
>>>>> On Fri, Apr 28, 2017 at 11:03:08PM -0700, Andrei Vagin wrote:
>>>>>> On Tue, Apr 18, 2017 at 06:57:07PM +0300, Kirill Tkhai wrote:
>>>>>>> Some users want them, so add these fields to the structure.
>>>>>>>
>>>>>>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>>>>>>> ---
>>>>>>>  compel/include/uapi/infect.h |    4 ++++
>>>>>>>  compel/src/lib/infect.c      |    6 ++++++
>>>>>>>  2 files changed, 10 insertions(+)
>>>>>>>
>>>>>>> diff --git a/compel/include/uapi/infect.h b/compel/include/uapi/infect.h
>>>>>>> index 7b681b0cb..4c6ab413d 100644
>>>>>>> --- a/compel/include/uapi/infect.h
>>>>>>> +++ b/compel/include/uapi/infect.h
>>>>>>> @@ -21,6 +21,10 @@ struct seize_task_status {
>>>>>>>  	int			seccomp_mode;
>>>>>>>  	unsigned long long	shdpnd;
>>>>>>>  	unsigned long long	sigpnd;
>>>>>>> +	size_t			ns_levels;
>>>>>>> +	uint32_t		*nspid;
>>>>>>> +	uint32_t		*nspgid;
>>>>>>> +	uint32_t		*nssid;
>>>>>>
>>>>>> Here is the compel code and now we try to add fields which are
>>>>>> not used in compel. It doesn't look good.
>>>>>
>>>>> Maybe we need to declare a new structure in criu which will encapsulate
>>>>> seize_task_status?
>>>>
>>>> We already have one ;) Called proc_status_creds.
>>>
>>> struct proc_status_creds is a mix entities of different types collected
>>> from different files. seize_task_status is the only structure for
>>> /proc/[pid]/status file, all content of this structure is collected
>>> from the single file. It's different things.
>>
>> proc_status_creds is also filled only with values collected from /proc/pid/status,
>> nothing more.
> 
> But it has only statically stored values, and it has no problems like the problems
> solved in this patchset.

We may continue the way suggested via proc_status_creds and also allocate

uint32_t nspid[32];
uint32_t nspgid[32];
uint32_t nssid[32];

in the same structure, but let's introduce the generic memory-efficient solution.
  
>>>>>>
>>>>>>
>>>>>>>  	/* Add new members to the bottom and do not change existing */
>>>>>>>  };
>>>>>>>  
>>>>>>> diff --git a/compel/src/lib/infect.c b/compel/src/lib/infect.c
>>>>>>> index 9ade844cf..ab7558a81 100644
>>>>>>> --- a/compel/src/lib/infect.c
>>>>>>> +++ b/compel/src/lib/infect.c
>>>>>>> @@ -190,11 +190,17 @@ static int skip_sigstop(int pid, int nr_signals)
>>>>>>>  /* Init dynamically allocated fields in NULL and do not touch other */
>>>>>>>  static void init_seize_task_status(struct seize_task_status *ss)
>>>>>>>  {
>>>>>>> +	ss->ns_levels = 0;
>>>>>>> +	ss->nspid = ss->nspgid = ss->nssid = NULL;
>>>>>>>  }
>>>>>>>  
>>>>>>>  /* Free dynamically allocated fields in compel_wait_task() and do not touch other */
>>>>>>>  void compel_consume_seize_task_status(struct seize_task_status *ss)
>>>>>>>  {
>>>>>>> +	xfree(ss->nspid);
>>>>>>> +	xfree(ss->nspgid);
>>>>>>> +	xfree(ss->nssid);
>>>>>>> +
>>>>>>>  	init_seize_task_status(ss);
>>>>>>>  }
>>>>>>>  
>>>>>>>
>>>>> .
>>>>>
>>>>
>>> .
>>>
>>
Andrey Vagin May 3, 2017, 12:18 a.m.
On Tue, May 02, 2017 at 12:30:28PM +0300, Kirill Tkhai wrote:
> On 02.05.2017 12:13, Kirill Tkhai wrote:
> > On 29.04.2017 09:12, Andrei Vagin wrote:
> >> On Fri, Apr 28, 2017 at 11:03:08PM -0700, Andrei Vagin wrote:
> >>> On Tue, Apr 18, 2017 at 06:57:07PM +0300, Kirill Tkhai wrote:
> >>>> Some users want them, so add these fields to the structure.
> >>>>
> >>>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> >>>> ---
> >>>>  compel/include/uapi/infect.h |    4 ++++
> >>>>  compel/src/lib/infect.c      |    6 ++++++
> >>>>  2 files changed, 10 insertions(+)
> >>>>
> >>>> diff --git a/compel/include/uapi/infect.h b/compel/include/uapi/infect.h
> >>>> index 7b681b0cb..4c6ab413d 100644
> >>>> --- a/compel/include/uapi/infect.h
> >>>> +++ b/compel/include/uapi/infect.h
> >>>> @@ -21,6 +21,10 @@ struct seize_task_status {
> >>>>  	int			seccomp_mode;
> >>>>  	unsigned long long	shdpnd;
> >>>>  	unsigned long long	sigpnd;
> >>>> +	size_t			ns_levels;
> >>>> +	uint32_t		*nspid;
> >>>> +	uint32_t		*nspgid;
> >>>> +	uint32_t		*nssid;
> >>>
> >>> Here is the compel code and now we try to add fields which are
> >>> not used in compel. It doesn't look good.
> >>
> >> Maybe we need to declare a new structure in criu which will encapsulate
> >> seize_task_status?
> > 
> > Look at the description of patch [3/5]. It's explained there, why I go this
> > way. A method get_status() of function compel_wait_task() may be called
> > twice, and the allocated fields must be freed before the second call.
> > 
> > Since different methods get_status() may allocate different arguments,
> > if we do not go my way, then we need either appropriate free_status()
> > methods (which is more complex) or to free the previously allocated
> > status in get_status() on the second call (which is ugly).
> > 
> > I suggest generic method, suitable for everything, which does not make
> > the code complicate and which is easily extensible for new fields.
> > 
> > I don't agree, it looks bad. Also, there is no a rule that a library
> > must not contain more methods and interfaces, when it's need for itself.
> 
> + It's why a library is need at all.

A library has to contain a generic part and avoid any project-specific
parts, doesn't it?
>   
> >>>
> >>>
> >>>>  	/* Add new members to the bottom and do not change existing */
> >>>>  };
> >>>>  
> >>>> diff --git a/compel/src/lib/infect.c b/compel/src/lib/infect.c
> >>>> index 9ade844cf..ab7558a81 100644
> >>>> --- a/compel/src/lib/infect.c
> >>>> +++ b/compel/src/lib/infect.c
> >>>> @@ -190,11 +190,17 @@ static int skip_sigstop(int pid, int nr_signals)
> >>>>  /* Init dynamically allocated fields in NULL and do not touch other */
> >>>>  static void init_seize_task_status(struct seize_task_status *ss)
> >>>>  {
> >>>> +	ss->ns_levels = 0;
> >>>> +	ss->nspid = ss->nspgid = ss->nssid = NULL;
> >>>>  }
> >>>>  
> >>>>  /* Free dynamically allocated fields in compel_wait_task() and do not touch other */
> >>>>  void compel_consume_seize_task_status(struct seize_task_status *ss)
> >>>>  {
> >>>> +	xfree(ss->nspid);
> >>>> +	xfree(ss->nspgid);
> >>>> +	xfree(ss->nssid);
> >>>> +
> >>>>  	init_seize_task_status(ss);
> >>>>  }
> >>>>  
> >>>>