linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] Trivial code clean for procfs
@ 2012-09-03 14:14 yan
  2012-09-03 14:14 ` [PATCH 1/3] proc: return -ENOMEM when inode allocation failed yan
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: yan @ 2012-09-03 14:14 UTC (permalink / raw)
  To: akpm; +Cc: linux-kernel


yan (3):
  proc: return -ENOMEM when inode allocation failed
  proc: no need to initialize proc_inode->fd in proc_get_inode
  proc: use kzalloc instead of kmalloc and memset

 fs/proc/generic.c |    5 ++---
 fs/proc/inode.c   |    1 -
 2 files changed, 2 insertions(+), 4 deletions(-)

-- 
1.7.9.5


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/3] proc: return -ENOMEM when inode allocation failed
  2012-09-03 14:14 [PATCH 0/3] Trivial code clean for procfs yan
@ 2012-09-03 14:14 ` yan
  2012-09-04  0:38   ` Ryan Mallon
  2012-09-04  3:02   ` Cong Wang
  2012-09-03 14:14 ` [PATCH 2/3] proc : no need to initialize proc_inode->fd in proc_get_inode yan
  2012-09-03 14:14 ` [PATCH 3/3] proc: use kzalloc instead of kmalloc and memset yan
  2 siblings, 2 replies; 13+ messages in thread
From: yan @ 2012-09-03 14:14 UTC (permalink / raw)
  To: akpm; +Cc: linux-kernel


Signed-off-by: yan <clouds.yan@gmail.com>
---
 fs/proc/generic.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/proc/generic.c b/fs/proc/generic.c
index b3647fe..9e8f631 100644
--- a/fs/proc/generic.c
+++ b/fs/proc/generic.c
@@ -427,7 +427,7 @@ struct dentry *proc_lookup_de(struct proc_dir_entry *de, struct inode *dir,
 		if (!memcmp(dentry->d_name.name, de->name, de->namelen)) {
 			pde_get(de);
 			spin_unlock(&proc_subdir_lock);
-			error = -EINVAL;
+			error = -ENOMEM;
 			inode = proc_get_inode(dir->i_sb, de);
 			goto out_unlock;
 		}
-- 
1.7.9.5


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 2/3] proc : no need to initialize proc_inode->fd in proc_get_inode
  2012-09-03 14:14 [PATCH 0/3] Trivial code clean for procfs yan
  2012-09-03 14:14 ` [PATCH 1/3] proc: return -ENOMEM when inode allocation failed yan
@ 2012-09-03 14:14 ` yan
  2012-09-04 22:49   ` Andrew Morton
  2012-09-03 14:14 ` [PATCH 3/3] proc: use kzalloc instead of kmalloc and memset yan
  2 siblings, 1 reply; 13+ messages in thread
From: yan @ 2012-09-03 14:14 UTC (permalink / raw)
  To: akpm; +Cc: linux-kernel

It has been initialized in proc_alloc_inode.

Signed-off-by: yan <clouds.yan@gmail.com>
---
 fs/proc/inode.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/proc/inode.c b/fs/proc/inode.c
index 7ac817b..3b22bbd 100644
--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -450,7 +450,6 @@ struct inode *proc_get_inode(struct super_block *sb, struct proc_dir_entry *de)
 		return NULL;
 	if (inode->i_state & I_NEW) {
 		inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
-		PROC_I(inode)->fd = 0;
 		PROC_I(inode)->pde = de;
 
 		if (de->mode) {
-- 
1.7.9.5


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 3/3] proc: use kzalloc instead of kmalloc and memset
  2012-09-03 14:14 [PATCH 0/3] Trivial code clean for procfs yan
  2012-09-03 14:14 ` [PATCH 1/3] proc: return -ENOMEM when inode allocation failed yan
  2012-09-03 14:14 ` [PATCH 2/3] proc : no need to initialize proc_inode->fd in proc_get_inode yan
@ 2012-09-03 14:14 ` yan
  2012-09-04  0:44   ` Ryan Mallon
  2 siblings, 1 reply; 13+ messages in thread
From: yan @ 2012-09-03 14:14 UTC (permalink / raw)
  To: akpm; +Cc: linux-kernel


Signed-off-by: yan <clouds.yan@gmail.com>
---
 fs/proc/generic.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/proc/generic.c b/fs/proc/generic.c
index 9e8f631..38de015 100644
--- a/fs/proc/generic.c
+++ b/fs/proc/generic.c
@@ -616,10 +616,9 @@ static struct proc_dir_entry *__proc_create(struct proc_dir_entry **parent,
 
 	len = strlen(fn);
 
-	ent = kmalloc(sizeof(struct proc_dir_entry) + len + 1, GFP_KERNEL);
+	ent = kzalloc(sizeof(struct proc_dir_entry) + len + 1, GFP_KERNEL);
 	if (!ent) goto out;
 
-	memset(ent, 0, sizeof(struct proc_dir_entry));
 	memcpy(ent->name, fn, len + 1);
 	ent->namelen = len;
 	ent->mode = mode;
-- 
1.7.9.5


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] proc: return -ENOMEM when inode allocation failed
  2012-09-03 14:14 ` [PATCH 1/3] proc: return -ENOMEM when inode allocation failed yan
@ 2012-09-04  0:38   ` Ryan Mallon
  2012-09-04  3:02   ` Cong Wang
  1 sibling, 0 replies; 13+ messages in thread
From: Ryan Mallon @ 2012-09-04  0:38 UTC (permalink / raw)
  To: yan; +Cc: akpm, linux-kernel

On 04/09/12 00:14, yan wrote:
> Signed-off-by: yan <clouds.yan@gmail.com>
> ---
>  fs/proc/generic.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/proc/generic.c b/fs/proc/generic.c
> index b3647fe..9e8f631 100644
> --- a/fs/proc/generic.c
> +++ b/fs/proc/generic.c
> @@ -427,7 +427,7 @@ struct dentry *proc_lookup_de(struct proc_dir_entry *de, struct inode *dir,
>  		if (!memcmp(dentry->d_name.name, de->name, de->namelen)) {
>  			pde_get(de);
>  			spin_unlock(&proc_subdir_lock);
> -			error = -EINVAL;
> +			error = -ENOMEM;

This seems incorrect. This function doesn't allocate anything, it looks
up an inode. From my reading it looks like -EINVAL is returned here if
the dentry name matches, but the inode cannot be found.

~Ryan


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] proc: use kzalloc instead of kmalloc and memset
  2012-09-03 14:14 ` [PATCH 3/3] proc: use kzalloc instead of kmalloc and memset yan
@ 2012-09-04  0:44   ` Ryan Mallon
  2012-09-04  9:10     ` yan yan
  0 siblings, 1 reply; 13+ messages in thread
From: Ryan Mallon @ 2012-09-04  0:44 UTC (permalink / raw)
  To: yan; +Cc: akpm, linux-kernel

On 04/09/12 00:14, yan wrote:
> Signed-off-by: yan <clouds.yan@gmail.com>
> ---
>  fs/proc/generic.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/proc/generic.c b/fs/proc/generic.c
> index 9e8f631..38de015 100644
> --- a/fs/proc/generic.c
> +++ b/fs/proc/generic.c
> @@ -616,10 +616,9 @@ static struct proc_dir_entry *__proc_create(struct proc_dir_entry **parent,
>  
>  	len = strlen(fn);
>  
> -	ent = kmalloc(sizeof(struct proc_dir_entry) + len + 1, GFP_KERNEL);
> +	ent = kzalloc(sizeof(struct proc_dir_entry) + len + 1, GFP_KERNEL);
>  	if (!ent) goto out;
>  
> -	memset(ent, 0, sizeof(struct proc_dir_entry));
>  	memcpy(ent->name, fn, len + 1);
>  	ent->namelen = len;
>  	ent->mode = mode;

Note that this change results in slightly different behaviour. Before
your change only sizeof(struct proc_dir_entry) is zero'ed by memset, and
then the name is filled in (the len + 1 part). After your change the
structure and the name field are both zeroed, so the name field is being
written to twice. The cost is probably negligible though.

~Ryan



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] proc: return -ENOMEM when inode allocation failed
  2012-09-03 14:14 ` [PATCH 1/3] proc: return -ENOMEM when inode allocation failed yan
  2012-09-04  0:38   ` Ryan Mallon
@ 2012-09-04  3:02   ` Cong Wang
  2012-09-04  9:22     ` yan yan
  1 sibling, 1 reply; 13+ messages in thread
From: Cong Wang @ 2012-09-04  3:02 UTC (permalink / raw)
  To: yan; +Cc: akpm, linux-kernel

On 09/03/2012 10:14 PM, yan wrote:
> Signed-off-by: yan <clouds.yan@gmail.com>

Please provide a changelog to explain why we need this patch.


> ---
>   fs/proc/generic.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/proc/generic.c b/fs/proc/generic.c
> index b3647fe..9e8f631 100644
> --- a/fs/proc/generic.c
> +++ b/fs/proc/generic.c
> @@ -427,7 +427,7 @@ struct dentry *proc_lookup_de(struct proc_dir_entry *de, struct inode *dir,
>   		if (!memcmp(dentry->d_name.name, de->name, de->namelen)) {
>   			pde_get(de);
>   			spin_unlock(&proc_subdir_lock);
> -			error = -EINVAL;
> +			error = -ENOMEM;

Why the !memcmp() case is related with ENOMEM ??


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] proc: use kzalloc instead of kmalloc and memset
  2012-09-04  0:44   ` Ryan Mallon
@ 2012-09-04  9:10     ` yan yan
  0 siblings, 0 replies; 13+ messages in thread
From: yan yan @ 2012-09-04  9:10 UTC (permalink / raw)
  To: Ryan Mallon; +Cc: akpm, linux-kernel

2012/9/4 Ryan Mallon <rmallon@gmail.com>
>
> On 04/09/12 00:14, yan wrote:
> > Signed-off-by: yan <clouds.yan@gmail.com>
> > ---
> >  fs/proc/generic.c |    3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/fs/proc/generic.c b/fs/proc/generic.c
> > index 9e8f631..38de015 100644
> > --- a/fs/proc/generic.c
> > +++ b/fs/proc/generic.c
> > @@ -616,10 +616,9 @@ static struct proc_dir_entry *__proc_create(struct
> > proc_dir_entry **parent,
> >
> >       len = strlen(fn);
> >
> > -     ent = kmalloc(sizeof(struct proc_dir_entry) + len + 1,
> > GFP_KERNEL);
> > +     ent = kzalloc(sizeof(struct proc_dir_entry) + len + 1,
> > GFP_KERNEL);
> >       if (!ent) goto out;
> >
> > -     memset(ent, 0, sizeof(struct proc_dir_entry));
> >       memcpy(ent->name, fn, len + 1);
> >       ent->namelen = len;
> >       ent->mode = mode;
>
> Note that this change results in slightly different behaviour. Before
> your change only sizeof(struct proc_dir_entry) is zero'ed by memset, and
> then the name is filled in (the len + 1 part). After your change the
> structure and the name field are both zeroed, so the name field is being
> written to twice. The cost is probably negligible though.

Oh, I didn't notice that actually. Thank you.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] proc: return -ENOMEM when inode allocation failed
  2012-09-04  3:02   ` Cong Wang
@ 2012-09-04  9:22     ` yan yan
  2012-09-04 22:41       ` Andrew Morton
  2012-09-05  7:15       ` Cong Wang
  0 siblings, 2 replies; 13+ messages in thread
From: yan yan @ 2012-09-04  9:22 UTC (permalink / raw)
  To: Cong Wang; +Cc: akpm, linux-kernel

2012/9/4 Cong Wang <xiyou.wangcong@gmail.com>:
> On 09/03/2012 10:14 PM, yan wrote:
>>
>> Signed-off-by: yan <clouds.yan@gmail.com>
>
>
> Please provide a changelog to explain why we need this patch.

I think the title is self explained.


>> ---
>>   fs/proc/generic.c |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/proc/generic.c b/fs/proc/generic.c
>> index b3647fe..9e8f631 100644
>> --- a/fs/proc/generic.c
>> +++ b/fs/proc/generic.c
>> @@ -427,7 +427,7 @@ struct dentry *proc_lookup_de(struct proc_dir_entry
>> *de, struct inode *dir,
>>                 if (!memcmp(dentry->d_name.name, de->name, de->namelen)) {
>>                         pde_get(de);
>>                         spin_unlock(&proc_subdir_lock);
>> -                       error = -EINVAL;
>> +                       error = -ENOMEM;
>
>
> Why the !memcmp() case is related with ENOMEM ??

We are presetting 'error' here. The following proc_get_inode() will try
to get an inode, either from inode cache or allocate a new one (and fill it).

If we get a NULL inode, that means allocation failed. That's how
ENOMEM involved.

Thank you for your reply.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] proc: return -ENOMEM when inode allocation failed
  2012-09-04  9:22     ` yan yan
@ 2012-09-04 22:41       ` Andrew Morton
  2012-09-05  7:15       ` Cong Wang
  1 sibling, 0 replies; 13+ messages in thread
From: Andrew Morton @ 2012-09-04 22:41 UTC (permalink / raw)
  To: yan yan; +Cc: Cong Wang, linux-kernel

On Tue, 4 Sep 2012 17:22:15 +0800
yan yan <clouds.yan@gmail.com> wrote:

> 2012/9/4 Cong Wang <xiyou.wangcong@gmail.com>:
> > On 09/03/2012 10:14 PM, yan wrote:
> >>
> >> Signed-off-by: yan <clouds.yan@gmail.com>
> >
> >
> > Please provide a changelog to explain why we need this patch.
> 
> I think the title is self explained.

Given this review feedback, the title alone was obviously insufficient!

I provided the following changelog text:

: If proc_get_inode() returns NULL then presumably it encountered memory
: exhaustion.  proc_lookup_de() should return -ENOMEM in this case, not
: -EINVAL.



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] proc : no need to initialize proc_inode->fd in proc_get_inode
  2012-09-03 14:14 ` [PATCH 2/3] proc : no need to initialize proc_inode->fd in proc_get_inode yan
@ 2012-09-04 22:49   ` Andrew Morton
  0 siblings, 0 replies; 13+ messages in thread
From: Andrew Morton @ 2012-09-04 22:49 UTC (permalink / raw)
  To: yan; +Cc: linux-kernel

On Mon,  3 Sep 2012 22:14:06 +0800
yan <clouds.yan@gmail.com> wrote:

> It has been initialized in proc_alloc_inode.
>
> ...

I think what you mean here is that the preceding call to iget_locked()
will call alloc_inode() which will call proc_alloc_inode() which clears
pro_inode.fd.  

And if iget_locked() instead found the inode via find_inode_fast(),
that inode will not have I_NEW set so this change has no effect.

Yes?  Please do spell things out in this level of detail so that others
can check your logic.

> --- a/fs/proc/inode.c
> +++ b/fs/proc/inode.c
> @@ -450,7 +450,6 @@ struct inode *proc_get_inode(struct super_block *sb, struct proc_dir_entry *de)
>  		return NULL;
>  	if (inode->i_state & I_NEW) {
>  		inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
> -		PROC_I(inode)->fd = 0;
>  		PROC_I(inode)->pde = de;
>  
>  		if (de->mode) {

I provided this changelog:

: proc_get_inode() obtains the inode via a call to iget_locked(). 
: iget_locked() calls alloc_inode() which will call proc_alloc_inode() which
: clears proc_inode.fd, so there is no need to clear this field in
: proc_get_inode().
:
: If iget_locked() instead found the inode via find_inode_fast(), that inode
: will not have I_NEW set so this change has no effect.


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] proc: return -ENOMEM when inode allocation failed
  2012-09-04  9:22     ` yan yan
  2012-09-04 22:41       ` Andrew Morton
@ 2012-09-05  7:15       ` Cong Wang
  2012-09-05  7:57         ` yan yan
  1 sibling, 1 reply; 13+ messages in thread
From: Cong Wang @ 2012-09-05  7:15 UTC (permalink / raw)
  To: yan yan; +Cc: akpm, linux-kernel

On 09/04/2012 05:22 PM, yan yan wrote:
> 2012/9/4 Cong Wang <xiyou.wangcong@gmail.com>:
>> On 09/03/2012 10:14 PM, yan wrote:
>>>
>>> Signed-off-by: yan <clouds.yan@gmail.com>
>>
>>
>> Please provide a changelog to explain why we need this patch.
>
> I think the title is self explained.
>
>
>>> ---
>>>    fs/proc/generic.c |    2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/proc/generic.c b/fs/proc/generic.c
>>> index b3647fe..9e8f631 100644
>>> --- a/fs/proc/generic.c
>>> +++ b/fs/proc/generic.c
>>> @@ -427,7 +427,7 @@ struct dentry *proc_lookup_de(struct proc_dir_entry
>>> *de, struct inode *dir,
>>>                  if (!memcmp(dentry->d_name.name, de->name, de->namelen)) {
>>>                          pde_get(de);
>>>                          spin_unlock(&proc_subdir_lock);
>>> -                       error = -EINVAL;
>>> +                       error = -ENOMEM;
>>
>>
>> Why the !memcmp() case is related with ENOMEM ??
>
> We are presetting 'error' here. The following proc_get_inode() will try
> to get an inode, either from inode cache or allocate a new one (and fill it).
>
> If we get a NULL inode, that means allocation failed. That's how
> ENOMEM involved.

Then the following patch is probably better than yours:


diff --git a/fs/proc/generic.c b/fs/proc/generic.c
index b3647fe..6b22913 100644
--- a/fs/proc/generic.c
+++ b/fs/proc/generic.c
@@ -427,12 +427,16 @@ struct dentry *proc_lookup_de(struct 
proc_dir_entry *de, struct inode *dir,
                 if (!memcmp(dentry->d_name.name, de->name, de->namelen)) {
                         pde_get(de);
                         spin_unlock(&proc_subdir_lock);
-                       error = -EINVAL;
                         inode = proc_get_inode(dir->i_sb, de);
+                       if (!inode) {
+                               error = -ENOMEM;
+                               goto out_put;
+                       }
                         goto out_unlock;
                 }
         }
         spin_unlock(&proc_subdir_lock);
+
  out_unlock:

         if (inode) {
@@ -440,6 +444,8 @@ out_unlock:
                 d_add(dentry, inode);
                 return NULL;
         }
+out_put:
+
         if (de)
                 pde_put(de);
         return ERR_PTR(error);



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] proc: return -ENOMEM when inode allocation failed
  2012-09-05  7:15       ` Cong Wang
@ 2012-09-05  7:57         ` yan yan
  0 siblings, 0 replies; 13+ messages in thread
From: yan yan @ 2012-09-05  7:57 UTC (permalink / raw)
  To: Cong Wang; +Cc: akpm, linux-kernel

2012/9/5 Cong Wang <xiyou.wangcong@gmail.com>:
>>> Why the !memcmp() case is related with ENOMEM ??
>>
>>
>> We are presetting 'error' here. The following proc_get_inode() will try
>> to get an inode, either from inode cache or allocate a new one (and fill
>> it).
>>
>> If we get a NULL inode, that means allocation failed. That's how
>> ENOMEM involved.
>
>
> Then the following patch is probably better than yours:
>
>
> diff --git a/fs/proc/generic.c b/fs/proc/generic.c
> index b3647fe..6b22913 100644
> --- a/fs/proc/generic.c
> +++ b/fs/proc/generic.c
> @@ -427,12 +427,16 @@ struct dentry *proc_lookup_de(struct proc_dir_entry
> *de, struct inode *dir,
>
>                 if (!memcmp(dentry->d_name.name, de->name, de->namelen)) {
>                         pde_get(de);
>                         spin_unlock(&proc_subdir_lock);
> -                       error = -EINVAL;
>                         inode = proc_get_inode(dir->i_sb, de);
> +                       if (!inode) {
> +                               error = -ENOMEM;
> +                               goto out_put;
> +                       }
>                         goto out_unlock;
>                 }
>         }
>         spin_unlock(&proc_subdir_lock);
> +
>  out_unlock:
>
>         if (inode) {
> @@ -440,6 +444,8 @@ out_unlock:
>                 d_add(dentry, inode);
>                 return NULL;
>         }
> +out_put:
> +
>         if (de)
>                 pde_put(de);
>         return ERR_PTR(error);
>
>

Change so many lines to save a assignment to 'error' ...

That's a stye issue. I prefer a simple change, though your
change seems OK to me.

Thanks

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2012-09-05  7:57 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-09-03 14:14 [PATCH 0/3] Trivial code clean for procfs yan
2012-09-03 14:14 ` [PATCH 1/3] proc: return -ENOMEM when inode allocation failed yan
2012-09-04  0:38   ` Ryan Mallon
2012-09-04  3:02   ` Cong Wang
2012-09-04  9:22     ` yan yan
2012-09-04 22:41       ` Andrew Morton
2012-09-05  7:15       ` Cong Wang
2012-09-05  7:57         ` yan yan
2012-09-03 14:14 ` [PATCH 2/3] proc : no need to initialize proc_inode->fd in proc_get_inode yan
2012-09-04 22:49   ` Andrew Morton
2012-09-03 14:14 ` [PATCH 3/3] proc: use kzalloc instead of kmalloc and memset yan
2012-09-04  0:44   ` Ryan Mallon
2012-09-04  9:10     ` yan yan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).