Hi Thuan,
Please see comments inline.
Thanks
Minh
On 7/10/19 3:18 pm, Tran Thuan wrote:
Hi Minh,
Some minor comments from me, check [Thuan] inline.
Thanks.
Best Regards,
ThuanTr
-----Original Message-----
From: Minh Chau <minh.c...@dektech.com.au>
Sent: Monday, October 7, 2019 7:12 AM
To: hans.nordeb...@ericsson.com; vu.m.ngu...@dektech.com.au;
gary....@dektech.com.au; thuan.t...@dektech.com.au
Cc: opensaf-devel@lists.sourceforge.net; Minh Chau
<minh.c...@dektech.com.au>
Subject: [PATCH 1/1] mds: Enhance decoding for mds flow control message
[#3097]
mds currently uses MDS_PROT_FCTRL_ID 4 bytes value (0x00AC13F5) from octet11
to octet14 to identify the flow control message e.g., chunkack message. In
case of fragmentation from big message, the second fragment onwards will
start from the octet11, which may have arbitrary value and cause mds to
incorrectly decode as a flow control message if the fragment starts with
value of 0x00AC13F5.
This patch fixes this rare case by decoding flow control message only if the
oct2-5 (mds global sequence number) and oct6-7 (mds fragment number) are
non-zero. Change MDS_PROT_FCTRL_ID:0xFDAC13F5
[Thuan]: typo "non-zero" -> "zero"?
[Minh]: Yes, typo, it's "zero"
[Thuan] Can you give info in commit message about why change
MDS_PROT_FCTRL_ID to FDAC13F5?
[Minh]: It is only a random number for identifier, but 0x00AC will
occupy the oct11&12 which is msd header length, and may cause a higher
probability to be identical
---
src/mds/mds_dt.h | 2 +-
src/mds/mds_tipc_fctrl_msg.cc | 20 +++++++++++++-------
2 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/src/mds/mds_dt.h b/src/mds/mds_dt.h index d9e8633..64da600
100644
--- a/src/mds/mds_dt.h
+++ b/src/mds/mds_dt.h
@@ -245,7 +245,7 @@ bool mdtm_mailbox_mbx_cleanup(NCSCONTEXT arg, NCSCONTEXT
msg);
/* MDS protocol/version for flow control */ #define MDS_PROT_FCTRL (0xB0 |
MDS_VERSION) -#define MDS_PROT_FCTRL_ID 0x00AC13F5
+#define MDS_PROT_FCTRL_ID 0xFDAC13F5
/* Added for the subscription changes */ #define MDS_NCS_CHASSIS_ID
(m_NCS_GET_NODE_ID & 0x00ff0000) diff --git a/src/mds/mds_tipc_fctrl_msg.cc
b/src/mds/mds_tipc_fctrl_msg.cc index 064d977..8375673 100644
--- a/src/mds/mds_tipc_fctrl_msg.cc
+++ b/src/mds/mds_tipc_fctrl_msg.cc
@@ -64,13 +64,19 @@ void HeaderMessage::Decode(uint8_t *msg) {
// decode flow control sequence number
ptr = &msg[HeaderMessage::FieldIndex::kFlowControlSequenceNumber];
fseq_ = ncs_decode_16bit(&ptr);
- // decode protocol identifier
- ptr = &msg[ChunkAck::FieldIndex::kProtocolIdentifier];
- pro_id_ = ncs_decode_32bit(&ptr);
- if (pro_id_ == MDS_PROT_FCTRL_ID) {
- // decode message type
- ptr = &msg[ChunkAck::FieldIndex::kFlowControlMessageType];
- msg_type_ = ncs_decode_8bit(&ptr);
+ // decode protocol identifier if the mfrag_ and mseq_ are 0
+ // otherwise it is always DataMessage within non-zero mseq_ and mfrag_
+ if (mfrag_ == 0 && mseq_ == 0) {
+ ptr = &msg[ChunkAck::FieldIndex::kProtocolIdentifier];
+ pro_id_ = ncs_decode_32bit(&ptr);
+ if (pro_id_ == MDS_PROT_FCTRL_ID) {
+ // decode message type
+ ptr = &msg[ChunkAck::FieldIndex::kFlowControlMessageType];
+ msg_type_ = ncs_decode_8bit(&ptr);
+ }
+ } else {
+ pro_id_ = 0;
+ msg_type_ = 0;
[Thuan] Don't need ELSE as values 0 already?
[Minh]: I think we should explicitly set again, the variable header
might be reused to decode
}
} else {
if (mfrag_ != 0) {
--
2.7.4
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
