On Tue, Dec 08, 2015 at 11:04:53AM -0500, Tejun Heo wrote:
> On Mon, Dec 07, 2015 at 05:06:18PM -0600, serge.hallyn@xxxxxxxxxx wrote:
> > static const char *proc_ns_follow_link(struct dentry *dentry, void **cookie)
> > diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> > index 2b3e2314..906f240 100644
> > --- a/include/linux/cgroup.h
> > +++ b/include/linux/cgroup.h
> > @@ -17,11 +17,36 @@
> > #include <linux/seq_file.h>
> > #include <linux/kernfs.h>
> > #include <linux/jump_label.h>
> > +#include <linux/nsproxy.h>
> > +#include <linux/types.h>
> > +#include <linux/ns_common.h>
> > +#include <linux/nsproxy.h>
> > +#include <linux/user_namespace.h>
> >
> > #include <linux/cgroup-defs.h>
> >
> > +struct cgroup_namespace {
> > + atomic_t count;
> > + struct ns_common ns;
> > + struct user_namespace *user_ns;
> > + struct css_set *root_cset;
> > +};
> > +
> > +extern struct cgroup_namespace init_cgroup_ns;
> > +
> > +static inline void get_cgroup_ns(struct cgroup_namespace *ns)
> > +{
> > + if (ns)
> > + atomic_inc(&ns->count);
> > +}
> > +
> ..
> > +void free_cgroup_ns(struct cgroup_namespace *ns);
> > +struct cgroup_namespace *copy_cgroup_ns(unsigned long flags,
> > + struct user_namespace *user_ns,
> > + struct cgroup_namespace *old_ns);
> ...
> > +char * __must_check cgroup_path_ns(struct cgroup *cgrp, char *buf,
> > + size_t buflen, struct cgroup_namespace *ns);
> > +char * __must_check cgroup_path(struct cgroup *cgrp, char *buf, size_t buflen);
> ...
> > +static inline void put_cgroup_ns(struct cgroup_namespace *ns)
> > +{
> > + if (ns && atomic_dec_and_test(&ns->count))
> > + free_cgroup_ns(ns);
> > +}
>
> I'd prefer collecting all ns related declarations in a single place.
I can group some of them, but free_cgroup_ns needs the
cgroup_namespace definition, put_cgroup_ns() needs free_cgroup_ns(),
and free_cgroup_ns() is static in the !CONFIG_CGROUPS case and a
non-static function in the other case.
So I'm going to put both get_cgroup_ns() and put_cgroup_ns() at the
end of the file, with the struct namespace define at the top. Is
that sufficient?
> > diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> > index 7f2f007..4fd07b5a 100644
> > --- a/kernel/cgroup.c
> > +++ b/kernel/cgroup.c
> > @@ -297,6 +300,13 @@ static bool cgroup_on_dfl(const struct cgroup *cgrp)
> > {
> > return cgrp->root == &cgrp_dfl_root;
> > }
>
> New line.
>
> > +struct cgroup_namespace init_cgroup_ns = {
> > + .count = { .counter = 1, },
> > + .user_ns = &init_user_ns,
> > + .ns.ops = &cgroupns_operations,
> > + .ns.inum = PROC_CGROUP_INIT_INO,
> > + .root_cset = &init_css_set,
> > +};
>
> Also, why inbetween two function defs? cgroup.c is messy but you can
> still put it after other variable definitions.
>
> > @@ -430,18 +440,18 @@ static inline bool cgroup_is_dead(const struct cgroup *cgrp)
> > return !(cgrp->self.flags & CSS_ONLINE);
> > }
> >
> > -static void cgroup_get(struct cgroup *cgrp)
> > +static inline void cgroup_get(struct cgroup *cgrp)
> > {
> > WARN_ON_ONCE(cgroup_is_dead(cgrp));
> > css_get(&cgrp->self);
> > }
> >
> > -static bool cgroup_tryget(struct cgroup *cgrp)
> > +static inline bool cgroup_tryget(struct cgroup *cgrp)
> > {
> > return css_tryget(&cgrp->self);
> > }
> >
> > -static void cgroup_put(struct cgroup *cgrp)
> > +static inline void cgroup_put(struct cgroup *cgrp)
> > {
> > css_put(&cgrp->self);
> > }
> >
> > @@ -780,7 +790,7 @@ static void put_css_set_locked(struct css_set *cset)
> > kfree_rcu(cset, rcu_head);
> > }
> >
> > -static void put_css_set(struct css_set *cset)
> > +static inline void put_css_set(struct css_set *cset)
>
> Stray diffs?
>
> > @@ -2189,6 +2199,25 @@ static struct file_system_type cgroup2_fs_type = {
> > .kill_sb = cgroup_kill_sb,
> > };
> >
> > +char * __must_check
> > +cgroup_path_ns(struct cgroup *cgrp, char *buf, size_t buflen,
> > + struct cgroup_namespace *ns)
> > +{
> > + struct cgroup *root;
> > +
> > + BUG_ON(!ns);
>
> Just let it crash on NULL deref.
>
> Thanks.
>
> --
> tejun
> _______________________________________________
> Containers mailing list
> Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
> https://lists.linuxfoundation.org/mailman/listinfo/containers
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/containers
