On 2/12/20 7:26 PM, Darrick J. Wong wrote:
> On Wed, Feb 12, 2020 at 05:00:59PM -0600, Eric Sandeen wrote:
>> On 12/31/19 7:11 PM, Darrick J. Wong wrote:
>>> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>>>
>>> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
...
>>> static inline void __init
>>> xfs_check_ondisk_structs(void)
>>> {
>>> + /* make sure timestamp limits are correct */
>>> + XFS_CHECK_VALUE(XFS_INO_TIME_MIN, -2147483648LL);
>>> + XFS_CHECK_VALUE(XFS_INO_TIME_MAX, 2147483647LL);
>>
>> IMHO this really shouldn't be in a function with this name, as it's not checking
>> an ondisk struct. And I'm not really sure what it's protecting against?
>> Basically you put an integer in one #define and check it in another?
>
> Admittedly /this/ part isn't so crucial, because S32_MAX is never going
> to be redefined. However, I added this for completeness; notice that
> the patch that widens xfs_timestamp_t adds similar checks for the new
> minimum and maximum timestamp, whose values are not so straightforward.
>
> Also, I get that this isn't directly checking an ondisk structure, but
> given that we use these constants, there ought to be a check against
> incorrect computation *somewhere*. The BUILD_BUG_ON macros don't
> produce any real code (and this function is called at __init time) so
> what's the harm?
OCD? Maybe just make a xfs_check_time_vals() to go with
xfs_check_ondisk_structs() or something.
-Eric
