<div dir="ltr">I have made the changes you requested. The exact properties to be restored are actually different here internally and I'm not sure specifically which properties upstream wants included and which they don't, can I send the completion of the properties list as a separate patch? For now I have separated them and I will send both patches momentarily. </div>
<div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Aug 5, 2014 at 10:58 PM, Pavel Emelyanov <span dir="ltr"><<a href="mailto:xemul@parallels.com" target="_blank">xemul@parallels.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">On 08/05/2014 11:50 PM, <a href="mailto:gbellack@google.com">gbellack@google.com</a> wrote:<br>
> From: Garrison Bellack <<a href="mailto:gbellack@google.com">gbellack@google.com</a>><br>
><br>
> Restores 2 cgroup properties after the criu restoration of tasks.<br>
> Currently the cgroup files to be restored are static but<br>
> are easily extendable. If a cgroup exists during restoration,<br>
> its properties will not be overwritten.<br>
> Work based off Tycho Anderson <a href="mailto:tycho.andersen@canonical.com">tycho.andersen@canonical.com</a><br>
<br>
</div>Thanks, Garrison! I like the patch, only a couple of small comments inline.<br>
<div class=""><br>
> --- a/cgroup.c<br>
> +++ b/cgroup.c<br>
> @@ -20,6 +20,23 @@<br>
> #include "protobuf/cgroup.pb-c.h"<br>
><br>
> /*<br>
> + * These string arrays have the names of all the properties that will be<br>
> + * restored. To add a property for a cgroup type, add it to the<br>
> + * corresponding char array above the NULL terminator.<br>
> + * Currently the code only supports properties with 1 value<br>
> + */<br>
> +<br>
> +static const char *cpu_props[] = {<br>
> + "cpu.shares",<br>
> + NULL<br>
> +};<br>
> +<br>
> +static const char *memory_props[] = {<br>
> + "memory.limit_in_bytes",<br>
> + NULL<br>
> +};<br>
<br>
</div>Would you accomplish these lists?<br>
<div class=""><br>
> +<br>
> +/*<br>
> * This structure describes set of controller groups<br>
> * a task lives in. The cg_ctl entries are stored in<br>
> * the @ctls list sorted by the .name field and then<br>
<br>
</div><div class="">> @@ -267,6 +407,16 @@ static int add_cgroup(const char *fpath, const struct stat *sb, int typeflag)<br>
><br>
> INIT_LIST_HEAD(&ncd->children);<br>
> ncd->n_children = 0;<br>
> +<br>
> + INIT_LIST_HEAD(&ncd->properties);<br>
> + ncd->n_properties = 0;<br>
> + if (add_cgroup_properties(fpath, &ncd->properties,<br>
> + &ncd->n_properties,<br>
> + current_controller) < 0) {<br>
<br>
</div>Presumably we can feed the ncd itself into add_cgroup_properties and<br>
let the latter init and set list and n_properties itself.<br>
<div class=""><br>
> + ret = -1;<br>
> + goto out;<br>
> + }<br>
> +<br>
> return 0;<br>
> } else<br>
> return 0;<br>
<br>
</div><div class="">> @@ -659,6 +857,126 @@ void fini_cgroup(void)<br>
> xfree(cg_yard);<br>
> }<br>
><br>
> +static int restore_cgroup_prop(const CgroupPropEntry * cg_prop_entry_p,<br>
> + const char *cname, const char *dir, const int *cg_p)<br>
> +{<br>
> + char path[PATH_MAX];<br>
> + FILE *f;<br>
> +<br>
> + if (!cg_prop_entry_p->value)<br>
<br>
</div>How can this happen? Would you add a code comment here?<br>
<div><div class="h5"><br>
> + return 0;<br>
> +<br>
> + if (snprintf(path, PATH_MAX, "%s/%s/%s", cname, dir, cg_prop_entry_p->name) >= PATH_MAX) {<br>
> + pr_err("snprintf output was truncated for %s\n", cg_prop_entry_p->name);<br>
> + return -1;<br>
> + }<br>
> +<br>
> + f = fopenat(*cg_p, path, "w+");<br>
> + if (!f) {<br>
> + pr_perror("Failed opening %s for writing\n", path);<br>
> + return -1;<br>
> + }<br>
> +<br>
> + if (fprintf(f, "%s", cg_prop_entry_p->value) < 0) {<br>
> + fclose(f);<br>
> + pr_err("Failed writing %s to %s\n", cg_prop_entry_p->value, path);<br>
> + return -1;<br>
> + }<br>
> +<br>
> + if (fclose(f) != 0) {<br>
> + pr_err("Failed closing %s\n", path);<br>
> + return -1;<br>
> + }<br>
> +<br>
> + pr_info("Restored cgroup property value %s to %s\n", cg_prop_entry_p->value, path);<br>
> + return 0;<br>
> +}<br>
> +<br>
> +static int prepare_cgroup_dir_properties(char *controller, CgroupDirEntry **ents,<br>
> + unsigned int n_ents)<br>
> +{<br>
> + unsigned int i, j;<br>
> + int cg;<br>
> +<br>
> + cg = get_service_fd(CGROUP_YARD);<br>
> +<br>
> + for (i = 0; i < n_ents; i++) {<br>
> + CgroupDirEntry *e = ents[i];<br>
> +<br>
> + /*<br>
> + * Check to see if we made e->properties NULL during restore<br>
> + * because directory already existed and as such we don't want to<br>
> + * change its properties<br>
> + */<br>
> + if (e->properties) {<br>
> + for (j = 0; j < e->n_properties; ++j) {<br>
> + if (restore_cgroup_prop(e->properties[j], controller, e->path, &cg) < 0)<br>
<br>
</div></div>The cg can be passed as value, instead of pointer.<br>
<div class=""><br>
> + return -1;<br>
> + }<br>
> + }<br>
> +<br>
> + if (prepare_cgroup_dir_properties(controller, e->children, e->n_children) < 0)<br>
> + return -1;<br>
> + }<br>
> +<br>
> + return 0;<br>
> +}<br>
<br>
</div><div class="">> --- a/protobuf/cgroup.proto<br>
> +++ b/protobuf/cgroup.proto<br>
> @@ -1,6 +1,12 @@<br>
> +message cgroup_prop_entry {<br>
> + required string name = 1;<br>
> + required string value = 2;<br>
> +}<br>
> +<br>
> message cgroup_dir_entry {<br>
> required string path = 1;<br>
> - repeated cgroup_dir_entry children = 4;<br>
> + repeated cgroup_dir_entry children = 2;<br>
<br>
</div>I'd appreciate if protobuf field renumbering goes in separate (quite small) patch.<br>
<div class="HOEnZb"><div class="h5"><br>
> + repeated cgroup_prop_entry properties = 3;<br>
> }<br>
><br>
> message cg_controller_entry {<br>
><br>
<br>
</div></div></blockquote></div><br></div>