[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 02:42:17PM +0300, Kirill Tkhai wrote:
> 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 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?
> >>>>
> >>>> 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.

What do you think about adding one more call-back in compel_wait_task to
release seize_task_status. In this case we can put ns*id fields to proc_status_creds.

diff --git a/compel/src/lib/infect.c b/compel/src/lib/infect.c
index ab7558a..5461edb 100644
--- a/compel/src/lib/infect.c
+++ b/compel/src/lib/infect.c
@@ -213,6 +213,7 @@ void compel_consume_seize_task_status(struct seize_task_status *ss)
  */
 int compel_wait_task(int pid, int ppid,
                int (*get_status)(int pid, struct seize_task_status *, void *),
+               int (*free_status)(struct seize_task_status *),
                struct seize_task_status *ss, void *data)
 {
        siginfo_t si;

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