| Message ID | 149253102796.15051.1723556388388431632.stgit@localhost.localdomain |
|---|---|
| State | New |
| Series | "compel, criu: Make struct seize_task_status extensible" |
| Headers | show |
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); }
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); > } > >
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); > > } > > > >
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); >>> } >>> >>> > . >
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); >>> } >>> >>>
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); >>>> } >>>> >>>> >> . >> >
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); >>>> } >>>> >>>>
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); >>>>> } >>>>> >>>>> >>> . >>> >> > . >
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); >>>>>> } >>>>>> >>>>>> >>>> . >>>> >>> >> . >> >
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); >>>>>>> } >>>>>>> >>>>>>> >>>>> . >>>>> >>>> >>> . >>> >>
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); > >>>> } > >>>> > >>>>
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(+)