Re: [PATCH 1/3] freevxfs: fix buffer_head leak

Re: [PATCH 1/3] freevxfs: fix buffer_head leak

[Andrew Morton]

Pekka Enberg <penberg@cs.helsinki.fi> wrote:
>
> This patch fixes a buffer_head leak in the function vxfs_getfsh by
> allocating the buffer before doing sb_bread(). In addition, the patch
> replaces misused SLAB_KERNEL flag with the proper GFP_KERNEL and adds
> a NULL check for sb_bread.
> 

Yes, there does seem to be a leak there.

> ===================================================================
> --- 2.6.orig/fs/freevxfs/vxfs_fshead.c	2005-06-28 19:48:12.000000000 +0300
> +++ 2.6/fs/freevxfs/vxfs_fshead.c	2005-06-28 19:48:34.000000000 +0300
> @@ -76,19 +76,22 @@
>  vxfs_getfsh(struct inode *ip, int which)
>  {
>  	struct buffer_head		*bp;
> +	struct vxfs_fsh			*fhp;
> +
> +	if (!(fhp = kmalloc(sizeof(*fhp), GFP_KERNEL)))
> +		goto failed;
>  
>  	bp = vxfs_bread(ip, which);
> -	if (buffer_mapped(bp)) {
> -		struct vxfs_fsh		*fhp;
> +	if (!bp || !buffer_mapped(bp))
> +		goto failed;
>  
> -		if (!(fhp = kmalloc(sizeof(*fhp), SLAB_KERNEL)))
> -			return NULL;
> -		memcpy(fhp, bp->b_data, sizeof(*fhp));
> +	memcpy(fhp, bp->b_data, sizeof(*fhp));
>  
> -		brelse(bp);
> -		return (fhp);
> -	}
> +	brelse(bp);
> +	return fhp;
>  
> +failed:
> +	kfree(fhp);
>  	return NULL;
>  }
>  

But your change means that we'll always perform that kmalloc, even if the
buffer came back !buffer_mapped().

<looks>

I don't think sb_bread() can return an unmapped buffer at all.

And sb_bread() can return NULL (I/O error) and we're not checking for that
in there.

Something like this?

diff -puN fs/freevxfs/vxfs_fshead.c~freevxfs-fix-buffer_head-leak fs/freevxfs/vxfs_fshead.c
--- 25/fs/freevxfs/vxfs_fshead.c~freevxfs-fix-buffer_head-leak	Tue Jun 28 16:19:53 2005
+++ 25-akpm/fs/freevxfs/vxfs_fshead.c	Tue Jun 28 16:24:53 2005
@@ -78,14 +78,16 @@ vxfs_getfsh(struct inode *ip, int which)
 	struct buffer_head		*bp;
 
 	bp = vxfs_bread(ip, which);
-	if (buffer_mapped(bp)) {
+	if (bp && buffer_mapped(bp)) {
 		struct vxfs_fsh		*fhp;
 
-		if (!(fhp = kmalloc(sizeof(*fhp), SLAB_KERNEL)))
+		if (!(fhp = kmalloc(sizeof(*fhp), GFP_KERNEL))) {
+			put_bh(bp);
 			return NULL;
+		}
 		memcpy(fhp, bp->b_data, sizeof(*fhp));
 
-		brelse(bp);
+		put_bh(bp);
 		return (fhp);
 	}
 
_

Re: [PATCH 2/3] freevxfs: minor cleanups

[Andrew Morton]

Pekka Enberg <penberg@cs.helsinki.fi> wrote:
>
> This patch addresses the following minor issues:
> 
>   - Typo in printk
>   - Redundant casts
>   - Use C99 struct initializers instead of memset
>   - Parenthesis around return value
>   - Use inline instead of __inline__

That struct initialisation:

> +	*infp = (struct vxfs_sb_info) {
> +		.vsi_raw = rsbp,
> +		.vsi_bp = bp,
> +		.vsi_oltext = rsbp->vs_oltext[0],
> +		.vsi_oltsize = rsbp->vs_oltsize,
> +	};
>  

Is a bit unconventional, but it doesn't alter the size of the .o file, so
whatever.

Re: [PATCH 3/3] freevxfs: remove 2.4 compatability

[Andrew Morton]

Pekka Enberg <penberg@cs.helsinki.fi> wrote:
>
> This patch removes 2.4 compatability header from freevxfs.

Maybe.  That's a bit of a pain if someone is actually maintaining this one
codebase under both kernels though...

Re: [PATCH 2/3] freevxfs: minor cleanups

[Pekka Enberg]

Hi,

On Tue, 2005-06-28 at 16:31 -0700, Andrew Morton wrote:
> That struct initialisation:
> 
> > +	*infp = (struct vxfs_sb_info) {
> > +		.vsi_raw = rsbp,
> > +		.vsi_bp = bp,
> > +		.vsi_oltext = rsbp->vs_oltext[0],
> > +		.vsi_oltsize = rsbp->vs_oltsize,
> > +	};
> 
> Is a bit unconventional, but it doesn't alter the size of the .o file, so
> whatever.

The rationale for this is that since NULL is not guaranteed to be zero
by the C standard, memset() doesn't really initialize pointers properly.
The above does. This came up when I wanted to replace explicit NULL
assignments from the NTFS code with kcalloc() and the maintainer
refused. Al Viro suggested the above form and was accepted by the NTFS
maintainer as well.

			Pekka

Re: [PATCH 1/3] freevxfs: fix buffer_head leak

[Pekka Enberg]

On Tue, 2005-06-28 at 16:28 -0700, Andrew Morton wrote:
> But your change means that we'll always perform that kmalloc, even if the
> buffer came back !buffer_mapped().

I think I originally dropped that buffer_mapped() check (which doesn't
make sense to me) which is why I shuffled the code around.

On Tue, 2005-06-28 at 16:28 -0700, Andrew Morton wrote:
> Something like this?

Works for me. Thanks.

> diff -puN fs/freevxfs/vxfs_fshead.c~freevxfs-fix-buffer_head-leak fs/freevxfs/vxfs_fshead.c
> --- 25/fs/freevxfs/vxfs_fshead.c~freevxfs-fix-buffer_head-leak	Tue Jun 28 16:19:53 2005
> +++ 25-akpm/fs/freevxfs/vxfs_fshead.c	Tue Jun 28 16:24:53 2005
> @@ -78,14 +78,16 @@ vxfs_getfsh(struct inode *ip, int which)
>  	struct buffer_head		*bp;
>  
>  	bp = vxfs_bread(ip, which);
> -	if (buffer_mapped(bp)) {
> +	if (bp && buffer_mapped(bp)) {
>  		struct vxfs_fsh		*fhp;
>  
> -		if (!(fhp = kmalloc(sizeof(*fhp), SLAB_KERNEL)))
> +		if (!(fhp = kmalloc(sizeof(*fhp), GFP_KERNEL))) {
> +			put_bh(bp);
>  			return NULL;
> +		}
>  		memcpy(fhp, bp->b_data, sizeof(*fhp));
>  
> -		brelse(bp);
> +		put_bh(bp);
>  		return (fhp);
>  	}
>  
> _
> 

Re: [PATCH 3/3] freevxfs: remove 2.4 compatability

[Christoph Hellwig]

On Tue, Jun 28, 2005 at 04:33:38PM -0700, Andrew Morton wrote:
> Pekka Enberg <penberg@cs.helsinki.fi> wrote:
> >
> > This patch removes 2.4 compatability header from freevxfs.
> 
> Maybe.  That's a bit of a pain if someone is actually maintaining this one
> codebase under both kernels though...

the header can go, these days there's too much difference for it to make
any sense.

Re: [PATCH 2/3] freevxfs: minor cleanups

[Christoph Hellwig]

On Tue, Jun 28, 2005 at 04:31:14PM -0700, Andrew Morton wrote:
> Pekka Enberg <penberg@cs.helsinki.fi> wrote:
> >
> > This patch addresses the following minor issues:
> > 
> >   - Typo in printk
> >   - Redundant casts
> >   - Use C99 struct initializers instead of memset
> >   - Parenthesis around return value
> >   - Use inline instead of __inline__
> 
> That struct initialisation:
> 
> > +	*infp = (struct vxfs_sb_info) {
> > +		.vsi_raw = rsbp,
> > +		.vsi_bp = bp,
> > +		.vsi_oltext = rsbp->vs_oltext[0],
> > +		.vsi_oltsize = rsbp->vs_oltsize,
> > +	};
> >  
> 
> Is a bit unconventional, but it doesn't alter the size of the .o file, so
> whatever.

It looks rather horrible, I wouldn't call that a cleanup.  Where's the
full patch?

Re: [PATCH 2/3] freevxfs: minor cleanups

[Christoph Hellwig]

On Wed, Jun 29, 2005 at 07:20:21AM +0300, Pekka Enberg wrote:
> The rationale for this is that since NULL is not guaranteed to be zero
> by the C standard, memset() doesn't really initialize pointers properly.

For all the machines we care it does. If a maintainer refuses to acccept
that he or she is stupid.

Re: [PATCH 1/3] freevxfs: fix buffer_head leak

[Christoph Hellwig]

> Something like this?
> 
> diff -puN fs/freevxfs/vxfs_fshead.c~freevxfs-fix-buffer_head-leak fs/freevxfs/vxfs_fshead.c
> --- 25/fs/freevxfs/vxfs_fshead.c~freevxfs-fix-buffer_head-leak	Tue Jun 28 16:19:53 2005
> +++ 25-akpm/fs/freevxfs/vxfs_fshead.c	Tue Jun 28 16:24:53 2005
> @@ -78,14 +78,16 @@ vxfs_getfsh(struct inode *ip, int which)
>  	struct buffer_head		*bp;
>  
>  	bp = vxfs_bread(ip, which);
> -	if (buffer_mapped(bp)) {
> +	if (bp && buffer_mapped(bp)) {
>  		struct vxfs_fsh		*fhp;
>  
> -		if (!(fhp = kmalloc(sizeof(*fhp), SLAB_KERNEL)))
> +		if (!(fhp = kmalloc(sizeof(*fhp), GFP_KERNEL))) {
> +			put_bh(bp);
>  			return NULL;
> +		}
>  		memcpy(fhp, bp->b_data, sizeof(*fhp));
>  
> -		brelse(bp);
> +		put_bh(bp);
>  		return (fhp);
>  	}

you're still leaking a buffer in the hypothetical !buffer_mapped() case.
Better remove that check completely.

Re: [PATCH 2/3] freevxfs: minor cleanups

[Andrew Morton]

Christoph Hellwig <hch@infradead.org> wrote:
>
> On Tue, Jun 28, 2005 at 04:31:14PM -0700, Andrew Morton wrote:
> > Pekka Enberg <penberg@cs.helsinki.fi> wrote:
> > >
> > > This patch addresses the following minor issues:
> > > 
> > >   - Typo in printk
> > >   - Redundant casts
> > >   - Use C99 struct initializers instead of memset
> > >   - Parenthesis around return value
> > >   - Use inline instead of __inline__
> > 
> > That struct initialisation:
> > 
> > > +	*infp = (struct vxfs_sb_info) {
> > > +		.vsi_raw = rsbp,
> > > +		.vsi_bp = bp,
> > > +		.vsi_oltext = rsbp->vs_oltext[0],
> > > +		.vsi_oltsize = rsbp->vs_oltsize,
> > > +	};
> > >  
> > 
> > Is a bit unconventional, but it doesn't alter the size of the .o file, so
> > whatever.
> 
> It looks rather horrible, I wouldn't call that a cleanup.  Where's the
> full patch?

Come to think of it, it could be a problem if the comnpiler was silly and
built an entire temporary on the stack and the copied it over.  Hopefull it
won't do that.


From: Pekka Enberg <penberg@cs.helsinki.fi>

This patch addresses the following minor issues:

  - Typo in printk
  - Redundant casts
  - Use C99 struct initializers instead of memset
  - Parenthesis around return value
  - Use inline instead of __inline__

Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Andrew Morton <akpm@osdl.org>
---

 fs/freevxfs/vxfs_bmap.c   |    2 +-
 fs/freevxfs/vxfs_lookup.c |    8 ++++----
 fs/freevxfs/vxfs_olt.c    |   10 +++++-----
 fs/freevxfs/vxfs_super.c  |   15 ++++++++-------
 4 files changed, 18 insertions(+), 17 deletions(-)

diff -puN fs/freevxfs/vxfs_bmap.c~freevxfs-minor-cleanups fs/freevxfs/vxfs_bmap.c
--- 25/fs/freevxfs/vxfs_bmap.c~freevxfs-minor-cleanups	Tue Jun 28 16:31:21 2005
+++ 25-akpm/fs/freevxfs/vxfs_bmap.c	Tue Jun 28 16:31:21 2005
@@ -101,7 +101,7 @@ vxfs_bmap_ext4(struct inode *ip, long bn
 	return 0;
 
 fail_size:
-	printk("vxfs: indirect extent to big!\n");
+	printk("vxfs: indirect extent too big!\n");
 fail_buf:
 	return 0;
 }
diff -puN fs/freevxfs/vxfs_lookup.c~freevxfs-minor-cleanups fs/freevxfs/vxfs_lookup.c
--- 25/fs/freevxfs/vxfs_lookup.c~freevxfs-minor-cleanups	Tue Jun 28 16:31:21 2005
+++ 25-akpm/fs/freevxfs/vxfs_lookup.c	Tue Jun 28 16:31:21 2005
@@ -61,13 +61,13 @@ struct file_operations vxfs_dir_operatio
 };
 
  
-static __inline__ u_long
+static inline u_long
 dir_pages(struct inode *inode)
 {
 	return (inode->i_size + PAGE_CACHE_SIZE - 1) >> PAGE_CACHE_SHIFT;
 }
  
-static __inline__ u_long
+static inline u_long
 dir_blocks(struct inode *ip)
 {
 	u_long			bsize = ip->i_sb->s_blocksize;
@@ -79,7 +79,7 @@ dir_blocks(struct inode *ip)
  *
  * len <= VXFS_NAMELEN and de != NULL are guaranteed by caller.
  */
-static __inline__ int
+static inline int
 vxfs_match(int len, const char * const name, struct vxfs_direct *de)
 {
 	if (len != de->d_namelen)
@@ -89,7 +89,7 @@ vxfs_match(int len, const char * const n
 	return !memcmp(name, de->d_name, len);
 }
 
-static __inline__ struct vxfs_direct *
+static inline struct vxfs_direct *
 vxfs_next_entry(struct vxfs_direct *de)
 {
 	return ((struct vxfs_direct *)((char*)de + de->d_reclen));
diff -puN fs/freevxfs/vxfs_olt.c~freevxfs-minor-cleanups fs/freevxfs/vxfs_olt.c
--- 25/fs/freevxfs/vxfs_olt.c~freevxfs-minor-cleanups	Tue Jun 28 16:31:21 2005
+++ 25-akpm/fs/freevxfs/vxfs_olt.c	Tue Jun 28 16:31:21 2005
@@ -38,7 +38,7 @@
 #include "vxfs_olt.h"
 
 
-static __inline__ void
+static inline void
 vxfs_get_fshead(struct vxfs_oltfshead *fshp, struct vxfs_sb_info *infp)
 {
 	if (infp->vsi_fshino)
@@ -46,7 +46,7 @@ vxfs_get_fshead(struct vxfs_oltfshead *f
 	infp->vsi_fshino = fshp->olt_fsino[0];
 }
 
-static __inline__ void
+static inline void
 vxfs_get_ilist(struct vxfs_oltilist *ilistp, struct vxfs_sb_info *infp)
 {
 	if (infp->vsi_iext)
@@ -54,7 +54,7 @@ vxfs_get_ilist(struct vxfs_oltilist *ili
 	infp->vsi_iext = ilistp->olt_iext[0]; 
 }
 
-static __inline__ u_long
+static inline u_long
 vxfs_oblock(struct super_block *sbp, daddr_t block, u_long bsize)
 {
 	if (sbp->s_blocksize % bsize)
@@ -104,8 +104,8 @@ vxfs_read_olt(struct super_block *sbp, u
 		goto fail;
 	}
 
-	oaddr = (char *)bp->b_data + op->olt_size;
-	eaddr = (char *)bp->b_data + (infp->vsi_oltsize * sbp->s_blocksize);
+	oaddr = bp->b_data + op->olt_size;
+	eaddr = bp->b_data + (infp->vsi_oltsize * sbp->s_blocksize);
 
 	while (oaddr < eaddr) {
 		struct vxfs_oltcommon	*ocp =
diff -puN fs/freevxfs/vxfs_super.c~freevxfs-minor-cleanups fs/freevxfs/vxfs_super.c
--- 25/fs/freevxfs/vxfs_super.c~freevxfs-minor-cleanups	Tue Jun 28 16:31:21 2005
+++ 25-akpm/fs/freevxfs/vxfs_super.c	Tue Jun 28 16:31:21 2005
@@ -160,7 +160,6 @@ static int vxfs_fill_super(struct super_
 		printk(KERN_WARNING "vxfs: unable to allocate incore superblock\n");
 		return -ENOMEM;
 	}
-	memset(infp, 0, sizeof(*infp));
 
 	bsize = sb_min_blocksize(sbp, BLOCK_SIZE);
 	if (!bsize) {
@@ -196,12 +195,14 @@ static int vxfs_fill_super(struct super_
 #endif
 
 	sbp->s_magic = rsbp->vs_magic;
-	sbp->s_fs_info = (void *)infp;
+	sbp->s_fs_info = infp;
 
-	infp->vsi_raw = rsbp;
-	infp->vsi_bp = bp;
-	infp->vsi_oltext = rsbp->vs_oltext[0];
-	infp->vsi_oltsize = rsbp->vs_oltsize;
+	*infp = (struct vxfs_sb_info) {
+		.vsi_raw = rsbp,
+		.vsi_bp = bp,
+		.vsi_oltext = rsbp->vs_oltext[0],
+		.vsi_oltsize = rsbp->vs_oltsize,
+	};
 
 	if (!sb_set_blocksize(sbp, rsbp->vs_bsize)) {
 		printk(KERN_WARNING "vxfs: unable to set final block size\n");
@@ -263,7 +264,7 @@ vxfs_init(void)
 			sizeof(struct vxfs_inode_info), 0, 
 			SLAB_RECLAIM_ACCOUNT, NULL, NULL);
 	if (vxfs_inode_cachep)
-		return (register_filesystem(&vxfs_fs_type));
+		return register_filesystem(&vxfs_fs_type);
 	return -ENOMEM;
 }
 
_

Re: [PATCH 2/3] freevxfs: minor cleanups

[Christoph Hellwig]

On Wed, Jun 29, 2005 at 12:17:17AM -0700, Andrew Morton wrote:
> Come to think of it, it could be a problem if the comnpiler was silly and
> built an entire temporary on the stack and the copied it over.  Hopefull it
> won't do that.

that patch is fine except for the second to last patch which should be
droped.

Re: [PATCH 1/3] freevxfs: fix buffer_head leak

[Andrew Morton]

Christoph Hellwig <hch@infradead.org> wrote:
>
> you're still leaking a buffer in the hypothetical !buffer_mapped() case.
>  Better remove that check completely.

OK..


--- 25/fs/freevxfs/vxfs_fshead.c~freevxfs-fix-buffer_head-leak	2005-06-28 23:55:53.000000000 -0700
+++ 25-akpm/fs/freevxfs/vxfs_fshead.c	2005-06-29 00:21:23.000000000 -0700
@@ -78,17 +78,18 @@ vxfs_getfsh(struct inode *ip, int which)
 	struct buffer_head		*bp;
 
 	bp = vxfs_bread(ip, which);
-	if (buffer_mapped(bp)) {
+	if (bp) {
 		struct vxfs_fsh		*fhp;
 
-		if (!(fhp = kmalloc(sizeof(*fhp), SLAB_KERNEL)))
-			return NULL;
+		if (!(fhp = kmalloc(sizeof(*fhp), GFP_KERNEL)))
+			goto out;
 		memcpy(fhp, bp->b_data, sizeof(*fhp));
 
-		brelse(bp);
+		put_bh(bp);
 		return (fhp);
 	}
-
+out:
+	brelse(bp);
 	return NULL;
 }
 
_

Re: [PATCH 2/3] freevxfs: minor cleanups

[Matthias Urlichs]

Hi, Pekka Enberg wrote:

> The rationale for this is that since NULL is not guaranteed to be zero
> by the C standard

... as opposed to the other 632719 places in the kernel source where
we do the exact same thing?

If Linux ever gets ported to an architecture where NULL is not
all-bits-zero, the resulting patch will be so damn huge that the
cleanliness-or-not of freevxfs will be the *least* of our worries.

-- 
Matthias Urlichs   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de
Disclaimer: The quote was selected randomly. Really. | http://smurf.noris.de
 - -
You'd best be snoozin', 'cause you don't be gettin' no work done at 5 a.m.
anyway.
		-- From the wall of the Wurster Hall stairwell

Re: [PATCH 2/3] freevxfs: minor cleanups

[Matthias Urlichs]

Hi, Christoph Hellwig wrote:

> On Wed, Jun 29, 2005 at 12:17:17AM -0700, Andrew Morton wrote:
>> Come to think of it, it could be a problem if the comnpiler was silly and
>> built an entire temporary on the stack and the copied it over.  Hopefull it
>> won't do that.
> 
> that patch is fine except for the second to last patch which should be
> droped.

If you do that, you also need to drop the third-to-last patch ...

Personally, I think struct assignments are fine if the compiler
does them right. As this example shows, at least they avoid the
I-forgot-to-zero-the-struct problem. ;-)

-- 
Matthias Urlichs   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de
Disclaimer: The quote was selected randomly. Really. | http://smurf.noris.de
 - -
First Rule of History:
	History doesn't repeat itself -- historians merely repeat each other.

Re: freevxfs: minor cleanups

[Pekka J Enberg]

On Wed, Jun 29, 2005 at 07:20:21AM +0300, Pekka Enberg wrote:
> > The rationale for this is that since NULL is not guaranteed to be zero
> > by the C standard, memset() doesn't really initialize pointers properly.

On Wed, 2005-06-29 at 08:08 +0100, Christoph Hellwig wrote:
> For all the machines we care it does. If a maintainer refuses to acccept
> that he or she is stupid.

I agree that pointer initialization is not really an issue but I do prefer 
the C99 struct initializers over an kcalloc(1, sizeof(*p)) call. 

Is this something you don't want for freevxfs or filesystems in general? 
Should it be removed from NTFS as well? 

			Pekka 

Re: freevxfs: minor cleanups

[Christoph Hellwig]

On Wed, Jun 29, 2005 at 10:42:39AM +0300, Pekka J Enberg wrote:
> 
> I agree that pointer initialization is not really an issue but I do prefer 
> the C99 struct initializers over an kcalloc(1, sizeof(*p)) call. 
> 
> Is this something you don't want for freevxfs or filesystems in general? 
> Should it be removed from NTFS as well? 

I don't have control over NTFS, but please don't introduce this idiom
anywhere else.

Re: [PATCH 2/3] freevxfs: minor cleanups

[Pekka J Enberg]

On Wed, Jun 29, 2005 at 12:17:17AM -0700, Andrew Morton wrote:
> > Come to think of it, it could be a problem if the comnpiler was silly and
> > built an entire temporary on the stack and the copied it over.  Hopefull it
> > won't do that.

On Wed, 29 Jun 2005, Christoph Hellwig wrote:
> that patch is fine except for the second to last patch which should be
> droped.

Andrew, here's an updated patch.

This patch addresses the following minor issues:

  - Typo in printk
  - Redundant casts
  - Use kcalloc instead of kmalloc and memset
  - Parenthesis around return value
  - Use inline instead of __inline__

Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---

 vxfs_bmap.c   |    2 +-
 vxfs_lookup.c |    8 ++++----
 vxfs_olt.c    |   10 +++++-----
 vxfs_super.c  |    7 +++----
 4 files changed, 13 insertions(+), 14 deletions(-)

Index: 2.6-git/fs/freevxfs/vxfs_bmap.c
===================================================================
--- 2.6-git.orig/fs/freevxfs/vxfs_bmap.c
+++ 2.6-git/fs/freevxfs/vxfs_bmap.c
@@ -101,7 +101,7 @@ vxfs_bmap_ext4(struct inode *ip, long bn
 	return 0;
 
 fail_size:
-	printk("vxfs: indirect extent to big!\n");
+	printk("vxfs: indirect extent too big!\n");
 fail_buf:
 	return 0;
 }
Index: 2.6-git/fs/freevxfs/vxfs_lookup.c
===================================================================
--- 2.6-git.orig/fs/freevxfs/vxfs_lookup.c
+++ 2.6-git/fs/freevxfs/vxfs_lookup.c
@@ -61,13 +61,13 @@ struct file_operations vxfs_dir_operatio
 };
 
  
-static __inline__ u_long
+static inline u_long
 dir_pages(struct inode *inode)
 {
 	return (inode->i_size + PAGE_CACHE_SIZE - 1) >> PAGE_CACHE_SHIFT;
 }
  
-static __inline__ u_long
+static inline u_long
 dir_blocks(struct inode *ip)
 {
 	u_long			bsize = ip->i_sb->s_blocksize;
@@ -79,7 +79,7 @@ dir_blocks(struct inode *ip)
  *
  * len <= VXFS_NAMELEN and de != NULL are guaranteed by caller.
  */
-static __inline__ int
+static inline int
 vxfs_match(int len, const char * const name, struct vxfs_direct *de)
 {
 	if (len != de->d_namelen)
@@ -89,7 +89,7 @@ vxfs_match(int len, const char * const n
 	return !memcmp(name, de->d_name, len);
 }
 
-static __inline__ struct vxfs_direct *
+static inline struct vxfs_direct *
 vxfs_next_entry(struct vxfs_direct *de)
 {
 	return ((struct vxfs_direct *)((char*)de + de->d_reclen));
Index: 2.6-git/fs/freevxfs/vxfs_olt.c
===================================================================
--- 2.6-git.orig/fs/freevxfs/vxfs_olt.c
+++ 2.6-git/fs/freevxfs/vxfs_olt.c
@@ -38,7 +38,7 @@
 #include "vxfs_olt.h"
 
 
-static __inline__ void
+static inline void
 vxfs_get_fshead(struct vxfs_oltfshead *fshp, struct vxfs_sb_info *infp)
 {
 	if (infp->vsi_fshino)
@@ -46,7 +46,7 @@ vxfs_get_fshead(struct vxfs_oltfshead *f
 	infp->vsi_fshino = fshp->olt_fsino[0];
 }
 
-static __inline__ void
+static inline void
 vxfs_get_ilist(struct vxfs_oltilist *ilistp, struct vxfs_sb_info *infp)
 {
 	if (infp->vsi_iext)
@@ -54,7 +54,7 @@ vxfs_get_ilist(struct vxfs_oltilist *ili
 	infp->vsi_iext = ilistp->olt_iext[0]; 
 }
 
-static __inline__ u_long
+static inline u_long
 vxfs_oblock(struct super_block *sbp, daddr_t block, u_long bsize)
 {
 	if (sbp->s_blocksize % bsize)
@@ -104,8 +104,8 @@ vxfs_read_olt(struct super_block *sbp, u
 		goto fail;
 	}
 
-	oaddr = (char *)bp->b_data + op->olt_size;
-	eaddr = (char *)bp->b_data + (infp->vsi_oltsize * sbp->s_blocksize);
+	oaddr = bp->b_data + op->olt_size;
+	eaddr = bp->b_data + (infp->vsi_oltsize * sbp->s_blocksize);
 
 	while (oaddr < eaddr) {
 		struct vxfs_oltcommon	*ocp =
Index: 2.6-git/fs/freevxfs/vxfs_super.c
===================================================================
--- 2.6-git.orig/fs/freevxfs/vxfs_super.c
+++ 2.6-git/fs/freevxfs/vxfs_super.c
@@ -155,12 +155,11 @@ static int vxfs_fill_super(struct super_
 
 	sbp->s_flags |= MS_RDONLY;
 
-	infp = kmalloc(sizeof(*infp), GFP_KERNEL);
+	infp = kcalloc(1, sizeof(*infp), GFP_KERNEL);
 	if (!infp) {
 		printk(KERN_WARNING "vxfs: unable to allocate incore superblock\n");
 		return -ENOMEM;
 	}
-	memset(infp, 0, sizeof(*infp));
 
 	bsize = sb_min_blocksize(sbp, BLOCK_SIZE);
 	if (!bsize) {
@@ -196,7 +195,7 @@ static int vxfs_fill_super(struct super_
 #endif
 
 	sbp->s_magic = rsbp->vs_magic;
-	sbp->s_fs_info = (void *)infp;
+	sbp->s_fs_info = infp;
 
 	infp->vsi_raw = rsbp;
 	infp->vsi_bp = bp;
@@ -263,7 +262,7 @@ vxfs_init(void)
 			sizeof(struct vxfs_inode_info), 0, 
 			SLAB_RECLAIM_ACCOUNT, NULL, NULL);
 	if (vxfs_inode_cachep)
-		return (register_filesystem(&vxfs_fs_type));
+		return register_filesystem(&vxfs_fs_type);
 	return -ENOMEM;
 }
 

Re: [PATCH 2/3] freevxfs: minor cleanups

[Pekka Enberg]

(Please don't trim the cc list when replying.)

Pekka Enberg wrote:
> > The rationale for this is that since NULL is not guaranteed to be zero
> > by the C standard

On 6/29/05, Matthias Urlichs <smurf@smurf.noris.de> wrote:
> ... as opposed to the other 632719 places in the kernel source where
> we do the exact same thing?

Well, as silly as it sounds to you, that _was_ the rationale for NTFS.
I actually like it better than using kcalloc() but at the end of the
day, I care more that the kernel uses same idioms all over. Makes the
code easier to understand for my tiny brain...

P.S. Those 632719 places can be fixed too with a handful of persistent
kernel janitors ;)

                               Pekka

Re: [PATCH 2/3] freevxfs: minor cleanups

[Roman Zippel]

Hi,

On Wed, 29 Jun 2005, Andrew Morton wrote:

> Come to think of it, it could be a problem if the comnpiler was silly and
> built an entire temporary on the stack and the copied it over.  Hopefull it
> won't do that.

It's not really silly, it's exactly what the compiler will do in such 
case, unless the final memcpy() is small enough to be inlined.

bye, Roman