[CRIU] [PATCH] add join-ns opt to criu restore

[Dengguangxing]

Hi, Pavel

Thanks for your reviewing.

I will refactor this patch, divide it into several patches and resend them later.

> Note, that once the patch gets merged we'll need the zdtm.py extention
> to test this functionaluty. Only after this (and after tests pass) I'll
> be able to move this patch from criu-dev branch into master and release.
> 

where can I get the zdtm guidance? I don't know how to write a test case for
this new-added opts. An existing similiar test-case will help :)

> 
>> @@ -99,6 +101,41 @@ bad_ns:
>>  	return -1;
>>  }
>>
>> +static int parse_join_ns(const char *ptr)
>> +{
>> +	char *aux, *ns_file, *extra_opts = NULL;
>> +
>> +	aux = strchr(ptr, ':');
>> +	if (aux == NULL)
>> +		return -1;
>> +	*aux = '\0';
>> +
>> +	if (!strncmp(ptr, "pid", 4)) {
>> +		pr_perror("pid namespace not supported in join-ns\n");
>> +		return -1;
>> +	} else if (strncmp(ptr, "net", 4) &&
>> +		strncmp(ptr, "uts", 4) &&
>> +		strncmp(ptr, "ipc", 4) &&
>> +		strncmp(ptr, "user", 5) &&
>> +		strncmp(ptr, "mnt", 4)) {
>> +		pr_perror("Illegal namespace %s in join-ns\n", ptr);
>> +		return -1;
>> +	}
> 
> We already have a code that turns namespaces' names into flags,
> it's in parse_unshare_arg. If you _do_ need to turn namesp into
> flags, please, pull this code into a separate function (with
> separate patch) and use it in your code.

I don't know exactly what do you mean. However, I found this code redundant
and have removed it :)

>> @@ -810,6 +857,12 @@ usage:
>>  "  --empty-ns {net}\n"
>>  "			Create a namespace, but don't restore its properies.\n"
>>  "			An user will retore them from action scripts.\n"
>> +"  -J|--join-ns NS:PID|NS_FILE,EXTRA_OPTS\n"
> 
> Please, separate extra opts with colon and mark them as optional. So the
> help would look like this
> 
> -J|--join-ns NS:PID|NS_FILE[:EXTRA_OPTS]
> 

thanks, fixed!

> 
>> diff --git a/criu/include/namespaces.h b/criu/include/namespaces.h
>> index 303c9e6..3729aed 100644
>> --- a/criu/include/namespaces.h
>> +++ b/criu/include/namespaces.h
>> @@ -19,6 +21,24 @@ struct ns_desc {
>>  	size_t		len;
>>  };
>>
>> +struct user_ns_extra {
>> +	char	*uid;
>> +	char	*gid;
>> +};
>> +
>> +/* struct join_ns is used for storing parameters specified by --join-ns */
>> +struct join_ns {
>> +	struct list_head 	list;
>> +	char			*ns_file;
>> +	int			ns_fd; 	/* namespace file descriptor */
> 
> Not sure this is required on join_ns. The fd obtained is purely used
> in one call-stack, so just keeping it on stack should work.
> 

yes, ns_fd is purely used in one call-stack. however as ns_fd and and ns_file are corresponding.
I think this is a convenient way to do it.


>> +	struct ns_desc		nd; 	/* namespace descriptor */
> 
> Better grab pointer on one of existing nd-s rather than constructing
> those by yourself.
> 
thanks, fixed!

>>
>> +unsigned int get_join_ns_flag() {
>> +	struct join_ns *jn;
>> +	unsigned int join_ns_flags = 0;
>> +
>> +	list_for_each_entry(jn, &opts.join_ns, list)
>> +		if (!strncmp(jn->nd.str, "uts", 4))
>> +			join_ns_flags |= CLONE_NEWUTS;
>> +		else if (!strncmp(jn->nd.str, "ipc", 4))
>> +			join_ns_flags |= CLONE_NEWIPC;
>> +		else if (!strncmp(jn->nd.str, "mnt", 4))
>> +			join_ns_flags |= CLONE_NEWNS;
>> +		else if (!strncmp(jn->nd.str, "pid", 4))
>> +			join_ns_flags |= CLONE_NEWPID;
>> +		else if (!strncmp(jn->nd.str, "net", 4))
>> +			join_ns_flags |= CLONE_NEWNET;
>> +		else if (!strncmp(jn->nd.str, "user", 5))
>> +			join_ns_flags |= CLONE_NEWNET;
>> +
>> +	return join_ns_flags;
> 
> This is too heavy. Instead of keepin the strings with names and
> parsing them every time, either store the namespaces' flags in
> a variable, or keep an array/list of required ns_desc-s.
> 
thanks, fixed!

>> +static int check_ns_file(char *ns_file) {
>> +	int pid, ret, proc_dir;
>> +
>> +	if (!check_int_str(ns_file)) {
>> +		pid = atoi(ns_file);
>> +		if (pid <= 0) {
>> +			pr_perror("Invalid join_ns pid %s", ns_file);
>> +			return -1;
>> +		}
>> +		proc_dir = open_pid_proc(pid);
>> +		if (proc_dir < 0) {
>> +			pr_perror("Invalid join_ns pid: /proc/%s not found", ns_file);
>> +			return -1;
>> +		}
>> +		return 0;
>> +	}
>> +
>> +	ret = access(ns_file, 0);
>> +	if (ret < 0) {
>> +		pr_perror("Can't access join-ns file: %s", ns_file);
>> +		return -1;
>> +	}
> 
> I doubt these checks should be done here. Once you'll try to join the
> namespace, specified by the option, you'll hit an error and restore
> would abort antway.
> 
These checks mean to prevent some obvious mistakes here, before restore.

>> +int join_ns_add(const char *type, char *ns_file, char *extra_opts) {
>> +	struct join_ns *jn;
>> +
>> +	jn = xmalloc(sizeof(*jn));
>> +	if (!jn) {
>> +		return -1;
>> +	}
>> +
>> +	if (check_ns_file(ns_file)) {
>> +		return -1;
>> +	}
>> +
>> +	jn->ns_file = ns_file;
>> +	if (!strncmp(type, "net", 4)) {
>> +			jn->nd = net_ns_desc;
>> +	} else if (!strncmp(type, "uts", 4)) {
>> +			jn->nd = uts_ns_desc;
>> +	} else if (!strncmp(type, "ipc", 4)) {
>> +			jn->nd = ipc_ns_desc;
>> +	} else if (!strncmp(type, "pid", 4)) {
>> +			jn->nd = pid_ns_desc;
>> +	} else if (!strncmp(type, "user", 5)) {
>> +			jn->nd = user_ns_desc;
>> +			if (set_user_extra_opts(jn, extra_opts)) {
>> +				pr_perror("invalid user namespace extra_opts %s\n", extra_opts);
>> +				return -1;
>> +			}
>> +	} else if (!strncmp(type, "mnt", 4)) {
>> +			jn->nd = mnt_ns_desc;
>> +	} else {
>> +			pr_perror("invalid namespace type %s\n", type);
>> +			return -1;
> 
> Indentation is one tab wider than it should.

thanks, fixed!

>> +	}
>> +
>> +	list_add_tail(&jn->list, &opts.join_ns);
>> +	pr_info("Added %s:%s join namespace\n", type, ns_file);
>> +	return 0;
>> +}
>> +
>>  static unsigned int parse_ns_link(char *link, size_t len, struct ns_desc *d)
>>  {
>>  	unsigned long kid = 0;
>> @@ -78,7 +240,7 @@ int switch_ns(int pid, struct ns_desc *nd, int *rst)
>>
>>  	nsfd = open_proc(pid, "ns/%s", nd->str);
>>  	if (nsfd < 0) {
>> -		pr_perror("Can't open ipcns file");
>> +		pr_perror("Can't open ns file");
> 
> All cleanups should go in separate patches, please.
> 
thanks, fixed.

>> +static int switch_join_ns(struct join_ns *jn) {
>> +	if (jn->ns_fd < 0) {
>> +		pr_perror("Invalid ns fd in join-ns");
>> +		return -1;
>> +	}
>> +
>> +	int ret = -1;
>> +	int self_fd;
>> +	struct stat st, self_st;
>> +	char buf[32];
> 
> We follow the Linux kernel coding style. In particular, in function
> declarations braces should be on separate line and all the variables
> should be declared before any code.
> 
fixed.

>> +
>> +	if (fstat(jn->ns_fd, &st) == -1) {
>> +		pr_perror("Can't get ns file %s stat", jn->ns_file);
>> +		goto err_ns;
>> +	}
>> +
>> +	snprintf(buf, sizeof(buf), "/proc/self/ns/%s", jn->nd.str);
>> +	self_fd = open(buf, O_RDONLY);
>> +	if (self_fd < 0) {
>> +		pr_perror("Can't open ns file: %s", buf);
>> +		goto err_ns;
>> +	}
>> +	if (fstat(self_fd, &self_st) == -1) {
> 
> There's no need in opening self-fd, just use stat() system call and
> stat by path.
fixed

> 
>> +		pr_perror("Can't get ns file %s stat", buf);
>> +		goto err_self;
>> +	}
>> +
>> +	if (st.st_ino != self_st.st_ino) {
> 
> Is this check _really_ required? Kernel would behave OK and won't
> move the process if namespaces already match.
yes, this is necessary. because calling set_ns() to the same user-ns
twice will return an error, so we will check it here.

>> +static int switch_user_join_ns(struct join_ns *jn) {
>> +	if (jn == NULL) {
>> +		return 0;
>> +	}
>> +
>> +	uid_t uid;
>> +	gid_t gid;
>> +
>> +	if (switch_join_ns(jn))
>> +		return -1;
>> +
>> +	if (jn->extra_opts.user_extra.uid == NULL) {
>> +		uid = getuid();
>> +	} else {
>> +		uid = atoi(jn->extra_opts.user_extra.uid);
>> +	}
>> +	if (jn->extra_opts.user_extra.gid == NULL) {
>> +		gid = getgid();
>> +	} else {
>> +		gid = atoi(jn->extra_opts.user_extra.gid);
>> +	}
> 
> All those { and } braces can be avoided here
thanks, fixed

>> index a67cd6e..ba871c7 100644
>> --- a/criu/net.c
>> +++ b/criu/net.c
>> @@ -1094,8 +1094,11 @@ int prepare_net_ns(int pid)
>>  {
>>  	int ret = 0;
>>  	NetnsEntry *netns = NULL;
>> +	unsigned int join_ns_flag = 0;
>>
>> -	if (!(opts.empty_ns & CLONE_NEWNET)) {
>> +	join_ns_flag = get_join_ns_flag();
>> +	if (!(opts.empty_ns & CLONE_NEWNET) ||
>> +			!(join_ns_flag & CLONE_NEWNET)) {
> 
> Hasn't this already been checked in check_namespace_opts()?

here I mean, if net emtpy-ns or net join-ns is set, will skip these operations below.
'||' should be replace with '&&' :) have fixed that.