I agree with doing the refactor, I just worry about the name. A timespec can legitimately have a valid tv_sec that is < 0 if it is representing a wall-time before the epoch (so a negative time_t value). OTOH, these checks here are for timespecs that represent an interval relative rather than an absolute wall time and so requiring tv_sec to be non-negative makes sense. I worry that timespecvalid is too generic of a name as it would fail on valid wall-time structures and that the name would tempt people to use it when they shouldn't. (And I wonder in fact if the checks in compat/linux are too strict, e.g. if it would both an attempt to set a time on a file via utimes(2) or the like to a time before Jan 1, 1970.)

I'd recommend 'valid_interval' instead of timespecvalid or something similar. All instances in this patch are for intervals (relative time) rather than an absolute time (which is just a relative time to 1970, but semantically different and the term 'abs time' or 'absolute time' is used to refer to this sort of time).

Otherwise it looks good.

Yeah, agreed, renamed to timespecvalid_interval. Thanks!

compat/linux utimes does not uses this check and allows negative sec values.

Harbormaster completed remote builds in B45143: Diff 104956.Apr 13 2022, 11:26 AM

jhb accepted this revision.Apr 14 2022, 10:40 PM

This revision is now accepted and ready to land.Apr 14 2022, 10:40 PM

Closed by commit rG91e7bdcdcf10: Add timespecvalid_interval macro and use it. (authored by dchagin). · Explain WhyApr 25 2022, 7:24 AM

This revision was automatically updated to reflect the committed changes.

dchagin added a commit: rG91e7bdcdcf10: Add timespecvalid_interval macro and use it..

dchagin added a commit: rGc5789e62ba30: Add timespecvalid_interval macro and use it..Jun 17 2022, 7:42 PM