On Wed, Feb 12, 2020 at 07:50:02PM -0600, Eric Sandeen wrote:
> 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.
Done.
--D
> -Eric
