[lxc-devel] [PATCH 6/9] utils: move useful helper functions from lxccontainer to utils.

[Serge Hallyn]

Quoting Dongsheng Yang (yangds.fnst at cn.fujitsu.com):
> Function of enter_to_ns() is useful but currently is static for
> lxccontainer.c.
> 
> This patch split it into two parts named as switch_to_newuser()
> and switch_to_newnet() into utils.c.
> 
> Signed-off-by: Dongsheng Yang <yangds.fnst at cn.fujitsu.com>

A few problems here:

> ---
>  src/lxc/lxccontainer.c | 53 ++++++------------------------------------------
>  src/lxc/utils.c        | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/lxc/utils.h        |  2 ++
>  3 files changed, 63 insertions(+), 47 deletions(-)
> 
> diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> index ee8f491..f210a88 100644
> --- a/src/lxc/lxccontainer.c
> +++ b/src/lxc/lxccontainer.c
> @@ -1478,56 +1478,15 @@ static bool lxcapi_clear_config_item(struct lxc_container *c, const char *key)
>  	return ret == 0;
>  }
>  
> -static inline bool enter_to_ns(struct lxc_container *c) {
> -	int netns, userns, ret = 0, init_pid = 0;;
> -	char new_netns_path[MAXPATHLEN];
> -	char new_userns_path[MAXPATHLEN];
> -
> -	if (!c->is_running(c))
> -		goto out;
> -
> -	init_pid = c->init_pid(c);
> +static inline bool enter_to_ns(struct lxc_container *c)
> +{
> +	pid_t pid = c->init_pid(c);
>  
> -	/* Switch to new userns */
>  	if ((geteuid() != 0 || (c->lxc_conf && !lxc_list_empty(&c->lxc_conf->id_map))) && access("/proc/self/ns/user", F_OK) == 0) {
> -		ret = snprintf(new_userns_path, MAXPATHLEN, "/proc/%d/ns/user", init_pid);
> -		if (ret < 0 || ret >= MAXPATHLEN)
> -			goto out;
> -
> -		userns = open(new_userns_path, O_RDONLY);
> -		if (userns < 0) {
> -			SYSERROR("failed to open %s", new_userns_path);
> -			goto out;
> -		}
> -
> -		if (setns(userns, CLONE_NEWUSER)) {
> -			SYSERROR("failed to setns for CLONE_NEWUSER");
> -			close(userns);
> -			goto out;
> -		}
> -		close(userns);
> +		switch_to_newuser(pid);

If the target pid is not in a separate userns, then the setns will
fail.

You have switch_to_newuser returning a bool but are simply ignoring
the return value, which will break some builds.

>  	}
> -
> -	/* Switch to new netns */
> -	ret = snprintf(new_netns_path, MAXPATHLEN, "/proc/%d/ns/net", init_pid);
> -	if (ret < 0 || ret >= MAXPATHLEN)
> -		goto out;
> -
> -	netns = open(new_netns_path, O_RDONLY);
> -	if (netns < 0) {
> -		SYSERROR("failed to open %s", new_netns_path);
> -		goto out;
> -	}
> -
> -	if (setns(netns, CLONE_NEWNET)) {
> -		SYSERROR("failed to setns for CLONE_NEWNET");
> -		close(netns);
> -		goto out;
> -	}
> -	close(netns);
> -	return true;
> -out:
> -	return false;
> +	
> +	return switch_to_newnet(pid);
>  }
>  
>  // used by qsort and bsearch functions for comparing names
> diff --git a/src/lxc/utils.c b/src/lxc/utils.c
> index ed34706..fc9c6b2 100644
> --- a/src/lxc/utils.c
> +++ b/src/lxc/utils.c
> @@ -43,6 +43,7 @@
>  #include "utils.h"
>  #include "log.h"
>  #include "lxclock.h"
> +#include "namespace.h"
>  
>  lxc_log_define(lxc_utils, lxc);
>  
> @@ -1260,6 +1261,60 @@ int detect_shared_rootfs(void)
>  	return 0;
>  }
>  
> +bool switch_to_newuser(pid_t pid)
> +{
> +	int userns, ret = 0;
> +	char new_userns_path[MAXPATHLEN];
> +
> +	ret = snprintf(new_userns_path, MAXPATHLEN, "/proc/%d/ns/user", pid);
> +	if (ret < 0 || ret >= MAXPATHLEN)
> +		goto out;
> +
> +	userns = open(new_userns_path, O_RDONLY);
> +	if (userns < 0) {
> +		SYSERROR("failed to open %s", new_userns_path);
> +		goto out;
> +	}
> +
> +	if (setns(userns, CLONE_NEWUSER)) {
> +		SYSERROR("failed to setns for CLONE_NEWUSER");
> +		close(userns);
> +		goto out;
> +	}
> +	close(userns);
> +
> +	return true;
> +out:
> +	return false;
> +
> +}
> +
> +bool switch_to_newnet(pid_t pid) {
> +	int netns, ret = 0;
> +	char new_netns_path[MAXPATHLEN];
> +
> +	/* Switch to new netns */
> +	ret = snprintf(new_netns_path, MAXPATHLEN, "/proc/%d/ns/net", pid);
> +	if (ret < 0 || ret >= MAXPATHLEN)
> +		goto out;
> +
> +	netns = open(new_netns_path, O_RDONLY);
> +	if (netns < 0) {
> +		SYSERROR("failed to open %s", new_netns_path);
> +		goto out;
> +	}
> +
> +	if (setns(netns, CLONE_NEWNET)) {
> +		SYSERROR("failed to setns for CLONE_NEWNET");
> +		close(netns);
> +		goto out;
> +	}
> +	close(netns);
> +	return true;
> +out:
> +	return false;
> +}
> +

This is a lot of duplication.  I'd rather see them as one function.  Something
like:

bool switch_to_ns(pid_t pid, char *ns) {
	int fd, ret;
	char nspath[MAXPATHLEN];

	/* Switch to new netns */
	ret = snprintf(nspath, MAXPATHLEN, "/proc/%d/ns/%", pid, ns);
	if (ret < 0 || ret >= MAXPATHLEN)
		return false;

	fd = open(nspath, O_RDONLY);
	if (fd < 0) {
		SYSERROR("failed to open %s", new_netns_path);
		return false;
	}

	ret = setns(fd, 0);
	if (ret) {
		SYSERROR("failed to setns");
		close(fd);
		return false;
	}
	close(fd);
	return true;
}

>  /*
>   * looking at fs/proc_namespace.c, it appears we can
>   * actually expect the rootfs entry to very specifically contain
> diff --git a/src/lxc/utils.h b/src/lxc/utils.h
> index cdfe56a..6a92dbf 100644
> --- a/src/lxc/utils.h
> +++ b/src/lxc/utils.h
> @@ -283,3 +283,5 @@ char *on_path(char *cmd, const char *rootfs);
>  bool file_exists(const char *f);
>  char *choose_init(const char *rootfs);
>  int print_to_file(const char *file, const char *content);
> +bool switch_to_newuser(pid_t pid);
> +bool switch_to_newnet(pid_t pid);
> -- 
> 1.8.4.2
>