* [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 related [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 related [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 related [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 related [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).