Re: [lxc-devel] [PATCH 3/4] Destroy bdevs using bdev_destroy() from bdev.h

2015-09-09 Thread Serge Hallyn 
Quoting Christian Brauner (christianvanbrau...@gmail.com):
> Signed-off-by: Christian Brauner 

Acked-by: Serge E. Hallyn 

> ---
>  src/lxc/lxccontainer.c | 62 
> --
>  1 file changed, 19 insertions(+), 43 deletions(-)
> 
> diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> index fb99892..0eaf98e 100644
> --- a/src/lxc/lxccontainer.c
> +++ b/src/lxc/lxccontainer.c
> @@ -2217,48 +2217,25 @@ static bool has_snapshots(struct lxc_container *c)
>   return count > 0;
>  }
>  
> +static bool do_destroy_container(struct lxc_conf *conf) {
> +if (am_unpriv()) {
> +if (userns_exec_1(conf, bdev_destroy_wrapper, conf) < 0)
> +return false;
> +return true;
> +}
> +return bdev_destroy(conf);
> +}
> +
>  static int lxc_rmdir_onedev_wrapper(void *data)
>  {
>   char *arg = (char *) data;
>   return lxc_rmdir_onedev(arg, "snaps");
>  }
>  
> -static int do_bdev_destroy(struct lxc_conf *conf)
> -{
> - struct bdev *r;
> - int ret = 0;
> -
> - r = bdev_init(conf, conf->rootfs.path, conf->rootfs.mount, NULL);
> - if (!r)
> - return -1;
> -
> - if (r->ops->destroy(r) < 0)
> - ret = -1;
> - bdev_put(r);
> - return ret;
> -}
> -
> -static int bdev_destroy_wrapper(void *data)
> -{
> - struct lxc_conf *conf = data;
> -
> - if (setgid(0) < 0) {
> - ERROR("Failed to setgid to 0");
> - return -1;
> - }
> - if (setgroups(0, NULL) < 0)
> - WARN("Failed to clear groups");
> - if (setuid(0) < 0) {
> - ERROR("Failed to setuid to 0");
> - return -1;
> - }
> - return do_bdev_destroy(conf);
> -}
> -
>  static bool container_destroy(struct lxc_container *c)
>  {
>   bool bret = false;
> - int ret;
> + int ret = 0;
>   struct lxc_conf *conf;
>  
>   if (!c || !do_lxcapi_is_defined(c))
> @@ -2310,16 +2287,13 @@ static bool container_destroy(struct lxc_container *c)
>   }
>   }
>  
> - if (conf && conf->rootfs.path && conf->rootfs.mount) {
> - if (am_unpriv())
> - ret = userns_exec_1(conf, bdev_destroy_wrapper, conf);
> - else
> - ret = do_bdev_destroy(conf);
> - if (ret < 0) {
> - ERROR("Error destroying rootfs for %s", c->name);
> - goto out;
> - }
> - }
> +if (conf && conf->rootfs.path && conf->rootfs.mount) {
> +if (!do_destroy_container(conf)) {
> +ERROR("Error destroying rootfs for %s", c->name);
> +goto out;
> +}
> +INFO("Destroyed rootfs for %s", c->name);
> +}
>  
>   mod_all_rdeps(c, false);
>  
> @@ -2334,6 +2308,8 @@ static bool container_destroy(struct lxc_container *c)
>   ERROR("Error destroying container directory for %s", c->name);
>   goto out;
>   }
> +INFO("Destroyed directory for %s", c->name);
> +
>   bret = true;
>  
>  out:
> -- 
> 2.5.1
> 
___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] [PATCH 3/4] Destroy bdevs using bdev_destroy() from bdev.h

2015-09-08 Thread Serge Hallyn 
Quoting Christian Brauner (christianvanbrau...@gmail.com):
> Signed-off-by: Christian Brauner 
> ---
>  src/lxc/lxccontainer.c | 60 
> ++
>  1 file changed, 17 insertions(+), 43 deletions(-)
> 
> diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> index fb99892..6cc3b08 100644
> --- a/src/lxc/lxccontainer.c
> +++ b/src/lxc/lxccontainer.c
> @@ -2217,48 +2217,25 @@ static bool has_snapshots(struct lxc_container *c)
>   return count > 0;
>  }
>  
> +static bool do_destroy_container(struct lxc_conf *conf) {
> +if (am_unpriv()) {
> +if (userns_exec_1(conf, bdev_destroy_wrapper, conf) < 0)
> +return false;
> +return true;
> +}
> +return bdev_destroy(conf);
> +}
> +
>  static int lxc_rmdir_onedev_wrapper(void *data)
>  {
>   char *arg = (char *) data;
>   return lxc_rmdir_onedev(arg, "snaps");
>  }
>  
> -static int do_bdev_destroy(struct lxc_conf *conf)
> -{
> - struct bdev *r;
> - int ret = 0;
> -
> - r = bdev_init(conf, conf->rootfs.path, conf->rootfs.mount, NULL);
> - if (!r)
> - return -1;
> -
> - if (r->ops->destroy(r) < 0)
> - ret = -1;
> - bdev_put(r);
> - return ret;
> -}
> -
> -static int bdev_destroy_wrapper(void *data)
> -{
> - struct lxc_conf *conf = data;
> -
> - if (setgid(0) < 0) {
> - ERROR("Failed to setgid to 0");
> - return -1;
> - }
> - if (setgroups(0, NULL) < 0)
> - WARN("Failed to clear groups");
> - if (setuid(0) < 0) {
> - ERROR("Failed to setuid to 0");
> - return -1;
> - }
> - return do_bdev_destroy(conf);
> -}
> -
>  static bool container_destroy(struct lxc_container *c)
>  {
>   bool bret = false;
> - int ret;
> + int ret = 0;
>   struct lxc_conf *conf;
>  
>   if (!c || !do_lxcapi_is_defined(c))
> @@ -2310,16 +2287,13 @@ static bool container_destroy(struct lxc_container *c)
>   }
>   }
>  
> - if (conf && conf->rootfs.path && conf->rootfs.mount) {
> - if (am_unpriv())
> - ret = userns_exec_1(conf, bdev_destroy_wrapper, conf);
> - else
> - ret = do_bdev_destroy(conf);
> - if (ret < 0) {
> - ERROR("Error destroying rootfs for %s", c->name);
> - goto out;
> - }
> - }
> +if (conf && conf->rootfs.path && conf->rootfs.mount) {
> +bret = do_destroy_container(conf);
> +if (!bret) {
> +ERROR("Error destroying rootfs for %s", c->name);
> +goto out;
> +}
> +}

Hm, unintended side-effect here - bret is now true, but if
lxc_rmdir_onedev() fails later then we goto out expecting
bret to be false.

>  
>   mod_all_rdeps(c, false);
>  
> -- 
> 2.5.1
> 
___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel