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
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.
Hi Anton,
This patch converts explicit NULL assignments to use struct initializers as
suggested by Al Viro.
Signed-off-by: Pekka Enberg <[email protected]>
---
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();
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/
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/