From c78112f5c5e621ccfda2d6946482cc554c1b98d2 Mon Sep 17 00:00:00 2001
From: lwardena <lasse.tjernaes.wardenaer@cern.ch>
Date: Mon, 12 Dec 2022 15:41:09 +0100
Subject: [PATCH 1/4] Add small code quality changes

---
 .../ChangeStorageClass.cpp                    | 25 ++++++++-----------
 .../ChangeStorageClass.hpp                    |  8 +++---
 2 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.cpp b/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.cpp
index a8ff83a083..ba5a535c75 100644
--- a/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.cpp
+++ b/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.cpp
@@ -85,7 +85,7 @@ ChangeStorageClass::ChangeStorageClass(
   }
 
   if (cmdLineArgs.m_archiveFileIds) {
-    for (const auto archiveFileId : cmdLineArgs.m_archiveFileIds.value() ) {
+    for (const auto& archiveFileId : cmdLineArgs.m_archiveFileIds.value() ) {
       m_archiveFileIds.push_back(archiveFileId);
     }
   }
@@ -104,12 +104,9 @@ ChangeStorageClass::ChangeStorageClass(
 //------------------------------------------------------------------------------
 // fileInFlight
 //------------------------------------------------------------------------------
-bool ChangeStorageClass::fileInFlight(const google::protobuf::RepeatedField<uint32_t> &locations) {
-  for (auto location : locations) {
-    // file is not in flight if fsid==65535
-    if (location == 65535) { return false; }
-  }
-  return true;
+bool ChangeStorageClass::fileInFlight(const google::protobuf::RepeatedField<uint32_t> &locations) const {
+  // file is not in flight if fsid==65535
+  return std::all_of(std::begin(locations), std::end(locations), [](const int &location) { return location != 65535; });
 }
 
 
@@ -128,8 +125,8 @@ void ChangeStorageClass::updateStorageClassInEosNamespace() {
     const auto [diskInstance, diskFileId] = CatalogueFetch::getInstanceAndFid(archiveFileId, m_serviceProviderPtr, m_log);
 
     // No files in flight should change storage class
-    const auto md_response = m_endpointMapPtr->getMD(diskInstance, ::eos::rpc::FILE, cta::utils::toUint64(diskFileId), "", false);
-    if (fileInFlight(md_response.fmd().locations())) {
+    if (const auto md_response = m_endpointMapPtr->getMD(diskInstance, ::eos::rpc::FILE, cta::utils::toUint64(diskFileId), "", false);
+      fileInFlight(md_response.fmd().locations())) {
       m_archiveIdsNotUpdatedInEos.push_back(archiveFileId);
       std::list<cta::log::Param> params;
       params.push_back(cta::log::Param("archiveFileId", archiveFileId));
@@ -154,7 +151,7 @@ void ChangeStorageClass::updateStorageClassInEosNamespace() {
 //------------------------------------------------------------------------------
 // storageClassExists
 //------------------------------------------------------------------------------
-void ChangeStorageClass::storageClassExists() {
+void ChangeStorageClass::storageClassExists() const {
   cta::xrd::Request request;
   const auto admincmd = request.mutable_admincmd();
 
@@ -185,7 +182,7 @@ void ChangeStorageClass::storageClassExists() {
   // wait until the data stream has been processed before exiting
   stream_future.wait();
 
-  std::list<std::string>::iterator storageClassFound = std::find(g_storageClasses.begin(), g_storageClasses.end(), m_storageClassName);
+  auto storageClassFound = std::find(g_storageClasses.begin(), g_storageClasses.end(), m_storageClassName);
   if ( g_storageClasses.end() == storageClassFound ){
     throw(exception::UserError("The storage class provided has not been defined."));
   }
@@ -194,7 +191,7 @@ void ChangeStorageClass::storageClassExists() {
 //------------------------------------------------------------------------------
 // updateCatalogue
 //------------------------------------------------------------------------------
-void ChangeStorageClass::updateStorageClassInCatalogue() {
+void ChangeStorageClass::updateStorageClassInCatalogue() const {
   cta::xrd::Request request;
 
   const auto admincmd = request.mutable_admincmd();
@@ -249,7 +246,7 @@ void ChangeStorageClass::updateStorageClassInCatalogue() {
 //------------------------------------------------------------------------------
 // writeSkippedArchiveIdsToFile
 //------------------------------------------------------------------------------
-void ChangeStorageClass::writeSkippedArchiveIdsToFile() {
+void ChangeStorageClass::writeSkippedArchiveIdsToFile() const {
   const std::filesystem::path filePath = "/var/log/skippedArchiveIds.txt";
   std::ofstream archiveIdFile(filePath);
 
@@ -258,7 +255,7 @@ void ChangeStorageClass::writeSkippedArchiveIdsToFile() {
   }
 
   if (archiveIdFile.is_open()) {
-    for (const auto archiveId : m_archiveIdsNotUpdatedInEos) {
+    for (const auto& archiveId : m_archiveIdsNotUpdatedInEos) {
       archiveIdFile << archiveId << std::endl;
     }
     archiveIdFile.close();
diff --git a/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.hpp b/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.hpp
index 87a5b6187a..4fe030f8c8 100644
--- a/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.hpp
+++ b/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.hpp
@@ -105,22 +105,22 @@ private:
    *  Checks if the storage class provided to the tool is defined,
    * and throws an exception::UserError if it is not found
   */
-  void storageClassExists();
+  void storageClassExists() const;
 
   /**
   * Updates the storage class name in the catalogue
   */
-  void updateStorageClassInCatalogue();
+  void updateStorageClassInCatalogue() const;
 
   /**
   * Writes the skipped archive ids to file
   */
-  void writeSkippedArchiveIdsToFile();
+  void writeSkippedArchiveIdsToFile() const;
 
   /**
    * Checks if a file is in flight
   */
-  bool fileInFlight(const google::protobuf::RepeatedField<unsigned int> &locations);
+  bool fileInFlight(const google::protobuf::RepeatedField<unsigned int> &locations) const;
 
   /**
    * An exception throwing version of main().
-- 
GitLab


From 3c241240c0ac139fa60e0c4bbd43a085a3ccc2d2 Mon Sep 17 00:00:00 2001
From: lwardena <lasse.tjernaes.wardenaer@cern.ch>
Date: Mon, 12 Dec 2022 15:58:17 +0100
Subject: [PATCH 2/4] Move code out of constructor

---
 .../ChangeStorageClass.cpp                    | 42 +++++++++----------
 .../ChangeStorageClass.hpp                    |  4 +-
 .../ChangeStorageClassMain.cpp                |  2 +-
 3 files changed, 21 insertions(+), 27 deletions(-)

diff --git a/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.cpp b/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.cpp
index ba5a535c75..b82cd778d5 100644
--- a/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.cpp
+++ b/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.cpp
@@ -34,9 +34,9 @@
 #include "cmdline/standalone_cli_tools/common/ConnectionConfiguration.hpp"
 #include "common/checksum/ChecksumBlob.hpp"
 #include "common/exception/CommandLineNotParsed.hpp"
-#include "common/utils/utils.hpp"
+#include "common/exception/UserError.hpp"
 #include "common/log/StdoutLogger.hpp"
-
+#include "common/utils/utils.hpp"
 #include "CtaFrontendApi.hpp"
 
 // GLOBAL VARIABLES : used to pass information between main thread and stream handler thread
@@ -55,27 +55,29 @@ ChangeStorageClass::ChangeStorageClass(
   std::istream &inStream,
   std::ostream &outStream,
   std::ostream &errStream,
-  cta::log::StdoutLogger &log,
-  const int& argc,
-  char *const *const argv):
+  cta::log::StdoutLogger &log):
   CmdLineTool(inStream, outStream, errStream),
-  m_log(log) {
+  m_log(log) {}
 
-  CmdLineArgs cmdLineArgs(argc, argv, StandaloneCliTool::CTA_CHANGE_STORAGE_CLASS);
+//------------------------------------------------------------------------------
+// exceptionThrowingMain
+//------------------------------------------------------------------------------
+int ChangeStorageClass::exceptionThrowingMain(const int argc, char *const *const argv) {
+    CmdLineArgs cmdLineArgs(argc, argv, StandaloneCliTool::CTA_CHANGE_STORAGE_CLASS);
 
   if (cmdLineArgs.m_help) {
     cmdLineArgs.printUsage(std::cout);
-    exit(0);
+    throw exception::UserError("");
   }
 
   if (!cmdLineArgs.m_storageClassName) {
     cmdLineArgs.printUsage(std::cout);
-    exit(1);
+    throw exception::UserError("Missing requried option: storage.class.name");
   }
 
   if (!cmdLineArgs.m_archiveFileId && !cmdLineArgs.m_archiveFileIds) {
     cmdLineArgs.printUsage(std::cout);
-    exit(1);
+    throw exception::UserError("filename or id must be provided");
   }
 
   m_storageClassName = cmdLineArgs.m_storageClassName.value();
@@ -96,9 +98,15 @@ ChangeStorageClass::ChangeStorageClass(
     m_eosUpdateFrequency = 100;
   }
 
-  auto [serviceProvider, endpointmap] = ConnConfiguration::readAndSetConfiguration(log, getUsername(), cmdLineArgs);
+  auto [serviceProvider, endpointmap] = ConnConfiguration::readAndSetConfiguration(m_log, getUsername(), cmdLineArgs);
   m_serviceProviderPtr = std::move(serviceProvider);
   m_endpointMapPtr = std::move(endpointmap);
+
+  storageClassExists();
+  updateStorageClassInEosNamespace();
+  updateStorageClassInCatalogue();
+  writeSkippedArchiveIdsToFile();
+  return 0;
 }
 
 //------------------------------------------------------------------------------
@@ -264,17 +272,5 @@ void ChangeStorageClass::writeSkippedArchiveIdsToFile() const {
   }
 }
 
-//------------------------------------------------------------------------------
-// exceptionThrowingMain
-//------------------------------------------------------------------------------
-int ChangeStorageClass::exceptionThrowingMain(const int argc, char *const *const argv) {
-  storageClassExists();
-  updateStorageClassInEosNamespace();
-  updateStorageClassInCatalogue();
-  writeSkippedArchiveIdsToFile();
-  return 0;
-}
-
-
 } // namespace admin
 } // namespace cta
diff --git a/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.hpp b/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.hpp
index 4fe030f8c8..64246a2b0c 100644
--- a/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.hpp
+++ b/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.hpp
@@ -50,9 +50,7 @@ public:
     std::istream &inStream,
     std::ostream &outStream,
     std::ostream &errStream,
-    cta::log::StdoutLogger &log,
-    const int& argc,
-    char *const *const argv);
+    cta::log::StdoutLogger &log);
 
 private:
 
diff --git a/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClassMain.cpp b/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClassMain.cpp
index 5175ab2e58..c0aedff3d1 100644
--- a/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClassMain.cpp
+++ b/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClassMain.cpp
@@ -35,7 +35,7 @@ int main(const int argc, char *const *const argv) {
 
   cta::log::StdoutLogger log(hostName, "cta-change-storage-class");
 
-  cta::cliTool::ChangeStorageClass cmd(std::cin, std::cout, std::cerr, log, argc, argv);
+  cta::cliTool::ChangeStorageClass cmd(std::cin, std::cout, std::cerr, log);
   int ret = cmd.main(argc, argv);
   // Delete all global objects allocated by libprotobuf
   google::protobuf::ShutdownProtobufLibrary();
-- 
GitLab


From 5ef5925928efc43b7036f2bba287ddfed2232b29 Mon Sep 17 00:00:00 2001
From: lwardena <lasse.tjernaes.wardenaer@cern.ch>
Date: Mon, 12 Dec 2022 17:22:41 +0100
Subject: [PATCH 3/4] Add search option for storage class check

---
 cmdline/CtaAdminCmdParse.hpp                    |  2 +-
 .../change_storage_class/ChangeStorageClass.cpp | 10 ++++++++--
 xroot_plugins/XrdCtaStorageClassLs.hpp          | 17 ++++++++++++++---
 xroot_plugins/XrdSsiCtaRequestMessage.cpp       |  4 +++-
 4 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/cmdline/CtaAdminCmdParse.hpp b/cmdline/CtaAdminCmdParse.hpp
index 7ceba7d4df..e7eac64628 100644
--- a/cmdline/CtaAdminCmdParse.hpp
+++ b/cmdline/CtaAdminCmdParse.hpp
@@ -634,7 +634,7 @@ const std::map<cmd_key_t, cmd_val_t> cmdOptions = {
    {{ AdminCmd::CMD_STORAGECLASS,         AdminCmd::SUBCMD_CH    },
       { opt_storageclass_alias, opt_copynb_alias.optional(), opt_vo.optional(), opt_comment.optional() }},
    {{ AdminCmd::CMD_STORAGECLASS,         AdminCmd::SUBCMD_RM    }, { opt_storageclass_alias }},
-   {{ AdminCmd::CMD_STORAGECLASS,         AdminCmd::SUBCMD_LS    }, { }},
+   {{ AdminCmd::CMD_STORAGECLASS,         AdminCmd::SUBCMD_LS    }, { opt_storageclass_alias.optional() }},
    /*----------------------------------------------------------------------------------------------------*/
    {{ AdminCmd::CMD_TAPE,                 AdminCmd::SUBCMD_ADD   },
       { opt_vid, opt_mediatype, opt_vendor, opt_logicallibrary, opt_tapepool, opt_full, 
diff --git a/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.cpp b/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.cpp
index b82cd778d5..fba8a08df1 100644
--- a/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.cpp
+++ b/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.cpp
@@ -168,6 +168,13 @@ void ChangeStorageClass::storageClassExists() const {
   admincmd->set_cmd(cta::admin::AdminCmd::CMD_STORAGECLASS);
   admincmd->set_subcmd(cta::admin::AdminCmd::SUBCMD_LS);
 
+  {
+    const auto key = cta::admin::OptionString::STORAGE_CLASS;
+    const auto new_opt = admincmd->add_option_str();
+    new_opt->set_key(key);
+    new_opt->set_value(m_storageClassName);
+  }
+
   cta::xrd::Response response;
   auto stream_future = m_serviceProviderPtr->SendAsync(request, response);
 
@@ -190,8 +197,7 @@ void ChangeStorageClass::storageClassExists() const {
   // wait until the data stream has been processed before exiting
   stream_future.wait();
 
-  auto storageClassFound = std::find(g_storageClasses.begin(), g_storageClasses.end(), m_storageClassName);
-  if ( g_storageClasses.end() == storageClassFound ){
+  if (g_storageClasses.empty()){
     throw(exception::UserError("The storage class provided has not been defined."));
   }
 }
diff --git a/xroot_plugins/XrdCtaStorageClassLs.hpp b/xroot_plugins/XrdCtaStorageClassLs.hpp
index ff5cdba7c0..a881f809fa 100644
--- a/xroot_plugins/XrdCtaStorageClassLs.hpp
+++ b/xroot_plugins/XrdCtaStorageClassLs.hpp
@@ -19,6 +19,7 @@
 
 #include <xroot_plugins/XrdCtaStream.hpp>
 #include <xroot_plugins/XrdSsiCtaRequestMessage.hpp>
+#include <optional>
 
 
 namespace cta { namespace xrd {
@@ -35,7 +36,7 @@ public:
    * @param[in]    catalogue     CTA Catalogue
    * @param[in]    scheduler     CTA Scheduler
    */
-  StorageClassLsStream(const RequestMessage &requestMsg, cta::catalogue::Catalogue &catalogue, cta::Scheduler &scheduler);
+  StorageClassLsStream(const RequestMessage &requestMsg, cta::catalogue::Catalogue &catalogue, cta::Scheduler &scheduler, const std::optional<std::string> storageClassName);
 
 private:
   /*!
@@ -52,17 +53,27 @@ private:
 
   std::list<cta::common::dataStructures::StorageClass> m_storageClassList;    //!< List of storage classes from the catalogue
 
+  std::optional<std::string> m_storageClassName;
+
   static constexpr const char* const LOG_SUFFIX  = "StorageClassLsStream";    //!< Identifier for log messages
 };
 
 
-StorageClassLsStream::StorageClassLsStream(const RequestMessage &requestMsg, cta::catalogue::Catalogue &catalogue, cta::Scheduler &scheduler) :
+StorageClassLsStream::StorageClassLsStream(const RequestMessage &requestMsg, cta::catalogue::Catalogue &catalogue, cta::Scheduler &scheduler, const std::optional<std::string> storageClassName) :
   XrdCtaStream(catalogue, scheduler),
-  m_storageClassList(catalogue.getStorageClasses())
+  m_storageClassName(storageClassName)
 {
   using namespace cta::admin;
 
   XrdSsiPb::Log::Msg(XrdSsiPb::Log::DEBUG, LOG_SUFFIX, "StorageClassLsStream() constructor");
+
+  if(m_storageClassName) {
+    m_storageClassList.push_back(m_catalogue.getStorageClass(m_storageClassName.value()));
+  } else {
+    for(const auto &storageClass : m_catalogue.getStorageClasses()) {
+      m_storageClassList.push_back(storageClass);
+    }
+  }
 }
 
 int StorageClassLsStream::fillBuffer(XrdSsiPb::OStreamBuffer<Data> *streambuf) {
diff --git a/xroot_plugins/XrdSsiCtaRequestMessage.cpp b/xroot_plugins/XrdSsiCtaRequestMessage.cpp
index f48b47247b..d81226f41a 100644
--- a/xroot_plugins/XrdSsiCtaRequestMessage.cpp
+++ b/xroot_plugins/XrdSsiCtaRequestMessage.cpp
@@ -1872,8 +1872,10 @@ void RequestMessage::processStorageClass_Ls(cta::xrd::Response &response, XrdSsi
 {
   using namespace cta::admin;
 
+  std::optional<std::string> scn = getOptional(OptionString::STORAGE_CLASS);
+
   // Create a XrdSsi stream object to return the results
-  stream = new StorageClassLsStream(*this, m_catalogue, m_scheduler);
+  stream = new StorageClassLsStream(*this, m_catalogue, m_scheduler, scn);
 
   response.set_show_header(HeaderType::STORAGECLASS_LS);
   response.set_type(cta::xrd::Response::RSP_SUCCESS);
-- 
GitLab


From dc7c37490f4cb5d9c899c2b38f96cfb5c1413fc7 Mon Sep 17 00:00:00 2001
From: lwardena <lasse.tjernaes.wardenaer@cern.ch>
Date: Thu, 15 Dec 2022 13:38:00 +0100
Subject: [PATCH 4/4] Update release notes

---
 ReleaseNotes.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ReleaseNotes.md b/ReleaseNotes.md
index 9b4c9ed5fe..ad6aeda97d 100644
--- a/ReleaseNotes.md
+++ b/ReleaseNotes.md
@@ -7,6 +7,7 @@
 - cta/CTA#213 - Add tool for injecting file into eos
 - cta/CTA#224 - Improve error message for cta-verify-file whn VID does not exist
 - cta/CTA#230 - Modify CTA code to enforce VID uppercase
+- cta/CTA#239 - Add improvments to the cta-change-storage-class tool
 ### Bug Fixes
 - cta/CTA#234 - Replace stoi with toUint64 in standalone cli tool
 - cta/CTA#238 - Compilation fails when using cta::common::Configuration::getConfEntInt(...)
-- 
GitLab