<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Aug 13, 2014 at 1:55 PM, Tycho Andersen <span dir="ltr"><<a href="mailto:tycho.andersen@canonical.com" target="_blank">tycho.andersen@canonical.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="">On Wed, Aug 13, 2014 at 01:32:01PM -0700, Garrison Bellack wrote:<br>
> On Wed, Aug 13, 2014 at 12:43 PM, Andrew Vagin <<a href="mailto:avagin@parallels.com">avagin@parallels.com</a>> wrote:<br>
><br>
> > On Wed, Aug 13, 2014 at 11:59:33AM -0700, <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>
> > > Because different kernel versions have different cgroup properties, criu<br>
> > > shouldn't crash just because the properties statically listed aren't<br>
> > exact.<br>
> > > Instead, during dump, ignore properties the kernel doesn't have and<br>
> > continue.<br>
> > > In addition, during restore, in the event of migration to a kernel that<br>
> > is<br>
> > > missing a property that was dumped, print an error but continue.<br>
> ><br>
> > I am not sure that we can continue in this case. Why do you think that<br>
> > it's safe? I think we need to ask a user about this. So can we add a new<br>
> > option to allow continuing in this case?<br>
><br>
><br>
> I'm a little confused about your concerns of safety. Are you referring to<br>
> missing properties on the dump or restore side?<br>
<br>
</div>I think restore side. I also wondered the same thing -- it seems that<br>
missing properties on restore should be an error (perhaps as Andrew<br>
says with an --allow-missing-cg-props flag on the restore side to<br>
allow you to override this error).</blockquote><div><br></div><div>Here is the reasoning behind this decision. Let say you are migrating properties A,B,C,D where B doesn't exist.</div><div><br></div><div>This is how it currently works:</div>
<div>A will be restored. B prints an error and causes failure because it is missing. However, because cgroup property restoration is the last thing to happen during restore, and because of designs further up the call chain, the restoration of the process still goes through. C,D are not restored because of previous criu failure.</div>
<div>End result -- Process and property A restored, C and D are not restored even though they exist on the new machine</div><div><br></div><div>With this patch:</div><div>A will be restored. B prints an error and continues. C and D are restored.</div>
<div>End result -- Process and property A, C, D all restored.</div><div><br></div><div>I think between these two options we clearly prefer the later behavior.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span class=""><font color="#888888"><br></font></span><div class=""><div class="h5">
><br>
> > ><br>
> > > Change-Id: I5a8b93d6a8a3a9664914f10cf8e2110340dd8b31<br>
> > > Signed-off-by: Garrison Bellack <<a href="mailto:gbellack@google.com">gbellack@google.com</a>><br>
> > > ---<br>
> > > cgroup.c | 40 ++++++++++++++++++++++++++--------------<br>
> > > 1 file changed, 26 insertions(+), 14 deletions(-)<br>
> > ><br>
> > > diff --git a/cgroup.c b/cgroup.c<br>
> > > index f8dbbde..b727a50 100644<br>
> > > --- a/cgroup.c<br>
> > > +++ b/cgroup.c<br>
> > > @@ -284,35 +284,30 @@ static int find_dir(const char *path, struct<br>
> > list_head *dirs, struct cgroup_dir<br>
> > > * Currently this function only supports properties that have 1 value,<br>
> > under 100<br>
> > > * chars<br>
> > > */<br>
> > > -static int read_cgroup_prop(struct cgroup_prop *property, const char<br>
> > *fpath)<br>
> > > +static int read_cgroup_prop(struct cgroup_prop *property, const char<br>
> > *fullpath)<br>
> > > {<br>
> > > - char pbuf[PATH_MAX], buf[100];<br>
> > > + char buf[100];<br>
> > > FILE *f;<br>
> > > char *endptr;<br>
> > ><br>
> > > - if (snprintf(pbuf, PATH_MAX, "%s/%s", fpath, property->name) >=<br>
> > PATH_MAX) {<br>
> > > - pr_err("snprintf output was truncated");<br>
> > > - return -1;<br>
> > > - }<br>
> > > -<br>
> > > - f = fopen(pbuf, "r");<br>
> > > + f = fopen(fullpath, "r");<br>
> > > if (!f) {<br>
> > > - pr_err("Failed opening %s\n", pbuf);<br>
> > > property->value = NULL;<br>
> > > + pr_perror("Failed opening %s\n", fullpath);<br>
> > > return -1;<br>
> > > }<br>
> > ><br>
> > > memset(buf, 0, sizeof(buf));<br>
> > > if (fread(buf, sizeof(buf), 1, f) != 1) {<br>
> > > if (!feof(f)) {<br>
> > > - pr_err("Failed scanning %s\n", pbuf);<br>
> > > + pr_err("Failed scanning %s\n", fullpath);<br>
> > > fclose(f);<br>
> > > return -1;<br>
> > > }<br>
> > > }<br>
> > ><br>
> > > if (fclose(f) != 0) {<br>
> > > - pr_err("Failed closing %s\n", pbuf);<br>
> > > + pr_err("Failed closing %s\n", fullpath);<br>
> > > return -1;<br>
> > > }<br>
> > ><br>
> > > @@ -390,6 +385,7 @@ static int add_cgroup_properties(const char *fpath,<br>
> > struct cgroup_dir *ncd,<br>
> > > struct cg_controller *controller)<br>
> > > {<br>
> > > int i, j;<br>
> > > + char buf[PATH_MAX];<br>
> > > struct cgroup_prop *prop;<br>
> > ><br>
> > > for (i = 0; i < controller->n_controllers; ++i) {<br>
> > > @@ -397,16 +393,28 @@ static int add_cgroup_properties(const char<br>
> > *fpath, struct cgroup_dir *ncd,<br>
> > > const char **prop_arr =<br>
> > get_known_properties(controller->controllers[i]);<br>
> > ><br>
> > > for (j = 0; prop_arr != NULL && prop_arr[j] != NULL; ++j) {<br>
> > > + if (snprintf(buf, PATH_MAX, "%s/%s", fpath,<br>
> > prop_arr[j]) >= PATH_MAX) {<br>
> > > + pr_err("snprintf output was truncated");<br>
> > > + return -1;<br>
> > > + }<br>
> > > +<br>
> > > + if (access(buf, F_OK) < 0 && errno == ENOENT) {<br>
> > > + pr_info("Couldn't open %s. This cgroup<br>
> > property may not exist on this kernel\n", buf);<br>
> > > + continue;<br>
> > > + }<br>
> > > +<br>
> > > prop = create_cgroup_prop(prop_arr[j]);<br>
> > > if (!prop) {<br>
> > > free_all_cgroup_props(ncd);<br>
> > > return -1;<br>
> > > }<br>
> > > - if (read_cgroup_prop(prop, fpath) < 0) {<br>
> > > +<br>
> > > + if (read_cgroup_prop(prop, buf) < 0) {<br>
> > > free_cgroup_prop(prop);<br>
> > > free_all_cgroup_props(ncd);<br>
> > > return -1;<br>
> > > }<br>
> > > +<br>
> > > pr_info("Dumping value %s from %s/%s\n",<br>
> > prop->value, fpath, prop->name);<br>
> > > list_add_tail(&prop->list, &ncd->properties);<br>
> > > ncd->n_properties++;<br>
> > > @@ -924,8 +932,12 @@ static int restore_cgroup_prop(const<br>
> > CgroupPropEntry * cg_prop_entry_p,<br>
> > ><br>
> > > f = fopenat(cg, path, "w+");<br>
> > > if (!f) {<br>
> > > - pr_perror("Failed opening %s for writing\n", path);<br>
> > > - return -1;<br>
> > > + if (errno != ENOENT) {<br>
> > > + pr_perror("Failed opening %s\n", path);<br>
> > > + return -1;<br>
> > > + }<br>
> > > + pr_perror("Couldn't open %s. This cgroup property may not<br>
> > exist on this kernel\n", path);<br>
> > > + return 0;<br>
> > > }<br>
> > ><br>
> > > if (fprintf(f, "%s", cg_prop_entry_p->value) < 0) {<br>
> > > --<br>
> > > 2.1.0.rc2.206.gedb03e5<br>
> > ><br>
> > > _______________________________________________<br>
> > > CRIU mailing list<br>
> > > <a href="mailto:CRIU@openvz.org">CRIU@openvz.org</a><br>
> > > <a href="https://lists.openvz.org/mailman/listinfo/criu" target="_blank">https://lists.openvz.org/mailman/listinfo/criu</a><br>
> ><br>
<br>
> _______________________________________________<br>
> CRIU mailing list<br>
> <a href="mailto:CRIU@openvz.org">CRIU@openvz.org</a><br>
> <a href="https://lists.openvz.org/mailman/listinfo/criu" target="_blank">https://lists.openvz.org/mailman/listinfo/criu</a><br>
<br>
</div></div></blockquote></div><br></div></div>