[lxc-devel] [PATCH] mod_rdep(): Write path and name of clone to file

[Serge Hallyn]

Quoting Christian Brauner (christianvanbrauner at 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 <christianvanbrauner at 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 at lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel