[CRIU] [PATCH] compel: compel_emergency_sigframe() helper introduced

[Dmitry Safonov]

2017-03-27 16:26 GMT+03:00 Stanislav Kinsburskiy <skinsbursky at virtuozzo.com>:
>
>
> 27.03.2017 15:15, Dmitry Safonov пишет:
>
>> 2017-03-27 15:57 GMT+03:00 Stanislav Kinsburskiy
>> <skinsbursky at virtuozzo.com>:
>>>
>>>
>>> 24.03.2017 11:14, Dmitry Safonov пишет:
>>>
>>>> 2017-03-24 12:32 GMT+03:00 Stanislav Kinsburskiy
>>>> <skinsbursky at virtuozzo.com>:
>>>>>
>>>>>
>>>>> 24.03.2017 00:13, Andrei Vagin пишет:
>>>>>
>>>>>> On Fri, Mar 17, 2017 at 01:17:58PM +0300, Stanislav Kinsburskiy wrote:
>>>>>>>
>>>>>>> This helper constructs sigframe, which can be used to resume process
>>>>>>> from
>>>>>>> the
>>>>>>> point where it was stopped.
>>>>>>> Result has to be copied to the process manually and then used with
>>>>>>> rt_sigreturn.
>>>>>>>
>>>>>>> Signed-off-by: Stanislav Kinsburskiy <skinsbursky at virtuozzo.com>
>>>>
>>>> [...]
>>>>>>>
>>>>>>> --- a/compel/src/lib/infect.c
>>>>>>> +++ b/compel/src/lib/infect.c
>>>>>>> @@ -1162,6 +1162,20 @@ static int make_sigframe_plain(void *from,
>>>>>>> struct
>>>>>>> rt_sigframe *f, struct rt_sigf
>>>>>>>           return 0;
>>>>>>>     }
>>>>>>>     +int compel_emergency_sigframe(struct parasite_ctl *ctl, struct
>>>>>>> rt_sigframe *f,
>>>>>>> +                             struct rt_sigframe *rtf)
>>>>>>> +{
>>>>>>> +       pid_t pid = ctl->rpid;
>>>>>>> +       struct thread_ctx *thread = &ctl->orig;
>>>>>>> +       struct plain_regs_struct regs;
>>>>>>> +
>>>>>>> +       if (get_task_regs(pid, &thread->regs, save_regs_plain,
>>>>>>> &regs))
>>>>>>> {
>>>>>>> +               pr_err("Can't obtain regs for thread %d\n", pid);
>>>>>>> +               return -1;
>>>>>>> +       }
>>>>>>> +       return make_sigframe_plain(&regs, f, rtf, &thread->sigmask);
>>>>>>
>>>>>> make_sigframe_plain is called from parasite_start_daemon(), why it
>>>>>> doesn't work for you. Could you describe your usecase for these
>>>>>> changes?
>>>>>>
>>>>> Because I don't have parasite plugin, on which parasite_start_daemon
>>>>> relies.
>>>>> But it makes sense to use this new helper in parasite_start_daemon to
>>>>> make
>>>>> sure it's tested.
>>>>
>>>> Maybe it worth not providing additional interfaces, but splitting the
>>>> existing.
>>>> If we can make non-parasite version of compel_infect() and reuse its
>>>> code
>>>> in parasite's one, so it'll suit criu's needs and needs of projects
>>>> without parasite.
>>>> Does it make sense?
>>>
>>>
>>> Definitely.
>>>
>>>> Also, this looks incomplete without curing part. How do you use
>>>> prepared sigframe
>>>> afterward? Calling rt_sigreturn() with the help of compel_syscall() or
>>>> somehow alike?
>>>
>>>
>>> Yes. Somehow alike.
>>>
>>>> I guess, there should be also a patch to complement API.
>>>> And (by @avagin's rules) - adding new API/features means adding tests
>>>> for
>>>> it,
>>>> looks like it wouldn't take much time to write a test for this.
>>>
>>>
>>> If this peace of code will be used by CRIU itself, then it looks like
>>> sufficient testing. No?
>>>
>> Ok for me, but the test also plays as an example how-to-use such API.
>> I don't insist, anyway.
>>
>
> Calling sigreturn makes sense when called from within of the process
> context.
> BTW, it's done already if libcompel plugins in case of fatal error on CRIU
> side.
> Thus, calling this from plugin explicitly doesn't make mush sense.
> And I don't see other ways to show it's usability, supported by libcompel.

Yes, but fini_sigreturn() at this moment is called only for parasite-based
infection. While the patch exports API for non-parasite case of infection.
So, as I've asked before, can we complement it with usage of the formed
sigframe in the case of non-parasite infection?
It may be just a helper that has inside compel_syscall() with rt_sigreturn.
I mean, if anyone is going to use API for forming sigframe, he will
have to write such helper. Maybe it's better to provide one for integrity?

-- 
             Dmitry