On Tue, Oct 20, 2015 at 04:17:18PM +0000, Serge Hallyn wrote: > Quoting Christian Brauner ([email protected]): > > 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 <[email protected]> > > --- > > 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.:
lxc.mount.entry = /sys/fs/fuse/connections sys/fs/fuse/connections none
bind,optional 0 0
We need to preserve the old ones somehow.
>
> > + ret = snprintf(new_upper, MAXPATHLEN, "%s=%s/%s", tmp,
> > cleanpath, newname);
> > + if (ret < 0 || ret >= MAXPATHLEN)
> > + goto err;
> > +
> > + if (strstr(mnt_entry, old_upper)) {
> > + tmp_mnt_entry = lxc_string_replace(old_upper,
> > new_upper, mnt_entry);
> > + tmp_unexpanded_config =
> > lxc_string_replace(old_upper, new_upper, lxc_conf->unexpanded_config);
> > + }
> > +
> > + if (strstr(mnt_entry, old_work)) {
> > + if (tmp_mnt_entry)
> > + new_mnt_entry =
> > lxc_string_replace(old_work, new_work, tmp_mnt_entry);
> > + if (tmp_unexpanded_config)
> > + new_unexpanded_config =
> > lxc_string_replace(old_work, new_work, tmp_unexpanded_config);
> > + }
> > +
> > + if (new_mnt_entry) {
> > + free(iterator->elem);
> > + iterator->elem = strdup(new_mnt_entry);
> > + } else if (tmp_mnt_entry) {
> > + free(iterator->elem);
> > + iterator->elem = strdup(tmp_mnt_entry);
> > + }
> > +
> > + if (new_unexpanded_config) {
> > + free(lxc_conf->unexpanded_config);
> > + lxc_conf->unexpanded_config =
> > strdup(new_unexpanded_config);
> > + } else if (tmp_unexpanded_config) {
> > + free(lxc_conf->unexpanded_config);
> > + lxc_conf->unexpanded_config =
> > strdup(tmp_unexpanded_config);
> > + }
> > +
> > + if ((new_unexpanded_config || tmp_unexpanded_config) &&
> > lxc_conf->unexpanded_config) {
> > + len = strlen(lxc_conf->unexpanded_config);
> > + lxc_conf->unexpanded_len = len;
> > + lxc_conf->unexpanded_alloced = len + 1;
> > + }
> > +
> > + free(new_mnt_entry);
> > + free(new_unexpanded_config);
> > + free(tmp_mnt_entry);
> > + free(tmp_unexpanded_config);
>
> In general, when you have to free all the same things as you do in the 'out'
> label, it's better to keep a function return variable which stays -1, then
> below where you do 'return 0' do 'ret = 0' instead and fall through the label.
>
> > + mnt_entry = NULL;
> > + new_mnt_entry = NULL;
> > + new_unexpanded_config = NULL;
> > + tmp_mnt_entry = NULL;
> > + tmp_unexpanded_config = NULL;
> > + tmp = NULL;
> > +
> > + if (!iterator->elem || !lxc_conf->unexpanded_config)
> > + goto err;
> > + }
> > + }
> > + free(cleanpath);
> > + return 0;
> > +
> > +err:
> > + free(cleanpath);
> > + free(new_mnt_entry);
> > + free(new_unexpanded_config);
> > + free(tmp_mnt_entry);
> > + free(tmp_unexpanded_config);
> > + INFO("%s", "Failed to update config file");
> > + return -1;
> > +}
> > +
> > static struct lxc_container *do_lxcapi_clone(struct lxc_container *c,
> > const char *newname,
> > const char *lxcpath, int flags,
> > const char *bdevtype, const char *bdevdata, uint64_t newsize,
> > @@ -2953,7 +3077,9 @@ static struct lxc_container *do_lxcapi_clone(struct
> > lxc_container *c, const char
> > fclose(fout);
> > c->lxc_conf->rootfs.path = origroot;
> >
> > - sprintf(newpath, "%s/%s/rootfs", lxcpath, newname);
> > + ret = snprintf(newpath, MAXPATHLEN, "%s/%s/rootfs", lxcpath, newname);
> > + if (ret < 0 || ret >= MAXPATHLEN)
> > + goto out;
> > if (mkdir(newpath, 0755) < 0) {
> > SYSERROR("error creating %s", newpath);
> > goto out;
> > @@ -3009,10 +3135,15 @@ static struct lxc_container *do_lxcapi_clone(struct
> > lxc_container *c, const char
> > }
> > }
> >
> > + // update absolute paths for union mount directories
> > + if (update_union_mount_entry_paths(c2->lxc_conf, c->config_path,
> > c->name, lxcpath, newname) < 0)
> > + goto out;
> > +
> > // We've now successfully created c2's storage, so clear it out if we
> > // fail after this
> > storage_copied = 1;
> >
> > +
> > if (!c2->save_config(c2, NULL))
> > goto out;
> >
> > --
> > 2.6.1
> >
> > _______________________________________________
> > lxc-devel mailing list
> > [email protected]
> > http://lists.linuxcontainers.org/listinfo/lxc-devel
> _______________________________________________
> lxc-devel mailing list
> [email protected]
> http://lists.linuxcontainers.org/listinfo/lxc-devel
signature.asc
Description: PGP signature
_______________________________________________ lxc-devel mailing list [email protected] http://lists.linuxcontainers.org/listinfo/lxc-devel
