<p>laforge <strong>submitted</strong> this change.</p><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25103">View Change</a></p><div style="white-space:pre-wrap">Approvals:
Jenkins Builder: Verified
laforge: Looks good to me, approved
osmith: Looks good to me, but someone else must approve
</div><pre style="font-family: monospace,monospace; white-space: pre-wrap;">mgcp_ratectr: do not set talloc destructor on library allocated item<br><br>The rate counter and stats item groups, which are allocated in<br>mgcp_ratectr.c are freed using a talloc destructor that is set in the<br>context of the item we just allocated using the stats / rate counter API<br>functions. When we do that, we risk overwriting an already existing<br>talloc destructor. Lets instead implement own free functions and set<br>those as talloc_destructor from above (trunk and MGCP config)<br><br>Change-Id: Ifc5091e9f95cc721e58d1eb2e55b97102c497706<br>Related: OS#5201<br>---<br>M include/osmocom/mgcp/mgcp_ratectr.h<br>M src/libosmo-mgcp/mgcp_protocol.c<br>M src/libosmo-mgcp/mgcp_ratectr.c<br>M src/libosmo-mgcp/mgcp_trunk.c<br>4 files changed, 80 insertions(+), 21 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/include/osmocom/mgcp/mgcp_ratectr.h b/include/osmocom/mgcp/mgcp_ratectr.h</span><br><span>index 5212f9b..48abc27 100644</span><br><span>--- a/include/osmocom/mgcp/mgcp_ratectr.h</span><br><span>+++ b/include/osmocom/mgcp/mgcp_ratectr.h</span><br><span>@@ -93,7 +93,9 @@</span><br><span> struct mgcp_trunk;</span><br><span> </span><br><span> int mgcp_ratectr_global_alloc(struct mgcp_config *cfg);</span><br><span style="color: hsl(120, 100%, 40%);">+void mgcp_ratectr_global_free(struct mgcp_config *cfg);</span><br><span> int mgcp_ratectr_trunk_alloc(struct mgcp_trunk *trunk);</span><br><span style="color: hsl(120, 100%, 40%);">+void mgcp_ratectr_trunk_free(struct mgcp_trunk *trunk);</span><br><span> </span><br><span> /* Trunk-global common stat items */</span><br><span> enum {</span><br><span>@@ -107,4 +109,4 @@</span><br><span> };</span><br><span> </span><br><span> int mgcp_stat_trunk_alloc(struct mgcp_trunk *trunk);</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(120, 100%, 40%);">+void mgcp_stat_trunk_free(struct mgcp_trunk *trunk);</span><br><span>diff --git a/src/libosmo-mgcp/mgcp_protocol.c b/src/libosmo-mgcp/mgcp_protocol.c</span><br><span>index b7368b2..88f1bd0 100644</span><br><span>--- a/src/libosmo-mgcp/mgcp_protocol.c</span><br><span>+++ b/src/libosmo-mgcp/mgcp_protocol.c</span><br><span>@@ -1583,6 +1583,13 @@</span><br><span> trunk->keepalive_interval, 0);</span><br><span> }</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+/* Free config, this function is automatically called by talloc_free when the configuration is freed. */</span><br><span style="color: hsl(120, 100%, 40%);">+static int config_free_talloc_destructor(struct mgcp_config *cfg)</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ mgcp_ratectr_global_free(cfg);</span><br><span style="color: hsl(120, 100%, 40%);">+ return 0;</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> /*! allocate configuration with default values.</span><br><span> * (called once at startup by main function) */</span><br><span> struct mgcp_config *mgcp_config_alloc(void)</span><br><span>@@ -1621,6 +1628,7 @@</span><br><span> }</span><br><span> </span><br><span> mgcp_ratectr_global_alloc(cfg);</span><br><span style="color: hsl(120, 100%, 40%);">+ talloc_set_destructor(cfg, config_free_talloc_destructor);</span><br><span> </span><br><span> return cfg;</span><br><span> }</span><br><span>diff --git a/src/libosmo-mgcp/mgcp_ratectr.c b/src/libosmo-mgcp/mgcp_ratectr.c</span><br><span>index d8e0374..3bf079b 100644</span><br><span>--- a/src/libosmo-mgcp/mgcp_ratectr.c</span><br><span>+++ b/src/libosmo-mgcp/mgcp_ratectr.c</span><br><span>@@ -150,12 +150,6 @@</span><br><span> .ctr_desc = all_rtp_conn_rate_ctr_desc</span><br><span> };</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-static int free_rate_counter_group(struct rate_ctr_group *rate_ctr_group)</span><br><span style="color: hsl(0, 100%, 40%);">-{</span><br><span style="color: hsl(0, 100%, 40%);">- rate_ctr_group_free(rate_ctr_group);</span><br><span style="color: hsl(0, 100%, 40%);">- return 0;</span><br><span style="color: hsl(0, 100%, 40%);">-}</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span> /*! allocate global rate counters</span><br><span> * (called once at startup).</span><br><span> * \param[in] cfg mgw configuration for which the rate counters are allocated.</span><br><span>@@ -173,12 +167,24 @@</span><br><span> return -EINVAL;</span><br><span> snprintf(ctr_name, sizeof(ctr_name), "%s:general", cfg->domain);</span><br><span> rate_ctr_group_set_name(ratectr->mgcp_general_ctr_group, ctr_name);</span><br><span style="color: hsl(0, 100%, 40%);">- talloc_set_destructor(ratectr->mgcp_general_ctr_group, free_rate_counter_group);</span><br><span> general_rate_ctr_index++;</span><br><span> }</span><br><span> return 0;</span><br><span> }</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+/*! free global rate counters</span><br><span style="color: hsl(120, 100%, 40%);">+ * (called once at process shutdown).</span><br><span style="color: hsl(120, 100%, 40%);">+ * \param[in] cfg mgw configuration for which the rate counters are allocated. */</span><br><span style="color: hsl(120, 100%, 40%);">+void mgcp_ratectr_global_free(struct mgcp_config *cfg)</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ struct mgcp_ratectr_global *ratectr = &cfg->ratectr;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ if (ratectr->mgcp_general_ctr_group) {</span><br><span style="color: hsl(120, 100%, 40%);">+ rate_ctr_group_free(ratectr->mgcp_general_ctr_group);</span><br><span style="color: hsl(120, 100%, 40%);">+ ratectr->mgcp_general_ctr_group = NULL;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> /*! allocate trunk specific rate counters</span><br><span> * (called once on trunk initialization).</span><br><span> * \param[in] trunk mgw trunk for which the rate counters are allocated.</span><br><span>@@ -200,7 +206,6 @@</span><br><span> snprintf(ctr_name, sizeof(ctr_name), "%s-%u:crcx", mgcp_trunk_type_strs_str(trunk->trunk_type),</span><br><span> trunk->trunk_nr);</span><br><span> rate_ctr_group_set_name(ratectr->mgcp_crcx_ctr_group, ctr_name);</span><br><span style="color: hsl(0, 100%, 40%);">- talloc_set_destructor(ratectr->mgcp_crcx_ctr_group, free_rate_counter_group);</span><br><span> crcx_rate_ctr_index++;</span><br><span> }</span><br><span> if (ratectr->mgcp_mdcx_ctr_group == NULL) {</span><br><span>@@ -211,7 +216,6 @@</span><br><span> snprintf(ctr_name, sizeof(ctr_name), "%s-%u:mdcx", mgcp_trunk_type_strs_str(trunk->trunk_type),</span><br><span> trunk->trunk_nr);</span><br><span> rate_ctr_group_set_name(ratectr->mgcp_mdcx_ctr_group, ctr_name);</span><br><span style="color: hsl(0, 100%, 40%);">- talloc_set_destructor(ratectr->mgcp_mdcx_ctr_group, free_rate_counter_group);</span><br><span> mdcx_rate_ctr_index++;</span><br><span> }</span><br><span> if (ratectr->mgcp_dlcx_ctr_group == NULL) {</span><br><span>@@ -222,7 +226,6 @@</span><br><span> snprintf(ctr_name, sizeof(ctr_name), "%s-%u:dlcx", mgcp_trunk_type_strs_str(trunk->trunk_type),</span><br><span> trunk->trunk_nr);</span><br><span> rate_ctr_group_set_name(ratectr->mgcp_dlcx_ctr_group, ctr_name);</span><br><span style="color: hsl(0, 100%, 40%);">- talloc_set_destructor(ratectr->mgcp_dlcx_ctr_group, free_rate_counter_group);</span><br><span> dlcx_rate_ctr_index++;</span><br><span> }</span><br><span> if (ratectr->all_rtp_conn_stats == NULL) {</span><br><span>@@ -233,7 +236,6 @@</span><br><span> snprintf(ctr_name, sizeof(ctr_name), "%s-%u:rtp_conn", mgcp_trunk_type_strs_str(trunk->trunk_type),</span><br><span> trunk->trunk_nr);</span><br><span> rate_ctr_group_set_name(ratectr->all_rtp_conn_stats, ctr_name);</span><br><span style="color: hsl(0, 100%, 40%);">- talloc_set_destructor(ratectr->all_rtp_conn_stats, free_rate_counter_group);</span><br><span> all_rtp_conn_rate_ctr_index++;</span><br><span> }</span><br><span> </span><br><span>@@ -245,12 +247,42 @@</span><br><span> snprintf(ctr_name, sizeof(ctr_name), "%s-%u:e1", mgcp_trunk_type_strs_str(trunk->trunk_type),</span><br><span> trunk->trunk_nr);</span><br><span> rate_ctr_group_set_name(ratectr->e1_stats, ctr_name);</span><br><span style="color: hsl(0, 100%, 40%);">- talloc_set_destructor(ratectr->e1_stats, free_rate_counter_group);</span><br><span> mdcx_rate_ctr_index++;</span><br><span> }</span><br><span> return 0;</span><br><span> }</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+/*! free trunk specific rate counters</span><br><span style="color: hsl(120, 100%, 40%);">+ * (called once when trunk is freed).</span><br><span style="color: hsl(120, 100%, 40%);">+ * \param[in] trunk mgw trunk on which the rate counters are allocated. */</span><br><span style="color: hsl(120, 100%, 40%);">+void mgcp_ratectr_trunk_free(struct mgcp_trunk *trunk)</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ struct mgcp_ratectr_trunk *ratectr = &trunk->ratectr;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ if (ratectr->mgcp_crcx_ctr_group) {</span><br><span style="color: hsl(120, 100%, 40%);">+ rate_ctr_group_free(ratectr->mgcp_crcx_ctr_group);</span><br><span style="color: hsl(120, 100%, 40%);">+ ratectr->mgcp_crcx_ctr_group = NULL;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+ if (ratectr->mgcp_mdcx_ctr_group) {</span><br><span style="color: hsl(120, 100%, 40%);">+ rate_ctr_group_free(ratectr->mgcp_mdcx_ctr_group);</span><br><span style="color: hsl(120, 100%, 40%);">+ ratectr->mgcp_mdcx_ctr_group = NULL;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+ if (ratectr->mgcp_dlcx_ctr_group) {</span><br><span style="color: hsl(120, 100%, 40%);">+ rate_ctr_group_free(ratectr->mgcp_dlcx_ctr_group);</span><br><span style="color: hsl(120, 100%, 40%);">+ ratectr->mgcp_dlcx_ctr_group = NULL;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+ if (ratectr->all_rtp_conn_stats) {</span><br><span style="color: hsl(120, 100%, 40%);">+ rate_ctr_group_free(ratectr->all_rtp_conn_stats);</span><br><span style="color: hsl(120, 100%, 40%);">+ ratectr->all_rtp_conn_stats = NULL;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ /* E1 specific */</span><br><span style="color: hsl(120, 100%, 40%);">+ if (ratectr->e1_stats) {</span><br><span style="color: hsl(120, 100%, 40%);">+ rate_ctr_group_free(ratectr->e1_stats);</span><br><span style="color: hsl(120, 100%, 40%);">+ ratectr->e1_stats = NULL;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> const struct osmo_stat_item_desc trunk_stat_desc[] = {</span><br><span> [TRUNK_STAT_ENDPOINTS_TOTAL] = { "endpoints:total",</span><br><span> "Number of endpoints that exist on the trunk",</span><br><span>@@ -268,12 +300,6 @@</span><br><span> .item_desc = trunk_stat_desc,</span><br><span> };</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-static int free_stat_item_group(struct osmo_stat_item_group *stat_item_group)</span><br><span style="color: hsl(0, 100%, 40%);">-{</span><br><span style="color: hsl(0, 100%, 40%);">- osmo_stat_item_group_free(stat_item_group);</span><br><span style="color: hsl(0, 100%, 40%);">- return 0;</span><br><span style="color: hsl(0, 100%, 40%);">-}</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span> /*! allocate trunk specific stat items</span><br><span> * (called once on trunk initialization).</span><br><span> * \param[in] trunk for which the stat items are allocated.</span><br><span>@@ -290,8 +316,20 @@</span><br><span> snprintf(stat_name, sizeof(stat_name), "%s-%u:common", mgcp_trunk_type_strs_str(trunk->trunk_type),</span><br><span> trunk->trunk_nr);</span><br><span> osmo_stat_item_group_set_name(stats->common, stat_name);</span><br><span style="color: hsl(0, 100%, 40%);">- talloc_set_destructor(stats->common, free_stat_item_group);</span><br><span> common_stat_index++;</span><br><span> </span><br><span> return 0;</span><br><span> }</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+/*! free trunk specific stat items</span><br><span style="color: hsl(120, 100%, 40%);">+ * (called once when trunk is freed).</span><br><span style="color: hsl(120, 100%, 40%);">+ * \param[in] trunk on which the stat items are allocated. */</span><br><span style="color: hsl(120, 100%, 40%);">+void mgcp_stat_trunk_free(struct mgcp_trunk *trunk)</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ struct mgcp_stat_trunk *stats = &trunk->stats;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ if (stats->common) {</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_stat_item_group_free(stats->common);</span><br><span style="color: hsl(120, 100%, 40%);">+ stats->common = NULL;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span>diff --git a/src/libosmo-mgcp/mgcp_trunk.c b/src/libosmo-mgcp/mgcp_trunk.c</span><br><span>index a97ad39..c69c242 100644</span><br><span>--- a/src/libosmo-mgcp/mgcp_trunk.c</span><br><span>+++ b/src/libosmo-mgcp/mgcp_trunk.c</span><br><span>@@ -35,7 +35,17 @@</span><br><span> { 0, NULL }</span><br><span> };</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-/*! allocate trunk and add it (if required) to the trunk list.</span><br><span style="color: hsl(120, 100%, 40%);">+/* Free trunk, this function is automatically called by talloc_free when the trunk is freed. It does not free the</span><br><span style="color: hsl(120, 100%, 40%);">+ * endpoints on the trunk, this must be done separately before freeing the trunk. */</span><br><span style="color: hsl(120, 100%, 40%);">+static int trunk_free_talloc_destructor(struct mgcp_trunk *trunk)</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ llist_del(&trunk->entry);</span><br><span style="color: hsl(120, 100%, 40%);">+ mgcp_ratectr_trunk_free(trunk);</span><br><span style="color: hsl(120, 100%, 40%);">+ mgcp_stat_trunk_free(trunk);</span><br><span style="color: hsl(120, 100%, 40%);">+ return 0;</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+/*! allocate trunk and add it to the trunk list.</span><br><span> * (called once at startup by VTY).</span><br><span> * \param[in] cfg mgcp configuration.</span><br><span> * \param[in] ttype trunk type.</span><br><span>@@ -66,6 +76,7 @@</span><br><span> </span><br><span> mgcp_ratectr_trunk_alloc(trunk);</span><br><span> mgcp_stat_trunk_alloc(trunk);</span><br><span style="color: hsl(120, 100%, 40%);">+ talloc_set_destructor(trunk, trunk_free_talloc_destructor);</span><br><span> </span><br><span> return trunk;</span><br><span> }</span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25103">change 25103</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/osmo-mgw/+/25103"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-mgw </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: Ifc5091e9f95cc721e58d1eb2e55b97102c497706 </div>
<div style="display:none"> Gerrit-Change-Number: 25103 </div>
<div style="display:none"> Gerrit-PatchSet: 8 </div>
<div style="display:none"> Gerrit-Owner: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: daniel <dwillmann@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: osmith <osmith@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: merged </div>