From 9cb201009d77966d73ffdecb50d323e77e3c2d42 Mon Sep 17 00:00:00 2001 From: Patrick Donnelly Date: Tue, 13 Oct 2020 09:47:32 -0700 Subject: [PATCH 1/4] rgw: simplify lua debug stack check The function should also return 0, i.e. no result. Signed-off-by: Patrick Donnelly --- src/rgw/rgw_lua_utils.cc | 19 ++++--------------- src/test/rgw/test_rgw_lua.cc | 24 ++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 15 deletions(-) diff --git a/src/rgw/rgw_lua_utils.cc b/src/rgw/rgw_lua_utils.cc index abb355592d4..45ef0e8e5e2 100644 --- a/src/rgw/rgw_lua_utils.cc +++ b/src/rgw/rgw_lua_utils.cc @@ -24,21 +24,10 @@ int RGWDebugLog(lua_State* L) { auto cct = reinterpret_cast(lua_touserdata(L, lua_upvalueindex(1))); - constexpr auto NUM_RETURN = 0; - - if (!lua_isstring(L, -1)) { - if (cct) { - ldout(cct, 1) << "Lua ERROR: missing/invalid 'message' parameter when calling Log" << dendl; - } - return NUM_RETURN; - } - const char* message = lua_tostring(L, -1); - if (cct) { - ldout(cct, 20) << "Lua INFO: " << message << dendl; - } - - return NUM_RETURN; -}; + auto message = luaL_checkstring(L, 1); + ldout(cct, 20) << "Lua INFO: " << message << dendl; + return 0; +} void create_debug_action(lua_State* L, CephContext* cct) { lua_pushlightuserdata(L, cct); diff --git a/src/test/rgw/test_rgw_lua.cc b/src/test/rgw/test_rgw_lua.cc index 632dda114cf..2df08bd3c28 100644 --- a/src/test/rgw/test_rgw_lua.cc +++ b/src/test/rgw/test_rgw_lua.cc @@ -111,6 +111,30 @@ TEST(TestRGWLua, Hello) ASSERT_EQ(rc, 0); } +TEST(TestRGWLua, RGWDebugLogNumber) +{ + const std::string script = R"( + RGWDebugLog(1234567890) + )"; + + DEFINE_REQ_STATE; + + const auto rc = lua::request::execute(nullptr, nullptr, nullptr, &s, "", script); + ASSERT_EQ(rc, 0); +} + +TEST(TestRGWLua, RGWDebugNil) +{ + const std::string script = R"( + RGWDebugLog(nil) + )"; + + DEFINE_REQ_STATE; + + const auto rc = lua::request::execute(nullptr, nullptr, nullptr, &s, "", script); + ASSERT_EQ(rc, -1); +} + TEST(TestRGWLua, URI) { const std::string script = R"( From 146448a0ff2d314162f89cf4b1a7f21d3195f6fe Mon Sep 17 00:00:00 2001 From: Patrick Donnelly Date: Tue, 13 Oct 2020 09:47:57 -0700 Subject: [PATCH 2/4] rgw: simplify Lua stack dump procedure Lua has methods which help with printing values as strings. Signed-off-by: Patrick Donnelly --- src/rgw/rgw_lua_utils.cc | 27 +++++---------------------- 1 file changed, 5 insertions(+), 22 deletions(-) diff --git a/src/rgw/rgw_lua_utils.cc b/src/rgw/rgw_lua_utils.cc index 45ef0e8e5e2..c3b380f7957 100644 --- a/src/rgw/rgw_lua_utils.cc +++ b/src/rgw/rgw_lua_utils.cc @@ -36,29 +36,12 @@ void create_debug_action(lua_State* L, CephContext* cct) { } void stack_dump(lua_State* L) { - auto i = lua_gettop(L); + int top = lua_gettop(L); std::cout << std::endl << " ---------------- Stack Dump ----------------" << std::endl; - std::cout << "Stack Size: " << i << std::endl; - while (i > 0) { - const auto t = lua_type(L, i); - switch (t) { - case LUA_TNIL: - std::cout << i << ": nil" << std::endl; - break; - case LUA_TSTRING: - std::cout << i << ": " << lua_tostring(L, i) << std::endl; - break; - case LUA_TBOOLEAN: - std::cout << i << ": " << lua_toboolean(L, i) << std::endl; - break; - case LUA_TNUMBER: - std::cout << i << ": " << lua_tonumber(L, i) << std::endl; - break; - default: - std::cout << i << ": " << lua_typename(L, t) << std::endl; - break; - } - i--; + std::cout << "Stack Size: " << top << std::endl; + for (int i = 1, j = -top; i <= top; i++, j++) { + std::cout << "[" << i << "," << j << "]: " << luaL_tolstring(L, i, NULL) << std::endl; + lua_pop(L, 1); } std::cout << "--------------- Stack Dump Finished ---------------" << std::endl; } From 4ceb4e0f064598aac79944bd11d23c5f888f09ce Mon Sep 17 00:00:00 2001 From: Patrick Donnelly Date: Tue, 13 Oct 2020 11:19:45 -0700 Subject: [PATCH 3/4] rgw: use more efficient lua_pushliteral This avoids an unnecessary call to strlen. Signed-off-by: Patrick Donnelly --- src/rgw/rgw_lua_utils.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/rgw/rgw_lua_utils.h b/src/rgw/rgw_lua_utils.h index 3462e0ed686..55545431b79 100644 --- a/src/rgw/rgw_lua_utils.h +++ b/src/rgw/rgw_lua_utils.h @@ -95,25 +95,25 @@ void create_metatable(lua_State* L, bool toplevel, Upvalues... upvalues) } // create metatable [[maybe_unused]] const auto rc = luaL_newmetatable(L, MetaTable::Name().c_str()); - lua_pushstring(L, "__index"); + lua_pushliteral(L, "__index"); for (const auto upvalue : upvalue_arr) { lua_pushlightuserdata(L, upvalue); } lua_pushcclosure(L, MetaTable::IndexClosure, upvals_size); lua_rawset(L, -3); - lua_pushstring(L, "__newindex"); + lua_pushliteral(L, "__newindex"); for (const auto upvalue : upvalue_arr) { lua_pushlightuserdata(L, upvalue); } lua_pushcclosure(L, MetaTable::NewIndexClosure, upvals_size); lua_rawset(L, -3); - lua_pushstring(L, "__pairs"); + lua_pushliteral(L, "__pairs"); for (const auto upvalue : upvalue_arr) { lua_pushlightuserdata(L, upvalue); } lua_pushcclosure(L, MetaTable::PairsClosure, upvals_size); lua_rawset(L, -3); - lua_pushstring(L, "__len"); + lua_pushliteral(L, "__len"); for (const auto upvalue : upvalue_arr) { lua_pushlightuserdata(L, upvalue); } From 463b388d2ea03e8341292c55ac544df88c953688 Mon Sep 17 00:00:00 2001 From: Patrick Donnelly Date: Tue, 13 Oct 2020 11:20:39 -0700 Subject: [PATCH 4/4] rgw: avoid using lua_ namespace. The Lua manual states this C namespace is reserved. Even if we're defining C++ procedures with obfuscated names, this should be avoided. Also: use std::string_view for pushstring to avoid intermediate string allocations when possible. Signed-off-by: Patrick Donnelly --- src/rgw/rgw_lua_request.cc | 92 +++++++++++++++++++------------------- src/rgw/rgw_lua_utils.cc | 5 --- src/rgw/rgw_lua_utils.h | 9 ++-- 3 files changed, 52 insertions(+), 54 deletions(-) diff --git a/src/rgw/rgw_lua_request.cc b/src/rgw/rgw_lua_request.cc index ea7940daa87..8a65426af33 100644 --- a/src/rgw/rgw_lua_request.cc +++ b/src/rgw/rgw_lua_request.cc @@ -54,9 +54,9 @@ struct ResponseMetaTable : public EmptyMetaTable { } else if (strcasecmp(index, "RGWCode") == 0) { lua_pushinteger(L, err->ret); } else if (strcasecmp(index, "HTTPStatus") == 0) { - lua_pushstring(L, err->err_code.c_str()); + pushstring(L, err->err_code); } else if (strcasecmp(index, "Message") == 0) { - lua_pushstring(L, err->message.c_str()); + pushstring(L, err->message); } else { throw_unknown_field(index, TableName()); } @@ -120,9 +120,9 @@ struct PlacementRuleMetaTable : public EmptyMetaTable { const char* index = lua_tostring(L, -1); if (strcasecmp(index, "Name") == 0) { - lua_pushstring(L, rule->name.c_str()); + pushstring(L, rule->name); } else if (strcasecmp(index, "StorageClass") == 0) { - lua_pushstring(L, rule->storage_class.c_str()); + pushstring(L, rule->storage_class); } else { throw_unknown_field(index, TableName()); } @@ -141,9 +141,9 @@ struct UserMetaTable : public EmptyMetaTable { const char* index = lua_tostring(L, -1); if (strcasecmp(index, "Tenant") == 0) { - lua_pushstring(L, user->tenant); + pushstring(L, user->tenant); } else if (strcasecmp(index, "Id") == 0) { - lua_pushstring(L, user->id); + pushstring(L, user->id); } else { throw_unknown_field(index, TableName()); } @@ -162,7 +162,7 @@ struct OwnerMetaTable : public EmptyMetaTable { const char* index = lua_tostring(L, -1); if (strcasecmp(index, "DisplayName") == 0) { - lua_pushstring(L, owner->get_display_name()); + pushstring(L, owner->get_display_name()); } else if (strcasecmp(index, "User") == 0) { create_metatable(L, false, &(owner->get_id())); } else { @@ -185,23 +185,23 @@ struct BucketMetaTable : public EmptyMetaTable { const char* index = lua_tostring(L, -1); if (strcasecmp(index, "Tenant") == 0) { - lua_pushstring(L, bucket->get_tenant()); + pushstring(L, bucket->get_tenant()); } else if (strcasecmp(index, "Name") == 0) { - lua_pushstring(L, bucket->get_name()); + pushstring(L, bucket->get_name()); } else if (strcasecmp(index, "Marker") == 0) { - lua_pushstring(L, bucket->get_marker()); + pushstring(L, bucket->get_marker()); } else if (strcasecmp(index, "Id") == 0) { - lua_pushstring(L, bucket->get_bucket_id()); + pushstring(L, bucket->get_bucket_id()); } else if (strcasecmp(index, "Count") == 0) { lua_pushinteger(L, bucket->get_count()); } else if (strcasecmp(index, "Size") == 0) { lua_pushinteger(L, bucket->get_size()); } else if (strcasecmp(index, "ZoneGroupId") == 0) { - lua_pushstring(L, bucket->get_info().zonegroup); + pushstring(L, bucket->get_info().zonegroup); } else if (strcasecmp(index, "CreationTime") == 0) { - lua_pushtime(L, bucket->get_creation_time()); + pushtime(L, bucket->get_creation_time()); } else if (strcasecmp(index, "MTime") == 0) { - lua_pushtime(L, bucket->get_modification_time()); + pushtime(L, bucket->get_modification_time()); } else if (strcasecmp(index, "Quota") == 0) { create_metatable(L, false, &(bucket->get_info().quota)); } else if (strcasecmp(index, "PlacementRule") == 0) { @@ -228,15 +228,15 @@ struct ObjectMetaTable : public EmptyMetaTable { const char* index = lua_tostring(L, -1); if (strcasecmp(index, "Name") == 0) { - lua_pushstring(L, obj->get_name().c_str()); + pushstring(L, obj->get_name()); } else if (strcasecmp(index, "Instance") == 0) { - lua_pushstring(L, obj->get_instance().c_str()); + pushstring(L, obj->get_instance()); } else if (strcasecmp(index, "Id") == 0) { - lua_pushstring(L, obj->get_oid()); + pushstring(L, obj->get_oid()); } else if (strcasecmp(index, "Size") == 0) { lua_pushinteger(L, obj->get_obj_size()); } else if (strcasecmp(index, "MTime") == 0) { - lua_pushtime(L, obj->get_mtime()); + pushtime(L, obj->get_mtime()); } else { throw_unknown_field(index, TableName()); } @@ -260,7 +260,7 @@ struct StringMapMetaTable : public EmptyMetaTable { if (it == map->end()) { lua_pushnil(L); } else { - lua_pushstring(L, it->second); + pushstring(L, it->second); } return ONE_RETURNVAL; } @@ -296,8 +296,8 @@ struct StringMapMetaTable : public EmptyMetaTable { lua_pushnil(L); // return nil, nil } else { - lua_pushstring(L, next_it->first); - lua_pushstring(L, next_it->second); + pushstring(L, next_it->first); + pushstring(L, next_it->second); // return key, value } @@ -337,7 +337,7 @@ struct GrantMetaTable : public EmptyMetaTable { } else if (strcasecmp(index, "GroupType") == 0) { lua_pushinteger(L, grant->get_group()); } else if (strcasecmp(index, "Referer") == 0) { - lua_pushstring(L, grant->get_referer()); + pushstring(L, grant->get_referer()); } else { throw_unknown_field(index, TableName()); } @@ -409,7 +409,7 @@ struct GrantsMetaTable : public EmptyMetaTable { } } - lua_pushstring(L, next_it->first); + pushstring(L, next_it->first); create_metatable(L, false, &(next_it->second)); // return key, value @@ -469,7 +469,7 @@ struct StatementsMetaTable : public EmptyMetaTable { lua_pushnil(L); } else { // TODO: policy language could be interpreted to lua and executed as such - lua_pushstring(L, statement_to_string((*statements)[index])); + pushstring(L, statement_to_string((*statements)[index])); } return ONE_RETURNVAL; } @@ -503,7 +503,7 @@ struct StatementsMetaTable : public EmptyMetaTable { // return nil, nil } else { lua_pushinteger(L, next_it); - lua_pushstring(L, statement_to_string((*statements)[next_it])); + pushstring(L, statement_to_string((*statements)[next_it])); // return key, value } @@ -530,13 +530,13 @@ struct PolicyMetaTable : public EmptyMetaTable { const char* index = lua_tostring(L, -1); if (strcasecmp(index, "Text") == 0) { - lua_pushstring(L, policy->text); + pushstring(L, policy->text); } else if (strcasecmp(index, "Id") == 0) { - // TODO create lua_pushstring for std::unique_ptr + // TODO create pushstring for std::unique_ptr if (!policy->id) { lua_pushnil(L); } else { - lua_pushstring(L, policy->id.get()); + pushstring(L, policy->id.get()); } } else if (strcasecmp(index, "Statements") == 0) { create_metatable(L, &(policy->statements)); @@ -630,15 +630,15 @@ struct HTTPMetaTable : public EmptyMetaTable { } else if (strcasecmp(index, "Metadata") == 0) { create_metatable>(L, false, &(info->x_meta_map)); } else if (strcasecmp(index, "Host") == 0) { - lua_pushstring(L, info->host); + pushstring(L, info->host); } else if (strcasecmp(index, "Method") == 0) { - lua_pushstring(L, info->method); + pushstring(L, info->method); } else if (strcasecmp(index, "URI") == 0) { - lua_pushstring(L, info->request_uri); + pushstring(L, info->request_uri); } else if (strcasecmp(index, "QueryString") == 0) { - lua_pushstring(L, info->request_params); + pushstring(L, info->request_params); } else if (strcasecmp(index, "Domain") == 0) { - lua_pushstring(L, info->domain); + pushstring(L, info->domain); } else { throw_unknown_field(index, TableName()); } @@ -657,9 +657,9 @@ struct CopyFromMetaTable : public EmptyMetaTable { const char* index = lua_tostring(L, -1); if (strcasecmp(index, "Tenant") == 0) { - lua_pushstring(L, s->src_tenant_name); + pushstring(L, s->src_tenant_name); } else if (strcasecmp(index, "Bucket") == 0) { - lua_pushstring(L, s->src_bucket_name); + pushstring(L, s->src_bucket_name); } else if (strcasecmp(index, "Object") == 0) { create_metatable(L, false, s->src_object); } else { @@ -680,9 +680,9 @@ struct ZoneGroupMetaTable : public EmptyMetaTable { const char* index = lua_tostring(L, -1); if (strcasecmp(index, "Name") == 0) { - lua_pushstring(L, s->zonegroup_name); + pushstring(L, s->zonegroup_name); } else if (strcasecmp(index, "Endpoint") == 0) { - lua_pushstring(L, s->zonegroup_endpoint); + pushstring(L, s->zonegroup_endpoint); } else { throw_unknown_field(index, TableName()); } @@ -703,9 +703,9 @@ struct RequestMetaTable : public EmptyMetaTable { const char* index = lua_tostring(L, -1); if (strcasecmp(index, "RGWOp") == 0) { - lua_pushstring(L, op_name); + pushstring(L, op_name); } else if (strcasecmp(index, "DecodedURI") == 0) { - lua_pushstring(L, s->decoded_uri); + pushstring(L, s->decoded_uri); } else if (strcasecmp(index, "ContentLength") == 0) { lua_pushinteger(L, s->content_length); } else if (strcasecmp(index, "GenericAttributes") == 0) { @@ -714,7 +714,7 @@ struct RequestMetaTable : public EmptyMetaTable { create_metatable(L, false, &(s->err)); } else if (strcasecmp(index, "SwiftAccountName") == 0) { if (s->dialect == "swift") { - lua_pushstring(L, s->account_name); + pushstring(L, s->account_name); } else { lua_pushnil(L); } @@ -750,17 +750,17 @@ struct RequestMetaTable : public EmptyMetaTable { } else if (strcasecmp(index, "UserPolicies") == 0) { create_metatable(L, false, &(s->iam_user_policies)); } else if (strcasecmp(index, "RGWId") == 0) { - lua_pushstring(L, s->host_id); + pushstring(L, s->host_id); } else if (strcasecmp(index, "HTTP") == 0) { create_metatable(L, false, &(s->info)); } else if (strcasecmp(index, "Time") == 0) { - lua_pushtime(L, s->time); + pushtime(L, s->time); } else if (strcasecmp(index, "Dialect") == 0) { - lua_pushstring(L, s->dialect); + pushstring(L, s->dialect); } else if (strcasecmp(index, "Id") == 0) { - lua_pushstring(L, s->req_id); + pushstring(L, s->req_id); } else if (strcasecmp(index, "TransactionId") == 0) { - lua_pushstring(L, s->trans_id); + pushstring(L, s->trans_id); } else if (strcasecmp(index, "Tags") == 0) { create_metatable>(L, false, &(s->tagset.get_tags())); } else { @@ -790,7 +790,7 @@ int execute( // add the ops log action lua_getglobal(L, RequestMetaTable::TableName().c_str()); ceph_assert(lua_istable(L, -1)); - lua_pushstring(L, RequestLogAction); + pushstring(L, RequestLogAction); lua_pushlightuserdata(L, store); lua_pushlightuserdata(L, rest); lua_pushlightuserdata(L, olog); diff --git a/src/rgw/rgw_lua_utils.cc b/src/rgw/rgw_lua_utils.cc index c3b380f7957..caf33a23ac3 100644 --- a/src/rgw/rgw_lua_utils.cc +++ b/src/rgw/rgw_lua_utils.cc @@ -8,11 +8,6 @@ namespace rgw::lua { -void lua_pushstring(lua_State* L, const std::string& str) -{ - lua_pushstring(L, str.c_str()); -} - // TODO - add the folowing generic functions // lua_push(lua_State* L, const std::string& str) // template lua_push(lua_State* L, const std::optional& val) diff --git a/src/rgw/rgw_lua_utils.h b/src/rgw/rgw_lua_utils.h index 55545431b79..2c4d724a6f2 100644 --- a/src/rgw/rgw_lua_utils.h +++ b/src/rgw/rgw_lua_utils.h @@ -2,6 +2,7 @@ #include #include +#include #include #include @@ -11,7 +12,7 @@ namespace rgw::lua { // push ceph time in string format: "%Y-%m-%d %H:%M:%S" template -void lua_pushtime(lua_State* L, const CephTime& tp) +void pushtime(lua_State* L, const CephTime& tp) { const auto tt = CephTime::clock::to_time_t(tp); const auto tm = *std::localtime(&tt); @@ -20,8 +21,10 @@ void lua_pushtime(lua_State* L, const CephTime& tp) lua_pushstring(L, buff); } -// push std::string -void lua_pushstring(lua_State* L, const std::string& str); +static inline void pushstring(lua_State* L, std::string_view str) +{ + lua_pushlstring(L, str.data(), str.size()); +} // dump the lua stack to stdout void stack_dump(lua_State* L);