On Mon, Sep 07, 2015 at 05:10:27PM +0000, Serge Hallyn wrote:
> Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > Signed-off-by: Christian Brauner <christianvanbrau...@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.
>
> 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;
>
> but it's probably nicer to just add a short static wrapper function
> returning bool, something like
>
> 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);
Sorry, I get it. Will do!
> }
>
> > ERROR("Error destroying rootfs for %s", c->name);
> > goto out;
> > }
> > --
> > 2.5.1
> >
signature.asc
Description: PGP signature
_______________________________________________ lxc-devel mailing list lxc-devel@lists.linuxcontainers.org http://lists.linuxcontainers.org/listinfo/lxc-devel
