Upstreaming diffs in os_compat_android.cc

Upstreaming diffs in os_compat_android.cc

The original change description is:
Add mkdtemp() implementation.
The Android C library doesn't expose mkdtemp() through the NDK
(even though it implements it internally starting with ICS).
To avoid any problem in the future and keep everything strictly
compliant, provide our own implementation of the function.

BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146813

[digit1, 2012-07-05 09:11:08]

lgtm

[Philippe, 2012-07-05 09:30:15]

lgtm

[felipeg, 2012-07-10 10:31:06]

Ping

[felipeg, 2012-07-10 15:51:06]

On 2012/07/10 10:31:06, felipeg1 wrote:
> Ping

FYI , I need brettw 's approval in base/

[brettw, 2012-07-10 20:56:04]

https://chromiumcodereview.appspot.com/10696114/diff/1/base/os_compat_android.cc
File base/os_compat_android.cc (right):

https://chromiumcodereview.appspot.com/10696114/diff/1/base/os_compat_android...
base/os_compat_android.cc:72: {
Style: Check placement of { throughout this patch

https://chromiumcodereview.appspot.com/10696114/diff/1/base/os_compat_android...
base/os_compat_android.cc:118: *dirsep = '\0'; // Terminating directory path
temporarily
Style: two spaces before end-of-line comments. Check also below.

https://chromiumcodereview.appspot.com/10696114/diff/1/base/os_compat_android...
base/os_compat_android.cc:125: }
Don't use {} here since you don't elsewhere.

https://chromiumcodereview.appspot.com/10696114/diff/1/base/os_compat_android...
base/os_compat_android.cc:135: // pid string. This matches the BSD
implementation.
I wonder if all of this is necessary. The man page says the last 6 chars must be
XXXXXX. Would it be sufficient to just do:

  std::string rand_part = IntToString(RandInt(0, 1000000));

and then copy that over the 6 XXXXXX at the end and save a lot of subtle code.
We can verify beforehand that there are 6 X's and return early to make the later
logic easier.

https://chromiumcodereview.appspot.com/10696114/diff/1/base/os_compat_android...
base/os_compat_android.cc:153: path[pos--] = (char)ch;
Don't use C casts, use static_cast.

https://chromiumcodereview.appspot.com/10696114/diff/1/base/os_compat_android...
base/os_compat_android.cc:160: if (mkdir(path, 0700) == 0) {
I'd skip the blank line above here.

[felipeg, 2012-07-11 15:26:24]

https://chromiumcodereview.appspot.com/10696114/diff/1/base/os_compat_android.cc
File base/os_compat_android.cc (right):

https://chromiumcodereview.appspot.com/10696114/diff/1/base/os_compat_android...
base/os_compat_android.cc:72: {
On 2012/07/10 20:56:04, brettw wrote:
> Style: Check placement of { throughout this patch

Done.

https://chromiumcodereview.appspot.com/10696114/diff/1/base/os_compat_android...
base/os_compat_android.cc:118: *dirsep = '\0'; // Terminating directory path
temporarily
On 2012/07/10 20:56:04, brettw wrote:
> Style: two spaces before end-of-line comments. Check also below.

Done.

https://chromiumcodereview.appspot.com/10696114/diff/1/base/os_compat_android...
base/os_compat_android.cc:125: }
On 2012/07/10 20:56:04, brettw wrote:
> Don't use {} here since you don't elsewhere.

Done.

https://chromiumcodereview.appspot.com/10696114/diff/1/base/os_compat_android...
base/os_compat_android.cc:153: path[pos--] = (char)ch;
On 2012/07/10 20:56:04, brettw wrote:
> Don't use C casts, use static_cast.

Done.

https://chromiumcodereview.appspot.com/10696114/diff/1/base/os_compat_android...
base/os_compat_android.cc:160: if (mkdir(path, 0700) == 0) {
On 2012/07/10 20:56:04, brettw wrote:
> I'd skip the blank line above here.

Done.

[digit1, 2012-07-11 15:40:48]

https://chromiumcodereview.appspot.com/10696114/diff/1/base/os_compat_android.cc
File base/os_compat_android.cc (right):

https://chromiumcodereview.appspot.com/10696114/diff/1/base/os_compat_android...
base/os_compat_android.cc:135: // pid string. This matches the BSD
implementation.
First, for the record, this code is taken from recent version of the Android C
library (hence the C-style cast and slightly different coding style). Nothing
that can't be fixed though.

On 2012/07/10 20:56:04, brettw wrote:
> I wonder if all of this is necessary. The man page says the last 6 chars must
be
> XXXXXX. Would it be sufficient to just do:
> 
>   std::string rand_part = IntToString(RandInt(0, 1000000));
> 
> and then copy that over the 6 XXXXXX at the end and save a lot of subtle code.
> We can verify beforehand that there are 6 X's and return early to make the
later
> logic easier.

Given that there can be a lot more than six Xs in the template string, and all
of them must be replaced, this won't be enough. It's true that writing the tid
value at the end is not necessary, but it's generally helpful when debugging the
origin of stale temporary files.

Besides, using std::string() and calling a base/ function from what is a
C-library-layer function seems a bit overkill.

[brettw, 2012-07-11 16:21:42]

On 2012/07/11 15:40:48, digit1 wrote:
> Given that there can be a lot more than six Xs in the template string, and all
> of them must be replaced, this won't be enough. It's true that writing the tid
> value at the end is not necessary, but it's generally helpful when debugging
the
> origin of stale temporary files.

Both the OpenGroup and the Linux man page say specifically 6 X's. Are these
sources incorrect?


> Besides, using std::string() and calling a base/ function from what is a
> C-library-layer function seems a bit overkill.

Overkill in what way? On the other side, I thought that writing 70 lines of
kind-of-complicated custom C code for something rarely used and easily emulated
in a simpler way seemed like overkill.

[digit1, 2012-07-11 17:18:48]

> 
> Both the OpenGroup and the Linux man page say specifically 6 X's. Are these
> sources incorrect?
>
Oh. My source is the FreeBSD implementation of this function which indeed can
take more than 6 XXXXs in the input. The FreeBSD manpage for mkdtemp is a bit
ambiguous sincce it talks about "some number of Xs", so I guess this is a
BSD-specific extension.

In this case, it's true that your proposed solution should be adequate if we
know that the string of Xs must be exactly 6 characters long.
 
> 
> Overkill in what way?

The fact that unbounded std::string operations can result in dynamic allocation
+ extra memory copies. That's less of a problem if we know that there are 6 Xs,
since the small-string optimization will avoid most of this.

 On the other side, I thought that writing 70 lines of
> kind-of-complicated custom C code for something rarely used and easily
emulated
> in a simpler way seemed like overkill.

Agreed, it's probably better to rewrite this as a simple loop that tries a new
random rewrite with a bounded number of tries.

[felipeg, 2012-07-12 14:32:27]

Hey guys.
I changed it in a way to make the code easier to read. I hope it is better now.
Please take a look again and tell me if I should make it even simpler.

Cheers,
felipeg


On 2012/07/11 17:18:48, digit1 wrote:
> > 
> > Both the OpenGroup and the Linux man page say specifically 6 X's. Are these
> > sources incorrect?
> >
> Oh. My source is the FreeBSD implementation of this function which indeed can
> take more than 6 XXXXs in the input. The FreeBSD manpage for mkdtemp is a bit
> ambiguous sincce it talks about "some number of Xs", so I guess this is a
> BSD-specific extension.
> 
> In this case, it's true that your proposed solution should be adequate if we
> know that the string of Xs must be exactly 6 characters long.
>  
> > 
> > Overkill in what way?
> 
> The fact that unbounded std::string operations can result in dynamic
allocation
> + extra memory copies. That's less of a problem if we know that there are 6
Xs,
> since the small-string optimization will avoid most of this.
> 
>  On the other side, I thought that writing 70 lines of
> > kind-of-complicated custom C code for something rarely used and easily
> emulated
> > in a simpler way seemed like overkill.
> 
> Agreed, it's probably better to rewrite this as a simple loop that tries a new
> random rewrite with a bounded number of tries.

[digit1, 2012-07-12 15:19:50]

Thank you felipe, lgtm.

[brettw, 2012-07-12 18:22:34]

LGTM, this does seem nicer, thanks.

https://chromiumcodereview.appspot.com/10696114/diff/13001/base/os_compat_and...
File base/os_compat_android.cc (right):

https://chromiumcodereview.appspot.com/10696114/diff/13001/base/os_compat_and...
base/os_compat_android.cc:132: for (int pos = 0; pos  < kSuffixLen; ++pos) {
Style nit: you got 2 spaces.

[felipeg, 2012-07-13 10:17:47]

Thanks.
I added a unittest , which is disabled, since it may be cause of flakyness
because it actually creates a dir.
When changing that function, one might manually run the unittest.

https://chromiumcodereview.appspot.com/10696114/diff/13001/base/os_compat_and...
File base/os_compat_android.cc (right):

https://chromiumcodereview.appspot.com/10696114/diff/13001/base/os_compat_and...
base/os_compat_android.cc:132: for (int pos = 0; pos  < kSuffixLen; ++pos) {
On 2012/07/12 18:22:34, brettw wrote:
> Style nit: you got 2 spaces.

Done.

[digit1, 2012-07-13 10:34:04]

https://chromiumcodereview.appspot.com/10696114/diff/1003/base/os_compat_andr...
File base/os_compat_android_unittest.cc (right):

https://chromiumcodereview.appspot.com/10696114/diff/1003/base/os_compat_andr...
base/os_compat_android_unittest.cc:20: char invalid_path[] = "/tmp/foobarXX";
the reason this test doesn't work is because there is no /tmp on Android. If you
run the test through "adb shell" you could use /data/local/tmp/ instead.

However, we're moving towards APK-based unit tests, which means you should use
the apk's private directory for this, since /data/local/tmp is not accessible
from regular applications.

Unfortunately, I'm not sure there is an easy way currently to retrieve this
path.

[felipeg, 2012-07-13 10:39:50]

On 2012/07/13 10:34:04, digit1 wrote:
>
https://chromiumcodereview.appspot.com/10696114/diff/1003/base/os_compat_andr...
> File base/os_compat_android_unittest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10696114/diff/1003/base/os_compat_andr...
> base/os_compat_android_unittest.cc:20: char invalid_path[] = "/tmp/foobarXX";
> the reason this test doesn't work is because there is no /tmp on Android. If
you
> run the test through "adb shell" you could use /data/local/tmp/ instead.
> 
> However, we're moving towards APK-based unit tests, which means you should use
> the apk's private directory for this, since /data/local/tmp is not accessible
> from regular applications.
> 
> Unfortunately, I'm not sure there is an easy way currently to retrieve this
> path.

Good point.
The solution is to call file_util::GetTempDir() which should return the
accessible temp dir.
done.

[digit1, 2012-07-13 12:38:56]

This seems fine. Thanks.

[commit-bot: I haz the power, 2012-07-16 14:53:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/felipeg@chromium.org/10696114/1004

[commit-bot: I haz the power, 2012-07-16 16:12:17]

Change committed as 146813