From 281584b0709f64b5ee224f70944512753072bb3b Mon Sep 17 00:00:00 2001 From: Yo Chiang Date: Mon, 24 Aug 2020 16:50:20 +0800 Subject: Check install status in ~PartitionInstaller() ~PartitionInstaller() should check the error code returned by Finish() and clean up the installed images if any error occured. Bug: 165471299 Test: Observe logcat when DSU installation fails Change-Id: If977b4da0f37a0706494259838c84bcd2752ed94 --- partition_installer.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/partition_installer.cpp b/partition_installer.cpp index 357df50..4de181e 100644 --- a/partition_installer.cpp +++ b/partition_installer.cpp @@ -56,8 +56,9 @@ PartitionInstaller::PartitionInstaller(GsiService* service, const std::string& i } PartitionInstaller::~PartitionInstaller() { - Finish(); - if (!succeeded_) { + if (Finish() != IGsiService::INSTALL_OK || !succeeded_) { + LOG(ERROR) << "Installation failed: install_dir=" << install_dir_ + << ", dsu_slot=" << active_dsu_ << ", partition_name=" << name_; // Close open handles before we remove files. system_device_ = nullptr; PostInstallCleanup(images_.get()); -- cgit v1.2.3 From 53692200123327c6bf8165bcd98eb9fb01290db6 Mon Sep 17 00:00:00 2001 From: Yo Chiang Date: Mon, 24 Aug 2020 17:05:00 +0800 Subject: Refactor and rename Finish() to CheckInstallState() CheckInstallState() returns IGsiService::INSTALL_OK if installation completes successfully, else an error code. Remove the PartitionInstaller::succeeded_ field as checking the value of CheckInstallState() is enough. Bug: 165471299 Test: Observe logcat when DSU installation fails Change-Id: Ie4d798c963c0d537ed5ab35713e1f8faacb89af2 --- partition_installer.cpp | 16 ++++++---------- partition_installer.h | 2 -- 2 files changed, 6 insertions(+), 12 deletions(-) diff --git a/partition_installer.cpp b/partition_installer.cpp index 4de181e..d7fab11 100644 --- a/partition_installer.cpp +++ b/partition_installer.cpp @@ -56,13 +56,14 @@ PartitionInstaller::PartitionInstaller(GsiService* service, const std::string& i } PartitionInstaller::~PartitionInstaller() { - if (Finish() != IGsiService::INSTALL_OK || !succeeded_) { + if (CheckInstallState() != IGsiService::INSTALL_OK) { LOG(ERROR) << "Installation failed: install_dir=" << install_dir_ << ", dsu_slot=" << active_dsu_ << ", partition_name=" << name_; // Close open handles before we remove files. system_device_ = nullptr; PostInstallCleanup(images_.get()); } + system_device_ = nullptr; if (IsAshmemMapped()) { UnmapAshmem(); } @@ -97,7 +98,6 @@ int PartitionInstaller::StartInstall() { if (!Format()) { return IGsiService::INSTALL_ERROR_GENERIC; } - succeeded_ = true; } else { // Map ${name}_gsi so we can write to it. system_device_ = OpenPartition(GetBackingFile(name_)); @@ -309,26 +309,22 @@ bool PartitionInstaller::Format() { return true; } -int PartitionInstaller::Finish() { - if (readOnly_ && gsi_bytes_written_ != size_) { +int PartitionInstaller::CheckInstallState() { + if (readOnly_ && !IsFinishedWriting()) { // We cannot boot if the image is incomplete. LOG(ERROR) << "image incomplete; expected " << size_ << " bytes, waiting for " << (size_ - gsi_bytes_written_) << " bytes"; return IGsiService::INSTALL_ERROR_GENERIC; } - if (system_device_ != nullptr && fsync(system_device_->fd())) { - PLOG(ERROR) << "fsync failed for " << name_ << "_gsi"; + if (system_device_ != nullptr && fsync(GetPartitionFd())) { + PLOG(ERROR) << "fsync failed for " << GetBackingFile(name_); return IGsiService::INSTALL_ERROR_GENERIC; } - system_device_ = {}; - // If files moved (are no longer pinned), the metadata file will be invalid. // This check can be removed once b/133967059 is fixed. if (!images_->Validate()) { return IGsiService::INSTALL_ERROR_GENERIC; } - - succeeded_ = true; return IGsiService::INSTALL_OK; } diff --git a/partition_installer.h b/partition_installer.h index 1503648..48f916d 100644 --- a/partition_installer.h +++ b/partition_installer.h @@ -60,7 +60,6 @@ class PartitionInstaller final { const std::string& install_dir() const { return install_dir_; } private: - int Finish(); int PerformSanityChecks(); int Preallocate(); bool Format(); @@ -82,7 +81,6 @@ class PartitionInstaller final { bool readOnly_; // Remaining data we're waiting to receive for the GSI image. uint64_t gsi_bytes_written_ = 0; - bool succeeded_ = false; uint64_t ashmem_size_ = -1; void* ashmem_data_ = MAP_FAILED; -- cgit v1.2.3 From f194dcec2e4508ecd7055b087e611e14e074885a Mon Sep 17 00:00:00 2001 From: Yo Chiang Date: Mon, 24 Aug 2020 17:21:10 +0800 Subject: Refactor ~PartitionInstaller() and add FinishInstall() Distill the error checking and resource management logic in ~PartitionInstaller() and PostInstallCleanup() into a public method FinishInstall(). FinishInstall() does the opposite of StartInstall(). It checks the installation result, release device handles and cleans up any failed installation files. Then it returns an error code indicating the installation status. Bug: 165471299 Test: Observe logcat when DSU installation fails Change-Id: Iee004bc29dc875c8cf656e11fe8fd259e3003992 --- partition_installer.cpp | 35 ++++++++++++++++------------------- partition_installer.h | 13 ++++++++++--- 2 files changed, 26 insertions(+), 22 deletions(-) diff --git a/partition_installer.cpp b/partition_installer.cpp index d7fab11..35ac884 100644 --- a/partition_installer.cpp +++ b/partition_installer.cpp @@ -56,35 +56,32 @@ PartitionInstaller::PartitionInstaller(GsiService* service, const std::string& i } PartitionInstaller::~PartitionInstaller() { - if (CheckInstallState() != IGsiService::INSTALL_OK) { + if (FinishInstall() != IGsiService::INSTALL_OK) { LOG(ERROR) << "Installation failed: install_dir=" << install_dir_ << ", dsu_slot=" << active_dsu_ << ", partition_name=" << name_; - // Close open handles before we remove files. - system_device_ = nullptr; - PostInstallCleanup(images_.get()); } - system_device_ = nullptr; if (IsAshmemMapped()) { UnmapAshmem(); } } -void PartitionInstaller::PostInstallCleanup() { - auto manager = ImageManager::Open(MetadataDir(active_dsu_), install_dir_); - if (!manager) { - LOG(ERROR) << "Could not open image manager"; - return; +int PartitionInstaller::FinishInstall() { + if (finished_) { + return finished_status_; } - return PostInstallCleanup(manager.get()); -} - -void PartitionInstaller::PostInstallCleanup(ImageManager* manager) { - std::string file = GetBackingFile(name_); - if (manager->IsImageMapped(file)) { - LOG(ERROR) << "unmap " << file; - manager->UnmapImageDevice(file); + finished_ = true; + finished_status_ = CheckInstallState(); + system_device_ = nullptr; + if (finished_status_ != IGsiService::INSTALL_OK) { + auto file = GetBackingFile(name_); + LOG(ERROR) << "Installation failed, clean up: " << file; + if (images_->IsImageMapped(file)) { + LOG(ERROR) << "unmap " << file; + images_->UnmapImageDevice(file); + } + images_->DeleteBackingImage(file); } - manager->DeleteBackingImage(file); + return finished_status_; } int PartitionInstaller::StartInstall() { diff --git a/partition_installer.h b/partition_installer.h index 48f916d..920af47 100644 --- a/partition_installer.h +++ b/partition_installer.h @@ -53,9 +53,13 @@ class PartitionInstaller final { static int WipeWritable(const std::string& active_dsu, const std::string& install_dir, const std::string& name); - // Clean up install state if gsid crashed and restarted. - void PostInstallCleanup(); - void PostInstallCleanup(ImageManager* manager); + // Finish a partition installation and release resources. + // If the installation is incomplete or corrupted, the backing image would + // be cleaned up and an error code is returned. + // No method other than FinishInstall() and ~PartitionInstaller() should be + // called after calling this method. + // This method is also called by the destructor to free up resources. + int FinishInstall(); const std::string& install_dir() const { return install_dir_; } @@ -84,6 +88,9 @@ class PartitionInstaller final { uint64_t ashmem_size_ = -1; void* ashmem_data_ = MAP_FAILED; + bool finished_ = false; + int finished_status_ = 0; + std::unique_ptr system_device_; }; -- cgit v1.2.3 From 99ab518ed6b7fae95ba29b132227f993e0a099d0 Mon Sep 17 00:00:00 2001 From: Yo Chiang Date: Mon, 24 Aug 2020 17:25:26 +0800 Subject: GsiService::enableGsi() checks return code of FinishInstall() If enabling a freshly installed GSI, GsiService::enableGsi() should call FinishInstall() to check if the installation completes successfully. Bug: 165471299 Test: Observe logcat when DSU installation fails Test: DsuEndToEndTest Change-Id: I3e2832322b272506f748fdd9e1ff60c6317ef42b --- gsi_service.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/gsi_service.cpp b/gsi_service.cpp index 1bcd3dc..490655c 100644 --- a/gsi_service.cpp +++ b/gsi_service.cpp @@ -272,7 +272,13 @@ binder::Status GsiService::enableGsi(bool one_shot, const std::string& dsuSlot, } if (installer_) { ENFORCE_SYSTEM; + int status = installer_->FinishInstall(); installer_ = {}; + if (status != IGsiService::INSTALL_OK) { + *_aidl_return = status; + LOG(ERROR) << "Installation failed, cannot enable DSU for slot: " << dsuSlot; + return binder::Status::ok(); + } // Note: create the install status file last, since this is the actual boot // indicator. if (!SetBootMode(one_shot) || !CreateInstallStatusFile()) { -- cgit v1.2.3