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. > + 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 > 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