improve code based on franku's review

- actually use std::string instead of char* - use a reference to VolSessionInfo - add str_to_uint16()/str_to_uint32()
bareos · Mar 15, 2019 · 76104e4 · 76104e4
1 parent a201483
commit 76104e4
Show file tree

Hide file tree

Showing 5 changed files with 23 additions and 27 deletions.
diff --git a/core/src/cats/cats.h b/core/src/cats/cats.h
@@ -35,8 +35,6 @@
 #ifndef BAREOS_CATS_CATS_H_
 #define BAREOS_CATS_CATS_H_ 1
 
-#include "lib/volume_session_info.h"
-
 /* import automatically generated SQL_QUERY_ENUM */
 #include "bdb_query_enum_class.h"
 
@@ -49,6 +47,8 @@
 
 #define faddr_t long
 
+struct VolumeSessionInfo;
+
 /**
  * Generic definitions of list types, list handlers and result handlers.
  */
@@ -823,7 +823,7 @@ class BareosDb
   int GetNdmpLevelMapping(JobControlRecord* jcr,
                           JobDbRecord* jr,
                           char* filesystem);
-  bool GetNdmpEnvironmentString(const VolumeSessionInfo vsi,
+  bool GetNdmpEnvironmentString(const VolumeSessionInfo& vsi,
                                 const int32_t FileIndex,
                                 DB_RESULT_HANDLER* ResultHandler,
                                 void* ctx);

diff --git a/core/src/cats/sql_get.cc b/core/src/cats/sql_get.cc
@@ -1722,13 +1722,9 @@ bool BareosDb::GetNdmpEnvironmentString(const JobId_t JobId,
                                         void* ctx)
 {
   ASSERT(JobId > 0)
-  /* ***FIXME*** better use std::string */
-  char query[100];
-  Bsnprintf(
-      query, 99,
-      "SELECT EnvName, EnvValue FROM NDMPJobEnvironment WHERE JobId=%lld ",
-      JobId);
-  bool status = SqlQueryWithHandler(query, ResultHandler, ctx);
+  std::string query{"SELECT EnvName, EnvValue FROM NDMPJobEnvironment"};
+  query += " WHERE JobId=" + std::to_string(JobId);
+  bool status = SqlQueryWithHandler(query.c_str(), ResultHandler, ctx);
   return status && SqlNumRows() > 0;  // no rows means no environment was found
 }
 
@@ -1744,13 +1740,10 @@ bool BareosDb::GetNdmpEnvironmentString(const JobId_t JobId,
                                         void* ctx)
 {
   ASSERT(JobId > 0)
-  /* ***FIXME*** better use std::string */
-  char query[150];
-  Bsnprintf(query, 149,
-            "SELECT EnvName, EnvValue FROM NDMPJobEnvironment "
-            "WHERE JobId=%lld AND FileIndex=%lld ",
-            JobId, FileIndex);
-  bool status = SqlQueryWithHandler(query, ResultHandler, ctx);
+  std::string query{"SELECT EnvName, EnvValue FROM NDMPJobEnvironment"};
+  query += " WHERE JobId=" + std::to_string(JobId);
+  query += " AND FileIndex=" + std::to_string(JobId);
+  bool status = SqlQueryWithHandler(query.c_str(), ResultHandler, ctx);
   return status && SqlNumRows() > 0;  // no rows means no environment was found
 }
 
@@ -1761,21 +1754,19 @@ bool BareosDb::GetNdmpEnvironmentString(const JobId_t JobId,
  * Returns false: on failure
  *         true: on success
  */
-bool BareosDb::GetNdmpEnvironmentString(const VolumeSessionInfo vsi,
+bool BareosDb::GetNdmpEnvironmentString(const VolumeSessionInfo& vsi,
                                         const int32_t FileIndex,
                                         DB_RESULT_HANDLER* ResultHandler,
                                         void* ctx)
 {
-  char query[150];
   db_int64_ctx lctx;
+  std::string query{"SELECT JobId FROM Job"};
+  query += " WHERE VolSessionId = " + std::to_string(vsi.id);
+  query += " AND VolSessionTime = " + std::to_string(vsi.time);
 
-  /* Lookup the JobId and pass it to the JobId-based version */
-  Bsnprintf(query, 149,
-            "SELECT JobId FROM Job WHERE VolSessionId = %lld "
-            "AND VolSessionTime = %lld ",
-            vsi.id, vsi.time);
-  if (SqlQueryWithHandler(query, db_int64_handler, &lctx)) {
+  if (SqlQueryWithHandler(query.c_str(), db_int64_handler, &lctx)) {
     if (lctx.count == 1) {
+      /* now lctx.value contains the jobid we restore */
       return GetNdmpEnvironmentString(lctx.value, FileIndex, ResultHandler,
                                       ctx);
     }

diff --git a/core/src/dird/ndmp_dma_restore_NDMP_NATIVE.cc b/core/src/dird/ndmp_dma_restore_NDMP_NATIVE.cc
@@ -73,7 +73,7 @@ static inline bool fill_restore_environment_ndmp_native(
    * We use the first jobid to get the environment string
    */
 
-  JobId_t JobId = (JobId_t)str_to_int32(jcr->JobIds);
+  JobId_t JobId{str_to_uint32(jcr->JobIds)};
   if (JobId <= 0) {
     Jmsg(jcr, M_FATAL, 0, "Impossible JobId: %d", JobId);
     return false;

diff --git a/core/src/lib/edit.h b/core/src/lib/edit.h
@@ -22,6 +22,8 @@
 #define BAREOS_LIB_EDIT_H_
 
 uint64_t str_to_uint64(const char* str);
+#define str_to_uint16(str) ((uint16_t)str_to_uint64(str))
+#define str_to_uint32(str) ((uint32_t)str_to_uint64(str))
 int64_t str_to_int64(const char* str);
 #define str_to_int16(str) ((int16_t)str_to_int64(str))
 #define str_to_int32(str) ((int32_t)str_to_int64(str))

diff --git a/core/src/lib/volume_session_info.h b/core/src/lib/volume_session_info.h
@@ -29,7 +29,10 @@ struct VolumeSessionInfo {
   uint32_t time;
 
   /* explicit constructor disables default construction */
-  VolumeSessionInfo(uint32_t t_id, uint32_t t_time) : id(t_id), time(t_time) {}
+  VolumeSessionInfo(const uint32_t t_id, const uint32_t t_time)
+      : id(t_id), time(t_time)
+  {
+  }
 };
 
 #endif /**  BAREOS_LIB_VOLUME_SESSION_INFO_H_ */