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

[Andrew Vagin]

On Sun, May 19, 2013 at 12:12:47PM -0700, Kir Kolyshkin wrote:
> 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.

Eh. Actually it's not one error code, we need to return VZ_RESTORE_ERROR
on restore. I prefer to return -VZ_*ERROR, but it's out of the common
style for this project. If we add pid_t *pid, we will need to add it in
env_create_FN too...