Quoting Christian Brauner (christianvanbrau...@gmail.com): > On Thu, Oct 22, 2015 at 01:13:35PM +0000, Serge Hallyn wrote: > > Quoting Christian Brauner (christianvanbrau...@gmail.com): > > > On Tue, Oct 20, 2015 at 04:17:18PM +0000, Serge Hallyn wrote: > > > > Quoting Christian Brauner (christianvanbrau...@gmail.com): > > > > > When using overlay and aufs mounts with lxc.mount.entry users have to > > > > > specify > > > > > absolute paths for upperdir and workdir which will then get created > > > > > automatically by mount_entry_create_overlay_dirs() and > > > > > mount_entry_create_aufs_dirs() in conf.c. When we clone a container > > > > > with > > > > > overlay or aufs lxc.mount.entry entries we need to update these > > > > > absolute paths. > > > > > In order to do this we add the function > > > > > update_union_mount_entry_paths() in > > > > > lxccontainer.c. The function updates the mounts in two locations: > > > > > > > > > > 1) lxc_conf->mount_list > > > > > > > > > > and > > > > > > > > > > 2) lxc_conf->unexpanded_config > > > > > > > > > > If we were to only update 2) we would end up with wrong upperdir and > > > > > workdir > > > > > mounts as the absolute paths would still point to the container that > > > > > serves as > > > > > the base for the clone. If we were to only update 1) we would end up > > > > > with wrong > > > > > upperdir and workdir lxc.mount.entry entries in the clone's config as > > > > > the > > > > > absolute paths in upperdir and workdir would still point to the > > > > > container that > > > > > serves as the base for the clone. Updating both will get the job > > > > > done. Note, > > > > > that an entry in lxc_conf->mount_list will only be updated if it is > > > > > also found > > > > > in the clones config. Mounts from other files are hence not updated. > > > > > In short, > > > > > automatic overlay and aufs mounts with lxc.mount.entry should only be > > > > > specified > > > > > in the containers own config. > > > > > > > > > > NOTE: This function does not sanitize paths apart from removing > > > > > trailing > > > > > slashes. (So when a user specifies //home//someone/// it will be > > > > > cleaned to > > > > > //home//someone. This is the minimal path cleansing which is also > > > > > done by > > > > > lxc_container_new().) But the mount_entry_create_overlay_dirs() and > > > > > mount_entry_create_aufs_dirs() functions both try to be extremely > > > > > strict about > > > > > when to create upperdirs and workdirs. They will only accept > > > > > sanitized paths, > > > > > i.e. they require /home/someone. I think this is a (safety) virtue > > > > > and we > > > > > should consider sanitizing paths in general. In short: > > > > > update_union_mount_entry_paths() does update all absolute paths to > > > > > the new > > > > > container but mount_entry_create_overlay_dirs() and > > > > > mount_entry_create_aufs_dirs() will still refuse to create upperdir > > > > > and workdir > > > > > when the updated path is unclean. This happens easily when e.g. a > > > > > user calls > > > > > lxc-clone -o OLD -n NEW -P //home//chb///. > > > > > > > > > > Signed-off-by: Christian Brauner <christianvanbrau...@gmail.com> > > > > > --- > > > > > src/lxc/lxccontainer.c | 133 > > > > > ++++++++++++++++++++++++++++++++++++++++++++++++- > > > > > 1 file changed, 132 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c > > > > > index 42e23e7..5ed7697 100644 > > > > > --- a/src/lxc/lxccontainer.c > > > > > +++ b/src/lxc/lxccontainer.c > > > > > @@ -2894,6 +2894,130 @@ static int create_file_dirname(char *path, > > > > > struct lxc_conf *conf) > > > > > return ret; > > > > > } > > > > > > > > > > +/* When we clone a container with overlay or aufs lxc.mount.entry > > > > > entries we > > > > > +* need to update these absolute paths. In order to do this we add > > > > > the function > > > > > +* update_union_mount_entry_paths() in lxccontainer.c. The function > > > > > operates on > > > > > +* c->lxc_conf->unexpanded_config instead of the intuitively > > > > > plausible > > > > > +* c->lxc_conf->mount_list because the latter also contains mounts > > > > > from other > > > > > +* files as well as generic mounts. */ > > > > > +static int update_union_mount_entry_paths(struct lxc_conf *lxc_conf, > > > > > + const char *lxc_path, > > > > > + const char *lxc_name, > > > > > + const char *newpath, > > > > > + const char *newname) > > > > > +{ > > > > > + char new_upper[MAXPATHLEN]; > > > > > + char new_work[MAXPATHLEN]; > > > > > + char old_upper[MAXPATHLEN]; > > > > > + char old_work[MAXPATHLEN]; > > > > > + char *cleanpath = NULL; > > > > > + char *mnt_entry = NULL; > > > > > + char *new_mnt_entry = NULL; > > > > > + char *new_unexpanded_config = NULL; > > > > > + char *tmp_mnt_entry = NULL; > > > > > + char *tmp_unexpanded_config = NULL; > > > > > + char *tmp = NULL; > > > > > + int ret = 0; > > > > > + size_t len = 0; > > > > > + struct lxc_list *iterator; > > > > > + > > > > > + cleanpath = strdup(newpath); > > > > > + if (!cleanpath) > > > > > + goto err; > > > > > + > > > > > + remove_trailing_slashes(cleanpath); > > > > > + > > > > > + ret = snprintf(old_work, MAXPATHLEN, "workdir=%s/%s", lxc_path, > > > > > lxc_name); > > > > > + if (ret < 0 || ret >= MAXPATHLEN) > > > > > + goto err; > > > > > + > > > > > + ret = snprintf(new_work, MAXPATHLEN, "workdir=%s/%s", > > > > > cleanpath, newname); > > > > > + if (ret < 0 || ret >= MAXPATHLEN) > > > > > + goto err; > > > > > + > > > > > + lxc_list_for_each(iterator, &lxc_conf->mount_list) { > > > > > > > > put > > > > > > > > char *tmp = NULL, new_mnt_entry = NULL; > > > > char *mnt_entry; > > > > > > > > here (and remove from above etc). > > > > > > > > > + mnt_entry = iterator->elem; > > > > > + > > > > > + if (strstr(mnt_entry, "overlay")) > > > > > + tmp = "upperdir"; > > > > > + else if (strstr(mnt_entry, "aufs")) > > > > > + tmp = "br"; > > > > > + > > > > > + if (!tmp) > > > > > + continue; > > > > > + > > > > > + if (strstr(lxc_conf->unexpanded_config, mnt_entry)) { > > > > > + ret = snprintf(old_upper, MAXPATHLEN, > > > > > "%s=%s/%s", tmp, lxc_path, lxc_name); > > > > > + if (ret < 0 || ret >= MAXPATHLEN) > > > > > + goto err; > > > > > > > > Alternate suggestion. How about using clear_unexp_config_line() to > > > > remove all > > > > lxc.mount.entry lines from unexpanded config first, then run through > > > > this loop > > > > and simply add the new ones? > > > > > > > > The lxc_string_replace approach feels problematic. > > > > > > I thought so too, but clear_unexp_config_line() will not just clear mount > > > entries from the containers config but also entries from files like > > > common.conf > > > which are also present in lxc_conf->mount_list e.g.: > > > > Hm, might be the other way around, but yeah we can't do that. > > > > But if the config file has say a commented version of the line before > > the real line, you'll update only the comment right? > > > > So can you do lik eclear_unexp_config_line() does and make sure you > > are looking for only lines beginning with "lxc.mount.entry += +" > > and then have the mnt_entry? I guess that should do. > > > > I'll test that and get back with an updated patch. I *try* to think of > something > smarter... Do you want me to avoid lxc_string_replace()?
Not because of lxc_string_replace itself, but because it won't confine itself to one ^.*$ line. So if you cna make it work, that's fine with me, but it feels like a wrecking ball in our unexpanded_config china shop. > > The need for unexpanded config is very unfortunate. > Well, we won't be able to work around it. Especially when starting an > ephemeral I just meant in general. (But it's the best option I've seen so far) > clone with lxc.mount.entry entries for overlay/aufs dir we need to update both > otherwise we'll end up either with a wrong config or with wrong entries in > lxc_conf->mount_list when we start it immediately after having cloned it. > > (The only other option is to have users specify relative paths but (as you > argued) this might confuse them since these would be relative paths to the > containerdir and not the rootfs.) > _______________________________________________ > lxc-devel mailing list > lxc-devel@lists.linuxcontainers.org > http://lists.linuxcontainers.org/listinfo/lxc-devel _______________________________________________ lxc-devel mailing list lxc-devel@lists.linuxcontainers.org http://lists.linuxcontainers.org/listinfo/lxc-devel