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

[Garrison Bellack]

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.


On Tue, Aug 5, 2014 at 10:58 PM, Pavel Emelyanov <xemul at parallels.com>
wrote:

> 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 {
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvz.org/pipermail/criu/attachments/20140807/9c5e8356/attachment-0001.html>