Date: Mon, 19 Jan 2009 12:31:38 +0300 From: Stanislav Sedov <stas@FreeBSD.org> To: Bruce Evans <brde@optusnet.com.au> Cc: Stanislav Sedov <stas@freebsd.org>, svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r187396 - head/sys/gnu/fs/ext2fs Message-ID: <20090119123138.f5404c60.stas@FreeBSD.org> In-Reply-To: <20090119192954.V37158@delplex.bde.org> References: <200901181454.n0IEskw4077045@svn.freebsd.org> <20090119192954.V37158@delplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On Mon, 19 Jan 2009 19:48:57 +1100 (EST)
Bruce Evans <brde@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...
> > @@ -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).?
> > @@ -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.
>
In the following commit I casted all fields to (unsigned long) explicitly,
so printfs will work for all fields.
> > @@ -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).
Thanks for comments!
- --
Stanislav Sedov
ST4096-RIPE
-----BEGIN PGP SIGNATURE-----
iEYEARECAAYFAkl0SH4ACgkQK/VZk+smlYE7CQCfYtheq6nRYDh/3DWjDJ41hcem
yVMAnjN3sKe8UacMMvH6e1wrR/C/DKES
=84cR
-----END PGP SIGNATURE-----
!DSPAM:4974487d967003034114393!
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090119123138.f5404c60.stas>