Cherrypick 229035: Fix error checking in get_temp_file_name().

test/support/platform_support.h

Index: test/support/platform_support.h
diff --git a/test/support/platform_support.h b/test/support/platform_support.h
index 09bb29a65e4e9f9ef0f36a411417ec90afe7c0a2..f1f31339c99c6745c30bed4c80210bd927eb7f3b 100644
--- a/test/support/platform_support.h
+++ b/test/support/platform_support.h
@@ -62,10 +62,13 @@ get_temp_file_name()
     std::string Name;
     int FD = -1;
     do {
-      Name = "libcxx.XXXXXX";
-      FD = mkstemp(&Name[0]);
-      assert(errno != EINVAL && "Something is wrong with the mkstemp's argument");
-    } while (FD == -1 || errno == EEXIST);
+        Name = "libcxx.XXXXXX";
+        FD = mkstemp(&Name[0]);
+        if (FD == -1 && errno == EINVAL) {

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

This makes an assert into something not affected by NDEBUG.
I think 'if (fd == -1 && errno == EINVAL) assert("explanation");' would be about
right.

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

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.

+            perror("mkstemp");
+            abort();
+        }
+    } while (FD == -1);

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

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:10]

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?

     close(FD);
     return Name;
 #endif