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

[Alexander Nikiforov <a.nikiforov-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>]

Hi, Tejun

On 05/04/2012 08:54 PM, Tejun Heo wrote:
> 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?
Of course you are right, miss that.
>
> Thanks.
>

What about other in v4, any suggestion?

-- 
Best regards,
      Alex Nikiforov,
      Mobile SW, Advanced Software Group,
      Moscow R&D center, Samsung Electronics