posix: replace nonstandard futimes call with futimens

posix: replace nonstandard futimes call with futimens

Replace a futimes call with futimens which is specified in POSIX.1-2008.

R=brettw@chromium.org,darin@chromium.org

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

[darin (slow to review), 2013-04-10 17:51:05]

https://codereview.chromium.org/13818027/diff/1/base/platform_file_posix.cc
File base/platform_file_posix.cc (right):

https://codereview.chromium.org/13818027/diff/1/base/platform_file_posix.cc#n...
base/platform_file_posix.cc:302: tv[0] = last_access_time.ToTimeVal();
nit: The code might be more readable with named variables here instead of array
slots.

timeval last_access_time_val = last_access_time.ToTimeVal();
timeval last_modified_time_val = ...

timespec ts[2];
ts[0].tv_sec = last_access_time_val.tv_sec;
ts[0].tv_nsec = last_access_time_val.tv_usec * 1000;
...

Another approach would be to create a static helper function for converting
timeval to timespec.  Or, better yet, since ToTimeVal() is actually only used by
this callsite, perhaps Time::ToTimeVal should be changed to ToTimeSpec??

(The other callsites are just unit tests that could be fixed up to use
ToTimeSpec instead of ToTimeVal.)

[darin (slow to review), 2013-04-10 17:52:26]

See usage here:
https://code.google.com/p/chromium/codesearch#search/&q=ToTimeVal&sq=package:...

[Mostyn Bramley-Moore, 2013-04-10 20:59:07]

https://codereview.chromium.org/13818027/diff/1/base/platform_file_posix.cc
File base/platform_file_posix.cc (right):

https://codereview.chromium.org/13818027/diff/1/base/platform_file_posix.cc#n...
base/platform_file_posix.cc:302: tv[0] = last_access_time.ToTimeVal();
On 2013/04/10 17:51:05, darin wrote:
> nit: The code might be more readable with named variables here instead of
array
> slots.
> 
> timeval last_access_time_val = last_access_time.ToTimeVal();
> timeval last_modified_time_val = ...
> 
> timespec ts[2];
> ts[0].tv_sec = last_access_time_val.tv_sec;
> ts[0].tv_nsec = last_access_time_val.tv_usec * 1000;
> ...
> 
> Another approach would be to create a static helper function for converting
> timeval to timespec.  Or, better yet, since ToTimeVal() is actually only used
by
> this callsite, perhaps Time::ToTimeVal should be changed to ToTimeSpec??
> 
> (The other callsites are just unit tests that could be fixed up to use
> ToTimeSpec instead of ToTimeVal.)

Done.

[darin (slow to review), 2013-04-10 23:32:47]

This looks mostly OK to me.  I guess the asymmetry between ToTimeSpec and
FromTimeVal is a little unfortunate.  It creates a conversion between timeval
and timespec in the FromTimeVal unittest :-/

Usually, brettw@ reviews changes to base/time.h, so I'm going to ask him to
review this as well.

[Mostyn Bramley-Moore, 2013-04-11 07:40:18]

On 2013/04/10 23:32:47, darin wrote:
> This looks mostly OK to me.  I guess the asymmetry between ToTimeSpec and
> FromTimeVal is a little unfortunate.  It creates a conversion between timeval
> and timespec in the FromTimeVal unittest :-/

I could add a conversion function to the unittest code, to beautify it a little,
if you like.

[brettw, 2013-04-11 18:11:53]

https://codereview.chromium.org/13818027/diff/6001/base/time.h
File base/time.h (right):

https://codereview.chromium.org/13818027/diff/6001/base/time.h#newcode297
base/time.h:297: struct timespec ToTimeSpec() const;
It seems weird to me that this isn't symmetric anymore. Does Posix say to use
TimeSpecs instead of TimeVals? Or did you change the To* function just because
ToTimeVal isn't needed any more?

[darin (slow to review), 2013-04-11 18:34:03]

I guided him here because I noted that ToTimeVal wasn't needed anymore.
 However, FromTimeVal is still needed.  I didn't realize that at the time
of my suggestion.

I'm also curious if timespec is preferred by posix over timeval.  Maybe the
use of FromTimeVal should also be changed to something more posix standard?


On Thu, Apr 11, 2013 at 11:11 AM, <brettw@chromium.org> wrote:

>
>
https://codereview.chromium.**org/13818027/diff/6001/base/**time.h<https://co...
> File base/time.h (right):
>
> https://codereview.chromium.**org/13818027/diff/6001/base/**
>
time.h#newcode297<https://codereview.chromium.org/13818027/diff/6001/base/time.h#newcode297>
> base/time.h:297: struct timespec ToTimeSpec() const;
> It seems weird to me that this isn't symmetric anymore. Does Posix say
> to use TimeSpecs instead of TimeVals? Or did you change the To* function
> just because ToTimeVal isn't needed any more?
>
>
https://codereview.chromium.**org/13818027/<https://codereview.chromium.org/1...
>

[Mostyn Bramley-Moore, 2013-04-11 19:45:01]

On 2013/04/11 18:34:03, darin wrote:
> I guided him here because I noted that ToTimeVal wasn't needed anymore.
>  However, FromTimeVal is still needed.  I didn't realize that at the time
> of my suggestion.

The only use of FromTimeVal (outside of the unittests) is in mac code for
finding process creation time, because that's the form the OSX kernel seems to
provide.

> I'm also curious if timespec is preferred by posix over timeval.  Maybe the
> use of FromTimeVal should also be changed to something more posix standard?

The timespec type comes from "POSIX.1b, Real-time extensions (IEEE Std
1003.1b-1993)" aka "POSIX.4".  It was added to support higher resolution timing.
 I think the general recommendation for choosing between timeval and timespec
is:
* If you need higher resolution than microseconds- use timespec.
* Otherwise, use whichever makes most sense based on the data source and the
library functions you want to use.

> On Thu, Apr 11, 2013 at 11:11 AM, <mailto:brettw@chromium.org> wrote:
> 
> >
> >
>
https://codereview.chromium.**org/13818027/diff/6001/base/**time.h%3Chttps://...>
> > File base/time.h (right):
> >
> > https://codereview.chromium.**org/13818027/diff/6001/base/**
> >
>
time.h#newcode297<https://codereview.chromium.org/13818027/diff/6001/base/time.h#newcode297>
> > base/time.h:297: struct timespec ToTimeSpec() const;
> > It seems weird to me that this isn't symmetric anymore. Does Posix say
> > to use TimeSpecs instead of TimeVals? Or did you change the To* function
> > just because ToTimeVal isn't needed any more?

It would be symmetric if we needed to take both formats as input and output both
formats, but that's not currently the case.  We could include all four possible
functions (FromTimeVal, FromTimeSpec, ToTimeVal, ToTimeSpec) - the code is all
very simple, but it would be dead code for the time being.

[darin (slow to review), 2013-04-11 20:29:01]

Yeah, I'm not a fan of dead-code.  This change looks good to me, but again
I want to defer to brettw :-)


On Thu, Apr 11, 2013 at 12:45 PM, <mostynb@opera.com> wrote:

> On 2013/04/11 18:34:03, darin wrote:
>
>> I guided him here because I noted that ToTimeVal wasn't needed anymore.
>>   However, FromTimeVal is still needed.  I didn't realize that at the time
>> of my suggestion.
>>
>
> The only use of FromTimeVal (outside of the unittests) is in mac code for
> finding process creation time, because that's the form the OSX kernel
> seems to
> provide.
>
>
>  I'm also curious if timespec is preferred by posix over timeval.  Maybe
>> the
>> use of FromTimeVal should also be changed to something more posix
>> standard?
>>
>
> The timespec type comes from "POSIX.1b, Real-time extensions (IEEE Std
> 1003.1b-1993)" aka "POSIX.4".  It was added to support higher resolution
> timing.
>  I think the general recommendation for choosing between timeval and
> timespec
> is:
> * If you need higher resolution than microseconds- use timespec.
> * Otherwise, use whichever makes most sense based on the data source and
> the
> library functions you want to use.
>
>  On Thu, Apr 11, 2013 at 11:11 AM, <mailto:brettw@chromium.org> wrote:
>>
>
>  >
>> >
>>
>
> https://codereview.chromium.****org/13818027/diff/6001/base/****
> time.h%3Chttps://codereview.**chromium.org/13818027/diff/**
>
6001/base/time.h<http://codereview.chromium.org/13818027/diff/6001/base/time.h>
> >
>
>> > File base/time.h (right):
>> >
>> > https://codereview.chromium.****org/13818027/diff/6001/base/**
>> >
>>
>
> time.h#newcode297<https://**codereview.chromium.org/**
>
13818027/diff/6001/base/time.**h#newcode297<https://codereview.chromium.org/13818027/diff/6001/base/time.h#newcode297>
> >
>
>  > base/time.h:297: struct timespec ToTimeSpec() const;
>> > It seems weird to me that this isn't symmetric anymore. Does Posix say
>> > to use TimeSpecs instead of TimeVals? Or did you change the To* function
>> > just because ToTimeVal isn't needed any more?
>>
>
> It would be symmetric if we needed to take both formats as input and
> output both
> formats, but that's not currently the case.  We could include all four
> possible
> functions (FromTimeVal, FromTimeSpec, ToTimeVal, ToTimeSpec) - the code is
> all
> very simple, but it would be dead code for the time being.
>
>
https://codereview.chromium.**org/13818027/<https://codereview.chromium.org/1...
>

[brettw, 2013-04-11 22:32:37]

I guess we can always add the "other" versions if we need them. LGTM

[commit-bot: I haz the power, 2013-04-11 22:34:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mostynb@opera.com/13818027/6001

[commit-bot: I haz the power, 2013-04-11 22:40:40]

Sorry for I got bad news for ya.
Compile failed with a clobber build on ios_rel_device.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Mostyn Bramley-Moore, 2013-04-11 22:51:20]

D'oh, looks like OSX and android don't have futimens (even though it's more
"standard" than futimes).  I'll poke around a bit and see if there are any other
posix alternatives on those systems.  If not, would a linux ifdef be acceptable
here?

[brettw, 2013-04-11 22:53:34]

I'm OK with an ifdef.

[darin (slow to review), 2013-04-11 22:53:51]

Yeah, this suggests that you might want to backtrack to the first version of
your patch #ifdef'd for OS_LINUX :-/

Sorry for the run about!

[Mostyn Bramley-Moore, 2013-04-13 13:24:19]

On 2013/04/11 22:53:51, darin wrote:
> Yeah, this suggests that you might want to backtrack to the first version of
> your patch #ifdef'd for OS_LINUX :-/
> 
> Sorry for the run about!

No problem.

Patchset 4 should work for at least the following cases:
linux glibc, uclibc (non-ancient versions): futimens
android/bionic: futimes
mac: futimes

I don't have easy access to *bsd or solaris to check those platforms, but I
believe they should still work.

[darin (slow to review), 2013-04-17 22:45:39]

I'm running try jobs before committing.

[Mostyn Bramley-Moore, 2013-04-18 07:06:07]

On 2013/04/17 22:45:39, darin wrote:
> I'm running try jobs before committing.

The two failures (win_rel, mac_asan) look unrelated to me.

[commit-bot: I haz the power, 2013-04-18 07:13:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mostynb@opera.com/13818027/28002

[darin (slow to review), 2013-04-18 07:19:41]

On 2013/04/18 07:06:07, Mostyn Bramley-Moore wrote:
> On 2013/04/17 22:45:39, darin wrote:
> > I'm running try jobs before committing.
> 
> The two failures (win_rel, mac_asan) look unrelated to me.

Yup

[commit-bot: I haz the power, 2013-04-18 09:21:48]

Change committed as 194862