Re: [devel] [PATCH 1/1] mds: Avoid message re-allocation [#3089]
Hi Vu, Thuan,
The patch misses the error cases and the kDisabled state. I rework for
the V2.
Thanks
Minh
On 25/11/19 6:44 pm, Nguyen Minh Vu wrote:
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(, 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
+++
Re: [devel] [PATCH 1/1] mds: Avoid message re-allocation [#3089]
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;
}
Re: [devel] [PATCH 1/1] mds: Avoid message re-allocation [#3089]
Hi Minh,
ACK from me.
Best Regards,
ThuanTr
-Original Message-
From: Minh Chau
Sent: Monday, November 25, 2019 1:13 PM
To: thuan.t...@dektech.com.au; gary@dektech.com.au;
vu.m.ngu...@dektech.com.au
Cc: opensaf-devel@lists.sourceforge.net; Minh Chau
Subject: [PATCH 1/1] mds: Avoid message re-allocation [#3089]
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);
/* 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);
+ }
+ 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;
}
[devel] [PATCH 1/1] mds: Avoid message re-allocation [#3089]
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);
/* 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);
+ }
+ 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(, 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
