[lxc-devel] [PATCH 1/3] lxccontainer.c: split up cleate_run_template()

[Serge Hallyn]

Quoting TAMUKI Shoichi (tamuki at linet.gr.jp):
> Split figureout_rootfs() off from cleate_run_template() to allow
> common use of the function.

Hi,

this worries me actually.  You'll see why when I ask "who frees the
bdev you allocated?"

The code you're splitting off was previously only run from within a
short-lived task that will exit after the template runs.  But by
splitting it off into a common function, you have to make sure to
not leak memory etc.

It's possible that that's the only thing that needs to be cleaned
up is to get a strdup() of the bdev->dest, bdev_put(bdev), then
return the copy, and require (clearly comment) that callers
free the returned string.

Oh, no, you'll also have to stop doing exit(1) on failure paths.

> Signed-off-by: TAMUKI Shoichi <tamuki at linet.gr.jp>
> ---
>  src/lxc/lxccontainer.c | 104 +++++++++++++++++++++++++++----------------------
>  1 file changed, 57 insertions(+), 47 deletions(-)
> 
> diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> index 4f90f35..b2ecfb3 100644
> --- a/src/lxc/lxccontainer.c
> +++ b/src/lxc/lxccontainer.c
> @@ -876,6 +876,8 @@ static char *lxcbasename(char *path)
>  	return p;
>  }
>  
> +static char *figureout_rootfs(struct lxc_conf *conf);
> +
>  static bool create_run_template(struct lxc_container *c, char *tpath, bool quiet,
>  				char *const argv[])
>  {
> @@ -891,8 +893,7 @@ static bool create_run_template(struct lxc_container *c, char *tpath, bool quiet
>  	}
>  
>  	if (pid == 0) { // child
> -		char *patharg, *namearg, *rootfsarg, *src;
> -		struct bdev *bdev = NULL;
> +		char *patharg, *namearg, *rootfsarg, *rootfs;
>  		int i;
>  		int ret, len, nargs = 0;
>  		char **newargv;
> @@ -907,49 +908,7 @@ static bool create_run_template(struct lxc_container *c, char *tpath, bool quiet
>  			open("/dev/null", O_RDWR);
>  		}
>  
> -		src = c->lxc_conf->rootfs.path;
> -		/*
> -		 * for an overlay create, what the user wants is the template to fill
> -		 * in what will become the readonly lower layer.  So don't mount for
> -		 * the template
> -		 */
> -		if (strncmp(src, "overlayfs:", 10) == 0)
> -			src = overlay_getlower(src+10);
> -		if (strncmp(src, "aufs:", 5) == 0)
> -			src = overlay_getlower(src+5);
> -
> -		bdev = bdev_init(c->lxc_conf, src, c->lxc_conf->rootfs.mount, NULL);
> -		if (!bdev) {
> -			ERROR("Error opening rootfs");
> -			exit(1);
> -		}
> -
> -		if (geteuid() == 0) {
> -			if (unshare(CLONE_NEWNS) < 0) {
> -				ERROR("error unsharing mounts");
> -				exit(1);
> -			}
> -			if (detect_shared_rootfs()) {
> -				if (mount(NULL, "/", NULL, MS_SLAVE|MS_REC, NULL)) {
> -					SYSERROR("Failed to make / rslave to run template");
> -					ERROR("Continuing...");
> -				}
> -			}
> -		}
> -		if (strcmp(bdev->type, "dir") && strcmp(bdev->type, "btrfs")) {
> -			if (geteuid() != 0) {
> -				ERROR("non-root users can only create btrfs and directory-backed containers");
> -				exit(1);
> -			}
> -			if (bdev->ops->mount(bdev) < 0) {
> -				ERROR("Error mounting rootfs");
> -				exit(1);
> -			}
> -		} else { // TODO come up with a better way here!
> -			if (bdev->dest)
> -				free(bdev->dest);
> -			bdev->dest = strdup(bdev->src);
> -		}
> +		rootfs=figureout_rootfs(conf);
>  
>  		/*
>  		 * create our new array, pre-pend the template name and
> @@ -981,11 +940,11 @@ static bool create_run_template(struct lxc_container *c, char *tpath, bool quiet
>  			exit(1);
>  		newargv[2] = namearg;
>  
> -		len = strlen("--rootfs=") + 1 + strlen(bdev->dest);
> +		len = strlen("--rootfs=") + 1 + strlen(rootfs);
>  		rootfsarg = malloc(len);
>  		if (!rootfsarg)
>  			exit(1);
> -		ret = snprintf(rootfsarg, len, "--rootfs=%s", bdev->dest);
> +		ret = snprintf(rootfsarg, len, "--rootfs=%s", rootfs);
>  		if (ret < 0 || ret >= len)
>  			exit(1);
>  		newargv[3] = rootfsarg;
> @@ -1125,6 +1084,57 @@ static bool create_run_template(struct lxc_container *c, char *tpath, bool quiet
>  	return true;
>  }
>  
> +static char *figureout_rootfs(struct lxc_conf *conf)
> +{
> +	char *src;
> +	struct bdev *bdev = NULL;
> +
> +	src = conf->rootfs.path;
> +	/*
> +	 * for an overlay create, what the user wants is the template to fill
> +	 * in what will become the readonly lower layer.  So don't mount for
> +	 * the template
> +	 */
> +	if (strncmp(src, "overlayfs:", 10) == 0)
> +		src = overlay_getlower(src+10);
> +	if (strncmp(src, "aufs:", 5) == 0)
> +		src = overlay_getlower(src+5);
> +
> +	bdev = bdev_init(conf, src, conf->rootfs.mount, NULL);
> +	if (!bdev) {
> +		ERROR("Error opening rootfs");
> +		exit(1);
> +	}
> +
> +	if (geteuid() == 0) {
> +		if (unshare(CLONE_NEWNS) < 0) {
> +			ERROR("error unsharing mounts");
> +			exit(1);
> +		}
> +		if (detect_shared_rootfs()) {
> +			if (mount(NULL, "/", NULL, MS_SLAVE|MS_REC, NULL)) {
> +				SYSERROR("Failed to make / rslave to run template");
> +				ERROR("Continuing...");
> +			}
> +		}
> +	}
> +	if (strcmp(bdev->type, "dir") && strcmp(bdev->type, "btrfs")) {
> +		if (geteuid() != 0) {
> +			ERROR("non-root users can only create btrfs and directory-backed containers");
> +			exit(1);
> +		}
> +		if (bdev->ops->mount(bdev) < 0) {
> +			ERROR("Error mounting rootfs");
> +			exit(1);
> +		}
> +	} else { // TODO come up with a better way here!
> +		if (bdev->dest)
> +			free(bdev->dest);
> +		bdev->dest = strdup(bdev->src);
> +	}
> +	return bdev->dest;
> +}
> +
>  static bool prepend_lxc_header(char *path, const char *t, char *const argv[])
>  {
>  	long flen;
> -- 
> 1.9.0
> _______________________________________________
> lxc-devel mailing list
> lxc-devel at lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel