xfs
[Top] [All Lists]

Re: [PATCH 4/9] xfs: Use defines for CRC offsets in all cases

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 4/9] xfs: Use defines for CRC offsets in all cases
From: Jeff Liu <jeff.liu@xxxxxxxxxx>
Date: Thu, 20 Feb 2014 17:33:28 +0800
Cc: Eric Sandeen <sandeen@xxxxxxxxxx>, xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20140220002713.GI4916@dastard>
References: <1392767549-25574-1-git-send-email-sandeen@xxxxxxxxxx> <1392767549-25574-5-git-send-email-sandeen@xxxxxxxxxx> <530463C6.6050509@xxxxxxxxxx> <20140220002713.GI4916@dastard>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.1.0
On 02/20 2014 08:27 AM, Dave Chinner wrote:
> On Wed, Feb 19, 2014 at 03:56:54PM +0800, Jeff Liu wrote:
>> Hi Eric,
>>
>> I read the previous comments from Dave about using defines for CRC offsets,
>> and with a grep search after applying this patch, looks there have another
>> two places maybe we should switch them to the macros as well:
>>
>> fs/xfs/xfs_log.c:
>> Do we need a log record crc offset macros for offsetof(struct 
>> xlog_rec_header, h_crc))?
>>
>> xfs_dinode.h:
>> we added the XFS_DINODE_CRC_OFF, just use it at below routine?
>>
>> static inline uint xfs_dinode_size(int version)
>> {
>>         if (version == 3)
>>                 return sizeof(struct xfs_dinode);
>>         return offsetof(struct xfs_dinode, di_crc);
>> }
> 
> No, that's a different case - it's not being used for determining
> the offset of a CRC varaible - it's being used to calculate the size
> of the version 2 inode core. Hence it should remain open coded like
> because it has a different purpose in life....

Thanks for the clarification, so we don't need that for the second inode
case, but the first case is used to determine the log record crc offset
to generate the crc for record header, shouldn't we make it consistent
with others?


Thanks,
-Jeff

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