Significantly cleans up the ImageWriter Operation class and subclasses.

chrome/browser/extensions/api/image_writer_private/operation_linux.cc

 // Copyright 2013 The Chromium 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 "base/file_util.h"
 #include "base/files/file_enumerator.h"
 #include "base/threading/worker_pool.h"
 #include "chrome/browser/extensions/api/image_writer_private/error_messages.h"
 #include "chrome/browser/extensions/api/image_writer_private/operation.h"
 #include "chrome/browser/extensions/api/image_writer_private/operation_manager.h"
 #include "content/public/browser/browser_thread.h"
 #include "third_party/zlib/google/zip.h"
 
 namespace extensions {
 namespace image_writer {
 
 using content::BrowserThread;
 
 const int kBurningBlockSize = 8 * 1024;  // 8 KiB
 
-void Operation::WriteStart() {
+bool Operation::OpenPlatformFiles() {
+  base::PlatformFileError result;
+
+  if (source_ == base::kInvalidPlatformFileValue) {
+    source_ = base::CreatePlatformFile(
+        image_path_,
+        base::PLATFORM_FILE_OPEN | base::PLATFORM_FILE_READ,
+        NULL,
+        &result);
+
+    if (result != base::PLATFORM_FILE_OK) {
+      DLOG(ERROR) << "Failed to open source(" << result
+                  << "): " << image_path_.value();
+      Error(error::kImageOpenError);
+      return false;
+    }
+
+    source_closer_.reset(&source_);
+  }
+
+  if (target_ == base::kInvalidPlatformFileValue) {
+    target_ = base::CreatePlatformFile(device_path_,
+                                       base::PLATFORM_FILE_OPEN |
+                                           base::PLATFORM_FILE_WRITE |
+                                           base::PLATFORM_FILE_READ,
+                                       NULL,
+                                       &result);
+
+    if (result != base::PLATFORM_FILE_OK) {
+      DLOG(ERROR) << "Failed to open target(" << result
+                  << "): " << device_path_.value();
+      Error(error::kDeviceOpenError);
+      return false;
+    }
+
+    target_closer_.reset(&target_);
+  }
+
+  return true;
+}
+
+void Operation::Write(const base::Closure& continuation) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   if (IsCancelled()) {
     return;
   }
 
-  if (image_path_.empty()) {
-    Error(error::kImageNotFound);
-    return;
-  }
-
-  DVLOG(1) << "Starting write of " << image_path_.value()
-           << " to " << storage_unit_id_;
-
   SetStage(image_writer_api::STAGE_WRITE);
 
   // TODO (haven): Unmount partitions before writing. http://crbug.com/284834
 
-  scoped_ptr<image_writer_utils::ImageReader> reader(
-      new image_writer_utils::ImageReader());
-  scoped_ptr<image_writer_utils::ImageWriter> writer(
-      new image_writer_utils::ImageWriter());
-  base::FilePath storage_path(storage_unit_id_);
+  if (!OpenPlatformFiles())
+    return;
 
-  if (reader->Open(image_path_)) {
-    if (!writer->Open(storage_path)) {
-      reader->Close();
-      Error(error::kDeviceOpenError);
-      return;
-    }
-  } else {
-    Error(error::kImageOpenError);
+  int64 total_size;
+  base::GetFileSize(image_path_, &total_size);
+
+  BrowserThread::PostTask(
+      BrowserThread::FILE,
+      FROM_HERE,
+      base::Bind(&Operation::WriteChunk, this, 0, total_size, continuation));
+}
+
+void Operation::WriteChunk(const int64& bytes_written,
+                           const int64& total_size,
+                           const base::Closure& continuation) {
+  if (IsCancelled()) {
     return;
   }
 
-  BrowserThread::PostTask(
-    BrowserThread::FILE,
-    FROM_HERE,
-    base::Bind(&Operation::WriteChunk,
-               this,
-               base::Passed(&reader),
-               base::Passed(&writer),
-               0));
-}
-
-void Operation::WriteChunk(
-    scoped_ptr<image_writer_utils::ImageReader> reader,
-    scoped_ptr<image_writer_utils::ImageWriter> writer,
-    int64 bytes_written) {
-  if (IsCancelled()) {
-    WriteCleanUp(reader.Pass(), writer.Pass());
-    return;
-  }
-
-  char buffer[kBurningBlockSize];
-  int64 image_size = reader->GetSize();
-  int len = reader->Read(buffer, kBurningBlockSize);
+  scoped_ptr<char[]> buffer(new char[kBurningBlockSize]);
+  int64 len = base::ReadPlatformFile(
+      source_, bytes_written, buffer.get(), kBurningBlockSize);
 
   if (len > 0) {
-    if (writer->Write(buffer, len) == len) {
-      int percent_prev = kProgressComplete * bytes_written / image_size;
-      int percent_curr = kProgressComplete * (bytes_written + len) / image_size;
+    if (base::WritePlatformFile(target_, bytes_written, buffer.get(), len) ==
+        len) {
+      int percent_curr = kProgressComplete * (bytes_written + len) / total_size;
 
-      if (percent_curr > percent_prev) {
-        SetProgress(percent_curr);
-      }
+      SetProgress(percent_curr);
 
-      BrowserThread::PostTask(
-        BrowserThread::FILE,
-        FROM_HERE,
-        base::Bind(&Operation::WriteChunk,
-                   this,
-                   base::Passed(&reader),
-                   base::Passed(&writer),
-                   bytes_written + len));
+      BrowserThread::PostTask(BrowserThread::FILE,
+                              FROM_HERE,
+                              base::Bind(&Operation::WriteChunk,
+                                         this,
+                                         bytes_written + len,
+                                         total_size,
+                                         continuation));
     } else {
-      WriteCleanUp(reader.Pass(), writer.Pass());
       Error(error::kDeviceWriteError);
     }
   } else if (len == 0) {
-    if (bytes_written == image_size) {
-      if (WriteCleanUp(reader.Pass(), writer.Pass())) {
-        BrowserThread::PostTask(
-          BrowserThread::FILE,
-          FROM_HERE,
-          base::Bind(&Operation::WriteComplete,
-                     this));
-      }
-    } else {
-      WriteCleanUp(reader.Pass(), writer.Pass());
-      Error(error::kImageReadError);
-    }
+    WriteComplete(continuation);
   } else { // len < 0
-    WriteCleanUp(reader.Pass(), writer.Pass());
     Error(error::kImageReadError);
   }
 }
 
-bool Operation::WriteCleanUp(
-    scoped_ptr<image_writer_utils::ImageReader> reader,
-    scoped_ptr<image_writer_utils::ImageWriter> writer) {
-
-  bool success = true;
-  if (!reader->Close()) {
-    Error(error::kImageCloseError);
-    success = false;
-  }
-
-  if (!writer->Close()) {
-    Error(error::kDeviceCloseError);
-    success = false;
-  }
-  return success;
+void Operation::WriteComplete(const base::Closure& continuation) {
+  SetProgress(kProgressComplete);
+  continuation.Run();
 }
 
-void Operation::WriteComplete() {
-
-  DVLOG(2) << "Completed write of " << image_path_.value();
-  SetProgress(kProgressComplete);
-
-  if (verify_write_) {
-    BrowserThread::PostTask(BrowserThread::FILE,
-                            FROM_HERE,
-                            base::Bind(&Operation::VerifyWriteStart, this));
-  } else {
-    BrowserThread::PostTask(BrowserThread::FILE,
-                            FROM_HERE,
-                            base::Bind(&Operation::Finish, this));
-  }
-}
-
-void Operation::VerifyWriteStart() {
+void Operation::VerifyWrite(const base::Closure& continuation) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   if (IsCancelled()) {
     return;
   }
 
-  DVLOG(1) << "Starting verification stage.";
-
   SetStage(image_writer_api::STAGE_VERIFYWRITE);
 
-  scoped_ptr<base::FilePath> image_path(new base::FilePath(image_path_));
+  if (!OpenPlatformFiles())

[tbarzic, 2014/02/13 06:58:52]

Is this needed?
I think you should either reset files after write is done (and remove source_ ==
base::kInvalidPlatformFileValue  from OpenPlatformFile) or expect the files to
be opened here (and possibly fail the operation if they are not)

[Drew Haven, 2014/02/13 20:48:10]

So this was to save us closing and reopening the files when we go from the write
to the verify phases.  This way they only get opened once and get closed by the
scoped closer however the operation terminates.  It also shares the code between
the write and verify functions. Granted, I'm not very experienced with using a
scoped closer.

The operation will be "failed" if the files fail to open because
OpenPlatformFiles actually throws the error, but that's not clear the way this
is written.

I'll split the two functions up and it's probably best to close and flush the
files anyway.  See what you think of this new change.

+    return;
 
-  GetMD5SumOfFile(
-      image_path.Pass(),
-      -1,
-      0, // progress_offset
-      50, // progress_scale
-      base::Bind(&Operation::VerifyWriteStage2,
-                 this));
+  int64 total_size;
+  base::GetFileSize(image_path_, &total_size);
+
+  BrowserThread::PostTask(
+      BrowserThread::FILE,
+      FROM_HERE,
+      base::Bind(
+          &Operation::VerifyWriteChunk, this, 0, total_size, continuation));
 }
 
-void Operation::VerifyWriteStage2(
-    scoped_ptr<std::string> image_hash) {
-  DVLOG(1) << "Building MD5 sum of device: " << storage_unit_id_;
-
-  int64 image_size;
-  scoped_ptr<base::FilePath> device_path(new base::FilePath(storage_unit_id_));
-
-  if (!base::GetFileSize(image_path_, &image_size)){
-    Error(error::kImageSizeError);
+void Operation::VerifyWriteChunk(const int64& bytes_processed,
+                                 const int64& total_size,
+                                 const base::Closure& continuation) {
+  if (IsCancelled()) {
     return;
   }
 
-  GetMD5SumOfFile(
-      device_path.Pass(),
-      image_size,
-      50, // progress_offset
-      50, // progress_scale
-      base::Bind(&Operation::VerifyWriteCompare,
-                 this,
-                 base::Passed(&image_hash)));
+  scoped_ptr<char[]> source_buffer(new char[kBurningBlockSize]);
+  scoped_ptr<char[]> target_buffer(new char[kBurningBlockSize]);
+
+  int64 source_bytes_read = base::ReadPlatformFile(
+      source_, bytes_processed, source_buffer.get(), kBurningBlockSize);
+
+  if (source_bytes_read > 0) {
+    int64 target_bytes_read = base::ReadPlatformFile(
+        target_, bytes_processed, target_buffer.get(), source_bytes_read);
+    if (source_bytes_read == target_bytes_read &&
+        memcmp(source_buffer.get(), target_buffer.get(), source_bytes_read) ==
+            0) {
+      int percent_curr = kProgressComplete *
+                         (bytes_processed + source_bytes_read) / total_size;
+
+      SetProgress(percent_curr);
+
+      BrowserThread::PostTask(BrowserThread::FILE,
+                              FROM_HERE,
+                              base::Bind(&Operation::VerifyWriteChunk,
+                                         this,
+                                         bytes_processed + source_bytes_read,
+                                         total_size,
+                                         continuation));
+    } else {
+      Error(error::kVerificationFailed);
+    }
+  } else if (source_bytes_read == 0) {

[tbarzic, 2014/02/13 06:58:52]

what if target file is not completely read at this point (e.g. if it somehow
ended up being larger)?

[Drew Haven, 2014/02/13 20:48:10]

So, the target file should be larger because it's the file representing the
entire disk, so we only want to verify up to the point where the source ends.

+    VerifyWriteComplete(continuation);
+  } else {  // len < 0
+    Error(error::kImageReadError);
+  }
 }
 
-void Operation::VerifyWriteCompare(
-    scoped_ptr<std::string> image_hash,
-    scoped_ptr<std::string> device_hash) {
-  DVLOG(1) << "Comparing hashes: " << *image_hash << " vs " << *device_hash;
-
-  if (*image_hash != *device_hash) {
-    Error(error::kVerificationFailed);
-    return;
-  }
-
-  DVLOG(2) << "Completed write verification of " << image_path_.value();
-
+void Operation::VerifyWriteComplete(const base::Closure& continuation) {
   SetProgress(kProgressComplete);
-
-  Finish();
+  continuation.Run();
 }
 
 }  // namespace image_writer
 }  // namespace extensions