svn commit: r187396 - head/sys/gnu/fs/ext2fs

[Bruce Evans]

On Mon, 19 Jan 2009, Stanislav Sedov wrote:

> On Mon, 19 Jan 2009 19:48:57 +1100 (EST)
> Bruce Evans <brde at optusnet.com.au> mentioned:
>
>>> Modified: head/sys/gnu/fs/ext2fs/ext2_vfsops.c
>>> ==============================================================================
>>> --- head/sys/gnu/fs/ext2fs/ext2_vfsops.c	Sun Jan 18 14:04:56 2009	(r187395)
>>> +++ head/sys/gnu/fs/ext2fs/ext2_vfsops.c	Sun Jan 18 14:54:46 2009	(r187396)
>>> @@ -5,7 +5,7 @@
>>>  *  University of Utah, Department of Computer Science
>>>  */
>>> /*-
>>> - * Copyright (c) 1989, 1991, 1993, 1994
>>> + * Copyright (c) 1989, 1991, 1993, 1994
>>>  *	The Regents of the University of California.  All rights reserved.
>>>  *
>>>  * Redistribution and use in source and binary forms, with or without
>>> @@ -120,7 +120,7 @@ static int	compute_sb_data(struct vnode
>>> static const char *ext2_opts[] = { "from", "export", "acls", "noexec",
>>>     "noatime", "union", "suiddir", "multilabel", "nosymfollow",
>>>     "noclusterr", "noclusterw", "force", NULL };
>>> -
>>> +
>>
>> The above diff shows null changes, presumably after dropping trailing
>> whitespace in the old version.
>>
>> Oops, actually the diff is invalid since something dropped the leading
>> space in unchanged lines.
>
> Yes, seems that svn mail diffs trims the spaces out...

The original diffs seem to be correct.  Cristoph Mallon replied that he
gets good diffs in mail.  The spaces seem to be lost here before my mail
arrives.

>>> @@ -318,7 +318,7 @@ static int ext2_check_descriptors (struc
>>>         {
>>> 		/* examine next descriptor block */
>>>                 if ((i % EXT2_DESC_PER_BLOCK(sb)) == 0)
>>> -                        gdp = (struct ext2_group_desc *)
>>> +                        gdp = (struct ext2_group_desc *)
>>> 				sb->s_group_desc[desc_block++]->b_data;
>>>                 if (gdp->bg_block_bitmap < block ||
>>>                     gdp->bg_block_bitmap >= block + EXT2_BLOCKS_PER_GROUP(sb))
>>
>> Here most leading tabs are corrupt in both the new and old versions.  Please
>> fix all whitespace on a line if fixing any.
>
> This file has style different from our conventions, so I only stripped
> leading spaces and left everything else intact. Should I convert the entire
> file to style(9).?

Only lines changed for other reasons.  ext2_vnops.c is non-KNF for pure
formatting on about 20% of its lines.  Fixing tabs using |unexpand reduces
this to about 18.5%.
>
>>> @@ -398,19 +398,19 @@ static int compute_sb_data(devvp, es, fs
>>>     int logic_sb_block = 1;	/* XXX for now */
>>>
>>> #if 1
>>> -#define V(v)
>>> +#define V(v)
>>> #else
>>> #define V(v)  printf(#v"= %d\n", fs->v);
>>> #endif
>>
>> The existence of this macro is a bug, since it can only work for one type,
>> but fields of different type need to be printed.  Most fields don't have
>> type int (the first one printed is s_blocksize which (especially bogusly
>> on 64-bit machines) has type u_long, so the non-'#if 1' case would cause
>> lots of errors.

Actually, this macro can be made usable using unportable complications
like:

%%%
#include <stdint.h>

%%%
#include <stdint.h>

/*
  * Can use even more complications to support FP or to avoid upcasting.
  * The unportable __typeof() is used so that the signedness can be
  * determined at compile time.  Expressions like (((__typeof(v))0.5 > 0.3)
  * can be used to determine floating point at compile time, and
  * expressions involving (((__typeof(v))UINTMAX_MAX == UINTMAX_MAX) can
  * be used to determine if the full upcast is needed.  There should be
  * a standard flag %I that does all of this automatically.  Failing that,
  * standards permit all of this to be done automatically in the usual
  * case where a type error gives undefined behaviour.
  */
#define	V(v) __extension__ ({						\
 	if ((__typeof(v))(-1) < 0)					\
 		printf(#v"= %jd\n", (intmax_t)v);			\
 	else								\
 		printf(#v"= %ju\n", (uintmax_t)v);			\
})
%%%

> In the following commit I casted all fields to (unsigned long) explicitly,
> so printfs will work for all fields.

Better, but still technically wrong for the signed fields on all
machines and for the 64-bit fields on 32-bit machines, and potentially
technically wrong for all the typedefed fields.  I think all fields
fit in 32-bit u_ints, but most of the types in the in-core superblock
are bogusly large or bogusly signed so this is not clear.  E.g.,
s_qbmask is quad_t, but it is less than s_blocksize which is unsigned
long, and the largest block size is normally 4K so only int16_t is
needed for all of these.  The V() for s_qbmask would also be fatal if
it were compiled, since it is spelled without a 'q'.

>>> @@ -1000,7 +1000,7 @@ ext2_vget(mp, ino, flags, vpp)
>>>
>>> 	/* Read in the disk contents for the inode, copy into the inode. */
>>> #if 0
>>> -printf("ext2_vget(%d) dbn= %d ", ino, fsbtodb(fs, ino_to_fsba(fs, ino)));
>>> +printf("ext2_vget(%d) dbn= %lu ", ino, fsbtodb(fs, ino_to_fsba(fs, ino)));
>>> #endif
>>> 	if ((error = bread(ump->um_devvp, fsbtodb(fs, ino_to_fsba(fs, ino)),
>>> 	    (int)fs->s_blocksize, NOCRED, &bp)) != 0) {
>>
>> ino has type ino_t, so it cannot be printed portably without using a
>> cast.  Its actual type is uint32_t.  If this code were actually, this
>> would give the following printf format errors on various supported
>> machines:
>>
>> old: sign mismatch only, since plain int happens to have the same size as
>>       ino_t on all supported machines.  gcc doesn't even warn about this
>>       error, so this error would be non-fatal
>>
>> new: now a size mismatch on machines with 64-bit longs (amd64, ia64, sparc64
>>       at least.  gcc warns about this error, so the code is now fatally
>>       broken on many supported machines, except it is never actually used.
>
> Actually, the third argument of printf has type of unsigned long for ext2fs
> code thanks to arithmetical operations around ino_to_fsba calculations. Thus
> the old code has been broken at least for 64-bit platforms, new one should
> work everywhere (both for 32 bit an 64bit platforms).

Oops.  I looked at the 2nd arg.  I suppose ext2fs doesn't support file
systems larger than 1TB, especially under FreeBSD, so the result of
fsbtodb() fits in an int32_t and using plain int for everything would
work.  Those arithmetical calculations are very fragile.  The corresponding
ones in ffs use lots of casts and still used to get the casts wrong
so that the shift overflowed at 1TB.  In general, the disk block address is
int64_t and the correct format is %jd after casting the int64_t up to
intmax_t.

Bruce