[CRIU] [PATCH 2/2] Cgroup property restoration infrastructure

[Pavel Emelyanov]

On 08/05/2014 11:50 PM, gbellack at google.com wrote:
> From: Garrison Bellack <gbellack at google.com>
> 
> Restores 2 cgroup properties after the criu restoration of tasks.
> Currently the cgroup files to be restored are static but
> are easily extendable. If a cgroup exists during restoration,
> its properties will not be overwritten.
> Work based off Tycho Anderson tycho.andersen at canonical.com

Thanks, Garrison! I like the patch, only a couple of small comments inline.

> --- a/cgroup.c
> +++ b/cgroup.c
> @@ -20,6 +20,23 @@
>  #include "protobuf/cgroup.pb-c.h"
>  
>  /*
> + * These string arrays have the names of all the properties that will be
> + * restored. To add a property for a cgroup type, add it to the
> + * corresponding char array above the NULL terminator.
> + * Currently the code only supports properties with 1 value
> + */
> +
> +static const char *cpu_props[] = {
> +	"cpu.shares",
> +	NULL
> +};
> +
> +static const char *memory_props[] = {
> +	"memory.limit_in_bytes",
> +	NULL
> +};

Would you accomplish these lists?

> +
> +/*
>   * This structure describes set of controller groups
>   * a task lives in. The cg_ctl entries are stored in
>   * the @ctls list sorted by the .name field and then

> @@ -267,6 +407,16 @@ static int add_cgroup(const char *fpath, const struct stat *sb, int typeflag)
>  
>  		INIT_LIST_HEAD(&ncd->children);
>  		ncd->n_children = 0;
> +
> +		INIT_LIST_HEAD(&ncd->properties);
> +		ncd->n_properties = 0;
> +		if (add_cgroup_properties(fpath, &ncd->properties,
> +					  &ncd->n_properties,
> +					  current_controller) < 0) {

Presumably we can feed the ncd itself into add_cgroup_properties and
let the latter init and set list and n_properties itself.

> +			ret = -1;
> +			goto out;
> +		}
> +
>  		return 0;
>  	} else
>  		return 0;

> @@ -659,6 +857,126 @@ void fini_cgroup(void)
>  	xfree(cg_yard);
>  }
>  
> +static int restore_cgroup_prop(const CgroupPropEntry * cg_prop_entry_p,
> +			       const char *cname, const char *dir, const int *cg_p)
> +{
> +	char path[PATH_MAX];
> +	FILE *f;
> +
> +	if (!cg_prop_entry_p->value)

How can this happen? Would you add a code comment here?

> +		return 0;
> +
> +	if (snprintf(path, PATH_MAX, "%s/%s/%s", cname, dir, cg_prop_entry_p->name) >= PATH_MAX) {
> +		pr_err("snprintf output was truncated for %s\n", cg_prop_entry_p->name);
> +		return -1;
> +	}
> +
> +	f = fopenat(*cg_p, path, "w+");
> +	if (!f) {
> +		pr_perror("Failed opening %s for writing\n", path);
> +		return -1;
> +	}
> +
> +	if (fprintf(f, "%s", cg_prop_entry_p->value) < 0) {
> +		fclose(f);
> +		pr_err("Failed writing %s to %s\n", cg_prop_entry_p->value, path);
> +		return -1;
> +	}
> +
> +	if (fclose(f) != 0) {
> +		pr_err("Failed closing %s\n", path);
> +		return -1;
> +	}
> +
> +	pr_info("Restored cgroup property value %s to %s\n", cg_prop_entry_p->value, path);
> +	return 0;
> +}
> +
> +static int prepare_cgroup_dir_properties(char *controller, CgroupDirEntry **ents,
> +					 unsigned int n_ents)
> +{
> +	unsigned int i, j;
> +	int cg;
> +
> +	cg = get_service_fd(CGROUP_YARD);
> +
> +	for (i = 0; i < n_ents; i++) {
> +		CgroupDirEntry *e = ents[i];
> +
> +		/*
> +		 * Check to see if we made e->properties NULL during restore
> +		 * because directory already existed and as such we don't want to
> +		 * change its properties
> +		 */
> +		if (e->properties) {
> +			for (j = 0; j < e->n_properties; ++j) {
> +				if (restore_cgroup_prop(e->properties[j], controller, e->path, &cg) < 0)

The cg can be passed as value, instead of pointer.

> +					return -1;
> +			}
> +		}
> +
> +		if (prepare_cgroup_dir_properties(controller, e->children, e->n_children) < 0)
> +			return -1;
> +	}
> +
> +	return 0;
> +}

> --- a/protobuf/cgroup.proto
> +++ b/protobuf/cgroup.proto
> @@ -1,6 +1,12 @@
> +message cgroup_prop_entry {
> +	required string			name		= 1;
> +	required string			value		= 2;
> +}
> +
>  message cgroup_dir_entry {
>  	required string 		path		= 1;
> -	repeated cgroup_dir_entry	children 	= 4;
> +	repeated cgroup_dir_entry	children 	= 2;

I'd appreciate if protobuf field renumbering goes in separate (quite small) patch.

> +	repeated cgroup_prop_entry	properties	= 3;
>  }
>  
>  message cg_controller_entry {
>