Base: Make file_util and file use the same code to translate from stat to File::Info

Base: Make file_util and file use the same code to translate from stat to File::Info

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

[rvargas (doing something else), 2014-03-19 02:58:18]

I have a question here: the behavior for OS_NACL differs between both files...
so which one should be?

[rvargas (doing something else), 2014-03-20 21:02:29]

-noelallen
+nfullagar

[rvargas (doing something else), 2014-03-24 21:35:05]

ping

[Roland McGrath, 2014-03-25 20:24:51]

The question wasn't very specific, so I'll answer as if the only issue you're
asking about is stat vs stat64 wrt st_size.
In all the NaCl C libraries, 'struct stat' and *stat (not *stat64) calls work
and st_size is always 64 bits.
This is consistent with OSX, for example.

[rvargas (doing something else), 2014-03-25 21:43:33]

On 2014/03/25 20:24:51, Roland McGrath wrote:
> The question wasn't very specific, so I'll answer as if the only issue you're
> asking about is stat vs stat64 wrt st_size.
> In all the NaCl C libraries, 'struct stat' and *stat (not *stat64) calls work
> and st_size is always 64 bits.
> This is consistent with OSX, for example.

The specific issue is that today the behavior is inconsistent because
file_util_posix.cc has:
#if defined(OS_BSD) || defined(OS_MACOSX)
  typedef struct stat stat_wrapper_t;
#else
  typedef struct stat64 stat_wrapper_t;

while file_posix.cc has
#if defined(OS_BSD) || defined(OS_MACOSX) || defined(OS_NACL)
  typedef struct stat stat_wrapper_t;
#else
  typedef struct stat64 stat_wrapper_t;

What I'm doing is placing OS_NACL always on the first group... and from your
answer it seems like that's what you want.

[rvargas (doing something else), 2014-03-25 21:45:01]

Brett, do you mind to review this?

[rvargas (doing something else), 2014-03-28 19:15:27]

On 2014/03/25 21:45:01, rvargas wrote:
> Brett, do you mind to review this?

ping

[nfullagar1, 2014-03-28 19:40:15]

https://codereview.chromium.org/203873002/diff/130001/base/files/file_posix.cc
File base/files/file_posix.cc (right):

https://codereview.chromium.org/203873002/diff/130001/base/files/file_posix.c...
base/files/file_posix.cc:147: // TODO(gavinp): Investigate a good high
resolution option for OS_NACL.
couple of nits
-  #else is for default, but comment is specifically for OS_NACL
-  why not use nsec on NaCl?  Not clear to me why setting to 0 would be better. 
(Roland is better than I to answer the details on what resolution NaCl should
return for nsec, but won't be back until Monday)

[rvargas (doing something else), 2014-03-28 19:53:31]

https://codereview.chromium.org/203873002/diff/130001/base/files/file_posix.cc
File base/files/file_posix.cc (right):

https://codereview.chromium.org/203873002/diff/130001/base/files/file_posix.c...
base/files/file_posix.cc:147: // TODO(gavinp): Investigate a good high
resolution option for OS_NACL.
On 2014/03/28 19:40:15, nfullagar1 wrote:
> couple of nits
> -  #else is for default, but comment is specifically for OS_NACL
> -  why not use nsec on NaCl?  Not clear to me why setting to 0 would be
better. 
> (Roland is better than I to answer the details on what resolution NaCl should
> return for nsec, but won't be back until Monday)

Sure. I was basically just moving this code :)
Even if we have a section for OS_NACL we'd need a default one (looking like
this?). I'm happy to add one if you guys tell me exactly what to do (I'm really
clueless).

[nfullagar1, 2014-03-31 19:59:06]

https://codereview.chromium.org/203873002/diff/130001/base/files/file_posix.cc
File base/files/file_posix.cc (right):

https://codereview.chromium.org/203873002/diff/130001/base/files/file_posix.c...
base/files/file_posix.cc:147: // TODO(gavinp): Investigate a good high
resolution option for OS_NACL.
Talking to Roland NaCl should be zero'ing the nsec fields anyway, so the default
could copy the nsec fields from stat_info as-is instead of resetting them here. 
Suggest also looping in gavinp why nsec fields were zero'd here in the first
place.

On 2014/03/28 19:53:31, rvargas wrote:
> On 2014/03/28 19:40:15, nfullagar1 wrote:
> > couple of nits
> > -  #else is for default, but comment is specifically for OS_NACL
> > -  why not use nsec on NaCl?  Not clear to me why setting to 0 would be
> better. 
> > (Roland is better than I to answer the details on what resolution NaCl
should
> > return for nsec, but won't be back until Monday)
> 
> Sure. I was basically just moving this code :)
> Even if we have a section for OS_NACL we'd need a default one (looking like
> this?). I'm happy to add one if you guys tell me exactly what to do (I'm
really
> clueless).

[rvargas (doing something else), 2014-03-31 23:02:27]

https://codereview.chromium.org/203873002/diff/130001/base/files/file_posix.cc
File base/files/file_posix.cc (right):

https://codereview.chromium.org/203873002/diff/130001/base/files/file_posix.c...
base/files/file_posix.cc:147: // TODO(gavinp): Investigate a good high
resolution option for OS_NACL.
On 2014/03/31 19:59:07, nfullagar1 wrote:
> Talking to Roland NaCl should be zero'ing the nsec fields anyway, so the
default
> could copy the nsec fields from stat_info as-is instead of resetting them
here. 
> Suggest also looping in gavinp why nsec fields were zero'd here in the first
> place.

Given that a default option is needed even if we add an explicit OS_NACL
section, and that this code works as is for NaCl (regardless of overwriting 0
with 0), I'm hesitant to change the behavior for any OS not explicitly covered.

What I'm reading is that there is no need to do anything else for NaCl, so I'm
removing the TODO and leaving the rest of the code as is.


> 
> On 2014/03/28 19:53:31, rvargas wrote:
> > On 2014/03/28 19:40:15, nfullagar1 wrote:
> > > couple of nits
> > > -  #else is for default, but comment is specifically for OS_NACL
> > > -  why not use nsec on NaCl?  Not clear to me why setting to 0 would be
> > better. 
> > > (Roland is better than I to answer the details on what resolution NaCl
> should
> > > return for nsec, but won't be back until Monday)
> > 
> > Sure. I was basically just moving this code :)
> > Even if we have a section for OS_NACL we'd need a default one (looking like
> > this?). I'm happy to add one if you guys tell me exactly what to do (I'm
> really
> > clueless).
>

[brettw, 2014-04-04 21:39:29]

lgtm

[rvargas (doing something else), 2014-04-04 22:54:09]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2014-04-04 22:54:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rvargas@chromium.org/203873002/150001

[commit-bot: I haz the power, 2014-04-05 02:39:24]

Change committed as 261956