[PATCH] vfs: use kstrdup()

On Sat, Jul 19, 2008 at 1:16 PM, Li Zefan <[email protected]> wrote:
> This piece of code can be replaced by a kstrdup().
>
> Signed-off-by: Li Zefan <[email protected]>

Looks good to me.

Reviewed-by: Pekka Enberg <[email protected]>

> ---
> fs/namespace.c | 9 +--------
> 1 files changed, 1 insertions(+), 8 deletions(-)
>
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 4f6f763..08c3b2d 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -127,14 +127,7 @@ struct vfsmount *alloc_vfsmnt(const char *name)
> INIT_LIST_HEAD(&mnt->mnt_slave_list);
> INIT_LIST_HEAD(&mnt->mnt_slave);
> atomic_set(&mnt->__mnt_writers, 0);
> - if (name) {
> - int size = strlen(name) + 1;
> - char *newname = kmalloc(size, GFP_KERNEL);
> - if (newname) {
> - memcpy(newname, name, size);
> - mnt->mnt_devname = newname;
> - }
> - }
> + mnt->mnt_devname = kstrdup(name, GFP_KERNEL);
> }
> return mnt;
> }
> --
> 1.5.4.rc3
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Li Zefan - Sat, Jul 19, 2008 at 06:16:20PM +0800]
| This piece of code can be replaced by a kstrdup().
|
| Signed-off-by: Li Zefan <[email protected]>
| ---
| fs/namespace.c | 9 +--------
| 1 files changed, 1 insertions(+), 8 deletions(-)
|
| diff --git a/fs/namespace.c b/fs/namespace.c
| index 4f6f763..08c3b2d 100644
| --- a/fs/namespace.c
| +++ b/fs/namespace.c
| @@ -127,14 +127,7 @@ struct vfsmount *alloc_vfsmnt(const char *name)
| INIT_LIST_HEAD(&mnt->mnt_slave_list);
| INIT_LIST_HEAD(&mnt->mnt_slave);
| atomic_set(&mnt->__mnt_writers, 0);
| - if (name) {
| - int size = strlen(name) + 1;
| - char *newname = kmalloc(size, GFP_KERNEL);
| - if (newname) {
| - memcpy(newname, name, size);
| - mnt->mnt_devname = newname;
| - }
| - }
| + mnt->mnt_devname = kstrdup(name, GFP_KERNEL);
| }
| return mnt;
| }
| --
| 1.5.4.rc3
|
|

but kstrdup may return NULL - is it safe there?
Sorry if that "not smart" question.

- Cyrill -
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Cyrill Gorcunov - Sat, Jul 19, 2008 at 05:13:17PM +0400]
[...]
| | - }
| | - }
| | + mnt->mnt_devname = kstrdup(name, GFP_KERNEL);
| | }
| | return mnt;
| | }
| | --
| | 1.5.4.rc3
| |
| |
|
| but kstrdup may return NULL - is it safe there?
| Sorry if that "not smart" question.
|
| - Cyrill -

ah, I see it is safe - sorry for noise

- Cyrill -
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Sat, Jul 19, 2008 at 05:19:09PM +0400, Cyrill Gorcunov wrote:
> [Cyrill Gorcunov - Sat, Jul 19, 2008 at 05:13:17PM +0400]
> [...]
> | | - }
> | | - }
> | | + mnt->mnt_devname = kstrdup(name, GFP_KERNEL);
> | | }
> | | return mnt;
> | | }
> | | --
> | | 1.5.4.rc3
> | |
> | |
> |
> | but kstrdup may return NULL - is it safe there?
> | Sorry if that "not smart" question.
> |
> | - Cyrill -
>
> ah, I see it is safe - sorry for noise

FWIW, it _is_ a good question.

* is all code treating ->mnt_devname as optional? AFAICS, there's
at least one place in NFS that doesn't. We could treat failing allocation
the same way we treat failing allocation of vfsmount itself - callers can
cope with that already.
* AFAICS, it should be const char *.
* ... or perhaps we shouldn't copy it at all. How about something
like
struct {
int count;
char name[];
}
with cloning sharing the reference and bumping the count, protecting it with
e.g. vfsmount_lock? For setups where we have a lot of bindings/namespaces
it might be noticable.

Any takers?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Al Viro wrote:
> On Sat, Jul 19, 2008 at 05:19:09PM +0400, Cyrill Gorcunov wrote:
>> [Cyrill Gorcunov - Sat, Jul 19, 2008 at 05:13:17PM +0400]
>> [...]
>> | | - }
>> | | - }
>> | | + mnt->mnt_devname = kstrdup(name, GFP_KERNEL);
>> | | }
>> | | return mnt;
>> | | }
>> | | --
>> | | 1.5.4.rc3
>> | |
>> | |
>> |
>> | but kstrdup may return NULL - is it safe there?
>> | Sorry if that "not smart" question.
>> |
>> | - Cyrill -
>>
>> ah, I see it is safe - sorry for noise
>
> FWIW, it _is_ a good question.
>
> * is all code treating ->mnt_devname as optional? AFAICS, there's
> at least one place in NFS that doesn't. We could treat failing allocation
> the same way we treat failing allocation of vfsmount itself - callers can
> cope with that already.

I just did a cleanup, and the original code didn't check for NULL.

I just looked into the git history, and I found out since fs/namespace.c was
created in v2.4.10.4, the code has never changed to check for failing
allocation of ->mnt_devname.

> * AFAICS, it should be const char *.

Agreed.

> * ... or perhaps we shouldn't copy it at all. How about something
> like
> struct {
> int count;
> char name[];
> }
> with cloning sharing the reference and bumping the count, protecting it with
> e.g. vfsmount_lock? For setups where we have a lot of bindings/namespaces
> it might be noticable.
>

I'm not sure whether this is a good idea, as I have limited knowledge about the
vfs internal.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/