Discussion:
[PATCH 3/3] build: default to system-provided talloc
Jan Engelhardt
2014-10-03 10:57:04 UTC
Permalink
Rename the option --enable-talloc to --enable-internal-talloc, and
then make disable-internal-talloc the default when nothing else is
specified.
---
configure.ac | 12 ++++++------
include/Makefile.am | 2 +-
src/Makefile.am | 2 +-
3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/configure.ac b/configure.ac
index f0d1206..f12675a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -82,14 +82,14 @@ CHECK_TM_INCLUDES_TM_GMTOFF
dnl Generate the output
AC_CONFIG_HEADER(config.h)

-AC_ARG_ENABLE(talloc,
+AC_ARG_ENABLE([internal-talloc],
[AS_HELP_STRING(
- [--disable-talloc],
+ [--enable-internal-talloc],
[Disable using bundled talloc memory allocator]
)],
- [enable_talloc=$enableval], [enable_talloc="yes"])
-AM_CONDITIONAL(ENABLE_TALLOC, [test x"$enable_talloc" = x"yes"])
-AS_IF([test x"$enable_talloc" = xno],
+ [internal_talloc=$enableval], [internal_talloc="no"])
+AM_CONDITIONAL([INTERNAL_TALLOC], [test x"$internal_talloc" = x"yes"])
+AS_IF([test x"$internal_talloc" = xno],
[
PKG_CHECK_MODULES([TALLOC], [talloc])
OSMOCORE_INTERNAL_TALLOC=0
@@ -186,7 +186,7 @@ then
AM_CONDITIONAL(ENABLE_MSGFILE, false)
AM_CONDITIONAL(ENABLE_SERIAL, false)
AM_CONDITIONAL(ENABLE_VTY, false)
- AM_CONDITIONAL(ENABLE_TALLOC, false)
+ AM_CONDITIONAL([INTERNAL_TALLOC], [true])
AM_CONDITIONAL(ENABLE_UTILITIES, false)
AM_CONDITIONAL(ENABLE_GB, false)
AC_DEFINE([PANIC_INFLOOP],[1],[Use infinite loop on panic rather than fprintf/abort])
diff --git a/include/Makefile.am b/include/Makefile.am
index 4d59a72..d8af474 100644
--- a/include/Makefile.am
+++ b/include/Makefile.am
@@ -90,7 +90,7 @@ nobase_include_HEADERS += osmocom/core/plugin.h
endif

nobase_nodist_include_HEADERS = osmocom/core/talloc.h
-if ENABLE_TALLOC
+if INTERNAL_TALLOC
nobase_include_HEADERS += osmocom/core/talloc_int.h
endif

diff --git a/src/Makefile.am b/src/Makefile.am
index 3924330..4a6ba5c 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -27,7 +27,7 @@ else
libosmocore_la_LDFLAGS = -version-info $(LIBVERSION) -no-undefined
endif

-if ENABLE_TALLOC
+if INTERNAL_TALLOC
libosmocore_la_SOURCES += talloc.c
else
libosmocore_la_LIBADD += $(TALLOC_LIBS)
--
2.0.0
Jan Engelhardt
2014-10-03 10:57:02 UTC
Permalink
libosmogsm.so:lapd-core.c uses talloc_free unconditionally (like
libosmocore), but in the absence of the builtin talloc, does not link
to libtalloc.so (like libosmocore would). This causes linker failures
when programs using libosmogsm are being built. Correct this issue.

CCLD osmo-arfcn
../src/gsm/.libs/libosmogsm.so: undefined reference to `talloc_free'
collect2: error: ld returned 1 exit status
make[2]: *** [osmo-arfcn] Error 1
---
src/gsm/Makefile.am | 3 +++
1 file changed, 3 insertions(+)

diff --git a/src/gsm/Makefile.am b/src/gsm/Makefile.am
index 828aa50..b0cdea2 100644
--- a/src/gsm/Makefile.am
+++ b/src/gsm/Makefile.am
@@ -24,5 +24,8 @@ libosmogsm_la_SOURCES = a5.c rxlev_stat.c tlv_parser.c comp128.c comp128v23.c \

libosmogsm_la_LDFLAGS = $(LTLDFLAGS_OSMOGSM) -version-info $(LIBVERSION) -no-undefined
libosmogsm_la_LIBADD = $(top_builddir)/src/libosmocore.la
+if !ENABLE_TALLOC
+libosmogsm_la_LIBADD += -ltalloc
+endif

EXTRA_DIST = libosmogsm.map
--
2.0.0
Holger Hans Peter Freyther
2014-10-03 11:04:06 UTC
Permalink
On Fri, Oct 03, 2014 at 12:57:02PM +0200, Jan Engelhardt wrote:

Hi!
Post by Jan Engelhardt
+if !ENABLE_TALLOC
+libosmogsm_la_LIBADD += -ltalloc
+endif
please see my other mail. You assume that a libtalloc exists on the
system and that it is found in the library search paths. This is
not an improvement, it is a change that breaks in different ways for
different people.

As in 02.2013 and as of today. I really appreciate your work but I
can't take this patch.

holger
Jan Engelhardt
2014-10-03 11:16:53 UTC
Permalink
Post by Holger Hans Peter Freyther
Post by Jan Engelhardt
+if !ENABLE_TALLOC
+libosmogsm_la_LIBADD += -ltalloc
+endif
You assume that a libtalloc exists on the
system and that it is found in the library search paths. This is
not an improvement, it is a change that breaks in different ways for
different people.
It is the same logic that was used for libosmocore_la_LIBADD.
It goes away with patch 2 anyway, so it should not hurt. Squash the two
if need be.
Jan Engelhardt
2014-10-03 10:57:03 UTC
Permalink
When --disable-talloc was given to configure, it would already link
libosmocore.so to libtalloc. Make it use pkg-config to discover
talloc's flags and location, and when the system talloc is used,
make sure to use its API header file too.
---
.gitignore | 1 +
configure.ac | 16 +++-
include/Makefile.am | 3 +-
include/osmocom/core/talloc.h | 189 --------------------------------------
include/osmocom/core/talloc.h.in | 6 ++
include/osmocom/core/talloc_int.h | 189 ++++++++++++++++++++++++++++++++++++++
src/Makefile.am | 5 +-
src/gsm/Makefile.am | 8 +-
tests/Makefile.am | 12 +--
tests/msgfile/msgfile_test.c | 1 +
10 files changed, 226 insertions(+), 204 deletions(-)
delete mode 100644 include/osmocom/core/talloc.h
create mode 100644 include/osmocom/core/talloc.h.in
create mode 100644 include/osmocom/core/talloc_int.h

diff --git a/.gitignore b/.gitignore
index 1299028..30707a6 100644
--- a/.gitignore
+++ b/.gitignore
@@ -92,6 +92,7 @@ doc/html.tar
src/crc*gen.c
include/osmocom/core/crc*gen.h
include/osmocom/core/bit*gen.h
+/include/osmocom/core/talloc.h

# vi files
*.sw?
diff --git a/configure.ac b/configure.ac
index deaa8bf..f0d1206 100644
--- a/configure.ac
+++ b/configure.ac
@@ -85,10 +85,23 @@ AC_CONFIG_HEADER(config.h)
AC_ARG_ENABLE(talloc,
[AS_HELP_STRING(
[--disable-talloc],
- [Disable building talloc memory allocator]
+ [Disable using bundled talloc memory allocator]
)],
[enable_talloc=$enableval], [enable_talloc="yes"])
AM_CONDITIONAL(ENABLE_TALLOC, [test x"$enable_talloc" = x"yes"])
+AS_IF([test x"$enable_talloc" = xno],
+ [
+ PKG_CHECK_MODULES([TALLOC], [talloc])
+ OSMOCORE_INTERNAL_TALLOC=0
+ ],
+ [
+ TALLOC_CFLAGS=""
+ TALLOC_LIBS=""
+ OSMOCORE_INTERNAL_TALLOC=1
+ AC_SUBST([TALLOC_CFLAGS])
+ AC_SUBST([TALLOC_LIBS])
+ ])
+AC_SUBST([OSMOCORE_INTERNAL_TALLOC])

AC_ARG_ENABLE(plugin,
[AS_HELP_STRING(
@@ -188,6 +201,7 @@ AC_OUTPUT(
libosmogb.pc
libosmoctrl.pc
include/Makefile
+ include/osmocom/core/talloc.h
src/Makefile
src/vty/Makefile
src/codec/Makefile
diff --git a/include/Makefile.am b/include/Makefile.am
index c59f9b2..4d59a72 100644
--- a/include/Makefile.am
+++ b/include/Makefile.am
@@ -89,8 +89,9 @@ if ENABLE_PLUGIN
nobase_include_HEADERS += osmocom/core/plugin.h
endif

+nobase_nodist_include_HEADERS = osmocom/core/talloc.h
if ENABLE_TALLOC
-nobase_include_HEADERS += osmocom/core/talloc.h
+nobase_include_HEADERS += osmocom/core/talloc_int.h
endif

if ENABLE_MSGFILE
diff --git a/include/osmocom/core/talloc.h b/include/osmocom/core/talloc.h
deleted file mode 100644
index dde102b..0000000
--- a/include/osmocom/core/talloc.h
+++ /dev/null
@@ -1,189 +0,0 @@
-#pragma once
-/*
- Unix SMB/CIFS implementation.
- Samba temporary memory allocation functions
-
- Copyright (C) Andrew Tridgell 2004-2005
- Copyright (C) Stefan Metzmacher 2006
-
- ** NOTE! The following LGPL license applies to the talloc
- ** library. This does NOT imply that all of Samba is released
- ** under the LGPL
-
- This library is free software; you can redistribute it and/or
- modify it under the terms of the GNU Lesser General Public
- License as published by the Free Software Foundation; either
- version 3 of the License, or (at your option) any later version.
-
- This library is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- Lesser General Public License for more details.
-
- You should have received a copy of the GNU Lesser General Public
- License along with this library; if not, see <http://www.gnu.org/licenses/>.
-*/
-
-#include <stdlib.h>
-#include <stdio.h>
-#include <stdarg.h>
-
-#define HAVE_VA_COPY
-
-/* this is only needed for compatibility with the old talloc */
-typedef void TALLOC_CTX;
-
-/*
- this uses a little trick to allow __LINE__ to be stringified
-*/
-#ifndef __location__
-#define __TALLOC_STRING_LINE1__(s) #s
-#define __TALLOC_STRING_LINE2__(s) __TALLOC_STRING_LINE1__(s)
-#define __TALLOC_STRING_LINE3__ __TALLOC_STRING_LINE2__(__LINE__)
-#define __location__ __FILE__ ":" __TALLOC_STRING_LINE3__
-#endif
-
-#ifndef TALLOC_DEPRECATED
-#define TALLOC_DEPRECATED 0
-#endif
-
-#ifndef PRINTF_ATTRIBUTE
-#if (__GNUC__ >= 3)
-/** Use gcc attribute to check printf fns. a1 is the 1-based index of
- * the parameter containing the format, and a2 the index of the first
- * argument. Note that some gcc 2.x versions don't handle this
- * properly **/
-#define PRINTF_ATTRIBUTE(a1, a2) __attribute__ ((format (__printf__, a1, a2)))
-#else
-#define PRINTF_ATTRIBUTE(a1, a2)
-#endif
-#endif
-
-/* try to make talloc_set_destructor() and talloc_steal() type safe,
- if we have a recent gcc */
-#if (__GNUC__ >= 3)
-#define _TALLOC_TYPEOF(ptr) __typeof__(ptr)
-#define talloc_set_destructor(ptr, function) \
- do { \
- int (*_talloc_destructor_fn)(_TALLOC_TYPEOF(ptr)) = (function); \
- _talloc_set_destructor((ptr), (int (*)(void *))_talloc_destructor_fn); \
- } while(0)
-/* this extremely strange macro is to avoid some braindamaged warning
- stupidity in gcc 4.1.x */
-#define talloc_steal(ctx, ptr) ({ _TALLOC_TYPEOF(ptr) __talloc_steal_ret = (_TALLOC_TYPEOF(ptr))_talloc_steal((ctx),(ptr)); __talloc_steal_ret; })
-#else
-#define talloc_set_destructor(ptr, function) \
- _talloc_set_destructor((ptr), (int (*)(void *))(function))
-#define _TALLOC_TYPEOF(ptr) void *
-#define talloc_steal(ctx, ptr) (_TALLOC_TYPEOF(ptr))_talloc_steal((ctx),(ptr))
-#endif
-
-#define talloc_reference(ctx, ptr) (_TALLOC_TYPEOF(ptr))_talloc_reference((ctx),(ptr))
-#define talloc_move(ctx, ptr) (_TALLOC_TYPEOF(*(ptr)))_talloc_move((ctx),(void *)(ptr))
-
-/* useful macros for creating type checked pointers */
-#define talloc(ctx, type) (type *)talloc_named_const(ctx, sizeof(type), #type)
-#define talloc_size(ctx, size) talloc_named_const(ctx, size, __location__)
-#define talloc_ptrtype(ctx, ptr) (_TALLOC_TYPEOF(ptr))talloc_size(ctx, sizeof(*(ptr)))
-
-#define talloc_new(ctx) talloc_named_const(ctx, 0, "talloc_new: " __location__)
-
-#define talloc_zero(ctx, type) (type *)_talloc_zero(ctx, sizeof(type), #type)
-#define talloc_zero_size(ctx, size) _talloc_zero(ctx, size, __location__)
-
-#define talloc_zero_array(ctx, type, count) (type *)_talloc_zero_array(ctx, sizeof(type), count, #type)
-#define talloc_array(ctx, type, count) (type *)_talloc_array(ctx, sizeof(type), count, #type)
-#define talloc_array_size(ctx, size, count) _talloc_array(ctx, size, count, __location__)
-#define talloc_array_ptrtype(ctx, ptr, count) (_TALLOC_TYPEOF(ptr))talloc_array_size(ctx, sizeof(*(ptr)), count)
-#define talloc_array_length(ctx) (talloc_get_size(ctx)/sizeof(*ctx))
-
-#define talloc_realloc(ctx, p, type, count) (type *)_talloc_realloc_array(ctx, p, sizeof(type), count, #type)
-#define talloc_realloc_size(ctx, ptr, size) _talloc_realloc(ctx, ptr, size, __location__)
-
-#define talloc_memdup(t, p, size) _talloc_memdup(t, p, size, __location__)
-
-#define talloc_set_type(ptr, type) talloc_set_name_const(ptr, #type)
-#define talloc_get_type(ptr, type) (type *)talloc_check_name(ptr, #type)
-#define talloc_get_type_abort(ptr, type) (type *)_talloc_get_type_abort(ptr, #type, __location__)
-
-#define talloc_find_parent_bytype(ptr, type) (type *)talloc_find_parent_byname(ptr, #type)
-
-#if TALLOC_DEPRECATED
-#define talloc_zero_p(ctx, type) talloc_zero(ctx, type)
-#define talloc_p(ctx, type) talloc(ctx, type)
-#define talloc_array_p(ctx, type, count) talloc_array(ctx, type, count)
-#define talloc_realloc_p(ctx, p, type, count) talloc_realloc(ctx, p, type, count)
-#define talloc_destroy(ctx) talloc_free(ctx)
-#define talloc_append_string(c, s, a) (s?talloc_strdup_append(s,a):talloc_strdup(c, a))
-#endif
-
-#define TALLOC_FREE(ctx) do { talloc_free(ctx); ctx=NULL; } while(0)
-
-/* The following definitions come from talloc.c */
-void *_talloc(const void *context, size_t size);
-void *talloc_pool(const void *context, size_t size);
-void _talloc_set_destructor(const void *ptr, int (*_destructor)(void *));
-int talloc_increase_ref_count(const void *ptr);
-size_t talloc_reference_count(const void *ptr);
-void *_talloc_reference(const void *context, const void *ptr);
-int talloc_unlink(const void *context, void *ptr);
-const char *talloc_set_name(const void *ptr, const char *fmt, ...) PRINTF_ATTRIBUTE(2,3);
-void talloc_set_name_const(const void *ptr, const char *name);
-void *talloc_named(const void *context, size_t size,
- const char *fmt, ...) PRINTF_ATTRIBUTE(3,4);
-void *talloc_named_const(const void *context, size_t size, const char *name);
-const char *talloc_get_name(const void *ptr);
-void *talloc_check_name(const void *ptr, const char *name);
-void *_talloc_get_type_abort(const void *ptr, const char *name, const char *location);
-void *talloc_parent(const void *ptr);
-const char *talloc_parent_name(const void *ptr);
-void *talloc_init(const char *fmt, ...) PRINTF_ATTRIBUTE(1,2);
-int talloc_free(void *ptr);
-void talloc_free_children(void *ptr);
-void *_talloc_realloc(const void *context, void *ptr, size_t size, const char *name);
-void *_talloc_steal(const void *new_ctx, const void *ptr);
-void *_talloc_move(const void *new_ctx, const void *pptr);
-size_t talloc_total_size(const void *ptr);
-size_t talloc_total_blocks(const void *ptr);
-void talloc_report_depth_cb(const void *ptr, int depth, int max_depth,
- void (*callback)(const void *ptr,
- int depth, int max_depth,
- int is_ref,
- void *private_data),
- void *private_data);
-void talloc_report_depth_file(const void *ptr, int depth, int max_depth, FILE *f);
-void talloc_report_full(const void *ptr, FILE *f);
-void talloc_report(const void *ptr, FILE *f);
-void talloc_enable_null_tracking(void);
-void talloc_disable_null_tracking(void);
-void talloc_enable_leak_report(void);
-void talloc_enable_leak_report_full(void);
-void *_talloc_zero(const void *ctx, size_t size, const char *name);
-void *_talloc_memdup(const void *t, const void *p, size_t size, const char *name);
-void *_talloc_array(const void *ctx, size_t el_size, unsigned count, const char *name);
-void *_talloc_zero_array(const void *ctx, size_t el_size, unsigned count, const char *name);
-void *_talloc_realloc_array(const void *ctx, void *ptr, size_t el_size, unsigned count, const char *name);
-void *talloc_realloc_fn(const void *context, void *ptr, size_t size);
-void *talloc_autofree_context(void);
-size_t talloc_get_size(const void *ctx);
-void *talloc_find_parent_byname(const void *ctx, const char *name);
-void talloc_show_parents(const void *context, FILE *file);
-int talloc_is_parent(const void *context, const void *ptr);
-
-char *talloc_strdup(const void *t, const char *p);
-char *talloc_strdup_append(char *s, const char *a);
-char *talloc_strdup_append_buffer(char *s, const char *a);
-
-char *talloc_strndup(const void *t, const char *p, size_t n);
-char *talloc_strndup_append(char *s, const char *a, size_t n);
-char *talloc_strndup_append_buffer(char *s, const char *a, size_t n);
-
-char *talloc_vasprintf(const void *t, const char *fmt, va_list ap) PRINTF_ATTRIBUTE(2,0);
-char *talloc_vasprintf_append(char *s, const char *fmt, va_list ap) PRINTF_ATTRIBUTE(2,0);
-char *talloc_vasprintf_append_buffer(char *s, const char *fmt, va_list ap) PRINTF_ATTRIBUTE(2,0);
-
-char *talloc_asprintf(const void *t, const char *fmt, ...) PRINTF_ATTRIBUTE(2,3);
-char *talloc_asprintf_append(char *s, const char *fmt, ...) PRINTF_ATTRIBUTE(2,3);
-char *talloc_asprintf_append_buffer(char *s, const char *fmt, ...) PRINTF_ATTRIBUTE(2,3);
-
-void talloc_set_abort_fn(void (*abort_fn)(const char *reason));
diff --git a/include/osmocom/core/talloc.h.in b/include/osmocom/core/talloc.h.in
new file mode 100644
index 0000000..edbbb63
--- /dev/null
+++ b/include/osmocom/core/talloc.h.in
@@ -0,0 +1,6 @@
+#pragma once
+#if @OSMOCORE_INTERNAL_TALLOC@
+#include <osmocom/core/talloc_int.h>
+#else
+#include <talloc.h>
+#endif
diff --git a/include/osmocom/core/talloc_int.h b/include/osmocom/core/talloc_int.h
new file mode 100644
index 0000000..dde102b
--- /dev/null
+++ b/include/osmocom/core/talloc_int.h
@@ -0,0 +1,189 @@
+#pragma once
+/*
+ Unix SMB/CIFS implementation.
+ Samba temporary memory allocation functions
+
+ Copyright (C) Andrew Tridgell 2004-2005
+ Copyright (C) Stefan Metzmacher 2006
+
+ ** NOTE! The following LGPL license applies to the talloc
+ ** library. This does NOT imply that all of Samba is released
+ ** under the LGPL
+
+ This library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 3 of the License, or (at your option) any later version.
+
+ This library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with this library; if not, see <http://www.gnu.org/licenses/>.
+*/
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <stdarg.h>
+
+#define HAVE_VA_COPY
+
+/* this is only needed for compatibility with the old talloc */
+typedef void TALLOC_CTX;
+
+/*
+ this uses a little trick to allow __LINE__ to be stringified
+*/
+#ifndef __location__
+#define __TALLOC_STRING_LINE1__(s) #s
+#define __TALLOC_STRING_LINE2__(s) __TALLOC_STRING_LINE1__(s)
+#define __TALLOC_STRING_LINE3__ __TALLOC_STRING_LINE2__(__LINE__)
+#define __location__ __FILE__ ":" __TALLOC_STRING_LINE3__
+#endif
+
+#ifndef TALLOC_DEPRECATED
+#define TALLOC_DEPRECATED 0
+#endif
+
+#ifndef PRINTF_ATTRIBUTE
+#if (__GNUC__ >= 3)
+/** Use gcc attribute to check printf fns. a1 is the 1-based index of
+ * the parameter containing the format, and a2 the index of the first
+ * argument. Note that some gcc 2.x versions don't handle this
+ * properly **/
+#define PRINTF_ATTRIBUTE(a1, a2) __attribute__ ((format (__printf__, a1, a2)))
+#else
+#define PRINTF_ATTRIBUTE(a1, a2)
+#endif
+#endif
+
+/* try to make talloc_set_destructor() and talloc_steal() type safe,
+ if we have a recent gcc */
+#if (__GNUC__ >= 3)
+#define _TALLOC_TYPEOF(ptr) __typeof__(ptr)
+#define talloc_set_destructor(ptr, function) \
+ do { \
+ int (*_talloc_destructor_fn)(_TALLOC_TYPEOF(ptr)) = (function); \
+ _talloc_set_destructor((ptr), (int (*)(void *))_talloc_destructor_fn); \
+ } while(0)
+/* this extremely strange macro is to avoid some braindamaged warning
+ stupidity in gcc 4.1.x */
+#define talloc_steal(ctx, ptr) ({ _TALLOC_TYPEOF(ptr) __talloc_steal_ret = (_TALLOC_TYPEOF(ptr))_talloc_steal((ctx),(ptr)); __talloc_steal_ret; })
+#else
+#define talloc_set_destructor(ptr, function) \
+ _talloc_set_destructor((ptr), (int (*)(void *))(function))
+#define _TALLOC_TYPEOF(ptr) void *
+#define talloc_steal(ctx, ptr) (_TALLOC_TYPEOF(ptr))_talloc_steal((ctx),(ptr))
+#endif
+
+#define talloc_reference(ctx, ptr) (_TALLOC_TYPEOF(ptr))_talloc_reference((ctx),(ptr))
+#define talloc_move(ctx, ptr) (_TALLOC_TYPEOF(*(ptr)))_talloc_move((ctx),(void *)(ptr))
+
+/* useful macros for creating type checked pointers */
+#define talloc(ctx, type) (type *)talloc_named_const(ctx, sizeof(type), #type)
+#define talloc_size(ctx, size) talloc_named_const(ctx, size, __location__)
+#define talloc_ptrtype(ctx, ptr) (_TALLOC_TYPEOF(ptr))talloc_size(ctx, sizeof(*(ptr)))
+
+#define talloc_new(ctx) talloc_named_const(ctx, 0, "talloc_new: " __location__)
+
+#define talloc_zero(ctx, type) (type *)_talloc_zero(ctx, sizeof(type), #type)
+#define talloc_zero_size(ctx, size) _talloc_zero(ctx, size, __location__)
+
+#define talloc_zero_array(ctx, type, count) (type *)_talloc_zero_array(ctx, sizeof(type), count, #type)
+#define talloc_array(ctx, type, count) (type *)_talloc_array(ctx, sizeof(type), count, #type)
+#define talloc_array_size(ctx, size, count) _talloc_array(ctx, size, count, __location__)
+#define talloc_array_ptrtype(ctx, ptr, count) (_TALLOC_TYPEOF(ptr))talloc_array_size(ctx, sizeof(*(ptr)), count)
+#define talloc_array_length(ctx) (talloc_get_size(ctx)/sizeof(*ctx))
+
+#define talloc_realloc(ctx, p, type, count) (type *)_talloc_realloc_array(ctx, p, sizeof(type), count, #type)
+#define talloc_realloc_size(ctx, ptr, size) _talloc_realloc(ctx, ptr, size, __location__)
+
+#define talloc_memdup(t, p, size) _talloc_memdup(t, p, size, __location__)
+
+#define talloc_set_type(ptr, type) talloc_set_name_const(ptr, #type)
+#define talloc_get_type(ptr, type) (type *)talloc_check_name(ptr, #type)
+#define talloc_get_type_abort(ptr, type) (type *)_talloc_get_type_abort(ptr, #type, __location__)
+
+#define talloc_find_parent_bytype(ptr, type) (type *)talloc_find_parent_byname(ptr, #type)
+
+#if TALLOC_DEPRECATED
+#define talloc_zero_p(ctx, type) talloc_zero(ctx, type)
+#define talloc_p(ctx, type) talloc(ctx, type)
+#define talloc_array_p(ctx, type, count) talloc_array(ctx, type, count)
+#define talloc_realloc_p(ctx, p, type, count) talloc_realloc(ctx, p, type, count)
+#define talloc_destroy(ctx) talloc_free(ctx)
+#define talloc_append_string(c, s, a) (s?talloc_strdup_append(s,a):talloc_strdup(c, a))
+#endif
+
+#define TALLOC_FREE(ctx) do { talloc_free(ctx); ctx=NULL; } while(0)
+
+/* The following definitions come from talloc.c */
+void *_talloc(const void *context, size_t size);
+void *talloc_pool(const void *context, size_t size);
+void _talloc_set_destructor(const void *ptr, int (*_destructor)(void *));
+int talloc_increase_ref_count(const void *ptr);
+size_t talloc_reference_count(const void *ptr);
+void *_talloc_reference(const void *context, const void *ptr);
+int talloc_unlink(const void *context, void *ptr);
+const char *talloc_set_name(const void *ptr, const char *fmt, ...) PRINTF_ATTRIBUTE(2,3);
+void talloc_set_name_const(const void *ptr, const char *name);
+void *talloc_named(const void *context, size_t size,
+ const char *fmt, ...) PRINTF_ATTRIBUTE(3,4);
+void *talloc_named_const(const void *context, size_t size, const char *name);
+const char *talloc_get_name(const void *ptr);
+void *talloc_check_name(const void *ptr, const char *name);
+void *_talloc_get_type_abort(const void *ptr, const char *name, const char *location);
+void *talloc_parent(const void *ptr);
+const char *talloc_parent_name(const void *ptr);
+void *talloc_init(const char *fmt, ...) PRINTF_ATTRIBUTE(1,2);
+int talloc_free(void *ptr);
+void talloc_free_children(void *ptr);
+void *_talloc_realloc(const void *context, void *ptr, size_t size, const char *name);
+void *_talloc_steal(const void *new_ctx, const void *ptr);
+void *_talloc_move(const void *new_ctx, const void *pptr);
+size_t talloc_total_size(const void *ptr);
+size_t talloc_total_blocks(const void *ptr);
+void talloc_report_depth_cb(const void *ptr, int depth, int max_depth,
+ void (*callback)(const void *ptr,
+ int depth, int max_depth,
+ int is_ref,
+ void *private_data),
+ void *private_data);
+void talloc_report_depth_file(const void *ptr, int depth, int max_depth, FILE *f);
+void talloc_report_full(const void *ptr, FILE *f);
+void talloc_report(const void *ptr, FILE *f);
+void talloc_enable_null_tracking(void);
+void talloc_disable_null_tracking(void);
+void talloc_enable_leak_report(void);
+void talloc_enable_leak_report_full(void);
+void *_talloc_zero(const void *ctx, size_t size, const char *name);
+void *_talloc_memdup(const void *t, const void *p, size_t size, const char *name);
+void *_talloc_array(const void *ctx, size_t el_size, unsigned count, const char *name);
+void *_talloc_zero_array(const void *ctx, size_t el_size, unsigned count, const char *name);
+void *_talloc_realloc_array(const void *ctx, void *ptr, size_t el_size, unsigned count, const char *name);
+void *talloc_realloc_fn(const void *context, void *ptr, size_t size);
+void *talloc_autofree_context(void);
+size_t talloc_get_size(const void *ctx);
+void *talloc_find_parent_byname(const void *ctx, const char *name);
+void talloc_show_parents(const void *context, FILE *file);
+int talloc_is_parent(const void *context, const void *ptr);
+
+char *talloc_strdup(const void *t, const char *p);
+char *talloc_strdup_append(char *s, const char *a);
+char *talloc_strdup_append_buffer(char *s, const char *a);
+
+char *talloc_strndup(const void *t, const char *p, size_t n);
+char *talloc_strndup_append(char *s, const char *a, size_t n);
+char *talloc_strndup_append_buffer(char *s, const char *a, size_t n);
+
+char *talloc_vasprintf(const void *t, const char *fmt, va_list ap) PRINTF_ATTRIBUTE(2,0);
+char *talloc_vasprintf_append(char *s, const char *fmt, va_list ap) PRINTF_ATTRIBUTE(2,0);
+char *talloc_vasprintf_append_buffer(char *s, const char *fmt, va_list ap) PRINTF_ATTRIBUTE(2,0);
+
+char *talloc_asprintf(const void *t, const char *fmt, ...) PRINTF_ATTRIBUTE(2,3);
+char *talloc_asprintf_append(char *s, const char *fmt, ...) PRINTF_ATTRIBUTE(2,3);
+char *talloc_asprintf_append_buffer(char *s, const char *fmt, ...) PRINTF_ATTRIBUTE(2,3);
+
+void talloc_set_abort_fn(void (*abort_fn)(const char *reason));
diff --git a/src/Makefile.am b/src/Makefile.am
index 65494a5..3924330 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -2,7 +2,8 @@
# Please read Chapter 6 "Library interface versions" of the libtool documentation before making any modification
LIBVERSION=5:0:0

-AM_CPPFLAGS = -I$(top_srcdir)/include -I$(top_builddir)/include
+AM_CPPFLAGS = -I$(top_srcdir)/include -I$(top_builddir)/include \
+ $(TALLOC_CFLAGS)
AM_CFLAGS = -Wall

lib_LTLIBRARIES = libosmocore.la
@@ -29,7 +30,7 @@ endif
if ENABLE_TALLOC
libosmocore_la_SOURCES += talloc.c
else
-libosmocore_la_LIBADD += -ltalloc
+libosmocore_la_LIBADD += $(TALLOC_LIBS)
endif

if ENABLE_MSGFILE
diff --git a/src/gsm/Makefile.am b/src/gsm/Makefile.am
index b0cdea2..40a1821 100644
--- a/src/gsm/Makefile.am
+++ b/src/gsm/Makefile.am
@@ -2,7 +2,8 @@
# Please read Chapter 6 "Library interface versions" of the libtool documentation before making any modification
LIBVERSION=6:0:1

-AM_CPPFLAGS = -I$(top_srcdir)/include -I$(top_builddir)/include
+AM_CPPFLAGS = -I$(top_srcdir)/include -I$(top_builddir)/include \
+ $(TALLOC_CFLAGS)
AM_CFLAGS = -Wall ${GCC_FVISIBILITY_HIDDEN}

# FIXME: this should eventually go into a milenage/Makefile.am
@@ -23,9 +24,6 @@ libosmogsm_la_SOURCES = a5.c rxlev_stat.c tlv_parser.c comp128.c comp128v23.c \
milenage/milenage.c gan.c ipa.c

libosmogsm_la_LDFLAGS = $(LTLDFLAGS_OSMOGSM) -version-info $(LIBVERSION) -no-undefined
-libosmogsm_la_LIBADD = $(top_builddir)/src/libosmocore.la
-if !ENABLE_TALLOC
-libosmogsm_la_LIBADD += -ltalloc
-endif
+libosmogsm_la_LIBADD = $(top_builddir)/src/libosmocore.la $(TALLOC_LIBS)

EXTRA_DIST = libosmogsm.map
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 3f7db1f..9f034c0 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -1,4 +1,4 @@
-AM_CPPFLAGS = -I$(top_srcdir)/include
+AM_CPPFLAGS = -I$(top_srcdir)/include $(TALLOC_CFLAGS)
AM_CFLAGS = -Wall

check_PROGRAMS = timer/timer_test sms/sms_test ussd/ussd_test \
@@ -45,7 +45,7 @@ lapd_lapd_test_SOURCES = lapd/lapd_test.c
lapd_lapd_test_LDADD = $(top_builddir)/src/libosmocore.la $(top_builddir)/src/gsm/libosmogsm.la

msgfile_msgfile_test_SOURCES = msgfile/msgfile_test.c
-msgfile_msgfile_test_LDADD = $(top_builddir)/src/libosmocore.la
+msgfile_msgfile_test_LDADD = $(top_builddir)/src/libosmocore.la $(TALLOC_LIBS)

smscb_smscb_test_SOURCES = smscb/smscb_test.c
smscb_smscb_test_LDADD = $(top_builddir)/src/libosmocore.la $(top_builddir)/src/gsm/libosmogsm.la
@@ -54,13 +54,13 @@ sms_sms_test_SOURCES = sms/sms_test.c
sms_sms_test_LDADD = $(top_builddir)/src/libosmocore.la $(top_builddir)/src/gsm/libosmogsm.la

timer_timer_test_SOURCES = timer/timer_test.c
-timer_timer_test_LDADD = $(top_builddir)/src/libosmocore.la
+timer_timer_test_LDADD = $(top_builddir)/src/libosmocore.la $(TALLOC_LIBS)

ussd_ussd_test_SOURCES = ussd/ussd_test.c
ussd_ussd_test_LDADD = $(top_builddir)/src/libosmocore.la $(top_builddir)/src/gsm/libosmogsm.la

gb_bssgp_fc_test_SOURCES = gb/bssgp_fc_test.c
-gb_bssgp_fc_test_LDADD = $(top_builddir)/src/libosmocore.la $(top_builddir)/src/gb/libosmogb.la
+gb_bssgp_fc_test_LDADD = $(top_builddir)/src/libosmocore.la $(top_builddir)/src/gb/libosmogb.la $(TALLOC_LIBS)

gb_gprs_ns_test_SOURCES = gb/gprs_ns_test.c
gb_gprs_ns_test_LDADD = $(top_builddir)/src/libosmocore.la $(top_builddir)/src/gb/libosmogb.la $(LIBRARY_DL)
@@ -75,10 +75,10 @@ loggingrb_loggingrb_test_SOURCES = logging/logging_test.c
loggingrb_loggingrb_test_LDADD = $(top_builddir)/src/libosmocore.la $(top_builddir)/src/vty/libosmovty.la

strrb_strrb_test_SOURCES = strrb/strrb_test.c
-strrb_strrb_test_LDADD = $(top_builddir)/src/libosmocore.la
+strrb_strrb_test_LDADD = $(top_builddir)/src/libosmocore.la $(TALLOC_LIBS)

vty_vty_test_SOURCES = vty/vty_test.c
-vty_vty_test_LDADD = $(top_builddir)/src/vty/libosmovty.la $(top_builddir)/src/libosmocore.la
+vty_vty_test_LDADD = $(top_builddir)/src/vty/libosmovty.la $(top_builddir)/src/libosmocore.la $(TALLOC_LIBS)


# The `:;' works around a Bash 3.2 bug when the output is not writeable.
diff --git a/tests/msgfile/msgfile_test.c b/tests/msgfile/msgfile_test.c
index a919694..2684b6a 100644
--- a/tests/msgfile/msgfile_test.c
+++ b/tests/msgfile/msgfile_test.c
@@ -20,6 +20,7 @@
*/

#include <osmocom/core/msgfile.h>
+#include <osmocom/core/talloc.h>

#include <stdio.h>
--
2.0.0
Holger Hans Peter Freyther
2014-10-03 11:19:25 UTC
Permalink
Post by Jan Engelhardt
When --disable-talloc was given to configure, it would already link
libosmocore.so to libtalloc. Make it use pkg-config to discover
talloc's flags and location, and when the system talloc is used,
make sure to use its API header file too.
include/osmocom/core/talloc.h.in | 6 ++
Have you considered patching libosmocore.pc.in as well to list talloc
as "required" there? This way users of libosmocore do not need to check
for talloc themselves?

$ pkg-config --cflags --libs libosmocore
-I/home/ich/install/openbsc/include/ -L/home/ich/install/openbsc/lib -losmocore -ltalloc
Post by Jan Engelhardt
AC_ARG_ENABLE(talloc,
[AS_HELP_STRING(
[--disable-talloc],
- [Disable building talloc memory allocator]
+ [Disable using bundled talloc memory allocator]
)],
[enable_talloc=$enableval], [enable_talloc="yes"])
AM_CONDITIONAL(ENABLE_TALLOC, [test x"$enable_talloc" = x"yes"])
+AS_IF([test x"$enable_talloc" = xno],
I think the wording does not reflect the two different usecases we have here.

* Firmware builds with no talloc
* People wanting to use
Post by Jan Engelhardt
+#pragma once
+#include <osmocom/core/talloc_int.h>
+#else
+#include <talloc.h>
+#endif
Let's hope no-one puts -Iinclude/osmocom/core into the compile line or we
might end up with recursion. :)
Post by Jan Engelhardt
diff --git a/include/osmocom/core/talloc_int.h b/include/osmocom/core/talloc_int.h
new file mode 100644
index 0000000..dde102b
--- /dev/null
+++ b/include/osmocom/core/talloc_int.h
Do you know the '-M' option for send-email/format-patch? It helps with
reviewing simple renames.
Post by Jan Engelhardt
diff --git a/tests/msgfile/msgfile_test.c b/tests/msgfile/msgfile_test.c
index a919694..2684b6a 100644
--- a/tests/msgfile/msgfile_test.c
+++ b/tests/msgfile/msgfile_test.c
@@ -20,6 +20,7 @@
*/
#include <osmocom/core/msgfile.h>
+#include <osmocom/core/talloc.h>
Is this hunk needed?


If you address the pkg-config issue I am happy to inclue your patch. In
terms of options.. maybe we can just remove --disable-talloc instead of
having sort of a tri-state?


thanks a lot
holger
Jan Engelhardt
2014-10-03 11:34:22 UTC
Permalink
Post by Holger Hans Peter Freyther
Post by Jan Engelhardt
When --disable-talloc was given to configure, it would already link
libosmocore.so to libtalloc. Make it use pkg-config to discover
talloc's flags and location, and when the system talloc is used,
make sure to use its API header file too.
include/osmocom/core/talloc.h.in | 6 ++
Have you considered patching libosmocore.pc.in as well to list talloc
as "required" there? This way users of libosmocore do not need to check
for talloc themselves?
I can do that.
Post by Holger Hans Peter Freyther
Post by Jan Engelhardt
AC_ARG_ENABLE(talloc,
[AS_HELP_STRING(
[--disable-talloc],
- [Disable building talloc memory allocator]
+ [Disable using bundled talloc memory allocator]
)],
[enable_talloc=$enableval], [enable_talloc="yes"])
AM_CONDITIONAL(ENABLE_TALLOC, [test x"$enable_talloc" = x"yes"])
+AS_IF([test x"$enable_talloc" = xno],
I think the wording does not reflect the two different usecases we have here.
* Firmware builds with no talloc
That may be the use case, but it is not what was implemented. If you
build 0.7.0 [336915aa0a] with --disable-talloc, `ldd
src/.libs/libosmocore.so` will show libtalloc.so.2. Which kind of
goes against this use case. Therefore, "disable-talloc" really just
meant "disable-system-talloc".
Post by Holger Hans Peter Freyther
Post by Jan Engelhardt
diff --git a/include/osmocom/core/talloc_int.h b/include/osmocom/core/talloc_int.h
new file mode 100644
index 0000000..dde102b
--- /dev/null
+++ b/include/osmocom/core/talloc_int.h
Do you know the '-M' option for send-email/format-patch? It helps with
reviewing simple renames.
Been there, tried that. And when I used it, comes someone who wants
a non-M submission because they are using some traditional utility
or something. Not here, but I remember it occurring on netfilter-devel.
Post by Holger Hans Peter Freyther
Post by Jan Engelhardt
diff --git a/tests/msgfile/msgfile_test.c b/tests/msgfile/msgfile_test.c
index a919694..2684b6a 100644
--- a/tests/msgfile/msgfile_test.c
+++ b/tests/msgfile/msgfile_test.c
@@ -20,6 +20,7 @@
*/
#include <osmocom/core/msgfile.h>
+#include <osmocom/core/talloc.h>
Is this hunk needed?
Yes, because system talloc has a

#define talloc_free(ctx) _talloc_free(ctx, __location__)

and otherwise, one gets "implicit definition of talloc_free" and
"undefined reference to `talloc_free'".
Post by Holger Hans Peter Freyther
If you address the pkg-config issue I am happy to inclue your patch. In
terms of options.. maybe we can just remove --disable-talloc instead of
having sort of a tri-state?
What's the third option?
Peter Stuge
2014-10-03 12:20:53 UTC
Permalink
Post by Jan Engelhardt
Post by Holger Hans Peter Freyther
* Firmware builds with no talloc
That may be the use case, but it is not what was implemented.
Why don't you look into fixing that instead of renaming configure
options - that's just pointless noise.
Post by Jan Engelhardt
If you build 0.7.0 [336915aa0a] with --disable-talloc, `ldd
src/.libs/libosmocore.so` will show libtalloc.so.2.
Do you want to investigate why?
Post by Jan Engelhardt
Post by Holger Hans Peter Freyther
Post by Jan Engelhardt
+++ b/tests/msgfile/msgfile_test.c
@@ -20,6 +20,7 @@
*/
#include <osmocom/core/msgfile.h>
+#include <osmocom/core/talloc.h>
Is this hunk needed?
Yes, because system talloc has a
#define talloc_free(ctx) _talloc_free(ctx, __location__)
and otherwise, one gets "implicit definition of talloc_free" and
"undefined reference to `talloc_free'".
Dude, that is a kludge at a callsite, as opposed to a proper
dependency fix. Try again.


//Peter
Jan Engelhardt
2014-10-03 12:34:46 UTC
Permalink
Post by Peter Stuge
Post by Jan Engelhardt
Post by Holger Hans Peter Freyther
* Firmware builds with no talloc
That may be the use case, but it is not what was implemented.
Why don't you look into fixing that
Why should I? Why can't you?
Post by Peter Stuge
Post by Jan Engelhardt
If you build 0.7.0 [336915aa0a] with --disable-talloc, `ldd
src/.libs/libosmocore.so` will show libtalloc.so.2.
Do you want to investigate why?
Because someone placed -ltalloc into libosmocore_la_LIBADD, the result
of satisfying the calls to talloc that the osmocore code is sprinkled
with. IOW, osmocore provides no alternative paths to using talloc.
Post by Peter Stuge
Post by Jan Engelhardt
Post by Holger Hans Peter Freyther
Post by Jan Engelhardt
+++ b/tests/msgfile/msgfile_test.c
@@ -20,6 +20,7 @@
*/
#include <osmocom/core/msgfile.h>
+#include <osmocom/core/talloc.h>
Is this hunk needed?
Yes, because system talloc has a
#define talloc_free(ctx) _talloc_free(ctx, __location__)
and otherwise, one gets "implicit definition of talloc_free" and
"undefined reference to `talloc_free'".
Dude, that is a kludge at a callsite, as opposed to a proper
dependency fix. Try again.
Well, enlighten us, mr-know-it-all.
Peter Stuge
2014-10-03 13:16:14 UTC
Permalink
Post by Jan Engelhardt
Post by Peter Stuge
Post by Jan Engelhardt
Post by Holger Hans Peter Freyther
* Firmware builds with no talloc
That may be the use case, but it is not what was implemented.
Why don't you look into fixing that
Why should I?
Because you seem to be working with this part of the code now and you
might as well do a thorough job if you're spending time on it.
Post by Jan Engelhardt
Why can't you?
I wish I could.
Post by Jan Engelhardt
Post by Peter Stuge
Post by Jan Engelhardt
Post by Holger Hans Peter Freyther
Post by Jan Engelhardt
+++ b/tests/msgfile/msgfile_test.c
@@ -20,6 +20,7 @@
*/
#include <osmocom/core/msgfile.h>
+#include <osmocom/core/talloc.h>
Is this hunk needed?
Yes, because system talloc has a
#define talloc_free(ctx) _talloc_free(ctx, __location__)
and otherwise, one gets "implicit definition of talloc_free" and
"undefined reference to `talloc_free'".
Dude, that is a kludge at a callsite, as opposed to a proper
dependency fix. Try again.
Well, enlighten us, mr-know-it-all.
Here's a hint at least: Think about symmetry and dependency.


//Peter
Jan Engelhardt
2014-10-03 14:42:55 UTC
Permalink
Post by Peter Stuge
Post by Jan Engelhardt
Post by Peter Stuge
Post by Jan Engelhardt
Post by Holger Hans Peter Freyther
* Firmware builds with no talloc
That may be the use case, but it is not what was implemented.
Why don't you look into fixing that
Why should I?
Because you seem to be working with this part of the code now and you
might as well do a thorough job if you're spending time on it.
I am not going to sprinkle osmocore with defines or otherwise make
talloc pluggable/replaceable by some other set of allocation
functions. I came to fix the problem I have an interest in, and that
is making osmocore support a system talloc in lieu or in addition to
a bundled talloc (->
http://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries),
exercising standard diliegence, which includes that previously
supported cases continue to function to the best of observation.
Post by Peter Stuge
Post by Jan Engelhardt
Post by Peter Stuge
Post by Jan Engelhardt
Post by Holger Hans Peter Freyther
Post by Jan Engelhardt
+++ b/tests/msgfile/msgfile_test.c
@@ -20,6 +20,7 @@
*/
#include <osmocom/core/msgfile.h>
+#include <osmocom/core/talloc.h>
Is this hunk needed?
Yes, because system talloc has a
#define talloc_free(ctx) _talloc_free(ctx, __location__)
and otherwise, one gets "implicit definition of talloc_free" and
"undefined reference to `talloc_free'".
Dude, that is a kludge at a callsite, as opposed to a proper
dependency fix. Try again.
Here's a hint at least: Think about symmetry and dependency.
I have no idea what you mean by that. Talk code, not riddles.
Other code also uses #include <osmocore/core/talloc.h>,
so it does not seem too far-fetched to use the exact line
to support the case of multiple talloc configurations.
Holger Hans Peter Freyther
2014-10-03 20:56:50 UTC
Permalink
On Fri, Oct 03, 2014 at 04:42:55PM +0200, Jan Engelhardt wrote:

Dear Jan, Peter,
Post by Jan Engelhardt
Post by Peter Stuge
Because you seem to be working with this part of the code now and you
might as well do a thorough job if you're spending time on it.
I am not going to sprinkle osmocore with defines or otherwise make
talloc pluggable/replaceable by some other set of allocation
functions. I came to fix the problem I have an interest in, and that
is making osmocore support a system talloc in lieu or in addition to
a bundled talloc (->
http://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries),
exercising standard diliegence, which includes that previously
supported cases continue to function to the best of observation.
Nobody is arguing about that. We should not even have a bundled
copy of talloc.c. The reason we have one is historic (IIRC there
were certainly no -dev/devel packages in any distribution back
then and I am not sure if talloc.c even existed outside the samba
code).

I do appreciate your work to help us move to using the system
version of libtalloc. So let's all calm down.
Post by Jan Engelhardt
Post by Peter Stuge
Post by Jan Engelhardt
Post by Holger Hans Peter Freyther
Is this hunk needed?
Yes, because system talloc has a
#define talloc_free(ctx) _talloc_free(ctx, __location__)
and otherwise, one gets "implicit definition of talloc_free" and
okay, I see the compiler warning now and will fix it.

cheers
holger
Peter Stuge
2014-10-03 22:45:43 UTC
Permalink
Post by Jan Engelhardt
Post by Peter Stuge
Post by Jan Engelhardt
Post by Peter Stuge
Post by Jan Engelhardt
Post by Holger Hans Peter Freyther
* Firmware builds with no talloc
That may be the use case, but it is not what was implemented.
Why don't you look into fixing that
Why should I?
Because you seem to be working with this part of the code now and you
might as well do a thorough job if you're spending time on it.
I am not going to
Okey then.
Post by Jan Engelhardt
Post by Peter Stuge
Post by Jan Engelhardt
Post by Peter Stuge
Post by Jan Engelhardt
Post by Holger Hans Peter Freyther
Post by Jan Engelhardt
+#include <osmocom/core/talloc.h>
Is this hunk needed?
Yes, because system talloc has a
#define talloc_free(ctx) _talloc_free(ctx, __location__)
and otherwise, one gets "implicit definition of talloc_free" and
"undefined reference to `talloc_free'".
Dude, that is a kludge at a callsite, as opposed to a proper
dependency fix. Try again.
Here's a hint at least: Think about symmetry and dependency.
I have no idea what you mean by that.
That's sad.. :\
Post by Jan Engelhardt
Talk code, not riddles.
Other code also uses #include <osmocore/core/talloc.h>,
so it does not seem too far-fetched to use the exact line
to support the case of multiple talloc configurations.
Think more about it.

Why did you add the talloc.h include?
- Because the file calls talloc_free().

Why is talloc_free() called?
- To free memory allocated and returned by osmo_config_list_parse().

Symmetry then dictates that the talloc.h include belongs in msgfile.h,
as opposed to in all files which includes msgfile.h.

I really hope that makes sense. Headers usually do include their
dependencies, rather than require callers to do that on their own.


//Peter
Jan Engelhardt
2014-10-04 08:54:25 UTC
Permalink
Post by Peter Stuge
Why did you add the talloc.h include?
- Because the file calls talloc_free().
Why is talloc_free() called?
- To free memory allocated and returned by osmo_config_list_parse().
Symmetry then dictates that the talloc.h include belongs in msgfile.h,
as opposed to in all files which includes msgfile.h.
Certainly. I even concur with the symmetry argument,
though I feel you have missed one point:

Symmetry would desire that there be a osmo_config_list_free()
function (or macro/static inline function), because the allocation
method of "struct osmo_config_list" would be an implementation detail
of osmo_config_list_parse().
Post by Peter Stuge
I really hope that makes sense. Headers usually do include their
dependencies, rather than require callers to do that on their own.
As of yet though, talloc is not a "direct" dependency of msgfile.h,
that is, there are no names used from talloc.h and
`gcc -x c msgfile.h` will succeed.

With a osmo_config_list_free() function/macro/SI,
the talloc #include would always be at the callsite too,
though the callsite is shifted.

Loading...