Hi Thuan,
Thanks. See my responses inline.
Regards, Vu
On 2/14/20 11:48 AM, Thuan Tran wrote:
Hi Vu,
Thanks for comments. Please check my replies inline.
*Best Regards,*
*ThuanTr***
*From:* Nguyen Minh Vu <vu.m.ngu...@dektech.com.au>
*Sent:* Thursday, February 13, 2020 5:50 PM
*To:* Thuan Tran <thuan.t...@dektech.com.au>; Minh Hon Chau
<minh.c...@dektech.com.au>; Thang Duc Nguyen
<thang.d.ngu...@dektech.com.au>; Gary Lee <gary....@dektech.com.au>
*Cc:* opensaf-devel@lists.sourceforge.net
*Subject:* Re: [PATCH 1/1] base: add serial number arithmetic
(RFC1982) [#3074]
Hi Thuan,
Ack with comments inline.
Regards, Vu
On 2/12/20 5:36 PM, thuan.tran wrote:
- Adapt MDS with this SNA implementation.
---
src/base/Makefile.am | 6 +-
src/base/sna.h | 136 +++++++++++++++++++++++++++++++
src/base/tests/sna_test.cc | 117 ++++++++++++++++++++++++++
src/mds/mds_tipc_fctrl_intf.cc | 2 +-
src/mds/mds_tipc_fctrl_portid.cc | 17 ++--
src/mds/mds_tipc_fctrl_portid.h | 64 +--------------
6 files changed, 267 insertions(+), 75 deletions(-)
create mode 100644 src/base/sna.h
create mode 100644 src/base/tests/sna_test.cc
diff --git a/src/base/Makefile.am b/src/base/Makefile.am
index 025fb86a2..5082175cf 100644
--- a/src/base/Makefile.am
+++ b/src/base/Makefile.am
@@ -173,7 +173,8 @@ noinst_HEADERS += \
src/base/unix_client_socket.h \
src/base/unix_server_socket.h \
src/base/unix_socket.h \
- src/base/usrbuf.h
+ src/base/usrbuf.h \
+ src/base/sna.h
TESTS += bin/testleap bin/libbase_test bin/core_common_test
@@ -237,7 +238,8 @@ bin_libbase_test_SOURCES = \
src/base/tests/time_compare_test.cc \
src/base/tests/time_convert_test.cc \
src/base/tests/time_subtract_test.cc \
- src/base/tests/unix_socket_test.cc
+ src/base/tests/unix_socket_test.cc \
+ src/base/tests/sna_test.cc
bin_libbase_test_LDADD = \
$(GTEST_DIR)/lib/libgtest.la \
diff --git a/src/base/sna.h b/src/base/sna.h
new file mode 100644
index 000000000..fee6627bb
--- /dev/null
+++ b/src/base/sna.h
@@ -0,0 +1,136 @@
+/* -*- OpenSAF -*-
+ *
+ * Copyright Ericsson AB 2020 - All Rights Reserved.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY
+ * or FITNESS FOR A PARTICULAR PURPOSE. This file and program are licensed
+ * under the GNU Lesser General Public License Version 2.1, February 1999.
+ * The complete license can be accessed from the following location:
+ *http://opensource.org/licenses/lgpl-license.php
+ * See the Copying file included with the OpenSAF distribution for full
+ * licensing terms.
+ *
+ * Reference: Serial Number Arithmetic from RFC1982
+ *
+ */
+
+#ifndef BASE_SNA_H_
+#define BASE_SNA_H_
+
+#include <typeinfo>
+#include <stdexcept>
+
+#define SNA16_MAX 65536 // 2^16
+#define SNA16_SPACE 32768 // (2^16)/2
+#define SNA32_MAX 4294967296 // 2^32
+#define SNA32_SPACE 2147483648 // (2^32)/2
[Vu] can use:
#define SNA16_MAX (1 << 16)
#define SNA32_MAX (1 << 32)
SPACE ones probably are not necessary. See my comment for space()
method below.
[Thuan] Is there any benefit with 1 << 16 or 32? I think define a
clear value is better.
[Vu] the benefit is you won't need to add the comment e.g. // 2^16
About space(), it is intended because I don’t want CPU calculate every
time refer to it.
+
+template <class T>
+class _sna {
[Vu] How about `class SerialNumber {}`
[Thuan] OK, I will change the class name.
+ private:
[Vu] Declaration order should start with a |public:| section, followed
by |protected:|, then |private:|
[Thuan] OK, will change order.
+ T i;
[Vu] Should use a descriptive name. e.g:
T value_ {0};
[Thuan] OK, will update the name.
+ uint64_t max() {
+ if (typeid(T) == typeid(uint64_t)) {
+ return SNA32_MAX;
+ }
+ if (typeid(T) == typeid(uint32_t)) {
+ return SNA16_MAX;
+ }
+ throw std::out_of_range("Invalid type");
[Vu] OpenSAF code does not handle exception. Should use assertion instead.
e.g: assert(0 && "Invalid data type");
[Thuan] I throw to do basic test, if assert() then cannot test the case.
Btw, I think throw exception without try catch will also end with
terminate?
[Vu] I think so; and if you you do tests on Seq16, Seq32, you probably
won't reach exceptions.
+ }
+ uint64_t space() {
+ if (typeid(T) == typeid(uint64_t)) {
+ return SNA32_SPACE;
+ }
+ if (typeid(T) == typeid(uint32_t)) {
+ return SNA16_SPACE;
+ }
+ throw std::out_of_range("Invalid type");
[Vu] uint64_t space() { return max()/2;}
[Thuan] As explain above, I don’t want calculate every time refer it.
+ }
+
+ public:
+ _sna(): i(0) {}
+ _sna(const _sna &t) {
+ i = t.i;
+ }
+ explicit _sna(const uint64_t &n) {
+ if ((n < 0) || (n > (max()-1)))
+ throw std::out_of_range("Invalid initial value");
[Vu] Use assert() instead of throwing exception.
[Thuan] Same above.
+ i = n;
+ }
+ _sna& operator=(const _sna &t) {
+ // check for self-assignment
+ if (&t == this)
+ return *this;
+ i = t.i;
+ return *this;
+ }
+ T v() const {
+ return i;
+ }
+ _sna& operator+=(const uint64_t& n) {
+ if ((n < 0) || (n > (space() - 1)))
+ throw std::out_of_range("Invalid addition value");
+ i = (i + n) % max();
+ return *this;
+ }
+ friend _sna operator+(_sna m, const uint64_t& n) {
[Vu] my suggestion:
friend _sna operator+(const _sna& m, ...) {
_sna s{m};
s += n;
return s;
}
[Thuan] passing lhs by value helps optimize chained a+b+c
I prefer to this link
https://en.cppreference.com/w/cpp/language/operators
<https://en.cppreference.com/w/cpp/language/operators>
+ m += n;
+ return m;
+ }
+ // prefix ++
+ _sna& operator++() {
+ *this += 1;
+ return *this;
+ }
+ // postfix ++
+ _sna operator++(int) {
+ _sna tmp(*this);
+ operator++();
+ return tmp;
+ }
+ bool operator==(const _sna& rhs) {
+ return i == rhs.i;
+ }
+ bool operator==(const uint32_t val) {
+ return i == val;
+ }
+ bool operator!=(const _sna& rhs) {
+ return i != rhs.i;
+ }
+ bool operator<(const _sna& rhs) {
+ return (i < rhs.i && rhs.i - i < space()) || \
+ (i > rhs.i && i - rhs.i > space());
+ }
+ bool operator<=(const _sna& rhs) {
+ return *this == rhs || *this < rhs;
+ }
+ bool operator>(const _sna& rhs) {
+ return (i < rhs.i && rhs.i - i > space()) || \
+ (i > rhs.i && i - rhs.i < space());
[Vu] shorter version:
return !(*this <= rhs);
[Thuan] This statement cause the issue in Erase message which run test
sometimes failure.
+ }
+ bool operator>=(const _sna& rhs) {
+ return *this == rhs || *this > rhs;
+ }
[Vu] Shorter version:
return !(*this < rhs);
[Thuan] This statement cause the issue in Erase message which run test
sometimes failure.
+ int64_t operator-(const _sna& rhs) {
+ if (*this >= rhs) {
+ if (i >= rhs.v()) {
+ return i - rhs.v();
+ } else {
+ return (i + max()) - rhs.v();
+ }
+ } else {
+ if (i < rhs.v()) {
+ return i - rhs.v();
+ } else {
+ return i - (rhs.v() + max());
+ }
+ }
+ }
+};
+
+typedef _sna<uint32_t> Seq16;
+typedef _sna<uint64_t> Seq32;
[Vu] another option:
using Seq16 = _sna<uint32_t>;
[Thuan] Is there any benefit with this instead of typedef?
[Vu] 'using' is a recommendation in c++ google coding rule.
https://google.github.io/styleguide/cppguide.html#Aliases
+
+#endif // BASE_SNA_H_
diff --git a/src/base/tests/sna_test.cc b/src/base/tests/sna_test.cc
new file mode 100644
index 000000000..b3d2d014c
--- /dev/null
+++ b/src/base/tests/sna_test.cc
@@ -0,0 +1,117 @@
+/* -*- OpenSAF -*-
+ *
+ * Copyright Ericsson AB 2019 - All Rights Reserved.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY
+ * or FITNESS FOR A PARTICULAR PURPOSE. This file and program are licensed
+ * under the GNU Lesser General Public License Version 2.1, February 1999.
+ * The complete license can be accessed from the following location:
+ *http://opensource.org/licenses/lgpl-license.php
+ * See the Copying file included with the OpenSAF distribution for full
+ * licensing terms.
+ *
+ * Reference: Serial Number Arithmetic from RFC1982
+ *
+ */
+
+#include "base/sna.h"
+#include "gtest/gtest.h"
+
+template <class T>
+int test_sna(T x) {
+ int rc = 1;
+ printf("\n============= START with x=%lu =============\n",
(uint64_t)x.v());
+ T y = x;
+ printf("x=%lu, y=%lu: check x == y++ is TRUE\n",
+ (uint64_t)x.v(), (uint64_t)y.v());
+ if (x == y++) {
+ printf("now y=%lu, reset y = x\n", (uint64_t)y.v());
+ y = x;
+ printf("x=%lu, y=%lu: check x != ++y is TRUE\n",
+ (uint64_t)x.v(), (uint64_t)y.v());
+ if (x != ++y) {
+ printf("now y=%lu, reset y = x\n", (uint64_t)y.v());
+ y = x;
+ printf("x=%lu, y=%lu: check x < ++y is TRUE\n",
+ (uint64_t)x.v(), (uint64_t)y.v());
+ if (x < ++y) {
+ printf("x=%lu: check x + 1 > x and x + 1 >= x is TRUE\n",
+ (uint64_t)x.v());
+ if ((x + 1 > x) && (x + 1 >= x)) {
+ printf("x=%lu: check x < x + 1 and x <= x + 1 is TRUE\n",
+ (uint64_t)x.v());
+ y = x + 1;
+ printf("y = x+1 => y=%lu\n", (uint64_t)y.v());
+ y = y + 1;
+ printf("y = y+1 => y=%lu\n", (uint64_t)y.v());
+ if ((x < x + 1) && (x <= x + 1)) {
+ try {
+ printf("x=%lu: add invalid (-1)\n", (uint64_t)x.v());
+ x = x + (-1);
+ } catch (const std::out_of_range& oor) {
+ printf("Expected error: %s\n", oor.what());
+ try {
+ uint64_t max_value = 0;
+ if (typeid(T) == typeid(Seq16))
+ max_value = SNA16_MAX;
+ else if (typeid(T) == typeid(Seq32))
+ max_value = SNA32_MAX;
+ printf("x=%lu: add invalid (%lu)\n",
+ (uint64_t)x.v(), max_value);
+ x = x + max_value;
+ } catch (const std::out_of_range& oor) {
+ printf("Expected error: %s\n", oor.what());
+ rc = 0;
+ }
+ }
+ }
+ }
+ }
+ }
+ }
+ printf("================ END with x=%lu ==============\n",
(uint64_t)x.v());
[Vu] Should remove all printf(); Developer can add it when following
macros got failures.
[Thuan] Is there any problem if I keep it?
[Vu] I think printf should only be using in 'debug' version.
[Vu] However, I think we should test on `Seq16` and `Seq32` data type
only.
[Thuan] Yes, I test Seq16 and Seq32.
+ return rc;
+}
+
+
+class SnaTest : public ::testing::Test {
+ protected:
+ SnaTest() {}
+ virtual ~SnaTest() {
+ // Cleanup work that doesn't throw exceptions here.
+ }
+ virtual void SetUp() {
+ // Code here will be called immediately after the constructor (right
+ // before each test)
+ }
+ virtual void TearDown() {}
+};
+
[Vu] Probably don't need this class.
[Thuan] I just keep them as other test files in base.
+TEST_F(SnaTest, unit16_sna) {
+ Seq16 x;
+ EXPECT_EQ(0, test_sna(x));
+ Seq16 x1 = Seq16(1);
+ Seq16 x2 = Seq16(SNA16_MAX - 1);
+ EXPECT_EQ(2, x1 - x2);
+ EXPECT_EQ(-2, x2 - x1);
+ EXPECT_EQ(0, test_sna(x1));
+ EXPECT_EQ(0, test_sna(x2));
[Vu] The production code only use Seq16 & Seq32; so I think we only do
test on these data types.
[Thuan] Yes, I only test Seq16 & Seq32.
+}
+
+TEST_F(SnaTest, unit32_sna) {
+ Seq32 x;
+ EXPECT_EQ(0, test_sna(x));
+ Seq32 x1 = Seq32(1);
+ Seq32 x2 = Seq32(SNA32_MAX - 1);
+ EXPECT_EQ(2, x1 - x2);
+ EXPECT_EQ(-2, x2 - x1);
+ EXPECT_EQ(0, test_sna(x1));
+ EXPECT_EQ(0, test_sna(x2));
+}
+
+
+int main(int argc, char **argv) {
+ ::testing::InitGoogleTest(&argc, argv);
+ return RUN_ALL_TESTS();
+}
diff --git a/src/mds/mds_tipc_fctrl_intf.cc b/src/mds/mds_tipc_fctrl_intf.cc
index f3883ba36..9c7d4e35c 100644
--- a/src/mds/mds_tipc_fctrl_intf.cc
+++ b/src/mds/mds_tipc_fctrl_intf.cc
@@ -106,7 +106,7 @@ void process_timer_event(const Event& evt) {
static_cast<int>(evt.type_));
for (auto i : portid_map) {
TipcPortId* portid = i.second;
-
+ if (!portid) continue;
if (evt.type_ == Event::Type::kEvtTmrTxProb) {
if (portid->ReceiveTmrTxProb(kTxProbMaxRetries) == true) {
txprob_restart = true;
diff --git a/src/mds/mds_tipc_fctrl_portid.cc
b/src/mds/mds_tipc_fctrl_portid.cc
index 3562c4a00..b0f765d69 100644
--- a/src/mds/mds_tipc_fctrl_portid.cc
+++ b/src/mds/mds_tipc_fctrl_portid.cc
@@ -339,7 +339,7 @@ uint32_t TipcPortId::ReceiveData(uint32_t mseq,
uint16_t mfrag,
}
// update receiver sequence window
- if (rcvwnd_.acked_ < Seq16(fseq) && rcvwnd_.rcv_ + Seq16(1) ==
Seq16(fseq)) {
+ if (rcvwnd_.acked_ < Seq16(fseq) && rcvwnd_.rcv_ + 1 == Seq16(fseq)) {
m_MDS_LOG_DBG("FCTRL: [me] <-- [node:%x, ref:%u], "
"RcvData[mseq:%u, mfrag:%u, fseq:%u], "
"rcvwnd[acked:%u, rcv:%u, nacked:%" PRIu64 "]",
@@ -370,7 +370,7 @@ uint32_t TipcPortId::ReceiveData(uint32_t mseq,
uint16_t mfrag,
// It is not used for now, so ignore it.
// check for transmission error
- if (rcvwnd_.rcv_ + Seq16(1) < Seq16(fseq)) {
+ if (rcvwnd_.rcv_ + 1 < Seq16(fseq)) {
if (rcvwnd_.rcv_ == 0 && rcvwnd_.acked_ == 0) {
// peer does not realize that this portid reset
m_MDS_LOG_ERR("FCTRL: [me] <-- [node:%x, ref:%u], "
@@ -381,7 +381,7 @@ uint32_t TipcPortId::ReceiveData(uint32_t mseq,
uint16_t mfrag,
mseq, mfrag, fseq,
rcvwnd_.acked_.v(), rcvwnd_.rcv_.v(), rcvwnd_.nacked_space_);
- rcvwnd_.rcv_ = fseq;
+ rcvwnd_.rcv_ = Seq16(fseq);
} else {
rc = NCSCC_RC_FAILURE;
// msg loss
@@ -393,7 +393,7 @@ uint32_t TipcPortId::ReceiveData(uint32_t mseq,
uint16_t mfrag,
mseq, mfrag, fseq,
rcvwnd_.acked_.v(), rcvwnd_.rcv_.v(), rcvwnd_.nacked_space_);
// send nack
- SendNack((rcvwnd_.rcv_ + Seq16(1)).v(), svc_id);
+ SendNack((rcvwnd_.rcv_ + 1).v(), svc_id);
}
} else if (Seq16(fseq) <= rcvwnd_.rcv_) {
rc = NCSCC_RC_FAILURE;
@@ -456,13 +456,12 @@ void TipcPortId::ReceiveChunkAck(uint16_t fseq,
uint16_t chksize) {
sndwnd_.acked_.v(), sndwnd_.send_.v(), sndwnd_.nacked_space_,
sndqueue_.Size());
- // fast forward the sndwnd_.acked_ sequence to fseq
- sndwnd_.acked_ = fseq;
-
// remove a number @chksize messages out of sndqueue_ and decrease
// the nacked_space_ of sender
- uint64_t acked_bytes = sndqueue_.Erase(Seq16(fseq) - (chksize-1),
- Seq16(fseq));
+ uint64_t acked_bytes = sndqueue_.Erase(sndwnd_.acked_ + 1,
Seq16(fseq));
+ // fast forward the sndwnd_.acked_ sequence to fseq
+ sndwnd_.acked_ = Seq16(fseq);
+
assert(sndwnd_.nacked_space_ >= acked_bytes);
sndwnd_.nacked_space_ -= acked_bytes;
diff --git a/src/mds/mds_tipc_fctrl_portid.h
b/src/mds/mds_tipc_fctrl_portid.h
index ef74921e7..83564459b 100644
--- a/src/mds/mds_tipc_fctrl_portid.h
+++ b/src/mds/mds_tipc_fctrl_portid.h
@@ -24,73 +24,11 @@
#include <stdio.h>
#include <unistd.h>
#include <deque>
+#include "base/sna.h"
#include "mds/mds_tipc_fctrl_msg.h"
namespace mds {
-class Seq16 {
- public:
-#define SEQ16_MAX 65536
-#define SEQ16_SPACE 32768
- uint16_t value_;
- explicit Seq16(uint16_t v) {
- value_ = uint16_t((uint32_t)v % SEQ16_MAX);
- }
- uint16_t v() {
- return value_;
- }
- Seq16 operator + (const Seq16 add) const {
- return Seq16(((uint32_t)value_ + (uint32_t)add.value_) % SEQ16_MAX);
- }
-
- int16_t operator - (const Seq16 sub) const {
- if (value_ < sub.value_ && (sub.value_ - value_ < SEQ16_SPACE)) {
- return value_ - sub.value_;
- }
- if (value_ > sub.value_ && (value_ - sub.value_ > SEQ16_SPACE)) {
- return (int32_t)value_ + SEQ16_MAX - (int32_t)sub.value_;
- }
- if (value_ < sub.value_ && (sub.value_ - value_ > SEQ16_SPACE)) {
- return (int32_t)value_ + SEQ16_MAX - (int32_t)sub.value_;
- }
- if (value_ > sub.value_ && (value_ - sub.value_ < SEQ16_SPACE)) {
- return value_ - sub.value_;
- }
- return 0;
- }
- Seq16 operator - (const uint16_t sub) const {
- return Seq16(((uint32_t)value_ + 65536 - sub) % SEQ16_MAX);
- }
- void operator ++() {
- value_ = (value_ + 1) % SEQ16_MAX;
- }
- void operator = (const uint16_t v) {
- value_ = v % SEQ16_MAX;
- }
- bool operator == (const Seq16& seq) const {
- return value_ == seq.value_;
- }
- bool operator == (uint16_t val) const {
- return value_ == val;
- }
- bool operator <= (const Seq16& seq) {
- return *this == seq || *this < seq;
- }
- bool operator < (const Seq16& seq) {
- if (value_ < seq.value_ && (seq.value_ - value_ < SEQ16_SPACE)) return
true;
- if (value_ > seq.value_ && (value_ - seq.value_ > SEQ16_SPACE)) return
true;
- return false;
- }
- bool operator > (const Seq16& seq) {
- if (value_ < seq.value_ && (seq.value_ - value_ > SEQ16_SPACE)) return
true;
- if (value_ > seq.value_ && (value_ - seq.value_ < SEQ16_SPACE)) return
true;
- return false;
- }
- bool operator >= (const Seq16& seq) {
- return *this == seq || *this > seq;
- }
-};
-
class MessageQueue {
public:
void Queue(DataMessage* msg);
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
