This is the new version of my modifications to mod_rdep(). It's part of a series
of commits present as a PR on Github which also includes the changes necessary
to adapt lxc-destroy: https://github.com/lxc/lxc/pull/641
Christian Brauner (1):
mod_rdep(): Write path and name of clone to file
src/lxc/lxccontainer.c | 164 ++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 128 insertions(+), 36 deletions(-)
--
2.5.0
On Thu, Aug 27, 2015 at 04:17:28PM +0000, Serge Hallyn wrote:
> Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > This is rather a first-pass suggestion than a full commit to discuss:
> >
> > The idea:
> > ---------
> >
> > - If a container has clone-snapshots created by
> >
> > lxc-clone -n NAME -N NEWNAME -s
> >
> > then even the new version lxc-destroy (see patch on github) cannot remove
> > the
> > container because the original container only stores the number of
> > clone-snapshotted containers in the file "lxc_snapshots". Hence, it has
> > no way
> > of looking up the copy-snapshots containers that belong to the container
> > to
> > delete them and then delete the original container. I suggest modifying
> > mod_rdep() so it will store not the plain number in the file but rather
> > the
> > paths and names to the containers that are a clone-snapshots (similar to
> > the
> > "lxc_rdepends" file for the clones). An example how the file
> > "lxc_snapshots"
> > of the original container could look like would be:
> >
> > /var/lib/lxc:bb
> > /var/lib/lxc:cc
> > /opt:dd
>
> Hi,
>
> this seems like a good idea to me. Thanks for working on it. A few notes
> below, but overall +1.
>
> > This would be an example of a container that provides the base for
> > three clone-snapshots bb, cc, and dd. Where bb and cc both are placed
> > in the usual path for privileged containers and dd is placed in a
> > custom path. I could then go on to modify lxc-destroy to actually not just
> > remove the original container including all snapshots but also with all
> > its
> > clone-snapshots.
> >
> > Following is a first idea for rewriting mod_rdep(). If you think its
> > generally
> > not something you want just leave a short reply. If you would like to have
> > this feature but rather implement it in a different way it would be nice
> > if
> > you could leave some more feedback and suggestions and then I'll rewrite
> > the
> > patch. Thanks!
> >
> > Summary:
> > --------
> >
> > - Add additional argument to function that takes in the clone-snapshotted
> > lxc_container.
> > - Have mod_rdep() write the path and name of the clone-snapshotted
> > container the
> > file lxc_snapshots of the original container.
> > - If a clone-snapshot gets deleted the corresponding line in the file
> > lxc_snapshot of the original container will be deleted and the file
> > updated.
> > - Change has_fs_snapshots() to check if the file is empty. If it is it is
> > safe
> > to delete the original container if the file is not empty then it is not
> > safe.
> >
> > Signed-off-by: Christian Brauner <christianvanbrau...@gmail.com>
> > ---
> > src/lxc/lxccontainer.c | 98
> > +++++++++++++++++++++++++++++++-------------------
> > 1 file changed, 61 insertions(+), 37 deletions(-)
> >
> > diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> > index 1c103e8..e10ccc7 100644
> > --- a/src/lxc/lxccontainer.c
> > +++ b/src/lxc/lxccontainer.c
> > @@ -1970,47 +1970,76 @@ out:
> >
> > WRAP_API_1(bool, lxcapi_save_config, const char *)
> >
> > -static bool mod_rdep(struct lxc_container *c, bool inc)
> > +static bool mod_rdep(struct lxc_container *c0, struct lxc_container *c,
> > bool inc)
> > {
> > - char path[MAXPATHLEN];
> > - int ret, v = 0;
> > - FILE *f;
> > + char *buf;
>
> please initialize buf to NULL.
>
> > + size_t len = 0;
> > + FILE *f1, *f2;
> > + char path1[MAXPATHLEN];
> > + char path2[MAXPATHLEN];
> > + int ret;
> > bool bret = false;
> >
> > - if (container_disk_lock(c))
> > + if (container_disk_lock(c0))
> > return false;
> > - ret = snprintf(path, MAXPATHLEN, "%s/%s/lxc_snapshots", c->config_path,
> > - c->name);
> > +
> > + ret = snprintf(path1, MAXPATHLEN, "%s/%s/lxc_snapshots",
> > c0->config_path,
> > + c0->name);
> > if (ret < 0 || ret > MAXPATHLEN)
> > goto out;
> > - f = fopen(path, "r");
> > - if (f) {
> > - ret = fscanf(f, "%d", &v);
> > - fclose(f);
> > - if (ret != 1) {
> > - ERROR("Corrupted file %s", path);
> > +
> > + if (inc) {
> > + f1 = fopen(path1, "a");
> > + if (!f1)
> > + goto out;
> > +
> > + if (fprintf(f1, "%s:%s\n", c->config_path, c->name) < 0) {
> > + ERROR("Error writing new snapshots value");
> > + fclose(f1);
> > goto out;
> > }
> > - }
> > - v += inc ? 1 : -1;
> > - f = fopen(path, "w");
> > - if (!f)
> > - goto out;
> > - if (fprintf(f, "%d\n", v) < 0) {
> > - ERROR("Error writing new snapshots value");
> > - fclose(f);
> > - goto out;
> > - }
> > - ret = fclose(f);
> > - if (ret != 0) {
> > - SYSERROR("Error writing to or closing snapshots file");
> > - goto out;
> > +
> > + ret = fclose(f1);
> > + if (ret != 0) {
> > + SYSERROR("Error writing to or closing snapshots file");
> > + goto out;
> > + }
> > + } else if (!inc) {
> > + ret = snprintf(path1, MAXPATHLEN, "%s/%s/lxc_snapshots",
> > + c0->config_path, c0->name);
> > + if (ret < 0 || ret > MAXPATHLEN)
> > + goto out;
> > + ret = snprintf(path2, MAXPATHLEN, "%s/%s/lxc_snapshots_tmp",
> > + c0->config_path, c0->name);
> > + if (ret < 0 || ret > MAXPATHLEN)
> > + goto out;
> > + f1 = fopen(path1, "r");
> > + if (!f1)
> > + goto out;
> > + f2 = fopen(path2, "w");
> > + if (!f2)
>
> close f1
>
> > + goto out;
> > + while (getline(&buf, &len, f1) != -1) {
> > + if (!strstr(strstr(buf, c->config_path), c->name)) {
> > + if (fprintf(f2, "%s", buf) < 0) {
> > + ERROR("Error writing new snapshots "
> > + "value");
> > + fclose(f2);
>
> close f1
>
> > + remove(path2);
> > + goto out;
> > + }
> > + }
> > + }
> > + free(buf);
> > + fclose(f1);
> > + fclose(f2);
> > + rename(path2, path1);
>
> I htink it's best to check for failures in these fclose
> and renames. If either fclose fails, you can salvage
> the old snapshots file. If rename fails, well you can at
> least tell the user he's hosed.
>
> > }
> >
> > bret = true;
> >
> > out:
> > - container_disk_unlock(c);
> > + container_disk_unlock(c0);
> > return bret;
> > }
> >
> > @@ -2052,7 +2081,7 @@ static void mod_all_rdeps(struct lxc_container *c,
> > bool inc)
> > lxcpath, lxcname);
> > continue;
> > }
> > - if (!mod_rdep(p, inc))
> > + if (!mod_rdep(p, c, inc))
> > ERROR("Failed to increase numsnapshots for %s:%s",
> > lxcpath, lxcname);
> > lxc_container_put(p);
> > @@ -2067,20 +2096,15 @@ static bool has_fs_snapshots(struct lxc_container
> > *c)
> > {
> > char path[MAXPATHLEN];
> > int ret, v;
> > - FILE *f;
> > + struct stat fbuf;
> > bool bret = false;
> >
> > ret = snprintf(path, MAXPATHLEN, "%s/%s/lxc_snapshots", c->config_path,
> > c->name);
> > if (ret < 0 || ret > MAXPATHLEN)
> > goto out;
> > - f = fopen(path, "r");
> > - if (!f)
> > - goto out;
> > - ret = fscanf(f, "%d", &v);
> > - fclose(f);
> > - if (ret != 1)
> > - goto out;
> > + stat(path, &fbuf);
>
> Check for stat failure here.
>
> > + v = fbuf.st_size;
> > bret = v != 0;
> >
> > out:
> > --
> > 2.5.0
> >
> > _______________________________________________
> > 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
