From: Adam C. Emerson Date: Tue, 15 Apr 2025 11:30:35 +0000 (-0400) Subject: rgw/multi: Fix error handling in public Datalog APIs X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=9d4968b4a075cb764b4139d864e0ec199890a89a;p=ceph.git 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 --- diff --git a/src/rgw/driver/rados/rgw_datalog.cc b/src/rgw/driver/rados/rgw_datalog.cc index 9f5cacb763eb8..942a0e666de81 100644 --- 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 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& 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::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 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 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 8da59c67e9d19..da9fc5fd68afd 100644 --- 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& entries, std::string_view marker, std::string* out_marker, - bool* truncated, optional_yield y); + bool* truncated, std::string* errstr, optional_yield y); asio::awaitable, 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 0f1ea2a783e4f..d3d71465d0a18 100644 --- 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(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(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(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(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 821ead35e4bef..e9cb0a07e6110 100644 --- 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(driver)->svc()->datalog_rados; RGWDataChangesLogMarker log_marker; + std::string errstr; do { std::vector 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(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(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;