Hi Minh,
Ack with comments inline.
Regards, Vu
On 11/25/19 1:12 PM, Minh Chau wrote:
The patch avoids message reallocation if enable
MDS_TIPC_FCTRL_ENABLED
---
src/mds/mds_dt_tipc.c | 27 ++++++++++++++++++++-------
src/mds/mds_tipc_fctrl_msg.cc | 2 +-
src/mds/mds_tipc_fctrl_portid.cc | 9 +++------
3 files changed, 24 insertions(+), 14 deletions(-)
diff --git a/src/mds/mds_dt_tipc.c b/src/mds/mds_dt_tipc.c
index fdf0da7..aa8d5c2 100644
--- a/src/mds/mds_dt_tipc.c
+++ b/src/mds/mds_dt_tipc.c
@@ -2644,7 +2644,7 @@ uint32_t mds_mdtm_send_tipc(MDTM_SEND_REQ *req)
if (req->snd_type == MDS_SENDTYPE_ACK ||
req->snd_type == MDS_SENDTYPE_RACK) {
uint8_t len = sum_mds_hdr_plus_mdtm_hdr_plus_len;
- uint8_t buffer_ack[len];
+ uint8_t* buffer_ack = calloc(1, len);
[Vu] Below this allocation, there are several error handlings, but not
free memory before returning.
Is that expected?
/* Add mds_hdr */
if (NCSCC_RC_SUCCESS !=
@@ -2667,7 +2667,11 @@ uint32_t mds_mdtm_send_tipc(MDTM_SEND_REQ *req)
m_MDS_LOG_DBG(
"MDTM:Sending message with Service Seqno=%d, TO
Dest_Tipc_id=<0x%08x:%u> ",
req->svc_seq_num, tipc_id.node, tipc_id.ref);
- return mdtm_sendto(buffer_ack, len, tipc_id);
+ status = mdtm_sendto(buffer_ack, len, tipc_id);
+ if (gl_mds_pro_ver != MDS_PROT_FCTRL) {
+ free(buffer_ack);
+ }
[Vu] Above allocation does not stick with `MDS_PROT_FCTRL` check, so
if the above condition check
gets failure, the allocated memory is leaked?
+ return status;
}
if (MDS_ENC_TYPE_FLAT == req->msg.encoding) {
@@ -2815,6 +2819,8 @@ uint32_t mds_mdtm_send_tipc(MDTM_SEND_REQ *req)
free(body);
return NCSCC_RC_FAILURE;
}
+ m_MMGR_FREE_BUFR_LIST(usrbuf);
+ free(body);
} else {
if (NCSCC_RC_SUCCESS !=
mdtm_sendto(body, len, tipc_id)) {
@@ -2824,9 +2830,12 @@ uint32_t mds_mdtm_send_tipc(MDTM_SEND_REQ *req)
free(body);
return NCSCC_RC_FAILURE;
}
+ if (gl_mds_pro_ver != MDS_PROT_FCTRL) {
+ m_MMGR_FREE_BUFR_LIST(usrbuf);
+ free(body);
+ }
}
- m_MMGR_FREE_BUFR_LIST(usrbuf);
- free(body);
+
return NCSCC_RC_SUCCESS;
}
} break;
@@ -2909,7 +2918,9 @@ uint32_t mds_mdtm_send_tipc(MDTM_SEND_REQ *req)
mds_free_direct_buff(
req->msg.data.buff_info.buff);
}
- free(body);
+ if (gl_mds_pro_ver != MDS_PROT_FCTRL) {
+ free(body);
+ }
return NCSCC_RC_SUCCESS;
} break;
@@ -3059,21 +3070,23 @@ uint32_t mdtm_frag_and_send(MDTM_SEND_REQ
*req, uint32_t seq_num,
get_svc_names(req->src_svc_id), req->src_svc_id,
get_svc_names(req->dest_svc_id),
req->dest_svc_id);
ret = mdtm_mcast_sendto(body, len_buf, req);
+ free(body);
} else {
m_MDS_LOG_DBG(
"MDTM:Sending message with Service Seqno=%d,
Fragment Seqnum=%d, frag_num=%d, TO Dest_Tipc_id=<0x%08x:%u>",
req->svc_seq_num, seq_num, frag_val,
id.node, id.ref);
ret = mdtm_sendto(body, len_buf, id);
+ if (gl_mds_pro_ver != MDS_PROT_FCTRL) {
+ free(body);
+ }
}
if (ret != NCSCC_RC_SUCCESS) {
// Failed to send a fragmented msg, stop sending
m_MMGR_FREE_BUFR_LIST(usrbuf);
- free(body);
break;
}
m_MMGR_REMOVE_FROM_START(&usrbuf, len_buf - hdr_plus);
- free(body);
len = len - (len_buf - hdr_plus);
if (len == 0)
break;
diff --git a/src/mds/mds_tipc_fctrl_msg.cc
b/src/mds/mds_tipc_fctrl_msg.cc
index 454c02c..0f9fd09 100644
--- a/src/mds/mds_tipc_fctrl_msg.cc
+++ b/src/mds/mds_tipc_fctrl_msg.cc
@@ -138,7 +138,7 @@ void DataMessage::Decode(uint8_t *msg) {
DataMessage::~DataMessage() {
if (msg_data_ != nullptr) {
- delete[] msg_data_;
+ free(msg_data_);
msg_data_ = nullptr;
}
}
diff --git a/src/mds/mds_tipc_fctrl_portid.cc
b/src/mds/mds_tipc_fctrl_portid.cc
index 724eb7b..08e8dce 100644
--- a/src/mds/mds_tipc_fctrl_portid.cc
+++ b/src/mds/mds_tipc_fctrl_portid.cc
@@ -175,13 +175,12 @@ uint32_t TipcPortId::Queue(const uint8_t* data,
uint16_t length,
DataMessage *msg = new DataMessage;
msg->header_.Decode(const_cast<uint8_t*>(data));
msg->Decode(const_cast<uint8_t*>(data));
- msg->msg_data_ = new uint8_t[length];
msg->is_sent_ = is_sent;
- memcpy(msg->msg_data_, data, length);
+ msg->msg_data_ = const_cast<uint8_t*>(data);
[Vu] In `mds_tipc_fctrl_trysend`, there are some cases the allocated
memory won't be passed to DataMessage.
Should be freed in such cases? e.g: portid = nullptr and
portid->state_ = TipcPortId::State::kDisabled ?
Regards, Vu
sndqueue_.Queue(msg);
+ ++sndwnd_.send_;
+ sndwnd_.nacked_space_ += length;
if (is_sent) {
- ++sndwnd_.send_;
- sndwnd_.nacked_space_ += length;
m_MDS_LOG_DBG("FCTRL: [me] --> [node:%x, ref:%u], "
"SndData[mseq:%u, mfrag:%u, fseq:%u, len:%u], "
"sndwnd[acked:%u, send:%u, nacked:%" PRIu64 "]",
@@ -189,8 +188,6 @@ uint32_t TipcPortId::Queue(const uint8_t* data,
uint16_t length,
msg->header_.mseq_, msg->header_.mfrag_,
msg->header_.fseq_, length,
sndwnd_.acked_.v(), sndwnd_.send_.v(), sndwnd_.nacked_space_);
} else {
- ++sndwnd_.send_;
- sndwnd_.nacked_space_ += length;
m_MDS_LOG_NOTIFY("FCTRL: [me] --> [node:%x, ref:%u], "
"QueData[mseq:%u, mfrag:%u, fseq:%u, len:%u], "
"sndwnd[acked:%u, send:%u, nacked:%" PRIu64 "]",
