xfs
[Top] [All Lists]

Re: [PATCH 0/2] xfs: pass mp to XFS_WANT_CORRUPTED_*

To: Brian Foster <bfoster@xxxxxxxxxx>, Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 0/2] xfs: pass mp to XFS_WANT_CORRUPTED_*
From: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Mon, 09 Feb 2015 10:42:43 -0600
Cc: Eric Sandeen <sandeen@xxxxxxxxxx>, xfs-oss <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20150209130926.GA18336@xxxxxxxxxxxxxx>
References: <54D53E8C.8070207@xxxxxxxxxx> <20150208213502.GA4251@dastard> <20150209130926.GA18336@xxxxxxxxxxxxxx>
On 2/9/15 7:09 AM, 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.
> 
> Unfortunately, nothing shorter than the current naming immediately comes
> to mind... :/ We could kill the XFS_ prefix I suppose or even invert the
> logic of the calls, but that's certainly a more significant change.
> Thoughts?

Right, so today it's XFS_WANT_CORRUPTED_RETURN(thing_that_should_be_true)
and I agree, that's always felt a bit odd.

Dave suggests XFS_CORRUPT_RETURN(thing_that_should_be_true)

I guess the "WANT" was supposed to imply that the argument is the
test that we "want" to be true? :)

I'm not super excited about inverting every test, but we could ...

XFS_CORRUPT_RETURN_IF_NOT(test) would be explicit, at least.
Or XFS_CORRUPT_RETURN_UNLESS(test).

I can't think of a nice short name that conveys more meaning, either,
but I'm not really sure that it's critical to change it at this point.

-Eric

<Prev in Thread] Current Thread [Next in Thread>