[lxc-devel] [PATCH 5/6] Destroy bdevs using new bdev_destroy() from bdev.h

[Christian Brauner]

On Mon, Sep 07, 2015 at 05:10:27PM +0000, Serge Hallyn wrote:
> Quoting Christian Brauner (christianvanbrauner at gmail.com):
> > Signed-off-by: Christian Brauner <christianvanbrauner at gmail.com>
> > 
> >  100.0% src/lxc/
> > diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> > index 2103437..9f22fdc 100644
> > --- a/src/lxc/lxccontainer.c
> > +++ b/src/lxc/lxccontainer.c
> > @@ -2213,21 +2213,6 @@ static int lxc_rmdir_onedev_wrapper(void *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;
> > @@ -2242,13 +2227,16 @@ static int bdev_destroy_wrapper(void *data)
> >  		ERROR("Failed to setuid to 0");
> >  		return -1;
> >  	}
> > -	return do_bdev_destroy(conf);
> > +	if (!bdev_destroy(conf))
> > +		return -1;
> > +	else
> > +		return 0;
> >  }
> >  
> >  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))
> > @@ -2304,8 +2292,8 @@ static bool container_destroy(struct lxc_container *c)
> >  		if (am_unpriv())
> >  			ret = userns_exec_1(conf, bdev_destroy_wrapper, conf);
> >  		else
> > -			ret = do_bdev_destroy(conf);
> > -		if (ret < 0) {
> > +			bret = bdev_destroy(conf);
> > +		if (ret < 0 || !bret) {
> 
> I don't see how this can work - if the container is unprivileged, bret
> will remain false as initialized, so an error will always be raised.

This has been fixed.

> 
> I think it's better to have a single return value from this if/else
> block.  You could just do
> 
> 	bret = userns_exec_1(conf, bdev_destroy_wrapper, conf) ? 0 : -1;

Yes, this is the solution we should use. But I think you meant:

        ret = bdev_destroy(conf) ? 0 : -1;

> 
> but it's probably nicer to just add a short static wrapper function
> returning bool, something like

Not possible, since userns_exec_1() expects a callback function pointer of type
        int (*fn)(void *)

> 
> static bool do_destroy_container(struct lxc_container *conf) {
> 	if (am_unpriv()) {
> 		if (userns_exec_1(conf, bdev_destroy_wrapper, conf) < 0)
> 			return false;
> 		return true;
> 	}
> 	return bdev_destroy(conf);
> }
> 
> >  			ERROR("Error destroying rootfs for %s", c->name);
> >  			goto out;
> >  		}
> > -- 
> > 2.5.1
> > 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20150908/9c20f001/attachment.sig>