From 62e8d805a1e5c116666d877d3232cdbe3c35ebab Mon Sep 17 00:00:00 2001 From: Ze Gan Date: Thu, 13 Jul 2023 12:55:11 +0800 Subject: [PATCH] Producer/Consumer table binary support (#801) We want to make the Producer/Consumer table can support binary messages, The native C string (char *) will be replaced to pointers and its lengths in all paths. Meanwhile, the Python interfaces of SWIG can only handle the type, str, with UTF-8. So, we need to specialize the SWIG interfaces from bytes of Python to string of C++. --------- Signed-off-by: Ze Gan Co-authored-by: Qi Luo --- common/consumerstatetable.cpp | 6 ++-- common/luatable.cpp | 12 ++------ common/producerstatetable.cpp | 36 ++++------------------ common/redisapi.h | 10 +----- common/rediscommand.cpp | 35 ++++++++++++++------- common/rediscommand.h | 16 +++++++--- common/redispipeline.h | 2 +- common/redisreply.cpp | 12 ++++---- common/stringutility.h | 58 +++++++++++++++++++++++++++++++++++ pyext/swsscommon.i | 56 +++++++++++++++++++++++++++++++++ tests/stringutility_ut.cpp | 12 ++++++++ 11 files changed, 181 insertions(+), 74 deletions(-) diff --git a/common/consumerstatetable.cpp b/common/consumerstatetable.cpp index 40bc9d71e..495ef503d 100644 --- a/common/consumerstatetable.cpp +++ b/common/consumerstatetable.cpp @@ -68,7 +68,7 @@ void ConsumerStateTable::pops(std::deque &vkco, const st auto& ctx = ctx0->element[ie]; assert(ctx->element[0]->type == REDIS_REPLY_STRING); - std::string key = ctx->element[0]->str; + std::string key(ctx->element[0]->str, ctx->element[0]->len); kfvKey(kco) = key; assert(ctx->element[1]->type == REDIS_REPLY_ARRAY); @@ -76,8 +76,8 @@ void ConsumerStateTable::pops(std::deque &vkco, const st for (size_t i = 0; i < ctx1->elements / 2; i++) { FieldValueTuple e; - fvField(e) = ctx1->element[i * 2]->str; - fvValue(e) = ctx1->element[i * 2 + 1]->str; + fvField(e).assign(ctx1->element[i * 2]->str, ctx1->element[i * 2]->len); + fvValue(e).assign(ctx1->element[i * 2 + 1]->str, ctx1->element[i * 2 + 1]->len); values.push_back(e); } diff --git a/common/luatable.cpp b/common/luatable.cpp index addcd0740..53527381b 100644 --- a/common/luatable.cpp +++ b/common/luatable.cpp @@ -54,13 +54,9 @@ bool LuaTable::get(const vector &luaKeys, vector &value args.emplace_back(v); } - // Transform data structure - vector args1; - transform(args.begin(), args.end(), back_inserter(args1), [](const string &s) { return s.c_str(); } ); - // Invoke redis command RedisCommand command; - command.formatArgv((int)args1.size(), &args1[0], NULL); + command.format(args); RedisReply r(m_db.get(), command, REDIS_REPLY_ARRAY); redisReply *reply = r.getContext(); @@ -109,13 +105,9 @@ bool LuaTable::hget(const vector &luaKeys, const string &field, string & args.emplace_back(v); } - // Transform data structure - vector args1; - transform(args.begin(), args.end(), back_inserter(args1), [](const string &s) { return s.c_str(); } ); - // Invoke redis command RedisCommand command; - command.formatArgv((int)args1.size(), &args1[0], NULL); + command.format(args); RedisReply r(m_db.get(), command); redisReply *reply = r.getContext(); diff --git a/common/producerstatetable.cpp b/common/producerstatetable.cpp index 384ba7cd3..d0db5e2a5 100644 --- a/common/producerstatetable.cpp +++ b/common/producerstatetable.cpp @@ -134,13 +134,9 @@ void ProducerStateTable::set(const string &key, const vector &v args.emplace_back(fvValue(iv)); } - // Transform data structure - vector args1; - transform(args.begin(), args.end(), back_inserter(args1), [](const string &s) { return s.c_str(); } ); - // Invoke redis command RedisCommand command; - command.formatArgv((int)args1.size(), &args1[0], NULL); + command.format(args); m_pipe->push(command, REDIS_REPLY_NIL); if (!m_buffered) { @@ -171,13 +167,9 @@ void ProducerStateTable::del(const string &key, const string &op /*= DEL_COMMAND args.emplace_back("''"); args.emplace_back("''"); - // Transform data structure - vector args1; - transform(args.begin(), args.end(), back_inserter(args1), [](const string &s) { return s.c_str(); } ); - // Invoke redis command RedisCommand command; - command.formatArgv((int)args1.size(), &args1[0], NULL); + command.format(args); m_pipe->push(command, REDIS_REPLY_NIL); if (!m_buffered) { @@ -224,13 +216,9 @@ void ProducerStateTable::set(const std::vector& values) } } - // Transform data structure - vector args1; - transform(args.begin(), args.end(), back_inserter(args1), [](const string &s) { return s.c_str(); } ); - // Invoke redis command RedisCommand command; - command.formatArgv((int)args1.size(), &args1[0], NULL); + command.format(args); m_pipe->push(command, REDIS_REPLY_NIL); if (!m_buffered) { @@ -265,13 +253,9 @@ void ProducerStateTable::del(const std::vector& keys) } args.emplace_back("G"); - // Transform data structure - vector args1; - transform(args.begin(), args.end(), back_inserter(args1), [](const string &s) { return s.c_str(); } ); - // Invoke redis command RedisCommand command; - command.formatArgv((int)args1.size(), &args1[0], NULL); + command.format(args); m_pipe->push(command, REDIS_REPLY_NIL); if (!m_buffered) { @@ -307,13 +291,9 @@ void ProducerStateTable::clear() args.emplace_back(getStateHashPrefix() + getTableName()); args.emplace_back(getDelKeySetName()); - // Transform data structure - vector args1; - transform(args.begin(), args.end(), back_inserter(args1), [](const string &s) { return s.c_str(); } ); - // Invoke redis command RedisCommand cmd; - cmd.formatArgv((int)args1.size(), &args1[0], NULL); + cmd.format(args); m_pipe->push(cmd, REDIS_REPLY_NIL); m_pipe->flush(); } @@ -466,13 +446,9 @@ void ProducerStateTable::apply_temp_view() SWSS_LOG_DEBUG("apply_view.lua is called with following argument list: %s", ss.str().c_str()); } - // Transform data structure - vector args1; - transform(args.begin(), args.end(), back_inserter(args1), [](const string &s) { return s.c_str(); } ); - // Invoke redis command RedisCommand command; - command.formatArgv((int)args1.size(), &args1[0], NULL); + command.format(args); m_pipe->push(command, REDIS_REPLY_NIL); m_pipe->flush(); diff --git a/common/redisapi.h b/common/redisapi.h index d856639ff..bdb32b5cd 100644 --- a/common/redisapi.h +++ b/common/redisapi.h @@ -83,16 +83,8 @@ static inline std::set runRedisScript(RedisContext &ctx, const std: args.insert(args.end(), argv.begin(), argv.end()); args.push_back("''"); - // Convert to vector of char * - std::vector c_args; - transform( - args.begin(), - args.end(), - std::back_inserter(c_args), - [](const std::string& s) { return s.c_str(); } ); - RedisCommand command; - command.formatArgv(static_cast(c_args.size()), c_args.data(), NULL); + command.format(args); std::set ret; try diff --git a/common/rediscommand.cpp b/common/rediscommand.cpp index 8156078e5..3b8ed7041 100644 --- a/common/rediscommand.cpp +++ b/common/rediscommand.cpp @@ -1,13 +1,15 @@ #include #include #include "rediscommand.h" +#include "stringutility.h" using namespace std; namespace swss { RedisCommand::RedisCommand() - : temp(NULL) + : temp(NULL), + len(0) { } @@ -26,7 +28,7 @@ void RedisCommand::format(const char *fmt, ...) va_list ap; va_start(ap, fmt); - int len = redisvFormatCommand(&temp, fmt, ap); + len = redisvFormatCommand(&temp, fmt, ap); va_end(ap); if (len == -1) { throw std::bad_alloc(); @@ -43,7 +45,7 @@ void RedisCommand::formatArgv(int argc, const char **argv, const size_t *argvlen temp = nullptr; } - int len = redisFormatCommandArgv(&temp, argc, argv, argvlen); + len = redisFormatCommandArgv(&temp, argc, argv, argvlen); if (len == -1) { throw std::bad_alloc(); } @@ -52,11 +54,13 @@ void RedisCommand::formatArgv(int argc, const char **argv, const size_t *argvlen void RedisCommand::format(const vector &commands) { vector args; + vector lens; for (auto& command : commands) { args.push_back(command.c_str()); + lens.push_back(command.size()); } - formatArgv(static_cast(args.size()), args.data(), NULL); + formatArgv(static_cast(args.size()), args.data(), lens.data()); } /* Format HSET key multiple field value command */ @@ -96,12 +100,9 @@ void RedisCommand::formatHDEL(const std::string& key, const std::vector args = {"HDEL", key.c_str()}; - for (const std::string &f : fields) - { - args.push_back(f.c_str()); - } - formatArgv(static_cast(args.size()), args.data(), NULL); + std::vector args = {"HDEL", key}; + args.insert(args.end(), fields.begin(), fields.end()); + format(args); } /* Format EXPIRE key field command */ @@ -122,6 +123,16 @@ void RedisCommand::formatDEL(const std::string& key) return format("DEL %s", key.c_str()); } +int RedisCommand::appendTo(redisContext *ctx) const +{ + return redisAppendFormattedCommand(ctx, c_str(), length()); +} + +std::string RedisCommand::toPrintableString() const +{ + return binary_to_printable(temp, len); +} + const char *RedisCommand::c_str() const { return temp; @@ -129,7 +140,9 @@ const char *RedisCommand::c_str() const size_t RedisCommand::length() const { - return strlen(temp); + if (len <= 0) + return 0; + return static_cast(len); } } diff --git a/common/rediscommand.h b/common/rediscommand.h index 74536c6ec..ed6cd846b 100644 --- a/common/rediscommand.h +++ b/common/rediscommand.h @@ -6,6 +6,7 @@ #include #include #include +#include namespace swss { @@ -17,6 +18,7 @@ typedef std::tuple > KeyO #define kfvOp std::get<1> #define kfvFieldsValues std::get<2> + class RedisCommand { public: RedisCommand(); @@ -64,12 +66,18 @@ class RedisCommand { /* Format DEL key command */ void formatDEL(const std::string& key); + int appendTo(redisContext *ctx) const; + + std::string toPrintableString() const; + +protected: const char *c_str() const; size_t length() const; private: char *temp; + int len; }; template @@ -80,15 +88,15 @@ void RedisCommand::formatHSET(const std::string &key, const char* cmd = "HSET"; - std::vector args = { cmd, key.c_str() }; + std::vector args = { cmd, key.c_str() }; for (auto i = start; i != stop; i++) { - args.push_back(fvField(*i).c_str()); - args.push_back(fvValue(*i).c_str()); + args.push_back(fvField(*i)); + args.push_back(fvValue(*i)); } - formatArgv((int)args.size(), args.data(), NULL); + format(args); } } diff --git a/common/redispipeline.h b/common/redispipeline.h index 1951e02e2..b8efa3840 100644 --- a/common/redispipeline.h +++ b/common/redispipeline.h @@ -49,7 +49,7 @@ class RedisPipeline { case REDIS_REPLY_STATUS: case REDIS_REPLY_INTEGER: { - int rc = redisAppendFormattedCommand(m_db->getContext(), command.c_str(), command.length()); + int rc = command.appendTo(m_db->getContext()); if (rc != REDIS_OK) { // The only reason of error is REDIS_ERR_OOM (Out of memory) diff --git a/common/redisreply.cpp b/common/redisreply.cpp index 05b97df38..8e3e73602 100644 --- a/common/redisreply.cpp +++ b/common/redisreply.cpp @@ -78,7 +78,7 @@ inline void guard(FUNC func, const char* command) RedisReply::RedisReply(RedisContext *ctx, const RedisCommand& command) { - int rc = redisAppendFormattedCommand(ctx->getContext(), command.c_str(), command.length()); + int rc = command.appendTo(ctx->getContext()); if (rc != REDIS_OK) { // The only reason of error is REDIS_ERR_OOM (Out of memory) @@ -89,9 +89,9 @@ RedisReply::RedisReply(RedisContext *ctx, const RedisCommand& command) rc = redisGetReply(ctx->getContext(), (void**)&m_reply); if (rc != REDIS_OK) { - throw RedisError("Failed to redisGetReply with " + string(command.c_str()), ctx->getContext()); + throw RedisError("Failed to redisGetReply with " + command.toPrintableString(), ctx->getContext()); } - guard([&]{checkReply();}, command.c_str()); + guard([&]{checkReply();}, command.toPrintableString().c_str()); } RedisReply::RedisReply(RedisContext *ctx, const string& command) @@ -109,19 +109,19 @@ RedisReply::RedisReply(RedisContext *ctx, const string& command) { throw RedisError("Failed to redisGetReply with " + command, ctx->getContext()); } - guard([&]{checkReply();}, command.c_str()); + guard([&]{checkReply();}, binary_to_printable(command.c_str(), command.length()).c_str()); } RedisReply::RedisReply(RedisContext *ctx, const RedisCommand& command, int expectedType) : RedisReply(ctx, command) { - guard([&]{checkReplyType(expectedType);}, command.c_str()); + guard([&]{checkReplyType(expectedType);}, command.toPrintableString().c_str()); } RedisReply::RedisReply(RedisContext *ctx, const string& command, int expectedType) : RedisReply(ctx, command) { - guard([&]{checkReplyType(expectedType);}, command.c_str()); + guard([&]{checkReplyType(expectedType);}, binary_to_printable(command.c_str(), command.length()).c_str()); } RedisReply::RedisReply(redisReply *reply) : diff --git a/common/stringutility.h b/common/stringutility.h index bcc4c31e4..e529a7d59 100644 --- a/common/stringutility.h +++ b/common/stringutility.h @@ -166,4 +166,62 @@ static inline std::string binary_to_hex(const void *buffer, size_t length) return s; } +static inline std::string binary_to_printable(const void *buffer, size_t length) +{ + std::string printable; + printable.reserve(length * 4); + + auto buf = static_cast(buffer); + + for (size_t i = 0; i < length; i++) + { + std::uint8_t c = buf[i]; + if (std::isprint(c)) + { + if (c == '\\') + { + printable.push_back('\\'); + printable.push_back('\\'); + } + else + { + printable.push_back(c); + } + } + else if (std::isspace(c)) + { + printable.push_back('\\'); + if (c == '\n') + { + printable.push_back('n'); + } + else if (c == '\r') + { + printable.push_back('r'); + } + else if (c == '\t') + { + printable.push_back('t'); + } + else if (c == '\v') + { + printable.push_back('v'); + } + else if (c == '\f') + { + printable.push_back('f'); + } + } + else + { + printable.push_back('\\'); + printable.push_back('x'); + printable.push_back("0123456789ABCDEF"[c >> 4]); + printable.push_back("0123456789ABCDEF"[c & 0xf]); + } + } + + return printable; +} + } diff --git a/pyext/swsscommon.i b/pyext/swsscommon.i index 48de846d1..9c475a201 100644 --- a/pyext/swsscommon.i +++ b/pyext/swsscommon.i @@ -151,6 +151,62 @@ temp = SWIG_NewPointerObj(*$1, SWIGTYPE_p_swss__Selectable, 0); SWIG_Python_AppendOutput($result, temp); } + +%typemap(in, fragment="SWIG_AsPtr_std_string") + const std::vector,std::allocator< std::pair< std::string,std::string > > > & + (std::vector< std::pair< std::string,std::string >,std::allocator< std::pair< std::string,std::string > > > temp, + int res) { + res = SWIG_OK; + for (int i = 0; i < PySequence_Length($input); ++i) { + temp.push_back(std::pair< std::string,std::string >()); + PyObject *item = PySequence_GetItem($input, i); + if (!PyTuple_Check(item) || PyTuple_Size(item) != 2) { + SWIG_fail; + } + PyObject *key = PyTuple_GetItem(item, 0); + PyObject *value = PyTuple_GetItem(item, 1); + std::string *ptr = (std::string *)0; + if (PyBytes_Check(key)) { + temp.back().first.assign(PyBytes_AsString(key), PyBytes_Size(key)); + } else if (SWIG_AsPtr_std_string(key, &ptr)) { + temp.back().first = *ptr; + } else { + SWIG_fail; + } + if (PyBytes_Check(value)) { + temp.back().second.assign(PyBytes_AsString(value), PyBytes_Size(value)); + } else if (SWIG_AsPtr_std_string(value, &ptr)) { + temp.back().second = *ptr; + } else { + SWIG_fail; + } + } + $1 = &temp; +} + +%typemap(typecheck) const std::vector< std::pair< std::string,std::string >,std::allocator< std::pair< std::string,std::string > > > &{ + $1 = 1; + for (int i = 0; i < PySequence_Length($input); ++i) { + PyObject *item = PySequence_GetItem($input, i); + if (!PyTuple_Check(item) || PyTuple_Size(item) != 2) { + $1 = 0; + break; + } + PyObject *key = PyTuple_GetItem(item, 0); + PyObject *value = PyTuple_GetItem(item, 1); + if (!PyBytes_Check(key) + && !PyUnicode_Check(key) + && !PyString_Check(key) + && !PyBytes_Check(value) + && !PyUnicode_Check(value) + && !PyString_Check(value)) { + $1 = 0; + break; + } + } +} + + #endif #ifdef SWIGGO diff --git a/tests/stringutility_ut.cpp b/tests/stringutility_ut.cpp index 209392592..0536958b0 100644 --- a/tests/stringutility_ut.cpp +++ b/tests/stringutility_ut.cpp @@ -94,3 +94,15 @@ TEST(STRINGUTILITY, binary_to_hex) EXPECT_EQ(swss::binary_to_hex(nullptr, 0), ""); } + +TEST(STRINGUTILITY, binary_to_printable) +{ + std::array a{0x1, 0x2, 0x03, 0xff, 0x4}; + EXPECT_EQ(swss::binary_to_printable(a.data(), a.size()), "\\x01\\x02\\x03\\xFF\\x04"); + + std::array b{0x0, 'a', '\n', '\'', '\\'}; + EXPECT_EQ(swss::binary_to_printable(b.data(), b.size()), "\\x00a\\n'\\\\"); + + EXPECT_EQ(swss::binary_to_printable(nullptr, 0), ""); +} +