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

[Kirill Tkhai]

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 at 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);
>>>  }
>>>  
>>>