Android: Provide futimes() implementation using utimensat()

Android: Provide futimes() implementation using utimensat()

The earlier implementation using /proc/self/fd/<num> failed when called from a non-main thread in production builds of Chrome and Android.

Depends on NDK r8e: https://codereview.chromium.org/14860010/

BUG=236403
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=204633

[nilesh, 2013-06-04 23:13:09]

jar/digit: Please take a look.

[digit, 2013-06-05 08:00:38]

Excellent, lgtm :-)

[nilesh, 2013-06-05 22:36:51]

Adding Mark for base/OWNERS approval.

[Mark Mentovai, 2013-06-06 00:50:59]

https://codereview.chromium.org/14649009/diff/18001/base/os_compat_android.cc
File base/os_compat_android.cc (right):

https://codereview.chromium.org/14649009/diff/18001/base/os_compat_android.cc...
base/os_compat_android.cc:22: return syscall(__NR_utimensat, fd, NULL, tv, 0);
utimensat can be used to implement futimens. The time argument is a timespec[2].
A timespec specifies seconds and nanoseconds.

futimes’ time argument is a timeval[2]. A timeval specifies seconds and
microseconds.

timeval and timespec are both the same width and have the same alignment, so you
may not have noticed the problem unless you actually looked at the fractional
second portion of the times set on files, but your implementation confuses
nanoseconds and microseconds.

[nilesh, 2013-06-06 01:57:35]

https://codereview.chromium.org/14649009/diff/18001/base/os_compat_android.cc
File base/os_compat_android.cc (right):

https://codereview.chromium.org/14649009/diff/18001/base/os_compat_android.cc...
base/os_compat_android.cc:22: return syscall(__NR_utimensat, fd, NULL, tv, 0);
On 2013/06/06 00:50:59, Mark Mentovai wrote:
> utimensat can be used to implement futimens. The time argument is a
timespec[2].
> A timespec specifies seconds and nanoseconds.
> 
> futimes’ time argument is a timeval[2]. A timeval specifies seconds and
> microseconds.
> 
> timeval and timespec are both the same width and have the same alignment, so
you
> may not have noticed the problem unless you actually looked at the fractional
> second portion of the times set on files, but your implementation confuses
> nanoseconds and microseconds.

Thanks for catching this. I am now converting timeval to timespec.

I tried adding a test for this, but looks like the fractional value is ignored
on most filesystems. PlatformFile.TouchGetInfoPlatformFile says that "ext2/ext3
and HPS/HPS+ seem to have a timestamp granularity of 1s." and hence only checks
for the seconds value in tests.

Please take another look.

[Mark Mentovai, 2013-06-06 13:37:44]

https://codereview.chromium.org/14649009/diff/29001/base/os_compat_android.cc
File base/os_compat_android.cc (right):

https://codereview.chromium.org/14649009/diff/29001/base/os_compat_android.cc...
base/os_compat_android.cc:23: // timeval.usec is guaranteed to be less than one
million.
Guaranteed by whom? Any old user code might call this function and supply bogus
data.

For a faithful reimplementation of futimes, you should check the range, 0 <=
tv_usec < 1000000 as glibc does, and set errno to EINVAL and return -1 if not in
range. Why? Because there’s no guarantee that user code actually limits tv_usec
to the right range, and if a value comes in that’s in the wrong range, you need
to catch it before integer multiplication might transform it into a tv_nsec
value that actually is in range.

https://codereview.chromium.org/14649009/diff/29001/base/os_compat_android.cc...
base/os_compat_android.cc:25: ts[0].tv_sec = tv[0].tv_sec;
tv is allowed to be NULL, in which case you would pass NULL to utimensat in
place of ts. You should handle this to implement futimes fully.

https://codereview.chromium.org/14649009/diff/29001/base/os_compat_android.cc...
base/os_compat_android.cc:26: ts[0].tv_nsec = tv[0].tv_usec * 1000;
Normally you could use TIMEVAL_TO_TIMESPEC(&tv, &ts); but I guess Bionic doesn’t
have that either.

[Mark Mentovai, 2013-06-06 13:41:05]

nilesh wrote:
> I tried adding a test for this, but looks like the fractional value is ignored
> on most filesystems. PlatformFile.TouchGetInfoPlatformFile says that
"ext2/ext3
> and HPS/HPS+ seem to have a timestamp granularity of 1s." and hence only
checks
> for the seconds value in tests.

ext4 supports fractional timestamps. I just checked tmpfs on Linux 3.8.11 and it
does too. I don’t know if either of these are viable filesystems to use for
tests. The test would obviously have to be conditional on filesystem type and
we’d have to have some kind of whitelist. Sounds like a lot of trouble.

[nilesh, 2013-06-06 19:03:08]

https://codereview.chromium.org/14649009/diff/29001/base/os_compat_android.cc
File base/os_compat_android.cc (right):

https://codereview.chromium.org/14649009/diff/29001/base/os_compat_android.cc...
base/os_compat_android.cc:23: // timeval.usec is guaranteed to be less than one
million.
On 2013/06/06 13:37:44, Mark Mentovai wrote:
> Guaranteed by whom? Any old user code might call this function and supply
bogus
> data.
> 
> For a faithful reimplementation of futimes, you should check the range, 0 <=
> tv_usec < 1000000 as glibc does, and set errno to EINVAL and return -1 if not
in
> range. Why? Because there’s no guarantee that user code actually limits
tv_usec
> to the right range, and if a value comes in that’s in the wrong range, you
need
> to catch it before integer multiplication might transform it into a tv_nsec
> value that actually is in range.

Thanks. Done.

https://codereview.chromium.org/14649009/diff/29001/base/os_compat_android.cc...
base/os_compat_android.cc:25: ts[0].tv_sec = tv[0].tv_sec;
On 2013/06/06 13:37:44, Mark Mentovai wrote:
> tv is allowed to be NULL, in which case you would pass NULL to utimensat in
> place of ts. You should handle this to implement futimes fully.

Done.

https://codereview.chromium.org/14649009/diff/29001/base/os_compat_android.cc...
base/os_compat_android.cc:26: ts[0].tv_nsec = tv[0].tv_usec * 1000;
On 2013/06/06 13:37:44, Mark Mentovai wrote:
> Normally you could use TIMEVAL_TO_TIMESPEC(&tv, &ts); but I guess Bionic
doesn’t
> have that either.

You are right. TIMEVAL_TO_TIMESPEC is not avialable in the ndk.

[Mark Mentovai, 2013-06-06 19:14:19]

Excellent. LGTM.

[commit-bot: I haz the power, 2013-06-06 19:49:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nileshagrawal@chromium.org/14649009/34001

[commit-bot: I haz the power, 2013-06-06 22:42:48]

Change committed as 204633