From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay.sgi.com (relay3.corp.sgi.com [198.149.34.15]) by oss.sgi.com (Postfix) with ESMTP id 6B0997FB6 for ; Mon, 9 Feb 2015 15:58:31 -0600 (CST) Received: from cuda.sgi.com (cuda1.sgi.com [192.48.157.11]) by relay3.corp.sgi.com (Postfix) with ESMTP id 05483AC007 for ; Mon, 9 Feb 2015 13:58:30 -0800 (PST) Received: from ipmail06.adl6.internode.on.net (ipmail06.adl6.internode.on.net [150.101.137.145]) by cuda.sgi.com with ESMTP id w1u8XdGuS1Cwcz6T for ; Mon, 09 Feb 2015 13:58:28 -0800 (PST) Date: Tue, 10 Feb 2015 08:58:27 +1100 From: Dave Chinner Subject: Re: [PATCH 0/2] xfs: pass mp to XFS_WANT_CORRUPTED_* Message-ID: <20150209215827.GW12722@dastard> References: <54D53E8C.8070207@redhat.com> <20150208213502.GA4251@dastard> <20150209130926.GA18336@laptop.bfoster> <20150209211744.GT12722@dastard> <20150209214359.GN18336@laptop.bfoster> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20150209214359.GN18336@laptop.bfoster> List-Id: XFS Filesystem from SGI List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: xfs-bounces@oss.sgi.com Sender: xfs-bounces@oss.sgi.com To: Brian Foster Cc: Eric Sandeen , xfs-oss On Mon, Feb 09, 2015 at 04:43:59PM -0500, Brian Foster wrote: > On Tue, Feb 10, 2015 at 08:17:44AM +1100, Dave Chinner wrote: > > On Mon, Feb 09, 2015 at 08:09:26AM -0500, Brian Foster wrote: > > > On Mon, Feb 09, 2015 at 08:35:02AM +1100, Dave Chinner wrote: > > > > On Fri, Feb 06, 2015 at 04:22:04PM -0600, Eric Sandeen wrote: > > > > > These 2 patches provide information about which filesystem > > > > > hit the error... > > > > > > > > If we are going to touch every one of these macros, then can we > > > > rename them to something a little shorter like XFS_CORRUPT_GOTO() > > > > and XFS_CORRUPT_RETURN() at the same time? That will make the code a > > > > little less eye-bleedy where there are lots of these statements, > > > > and make formatting of complex checks a bit easier, too... > > > > > > > > > > XFS_CORRUPT_DOSOMETHING() jumps out to me as indicate corruption if the > > > logic statement evaluates as true rather than false. The latter (e.g., > > > assert-like logic) is how they work today, so that could be a bit > > > confusing to somebody who isn't already familiar with how these macros > > > work. > > > > Someone not familiar with XFS conventions is already going to get > > caught by "should be true" logic of these statements anyway as the > > logic is the opposite of BUG_ON() and WARN_ON(). i.e. BUG_ON(1) > > will kill the kernel, while ASSERT(1) indicates everything is fine. > > > > BUG_ON() and ASSERT() are self-explanatory, the latter being a pretty > standard/common thing ('man assert'). As Eric mentioned, the WANT bit of > the macro is what suggests assert-like semantics. > > > I suggested shortening the macro because it makes the code that uses > > it extensively shouty and hard to read because it splits logic > > statements across lines regularly (e.g __xfs_dir3_data_check). I > > want to use this more extensively in verifiers to give better > > corruption detection reporting, but the current macro will make the > > verifier code rather ugly. Hence my suggestion to make it shorter, > > neater and a little less shouty... > > > > Sure, but ASSERT_CORRUPT_RET() is the same length as the example above. > ASSERT_CORRUPT_GOTO() is only a few chars longer than the associated > example. We could still use WANT over ASSERT I suppose to shorten it up > further. Either of those are at least still self-explanatory in my > opinion. Thinking on it a bit further, the XFS_WANT_CORRUPTED macros have an internal ASSERT in them, so they are effectively an ASSERT statement. I could live with those names, especially as ASSERT is something that can be compiled into production kernels via CONFIG_XFS_WARN=y to turn them into error messages... Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs