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

[Ruslan Kuprieiev]

On 12/02/2014 08:26 PM, Pavel Emelyanov wrote:
> 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.
>

I already started doing that, but I'm a bit scared =D.

>> How about we use here futex_t cr_err and futex_set() for now?
>
> No, futexes are for different things.
>

Oh, I see.

>> 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 ;)
>

Ok, will do.
Thanks! =)

>>>>
>>>
>>> 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 {
>>>>>
>>>>
>>>
>>
>> .
>>
>