dexter has uploaded this change for review. ( 
https://gerrit.osmocom.org/c/osmo-mgw/+/25103 )


Change subject: mgcp_ratectr: do not set talloc destructor on library allocated 
item
......................................................................

mgcp_ratectr: do not set talloc destructor on library allocated item

The rate counter and stats item groups, which are allocated in
mgcp_ratectr.c are freed using a talloc destructor that is set in the
context of the item we just allocated using the stats / rate counter API
functions. When we do that, we risk overwriting an already existing
talloc destructor. Lets instead implement own free functions and set
those as talloc_destructor from above (trunk and MGCP config)

Change-Id: Ifc5091e9f95cc721e58d1eb2e55b97102c497706
Related: SYS#5201
---
M include/osmocom/mgcp/mgcp_ratectr.h
M src/libosmo-mgcp/mgcp_protocol.c
M src/libosmo-mgcp/mgcp_ratectr.c
M src/libosmo-mgcp/mgcp_trunk.c
4 files changed, 83 insertions(+), 21 deletions(-)



  git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/03/25103/1

diff --git a/include/osmocom/mgcp/mgcp_ratectr.h 
b/include/osmocom/mgcp/mgcp_ratectr.h
index 5212f9b..a01e025 100644
--- a/include/osmocom/mgcp/mgcp_ratectr.h
+++ b/include/osmocom/mgcp/mgcp_ratectr.h
@@ -93,7 +93,9 @@
 struct mgcp_trunk;

 int mgcp_ratectr_global_alloc(struct mgcp_config *cfg);
+int mgcp_ratectr_global_free(struct mgcp_config *cfg);
 int mgcp_ratectr_trunk_alloc(struct mgcp_trunk *trunk);
+int mgcp_ratectr_trunk_free(struct mgcp_trunk *trunk);

 /* Trunk-global common stat items */
 enum {
@@ -107,4 +109,4 @@
 };

 int mgcp_stat_trunk_alloc(struct mgcp_trunk *trunk);
-
+int mgcp_stat_trunk_free(struct mgcp_trunk *trunk);
diff --git a/src/libosmo-mgcp/mgcp_protocol.c b/src/libosmo-mgcp/mgcp_protocol.c
index e69c00f..74ee758 100644
--- a/src/libosmo-mgcp/mgcp_protocol.c
+++ b/src/libosmo-mgcp/mgcp_protocol.c
@@ -1687,6 +1687,7 @@
        }

        mgcp_ratectr_global_alloc(cfg);
+       talloc_set_destructor(cfg, mgcp_ratectr_global_free);

        return cfg;
 }
diff --git a/src/libosmo-mgcp/mgcp_ratectr.c b/src/libosmo-mgcp/mgcp_ratectr.c
index d8e0374..7362fe2 100644
--- a/src/libosmo-mgcp/mgcp_ratectr.c
+++ b/src/libosmo-mgcp/mgcp_ratectr.c
@@ -150,12 +150,6 @@
        .ctr_desc = all_rtp_conn_rate_ctr_desc
 };

-static int free_rate_counter_group(struct rate_ctr_group *rate_ctr_group)
-{
-       rate_ctr_group_free(rate_ctr_group);
-       return 0;
-}
-
 /*! allocate global rate counters
  *  (called once at startup).
  *  \param[in] cfg mgw configuration for which the rate counters are allocated.
@@ -173,12 +167,27 @@
                        return -EINVAL;
                snprintf(ctr_name, sizeof(ctr_name), "%s:general", cfg->domain);
                rate_ctr_group_set_name(ratectr->mgcp_general_ctr_group, 
ctr_name);
-               talloc_set_destructor(ratectr->mgcp_general_ctr_group, 
free_rate_counter_group);
                general_rate_ctr_index++;
        }
        return 0;
 }

+/*! free global rate counters
+ *  (called once at process shutdown).
+ *  \param[in] cfg mgw configuration for which the rate counters are allocated.
+ *  \returns 0 on success, -EINVAL on failure. */
+int mgcp_ratectr_global_free(struct mgcp_config *cfg)
+{
+       struct mgcp_ratectr_global *ratectr = &cfg->ratectr;
+
+       if (ratectr->mgcp_general_ctr_group) {
+               rate_ctr_group_free(ratectr->mgcp_general_ctr_group);
+               ratectr->mgcp_general_ctr_group = NULL;
+       }
+
+       return 0;
+}
+
 /*! allocate trunk specific rate counters
  *  (called once on trunk initialization).
  *  \param[in] trunk mgw trunk for which the rate counters are allocated.
@@ -200,7 +209,6 @@
                snprintf(ctr_name, sizeof(ctr_name), "%s-%u:crcx", 
mgcp_trunk_type_strs_str(trunk->trunk_type),
                         trunk->trunk_nr);
                rate_ctr_group_set_name(ratectr->mgcp_crcx_ctr_group, ctr_name);
-               talloc_set_destructor(ratectr->mgcp_crcx_ctr_group, 
free_rate_counter_group);
                crcx_rate_ctr_index++;
        }
        if (ratectr->mgcp_mdcx_ctr_group == NULL) {
@@ -211,7 +219,6 @@
                snprintf(ctr_name, sizeof(ctr_name), "%s-%u:mdcx", 
mgcp_trunk_type_strs_str(trunk->trunk_type),
                         trunk->trunk_nr);
                rate_ctr_group_set_name(ratectr->mgcp_mdcx_ctr_group, ctr_name);
-               talloc_set_destructor(ratectr->mgcp_mdcx_ctr_group, 
free_rate_counter_group);
                mdcx_rate_ctr_index++;
        }
        if (ratectr->mgcp_dlcx_ctr_group == NULL) {
@@ -222,7 +229,6 @@
                snprintf(ctr_name, sizeof(ctr_name), "%s-%u:dlcx", 
mgcp_trunk_type_strs_str(trunk->trunk_type),
                         trunk->trunk_nr);
                rate_ctr_group_set_name(ratectr->mgcp_dlcx_ctr_group, ctr_name);
-               talloc_set_destructor(ratectr->mgcp_dlcx_ctr_group, 
free_rate_counter_group);
                dlcx_rate_ctr_index++;
        }
        if (ratectr->all_rtp_conn_stats == NULL) {
@@ -233,10 +239,10 @@
                snprintf(ctr_name, sizeof(ctr_name), "%s-%u:rtp_conn", 
mgcp_trunk_type_strs_str(trunk->trunk_type),
                         trunk->trunk_nr);
                rate_ctr_group_set_name(ratectr->all_rtp_conn_stats, ctr_name);
-               talloc_set_destructor(ratectr->all_rtp_conn_stats, 
free_rate_counter_group);
                all_rtp_conn_rate_ctr_index++;
        }

+
        /* E1 specific */
        if (trunk->trunk_type == MGCP_TRUNK_E1 && ratectr->e1_stats == NULL) {
                ratectr->e1_stats = rate_ctr_group_alloc(trunk, 
&e1_rate_ctr_group_desc, mdcx_rate_ctr_index);
@@ -245,9 +251,42 @@
                snprintf(ctr_name, sizeof(ctr_name), "%s-%u:e1", 
mgcp_trunk_type_strs_str(trunk->trunk_type),
                         trunk->trunk_nr);
                rate_ctr_group_set_name(ratectr->e1_stats, ctr_name);
-               talloc_set_destructor(ratectr->e1_stats, 
free_rate_counter_group);
                mdcx_rate_ctr_index++;
        }
+
+       return 0;
+}
+
+/*! free trunk specific rate counters
+ *  (called once when trunk is freed).
+ *  \param[in] trunk mgw trunk on which the rate counters are allocated. */
+int mgcp_ratectr_trunk_free(struct mgcp_trunk *trunk)
+{
+       struct mgcp_ratectr_trunk *ratectr = &trunk->ratectr;
+
+       if (ratectr->mgcp_crcx_ctr_group) {
+               rate_ctr_group_free(ratectr->mgcp_crcx_ctr_group);
+               ratectr->mgcp_crcx_ctr_group = NULL;
+       }
+       if (ratectr->mgcp_mdcx_ctr_group) {
+               rate_ctr_group_free(ratectr->mgcp_mdcx_ctr_group);
+               ratectr->mgcp_mdcx_ctr_group = NULL;
+       }
+       if (ratectr->mgcp_dlcx_ctr_group) {
+               rate_ctr_group_free(ratectr->mgcp_dlcx_ctr_group);
+               ratectr->mgcp_dlcx_ctr_group = NULL;
+       }
+       if (ratectr->all_rtp_conn_stats) {
+               rate_ctr_group_free(ratectr->all_rtp_conn_stats);
+               ratectr->all_rtp_conn_stats = NULL;
+       }
+
+       /* E1 specific */
+       if (ratectr->e1_stats) {
+               rate_ctr_group_free(ratectr->e1_stats);
+               ratectr->e1_stats = NULL;
+       }
+
        return 0;
 }

@@ -268,12 +307,6 @@
        .item_desc = trunk_stat_desc,
 };

-static int free_stat_item_group(struct osmo_stat_item_group *stat_item_group)
-{
-       osmo_stat_item_group_free(stat_item_group);
-       return 0;
-}
-
 /*! allocate trunk specific stat items
  *  (called once on trunk initialization).
  *  \param[in] trunk for which the stat items are allocated.
@@ -290,8 +323,22 @@
        snprintf(stat_name, sizeof(stat_name), "%s-%u:common", 
mgcp_trunk_type_strs_str(trunk->trunk_type),
                 trunk->trunk_nr);
        osmo_stat_item_group_set_name(stats->common, stat_name);
-       talloc_set_destructor(stats->common, free_stat_item_group);
        common_stat_index++;

        return 0;
 }
+
+/*! free trunk specific stat items
+ *  (called once when trunk is freed).
+ *  \param[in] trunk on which the stat items are allocated. */
+int mgcp_stat_trunk_free(struct mgcp_trunk *trunk)
+{
+       struct mgcp_stat_trunk *stats = &trunk->stats;
+
+       if (stats->common) {
+               osmo_stat_item_group_free(stats->common);
+               stats->common = NULL;
+       }
+
+       return 0;
+}
diff --git a/src/libosmo-mgcp/mgcp_trunk.c b/src/libosmo-mgcp/mgcp_trunk.c
index b2d1969..ecfb48f 100644
--- a/src/libosmo-mgcp/mgcp_trunk.c
+++ b/src/libosmo-mgcp/mgcp_trunk.c
@@ -35,7 +35,18 @@
        { 0, NULL }
 };

-/*! allocate trunk and add it (if required) to the trunk list.
+/* Free trunk, this function is automatically called by talloc_free when the 
trunk is freed. It does not free the
+ * endpoints on the trunk, this must be done seperately before freeing the 
trunk. */
+static int trunk_free(struct mgcp_trunk *trunk)
+{
+       llist_del(&trunk->entry);
+       mgcp_ratectr_trunk_free(trunk);
+       mgcp_stat_trunk_free(trunk);
+       talloc_free(trunk);
+       return 0;
+}
+
+/*! allocate trunk and add it to the trunk list.
  *  (called once at startup by VTY).
  *  \param[in] cfg mgcp configuration.
  *  \param[in] ttype trunk type.
@@ -66,6 +77,7 @@

        mgcp_ratectr_trunk_alloc(trunk);
        mgcp_stat_trunk_alloc(trunk);
+       talloc_set_destructor(trunk, trunk_free);

        return trunk;
 }

--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/25103
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: Ifc5091e9f95cc721e58d1eb2e55b97102c497706
Gerrit-Change-Number: 25103
Gerrit-PatchSet: 1
Gerrit-Owner: dexter <pma...@sysmocom.de>
Gerrit-MessageType: newchange

Reply via email to