Minor refactor pkg_upgrade tool

- Refactor CompareVersion() to return verson compare result directly.
- Change enum type name to use more common abbreviation of "compare".
- Fix using UnzipPkgFromZip() to deliver std::string.
- Remove unnecessary variable initialization.
- Change return type of UnzipPkgFromZip to boolean.
- Replace some command and its arguments into constexpr.
- Extract BackendInvoker constructor with several functions for readability.
- Adjust ordering of include files with alphabetical order.

Change-Id: I4c6f7a8617da1e3fbbf6eca53fe308a8f0b61d8d
Signed-off-by: Junghyun Yeon <jungh.yeon@samsung.com>

diff --git a/src/pkg_upgrade/include/backend_invoker.hh b/src/pkg_upgrade/include/backend_invoker.hh
index 0f30fd2..4343dca 100644 (file)
--- a/src/pkg_upgrade/include/backend_invoker.hh
+++ b/src/pkg_upgrade/include/backend_invoker.hh
@@ -38,6 +38,11 @@ class BackendInvoker {
 
  private:
   std::list<std::string> parameters_;
+
+  void SetBackend(PkgType type);
+  void SetOperation(PkgOperation op, const std::string& pkgid);
+  void SetPreload(PkgLocation loc);
+  void SetRemovable(bool removable);
 };
 
 }  // common_fota

diff --git a/src/pkg_upgrade/include/common_type.hh b/src/pkg_upgrade/include/common_type.hh
index abdf6dc..a003d03 100644 (file)
--- a/src/pkg_upgrade/include/common_type.hh
+++ b/src/pkg_upgrade/include/common_type.hh
@@ -42,7 +42,7 @@ enum class PkgOperation {
   COMPLEX
 };
 
-enum class PkgVersionCompResult {
+enum class PkgVersionCmpResult {
   LOWER,
   SAME,
   HIGHER,

diff --git a/src/pkg_upgrade/include/pkg_upgrader.hh b/src/pkg_upgrade/include/pkg_upgrader.hh
index fc7c096..fe96679 100644 (file)
--- a/src/pkg_upgrade/include/pkg_upgrader.hh
+++ b/src/pkg_upgrade/include/pkg_upgrader.hh
@@ -36,8 +36,7 @@ class PkgUpgrader {
   std::string GetId() const;
   std::string GetVersion() const;
   const BackendInvoker& GetBackendInvoker() const;
-  bool CompareVersion(const PkgUpgrader& pkg,
-      PkgVersionCompResult* result) const;
+  PkgVersionCmpResult CompareVersion(const PkgUpgrader& pkg) const;
 
   virtual bool Upgrade() = 0;
 

diff --git a/src/pkg_upgrade/include/rw_upgrader.hh b/src/pkg_upgrade/include/rw_upgrader.hh
index 8bb4dcf..a11a77a 100644 (file)
--- a/src/pkg_upgrade/include/rw_upgrader.hh
+++ b/src/pkg_upgrade/include/rw_upgrader.hh
@@ -32,7 +32,7 @@ class RwUpgrader : public PkgUpgrader {
   int UnzipFiles(const std::string& dest_path);
   int UnzipXml(const std::string& pkgid);
   int UnzipData(const std::string& pkgid, const std::string& dest);
-  int UnzipPkgFromZip(const std::string& pkgid);
+  bool UnzipPkgFromZip(const std::string& pkgid);
 
  private:
   std::unique_ptr<PkgUpgrader> old_pkg_;

diff --git a/src/pkg_upgrade/src/backend_invoker.cc b/src/pkg_upgrade/src/backend_invoker.cc
index 5326855..c2f5823 100644 (file)
--- a/src/pkg_upgrade/src/backend_invoker.cc
+++ b/src/pkg_upgrade/src/backend_invoker.cc
@@ -14,58 +14,61 @@
  * limitations under the License.
  */
 
-#include <unistd.h>
-#include <sys/stat.h>
-#include <errno.h>
+#include "backend_invoker.hh"
+
 #include <ctype.h>
+#include <errno.h>
+
+#include <sys/stat.h>
 #include <sys/types.h>
 #include <sys/wait.h>
+#include <unistd.h>
 
-#include "backend_invoker.hh"
 #include "logging.hh"
 
 namespace {
 
-constexpr char kManifestDirectInstallCmdStr[] = "-y";
-constexpr char kUninstallCmdStr[] = "-d";
+constexpr char kTpkBackendCmdStr[] = "/usr/bin/tpk-backend";
+constexpr char kWgtBackendCmdStr[] = "/usr/bin/wgt-backend";
+
+constexpr char kManifestDirectInstallOptStr[] = "-y";
+constexpr char kUninstallOptStr[] = "-d";
+
+constexpr char kPreloadOptStr[] = "--preload";
+constexpr char kPreloadRWOptStr[] = "--preload-rw";
+constexpr char kPartialRWOptStr[] = "--partial-rw";
+
+constexpr char kForceRemoveOptStr[] = "--force-remove";
+constexpr char kNoRemoveOptStr[] = "--no-remove";
+
+constexpr char kKeepRWDataOptStr[] = "--keep-rwdata";
+constexpr char kSkipCheckReferenceOptStr[] = "--skip-check-reference";
 
 }  // namespace
 
 namespace common_fota {
 
-BackendInvoker::BackendInvoker(std::string pkgid, PkgType type, PkgLocation loc,
-    PkgOperation op, bool removable) {
-  if (type == PkgType::WGT)
-    parameters_.push_back("/usr/bin/wgt-backend");
-  else
-    parameters_.push_back("/usr/bin/tpk-backend");
+BackendInvoker::BackendInvoker(std::string pkgid, PkgType type,
+    PkgLocation loc, PkgOperation op, bool removable) {
+  SetBackend(type);
+  SetPreload(loc);
+  SetOperation(op, pkgid);
 
   if (op == PkgOperation::INSTALL || op == PkgOperation::UPDATE) {
-    parameters_.push_back(kManifestDirectInstallCmdStr);
-    parameters_.push_back(std::move(pkgid));
     if (loc == PkgLocation::RO) {
-      parameters_.push_back("--preload");
-      parameters_.push_back("--partial-rw");
+      parameters_.push_back(kPartialRWOptStr);
     } else {
-      if (!removable)
-        parameters_.push_back("--no-remove");
-      parameters_.push_back("--preload-rw");
+      SetRemovable(removable);
+
+      parameters_.push_back(kPreloadRWOptStr);
     }
-    parameters_.push_back("--skip-check-reference");
+    parameters_.push_back(kSkipCheckReferenceOptStr);
   } else if (op == PkgOperation::UNINSTALL) {
-    parameters_.push_back(kUninstallCmdStr);
-    parameters_.push_back(std::move(pkgid));
-    if (loc == PkgLocation::RO)
-      parameters_.push_back("--preload");
-    parameters_.push_back("--force-remove");
-    parameters_.push_back("--partial-rw");
+    parameters_.push_back(kForceRemoveOptStr);
+    parameters_.push_back(kPartialRWOptStr);
   } else if (op == PkgOperation::UNINSTALL_KEEP_RW_DATA) {
-    parameters_.push_back(kUninstallCmdStr);
-    parameters_.push_back(std::move(pkgid));
-    if (loc == PkgLocation::RO)
-      parameters_.push_back("--preload");
-    parameters_.push_back("--force-remove");
-    parameters_.push_back("--keep-rwdata");
+    parameters_.push_back(kForceRemoveOptStr);
+    parameters_.push_back(kKeepRWDataOptStr);
   }
 }
 
@@ -73,6 +76,35 @@ std::list<std::string> BackendInvoker::GetParameter() {
   return parameters_;
 }
 
+void BackendInvoker::SetBackend(PkgType type) {
+  if (type == PkgType::WGT)
+    parameters_.push_back(kWgtBackendCmdStr);
+  else
+    parameters_.push_back(kTpkBackendCmdStr);
+}
+
+void BackendInvoker::SetOperation(
+    PkgOperation op, const std::string& pkgid) {
+  if (op == PkgOperation::INSTALL || op == PkgOperation::UPDATE)
+    parameters_.push_back(kManifestDirectInstallOptStr);
+  else if (op == PkgOperation::UNINSTALL ||
+      op == PkgOperation::UNINSTALL_KEEP_RW_DATA)
+    parameters_.push_back(kUninstallOptStr);
+
+  parameters_.push_back(std::move(pkgid));
+}
+
+void BackendInvoker::SetPreload(PkgLocation loc) {
+  if (loc == PkgLocation::RO)
+    parameters_.push_back(kPreloadOptStr);
+}
+
+void BackendInvoker::SetRemovable(bool removable) {
+  if (!removable)
+    parameters_.push_back(kNoRemoveOptStr);
+}
+
+
 int BackendInvoker::Run() const {
   const char* param[parameters_.size() + 1] = { nullptr, };
 

diff --git a/src/pkg_upgrade/src/file_logbackend.cc b/src/pkg_upgrade/src/file_logbackend.cc
index 3303a67..6e36fe3 100644 (file)
--- a/src/pkg_upgrade/src/file_logbackend.cc
+++ b/src/pkg_upgrade/src/file_logbackend.cc
@@ -14,6 +14,8 @@
  * limitations under the License.
  */
 
+#include "file_logbackend.hh"
+
 #include <sys/stat.h>
 #include <sys/types.h>
 #include <unistd.h>
@@ -25,7 +27,6 @@
 #include <utility>
 
 #include "logging.hh"
-#include "file_logbackend.hh"
 
 namespace utils {
 

diff --git a/src/pkg_upgrade/src/pkg_finder.cc b/src/pkg_upgrade/src/pkg_finder.cc
index f539faa..8763431 100644 (file)
--- a/src/pkg_upgrade/src/pkg_finder.cc
+++ b/src/pkg_upgrade/src/pkg_finder.cc
@@ -16,14 +16,15 @@
 
 #include "pkg_finder.hh"
 
+#include <dirent.h>
 #include <stdlib.h>
 #include <stdio.h>
-#include <dirent.h>
+
 #include <tzplatform_config.h>
 
-#include <cstring>
 #include <algorithm>
 #include <cctype>
+#include <cstring>
 
 #include "logging.hh"
 #include "backend_invoker.hh"

diff --git a/src/pkg_upgrade/src/pkg_upgrader.cc b/src/pkg_upgrade/src/pkg_upgrader.cc
index e683c8d..4043183 100644 (file)
--- a/src/pkg_upgrade/src/pkg_upgrader.cc
+++ b/src/pkg_upgrade/src/pkg_upgrader.cc
@@ -13,7 +13,9 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+
 #include <iostream>
+
 #include <pkgmgr-info.h>
 
 #include "pkg_upgrader.hh"
@@ -57,22 +59,19 @@ const BackendInvoker& PkgUpgrader::GetBackendInvoker() const {
   return backend_;
 }
 
-bool PkgUpgrader::CompareVersion(const PkgUpgrader& pkg,
-    PkgVersionCompResult* result) const {
+PkgVersionCmpResult PkgUpgrader::CompareVersion(
+    const PkgUpgrader& pkg) const {
   pkgmgrinfo_version_compare_type compare = PMINFO_VERSION_OLD;
-  int ret = pkgmgrinfo_compare_package_version(version_.c_str(),
-          pkg.GetVersion().c_str(), &compare);
-  if (ret != 0)
-    return false;
+  if (pkgmgrinfo_compare_package_version(version_.c_str(),
+          pkg.GetVersion().c_str(), &compare) != 0)
+    return PkgVersionCmpResult::UNKNOWN;
 
   if (compare == PMINFO_VERSION_NEW)
-    *result = PkgVersionCompResult::HIGHER;
+    return PkgVersionCmpResult::HIGHER;
   else if (compare == PMINFO_VERSION_OLD)
-    *result = PkgVersionCompResult::LOWER;
+    return PkgVersionCmpResult::LOWER;
   else
-    *result = PkgVersionCompResult::SAME;
-
-  return true;
+    return PkgVersionCmpResult::SAME;
 }
 
 

diff --git a/src/pkg_upgrade/src/pkg_upgrader_factory.cc b/src/pkg_upgrade/src/pkg_upgrader_factory.cc
index b27c438..f0dffbb 100644 (file)
--- a/src/pkg_upgrade/src/pkg_upgrader_factory.cc
+++ b/src/pkg_upgrade/src/pkg_upgrader_factory.cc
@@ -14,13 +14,14 @@
  * limitations under the License.
  */
 
+#include "pkg_upgrader_factory.hh"
+
 #include <memory>
 
 #include "logging.hh"
-#include "pkg_upgrader_factory.hh"
 #include "ro2rw_upgrader.hh"
-#include "rw2ro_upgrader.hh"
 #include "ro_upgrader.hh"
+#include "rw2ro_upgrader.hh"
 #include "rw_upgrader.hh"
 #include "simple_upgrader.hh"
 

diff --git a/src/pkg_upgrade/src/ro_upgrader.cc b/src/pkg_upgrade/src/ro_upgrader.cc
index daaae49..846724a 100644 (file)
--- a/src/pkg_upgrade/src/ro_upgrader.cc
+++ b/src/pkg_upgrade/src/ro_upgrader.cc
@@ -29,14 +29,14 @@ RoUpgrader::RoUpgrader(std::unique_ptr<PkgUpgrader> old_pkg,
 
 bool RoUpgrader::Upgrade() {
   if (old_pkg_) {
-    PkgVersionCompResult result = PkgVersionCompResult::UNKNOWN;
-    if (!new_pkg_->CompareVersion(*old_pkg_, &result)) {
-      LOG(ERROR) << "Failed to compare version of package(" << new_pkg_->GetId()
-                 << ")";
+    PkgVersionCmpResult result = new_pkg_->CompareVersion(*old_pkg_);
+    if (result == PkgVersionCmpResult::UNKNOWN) {
+      LOG(ERROR) << "Failed to compare version of package("
+                 << new_pkg_->GetId() << ")";
       return false;
     }
 
-    if (result == PkgVersionCompResult::SAME) {
+    if (result == PkgVersionCmpResult::SAME) {
       LOG(INFO) << new_pkg_->GetId() << " has same version("
                 << new_pkg_->GetVersion() << "). Nothing to do.";
       return true;

diff --git a/src/pkg_upgrade/src/rw2ro_upgrader.cc b/src/pkg_upgrade/src/rw2ro_upgrader.cc
index 97fc084..3cd9911 100644 (file)
--- a/src/pkg_upgrade/src/rw2ro_upgrader.cc
+++ b/src/pkg_upgrade/src/rw2ro_upgrader.cc
@@ -30,14 +30,14 @@ Rw2RoUpgrader::Rw2RoUpgrader(std::unique_ptr<RwUpgrader> old_pkg,
 
 bool Rw2RoUpgrader::Upgrade() {
   // Update only if New RO package has higher(or same) version.
-  PkgVersionCompResult result = PkgVersionCompResult::UNKNOWN;
-  if (!new_pkg_->CompareVersion(*old_pkg_, &result)) {
+  PkgVersionCmpResult result = new_pkg_->CompareVersion(*old_pkg_);
+  if (result == PkgVersionCmpResult::UNKNOWN) {
     LOG(ERROR) << "Failed to compare version of package(" << new_pkg_->GetId()
                << ")";
     return false;
   }
 
-  if (result == PkgVersionCompResult::LOWER) {
+  if (result == PkgVersionCmpResult::LOWER) {
     LOG(INFO) << old_pkg_->GetId() << " has higher version("
               << old_pkg_->GetVersion() << ") already. Nothing to do.";
     return true;

diff --git a/src/pkg_upgrade/src/rw_upgrader.cc b/src/pkg_upgrade/src/rw_upgrader.cc
index e5286bc..666cabe 100644 (file)
--- a/src/pkg_upgrade/src/rw_upgrader.cc
+++ b/src/pkg_upgrade/src/rw_upgrader.cc
@@ -35,24 +35,22 @@ RwUpgrader::RwUpgrader(std::unique_ptr<PkgUpgrader> old_pkg,
 
 bool RwUpgrader::Upgrade() {
   if (old_pkg_) {
-    PkgVersionCompResult result = PkgVersionCompResult::UNKNOWN;
-    if (!new_pkg_->CompareVersion(*old_pkg_, &result)) {
-      LOG(ERROR) << "Failed to compare version of package(" << new_pkg_->GetId()
-                 << ")";
+    PkgVersionCmpResult result = new_pkg_->CompareVersion(*old_pkg_);
+    if (result == PkgVersionCmpResult::UNKNOWN) {
+      LOG(ERROR) << "Failed to compare version of package("
+                 << new_pkg_->GetId() << ")";
       return false;
     }
 
-    if (result == PkgVersionCompResult::SAME) {
+    if (result == PkgVersionCmpResult::SAME) {
       LOG(INFO) << new_pkg_->GetId() << " has same version("
                 << new_pkg_->GetVersion() << "). Nothing to do.";
       return true;
     }
   }
 
-  if (UnzipPkgFromZip(new_pkg_->GetId().c_str()) != 0) {
-    LOG(ERROR) << "UnzipPkgFromZip(" << new_pkg_->GetId() << ") failed";
+  if (!UnzipPkgFromZip(new_pkg_->GetId()))
     return false;
-  }
 
   if (!new_pkg_->Upgrade()) {
     LOG(ERROR) << "Upgrade operation failed";
@@ -65,42 +63,40 @@ bool RwUpgrader::Upgrade() {
 int RwUpgrader::UnzipFiles(const std::string& dest_path) {
   const char* unzip_argv[] = { "/usr/bin/unzip", "-oXqq", kOptZipFile,
       dest_path.c_str(), "-d", "/", nullptr };
+
   return BackendInvoker::XSystem(unzip_argv);
 }
 
 int RwUpgrader::UnzipXml(const std::string& pkgid) {
   std::string path = "opt/share/packages/" + pkgid + ".xml";
+
   return UnzipFiles(path);
 }
 
 int RwUpgrader::UnzipData(const std::string& pkgid,
     const std::string& dest) {
   std::string path = dest + pkgid + "/*";
+
   return UnzipFiles(path);
 }
 
-int RwUpgrader::UnzipPkgFromZip(const std::string& pkgid) {
-  int ret = -1;
-
-  ret = UnzipXml(pkgid);
-  if (ret != 0) {
+bool RwUpgrader::UnzipPkgFromZip(const std::string& pkgid) {
+  if (UnzipXml(pkgid) != 0) {
     LOG(ERROR) << "UnzipXml(" << pkgid << ") failed";
-    return ret;
+    return false;
   }
 
-  ret = UnzipData(pkgid, "opt/usr/globalapps/");
-  if (ret != 0) {
-    LOG(ERROR) << "UnzipData(" << pkgid << ") failed";
-    return ret;
+  if (UnzipData(pkgid, "opt/usr/globalapps/") != 0) {
+    LOG(ERROR) << "Failed to unzip userdata for pkg(" << pkgid << ")";
+    return false;
   }
 
-  ret = UnzipData(pkgid, "opt/etc/skel/apps_rw/");
-  if (ret != 0) {
-    LOG(ERROR) << "UnzipData(" << pkgid << ") failed";
-    return ret;
+  if (UnzipData(pkgid, "opt/etc/skel/apps_rw/") != 0) {
+    LOG(ERROR) << "Failed to unzip skel for pkg(" << pkgid << ")";
+    return false;
   }
 
-  return 0;
+  return true;
 }
 
 }  // namespace common_fota

diff --git a/src/pkg_upgrade/src/upgrader.cc b/src/pkg_upgrade/src/upgrader.cc
index c085bfe..63e9fca 100644 (file)
--- a/src/pkg_upgrade/src/upgrader.cc
+++ b/src/pkg_upgrade/src/upgrader.cc
@@ -14,26 +14,28 @@
  * limitations under the License.
  */
 
+#include "upgrader.hh"
+
+#include <ctype.h>
+#include <dirent.h>
+#include <errno.h>
 #include <fcntl.h>
+#include <pwd.h>
 #include <sqlite3.h>
-#include <string.h>
 #include <stdlib.h>
 #include <stdio.h>
-#include <dirent.h>
-#include <unistd.h>
+#include <string.h>
 #include <sys/smack.h>
 #include <sys/stat.h>
-#include <errno.h>
-#include <ctype.h>
+#include <sys/time.h>
 #include <sys/types.h>
 #include <sys/wait.h>
-#include <sys/time.h>
-#include <pwd.h>
+#include <unistd.h>
+
 #include <tzplatform_config.h>
 
 #include "logging.hh"
 #include "pkg_upgrader_factory.hh"
-#include "upgrader.hh"
 
 using std::list;
 using std::shared_ptr;