Producer/Consumer table binary support #801

Pterosaur · 2023-06-26T06:38:42Z

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 <ganze718@gmail.com>

common/luatable.cpp

common/rediscommand.h

common/redisapi.h

common/luatable.cpp

qiluo-msft · 2023-06-29T07:17:39Z

void RedisCommand::formatArgv(int argc, const char **argv, const size_t *argvlen)

I see a common pattern when using this function. Better to create an overload function, and keep this as is.

void RedisCommand::formatArgv(int argc, iterator<string>& argv)

In reply to: 1612539336

Refers to: common/rediscommand.cpp:39 in 466a131. [](commit_id = 466a131, deletion_comment = False)

Signed-off-by: Ze Gan <ganze718@gmail.com>

Pterosaur · 2023-07-07T15:36:29Z

void RedisCommand::formatArgv(int argc, const char **argv, const size_t *argvlen)

I see a common pattern when using this function. Better to create an overload function, and keep this as is.
void RedisCommand::formatArgv(int argc, iterator<string>& argv)
Refers to: common/rediscommand.cpp:39 in 466a131. [](commit_id = 466a131, deletion_comment = False)

I found an existing API, void RedisCommand::format(const vector<string> &commands), which can meet our requirement, so directly leverage it.

qiluo-msft · 2023-07-08T02:10:22Z

common/redisstring.h

+
+    void reset(char *str = nullptr, int len = 0);
+
+    const char *getStr() const;


getStr

If you align the function names with RedisCommand, then RedisCommand could be a subclass of RedisString #Closed

qiluo-msft · 2023-07-08T02:11:24Z

common/rediscommand.cpp

@@ -124,12 +113,12 @@ void RedisCommand::formatDEL(const std::string& key)

 const char *RedisCommand::c_str() const


c_str

In your new code, m_str may contains \0. Then it is super dangerous to expose c_str to outside. Do you want to deprecate this function? #Closed

I don't think so.

Because we have already provided the function len to get the length, so \0 will be handled.

As the STL string's convention, const char * doesn't matter to the `\0'.

If we don't expose the const char * instead of expose a std::string, which is safer but MAY introduce one extra deep copy which is unnecessary.

This reverts commit bcf697e.

Signed-off-by: Ze Gan <ganze718@gmail.com>

qiluo-msft · 2023-07-12T01:13:05Z

common/rediscommand.h

@@ -64,12 +66,17 @@ class RedisCommand {
    /* Format DEL key command */
    void formatDEL(const std::string& key);

+    int appendTo(redisContext *ctx) const;
+
+    std::string printable_string() const;


printable_string

As this repo's practice, we implement const std::string to_string() const member function.

Or if you think the semantics are different, you may follow the function naming convention toPrintableString

qiluo-msft · 2023-07-12T01:19:28Z

common/stringutility.h

@@ -166,4 +166,61 @@ 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)


binary_to_printable

Will you implement a reverse function? #WontFix

qiluo-msft · 2023-07-12T01:22:03Z

common/rediscommand.h

+
+    std::string printable_string() const;
+
+private:


private

This library may be used by unknown app. Let's be safe to mark it as deprecated for a while. This will serve the same purpose to fail sonic related build.

It's not available, because functions, c_str and len, have been used in other functions of this class.

Signed-off-by: Ze Gan <ganze718@gmail.com>

PR sonic-net#801 added some code to convert binary strings to be printable strings. However, one code path used the raw value of `len` without sanity checking as the length of the string, when the `length()` function should have been used instead. Sample syncd backtrace: ``` (gdb) print *this $22 = {temp = 0x55feeaced250 "*3\r\n$6\r\nSCRIPT\r\n$4\r\nLOAD\r\n$863\r\nlocal keys = redis.call('KEYS', KEYS[1])\nlocal n = table.getn(keys)\n\nfor i = 1, n do\n if KEYS[2] == \"\" and KEYS[3] == \"1\" then\n redis.call('DEL', keys[i]) \n e"..., len = -355539936} ``` Fix this by using the `length()` function. Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com>

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 <ganze718@gmail.com> Co-authored-by: Qi Luo <qiluo-msft@users.noreply.github.com>

Pterosaur added 6 commits June 26, 2023 13:46

Pass args lengths to formatArgv

f2ded7e

Signed-off-by: Ze Gan <ganze718@gmail.com>

fix

07dda63

Signed-off-by: Ze Gan <ganze718@gmail.com>

fix

1251278

Signed-off-by: Ze Gan <ganze718@gmail.com>

fix

b15ff6a

Signed-off-by: Ze Gan <ganze718@gmail.com>

Record len in RedisCommand

9622c76

Binary support

466a131

Signed-off-by: Ze Gan <ganze718@gmail.com>

Pterosaur force-pushed the redis_binary_support branch from d694c72 to 466a131 Compare June 28, 2023 10:54

Pterosaur changed the title ~~Redis binary support~~ Producer/Consumer table binary support Jun 28, 2023

Pterosaur marked this pull request as ready for review June 28, 2023 13:49

Pterosaur requested review from liuh-80, qiluo-msft and lguohan June 29, 2023 02:27

liuh-80 reviewed Jun 29, 2023

View reviewed changes

common/luatable.cpp Outdated Show resolved Hide resolved

qiluo-msft reviewed Jun 29, 2023

View reviewed changes

common/rediscommand.h Show resolved Hide resolved

qiluo-msft reviewed Jun 29, 2023

View reviewed changes

common/redisapi.h Outdated Show resolved Hide resolved

qiluo-msft reviewed Jun 29, 2023

View reviewed changes

common/luatable.cpp Outdated Show resolved Hide resolved

Pterosaur force-pushed the redis_binary_support branch 2 times, most recently from 98be4ca to 837e19d Compare July 7, 2023 02:15

Fix comments

bcf697e

Signed-off-by: Ze Gan <ganze718@gmail.com>

Pterosaur force-pushed the redis_binary_support branch from 837e19d to bcf697e Compare July 7, 2023 03:50

Pterosaur requested a review from qiluo-msft July 7, 2023 15:36

qiluo-msft reviewed Jul 8, 2023

View reviewed changes

Revert "Fix comments"

3b0f671

This reverts commit bcf697e.

Pterosaur requested a review from qiluo-msft July 9, 2023 03:17

qiluo-msft previously approved these changes Jul 10, 2023

View reviewed changes

qiluo-msft and others added 2 commits July 9, 2023 21:00

Merge branch 'master' into redis_binary_support

8d74ece

Merge branch 'master' into redis_binary_support

f3c3046

Pterosaur dismissed qiluo-msft’s stale review via ddb4e6d July 11, 2023 09:32

Pterosaur force-pushed the redis_binary_support branch 6 times, most recently from 6d60c8b to 403c6a1 Compare July 11, 2023 10:58

Add new function printable_string

142c060

Signed-off-by: Ze Gan <ganze718@gmail.com>

Pterosaur force-pushed the redis_binary_support branch from 403c6a1 to 142c060 Compare July 11, 2023 14:19

Pterosaur requested a review from qiluo-msft July 12, 2023 00:38

qiluo-msft reviewed Jul 12, 2023

View reviewed changes

Pterosaur force-pushed the redis_binary_support branch from 53171aa to e73f7ad Compare July 12, 2023 02:27

Fix comments

35f70ec

Signed-off-by: Ze Gan <ganze718@gmail.com>

Pterosaur force-pushed the redis_binary_support branch from e73f7ad to 35f70ec Compare July 12, 2023 02:42

Pterosaur requested a review from qiluo-msft July 12, 2023 02:44

Private to protected

8f8d501

Signed-off-by: Ze Gan <ganze718@gmail.com>

qiluo-msft previously approved these changes Jul 12, 2023

View reviewed changes

Pterosaur dismissed qiluo-msft’s stale review via f4db021 July 12, 2023 07:10

Fix a bug to escape slash in binary_to_printable

9370b19

Signed-off-by: Ze Gan <ganze718@gmail.com>

Pterosaur force-pushed the redis_binary_support branch from f4db021 to 9370b19 Compare July 12, 2023 07:12

qiluo-msft approved these changes Jul 12, 2023

View reviewed changes

Pterosaur merged commit 62e8d80 into sonic-net:master Jul 13, 2023
13 checks passed

saiarcot895 mentioned this pull request Jul 28, 2023

Fix syncd crashing due to invalid length being used for string printing #806

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Producer/Consumer table binary support #801

Producer/Consumer table binary support #801

Pterosaur commented Jun 26, 2023 •

edited

Loading

qiluo-msft commented Jun 29, 2023 •

edited

Loading

Pterosaur commented Jul 7, 2023

qiluo-msft Jul 8, 2023 •

edited

Loading

qiluo-msft Jul 8, 2023 •

edited

Loading

Pterosaur Jul 9, 2023 •

edited

Loading

qiluo-msft Jul 12, 2023

qiluo-msft Jul 12, 2023

Pterosaur Jul 12, 2023

qiluo-msft Jul 12, 2023 •

edited

Loading

qiluo-msft Jul 12, 2023 •

edited

Loading

Pterosaur Jul 12, 2023


		void reset(char *str = nullptr, int len = 0);

		const char *getStr() const;

		@@ -124,12 +113,12 @@ void RedisCommand::formatDEL(const std::string& key)

		const char *RedisCommand::c_str() const

Producer/Consumer table binary support #801

Producer/Consumer table binary support #801

Conversation

Pterosaur commented Jun 26, 2023 • edited Loading

qiluo-msft commented Jun 29, 2023 • edited Loading

Pterosaur commented Jul 7, 2023

qiluo-msft Jul 8, 2023 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Jul 8, 2023 • edited Loading

Choose a reason for hiding this comment

Pterosaur Jul 9, 2023 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Jul 12, 2023

Choose a reason for hiding this comment

qiluo-msft Jul 12, 2023

Choose a reason for hiding this comment

Pterosaur Jul 12, 2023

Choose a reason for hiding this comment

qiluo-msft Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

Pterosaur Jul 12, 2023

Choose a reason for hiding this comment

Pterosaur commented Jun 26, 2023 •

edited

Loading

qiluo-msft commented Jun 29, 2023 •

edited

Loading

qiluo-msft Jul 8, 2023 •

edited

Loading

qiluo-msft Jul 8, 2023 •

edited

Loading

Pterosaur Jul 9, 2023 •

edited

Loading

qiluo-msft Jul 12, 2023 •

edited

Loading

qiluo-msft Jul 12, 2023 •

edited

Loading