[snapshot] only create snapshot files as last step in mksnapshot.

src/snapshot/mksnapshot.cc

 // Copyright 2006-2008 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <errno.h>
 #include <signal.h>
 #include <stdio.h>
 
 #include "include/libplatform/libplatform.h"
 #include "src/assembler.h"
 #include "src/base/platform/platform.h"
 #include "src/flags.h"
 #include "src/list.h"
 #include "src/snapshot/natives.h"
 #include "src/snapshot/partial-serializer.h"
 #include "src/snapshot/startup-serializer.h"
 
 using namespace v8;
 
 class SnapshotWriter {
  public:
-  SnapshotWriter() : fp_(NULL), startup_blob_file_(NULL) {}
+  SnapshotWriter() : snapshot_cpp_path_(NULL), snapshot_blob_path_(NULL) {}
 
-  ~SnapshotWriter() {
-    if (fp_) fclose(fp_);
-    if (startup_blob_file_) fclose(startup_blob_file_);
+  void SetSnapshotFile(const char* snapshot_cpp_file) {
+    snapshot_cpp_path_ = snapshot_cpp_file;
   }
 
-  void SetSnapshotFile(const char* snapshot_file) {
-    if (snapshot_file != NULL) fp_ = GetFileDescriptorOrDie(snapshot_file);
+  void SetStartupBlobFile(const char* snapshot_blob_file) {
+    snapshot_blob_path_ = snapshot_blob_file;
   }
 
-  void SetStartupBlobFile(const char* startup_blob_file) {
-    if (startup_blob_file != NULL)
-      startup_blob_file_ = GetFileDescriptorOrDie(startup_blob_file);
-  }
-
-  void WriteSnapshot(v8::StartupData blob) const {
+  void WriteSnapshot(v8::StartupData blob) {
     i::Vector<const i::byte> blob_vector(
         reinterpret_cast<const i::byte*>(blob.data), blob.raw_size);
     MaybeWriteSnapshotFile(blob_vector);
     MaybeWriteStartupBlob(blob_vector);
   }
 
  private:
-  void MaybeWriteStartupBlob(const i::Vector<const i::byte>& blob) const {
-    if (!startup_blob_file_) return;
+  void MaybeWriteStartupBlob(const i::Vector<const i::byte>& blob) {

[Leszek Swirski, 2017/03/22 10:58:21]

if this function fails, then we still have the snapshot cpp file -- is this ok?

[Yang, 2017/03/22 11:41:42]

That's correct. But I don't think this will happen a lot. The problem this CL is
solving is when running V8 to create a snapshot fails, and we still end up
creating these files.

[Leszek Swirski, 2017/03/22 11:45:59]

SGTM, maybe add a TODO for this rare error case since ideally we would make this
entirely atomic.

+    if (!snapshot_blob_path_) return;
 
-    size_t written = fwrite(blob.begin(), 1, blob.length(), startup_blob_file_);
+    FILE* fp = GetFileDescriptorOrDie(snapshot_blob_path_);
+    size_t written = fwrite(blob.begin(), 1, blob.length(), fp);
     if (written != static_cast<size_t>(blob.length())) {
       i::PrintF("Writing snapshot file failed.. Aborting.\n");
+      remove(snapshot_blob_path_);
       exit(1);
     }
+    fclose(fp);

[wiktorg, 2017/03/22 11:01:15]

nit but shouldn't it also go before remove @ line 47

[Yang, 2017/03/22 11:41:41]

Done.

   }
 
-  void MaybeWriteSnapshotFile(const i::Vector<const i::byte>& blob) const {
-    if (!fp_) return;
+  void MaybeWriteSnapshotFile(const i::Vector<const i::byte>& blob) {
+    if (!snapshot_cpp_path_) return;
 
-    WriteFilePrefix();
-    WriteData(blob);
-    WriteFileSuffix();
+    FILE* fp = GetFileDescriptorOrDie(snapshot_cpp_path_);
+
+    WriteFilePrefix(fp);
+    WriteData(fp, blob);
+    WriteFileSuffix(fp);
+
+    fclose(fp);
   }
 
-  void WriteFilePrefix() const {

[Leszek Swirski, 2017/03/22 10:58:21]

why not const? Also, could these be static now?

[Yang, 2017/03/22 11:41:42]

I just didn't see much use in const here. Made them static instead.

-    fprintf(fp_, "// Autogenerated snapshot file. Do not edit.\n\n");
-    fprintf(fp_, "#include \"src/v8.h\"\n");
-    fprintf(fp_, "#include \"src/base/platform/platform.h\"\n\n");
-    fprintf(fp_, "#include \"src/snapshot/snapshot.h\"\n\n");
-    fprintf(fp_, "namespace v8 {\n");
-    fprintf(fp_, "namespace internal {\n\n");
+  void WriteFilePrefix(FILE* fp) {
+    fprintf(fp, "// Autogenerated snapshot file. Do not edit.\n\n");
+    fprintf(fp, "#include \"src/v8.h\"\n");
+    fprintf(fp, "#include \"src/base/platform/platform.h\"\n\n");
+    fprintf(fp, "#include \"src/snapshot/snapshot.h\"\n\n");
+    fprintf(fp, "namespace v8 {\n");
+    fprintf(fp, "namespace internal {\n\n");
   }
 
-  void WriteFileSuffix() const {
-    fprintf(fp_, "const v8::StartupData* Snapshot::DefaultSnapshotBlob() {\n");
-    fprintf(fp_, "  return &blob;\n");
-    fprintf(fp_, "}\n\n");
-    fprintf(fp_, "}  // namespace internal\n");
-    fprintf(fp_, "}  // namespace v8\n");
+  void WriteFileSuffix(FILE* fp) {
+    fprintf(fp, "const v8::StartupData* Snapshot::DefaultSnapshotBlob() {\n");
+    fprintf(fp, "  return &blob;\n");
+    fprintf(fp, "}\n\n");
+    fprintf(fp, "}  // namespace internal\n");
+    fprintf(fp, "}  // namespace v8\n");
   }
 
-  void WriteData(const i::Vector<const i::byte>& blob) const {
-    fprintf(fp_, "static const byte blob_data[] = {\n");
-    WriteSnapshotData(blob);
-    fprintf(fp_, "};\n");
-    fprintf(fp_, "static const int blob_size = %d;\n", blob.length());
-    fprintf(fp_, "static const v8::StartupData blob =\n");
-    fprintf(fp_, "{ (const char*) blob_data, blob_size };\n");
+  void WriteData(FILE* fp, const i::Vector<const i::byte>& blob) {
+    fprintf(fp, "static const byte blob_data[] = {\n");
+    WriteSnapshotData(fp, blob);
+    fprintf(fp, "};\n");
+    fprintf(fp, "static const int blob_size = %d;\n", blob.length());
+    fprintf(fp, "static const v8::StartupData blob =\n");
+    fprintf(fp, "{ (const char*) blob_data, blob_size };\n");
   }
 
-  void WriteSnapshotData(const i::Vector<const i::byte>& blob) const {
+  void WriteSnapshotData(FILE* fp, const i::Vector<const i::byte>& blob) {
     for (int i = 0; i < blob.length(); i++) {
-      if ((i & 0x1f) == 0x1f) fprintf(fp_, "\n");
-      if (i > 0) fprintf(fp_, ",");
-      fprintf(fp_, "%u", static_cast<unsigned char>(blob.at(i)));
+      if ((i & 0x1f) == 0x1f) fprintf(fp, "\n");
+      if (i > 0) fprintf(fp, ",");
+      fprintf(fp, "%u", static_cast<unsigned char>(blob.at(i)));
     }
-    fprintf(fp_, "\n");
+    fprintf(fp, "\n");
   }
 
   FILE* GetFileDescriptorOrDie(const char* filename) {
     FILE* fp = base::OS::FOpen(filename, "wb");
     if (fp == NULL) {
       i::PrintF("Unable to open file \"%s\" for writing.\n", filename);
       exit(1);
     }
     return fp;
   }
 
-  FILE* fp_;
-  FILE* startup_blob_file_;
+  const char* snapshot_cpp_path_;
+  const char* snapshot_blob_path_;
 };
 
 char* GetExtraCode(char* filename, const char* description) {
   if (filename == NULL || strlen(filename) == 0) return NULL;
   ::printf("Loading script for %s: %s\n", description, filename);
   FILE* file = base::OS::FOpen(filename, "rb");
   if (file == NULL) {
     fprintf(stderr, "Failed to open '%s': errno %d\n", filename, errno);
     exit(1);
   }
@@ skipping 55 matching lines @@
     CHECK(blob.data);
     writer.WriteSnapshot(blob);
     delete[] blob.data;
   }
 
   V8::Dispose();
   V8::ShutdownPlatform();
   delete platform;
   return 0;
 }