[CRIU] Re: [PATCH] restorer: close log file before detaching from crtools

[Kinsbursky Stanislav]

20.02.2012 18:36, Andrew Vagin пишет:
> On Mon, Feb 20, 2012 at 05:12:13PM +0300, Kinsbursky Stanislav wrote:
>> There is a race between log close by process being restoring and opened file
>> desctriptors check in zdtm test suite - crtools can exit and compare file
>> descriptors before detached restored process will perform all the rest tasks
>> (including close of the log) and execute final system call:
>>
>> --- dump/sleeping00/8578/dump.fd        2012-02-20 14:31:31.246096000 +0300
>> +++ dump/sleeping00/8578/restore.fd     2012-02-20 14:31:31.418095999 +0300
>> @@ -1,4 +1,5 @@
>>
>>   0 ->  /dev/null
>>   1 ->  /dev/null
>> +1023 ->  /root/crtools/test/dump/sleeping00/8578/restore.log
>>   2 ->  /dev/null
>>
>> The solution is to close log in restorer before final command received. But
>> this leads to another problem: we have to inform somehow about possible errors
>> afterwards This is done by forced segmentation fault and looks like this
>> (dmesg):
>>
>> pipe00[17373]: segfault at 2ed ip 00000000000002ed sp ffffffffffffffff error
>> 14
>>
>> Where "ip" is restorer's line number and "sp" is the reason why we failed.
>>
>> Signed-off-by: Stanislav Kinsbursky<skinsbursky at openvz.org>
>>
>> ---
>>   restorer.c |   21 +++++++++++++++------
>>   1 files changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/restorer.c b/restorer.c
>> index ea4af04..d25a571 100644
>> --- a/restorer.c
>> +++ b/restorer.c
>> @@ -336,7 +336,7 @@ static u64 restore_mapping(const struct vma_entry *vma_entry)
>>    */
>>   long restore_task(struct task_restore_core_args *args)
>>   {
>> -	long ret = -1;
>> +	long ret = -1, line;
>>   	struct task_entry *task_entry;
>>   	struct core_entry *core_entry;
>>   	struct vma_entry *vma_entry;
>> @@ -722,6 +722,9 @@ long restore_task(struct task_restore_core_args *args)
>>   	sys_sigaction(SIGCHLD,&args->sigchld_act, NULL);
>>
>>   	cr_wait_dec(&args->task_entries->nr_in_progress);
>> +
>> +	sys_close(args->logfd);
>> +
>>   	cr_wait_while(&args->task_entries->start, CR_STATE_RESTORE_SIGCHLD);
>>
>>   	/*
>> @@ -742,13 +745,10 @@ long restore_task(struct task_restore_core_args *args)
>>
>>   	ret = sys_munmap(args->task_entries, TASK_ENTRIES_SIZE);
>>   	if (ret<  0) {
>> -		write_num_n(__LINE__);
>> -		write_num_n(ret);
>> -		goto core_restore_end;
>> +		line = __LINE__;
>> +		goto core_restore_failed;
>>   	}
>>
>> -	sys_close(args->logfd);
>> -
>>   	/*
>>   	 * Sigframe stack.
>>   	 */
>> @@ -773,4 +773,13 @@ core_restore_end:
>>   	write_num_n(sys_getpid());
>>   	sys_exit(-1);
>>   	return -1;
>> +
>> +core_restore_failed:
>> +	asm volatile(
>> +		"movq %0, %%rsp				\n"
>> +		"jmp *%1				\n"
>> +		:
>> +		: "r"(ret), "r"(line)
>> +		: );
>> +	return ret;
> We have a similar code in BUG_ON_HANDLER, but this code is better, so
> I think you can improve BUG_ON_HANDLER and use it.

Thanks for your valuable opinion. I'll keep that in mind.

-- 
Best regards,
Stanislav Kinsbursky