[lxc-devel] [PATCH v3] Parse rootfs->path

[Serge Hallyn]

Quoting Christian Brauner (christianvanbrauner at gmail.com):
> On Tue, Oct 20, 2015 at 04:31:19PM +0000, Serge Hallyn wrote:
> > Quoting Christian Brauner (christianvanbrauner at gmail.com):
> > > On Tue, Oct 20, 2015 at 03:30:19PM +0000, Serge Hallyn wrote:
> > > > Quoting Christian Brauner (christianvanbrauner at gmail.com):
> > > > > The mount_entry_overlay_dirs() and mount_entry_aufs_dirs() functions create
> > > > > workdirs and upperdirs for overlay and aufs lxc.mount.entry entries. They try
> > > > > to make sure that the workdirs and upperdirs can only be created under the
> > > > > containerdir (e.g. /path/to/the/container/CONTAINERNAME). In order to do this
> > > > > the right hand side of
> > > > > 
> > > > >                 if ((strncmp(upperdir, lxcpath, dirlen) == 0) && (strncmp(upperdir, rootfs->path, rootfslen) != 0))
> > > > > 
> > > > > was thought to check if the rootfs->path is not present in the workdir and
> > > > > upperdir mount options. But the current check is bogus since it will be
> > > > > trivially true whenever the container is a block-dev or overlay or aufs backed
> > > > > since the rootfs->path will then have a form like e.g.
> > > > > 
> > > > >         overlayfs:/some/path:/some/other/path
> > > > > 
> > > > > This patch adds the function ovl_get_rootfs_dir() which parses rootfs->path by
> > > > > searching backwards for the first occurrence of the delimiter pair ":/". We do
> > > > > not simply search for ":" since it might be used in path names. If ":/" is not
> > > > > found we assume the container is directory backed and simply return
> > > > > strdup(rootfs->path).
> > > > > 
> > > > > Signed-off-by: Christian Brauner <christianvanbrauner at gmail.com>
> > > > > ---
> > > > >  src/lxc/conf.c | 115 ++++++++++++++++++++++++++++++++++++++++++++-------------
> > > > >  1 file changed, 90 insertions(+), 25 deletions(-)
> > > > > 
> > > > > diff --git a/src/lxc/conf.c b/src/lxc/conf.c
> > > > > index 16a62f8..301fe50 100644
> > > > > --- a/src/lxc/conf.c
> > > > > +++ b/src/lxc/conf.c
> > > > > @@ -1815,12 +1815,65 @@ static void cull_mntent_opt(struct mntent *mntent)
> > > > >  	}
> > > > >  }
> > > > >  
> > > > > +static char *ovl_get_rootfs_dir(const char *rootfs_path, size_t *rootfslen)
> > > > > +{
> > > > > +	char *end = NULL;
> > > > > +	char *s1 = NULL;
> > > > > +	char *s2 = NULL;
> > > > > +	char *rootfsdir = NULL;
> > > > > +	char *tmp = NULL;
> > > > > +	size_t len = 0;
> > > > > +	size_t slen = 0;
> > > > > +
> > > > > +	if (!rootfs_path || !rootfslen )
> > > > > +		return NULL;
> > > > > +
> > > > > +	*rootfslen = 0;
> > > > > +
> > > > > +	s1 = strdup(rootfs_path);
> > > > > +	if (!s1)
> > > > > +		return NULL;
> > > > > +
> > > > > +	s2 = malloc(strlen(rootfs_path) + 1);
> > > > > +	if (!s2) {
> > > > > +		free(s1);
> > > > > +		return NULL;
> > > > > +	}
> > > > > +	end = stpcpy(s2, rootfs_path);
> > > > > +
> > > > > +	/* If we find :/ in rootfs_path it means we either have a block-dev or
> > > > > +	 * overlay or aufs container. */
> > > > > +	while ((tmp = strrchr(s1, ':'))) {
> > > > > +		len = strlen(tmp);
> > > > > +		*rootfslen += len;
> > > > 
> > > > If it is "overlay:/some/rootfs:/some/delta0", you will find the
> > > > delta0 and return it as rootfsdir?
> > > > 
> > > > (or am i misreading?)
> > > Nope, I thought that in the case where the /some/rootfs refers to another
> > > containers rootfs it doesn't make sense to check against that path since this is
> > > covered by the left hand side of the check further down. But in the case of
> > > purely overlay-backed container it doesn't make sense... I'll rewrite that
> > > part. Are there any more special cases apart from aufs and overlay. We have
> > > 
> > > blockdev:/some/path
> > > 
> > > overlayfs:/some/path:/some/delta0
> > > aufs:/some/path:/some/delta0
> > > 
> > > Any more?
> > 
> > well bdev.c also looks for
> > 
> > loop:, lvm:, nbd:, and even dir:.  Not sure anyone uses these.
> >
> 
> But do they all have the rootfs path after the first ":/" delimiter? If so we
> can abstract here and don't need to check for every backing storage type.
> (Sorry, I'm more familiar with the codebase now but not so intimate as to be
> able to answer that with certainty.)

Yeah, well the rootfs source (i.e. nbd:/dev/nbd0)

Like I said I think t his is redundant for these, and noone afaik has
ever used them, somaybe we should drop them.