[lxc-devel] [PATCH 1/2]: Ensure freezer state has changed

[Matt Helsley]

On Fri, Jul 09, 2010 at 07:51:32PM -0700, Sukadev Bhattiprolu wrote:
> 
> From: Sukadev Bhattiprolu <sukadev at linux.vnet.ibm.com>
> Subject: [PATCH 1/2] Ensure frezer state has changed
> 
> A write to the freezer.state file does not gurantee that the state has
> changed. To ensure that the freezer state is either FROZEN or THAWED,
> read the freezer state and if it has not changed, repeat the write.

Technically this is only necessary for the THAWED -> FROZEN
transition. In other words, if we're FROZEN and write THAWED then
we don't need to read the state. However, it doesn't hurt to check.

Reviewed-by: Matt Helsley <matthltc at us.ibm.com>

> 
> Changelog[v2]:
> 	- Minor reorg of code
> 	- Comments from Daniel Lezcano:
> 		- lseek() before each read/write of freezer.state
> 		- Have lxc_freeze_unfreeze() return -1 on error
> 
> Signed-off-by: Sukadev Bhattiprolu <sukadev at linux.vnet.ibm.com>
> ---
>  src/lxc/freezer.c |   50 +++++++++++++++++++++++++++++++++++++++++++-------
>  1 files changed, 43 insertions(+), 7 deletions(-)
> 
> diff --git a/src/lxc/freezer.c b/src/lxc/freezer.c
> index cff954e..d144f89 100644
> --- a/src/lxc/freezer.c
> +++ b/src/lxc/freezer.c
> @@ -42,6 +42,7 @@ static int freeze_unfreeze(const char *name, int freeze)
>  {
>  	char *nsgroup;
>  	char freezer[MAXPATHLEN], *f;
> +	char tmpf[32];
>  	int fd, ret;
>  	
>  	ret = lxc_cgroup_path_get(&nsgroup, name);
> @@ -50,7 +51,7 @@ static int freeze_unfreeze(const char *name, int freeze)
> 
>  	snprintf(freezer, MAXPATHLEN, "%s/freezer.state", nsgroup);
> 
> -	fd = open(freezer, O_WRONLY);
> +	fd = open(freezer, O_RDWR);
>  	if (fd < 0) {
>  		SYSERROR("failed to open freezer for '%s'", name);
>  		return -1;
> @@ -58,22 +59,57 @@ static int freeze_unfreeze(const char *name, int freeze)
> 
>  	if (freeze) {
>  		f = "FROZEN";
> -		ret = write(fd, f, strlen(f) + 1) < 0;
> +		ret = write(fd, f, strlen(f) + 1);
>  	} else {
>  		f = "THAWED";
> -		ret = write(fd, f, strlen(f) + 1) < 0;
> +		ret = write(fd, f, strlen(f) + 1);
> 
>  		/* compatibility code with old freezer interface */
> -		if (ret) {
> +		if (ret < 0) {
>  			f = "RUNNING";

Just an informational note that occurred to me while reviewing this:

This only shows up for a single commit in the mainline kernel. The very next
commit changed the string to "THAWED". It doesn't even show up in an -rcX
kernel. So the only good reason to have it is for reliable kernel bisection
of any bug cases involving lxc.

>  			ret = write(fd, f, strlen(f) + 1) < 0;
>  		}
>  	}
> 
> -	close(fd);
> -	if (ret) 
> -		SYSERROR("failed to write to '%s'", freezer);
> +	if (ret < 0) {
> +		SYSERROR("failed to write '%s' to '%s'", f, freezer);
> +		goto out;
> +	}
> +
> +	while (1) {

You could shift the write to the top of the loop body and eliminate the
writes above I think.

> +		ret = lseek(fd, 0L, SEEK_SET);
> +		if (ret < 0) {
> +			SYSERROR("failed to lseek on file '%s'", freezer);
> +			goto out;
> +		}
> +
> +		ret = read(fd, tmpf, sizeof(tmpf));
> +		if (ret < 0) {
> +			SYSERROR("failed to read to '%s'", freezer);
> +			goto out;
> +		}
> +
> +		ret = strncmp(f, tmpf, strlen(f));
> +		if (!ret)
> +			break;		/* Success */
> 
> +		sleep(1);
> +
> +		ret = lseek(fd, 0L, SEEK_SET);
> +		if (ret < 0) {
> +			SYSERROR("failed to lseek on file '%s'", freezer);
> +			goto out;
> +		}
> +
> +		ret = write(fd, f, strlen(f) + 1); 
> +		if (ret < 0) {
> +			SYSERROR("failed to write '%s' to '%s'", f, freezer);
> +			goto out;
> +		}
> +	}
> +
> +out:
> +	close(fd);
>  	return ret;
>  }
> 
> -- 
> 1.6.0.4
>