Cherrypick 229035: Fix error checking in get_temp_file_name().

Cherrypick 229035: Fix error checking in get_temp_file_name().

Checking errno without first checking that the call failed means that
if some other call prior to mkstemp failed with EINVAL prior to this,
the assert would fire even if mkstemp succeeded. If something failed
with EEXIST, it would go in to an infinite loop.

Change-Id: I3f140a3e15fe08664a38a8c9a950c4ed547eb481

git-svn-id: https://llvm.org/svn/llvm-project/libcxx/trunk@229035 91177308-0d34-0410-b5e6-96231b3b80d8

BUG=none -- infinite looping bot
R=dschuff@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-libcxx.git;a=commit;h=640116b36573795793453c2b0671b161deb9256f

[jvoung (off chromium), 2015-04-15 22:27:43]

jvoung@chromium.org changed reviewers:
+ dschuff@chromium.org

[Derek Schuff, 2015-04-15 22:34:47]

LGTM
interesting, so does sbc's addition of O_EXCL to open change the behavior of
mkstemp?

[Derek Schuff, 2015-04-15 22:34:55]

thanks for chasing this down btw

[Roland McGrath, 2015-04-15 22:35:21]

mcgrathr@chromium.org changed reviewers:
+ mcgrathr@chromium.org

[Roland McGrath, 2015-04-15 22:35:22]

https://codereview.chromium.org/1092513002/diff/1/test/support/platform_suppo...
File test/support/platform_support.h (right):

https://codereview.chromium.org/1092513002/diff/1/test/support/platform_suppo...
test/support/platform_support.h:67: if (FD == -1 && errno == EINVAL) {
This makes an assert into something not affected by NDEBUG.
I think 'if (fd == -1 && errno == EINVAL) assert("explanation");' would be about
right.

https://codereview.chromium.org/1092513002/diff/1/test/support/platform_suppo...
test/support/platform_support.h:71: } while (FD == -1);
do ... while (FD == -1 && errno != EEXIST);
seems to have been the original intent.

But there is no explanation about why there is a loop here at all, so some
comments are warranted.

EEXIST is the only "normal" failure mode for mkstemp, and it indicates that you
must give up now because all candidate file names are exhausted.  Other errors
mean that open failed with some other error, and repeating mkstemp might well
just repeat that other error.

Using mkstemp in a loop doesn't make sense to me.

[Sam Clegg, 2015-04-15 22:40:09]

sbc@chromium.org changed reviewers:
+ sbc@chromium.org

[Sam Clegg, 2015-04-15 22:40:10]

https://codereview.chromium.org/1092513002/diff/1/test/support/platform_suppo...
File test/support/platform_support.h (right):

https://codereview.chromium.org/1092513002/diff/1/test/support/platform_suppo...
test/support/platform_support.h:71: } while (FD == -1);
On 2015/04/15 22:35:22, Roland McGrath wrote:
> do ... while (FD == -1 && errno != EEXIST);
> seems to have been the original intent.
> 
> But there is no explanation about why there is a loop here at all, so some
> comments are warranted.
> 
> EEXIST is the only "normal" failure mode for mkstemp, and it indicates that
you
> must give up now because all candidate file names are exhausted.  Other errors
> mean that open failed with some other error, and repeating mkstemp might well
> just repeat that other error.
> 
> Using mkstemp in a loop doesn't make sense to me.

Yes, we couldn't figure out why they would want a loop here at all.  But this is
upstream's fix for the issue.. and it does technically work.

Presumably the while loop in the win32 section above is also pointless (or
perhaps dangerous).

Simply "if (fd == -1) then ... abort" would make the most sense to me.  There is
no sensible recovery from mkstemp failure, right?

[jvoung (off chromium), 2015-04-15 22:45:33]

For the record -- previously about four tests would fail because it ended up
with the same filename when it actually wanted two separate temp files instead.

So after this we'll need to update the known failures also.

https://codereview.chromium.org/1092513002/diff/1/test/support/platform_suppo...
File test/support/platform_support.h (right):

https://codereview.chromium.org/1092513002/diff/1/test/support/platform_suppo...
test/support/platform_support.h:67: if (FD == -1 && errno == EINVAL) {
On 2015/04/15 22:35:22, Roland McGrath wrote:
> This makes an assert into something not affected by NDEBUG.
> I think 'if (fd == -1 && errno == EINVAL) assert("explanation");' would be
about
> right.

I think the way it is now without the assert is okay/better. This is support
code purely for tests, so it shouldn't have mattered if there was NDEBUG or not
when building the test files.

[jvoung (off chromium), 2015-04-15 23:04:27]

Committed patchset #1 (id:1) manually as
640116b36573795793453c2b0671b161deb9256f (tree was closed).