Re: [devel] [PATCH 1/1] mds: fix memleak in agent enable flow control [#3151]

2020-02-12 Thread Minh Hon Chau 

Hi Thuan,

Ack from me.

Thanks

Minh

On 12/2/20 9:29 pm, thuan.tran wrote:

Agent enable flow control keep add new portid without remove.
Remove portid when svc count become zero then handle portid reset
properly, peer A may see portid reset (peer B) then peer B should
accept fseq(1) message from peer A.
---
  src/mds/mds_tipc_fctrl_intf.cc   |  6 ++
  src/mds/mds_tipc_fctrl_portid.cc | 17 -
  2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/src/mds/mds_tipc_fctrl_intf.cc b/src/mds/mds_tipc_fctrl_intf.cc
index f3883ba36..f3504b901 100644
--- a/src/mds/mds_tipc_fctrl_intf.cc
+++ b/src/mds/mds_tipc_fctrl_intf.cc
@@ -428,6 +428,12 @@ uint32_t mds_tipc_fctrl_portid_down(struct tipc_portid id, 
uint32_t type) {
  portid->svc_cnt_--;
  m_MDS_LOG_DBG("FCTRL: Remove svc[node:%x, ref:%u svc_id:%u], svc_cnt:%u",
  id.node, id.ref, svc_id, portid->svc_cnt_);
+if (portid->svc_cnt_ == 0) {
+  delete portid;
+  portid_map.erase(TipcPortId::GetUniqueId(id));
+  m_MDS_LOG_NOTIFY("FCTRL: Remove portid[node:%x, ref:%u]",
+  id.node, id.ref);
+}
}
portid_map_mutex.unlock();
  
diff --git a/src/mds/mds_tipc_fctrl_portid.cc b/src/mds/mds_tipc_fctrl_portid.cc

index 3562c4a00..57843b6de 100644
--- a/src/mds/mds_tipc_fctrl_portid.cc
+++ b/src/mds/mds_tipc_fctrl_portid.cc
@@ -373,7 +373,7 @@ uint32_t TipcPortId::ReceiveData(uint32_t mseq, uint16_t 
mfrag,
  if (rcvwnd_.rcv_ + Seq16(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], "
+m_MDS_LOG_NOTIFY("FCTRL: [me] <-- [node:%x, ref:%u], "
  "RcvData[mseq:%u, mfrag:%u, fseq:%u], "
  "rcvwnd[acked:%u, rcv:%u, nacked:%" PRIu64 "], "
  "Warning[portid reset]",
@@ -381,7 +381,9 @@ uint32_t TipcPortId::ReceiveData(uint32_t mseq, uint16_t 
mfrag,
  mseq, mfrag, fseq,
  rcvwnd_.acked_.v(), rcvwnd_.rcv_.v(), rcvwnd_.nacked_space_);
  
+SendChunkAck(fseq, svc_id, 1);

  rcvwnd_.rcv_ = fseq;
+rcvwnd_.acked_ = rcvwnd_.rcv_;
} else {
  rc = NCSCC_RC_FAILURE;
  // msg loss
@@ -395,6 +397,19 @@ uint32_t TipcPortId::ReceiveData(uint32_t mseq, uint16_t 
mfrag,
  // send nack
  SendNack((rcvwnd_.rcv_ + Seq16(1)).v(), svc_id);
}
+} else if (fseq == 1) {
+  // sender realize me as portid reset
+  m_MDS_LOG_NOTIFY("FCTRL: [me] <-- [node:%x, ref:%u], "
+  "RcvData[mseq:%u, mfrag:%u, fseq:%u], "
+  "rcvwnd[acked:%u, rcv:%u, nacked:%" PRIu64 "], "
+  "Warning[portid reset on sender]",
+  id_.node, id_.ref,
+  mseq, mfrag, fseq,
+  rcvwnd_.acked_.v(), rcvwnd_.rcv_.v(), rcvwnd_.nacked_space_);
+
+  SendChunkAck(fseq, svc_id, 1);
+  rcvwnd_.rcv_ = fseq;
+  rcvwnd_.acked_ = rcvwnd_.rcv_;
  } else if (Seq16(fseq) <= rcvwnd_.rcv_) {
rc = NCSCC_RC_FAILURE;
// unexpected retransmission



___
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel

[devel] [PATCH 1/1] mds: fix memleak in agent enable flow control [#3151]

2020-02-12 Thread thuan.tran 
Agent enable flow control keep add new portid without remove.
Remove portid when svc count become zero then handle portid reset
properly, peer A may see portid reset (peer B) then peer B should
accept fseq(1) message from peer A.
---
 src/mds/mds_tipc_fctrl_intf.cc   |  6 ++
 src/mds/mds_tipc_fctrl_portid.cc | 17 -
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/src/mds/mds_tipc_fctrl_intf.cc b/src/mds/mds_tipc_fctrl_intf.cc
index f3883ba36..f3504b901 100644
--- a/src/mds/mds_tipc_fctrl_intf.cc
+++ b/src/mds/mds_tipc_fctrl_intf.cc
@@ -428,6 +428,12 @@ uint32_t mds_tipc_fctrl_portid_down(struct tipc_portid id, 
uint32_t type) {
 portid->svc_cnt_--;
 m_MDS_LOG_DBG("FCTRL: Remove svc[node:%x, ref:%u svc_id:%u], svc_cnt:%u",
 id.node, id.ref, svc_id, portid->svc_cnt_);
+if (portid->svc_cnt_ == 0) {
+  delete portid;
+  portid_map.erase(TipcPortId::GetUniqueId(id));
+  m_MDS_LOG_NOTIFY("FCTRL: Remove portid[node:%x, ref:%u]",
+  id.node, id.ref);
+}
   }
   portid_map_mutex.unlock();
 
diff --git a/src/mds/mds_tipc_fctrl_portid.cc b/src/mds/mds_tipc_fctrl_portid.cc
index 3562c4a00..57843b6de 100644
--- a/src/mds/mds_tipc_fctrl_portid.cc
+++ b/src/mds/mds_tipc_fctrl_portid.cc
@@ -373,7 +373,7 @@ uint32_t TipcPortId::ReceiveData(uint32_t mseq, uint16_t 
mfrag,
 if (rcvwnd_.rcv_ + Seq16(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], "
+m_MDS_LOG_NOTIFY("FCTRL: [me] <-- [node:%x, ref:%u], "
 "RcvData[mseq:%u, mfrag:%u, fseq:%u], "
 "rcvwnd[acked:%u, rcv:%u, nacked:%" PRIu64 "], "
 "Warning[portid reset]",
@@ -381,7 +381,9 @@ uint32_t TipcPortId::ReceiveData(uint32_t mseq, uint16_t 
mfrag,
 mseq, mfrag, fseq,
 rcvwnd_.acked_.v(), rcvwnd_.rcv_.v(), rcvwnd_.nacked_space_);
 
+SendChunkAck(fseq, svc_id, 1);
 rcvwnd_.rcv_ = fseq;
+rcvwnd_.acked_ = rcvwnd_.rcv_;
   } else {
 rc = NCSCC_RC_FAILURE;
 // msg loss
@@ -395,6 +397,19 @@ uint32_t TipcPortId::ReceiveData(uint32_t mseq, uint16_t 
mfrag,
 // send nack
 SendNack((rcvwnd_.rcv_ + Seq16(1)).v(), svc_id);
   }
+} else if (fseq == 1) {
+  // sender realize me as portid reset
+  m_MDS_LOG_NOTIFY("FCTRL: [me] <-- [node:%x, ref:%u], "
+  "RcvData[mseq:%u, mfrag:%u, fseq:%u], "
+  "rcvwnd[acked:%u, rcv:%u, nacked:%" PRIu64 "], "
+  "Warning[portid reset on sender]",
+  id_.node, id_.ref,
+  mseq, mfrag, fseq,
+  rcvwnd_.acked_.v(), rcvwnd_.rcv_.v(), rcvwnd_.nacked_space_);
+
+  SendChunkAck(fseq, svc_id, 1);
+  rcvwnd_.rcv_ = fseq;
+  rcvwnd_.acked_ = rcvwnd_.rcv_;
 } else if (Seq16(fseq) <= rcvwnd_.rcv_) {
   rc = NCSCC_RC_FAILURE;
   // unexpected retransmission
-- 
2.17.1



___
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel

[devel] [PATCH 0/1] Review Request for mds: fix memleak in agent enable flow control [#3151]

2020-02-12 Thread thuan.tran 
Summary: mds: fix memleak in agent enable flow control [#3151]
Review request for Ticket(s): 3151
Peer Reviewer(s): Minh, Thang, Vu, Gary
Pull request to: *** LIST THE PERSON WITH PUSH ACCESS HERE ***
Affected branch(es): develop
Development branch: ticket-3151
Base revision: 09604b503698a6257272668ae982f03f76836fa7
Personal repository: git://git.code.sf.net/u/thuantr/review


Impacted area   Impact y/n

 Docsn
 Build systemn
 RPM/packaging   n
 Configuration files n
 Startup scripts n
 SAF servicesn
 OpenSAF servicesn
 Core libraries  y
 Samples n
 Tests   n
 Other   n


Comments (indicate scope for each "y" above):
-
N/A

revision b8ab2c8a180b5b1ba110a02ecd60a1001ebddbc6
Author: thuan.tran 
Date:   Wed, 12 Feb 2020 16:11:58 +0700

mds: fix memleak in agent enable flow control [#3151]

Agent enable flow control keep add new portid without remove.
Remove portid when svc count become zero then handle portid reset
properly, peer A may see portid reset (peer B) then peer B should
accept fseq(1) message from peer A.



Complete diffstat:
--
 src/mds/mds_tipc_fctrl_intf.cc   |  6 ++
 src/mds/mds_tipc_fctrl_portid.cc | 17 -
 2 files changed, 22 insertions(+), 1 deletion(-)


Testing Commands:
-
N/A

Testing, Expected Results:
--
N/A

Conditions of Submission:
-
ACK by reviewers

Arch  Built StartedLinux distro
---
mipsn  n
mips64  n  n
x86 n  n
x86_64  y  y
powerpc n  n
powerpc64   n  n


Reviewer Checklist:
---
[Submitters: make sure that your review doesn't trigger any checkmarks!]


Your checkin has not passed review because (see checked entries):

___ Your RR template is generally incomplete; it has too many blank entries
that need proper data filled in.

___ You have failed to nominate the proper persons for review and push.

___ Your patches do not have proper short+long header

___ You have grammar/spelling in your header that is unacceptable.

___ You have exceeded a sensible line length in your headers/comments/text.

___ You have failed to put in a proper Trac Ticket # into your commits.

___ You have incorrectly put/left internal data in your comments/files
(i.e. internal bug tracking tool IDs, product names etc)

___ You have not given any evidence of testing beyond basic build tests.
Demonstrate some level of runtime or other sanity testing.

___ You have ^M present in some of your files. These have to be removed.

___ You have needlessly changed whitespace or added whitespace crimes
like trailing spaces, or spaces before tabs.

___ You have mixed real technical changes with whitespace and other
cosmetic code cleanup changes. These have to be separate commits.

___ You need to refactor your submission into logical chunks; there is
too much content into a single commit.

___ You have extraneous garbage in your review (merge commits etc)

___ You have giant attachments which should never have been sent;
Instead you should place your content in a public tree to be pulled.

___ You have too many commits attached to an e-mail; resend as threaded
commits, or place in a public tree for a pull.

___ You have resent this content multiple times without a clear indication
of what has changed between each re-send.

___ You have failed to adequately and individually address all of the
comments and change requests that were proposed in the initial review.

___ You have a misconfigured ~/.gitconfig file (i.e. user.name, user.email etc)

___ Your computer have a badly configured date and time; confusing the
the threaded patch review.

___ Your changes affect IPC mechanism, and you don't present any results
for in-service upgradability test.

___ Your changes affect user manual and documentation, your patch series
do not contain the patch that updates the Doxygen manual.



___
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel

[devel] [PATCH 0/1] Review Request for base: add serial number arithmetic (RFC1982) [#3074]

2020-02-12 Thread thuan.tran 
Summary: base: add serial number arithmetic (RFC1982) [#3074]
Review request for Ticket(s): 3074
Peer Reviewer(s): Minh, Thang, Vu, Gary
Pull request to: *** LIST THE PERSON WITH PUSH ACCESS HERE ***
Affected branch(es): develop
Development branch: ticket-3074
Base revision: 09604b503698a6257272668ae982f03f76836fa7
Personal repository: git://git.code.sf.net/u/thuantr/review


Impacted area   Impact y/n

 Docsn
 Build systemn
 RPM/packaging   n
 Configuration files n
 Startup scripts n
 SAF servicesn
 OpenSAF servicesn
 Core libraries  y
 Samples n
 Tests   n
 Other   n


Comments (indicate scope for each "y" above):
-
N/A

revision e2e9023da84dc141feebd3fdae8650d85ea19403
Author: thuan.tran 
Date:   Wed, 12 Feb 2020 13:24:44 +0700

base: add serial number arithmetic (RFC1982) [#3074]

- Adapt MDS with this SNA implementation.



Added Files:

 src/base/sna.h
 src/base/tests/sna_test.cc


Complete diffstat:
--
 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(-)


Testing Commands:
-
N/A

Testing, Expected Results:
--
N/A

Conditions of Submission:
-
ACK by reviewers

Arch  Built StartedLinux distro
---
mipsn  n
mips64  n  n
x86 n  n
x86_64  y  y
powerpc n  n
powerpc64   n  n


Reviewer Checklist:
---
[Submitters: make sure that your review doesn't trigger any checkmarks!]


Your checkin has not passed review because (see checked entries):

___ Your RR template is generally incomplete; it has too many blank entries
that need proper data filled in.

___ You have failed to nominate the proper persons for review and push.

___ Your patches do not have proper short+long header

___ You have grammar/spelling in your header that is unacceptable.

___ You have exceeded a sensible line length in your headers/comments/text.

___ You have failed to put in a proper Trac Ticket # into your commits.

___ You have incorrectly put/left internal data in your comments/files
(i.e. internal bug tracking tool IDs, product names etc)

___ You have not given any evidence of testing beyond basic build tests.
Demonstrate some level of runtime or other sanity testing.

___ You have ^M present in some of your files. These have to be removed.

___ You have needlessly changed whitespace or added whitespace crimes
like trailing spaces, or spaces before tabs.

___ You have mixed real technical changes with whitespace and other
cosmetic code cleanup changes. These have to be separate commits.

___ You need to refactor your submission into logical chunks; there is
too much content into a single commit.

___ You have extraneous garbage in your review (merge commits etc)

___ You have giant attachments which should never have been sent;
Instead you should place your content in a public tree to be pulled.

___ You have too many commits attached to an e-mail; resend as threaded
commits, or place in a public tree for a pull.

___ You have resent this content multiple times without a clear indication
of what has changed between each re-send.

___ You have failed to adequately and individually address all of the
comments and change requests that were proposed in the initial review.

___ You have a misconfigured ~/.gitconfig file (i.e. user.name, user.email etc)

___ Your computer have a badly configured date and time; confusing the
the threaded patch review.

___ Your changes affect IPC mechanism, and you don't present any results
for in-service upgradability test.

___ Your changes affect user manual and documentation, your patch series
do not contain the patch that updates the Doxygen manual.



___
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel

[devel] [PATCH 1/1] base: add serial number arithmetic (RFC1982) [#3074]

2020-02-12 Thread thuan.tran 
- 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 0..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 
+#include 
+
+#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
+
+template 
+class _sna {
+ private:
+  T i;
+  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");
+  }
+  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");
+  }
+
+ public:
+  _sna(): i(0) {}
+  _sna(const _sna ) {
+i = t.i;
+  }
+  explicit _sna(const uint64_t ) {
+if ((n < 0) || (n > (max()-1)))
+  throw std::out_of_range("Invalid initial value");
+i = n;
+  }
+  _sna& operator=(const _sna ) {
+// check for self-assignment
+if ( == 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) {
+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());
+  }
+  bool operator>=(const _sna& rhs) {
+return *this == rhs || *this > rhs;
+  }
+  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 Seq16;
+typedef _sna Seq32;
+
+#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 0..b3d2d014c
--- /dev/null
+++ b/src/base/tests/sna_test.cc
@@ -0,0 +1,117 @@
+/*  -*- OpenSAF  -*-