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

src/snapshot/mksnapshot.cc

Index: src/snapshot/mksnapshot.cc
diff --git a/src/snapshot/mksnapshot.cc b/src/snapshot/mksnapshot.cc
index f4362e5077e1231e2e57ccaf06ccc9769ebaae61..24a01207eb2e6a530dd3873f76037e50df459672 100644
--- a/src/snapshot/mksnapshot.cc
+++ b/src/snapshot/mksnapshot.cc
@@ -19,23 +19,17 @@ 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);
@@ -43,57 +37,64 @@ class SnapshotWriter {
   }
 
  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;
+
+    FILE* fp = GetFileDescriptorOrDie(snapshot_cpp_path_);
+
+    WriteFilePrefix(fp);
+    WriteData(fp, blob);
+    WriteFileSuffix(fp);
 
-    WriteFilePrefix();
-    WriteData(blob);
-    WriteFileSuffix();
+    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) {
@@ -105,8 +106,8 @@ class SnapshotWriter {
     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) {