From owner-svn-src-all@FreeBSD.ORG Mon Jan 19 14:42:03 2009 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 0F6C91065674; Mon, 19 Jan 2009 14:42:03 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail06.syd.optusnet.com.au (mail06.syd.optusnet.com.au [211.29.132.187]) by mx1.freebsd.org (Postfix) with ESMTP id 882E68FC13; Mon, 19 Jan 2009 14:42:02 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c122-107-120-227.carlnfd1.nsw.optusnet.com.au (c122-107-120-227.carlnfd1.nsw.optusnet.com.au [122.107.120.227]) by mail06.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id n0JEfwOb025014 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 20 Jan 2009 01:41:59 +1100 Date: Tue, 20 Jan 2009 01:41:58 +1100 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: Stanislav Sedov In-Reply-To: <20090119123138.f5404c60.stas@FreeBSD.org> Message-ID: <20090120002911.A37309@delplex.bde.org> References: <200901181454.n0IEskw4077045@svn.freebsd.org> <20090119192954.V37158@delplex.bde.org> <20090119123138.f5404c60.stas@FreeBSD.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Bruce Evans Subject: Re: svn commit: r187396 - head/sys/gnu/fs/ext2fs X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 19 Jan 2009 14:42:04 -0000 On Mon, 19 Jan 2009, Stanislav Sedov wrote: > On Mon, 19 Jan 2009 19:48:57 +1100 (EST) > Bruce Evans 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 %%% #include /* * 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