In 2.5.7 there is a thinko in the allocation and initialisation
of the fs-private superblock for ext2. It's passing the wrong type
to the sizeof operator (which of course gives the wrong size)
when allocating and clearing the memory.
Lesson for the day: this is one of the reasons why this idiom:
some_type *p;
p = malloc(sizeof(*p));
...
memset(p, 0, sizeof(*p));
is preferable to
some_type *p;
p = malloc(sizeof(some_type));
...
memset(p, 0, sizeof(some_type));
I checked the other filesystems. They're OK (but idiomatically
impure). I've added a couple of defensive memsets where
they were missing.
--- 2.5.7/fs/autofs/inode.c~fill-super Wed Mar 27 23:14:20 2002
+++ 2.5.7-akpm/fs/autofs/inode.c Wed Mar 27 23:14:54 2002
@@ -119,9 +119,10 @@ int autofs_fill_super(struct super_block
struct autofs_sb_info *sbi;
int minproto, maxproto;
- sbi = (struct autofs_sb_info *) kmalloc(sizeof(struct autofs_sb_info), GFP_KERNEL);
+ sbi = kmalloc(sizeof(*sbi), GFP_KERNEL);
if ( !sbi )
goto fail_unlock;
+ memset(sbi, 0, sizeof(*sbi));
DPRINTK(("autofs: starting up, sbi = %p\n",sbi));
s->u.generic_sbp = sbi;
--- 2.5.7/fs/devpts/inode.c~fill-super Wed Mar 27 23:16:05 2002
+++ 2.5.7-akpm/fs/devpts/inode.c Wed Mar 27 23:16:33 2002
@@ -123,9 +123,10 @@ static int devpts_fill_super(struct supe
struct inode * inode;
struct devpts_sb_info *sbi;
- sbi = (struct devpts_sb_info *) kmalloc(sizeof(struct devpts_sb_info), GFP_KERNEL);
+ sbi = kmalloc(sizeof(*sbi), GFP_KERNEL);
if ( !sbi )
goto fail;
+ memset(sbi, 0, sizeof(*sbi));
sbi->magic = DEVPTS_SBI_MAGIC;
sbi->max_ptys = unix98_max_ptys;
--- 2.5.7/fs/ext2/super.c~fill-super Wed Mar 27 23:16:57 2002
+++ 2.5.7-akpm/fs/ext2/super.c Wed Mar 27 23:17:25 2002
@@ -465,11 +465,11 @@ static int ext2_fill_super(struct super_
int db_count;
int i, j;
- sbi = kmalloc(sizeof(struct ext2_super_block), GFP_KERNEL);
+ sbi = kmalloc(sizeof(*sbi), GFP_KERNEL);
if (!sbi)
return -ENOMEM;
sb->u.generic_sbp = sbi;
- memset(sbi, 0, sizeof(struct ext2_super_block));
+ memset(sbi, 0, sizeof(*sbi));
/*
* See what the current blocksize for the device is, and
-
Andrew Morton wrote:
> In 2.5.7 there is a thinko in the allocation and initialisation
> of the fs-private superblock for ext2. It's passing the wrong type
> to the sizeof operator (which of course gives the wrong size)
> when allocating and clearing the memory.
>
> Lesson for the day: this is one of the reasons why this idiom:
>
> some_type *p;
>
> p = malloc(sizeof(*p));
> ...
> memset(p, 0, sizeof(*p));
>
> is preferable to
>
> some_type *p;
>
> p = malloc(sizeof(some_type));
> ...
> memset(p, 0, sizeof(some_type));
>
> I checked the other filesystems. They're OK (but idiomatically
> impure). I've added a couple of defensive memsets where
> they were missing.
I'm not sure I follow you here. Compiling this code (gcc 2.96):
int foo1(void) { return sizeof(struct ext2_sb_info); }
int foo2(struct ext2_sb_info *sbi) { return sizeof(*sbi); }
yields:
00000b70 <foo1>:
b70: b8 dc 00 00 00 mov $0xdc,%eax
b75: c3 ret
00000b80 <foo2>:
b80: b8 dc 00 00 00 mov $0xdc,%eax
b85: c3 ret
The sizes are the same, so unless it makes a difference with another
version of gcc then this is just a cosmetic change.
--
Brian Gerst
On Thu, 28 Mar 2002, Brian Gerst wrote:
> Andrew Morton wrote:
> > Lesson for the day: this is one of the reasons why this idiom:
> > some_type *p;
> > p = malloc(sizeof(*p));
> > ...
> > memset(p, 0, sizeof(*p));
> > is preferable to
> > some_type *p;
> > p = malloc(sizeof(some_type));
> > ...
> > memset(p, 0, sizeof(some_type));
> I'm not sure I follow you here. Compiling this code (gcc 2.96):
....
It is not about what comes out of the compiler, it is about the fact that
in the second case, when some_type becomes another_type, you have to
replace some_type at 3 locations, easy to forget one. In the first case,
your compiler checks what the size of your variable is, the memset will
never fill beyond the end of your allocated memory.
It is about preventing patching again and again for someone forgot to use
:s/something/anotherthing/g. Besides, the code looks much better imho.
Jos
On Thursday 28 March 2002 08:34 am, Brian Gerst wrote:
> Andrew Morton wrote:
> > In 2.5.7 there is a thinko in the allocation and initialisation
> > of the fs-private superblock for ext2. It's passing the wrong type
> > to the sizeof operator (which of course gives the wrong size)
> > when allocating and clearing the memory.
> >
> > Lesson for the day: this is one of the reasons why this idiom:
> >
> > some_type *p;
> >
> > p = malloc(sizeof(*p));
> > ...
> > memset(p, 0, sizeof(*p));
> >
> > is preferable to
> >
> > some_type *p;
> >
> > p = malloc(sizeof(some_type));
> > ...
> > memset(p, 0, sizeof(some_type));
> >
> > I checked the other filesystems. They're OK (but idiomatically
> > impure). I've added a couple of defensive memsets where
> > they were missing.
>
> I'm not sure I follow you here. Compiling this code (gcc 2.96):
>
> int foo1(void) { return sizeof(struct ext2_sb_info); }
> int foo2(struct ext2_sb_info *sbi) { return sizeof(*sbi); }
>
> yields:
>
> 00000b70 <foo1>:
> b70: b8 dc 00 00 00 mov $0xdc,%eax
> b75: c3 ret
>
> 00000b80 <foo2>:
> b80: b8 dc 00 00 00 mov $0xdc,%eax
> b85: c3 ret
>
> The sizes are the same, so unless it makes a difference with another
> version of gcc then this is just a cosmetic change.
If you take the sizeof based on the type of the actual variable is using,
you're guaranteed to get the right result. (The type information is only in
one place, so what the compiler's using and what you're reserving space for
have to be the same thing.)
If you sizeof( ) the type and declare the type of the variable seperately,
you have the possibility of version skew between them. The sizeof( ) and the
variable declaration itself may not match. Hence bugs.
Rob
On Wed, 27 Mar 2002, Andrew Morton wrote:
> In 2.5.7 there is a thinko in the allocation and initialisation
> of the fs-private superblock for ext2. It's passing the wrong type
> to the sizeof operator (which of course gives the wrong size)
> when allocating and clearing the memory.
> Lesson for the day: this is one of the reasons why this idiom:
>
> some_type *p;
>
> p = malloc(sizeof(*p));
> ...
> memset(p, 0, sizeof(*p));
>
> is preferable to
>
> some_type *p;
>
> p = malloc(sizeof(some_type));
> ...
> memset(p, 0, sizeof(some_type));
... however, there is a lot of reasons why the former is preferable.
For one thing, the latter is hell on any search. Moreover, I would
argue that memset() on a structure is not a good idea - better do
the explicit initialization.
Alexander Viro writes:
>
>
> On Wed, 27 Mar 2002, Andrew Morton wrote:
>
> > In 2.5.7 there is a thinko in the allocation and initialisation
> > of the fs-private superblock for ext2. It's passing the wrong type
> > to the sizeof operator (which of course gives the wrong size)
> > when allocating and clearing the memory.
>
> > Lesson for the day: this is one of the reasons why this idiom:
> >
> > some_type *p;
> >
> > p = malloc(sizeof(*p));
> > ...
> > memset(p, 0, sizeof(*p));
> >
> > is preferable to
> >
> > some_type *p;
> >
> > p = malloc(sizeof(some_type));
> > ...
> > memset(p, 0, sizeof(some_type));
>
> ... however, there is a lot of reasons why the former is preferable.
> For one thing, the latter is hell on any search. Moreover, I would
> argue that memset() on a structure is not a good idea - better do
> the explicit initialization.
Explicit initialization always leaves room for some "pad" field inserted
by compiler for alignment to be left with garbage. This is more than
just annoyance when structure is something that will be written to the
disk. Reiserfs had such problems.
>
>
Nikita.
On Thu, 28 Mar 2002, Nikita Danilov wrote:
> Explicit initialization always leaves room for some "pad" field inserted
> by compiler for alignment to be left with garbage. This is more than
> just annoyance when structure is something that will be written to the
> disk. Reiserfs had such problems.
If your structure will be written on disk you'd better have full control
over alignment - otherwise you are risking incompatibilities between
platforms and compiler versions.
> Explicit initialization always leaves room for some "pad" field inserted
> by compiler for alignment to be left with garbage. This is more than
> just annoyance when structure is something that will be written to the
> disk. Reiserfs had such problems.
such compiler-based padding is architecture specific.. I'd hope the
reiserfs
on disk format isn't architecture specific ?
Alexander Viro writes:
>
>
> On Thu, 28 Mar 2002, Nikita Danilov wrote:
>
> > Explicit initialization always leaves room for some "pad" field inserted
> > by compiler for alignment to be left with garbage. This is more than
> > just annoyance when structure is something that will be written to the
> > disk. Reiserfs had such problems.
>
> If your structure will be written on disk you'd better have full control
> over alignment - otherwise you are risking incompatibilities between
> platforms and compiler versions.
Yes, but such experience frequently is only gained after format is
already carved in stone^Wdisk.
>
>
Nikita.
Arjan van de Ven writes:
>
> > Explicit initialization always leaves room for some "pad" field inserted
> > by compiler for alignment to be left with garbage. This is more than
> > just annoyance when structure is something that will be written to the
> > disk. Reiserfs had such problems.
>
> such compiler-based padding is architecture specific.. I'd hope the
> reiserfs
> on disk format isn't architecture specific ?
It is not.
>
Nikita.
On Thu, 28 Mar 2002, Nikita Danilov wrote:
> > If your structure will be written on disk you'd better have full control
> > over alignment - otherwise you are risking incompatibilities between
> > platforms and compiler versions.
>
> Yes, but such experience frequently is only gained after format is
> already carved in stone^Wdisk.
Or after reading through a FAQ or two. Not to mention any half-decent
textbook on C from mid-80s or later...
On Thu, 28 Mar 2002, Brian Gerst wrote:
> I'm not sure I follow you here. Compiling this code (gcc 2.96):
>
> int foo1(void) { return sizeof(struct ext2_sb_info); }
> int foo2(struct ext2_sb_info *sbi) { return sizeof(*sbi); }
>
> yields:
>
> 00000b70 <foo1>:
> b70: b8 dc 00 00 00 mov $0xdc,%eax
> b75: c3 ret
>
> 00000b80 <foo2>:
> b80: b8 dc 00 00 00 mov $0xdc,%eax
> b85: c3 ret
>
> The sizes are the same, so unless it makes a difference with another
> version of gcc then this is just a cosmetic change.
Not at all, it's good programming practice. If you use the sizeof(*p)
notation then the code work right, first time, every time, *even if you
change the type of the pointer*. Without that you have to search all the
code following the pointer declaration for a use of the type where the
pointer should have been dereferenced.
Now scale that to the case where you make a similar change in a macro or
typedef. Now you have to search the whole kernel (or NIC drivers, or ...).
Doing it right assures a change in one place will not break things at
random places all of the source tree.
That used to be part of Intro To Programming Computers...
--
bill davidsen <[email protected]>
CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.
Brian Gerst wrote:
>
> Andrew Morton wrote:
> > In 2.5.7 there is a thinko in the allocation and initialisation
> > of the fs-private superblock for ext2. It's passing the wrong type
> > to the sizeof operator (which of course gives the wrong size)
> > when allocating and clearing the memory.
> >
> > Lesson for the day: this is one of the reasons why this idiom:
> >
> > some_type *p;
> >
> > p = malloc(sizeof(*p));
> > ...
> > memset(p, 0, sizeof(*p));
> >
> > is preferable to
> >
> > some_type *p;
> >
> > p = malloc(sizeof(some_type));
> > ...
> > memset(p, 0, sizeof(some_type));
> >
> > I checked the other filesystems. They're OK (but idiomatically
> > impure). I've added a couple of defensive memsets where
> > they were missing.
>
> I'm not sure I follow you here. Compiling this code (gcc 2.96):
>
> int foo1(void) { return sizeof(struct ext2_sb_info); }
> int foo2(struct ext2_sb_info *sbi) { return sizeof(*sbi); }
The current code is using sizeof(ext2_super_block) for
something which is of type ext2_sb_info.
> yields:
>
> 00000b70 <foo1>:
> b70: b8 dc 00 00 00 mov $0xdc,%eax
> b75: c3 ret
>
> 00000b80 <foo2>:
> b80: b8 dc 00 00 00 mov $0xdc,%eax
> b85: c3 ret
>
> The sizes are the same, so unless it makes a difference with another
> version of gcc then this is just a cosmetic change.
The stylistic change tends to provide insulation from the
above bug.
-
Alexander Viro wrote:
>
> On Wed, 27 Mar 2002, Andrew Morton wrote:
>
> > In 2.5.7 there is a thinko in the allocation and initialisation
> > of the fs-private superblock for ext2. It's passing the wrong type
> > to the sizeof operator (which of course gives the wrong size)
> > when allocating and clearing the memory.
>
> > Lesson for the day: this is one of the reasons why this idiom:
> >
> > some_type *p;
> >
> > p = malloc(sizeof(*p));
> > ...
> > memset(p, 0, sizeof(*p));
> >
> > is preferable to
> >
> > some_type *p;
> >
> > p = malloc(sizeof(some_type));
> > ...
> > memset(p, 0, sizeof(some_type));
>
> ... however, there is a lot of reasons why the former is preferable.
Yeah, a lot of newbies think that :)
> For one thing, the latter is hell on any search.
If the usage of the type is hard to search for then
then wrong identifier was chosen.
> Moreover, I would
> argue that memset() on a structure is not a good idea - better do
> the explicit initialization.
memset will run at up to twice the speed (according to
Arjan). Dunno if this includes I-cache misses - probably
not.
I'm not particularly fussed about this one, but I do prefer
the sleep-at-night safety of a blanket memset. Because
(and I think this is something on which you and I somewhat
differ) code should be written for the convenience of others,
not the original author. A nice memset will leave no doubt
in the reader's mind that all members of the structure have
been initialised.
BTW, Linus: while we're on the topic, I think we should do
this again:
--- linux-2.5.7/mm/slab.c Sat Mar 9 00:18:43 2002
+++ 25/mm/slab.c Thu Mar 28 09:42:41 2002
@@ -95,9 +95,9 @@
#define STATS 1
#define FORCED_DEBUG 1
#else
-#define DEBUG 0
-#define STATS 0
-#define FORCED_DEBUG 0
+#define DEBUG 1 /* It's a development kernel */
+#define STATS 1
+#define FORCED_DEBUG 1
#endif
/*
-
Andrew Morton wrote:
>
> Brian Gerst wrote:
> >
> > Andrew Morton wrote:
> > > In 2.5.7 there is a thinko in the allocation and initialisation
> > > of the fs-private superblock for ext2. It's passing the wrong type
> > > to the sizeof operator (which of course gives the wrong size)
> > > when allocating and clearing the memory.
> > >
> > > Lesson for the day: this is one of the reasons why this idiom:
> > >
> > > some_type *p;
> > >
> > > p = malloc(sizeof(*p));
> > > ...
> > > memset(p, 0, sizeof(*p));
> > >
> > > is preferable to
> > >
> > > some_type *p;
> > >
> > > p = malloc(sizeof(some_type));
> > > ...
> > > memset(p, 0, sizeof(some_type));
> > >
> > > I checked the other filesystems. They're OK (but idiomatically
> > > impure). I've added a couple of defensive memsets where
> > > they were missing.
> >
> > I'm not sure I follow you here. Compiling this code (gcc 2.96):
> >
> > int foo1(void) { return sizeof(struct ext2_sb_info); }
> > int foo2(struct ext2_sb_info *sbi) { return sizeof(*sbi); }
>
> The current code is using sizeof(ext2_super_block) for
> something which is of type ext2_sb_info.
>
> The stylistic change tends to provide insulation from the
> above bug.
Doh. Point taken.
--
Brian Gerst
diff -urN linux-2.5.7-dj2/fs/bfs/inode.c linux/fs/bfs/inode.c
--- linux-2.5.7-dj2/fs/bfs/inode.c Thu Mar 28 16:34:37 2002
+++ linux/fs/bfs/inode.c Thu Mar 28 16:35:43 2002
@@ -292,11 +292,11 @@
int i, imap_len;
struct bfs_sb_info * info;
- info = kmalloc(sizeof(struct bfs_super_block), GFP_KERNEL);
+ info = kmalloc(sizeof(*info), GFP_KERNEL);
if (!info)
return -ENOMEM;
s->u.generic_sbp = info;
- memset(info, 0, sizeof(struct bfs_super_block));
+ memset(info, 0, sizeof(*info));
sb_set_blocksize(s, BFS_BSIZE);
On Thu, 28 Mar 2002, Andrew Morton wrote:
> > For one thing, the latter is hell on any search.
>
> If the usage of the type is hard to search for then
> then wrong identifier was chosen.
Huh?
Search for ext2_sb_info will give you all places that refer to it.
Including a buttload of
struct ext2_sb_info *p;
Now, search for ext2_sb_info[ ]*[^ *] is much more interesting.
With explicit sizeof it is guaranteed to give you all places where
such beast is allocated or subjected to memset, etc.
In this case it's not too interesting. But try it for struct super_block
or struct dentry or struct buffer_head, etc. - anything that is widely
used.
> (and I think this is something on which you and I somewhat
> differ) code should be written for the convenience of others,
> not the original author.
Ahem. Right now I'm digging through the <expletives> known as lvm.c
trying to fix the use of kdev_t in ioctls. Believe me, I'm _not_
the original author. "Where the heck do they initialize <field>" is
the question I've to deal with all the time and I'd rather have it
(along with "where the heck do they allocate and free their monsters")
as greppable as possible.
BTW, I _really_ wonder who had audited lvm.c for inclusion - quite a
few places in there pull such lovely stunts as, say it, use of strcmp()
on a user-supplied array of characters. Whaddya mean, "what if there's
no NUL"? Sigh...
Alexander Viro wrote:
>
> On Thu, 28 Mar 2002, Andrew Morton wrote:
>
> > > For one thing, the latter is hell on any search.
> >
> > If the usage of the type is hard to search for then
> > then wrong identifier was chosen.
>
> Huh?
>
> Search for ext2_sb_info will give you all places that refer to it.
> Including a buttload of
> struct ext2_sb_info *p;
>
> Now, search for ext2_sb_info[ ]*[^ *] is much more interesting.
> With explicit sizeof it is guaranteed to give you all places where
> such beast is allocated or subjected to memset, etc.
Where "etc" is "typecast". I can't think of much else
which would be found.
So the search for ext2_sb_info found the function, and
then you're down to perfoming a forward search for "p".
Which, of course, doesn't work because "p" is an asinine
identifier. Which was my point. (Insert monthly whine
about ext2_new_block)
> ...
> BTW, I _really_ wonder who had audited lvm.c for inclusion - quite a
> few places in there pull such lovely stunts as, say it, use of strcmp()
> on a user-supplied array of characters. Whaddya mean, "what if there's
> no NUL"? Sigh...
We do not appear to have an "audit for inclusion" process.
I wish we did. If a tree owner threw a patch at me and
asked for comments I'd gladly help out that way. Jeff
did some absolutely brilliant work on the e100/e1000
drivers behind the scenes - it'd be nice to have more of that.
BTW, ext3 keeps a kdev_t on-disk for external journals. The
external journal support is experimental, added to allow people
to evaluate the usefulness of external journalling. If we
decide to retain the capability we'll be moving it to a UUID
or mount-based scheme. So if the kdev_t is being a problem,
I think we can just break it.
-
On Thu, 28 Mar 2002, Andrew Morton wrote:
> I'm not particularly fussed about this one, but I do prefer
> the sleep-at-night safety of a blanket memset. Because
> (and I think this is something on which you and I somewhat
> differ) code should be written for the convenience of others,
> not the original author. A nice memset will leave no doubt
> in the reader's mind that all members of the structure have
> been initialised.
Definitely. It's easy to forget to initialize something when you reuse a
struct.
--
bill davidsen <[email protected]>
CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.
On Mar 28, 2002 16:25 -0800, Andrew Morton wrote:
> BTW, ext3 keeps a kdev_t on-disk for external journals. The
> external journal support is experimental, added to allow people
> to evaluate the usefulness of external journalling. If we
> decide to retain the capability we'll be moving it to a UUID
> or mount-based scheme. So if the kdev_t is being a problem,
> I think we can just break it.
Actually, I believe it would be keeping a "dev_t" on the disk, so
if we are using it from within the kernel we should be using
"to_kdevt()" or whatever the function is called.
That reminds me - I _do_ have the mount-based stuff for journal
devices set up. At fs mount time, the ext3 code calls a helper
function which will locate the journal by its UUID. The only
real reason for the s_journal_dev might be for e2fsck to locate
the journal device more easily, but it is also pretty easy to
find the journal by UUID in user space.
I'm just woefully behind on getting patches out of my tree. At least
with my OLS paper I'm "forced" to finish off the ext3 online resizing
code within the next 3 months or so.
Cheers, Andreas
--
Andreas Dilger \ "If a man ate a pound of pasta and a pound of antipasto,
\ would they cancel out, leaving him still hungry?"
http://www-mddsp.enel.ucalgary.ca/People/adilger/ -- Dogbert
On Thu, Mar 28, 2002 at 04:25:51PM -0800, Andrew Morton wrote:
> BTW, ext3 keeps a kdev_t on-disk for external journals.
A kdev_t is conceptually a struct block_device *.
Points to allocated kernel space.
Writing such an animal to disk is completely pointless.
You want to apply some conversion function first,
even when that conversion function may be the identity
in the setup where a kdev_t has integral type. And make
sure that the resulting integer can hold at least 64 bits.
Andries
On Thu, 28 Mar 2002, Andrew Morton wrote:
> BTW, ext3 keeps a kdev_t on-disk for external journals. The
> external journal support is experimental, added to allow people
> to evaluate the usefulness of external journalling. If we
> decide to retain the capability we'll be moving it to a UUID
> or mount-based scheme. So if the kdev_t is being a problem,
> I think we can just break it.
If experience on JFS is any predictor, the external journal will be
quite useful as a performance issue. It can be put on a faster device to
avoid bottlenecks. With JFS I finally wound up with the journal on a
battery-backed SCSI solid state disk, and got about 30% faster completion
of my daily audit run which deleted ~1000000 (yes one million) files as
fast as it could when done.
I was also creating the same number of files over the course of a day,
which is still a respectable directory change rate!
I predict that applications which create/delete a lot of files will run
better with tuning this feature.
--
bill davidsen <[email protected]>
CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.