Add DestroyDiskImage to crostini_manager and concierge_client

Add a dbus client method for the DestroyDiskImage Concierge service. Add a method to CorstiniManager to check the input. This is not used yet. Will be used in my next CL which adds a button in the Settings page to delete Crostini VMs.

Bug: 833125

Change-Id: I6dd817a23764b484f4ac507adb0566ccbfeac5df
Reviewed-on: https://chromium-review.googlesource.com/1013657Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Reviewed-by: Nicholas Verne <nverne@chromium.org>
Commit-Queue: Renée Wright <rjwright@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551244}

chrome/browser/chromeos/crostini/crostini_manager.cc

@@ -78,7 +78,7 @@ void CrostiniManager::CreateDiskImage(
     return;
   }
 
-  std::string disk_path_string = disk_path.MaybeAsASCII();
+  std::string disk_path_string = disk_path.AsUTF8Unsafe();
   if (disk_path_string.empty()) {
     LOG(ERROR) << "Disk path cannot be empty";
     std::move(callback).Run(ConciergeClientResult::CLIENT_ERROR,
@@ -134,6 +134,43 @@ void CrostiniManager::CreateDiskImageAfterSizeCheck(
                      weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
 }
 
+void CrostiniManager::DestroyDiskImage(
+    const std::string& cryptohome_id,
+    const base::FilePath& disk_path,
+    vm_tools::concierge::StorageLocation storage_location,
+    DestroyDiskImageCallback callback) {
+  if (cryptohome_id.empty()) {
+    LOG(ERROR) << "Cryptohome id cannot be empty";
+    std::move(callback).Run(ConciergeClientResult::CLIENT_ERROR);
+    return;
+  }
+
+  std::string disk_path_string = disk_path.AsUTF8Unsafe();
+  if (disk_path_string.empty()) {
+    LOG(ERROR) << "Disk path cannot be empty";
+    std::move(callback).Run(ConciergeClientResult::CLIENT_ERROR);
+    return;
+  }
+
+  vm_tools::concierge::DestroyDiskImageRequest request;
+  request.set_cryptohome_id(std::move(cryptohome_id));
+  request.set_disk_path(std::move(disk_path_string));
+
+  if (storage_location != vm_tools::concierge::STORAGE_CRYPTOHOME_ROOT &&
+      storage_location != vm_tools::concierge::STORAGE_CRYPTOHOME_DOWNLOADS) {
+    LOG(ERROR) << "'" << storage_location
+               << "' is not a valid storage location";
+    std::move(callback).Run(ConciergeClientResult::CLIENT_ERROR);
+    return;
+  }
+  request.set_storage_location(storage_location);
+
+  GetConciergeClient()->DestroyDiskImage(
+      std::move(request),
+      base::BindOnce(&CrostiniManager::OnDestroyDiskImage,
+                     weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+}
+
 void CrostiniManager::StartTerminaVm(std::string name,
                                      const base::FilePath& disk_path,
                                      StartTerminaVmCallback callback) {
@@ -143,7 +180,7 @@ void CrostiniManager::StartTerminaVm(std::string name,
     return;
   }
 
-  std::string disk_path_string = disk_path.MaybeAsASCII();
+  std::string disk_path_string = disk_path.AsUTF8Unsafe();
   if (disk_path_string.empty()) {
     LOG(ERROR) << "Disk path cannot be empty";
     std::move(callback).Run(ConciergeClientResult::CLIENT_ERROR);
@@ -298,6 +335,27 @@ void CrostiniManager::OnCreateDiskImage(
                           base::FilePath(response.disk_path()));
 }
 
+void CrostiniManager::OnDestroyDiskImage(
+    DestroyDiskImageCallback callback,
+    base::Optional<vm_tools::concierge::DestroyDiskImageResponse> reply) {
+  if (!reply.has_value()) {
+    LOG(ERROR) << "Failed to destroy disk image. Empty response.";
+    std::move(callback).Run(ConciergeClientResult::DESTROY_DISK_IMAGE_FAILED);
+    return;
+  }
+  vm_tools::concierge::DestroyDiskImageResponse response =
+      std::move(reply).value();
+
+  if (response.status() != vm_tools::concierge::DISK_STATUS_DESTROYED &&
+      response.status() != vm_tools::concierge::DISK_STATUS_DOES_NOT_EXIST) {
+    LOG(ERROR) << "Failed to destroy disk image: " << response.failure_reason();
+    std::move(callback).Run(ConciergeClientResult::DESTROY_DISK_IMAGE_FAILED);
+    return;
+  }
+
+  std::move(callback).Run(ConciergeClientResult::SUCCESS);
+}
+
 void CrostiniManager::OnStartTerminaVm(
     StartTerminaVmCallback callback,
     base::Optional<vm_tools::concierge::StartVmResponse> reply) {

chrome/browser/chromeos/crostini/crostini_manager.h

@@ -27,6 +27,7 @@ enum class ConciergeClientResult {
   CREATE_DISK_IMAGE_FAILED,
   VM_START_FAILED,
   VM_STOP_FAILED,
+  DESTROY_DISK_IMAGE_FAILED,
   CLIENT_ERROR,
   DISK_TYPE_ERROR,
   CONTAINER_START_FAILED,
@@ -49,6 +50,9 @@ class CrostiniManager : public chromeos::ConciergeClient::Observer {
   using CreateDiskImageCallback =
       base::OnceCallback<void(ConciergeClientResult result,
                               const base::FilePath& disk_path)>;
+  // The type of the callback for CrostiniManager::DestroyDiskImage.
+  using DestroyDiskImageCallback =
+      base::OnceCallback<void(ConciergeClientResult result)>;
   // The type of the callback for CrostiniManager::StopVm.
   using StopVmCallback = base::OnceCallback<void(ConciergeClientResult result)>;
   // The type of the callback for CrostiniManager::StartContainer.
@@ -77,6 +81,20 @@ class CrostiniManager : public chromeos::ConciergeClient::Observer {
       vm_tools::concierge::StorageLocation storage_location,
       CreateDiskImageCallback callback);
 
+  // Checks the arguments for destroying a named Termina VM disk image.
+  // Removes the named Termina VM via ConciergeClient::DestroyDiskImage.
+  // |callback| is called if the arguments are bad, or after the method call
+  // finishes.
+  void DestroyDiskImage(
+      // The cryptohome id for the user's encrypted storage.
+      const std::string& cryptohome_id,
+      // The path to the disk image, including the name of
+      // the image itself.
+      const base::FilePath& disk_path,
+      // The storage location of the disk image
+      vm_tools::concierge::StorageLocation storage_location,
+      DestroyDiskImageCallback callback);
+
   // Checks the arguments for starting a Termina VM. Starts a Termina VM via
   // ConciergeClient::StartTerminaVm. |callback| is called if the arguments
   // are bad, or after the method call finishes.
@@ -136,6 +154,12 @@ class CrostiniManager : public chromeos::ConciergeClient::Observer {
       CreateDiskImageCallback callback,
       base::Optional<vm_tools::concierge::CreateDiskImageResponse> response);
 
+  // Callback for ConciergeClient::DestroyDiskImage. Called after the Concierge
+  // service method finishes.
+  void OnDestroyDiskImage(
+      DestroyDiskImageCallback callback,
+      base::Optional<vm_tools::concierge::DestroyDiskImageResponse> response);
+
   // Callback for ConciergeClient::StartTerminaVm. Called after the Concierge
   // service method finishes.
   void OnStartTerminaVm(

chrome/browser/chromeos/crostini/crostini_manager_unittest.cc

@@ -30,6 +30,14 @@ class CrostiniManagerTest : public testing::Test {
     std::move(closure).Run();
   }
 
+  void DestroyDiskImageClientErrorCallback(
+      base::OnceClosure closure,
+      crostini::ConciergeClientResult result) {
+    EXPECT_FALSE(fake_concierge_client_->destroy_disk_image_called());
+    EXPECT_EQ(result, crostini::ConciergeClientResult::CLIENT_ERROR);
+    std::move(closure).Run();
+  }
+
   void StartTerminaVmClientErrorCallback(
       base::OnceClosure closure,
       crostini::ConciergeClientResult result) {
@@ -60,6 +68,12 @@ class CrostiniManagerTest : public testing::Test {
     std::move(closure).Run();
   }
 
+  void DestroyDiskImageSuccessCallback(base::OnceClosure closure,
+                                       crostini::ConciergeClientResult result) {
+    EXPECT_TRUE(fake_concierge_client_->destroy_disk_image_called());
+    std::move(closure).Run();
+  }
+
   void StartTerminaVmSuccessCallback(base::OnceClosure closure,
                                      crostini::ConciergeClientResult result) {
     EXPECT_TRUE(fake_concierge_client_->start_termina_vm_called());
@@ -138,6 +152,49 @@ TEST_F(CrostiniManagerTest, CreateDiskImageSuccess) {
   loop.Run();
 }
 
+TEST_F(CrostiniManagerTest, DestroyDiskImageNameError) {
+  const base::FilePath& disk_path = base::FilePath("");
+  base::RunLoop loop;
+  crostini::CrostiniManager::GetInstance()->DestroyDiskImage(
+      "i_dont_know_what_cryptohome_id_is", disk_path,
+      vm_tools::concierge::STORAGE_CRYPTOHOME_ROOT,
+      base::BindOnce(&CrostiniManagerTest::DestroyDiskImageClientErrorCallback,
+                     base::Unretained(this), loop.QuitClosure()));
+  loop.Run();
+}
+
+TEST_F(CrostiniManagerTest, DestroyDiskImageCryptohomeError) {
+  const base::FilePath& disk_path = base::FilePath(kVmName);
+  base::RunLoop loop;
+  crostini::CrostiniManager::GetInstance()->DestroyDiskImage(
+      "", disk_path, vm_tools::concierge::STORAGE_CRYPTOHOME_ROOT,
+      base::BindOnce(&CrostiniManagerTest::DestroyDiskImageClientErrorCallback,
+                     base::Unretained(this), loop.QuitClosure()));
+  loop.Run();
+}
+
+TEST_F(CrostiniManagerTest, DestroyDiskImageStorageLocationError) {
+  const base::FilePath& disk_path = base::FilePath(kVmName);
+  base::RunLoop loop;
+  crostini::CrostiniManager::GetInstance()->DestroyDiskImage(
+      "i_dont_know_what_cryptohome_id_is", disk_path,
+      vm_tools::concierge::StorageLocation_INT_MIN_SENTINEL_DO_NOT_USE_,
+      base::BindOnce(&CrostiniManagerTest::DestroyDiskImageClientErrorCallback,
+                     base::Unretained(this), loop.QuitClosure()));
+  loop.Run();
+}
+
+TEST_F(CrostiniManagerTest, DestroyDiskImageSuccess) {
+  const base::FilePath& disk_path = base::FilePath(kVmName);
+  base::RunLoop loop;
+  crostini::CrostiniManager::GetInstance()->DestroyDiskImage(
+      "i_dont_know_what_cryptohome_id_is", disk_path,
+      vm_tools::concierge::STORAGE_CRYPTOHOME_DOWNLOADS,
+      base::BindOnce(&CrostiniManagerTest::DestroyDiskImageSuccessCallback,
+                     base::Unretained(this), loop.QuitClosure()));
+  loop.Run();
+}
+
 TEST_F(CrostiniManagerTest, StartTerminaVmNameError) {
   const base::FilePath& disk_path = base::FilePath(kVmName);
   base::RunLoop loop;

chromeos/dbus/concierge_client.cc

@@ -55,6 +55,27 @@ class ConciergeClientImpl : public ConciergeClient {
                        weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
   }
 
+  void DestroyDiskImage(
+      const vm_tools::concierge::DestroyDiskImageRequest& request,
+      DBusMethodCallback<vm_tools::concierge::DestroyDiskImageResponse>
+          callback) override {
+    dbus::MethodCall method_call(vm_tools::concierge::kVmConciergeInterface,
+                                 vm_tools::concierge::kDestroyDiskImageMethod);
+    dbus::MessageWriter writer(&method_call);
+
+    if (!writer.AppendProtoAsArrayOfBytes(request)) {
+      LOG(ERROR) << "Failed to encode DestroyDiskImageRequest protobuf";
+      std::move(callback).Run(base::nullopt);
+      return;
+    }
+
+    concierge_proxy_->CallMethod(
+        &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+        base::BindOnce(&ConciergeClientImpl::OnDBusProtoResponse<
+                           vm_tools::concierge::DestroyDiskImageResponse>,
+                       weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+  }
+
   void StartTerminaVm(const vm_tools::concierge::StartVmRequest& request,
                       DBusMethodCallback<vm_tools::concierge::StartVmResponse>
                           callback) override {

chromeos/dbus/concierge_client.h

@@ -45,6 +45,13 @@ class CHROMEOS_EXPORT ConciergeClient : public DBusClient {
       DBusMethodCallback<vm_tools::concierge::CreateDiskImageResponse>
           callback) = 0;
 
+  // Destroys a Termina VM and removes its disk image.
+  // |callback| is called after the method call finishes.
+  virtual void DestroyDiskImage(
+      const vm_tools::concierge::DestroyDiskImageRequest& request,
+      DBusMethodCallback<vm_tools::concierge::DestroyDiskImageResponse>
+          callback) = 0;
+
   // Starts a Termina VM if there is not alread one running.
   // |callback| is called after the method call finishes.
   virtual void StartTerminaVm(

chromeos/dbus/fake_concierge_client.cc

@@ -34,6 +34,15 @@ void FakeConciergeClient::CreateDiskImage(
       FROM_HERE, base::BindOnce(std::move(callback), base::nullopt));
 }
 
+void FakeConciergeClient::DestroyDiskImage(
+    const vm_tools::concierge::DestroyDiskImageRequest& request,
+    DBusMethodCallback<vm_tools::concierge::DestroyDiskImageResponse>
+        callback) {
+  destroy_disk_image_called_ = true;
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::BindOnce(std::move(callback), base::nullopt));
+}
+
 void FakeConciergeClient::StartTerminaVm(
     const vm_tools::concierge::StartVmRequest& request,
     DBusMethodCallback<vm_tools::concierge::StartVmResponse> callback) {

chromeos/dbus/fake_concierge_client.h

@@ -27,22 +27,30 @@ class CHROMEOS_EXPORT FakeConciergeClient : public ConciergeClient {
   bool IsContainerStartedSignalConnected() override;
 
   // Fake version of the method that creates a disk image for the Termina VM.
-  // Sets fake_create_disk_image_called. |callback| is called after the method
+  // Sets create_disk_image_called. |callback| is called after the method
   // call finishes.
   void CreateDiskImage(
       const vm_tools::concierge::CreateDiskImageRequest& request,
       DBusMethodCallback<vm_tools::concierge::CreateDiskImageResponse> callback)
       override;
 
+  // Fake version of the method that destroys a Termina VM and removes its disk
+  // image. Sets destroy_disk_image_called. |callback| is called after the
+  // method call finishes.
+  void DestroyDiskImage(
+      const vm_tools::concierge::DestroyDiskImageRequest& request,
+      DBusMethodCallback<vm_tools::concierge::DestroyDiskImageResponse>
+          callback) override;
+
   // Fake version of the method that starts a Termina VM. Sets
-  // fake_start_termina_vm_called. |callback| is called after the method call
+  // start_termina_vm_called. |callback| is called after the method call
   // finishes.
   void StartTerminaVm(const vm_tools::concierge::StartVmRequest& request,
                       DBusMethodCallback<vm_tools::concierge::StartVmResponse>
                           callback) override;
 
   // Fake version of the method that stops the named Termina VM if it is
-  // running. Sets fake_stop_vm_called. |callback| is called after the method
+  // running. Sets stop_vm_called. |callback| is called after the method
   // call finishes.
   void StopVm(const vm_tools::concierge::StopVmRequest& request,
               DBusMethodCallback<vm_tools::concierge::StopVmResponse> callback)
@@ -70,6 +78,8 @@ class CHROMEOS_EXPORT FakeConciergeClient : public ConciergeClient {
 
   // Indicates whether CreateDiskImage has been called
   bool create_disk_image_called() const { return create_disk_image_called_; }
+  // Indicates whether DestroyDiskImage has been called
+  bool destroy_disk_image_called() const { return destroy_disk_image_called_; }
   // Indicates whether StartTerminaVm has been called
   bool start_termina_vm_called() const { return start_termina_vm_called_; }
   // Indicates whether StopVm has been called
@@ -86,6 +96,7 @@ class CHROMEOS_EXPORT FakeConciergeClient : public ConciergeClient {
 
  private:
   bool create_disk_image_called_ = false;
+  bool destroy_disk_image_called_ = false;
   bool start_termina_vm_called_ = false;
   bool stop_vm_called_ = false;
   bool start_container_called_ = false;