Re: ntfs: remove redundant assignments

Re: ntfs: remove redundant assignments

[Pekka J Enberg]

Hi Anton, 

At some point in time, I wrote:
> Index: 2.6-mm/fs/ntfs/super.c
> > ===================================================================
> > --- 2.6-mm.orig/fs/ntfs/super.c	2005-05-25 20:51:57.000000000 +0300
> > +++ 2.6-mm/fs/ntfs/super.c	2005-05-25 20:54:02.000000000 +0300
> > @@ -2283,7 +2283,7 @@
> >  	sb->s_flags |= MS_RDONLY | MS_NOATIME | MS_NODIRATIME;
> >  #endif /* ! NTFS_RW */
> >  	/* Allocate a new ntfs_volume and place it in sb->s_fs_info. */
> > -	sb->s_fs_info = kmalloc(sizeof(ntfs_volume), GFP_NOFS);
> > +	sb->s_fs_info = kcalloc(1, sizeof(ntfs_volume), GFP_NOFS);
> >  	vol = NTFS_SB(sb);
> >  	if (!vol) {
> >  		if (!silent)
> > @@ -2292,28 +2292,9 @@
> >  		return -ENOMEM;
> >  	}
> >  	/* Initialize ntfs_volume structure. */
> > -	memset(vol, 0, sizeof(ntfs_volume));
> >  	vol->sb = sb; 
> 
> The above is fine, thanks. 
> 
> > -	vol->upcase = NULL;
> > -	vol->attrdef = NULL;
> > -	vol->mft_ino = NULL;
> > -	vol->mftbmp_ino = NULL;
> >  	init_rwsem(&vol->mftbmp_lock);
> > -#ifdef NTFS_RW
> > -	vol->mftmirr_ino = NULL;
> > -	vol->logfile_ino = NULL;
> > -#endif /* NTFS_RW */
> > -	vol->lcnbmp_ino = NULL;
> >  	init_rwsem(&vol->lcnbmp_lock);
> > -	vol->vol_ino = NULL;
> > -	vol->root_ino = NULL;
> > -	vol->secure_ino = NULL;
> > -	vol->extend_ino = NULL;
> > -#ifdef NTFS_RW
> > -	vol->quota_ino = NULL;
> > -	vol->quota_q_ino = NULL;
> > -#endif /* NTFS_RW */
> > -	vol->nls_map = NULL;

On Wed, 2005-05-25 at 22:10 +0100, Anton Altaparmakov wrote:
> This is not.  memset(0) is not the same as = NULL IMO.  I don't care if 
> the compiler thinks it is the same.  NULL does not have to be 0 so I 
> prefer to initialize pointers explicitly to NULL.  Even more so since this 
> code is not performance critical at all so I prefer clarity here.

I kind of figured out you were doing it on purpose. The fact is, NULL is 
zero on _all_ Linux architectures. If it weren't, we'd have a lot of broken 
code. Let me play the devils advocate here: why do you memset() (now 
kcalloc()) in the first place? 

At some point in time, I wrote:
> > Index: 2.6-mm/fs/ntfs/index.c
> > ===================================================================
> > --- 2.6-mm.orig/fs/ntfs/index.c	2005-05-25 20:51:57.000000000 +0300
> > +++ 2.6-mm/fs/ntfs/index.c	2005-05-25 21:07:38.000000000 +0300
> > @@ -40,16 +40,8 @@
> >  
> >  	ictx = kmem_cache_alloc(ntfs_index_ctx_cache, SLAB_NOFS);
> >  	if (ictx) {
> > +		memset(ictx, 0, sizeof(*ictx));
> >  		ictx->idx_ni = idx_ni;
> > -		ictx->entry = NULL;
> > -		ictx->data = NULL;
> > -		ictx->data_len = 0;
> > -		ictx->is_in_root = 0;
> > -		ictx->ir = NULL;
> > -		ictx->actx = NULL;
> > -		ictx->base_ni = NULL;
> > -		ictx->ia = NULL;
> > -		ictx->page = NULL;

On Wed, 2005-05-25 at 22:10 +0100, Anton Altaparmakov wrote:
> Again, as above, I prefer to have the explicit = NULL instead of a memset.

There's a simple reason why I don't like explicit assignments: it's way too 
easy to forget to initialize something. 

			Pekka 

Re: ntfs: remove redundant assignments

[Al Viro]

On Thu, May 26, 2005 at 09:21:46AM +0300, Pekka J Enberg wrote:
> On Wed, 2005-05-25 at 22:10 +0100, Anton Altaparmakov wrote:
> >This is not.  memset(0) is not the same as = NULL IMO.  I don't care if 
> >the compiler thinks it is the same.  NULL does not have to be 0 so I 
> >prefer to initialize pointers explicitly to NULL.  Even more so since this 
> >code is not performance critical at all so I prefer clarity here.
> 
> I kind of figured out you were doing it on purpose. The fact is, NULL is 
> zero on _all_ Linux architectures. If it weren't, we'd have a lot of broken 
> code. Let me play the devils advocate here: why do you memset() (now 
> kcalloc()) in the first place? 

Oh, come on...

	ictx = kmalloc(sizeof(ntfs_index_context), GFP_NOFS);
	if (ictx)
		*ictx = (ntfs_index_context){.idx_ni = idx_ni};
	return ictx;

and be done with that.  Let compiler do its job.  And yes, that *will*
give properly initialized pointers even for weird platforms.  Not that
we had the slightest chance of porting to any of them...

> There's a simple reason why I don't like explicit assignments: it's way too 
> easy to forget to initialize something. 

So use the proper constructs.  Variant above is guaranteed to do the right
thing on any C99 compiler, provided that kmalloc() returns NULL or pointer
to object sufficiently large and properly aligned for ntfs_index_context.
All missing fields will be initialized the same way they would for initializer
of a static object.

[PATCH] ntfs: use struct initializers

[Pekka J Enberg]

Hi Anton,

This patch converts explicit NULL assignments to use struct initializers as
suggested by Al Viro.

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

 super.c |   40 ++++++++++++----------------------------
 1 files changed, 12 insertions(+), 28 deletions(-)

Index: 2.6-mm/fs/ntfs/super.c
===================================================================
--- 2.6-mm.orig/fs/ntfs/super.c	2005-05-26 10:18:41.000000000 +0300
+++ 2.6-mm/fs/ntfs/super.c	2005-05-26 11:07:44.000000000 +0300
@@ -2292,36 +2292,20 @@
 		return -ENOMEM;
 	}
 	/* Initialize ntfs_volume structure. */
-	memset(vol, 0, sizeof(ntfs_volume));
-	vol->sb = sb;
-	vol->upcase = NULL;
-	vol->attrdef = NULL;
-	vol->mft_ino = NULL;
-	vol->mftbmp_ino = NULL;
+	*vol = (ntfs_volume) {
+		.sb = sb,
+		/*
+		 * Default is group and other don't have any access to files or
+		 * directories while owner has full access. Further, files by
+		 * default are not executable but directories are of course
+		 * browseable.
+		 */
+		.fmask = 0177,
+		.dmask = 0077,
+
+	};
 	init_rwsem(&vol->mftbmp_lock);
-#ifdef NTFS_RW
-	vol->mftmirr_ino = NULL;
-	vol->logfile_ino = NULL;
-#endif /* NTFS_RW */
-	vol->lcnbmp_ino = NULL;
 	init_rwsem(&vol->lcnbmp_lock);
-	vol->vol_ino = NULL;
-	vol->root_ino = NULL;
-	vol->secure_ino = NULL;
-	vol->extend_ino = NULL;
-#ifdef NTFS_RW
-	vol->quota_ino = NULL;
-	vol->quota_q_ino = NULL;
-#endif /* NTFS_RW */
-	vol->nls_map = NULL;
-
-	/*
-	 * Default is group and other don't have any access to files or
-	 * directories while owner has full access. Further, files by default
-	 * are not executable but directories are of course browseable.
-	 */
-	vol->fmask = 0177;
-	vol->dmask = 0077;
 
 	unlock_kernel();
 

Re: [PATCH] ntfs: use struct initializers

[Anton Altaparmakov]

Hi Pekka,

On Thu, 26 May 2005, Pekka J Enberg wrote:
> This patch converts explicit NULL assignments to use struct initializers as
> suggested by Al Viro.

Thanks.  I applied this (slightly modified since you went outside the 80 
char width) as well as equivalent changes to attrib.c and index.c.

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/

Re: ntfs: remove redundant assignments

[cutaway]

---- Original Message ----- 
From: "Al Viro" <viro@parcelfarce.linux.theplanet.co.uk>
To: "Pekka J Enberg" <penberg@cs.helsinki.fi>
Cc: "Anton Altaparmakov" <aia21@cam.ac.uk>;
<linux-ntfs-dev@lists.sourceforge.net>; <linux-kernel@vger.kernel.org>
Sent: Thursday, May 26, 2005 03:04
Subject: Re: ntfs: remove redundant assignments


> On Thu, May 26, 2005 at 09:21:46AM +0300, Pekka J Enberg wrote:
> > On Wed, 2005-05-25 at 22:10 +0100, Anton Altaparmakov wrote:
> > >This is not.  memset(0) is not the same as = NULL IMO.  I don't care if
> > >the compiler thinks it is the same.  NULL does not have to be 0 so I
> > >prefer to initialize pointers explicitly to NULL.  Even more so since
this
> > >code is not performance critical at all so I prefer clarity here.

FWIW, a series of explicit assignments to zero puffs up the code on x86.
Several GCC releases using the default kernel -O2 build are too dumb to zero
EAX, or some other reg, and assign using it, so you're looking at 4 bytes of
immediate zero plus opcode ModRm/SIB.  If the locations being assigned
happened to be statics you can often wind up with a 10 byte instruction to
zero a single dword somewhere.

I have tried a few tricks trying to get GCC to not use this 4 byte immediate
0 in non speed critical areas all to no avail (ex. foo &= 0;  the AND
instruction can use a one byte sign extended operand, but GCC morphs
something like " &= 0" into a straight up "MOV thing,imm32" instruction)

Re: ntfs: remove redundant assignments

[Anton Altaparmakov]

On Thu, 26 May 2005, Al Viro wrote:
> On Thu, May 26, 2005 at 09:21:46AM +0300, Pekka J Enberg wrote:
> > On Wed, 2005-05-25 at 22:10 +0100, Anton Altaparmakov wrote:
> > >This is not.  memset(0) is not the same as = NULL IMO.  I don't care if 
> > >the compiler thinks it is the same.  NULL does not have to be 0 so I 
> > >prefer to initialize pointers explicitly to NULL.  Even more so since this 
> > >code is not performance critical at all so I prefer clarity here.
> > 
> > I kind of figured out you were doing it on purpose. The fact is, NULL is 
> > zero on _all_ Linux architectures. If it weren't, we'd have a lot of broken 
> > code. Let me play the devils advocate here: why do you memset() (now 
> > kcalloc()) in the first place? 
> 
> Oh, come on...
> 
> 	ictx = kmalloc(sizeof(ntfs_index_context), GFP_NOFS);
> 	if (ictx)
> 		*ictx = (ntfs_index_context){.idx_ni = idx_ni};
> 	return ictx;
> 
> and be done with that.  Let compiler do its job.  And yes, that *will*
> give properly initialized pointers even for weird platforms.  Not that
> we had the slightest chance of porting to any of them...

Oh, cool.  I didn't think gcc-2.95 did this but I just tried it with 
2.95.2 and it worked.

Thanks,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/