<p dir="ltr"><br>
On Oct 16, 2015 19:11, "Serge Hallyn" <<a href="mailto:serge.hallyn@ubuntu.com">serge.hallyn@ubuntu.com</a>> wrote:<br>
><br>
> Quoting Christian Brauner (<a href="mailto:christianvanbrauner@gmail.com">christianvanbrauner@gmail.com</a>):<br>
> > On Fri, Oct 16, 2015 at 03:52:04PM +0000, Serge Hallyn wrote:<br>
> > > Quoting Christian Brauner (<a href="mailto:christianvanbrauner@gmail.com">christianvanbrauner@gmail.com</a>):<br>
> > > > On Thu, Oct 15, 2015 at 08:32:25PM +0000, Serge Hallyn wrote:<br>
> > > > > Quoting Christian Brauner (<a href="mailto:christianvanbrauner@gmail.com">christianvanbrauner@gmail.com</a>):<br>
> > > > > > The mount_entry_create_*_dirs() functions currently assume that the rootfs of<br>
> > > > > > the container is actually named "rootfs". This has the consequence that<br>
> > > > > ><br>
> > > > > >       del = strstr(lxcpath, "/rootfs");<br>
> > > > > >       if (!del) {<br>
> > > > > >               free(lxcpath);<br>
> > > > > >               lxc_free_array((void **)opts, free);<br>
> > > > > >               return -1;<br>
> > > > > >       }<br>
> > > > > >       *del = '\0';<br>
> > > > > ><br>
> > > > > > will return NULL when the rootfs of a container is not actually named "rootfs".<br>
> > > > > > This means the we return -1 and do not create the necessary upperdir/workdir<br>
> > > > > > directories required for the overlay/aufs mount to work. Hence, let's not make<br>
> > > > > > that assumption. We now pass lxc_path and lxc_name to<br>
> > > > > > mount_entry_create_*_dirs() and create the path directly. To prevent failure we<br>
> > > > > > also have mount_entry_create_*_dirs() check that lxc_name and lxc_path are not<br>
> > > > > > empty when they are passed in.<br>
> > > > > ><br>
> > > > > > Signed-off-by: Christian Brauner <<a href="mailto:christianvanbrauner@gmail.com">christianvanbrauner@gmail.com</a>><br>
> > > > ><br>
> > > > > Yeah this was bugging me a few years ago.  Overall the patch looks fine<br>
> > > > > to me, I'm running a full testsuite to ease my mind about it.  Will ack<br>
> > > > > after that passes and I look over it again.<br>
> > > ><br>
> > > > We should also consider parsing path->rootfs when the container is an overlay or<br>
> > > > aufs backed container. Because in this case the right hand side of the check:<br>
> > > ><br>
> > > >           if ((strncmp(upperdir, lxcpath, dirlen) == 0) && (strncmp(upperdir, rootfs->path, rootfslen) != 0))<br>
> > > ><br>
> > > > will be trivially true since path->rootfs will e.g. be overlayfs:/path1:path2.<br>
> > > > Parsing path->rootfs to extract path2 before doing the second check would be<br>
> > > > safer... Thoughts?<br>
> > > ><br>
> > ><br>
> > > True that the current check is bogus.  But I think you just want to<br>
> > > use rootfs->mount instead of rootfs->path.  By the time this code<br>
> > > hits we have converted whatever target path the user gave us into<br>
> > > concat(rootfs->mount, process(target)) where process(x) will take<br>
> > > off a leading $lxcpath/$lxcname/rootfs or rootfs->path.<br>
> > ><br>
> > > -serge<br>
> ><br>
> > It's not bogus. It just misses the single case where the container itself is an<br>
> > overlay container. :)<br>
><br>
> Or blockd-dev-backed, or any case where lxc.rootfs is in a nonstandard<br>
> location - but that's ok,<br>
><br>
Right, I always forget the block-devs...</p>
<p dir="ltr">> > Let's merge this patch as it is.<br>
><br>
> Yeah, that's fine.<br>
><br>
> @Stgraber - please do apply this :)<br>
><br>
> > I will test whether rootfs->mount really is what we want and send a follow-up<br>
> > patch that is based off of this one. (I'll probably send it tomorrow.)<br>
><br>
> Great, thanks.<br>
> _______________________________________________<br>
> lxc-devel mailing list<br>
> <a href="mailto:lxc-devel@lists.linuxcontainers.org">lxc-devel@lists.linuxcontainers.org</a><br>
> <a href="http://lists.linuxcontainers.org/listinfo/lxc-devel">http://lists.linuxcontainers.org/listinfo/lxc-devel</a><br>
</p>