rgw/multi: Fix error handling in public Datalog APIs

I had been thinking of list and trim as purely internal interfaces,
but they are called through HTTP and thus need to be prepared for bad
input.

Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

diff --git a/src/rgw/driver/rados/rgw_datalog.cc b/src/rgw/driver/rados/rgw_datalog.cc
index 9f5cacb763eb850879c4cf0fb28f1fb7c01a92cb..942a0e666de816ec6827adc765da64357c8867ab 100644 (file)
--- a/src/rgw/driver/rados/rgw_datalog.cc
+++ b/src/rgw/driver/rados/rgw_datalog.cc
@@ -1042,6 +1042,7 @@ DataLogBackends::list(const DoutPrefixProvider *dpp, int shard,
                      std::span<rgw_data_change_log_entry> entries,
                      std::string marker)
 {
+  assert(shard < shards);
   const auto [start_id, // Starting generation
              start_cursor // Cursor to be used when listing the
                           // starting generation
@@ -1113,11 +1114,18 @@ int RGWDataChangesLog::list_entries(
   const DoutPrefixProvider *dpp, int shard,
   int max_entries, std::vector<rgw_data_change_log_entry>& entries,
   std::string_view marker, std::string* out_marker, bool* truncated,
-  optional_yield y)
+  std::string* errstr, optional_yield y)
 {
   std::exception_ptr eptr;
   std::tuple<std::span<rgw_data_change_log_entry>,
             std::string> out;
+  if (shard >= num_shards) [[unlikely]] {
+    if (errstr) {
+    *errstr = fmt::format("{} is not a valid shard. Valid shards are integers in [0, {})",
+                        shard, num_shards);
+    }
+    return -EINVAL;
+  }
   if (std::ssize(entries) < max_entries) {
     entries.resize(max_entries);
   }
@@ -1232,9 +1240,16 @@ int RGWDataChangesLog::list_entries(const DoutPrefixProvider *dpp,int max_entrie
 }
 
 int RGWDataChangesLog::get_info(const DoutPrefixProvider* dpp, int shard_id,
-                               RGWDataChangesLogInfo* info, optional_yield y)
+                               RGWDataChangesLogInfo* info,
+                               std::string* errstr, optional_yield y)
 {
-  assert(shard_id < num_shards);
+  if (shard_id >= num_shards) [[unlikely]] {
+    if (errstr) {
+      *errstr = fmt::format(
+       "{} is not a valid shard. Valid shards are integers in [0, {})",
+       shard_id, num_shards);
+    }
+  }
   auto be = bes->head();
   std::exception_ptr eptr;
   if (y) {
@@ -1262,6 +1277,7 @@ asio::awaitable<void> DataLogBackends::trim_entries(
   const DoutPrefixProvider *dpp, int shard_id,
   std::string_view marker)
 {
+  assert(shard_id < shards);
   auto [target_gen, cursor] = cursorgen(std::string{marker});
   std::unique_lock l(m);
 
@@ -1285,9 +1301,16 @@ asio::awaitable<void> DataLogBackends::trim_entries(
 }
 
 int RGWDataChangesLog::trim_entries(const DoutPrefixProvider *dpp, int shard_id,
-                                   std::string_view marker, optional_yield y)
+                                   std::string_view marker, std::string* errstr,
+                                   optional_yield y)
 {
-  assert(shard_id < num_shards);
+  if (shard_id >= num_shards) [[unlikely]] {
+    if (errstr) {
+      *errstr = fmt::format(
+       "{} is not a valid shard. Valid shards are integers in [0, {})",
+       shard_id, num_shards);
+    }
+  }
   std::exception_ptr eptr;
   if (y) {
     auto& yield = y.get_yield_context();
@@ -1309,7 +1332,11 @@ int RGWDataChangesLog::trim_entries(const DoutPrefixProvider *dpp, int shard_id,
 
 int RGWDataChangesLog::trim_entries(const DoutPrefixProvider* dpp, int shard_id,
                                    std::string_view marker,
-                                   librados::AioCompletion* c) {
+                                   librados::AioCompletion* c)
+{
+  if (shard_id >= num_shards) [[unlikely]] {
+    return -EINVAL;
+  }
   asio::co_spawn(rados->get_executor(),
                 bes->trim_entries(dpp, shard_id, marker),
                 c);
@@ -1527,6 +1554,7 @@ void RGWDataChangesLog::renew_stop()
 
 void RGWDataChangesLog::mark_modified(int shard_id, const rgw_bucket_shard& bs, uint64_t gen)
 {
+  assert(shard_id < num_shards);
   if (!cct->_conf->rgw_data_notify_interval_msec) {
     return;
   }

diff --git a/src/rgw/driver/rados/rgw_datalog.h b/src/rgw/driver/rados/rgw_datalog.h
index 8da59c67e9d191a787ff3b21884d47e0bac04369..da9fc5fd68afd8e37fd18ae249d5c4c76883eae0 100644 (file)
--- a/src/rgw/driver/rados/rgw_datalog.h
+++ b/src/rgw/driver/rados/rgw_datalog.h
@@ -476,7 +476,7 @@ public:
   int list_entries(const DoutPrefixProvider *dpp, int shard, int max_entries,
                   std::vector<rgw_data_change_log_entry>& entries,
                   std::string_view marker, std::string* out_marker,
-                  bool* truncated, optional_yield y);
+                  bool* truncated, std::string* errstr, optional_yield y);
   asio::awaitable<std::tuple<std::vector<rgw_data_change_log_entry>,
                             RGWDataChangesLogMarker>>
   list_entries(const DoutPrefixProvider *dpp, int max_entries,
@@ -487,11 +487,12 @@ public:
                   optional_yield y);
 
   int trim_entries(const DoutPrefixProvider *dpp, int shard_id,
-                  std::string_view marker, optional_yield y);
+                  std::string_view marker, std::string* errstr, optional_yield y);
   int trim_entries(const DoutPrefixProvider *dpp, int shard_id,
-                   std::string_view marker, librados::AioCompletion* c);
+                  std::string_view marker, librados::AioCompletion* c);
   int get_info(const DoutPrefixProvider *dpp, int shard_id,
-              RGWDataChangesLogInfo *info, optional_yield y);
+              RGWDataChangesLogInfo *info, std::string* errstr,
+              optional_yield y);
 
   void mark_modified(int shard_id, const rgw_bucket_shard& bs, uint64_t gen);
   auto read_clear_modified() {

diff --git a/src/rgw/driver/rados/rgw_rest_log.cc b/src/rgw/driver/rados/rgw_rest_log.cc
index 0f1ea2a783e4f19938e59833c270ed3cc7daddc8..d3d71465d0a18a6b5b43b188fa95beaa5604007f 100644 (file)
--- a/src/rgw/driver/rados/rgw_rest_log.cc
+++ b/src/rgw/driver/rados/rgw_rest_log.cc
@@ -690,11 +690,11 @@ void RGWOp_DATALog_List::execute(optional_yield y) {
   // entry listed
   op_ret = static_cast<rgw::sal::RadosStore*>(driver)->svc()->
     datalog_rados->list_entries(this, shard_id, max_entries, entries,
-                               marker, &last_marker, &truncated, y);
+                               marker, &last_marker, &truncated, nullptr, y);
 
   RGWDataChangesLogInfo info;
   op_ret = static_cast<rgw::sal::RadosStore*>(driver)->svc()->
-    datalog_rados->get_info(this, shard_id, &info, y);
+    datalog_rados->get_info(this, shard_id, &info, nullptr, y);
 
   last_update = info.last_update;
 }
@@ -757,7 +757,7 @@ void RGWOp_DATALog_ShardInfo::execute(optional_yield y) {
   }
 
   op_ret = static_cast<rgw::sal::RadosStore*>(driver)->svc()->
-    datalog_rados->get_info(this, shard_id, &info, y);
+    datalog_rados->get_info(this, shard_id, &info, nullptr, y);
 }
 
 void RGWOp_DATALog_ShardInfo::send_response() {
@@ -907,7 +907,7 @@ void RGWOp_DATALog_Delete::execute(optional_yield y) {
   }
 
   op_ret = static_cast<rgw::sal::RadosStore*>(driver)->svc()->
-    datalog_rados->trim_entries(this, shard_id, marker, y);
+    datalog_rados->trim_entries(this, shard_id, marker, nullptr, y);
 }
 
 // not in header to avoid pulling in rgw_sync.h

diff --git a/src/rgw/radosgw-admin/radosgw-admin.cc b/src/rgw/radosgw-admin/radosgw-admin.cc
index 821ead35e4bef8e2fdb19be7b6efd06663f5a1ae..e9cb0a07e61103e8968c1203b34838c68957bd5a 100644 (file)
--- a/src/rgw/radosgw-admin/radosgw-admin.cc
+++ b/src/rgw/radosgw-admin/radosgw-admin.cc
@@ -11046,19 +11046,21 @@ next:
     auto datalog_svc = static_cast<rgw::sal::RadosStore*>(driver)->svc()->datalog_rados;
     RGWDataChangesLogMarker log_marker;
 
+    std::string errstr;
     do {
       std::vector<rgw_data_change_log_entry> entries;
       if (specified_shard_id) {
         ret = datalog_svc->list_entries(dpp(), shard_id, max_entries - count,
                                        entries, marker,
                                        &marker, &truncated,
-                                       null_yield);
+                                       &errstr, null_yield);
       } else {
         ret = datalog_svc->list_entries(dpp(), max_entries - count, entries,
                                        log_marker, &truncated, null_yield);
       }
       if (ret < 0) {
-        cerr << "ERROR: datalog_svc->list_entries(): " << cpp_strerror(-ret) << std::endl;
+        cerr << "ERROR: datalog_svc->list_entries(): " << errstr << ": "
+            << cpp_strerror(-ret) << std::endl;
         return -ret;
       }
 
@@ -11087,7 +11089,7 @@ next:
 
       RGWDataChangesLogInfo info;
       static_cast<rgw::sal::RadosStore*>(driver)->svc()->
-       datalog_rados->get_info(dpp(), i, &info, null_yield);
+       datalog_rados->get_info(dpp(), i, &info, nullptr, null_yield);
 
       ::encode_json("info", info, formatter.get());
 
@@ -11150,7 +11152,7 @@ next:
     }
 
     auto datalog = static_cast<rgw::sal::RadosStore*>(driver)->svc()->datalog_rados;
-    ret = datalog->trim_entries(dpp(), shard_id, marker, null_yield);
+    ret = datalog->trim_entries(dpp(), shard_id, marker, nullptr, null_yield);
 
     if (ret < 0 && ret != -ENODATA) {
       cerr << "ERROR: trim_entries(): " << cpp_strerror(-ret) << std::endl;