xfs
[Top] [All Lists]

Re: [PATCH 1/2 V2] xfs_logprint: Handle multiply-logged inode fields

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/2 V2] xfs_logprint: Handle multiply-logged inode fields
From: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Fri, 02 Nov 2012 09:39:53 -0500
Cc: Eric Sandeen <sandeen@xxxxxxxxxx>, xfs-oss <xfs@xxxxxxxxxxx>
In-reply-to: <20121102130132.GA12578@xxxxxxxxxxxxx>
References: <5092A1DE.10609@xxxxxxxxxx> <5092A2B6.2000907@xxxxxxxxxx> <5092A46A.8080909@xxxxxxxxxxx> <20121102130132.GA12578@xxxxxxxxxxxxx>
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:16.0) Gecko/20121026 Thunderbird/16.0.2
On 11/2/12 8:01 AM, Christoph Hellwig wrote:
>> I've tested this by a simple test such as creating one
>> file on an selinux box, so that data+attr is set, and
>> logprinting; I've also tested by running logprint after
>> subsequent xfstest runs (although we hit other bugs that
>> way).
> 
> Can you add this test to xfstests, please?

Yeah that should be easy enough.

>> +
>> +    if (f->ilf_fields & (XFS_ILOG_DEV | XFS_ILOG_UUID)) {
>> +    switch (f->ilf_fields & (XFS_ILOG_DEV | XFS_ILOG_UUID)) {
>> +    case XFS_ILOG_DEV:
>> +        printf(_("DEV inode: no extra region\n"));
> 
> The if here looks odd, I think you should follow the style with
> a switch on a masked value as it's done in xlog_recover_inode_pass2()
> in the kernel.

Hm TBH I'm not sure why I left that if in there.
For the DFORK/AFORK case I think the if made sense, but not for
the DEV/UUID case I think.  I'll take another look.

> I also reall hate the indentation in this function, can you thrown in
> a preparatory patch to change it to the normal one?

to 8-char tabs?  Ok

-Eric

> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
> 

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