Re: [PATCH 1/2] cgroup map files: Add cgroup map data type

[Paul Menage]

On Sat, Feb 23, 2008 at 12:04 AM, Andrew Morton
<akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
>  > +static int cgroup_map_add(struct cgroup_map_cb *cb, const char *key, u64 value)
>  > +{
>  > +     struct seq_file *sf = cb->state;
>  > +     return seq_printf(sf, "%s %llu\n", key, value);
>  > +}
>
>  We don't know what type the architecture uses to implement u64.  This will
>  warn on powerpc, sparc64, maybe others.

OK, I'll add an (unsigned long long) cast.

>
>
>  > +static int cgroup_seqfile_show(struct seq_file *m, void *arg)
>  > +{
>  > +     struct cgroup_seqfile_state *state = m->private;
>  > +     struct cftype *cft = state->cft;
>  > +     if (cft->read_map) {
>  > +             struct cgroup_map_cb cb = {
>  > +                     .fill = cgroup_map_add,
>  > +                     .state = m,
>  > +             };
>  > +             return cft->read_map(state->cgroup, cft, &cb);
>  > +     } else {
>  > +             BUG();
>
>  That's not really needed.  Just call cft->read_map unconditionally.  if
>  it's zero we'll get a null-pointer deref which will have just the same
>  effect as a BUG.

OK. The long-term plan is to have other kinds of files also handled by
this function, so eventually it would look something like:

if (cft->read_map) {
...
} else if (cft->read_something_else) {
...
}
...
} else {
  BUG();
}

But I guess I can save that for the future.

>  >  static int cgroup_file_open(struct inode *inode, struct file *file)
>  >  {
>  >       int err;
>  > @@ -1499,7 +1539,18 @@ static int cgroup_file_open(struct inode
>  >       cft = __d_cft(file->f_dentry);
>  >       if (!cft)
>  >               return -ENODEV;
>  > -     if (cft->open)
>  > +     if (cft->read_map) {
>
>  But above a NULL value is illegal.  Why are we testing it here?
>
>

The existence of cft->read_map causes us to open a seq_file. Otherwise
we do nothing special and carry on down the normal open path.

Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/