[Devel] [CRIU] [PATCH 1/6] vzctl: split ct_env_create

[Kir Kolyshkin]

On 05/19/2013 11:59 AM, Andrew Vagin wrote:
> On Fri, May 17, 2013 at 10:24:55AM -0700, Kir Kolyshkin wrote:
>> On 05/16/2013 09:47 AM, Andrey Wagin wrote:
>>> 2013/5/16 Glauber Costa <glommer at parallels.com>:
>>>> On 05/16/2013 04:14 PM, Andrey Vagin wrote:
>>>>> +     ret = ct_env_create_real(arg);
>>>>> +     if (ret < 0)
>>>>>                return VZ_RESOURCE_ERROR;
>>>>> -     }
>>>> Isn't it better to just keep the return values intact in create_real,
>>>> and then return them as is if ret != 0 ?
>>> It returns PID of the init process. VZ_RESOURCE_ERROR is positive too
>>>
>> It does not (maybe it's a bug in your patch).
>>
>> +	/*
>> +	 * Belong in the setup phase
>> +	 */
>> +	clone_flags = SIGCHLD;
>> +	/* FIXME: USERNS is still work in progress */
>> +	clone_flags |= CLONE_NEWUTS|CLONE_NEWPID|CLONE_NEWIPC;
>> +	clone_flags |= CLONE_NEWNET|CLONE_NEWNS;
>> +
>> +	ret = clone(_env_create, child_stack, clone_flags, arg);
>> +	if (ret  < 0) {
>> +		logger(-1, errno, "Unable to clone");
>> +		/* FIXME: remove ourselves from container first */
>> +		destroy_container(arg->veid);
>> +		return -1;
>> +	}
>> +
>> +	return 0;
>> +}
>>
>> Did you mean "return ret" here?
> Yes, it is fixed in "vzctl: save PID of init in a state file". Sorry for
> that.
>> Also, to not change all those return statements, I suggest to pass pid_t * as a second argument.
> I don't like that. If you want, I can do that.
>

Let's do this. If there is no need to return different error codes
from ct_env_create_real() then I'm fine with your approach.
Just make sure to make the patch logical (i.e. return ret, otherwise
it's not clear from the patch why you change VZ_RESOURCE_ERROR to -1)
and briefly document its purpose and return code in a comment
just before the function.