Skip to content

Commit 9d4968b

Browse files
adamemersonadamemerson
committed
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 <[email protected]>
1 parent b84d7e3 commit 9d4968b

File tree

4 files changed

+49
-18
lines changed

4 files changed

+49
-18
lines changed

‎src/rgw/driver/rados/rgw_datalog.cc‎

Lines changed: 34 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1042,6 +1042,7 @@ DataLogBackends::list(const DoutPrefixProvider *dpp, int shard,
10421042
std::span<rgw_data_change_log_entry> entries,
10431043
std::string marker)
10441044
{
1045+
assert(shard < shards);
10451046
const auto [start_id, // Starting generation
10461047
start_cursor // Cursor to be used when listing the
10471048
// starting generation
@@ -1113,11 +1114,18 @@ int RGWDataChangesLog::list_entries(
11131114
const DoutPrefixProvider *dpp, int shard,
11141115
int max_entries, std::vector<rgw_data_change_log_entry>& entries,
11151116
std::string_view marker, std::string* out_marker, bool* truncated,
1116-
optional_yield y)
1117+
std::string* errstr, optional_yield y)
11171118
{
11181119
std::exception_ptr eptr;
11191120
std::tuple<std::span<rgw_data_change_log_entry>,
11201121
std::string> out;
1122+
if (shard >= num_shards) [[unlikely]] {
1123+
if (errstr) {
1124+
*errstr = fmt::format("{} is not a valid shard. Valid shards are integers in [0, {})",
1125+
shard, num_shards);
1126+
}
1127+
return -EINVAL;
1128+
}
11211129
if (std::ssize(entries) < max_entries) {
11221130
entries.resize(max_entries);
11231131
}
@@ -1232,9 +1240,16 @@ int RGWDataChangesLog::list_entries(const DoutPrefixProvider *dpp,int max_entrie
12321240
}
12331241

12341242
int RGWDataChangesLog::get_info(const DoutPrefixProvider* dpp, int shard_id,
1235-
RGWDataChangesLogInfo* info, optional_yield y)
1243+
RGWDataChangesLogInfo* info,
1244+
std::string* errstr, optional_yield y)
12361245
{
1237-
assert(shard_id < num_shards);
1246+
if (shard_id >= num_shards) [[unlikely]] {
1247+
if (errstr) {
1248+
*errstr = fmt::format(
1249+
"{} is not a valid shard. Valid shards are integers in [0, {})",
1250+
shard_id, num_shards);
1251+
}
1252+
}
12381253
auto be = bes->head();
12391254
std::exception_ptr eptr;
12401255
if (y) {
@@ -1262,6 +1277,7 @@ asio::awaitable<void> DataLogBackends::trim_entries(
12621277
const DoutPrefixProvider *dpp, int shard_id,
12631278
std::string_view marker)
12641279
{
1280+
assert(shard_id < shards);
12651281
auto [target_gen, cursor] = cursorgen(std::string{marker});
12661282
std::unique_lock l(m);
12671283

@@ -1285,9 +1301,16 @@ asio::awaitable<void> DataLogBackends::trim_entries(
12851301
}
12861302

12871303
int RGWDataChangesLog::trim_entries(const DoutPrefixProvider *dpp, int shard_id,
1288-
std::string_view marker, optional_yield y)
1304+
std::string_view marker, std::string* errstr,
1305+
optional_yield y)
12891306
{
1290-
assert(shard_id < num_shards);
1307+
if (shard_id >= num_shards) [[unlikely]] {
1308+
if (errstr) {
1309+
*errstr = fmt::format(
1310+
"{} is not a valid shard. Valid shards are integers in [0, {})",
1311+
shard_id, num_shards);
1312+
}
1313+
}
12911314
std::exception_ptr eptr;
12921315
if (y) {
12931316
auto& yield = y.get_yield_context();
@@ -1309,7 +1332,11 @@ int RGWDataChangesLog::trim_entries(const DoutPrefixProvider *dpp, int shard_id,
13091332

13101333
int RGWDataChangesLog::trim_entries(const DoutPrefixProvider* dpp, int shard_id,
13111334
std::string_view marker,
1312-
librados::AioCompletion* c) {
1335+
librados::AioCompletion* c)
1336+
{
1337+
if (shard_id >= num_shards) [[unlikely]] {
1338+
return -EINVAL;
1339+
}
13131340
asio::co_spawn(rados->get_executor(),
13141341
bes->trim_entries(dpp, shard_id, marker),
13151342
c);
@@ -1527,6 +1554,7 @@ void RGWDataChangesLog::renew_stop()
15271554

15281555
void RGWDataChangesLog::mark_modified(int shard_id, const rgw_bucket_shard& bs, uint64_t gen)
15291556
{
1557+
assert(shard_id < num_shards);
15301558
if (!cct->_conf->rgw_data_notify_interval_msec) {
15311559
return;
15321560
}

‎src/rgw/driver/rados/rgw_datalog.h‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -476,7 +476,7 @@ class RGWDataChangesLog {
476476
int list_entries(const DoutPrefixProvider *dpp, int shard, int max_entries,
477477
std::vector<rgw_data_change_log_entry>& entries,
478478
std::string_view marker, std::string* out_marker,
479-
bool* truncated, optional_yield y);
479+
bool* truncated, std::string* errstr, optional_yield y);
480480
asio::awaitable<std::tuple<std::vector<rgw_data_change_log_entry>,
481481
RGWDataChangesLogMarker>>
482482
list_entries(const DoutPrefixProvider *dpp, int max_entries,
@@ -487,11 +487,12 @@ class RGWDataChangesLog {
487487
optional_yield y);
488488

489489
int trim_entries(const DoutPrefixProvider *dpp, int shard_id,
490-
std::string_view marker, optional_yield y);
490+
std::string_view marker, std::string* errstr, optional_yield y);
491491
int trim_entries(const DoutPrefixProvider *dpp, int shard_id,
492-
std::string_view marker, librados::AioCompletion* c);
492+
std::string_view marker, librados::AioCompletion* c);
493493
int get_info(const DoutPrefixProvider *dpp, int shard_id,
494-
RGWDataChangesLogInfo *info, optional_yield y);
494+
RGWDataChangesLogInfo *info, std::string* errstr,
495+
optional_yield y);
495496

496497
void mark_modified(int shard_id, const rgw_bucket_shard& bs, uint64_t gen);
497498
auto read_clear_modified() {

‎src/rgw/driver/rados/rgw_rest_log.cc‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -690,11 +690,11 @@ void RGWOp_DATALog_List::execute(optional_yield y) {
690690
// entry listed
691691
op_ret = static_cast<rgw::sal::RadosStore*>(driver)->svc()->
692692
datalog_rados->list_entries(this, shard_id, max_entries, entries,
693-
marker, &last_marker, &truncated, y);
693+
marker, &last_marker, &truncated, nullptr, y);
694694

695695
RGWDataChangesLogInfo info;
696696
op_ret = static_cast<rgw::sal::RadosStore*>(driver)->svc()->
697-
datalog_rados->get_info(this, shard_id, &info, y);
697+
datalog_rados->get_info(this, shard_id, &info, nullptr, y);
698698

699699
last_update = info.last_update;
700700
}
@@ -757,7 +757,7 @@ void RGWOp_DATALog_ShardInfo::execute(optional_yield y) {
757757
}
758758

759759
op_ret = static_cast<rgw::sal::RadosStore*>(driver)->svc()->
760-
datalog_rados->get_info(this, shard_id, &info, y);
760+
datalog_rados->get_info(this, shard_id, &info, nullptr, y);
761761
}
762762

763763
void RGWOp_DATALog_ShardInfo::send_response() {
@@ -907,7 +907,7 @@ void RGWOp_DATALog_Delete::execute(optional_yield y) {
907907
}
908908

909909
op_ret = static_cast<rgw::sal::RadosStore*>(driver)->svc()->
910-
datalog_rados->trim_entries(this, shard_id, marker, y);
910+
datalog_rados->trim_entries(this, shard_id, marker, nullptr, y);
911911
}
912912

913913
// not in header to avoid pulling in rgw_sync.h

‎src/rgw/radosgw-admin/radosgw-admin.cc‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11046,19 +11046,21 @@ int main(int argc, const char **argv)
1104611046
auto datalog_svc = static_cast<rgw::sal::RadosStore*>(driver)->svc()->datalog_rados;
1104711047
RGWDataChangesLogMarker log_marker;
1104811048

11049+
std::string errstr;
1104911050
do {
1105011051
std::vector<rgw_data_change_log_entry> entries;
1105111052
if (specified_shard_id) {
1105211053
ret = datalog_svc->list_entries(dpp(), shard_id, max_entries - count,
1105311054
entries, marker,
1105411055
&marker, &truncated,
11055-
null_yield);
11056+
&errstr, null_yield);
1105611057
} else {
1105711058
ret = datalog_svc->list_entries(dpp(), max_entries - count, entries,
1105811059
log_marker, &truncated, null_yield);
1105911060
}
1106011061
if (ret < 0) {
11061-
cerr << "ERROR: datalog_svc->list_entries(): " << cpp_strerror(-ret) << std::endl;
11062+
cerr << "ERROR: datalog_svc->list_entries(): " << errstr << ": "
11063+
<< cpp_strerror(-ret) << std::endl;
1106211064
return -ret;
1106311065
}
1106411066

@@ -11087,7 +11089,7 @@ int main(int argc, const char **argv)
1108711089

1108811090
RGWDataChangesLogInfo info;
1108911091
static_cast<rgw::sal::RadosStore*>(driver)->svc()->
11090-
datalog_rados->get_info(dpp(), i, &info, null_yield);
11092+
datalog_rados->get_info(dpp(), i, &info, nullptr, null_yield);
1109111093

1109211094
::encode_json("info", info, formatter.get());
1109311095

@@ -11150,7 +11152,7 @@ int main(int argc, const char **argv)
1115011152
}
1115111153

1115211154
auto datalog = static_cast<rgw::sal::RadosStore*>(driver)->svc()->datalog_rados;
11153-
ret = datalog->trim_entries(dpp(), shard_id, marker, null_yield);
11155+
ret = datalog->trim_entries(dpp(), shard_id, marker, nullptr, null_yield);
1115411156

1115511157
if (ret < 0 && ret != -ENODATA) {
1115611158
cerr << "ERROR: trim_entries(): " << cpp_strerror(-ret) << std::endl;

0 commit comments

Comments
 (0)