This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
pespin gerrit-no-reply at lists.osmocom.orgpespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-abis/+/19256 )
Change subject: e1_input: Use osmo_use_count in e1inp_line
......................................................................
e1_input: Use osmo_use_count in e1inp_line
osmo_use_count is available since libosmocore 1.1.0 release, so bump
required libosmocore version in autotools and packages.
struct e1inp_line field refcnt is kept in order to keep ABI
compatibility accessing struct fields. The new use_count is added at the
end. Size of struct changing is fine since it is allocated through
an API and a pointer should be used by clients.
e1inp_line_clone API is changed but it's not used by anyone outside
libosmo-abis, so it's fine.
Related: OS#4624
Change-Id: I0658b2e9c452598025cc0f1d0b060076171767cc
---
M TODO-RELEASE
M configure.ac
M contrib/libosmo-abis.spec.in
M debian/control
M include/osmocom/abis/e1_input.h
M src/e1_input.c
M src/input/ipaccess.c
7 files changed, 85 insertions(+), 54 deletions(-)
Approvals:
Jenkins Builder: Verified
laforge: Looks good to me, but someone else must approve
Hoernchen: Looks good to me, but someone else must approve
pespin: Looks good to me, approved
diff --git a/TODO-RELEASE b/TODO-RELEASE
index 85b16a7..aa278ec 100644
--- a/TODO-RELEASE
+++ b/TODO-RELEASE
@@ -10,3 +10,4 @@
libosmo-abis API change major: add parameter to struct e1inp_line
libosmo-trau API change add new function osmo_rtp_socket_set_dscp()
libosmo-abis API change major: add parameter to struct lapd_instance
+libosmo-abis Field added struct e1inp_line "use_count". REMINDER: Upon LIBVERSION c bump, take the chance to drop struct e1inp_line "refcnt" field.
diff --git a/configure.ac b/configure.ac
index f095cf5..04ffca6 100644
--- a/configure.ac
+++ b/configure.ac
@@ -64,9 +64,9 @@
dnl Generate the output
AM_CONFIG_HEADER(config.h)
-PKG_CHECK_MODULES(LIBOSMOCORE, libosmocore >= 1.0.0)
-PKG_CHECK_MODULES(LIBOSMOVTY, libosmovty >= 1.0.0)
-PKG_CHECK_MODULES(LIBOSMOGSM, libosmogsm >= 1.0.0)
+PKG_CHECK_MODULES(LIBOSMOCORE, libosmocore >= 1.1.0)
+PKG_CHECK_MODULES(LIBOSMOVTY, libosmovty >= 1.1.0)
+PKG_CHECK_MODULES(LIBOSMOGSM, libosmogsm >= 1.1.0)
PKG_CHECK_MODULES(ORTP, ortp >= 0.22.0)
AC_ARG_ENABLE([dahdi],
diff --git a/contrib/libosmo-abis.spec.in b/contrib/libosmo-abis.spec.in
index 3ad36b1..da0c2d8 100644
--- a/contrib/libosmo-abis.spec.in
+++ b/contrib/libosmo-abis.spec.in
@@ -27,9 +27,9 @@
BuildRequires: libtool >= 2
BuildRequires: pkgconfig >= 0.20
BuildRequires: xz
-BuildRequires: pkgconfig(libosmocore) >= 1.0.0
-BuildRequires: pkgconfig(libosmogsm) >= 1.0.0
-BuildRequires: pkgconfig(libosmovty) >= 1.0.0
+BuildRequires: pkgconfig(libosmocore) >= 1.1.0
+BuildRequires: pkgconfig(libosmogsm) >= 1.1.0
+BuildRequires: pkgconfig(libosmovty) >= 1.1.0
BuildRequires: pkgconfig(ortp) >= 0.22
BuildRequires: pkgconfig(talloc)
diff --git a/debian/control b/debian/control
index b9e2078..7039bfe 100644
--- a/debian/control
+++ b/debian/control
@@ -11,7 +11,7 @@
dh-autoreconf,
libdpkg-perl,
git,
- libosmocore-dev (>= 1.0.0),
+ libosmocore-dev (>= 1.1.0),
pkg-config,
libortp-dev
Standards-Version: 3.9.7
diff --git a/include/osmocom/abis/e1_input.h b/include/osmocom/abis/e1_input.h
index ee33ae6..44708bb 100644
--- a/include/osmocom/abis/e1_input.h
+++ b/include/osmocom/abis/e1_input.h
@@ -5,6 +5,7 @@
#include <netinet/in.h>
#include <osmocom/core/linuxlist.h>
+#include <osmocom/core/use_count.h>
#include <osmocom/core/timer.h>
#include <osmocom/core/msgb.h>
#include <osmocom/core/select.h>
@@ -191,7 +192,7 @@
struct e1inp_line {
struct llist_head list;
- int refcnt;
+ int refcnt; /* unusued, kept for ABI compat, use_count is used instead */
unsigned int num;
const char *name;
@@ -215,6 +216,8 @@
struct e1inp_driver *driver;
void *driver_data;
+
+ struct osmo_use_count use_count;
};
#define e1inp_line_ipa_oml_ts(line) (&line->ts[0])
#define e1inp_line_ipa_rsl_ts(line, trx_id) (&line->ts[1 + (trx_id)])
@@ -245,13 +248,17 @@
struct e1inp_line *e1inp_line_create(uint8_t e1_nr, const char *driver_name);
/* clone one existing E1 input line */
-struct e1inp_line *e1inp_line_clone(void *ctx, struct e1inp_line *line);
+struct e1inp_line *e1inp_line_clone(void *ctx, struct e1inp_line *line, const char *use);
/* increment refcount use of E1 input line */
-void e1inp_line_get(struct e1inp_line *line);
+void e1inp_line_get(struct e1inp_line *line) OSMO_DEPRECATED("Use e1inp_line_get2() instead");
/* decrement refcount use of E1 input line, release if unused */
-void e1inp_line_put(struct e1inp_line *line);
+void e1inp_line_put(struct e1inp_line *line) OSMO_DEPRECATED("Use e1inp_line_put2() instead");
+
+/* Convenience macros for struct foo instances. These are strict about use count errors. */
+#define e1inp_line_get2(line, USE) OSMO_ASSERT( osmo_use_count_get_put(&(line)->use_count, USE, 1) == 0 );
+#define e1inp_line_put2(line, USE) OSMO_ASSERT( osmo_use_count_get_put(&(line)->use_count, USE, -1) == 0 );
/* bind operations to one E1 input line */
void e1inp_line_bind_ops(struct e1inp_line *line, const struct e1inp_line_ops *ops);
diff --git a/src/e1_input.c b/src/e1_input.c
index 9ea4f17..fc0370d 100644
--- a/src/e1_input.c
+++ b/src/e1_input.c
@@ -26,6 +26,7 @@
#include <stdio.h>
#include <unistd.h>
+#include <inttypes.h>
#include <stdlib.h>
#include <errno.h>
#include <string.h>
@@ -366,6 +367,48 @@
return 0;
}
+static int e1inp_line_use_cb(struct osmo_use_count_entry *use_count_entry, int32_t old_use_count,
+ const char *file, int file_line)
+{
+ char buf[512];
+ struct osmo_use_count *uc = use_count_entry->use_count;
+ struct e1inp_line *line = uc->talloc_object;
+
+ LOGPSRC(DLINP, LOGL_INFO, file, file_line,
+ "E1L(%u) Line (%p) reference count %s changed %" PRId32 " -> %" PRId32 " [%s]\n",
+ (line)->num, line, use_count_entry->use,
+ old_use_count, use_count_entry->count,
+ osmo_use_count_name_buf(buf, sizeof(buf), uc));
+
+ if (!use_count_entry->count)
+ osmo_use_count_free(use_count_entry);
+
+ if (osmo_use_count_total(uc) > 0)
+ return 0;
+
+ /* Remove our counter group from libosmocore's global counter
+ * list if we are freeing the last remaining talloc context.
+ * Otherwise we get a use-after-free when libosmocore's timer
+ * ticks again and attempts to update these counters (OS#3011).
+ *
+ * Note that talloc internally counts "secondary" references
+ * _in addition to_ the initial allocation context, so yes,
+ * we must check for *zero* remaining secondary contexts here. */
+ if (talloc_reference_count(line->rate_ctr) == 0) {
+ rate_ctr_group_free(line->rate_ctr);
+ } else {
+ /* We are not freeing the last talloc context.
+ * Instead of calling talloc_free(), unlink this 'line' pointer
+ * which serves as one of several talloc contexts for the rate
+ * counters and driver private state. */
+ talloc_unlink(line, line->rate_ctr);
+ if (line->driver_data)
+ talloc_unlink(line, line->driver_data);
+ }
+ talloc_free(line);
+ return 0;
+}
+
struct e1inp_line *e1inp_line_find(uint8_t e1_nr)
{
struct e1inp_line *e1i_line;
@@ -417,14 +460,18 @@
line->ts[i].num = i+1;
line->ts[i].line = line;
}
- line->refcnt++;
+
+ line->use_count.talloc_object = line;
+ line->use_count.use_cb = e1inp_line_use_cb;
+ e1inp_line_get2(line, "ctor");
+
llist_add_tail(&line->list, &e1inp_line_list);
return line;
}
struct e1inp_line *
-e1inp_line_clone(void *ctx, struct e1inp_line *line)
+e1inp_line_clone(void *ctx, struct e1inp_line *line, const char *use)
{
struct e1inp_line *clone;
@@ -453,47 +500,23 @@
if (line->driver_data)
clone->driver_data = talloc_reference(clone, line->driver_data);
- clone->refcnt = 1;
+ clone->use_count = (struct osmo_use_count) {
+ .talloc_object = clone,
+ .use_cb = e1inp_line_use_cb,
+ .use_counts = {0},
+ };
+ e1inp_line_get2(clone, use); /* Clone is used internally for bfd */
return clone;
}
void e1inp_line_get(struct e1inp_line *line)
{
- int old_refcnt = line->refcnt++;
-
- LOGPIL(line, DLINP, LOGL_DEBUG, "Line '%s' (%p) reference count get: %d -> %d\n",
- line->name, line, old_refcnt, line->refcnt);
+ e1inp_line_get2(line, "unknown");
}
void e1inp_line_put(struct e1inp_line *line)
{
- int old_refcnt = line->refcnt--;
-
- LOGPIL(line, DLINP, LOGL_DEBUG, "Line '%s' (%p) reference count put: %d -> %d\n",
- line->name, line, old_refcnt, line->refcnt);
-
- if (line->refcnt == 0) {
- /* Remove our counter group from libosmocore's global counter
- * list if we are freeing the last remaining talloc context.
- * Otherwise we get a use-after-free when libosmocore's timer
- * ticks again and attempts to update these counters (OS#3011).
- *
- * Note that talloc internally counts "secondary" references
- * _in addition to_ the initial allocation context, so yes,
- * we must check for *zero* remaining secondary contexts here. */
- if (talloc_reference_count(line->rate_ctr) == 0) {
- rate_ctr_group_free(line->rate_ctr);
- } else {
- /* We are not freeing the last talloc context.
- * Instead of calling talloc_free(), unlink this 'line' pointer
- * which serves as one of several talloc contexts for the rate
- * counters and driver private state. */
- talloc_unlink(line, line->rate_ctr);
- if (line->driver_data)
- talloc_unlink(line, line->driver_data);
- }
- talloc_free(line);
- }
+ e1inp_line_put2(line, "unknown");
}
void
@@ -586,7 +609,7 @@
link->tei = tei;
link->sapi = sapi;
- e1inp_line_get(link->ts->line);
+ e1inp_line_get2(link->ts->line, "e1inp_sign_link");
llist_add_tail(&link->list, &ts->sign.sign_links);
@@ -609,7 +632,7 @@
if (link->ts->line->driver->close)
link->ts->line->driver->close(link);
- e1inp_line_put(link->ts->line);
+ e1inp_line_put2(link->ts->line, "e1inp_sign_link");
talloc_free(link);
}
diff --git a/src/input/ipaccess.c b/src/input/ipaccess.c
index 917a195..5d17839 100644
--- a/src/input/ipaccess.c
+++ b/src/input/ipaccess.c
@@ -293,7 +293,7 @@
/* Finally, we know which OML link is associated with
* this RSL link, attach it to this socket. */
bfd->data = new_line = sign_link->ts->line;
- e1inp_line_get(new_line);
+ e1inp_line_get2(new_line, "ipa_bfd");
ts = e1inp_line_ipa_rsl_ts(new_line, unit_data.trx_id);
newbfd = &ts->driver.ipaccess.fd;
@@ -310,7 +310,7 @@
goto err;
}
/* now we can release the dummy RSL line. */
- e1inp_line_put(line);
+ e1inp_line_put2(line, "ipa_bfd");
e1i_ts = ipaccess_line_ts(newbfd, new_line);
ipaccess_bsc_keepalive_fsm_alloc(e1i_ts, newbfd, "rsl_bsc_to_bts");
@@ -328,7 +328,7 @@
close(bfd->fd);
bfd->fd = -1;
}
- e1inp_line_put(line);
+ e1inp_line_put2(line, "ipa_bfd");
return -1;
}
@@ -603,7 +603,7 @@
struct osmo_fd *bfd;
/* clone virtual E1 line for this new OML link. */
- line = e1inp_line_clone(tall_ipa_ctx, link->line);
+ line = e1inp_line_clone(tall_ipa_ctx, link->line, "ipa_bfd");
if (line == NULL) {
LOGP(DLINP, LOGL_ERROR, "could not clone E1 line\n");
return -ENOMEM;
@@ -642,7 +642,7 @@
err_line:
close(bfd->fd);
bfd->fd = -1;
- e1inp_line_put(line);
+ e1inp_line_put2(line, "ipa_bfd");
return ret;
}
@@ -655,7 +655,7 @@
/* We don't know yet which OML link to associate it with. Thus, we
* allocate a temporary E1 line until we have received ID. */
- line = e1inp_line_clone(tall_ipa_ctx, link->line);
+ line = e1inp_line_clone(tall_ipa_ctx, link->line, "ipa_bfd");
if (line == NULL) {
LOGP(DLINP, LOGL_ERROR, "could not clone E1 line\n");
return -ENOMEM;
@@ -692,7 +692,7 @@
err_line:
close(bfd->fd);
bfd->fd = -1;
- e1inp_line_put(line);
+ e1inp_line_put2(line, "ipa_bfd");
return ret;
}
--
To view, visit https://gerrit.osmocom.org/c/libosmo-abis/+/19256
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-abis
Gerrit-Branch: master
Gerrit-Change-Id: I0658b2e9c452598025cc0f1d0b060076171767cc
Gerrit-Change-Number: 19256
Gerrit-PatchSet: 3
Gerrit-Owner: pespin <pespin at sysmocom.de>
Gerrit-Reviewer: Hoernchen <ewild at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann at sysmocom.de>
Gerrit-Reviewer: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: fixeria <vyanitskiy at sysmocom.de>
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: lynxis lazus <lynxis at fe80.eu>
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20200720/04ed7a45/attachment.htm>