Re: [PATCH -V3 1/1] cgroup: Add inotify event on change tasks file (fork, exit, move pid from file)

[Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

Hello,

On Fri, May 04, 2012 at 04:55:44PM +0400, Alexander Nikiforov wrote:
> >>-static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
> >>+static struct cfent* cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
> >>  			   const struct cftype *cft)
> >>  {
> >>  	struct dentry *dir = cgrp->dentry;
> >>@@ -2696,12 +2717,13 @@ static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
> >>
> >>  	cfe = kzalloc(sizeof(*cfe), GFP_KERNEL);
> >>  	if (!cfe)
> >>-		return -ENOMEM;
> >>+		return NULL;
> >>
> >>  	dentry = lookup_one_len(name, dir, strlen(name));
> >>  	if (IS_ERR(dentry)) {
> >>  		error = PTR_ERR(dentry);
> >>-		goto out;
> >>+		kfree(cfe);
> >>+		return NULL;
> >>  	}
> >>
> >>  	mode = cgroup_file_mode(cft);
> >>@@ -2711,29 +2733,34 @@ static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
> >>  		cfe->dentry = dentry;
> >>  		dentry->d_fsdata = cfe;
> >>  		list_add_tail(&cfe->node,&parent->files);
> >>-		cfe = NULL;
> >>  	}
> >>  	dput(dentry);
> >>-out:
> >>-	kfree(cfe);
> >>-	return error;
> >>+
> >>+	return cfe;
> >>  }
> >Are you sure you're not creating a memory leak here?
> 
> code make the same things, just without goto. move kfree inside if
> (IS_ERR(dentry), but before dput(dentry)  cfe set to NULL, so
> kfree(cfe) will do nothing. I think it's without any mem leaks.

I'm confused.  Who's freeing cfe after cgroup_create_file() failure?
Also, why are you changing this at all?

Thanks.

-- 
tejun