Fix the following coccinelle warning:
fs/ufs/super.c:1418:7-28: WARNING: casting value returned by memory
allocation function to (struct ufs_inode_info *) is useless.
Cc: Evgeniy Dushistov <[email protected]>
Cc: Andrew Morton <[email protected]>
Signed-off-by: Fabian Frederick <[email protected]>
---
fs/ufs/super.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/ufs/super.c b/fs/ufs/super.c
index da73801..a15798f 100644
--- a/fs/ufs/super.c
+++ b/fs/ufs/super.c
@@ -1414,8 +1414,8 @@ static struct kmem_cache * ufs_inode_cachep;
static struct inode *ufs_alloc_inode(struct super_block *sb)
{
- struct ufs_inode_info *ei;
- ei = (struct ufs_inode_info *)kmem_cache_alloc(ufs_inode_cachep, GFP_NOFS);
+ struct ufs_inode_info *ei = kmem_cache_alloc(ufs_inode_cachep,
+ GFP_NOFS);
if (!ei)
return NULL;
ei->vfs_inode.i_version = 1;
--
2.1.0
On Sun, Dec 28, 2014 at 04:28:29PM +0100, Fabian Frederick wrote:
> Fix the following coccinelle warning:
> fs/ufs/super.c:1418:7-28: WARNING: casting value returned by memory
> allocation function to (struct ufs_inode_info *) is useless.
... except that it makes allocations harder to grep for. IOW, it's not
obviously an improvement and should be up to maintainer of the code in
question.
On Sun, 2014-12-28 at 15:33 +0000, Al Viro wrote:
> On Sun, Dec 28, 2014 at 04:28:29PM +0100, Fabian Frederick wrote:
> > Fix the following coccinelle warning:
> > fs/ufs/super.c:1418:7-28: WARNING: casting value returned by memory
> > allocation function to (struct ufs_inode_info *) is useless.
>
> ... except that it makes allocations harder to grep for.
How does it do that?
> IOW, it's not
> obviously an improvement and should be up to maintainer of the code in
> question.
That's always true in any case.
On Sun, 2014-12-28 at 16:28 +0100, Fabian Frederick wrote:
> Fix the following coccinelle warning:
> fs/ufs/super.c:1418:7-28: WARNING: casting value returned by memory
> allocation function to (struct ufs_inode_info *) is useless.
[]
> diff --git a/fs/ufs/super.c b/fs/ufs/super.c
[]
> @@ -1414,8 +1414,8 @@ static struct kmem_cache * ufs_inode_cachep;
>
> static struct inode *ufs_alloc_inode(struct super_block *sb)
> {
> - struct ufs_inode_info *ei;
> - ei = (struct ufs_inode_info *)kmem_cache_alloc(ufs_inode_cachep, GFP_NOFS);
> + struct ufs_inode_info *ei = kmem_cache_alloc(ufs_inode_cachep,
> + GFP_NOFS);
If you insist on the blank line after declaration style,
perhaps it's better to separate the declaration from
the alloc so the alloc is on a single line like:
static struct inode *ufs_alloc_inode(struct super_block *sb)
{
struct ufs_inode_info *ei;
ei = kmem_cache_alloc(ufs_inode_cachep, GFP_NOFS);
if (!ei)
return NULL;
ei->vfs_inode.i_version = 1;
etc...
On Sun, Dec 28, 2014 at 08:45:44AM -0800, Joe Perches wrote:
> On Sun, 2014-12-28 at 15:33 +0000, Al Viro wrote:
> > On Sun, Dec 28, 2014 at 04:28:29PM +0100, Fabian Frederick wrote:
> > > Fix the following coccinelle warning:
> > > fs/ufs/super.c:1418:7-28: WARNING: casting value returned by memory
> > > allocation function to (struct ufs_inode_info *) is useless.
> >
> > ... except that it makes allocations harder to grep for.
>
> How does it do that?
search for \<struct[ ]*$NAME[ ]*($|[^ *]|[*][ ]*[)])
generally gives a lot of interesting information (variables of that type,
members of anything having that type, sizeof, container_of, explicit casts)
with relatively little noise.
BTW, that's why I really don't like the stuff like
struct foo *p = kmalloc(sizeof(*p), GFP_KERNEL);
It's bloody hard to spot.
In case of UFS we still catch "sizeof(struct ufs_inode_info)" in
ufs_inode_cachep = kmem_cache_create("ufs_inode_cache",
sizeof(struct ufs_inode_info),
0, (SLAB_RECLAIM_ACCOUNT|
SLAB_MEM_SPREAD),
init_once);
which gives the obvious secondary search pattern, so it's not _that_ terrible,
but in general it's not something to do without thinking - such a cast could
be placed there exactly to make it stand out on grep.
> On 28 December 2014 at 19:21 Al Viro <[email protected]> wrote:
>
>
> On Sun, Dec 28, 2014 at 08:45:44AM -0800, Joe Perches wrote:
> > On Sun, 2014-12-28 at 15:33 +0000, Al Viro wrote:
> > > On Sun, Dec 28, 2014 at 04:28:29PM +0100, Fabian Frederick wrote:
> > > > Fix the following coccinelle warning:
> > > > fs/ufs/super.c:1418:7-28: WARNING: casting value returned by memory
> > > > allocation function to (struct ufs_inode_info *) is useless.
> > >
> > > ... except that it makes allocations harder to grep for.
> >
> > How does it do that?
>
> search for \<struct[ ]*$NAME[ ]*($|[^ *]|[*][ ]*[)])
> generally gives a lot of interesting information (variables of that type,
> members of anything having that type, sizeof, container_of, explicit casts)
> with relatively little noise.
>
> BTW, that's why I really don't like the stuff like
> struct foo *p = kmalloc(sizeof(*p), GFP_KERNEL);
> It's bloody hard to spot.
>
> In case of UFS we still catch "sizeof(struct ufs_inode_info)" in
> ufs_inode_cachep = kmem_cache_create("ufs_inode_cache",
> sizeof(struct ufs_inode_info),
> 0, (SLAB_RECLAIM_ACCOUNT|
> SLAB_MEM_SPREAD),
> init_once);
> which gives the obvious secondary search pattern, so it's not _that_ terrible,
> but in general it's not something to do without thinking - such a cast could
> be placed there exactly to make it stand out on grep.
Thanks a lot for taking the time to explain the problem Al.
I'll be more careful next time.
Regards,
Fabian