Add utility for string and redis #434

Pterosaur · 2020-12-22T14:33:59Z

This PR provides the following utilities

AlphaBool

This is a class to provide a unified bool string

#include <swss/common/boolean.h>

swss::AlphaBool bt(true);
swss::AlphaBool bf(false);

std::cout << bt << std::endl; // "true"
std::cout << bf << std::endl; // "false"

lexical_convert

This function can be used to convert the key in redis to variables with expected type

#include <swss/common/stringutility.h>

void doTask(const std::string &key, ...)
{
    // Assume the key is an applicant port "ETHERNET0:192.168.0.1:8080"
    std::string interface_name;
    IpAddress ip; // Need to define operator<<(ostream, IpAddress &)
    std::uint16 port;
    auto vs = tokenize(key, ':');
    lexical_convert(vs, interface_name, ip, port);
}

join

This function can be used to generate key for redis by some different material

#include <swss/common/stringutility.h>

void updateItem(
    const std::string &interface_name,
    const IpAddress &ip, // Need to define operator>>(istream, IpAddress &)
    std::uint16 port,
    const std::vector<FieldValueTuple> &attrs)
{
    const std::string key = join(interface_name, ip, port);
    m_table.set(key, attrs);
}

hex_to_binary

Generate a segment binary according to a hex string

#include <swss/common/stringutility.h>

struct CipherKey {
    ...
};

bool CreateCipherKey(const std::string &hex_str, CipherKey &key)
{
    // hex_str is "010203FF"
    // key will be 0x010203FF in memory
    return hex_to_binary(hex_str, &key, sizeof(key));
}

binary_to_hex

Serialize a segment binary to a hex string

#include <swss/common/stringutility.h>

struct CipherKey {
    ...
};

std::string GetCipherKey(CipherKey &key)
{
    // key is 0x010203FF in memory
    // hex_str will be "010203FF"
    return binary_to_hex(&key, sizeof(key));
}

fvtGetValue

This function provides a method to easily get the value from std::vector<FieldValueTuple> which is typical exchange format of redis in SONiC

#include <swss/common/redisutility.h>

void doTask(const std::string &key, std::vector<FieldValueTuple> &attrs)
{
    // Assume attrs includes
    // port_id = 0
    // port_name = Ethernet0
    // is_enable = true

    boost::optional<std::string> t1;
    boost::optional<std::string> t2;
    boost::optional<std::string> t3;
    boost::optional<std::string> t4;

    int port_id;
    std::string port_name;
    swss::AlphaBool is_enable;
    double duration_time;

    if ((t1 = swss::fvtGetValue(attrs, "port_id"))
        && (t2 = swss::fvtGetValue(attrs, "Port_Name", swss::FieldNameStrategy::CASE_INSENSITIVE))
        && (t3 = swss::fvtGetValue(attrs, "is_enable")))
    {
        swss::lexical_convert({t1, t2, t3}, port_id, port_name, is_enable);
        // Do Task
    }
    if (t4 = fvtGetValue(attrs, "duration_time"))
    {
        swss::lexical_convert(t4, duration_time);
    }
    else
    {
        // Error, duration time isn't in attrs
    }
}

Depends on: #435

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

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

qiluo-msft · 2020-12-22T18:32:55Z

common/stringutility.h

@@ -0,0 +1,75 @@
+#ifndef __STRINGUTILITY__


STRINGUTILITY [](start = 8, length = 17)

The use of two underscores (__) in identifiers is reserved for the compiler's internal use according to the ANSI-C standard.
Suggest use #pragma once #Closed

qiluo-msft · 2020-12-22T19:01:06Z

common/stringutility.h

+    {
+        return false;
+    }
+    std::istringstream istream(input);


istream [](start = 23, length = 7)

How is delimiter used in the input stream? #Closed

This function is as a terminator to bool split(const std::string &input, char delimiter, T &output, Args &... args), so I assume the last part of input string should not include any delimiter.

qiluo-msft · 2020-12-22T19:02:06Z

common/stringutility.h

+}
+
+template <typename T>
+std::string join(char delimiter, const T &input)


delimiter [](start = 22, length = 9)

Not used? #Closed

Yes, it isn't used. It's a terminator of std::string join(char delimiter, const T &input, const Args &... args). So this parameters is to adapt the recursive style.
I will add qualifier, __attribute__((unused)), before this parameter delimiter.

Got it. Then better to remove the variable name and just use char. No need to add attribute.

In reply to: 547451799 [](ancestors = 547451799)

qiluo-msft · 2020-12-22T19:25:17Z

common/stringutility.h

+namespace swss {
+
+template <typename T>
+bool split(const std::string &input, char delimiter, T &output)


split [](start = 5, length = 5)

Did you try boost split and join functions? #Closed

I want to directly get some variables with expected type from the input string. I think boost split can just help us to split string into vector.Meanwhile I know we have had a helper tokenize can split string like boost split.
So what's your suggestion about replacing my code to boost split or tokenize and then convert vector to expected type? or just keep it as it is.

Suggest you reuse existing function or common libraries (stl/boost/etc) first. If you find anything missing, try improve it based on existing ones by wrapping or preprocessing.
The last resort is to implement yourself.

In reply to: 547464854 [](ancestors = 547464854)

Sorry for misleading. I mean the caller could just use tokenize or any common library function instead of your split.
They are roughly the same, I don't see significant benefit added in the split.

In reply to: 547616749 [](ancestors = 547616749,547464854)

I want a helper function that I can directly get the variable with the expected type. If the caller use tokenize or split in boost, I have to explicitly convert the string to target type at many positions.

qiluo-msft · 2020-12-22T19:51:34Z

common/stringutility.cpp

+    std::istringstream &istream,
+    bool &b)
+{
+    std::string buffer = istream.str();


str [](start = 33, length = 3)

Assume istream is extremely long, calling str() is not efficient. #Closed

qiluo-msft · 2020-12-22T19:52:06Z

common/stringutility.cpp

+        buffer.end(),
+        buffer.begin(),
+        ::tolower);
+    if (buffer == "true" || buffer == "1")


buffer [](start = 8, length = 6)

You did not consume the string in istream #Closed

qiluo-msft · 2020-12-22T20:03:50Z

common/stringutility.cpp

+    return istream;
+}
+
+bool swss::hex_to_binary(


hex_to_binary [](start = 11, length = 13)

It would be easier if you use boost unhex or stringstream + std::hex #Closed

common/stringutility.cpp

common/redisutility.h

common/rediscommand.h

qiluo-msft

As comments

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

common/stringutility.h

common/stringutility.cpp

common/stringutility.h

tests/redisutility_ut.cpp

common/schema.h

qiluo-msft · 2020-12-24T23:10:21Z

common/stringutility.h

+}
+
+template <typename T, typename... Args>
+void cast(


cast [](start = 5, length = 4)

These functions are not expected to be used by applications directly. So suggest move them into another namespace swss::xxxx_details:: #Closed

qiluo-msft · 2020-12-24T23:12:23Z

common/stringutility.h

+}
+
+template <typename T, typename... Args>
+void cast(const std::vector<std::string> &strs, T &t, Args &... args)


cast [](start = 5, length = 4)

After 2nd thought, I see cast is too ambiguous. I propose the name swss::lexical_convert. #Closed

Agree, How about to follow the boost and C++ convention? use swss::lexical_cast?

qiluo-msft · 2020-12-24T23:13:41Z

common/stringutility.h

+}
+
+template <typename T>
+void join(std::ostringstream &ostream, char, const T &t)


join [](start = 5, length = 4)

These functions are not expected to be used by applications directly. So suggest move them into another namespace swss::xxxx_details:: #Closed

qiluo-msft · 2020-12-24T23:15:49Z

common/stringutility.h

+template<typename T>
+void hex_to_binary(
+    const std::string &s,
+    T &value)


Don't break lines for short parameter list. #Closed

qiluo-msft · 2020-12-24T23:21:06Z

common/redisutility.h

+    CASE_INSENSITIVE,
+};
+
+static inline boost::optional<std::string> fvtGetValue(


inline [](start = 7, length = 6)

Don't inline non-trivial functions. Separate into cpp will make dependent project easy to build. #Closed

Then make the include headers as minimal as possible. Only for function declaration purpose, not for function implementation.

In reply to: 548762018 [](ancestors = 548762018)

qiluo-msft · 2020-12-24T23:25:38Z

common/stringutility.h

+    t = boost::lexical_cast<T>(str);
+}
+
+static inline void cast(const std::string &str, bool &b)


cast [](start = 19, length = 4)

If it is ambiguous in schema, maybe we can improve schema to use only 0/1.
Then we don't need this flexible bool casting. #Closed

I think to support "true"/"false" in schema is useful because it can be consistent with SONiC doc, like https://github.com/Azure/SONiC/blob/b16791f3ca16398153556f40b11831cfbb6aba79/thermal-control-design.md.

Only if we will revise all existed bool to 0/1, otherwise, I think, it will be a little bit confused to a new developer about how to declare a bool.

Discussed offline. I anticipate a general solution will be:

support any customized pair, even like down/up -> 0/1

never mix pair, like down/enable

We could achieve this by new classes, which could be implicitly convert to bool. They are different only when serialize/deserialize.

In reply to: 548762444 [](ancestors = 548762444)

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

qiluo-msft · 2020-12-28T01:55:24Z

common/stringutility.h

+//             throw e;
+//         }
+//     }
+// }


Remove unused code #Closed

qiluo-msft · 2020-12-28T02:00:36Z

common/boolean.h

+class AlphaBoolean : public Boolean
+{
+public:
+    static const std::string ALPHA_TRUE;


ALPHA_TRUE [](start = 29, length = 10)

TRUE_STRING #Closed

qiluo-msft · 2020-12-28T02:00:46Z

common/boolean.h

+{
+public:
+    static const std::string ALPHA_TRUE;
+    static const std::string ALPHA_FALSE;


ALPHA_FALSE [](start = 29, length = 11)

FALSE_STRING #Closed

qiluo-msft · 2020-12-28T02:01:30Z

common/boolean.cpp

+}
+
+const std::string swss::AlphaBoolean::ALPHA_TRUE = "true";
+const std::string swss::AlphaBoolean::ALPHA_FALSE = "false";


You may inline the super short functions and member constants, to it will be optimized out. #Closed

qiluo-msft · 2020-12-28T02:04:22Z

common/boolean.cpp

+
+std::ostream &swss::operator<<(std::ostream &out, const swss::AlphaBoolean &b)
+{
+    out << (b ? swss::AlphaBoolean::ALPHA_TRUE : swss::AlphaBoolean::ALPHA_FALSE);


swss::AlphaBoolean::ALPHA_TRUE [](start = 16, length = 30)

b.TRUE_STRING
If you improve the classes, you may write a generic << for all Boolean and its subclasses. #Closed

qiluo-msft · 2020-12-28T02:09:47Z

common/stringutility.h

+    t = boost::lexical_cast<T>(str);
+}
+
+// static inline void lexical_convert(const std::string &str, bool &b)


lexical_convert [](start = 22, length = 15)

Do you need to implement lexical_convert for AlphaBoolean?
If there is a default one, is it implemented by stringstream? If yes, I think you can provide a simpler one by overload here. #WontFix

Yes, it will call the default one. But I cannot understand why we need a special overload lexical_convert for AlphaBoolean?

qiluo-msft · 2020-12-28T02:10:59Z

common/boolean.cpp

+    return out;
+}
+
+std::istream &swss::operator>>(std::istream &in, swss::AlphaBoolean &b)


[](start = 28, length = 2)

For this special class, you can use in >> boolalpha >> b #Closed

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

qiluo-msft · 2020-12-28T06:15:48Z

common/boolean.h

+    Boolean(bool boolean) : m_boolean(boolean)
+    {
+    }
+    operator bool() const


operator bool() [](start = 4, length = 15)

You need another cast operator

operator bool&() { return m_boolean; } ``` #Closed

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

Add utility for string and redis

8f4f88d

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

Pterosaur force-pushed the utility_for_string_and_redis branch from 5189e4e to 8f4f88d Compare December 22, 2020 14:35

Pterosaur mentioned this pull request Dec 22, 2020

Macsec High level design sonic-net/SONiC#652

Merged

Pterosaur marked this pull request as ready for review December 22, 2020 15:10

Pterosaur mentioned this pull request Dec 22, 2020

[orchagent]: Add MACsec Orchagent sonic-net/sonic-swss#1474

Merged

caizhenghui-juniper previously approved these changes Dec 22, 2020

View reviewed changes

lguohan assigned qiluo-msft Dec 22, 2020