[lxc-devel] [PATCH] criu: Add exec-cmd option (v2)

[Pavel Emelyanov]

On 03/21/2014 12:57 PM, Deyan Doychev wrote:
> Hi Pavel,
> 
> On 03/20/2014 08:38 PM, Pavel Emelyanov wrote:
>> On 03/20/2014 08:24 PM, Deyan Doychev wrote:
>>> From: Deyan Doychev <deyandoichev at gmail.com>
>>> ...
>>> --- a/cr-restore.c
>>> +++ b/cr-restore.c
>>> @@ -1552,7 +1552,7 @@ static int restore_root_task(struct pstree_item *init)
>>>  
>>>  	write_stats(RESTORE_STATS);
>>>  
>>> -	if (!opts.restore_detach)
>>> +	if (!opts.restore_detach && !opts.exec_cmd)
>> I feel that I'm missing something, but why is the check for
>> !opts.exec_cmd is needed here?
> 
> This code waits for the restored task to finish. Thus if we don't skip
> the wait call, we will exec after the restored task has already finished
> which is not intended. The exec-ed program should wait for the child
> when using --exec. That's why we need this check.

Ah, I see. This is to avoid calling wait() in vain.

>> ...
>>>  		wait(NULL);
>>>  
>>>  	return 0;
>>> @@ -1600,6 +1600,9 @@ int cr_restore_tasks(void)
>>>  {
>>>  	int ret = -1;
>>>  
>>> +	if (opts.exec_cmd && opts.restore_detach && daemon(1, 0))
>> In that case we will lose the ability to get error code from failed restore.
>> Can we move the daemonizing to the very end of the restore procedure?
> 
> We want the new process to be the father of the restored processes so
> the last place I see is right before the fork_with_pid(init); call in
> restore_root_task.

Hm... Indeed. The daemon will create a child process and make the parent exit.
Thus doing it after restore is not what we want :(

> Is this ok or do you have something else in mind?

This is OK, but this brings a problem -- we lose the ability to check whether
the restore failed or not.

I think that making --exec-cmd work together with --restore-detached is quite
tricky. We have to fork another task, make this child do restore, then execv()
and then the parent should somehow check, that restore succeeded and exit. Or
propagate the error code upwards.

Deyan, can we do it in two steps -- first you make --exec-cmd work without the -d
option, i.e. -- the crtools process will just call execv at the end and that's
it. It's parent will wait for it to finish. And with a check that --exec-cmd and
-d are not used together (for sanity). I will commit this patch.

Then you implement the support for -d and --exec-cmd together, so that we correctly
and synchronously handle the failed restore code.

Is that OK for you?

>>> @@ -37,6 +37,7 @@ message criu_opts {
>>>  
>>>  	optional uint32			cpu_cap		= 20 [default = 0xffffffff];
>>>  	optional bool			force_irmap	= 21;
>>> +	optional string			exec_cmd	= 22;
>> Maybe it's better to make this field repeated to allow for spaces in
>> arguments and to simplify the parsing (no strtoks would be required)?
> 
> Sure, will resubmit.

Thanks!