[CRIU] [PATCH 3/8] cr-restore: set cr_error to EEXIST if such pid already exists, v2

[Pavel Emelyanov]

On 12/02/2014 09:03 PM, Ruslan Kuprieiev wrote:
> On 12/02/2014 03:56 PM, Ruslan Kuprieiev wrote:
>> On 12/02/2014 03:20 PM, Pavel Emelyanov wrote:
>>> On 12/02/2014 03:29 PM, Ruslan Kuprieiev wrote:
>>>> This is a very common error when using criu.
>>>>
>>>> The problem here is that we need to somehow transfer cr_errno
>>>> from one process to another. I suggest using pipe to give
>>>> one end to children and read cr_errno on other after restore
>>>> is finished.
>>>>
>>>> v2, Pavel suggested putting errno into shared task_entries.
>>>>
>>>> Signed-off-by: Ruslan Kuprieiev <kupruser at gmail.com>
>>>> ---
>>>>   cr-restore.c       | 7 ++++++-
>>>>   include/cr-errno.h | 1 +
>>>>   include/rst_info.h | 1 +
>>>>   3 files changed, 8 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/cr-restore.c b/cr-restore.c
>>>> index 93a6ca6..60641d9 100644
>>>> --- a/cr-restore.c
>>>> +++ b/cr-restore.c
>>>> @@ -85,6 +85,8 @@
>>>>
>>>>   #include "asm/restore.h"
>>>>
>>>> +#include "cr-errno.h"
>>>> +
>>>>   static struct pstree_item *current;
>>>>
>>>>   static int restore_task_with_children(void *);
>>>> @@ -1412,6 +1414,7 @@ static int restore_task_with_children(void *_arg)
>>>>       pid = getpid();
>>>>       if (current->pid.virt != pid) {
>>>>           pr_err("Pid %d do not match expected %d\n", pid,
>>>> current->pid.virt);
>>>> +        task_entries->cr_err = EEXIST;
>>>
>>> This is racy. Presumably we want to get the very first error from
>>> children, thus we need the cmpxchg() here (wrapped in a helper).
> 
> Well, implementing cmpxchg() would require implementing futex_cmpxchg,
> which would require atomic_cmpxchg, which would require some assembly.

It will :)

> But we currently have __xchg_op only for x86.

You can pick the xchg for arm from kernel and send the patches
on the mailing list.

> How about we use here futex_t cr_err and futex_set() for now?

No, futexes are for different things.

> I mean, we don't care which child reported cr_errno=EEXIST.

We don't but when in the future we'll add more errors reported
on restore we'll start caring ;)

>>>
>>
>> Ah, i see.
>> Thanks.
>>
>>>>           goto err;
>>>>       }
>>>>
>>>> @@ -1535,8 +1538,10 @@ static int restore_wait_inprogress_tasks()
>>>>
>>>>       futex_wait_while_gt(np, 0);
>>>>       ret = (int)futex_get(np);
>>>> -    if (ret < 0)
>>>> +    if (ret < 0) {
>>>> +        cr_errno = task_entries->cr_err;
>>>>           return ret;
>>>> +    }
>>>>
>>>>       return 0;
>>>>   }
>>>> diff --git a/include/cr-errno.h b/include/cr-errno.h
>>>> index bec72a8..f86bba8 100644
>>>> --- a/include/cr-errno.h
>>>> +++ b/include/cr-errno.h
>>>> @@ -4,6 +4,7 @@ extern int cr_errno;
>>>>   /*
>>>>    * List of symbolic error names:
>>>>    * ESRCH    - no process can be found corresponding to that
>>>> specified by pid
>>>> + * EEXIST    - process with such pid already exists
>>>>    */
>>>>
>>>>   #endif /* __CR_ERRNO_H__ */
>>>> diff --git a/include/rst_info.h b/include/rst_info.h
>>>> index 6b00c50..cec0144 100644
>>>> --- a/include/rst_info.h
>>>> +++ b/include/rst_info.h
>>>> @@ -10,6 +10,7 @@ struct task_entries {
>>>>       futex_t nr_in_progress;
>>>>       futex_t start;
>>>>       mutex_t    zombie_lock;
>>>> +    int cr_err;
>>>>   };
>>>>
>>>>   struct fdt {
>>>>
>>>
>>
> 
> .
>