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

[Andrei Vagin]

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 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.
> 
> + 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);
> >>>>  }
> >>>>  
> >>>>