Hi List,
The symbol add_key is used by src/providers/krb5/krb5_delayed_online_authentication.c which is part of libsss_krb5_common.so
Fixes following error: [sssd[be[default]]] [load_backend_module] (0x0010): Unable to load ad module with path (/usr/lib64/sssd/libsss_ad.so), error: /usr/lib64/sssd/libsss_krb5_common.so: undefined symbol: add_key
-lkeyutils was passed to the libraries libsss_{krb5,ipa,ad}.so, but when compiling with -Wl,--as-needed this flag will be ignored, since it is not used directly. So it was unavailable to libsss_krb5_common.so which actually needs it.
This patch removes $(KEYUTILS_LIBS) from those libraries and adds it to libsss_krb5_common.so
Maybe libsss_krb5_common.so should be added to dlopen-tests? But then other libraries and functions are needed as well, which it currently inherits from libsss_{krb5,ipa,ad}.so.
BTW: are these common libraries (i mean ldap too) convenience build libraries, or to save disk space? If they're just for convencience maybe they should not be installed?
Regards, Ben
On Wed, 2013-10-16 at 14:49 +0200, Benjamin Franzke wrote:
Hi List,
The symbol add_key is used by src/providers/krb5/krb5_delayed_online_authentication.c which is part of libsss_krb5_common.so
Fixes following error: [sssd[be[default]]] [load_backend_module] (0x0010): Unable to load ad module with path (/usr/lib64/sssd/libsss_ad.so), error: /usr/lib64/sssd/libsss_krb5_common.so: undefined symbol: add_key
-lkeyutils was passed to the libraries libsss_{krb5,ipa,ad}.so, but when compiling with -Wl,--as-needed this flag will be ignored, since it is not used directly. So it was unavailable to libsss_krb5_common.so which actually needs it.
This patch removes $(KEYUTILS_LIBS) from those libraries and adds it to libsss_krb5_common.so
Maybe libsss_krb5_common.so should be added to dlopen-tests?
Yes please.
But then other libraries and functions are needed as well, which it currently inherits from libsss_{krb5,ipa,ad}.so.
You can just open a chain of libraries.
BTW: are these common libraries (i mean ldap too) convenience build libraries, or to save disk space?
Not just to save disk space but to avoid duplicate symbols in multiple modules.
Simo.
On (16/10/13 08:56), Simo Sorce wrote:
On Wed, 2013-10-16 at 14:49 +0200, Benjamin Franzke wrote:
Hi List,
The symbol add_key is used by src/providers/krb5/krb5_delayed_online_authentication.c which is part of libsss_krb5_common.so
Fixes following error: [sssd[be[default]]] [load_backend_module] (0x0010): Unable to load ad module with path (/usr/lib64/sssd/libsss_ad.so), error: /usr/lib64/sssd/libsss_krb5_common.so: undefined symbol: add_key
-lkeyutils was passed to the libraries libsss_{krb5,ipa,ad}.so, but when compiling with -Wl,--as-needed this flag will be ignored, since it is not used directly. So it was unavailable to libsss_krb5_common.so which actually needs it.
This patch removes $(KEYUTILS_LIBS) from those libraries and adds it to libsss_krb5_common.so
Maybe libsss_krb5_common.so should be added to dlopen-tests?
Yes please.
But then other libraries and functions are needed as well, which it currently inherits from libsss_{krb5,ipa,ad}.so.
You can just open a chain of libraries.
BTW: are these common libraries (i mean ldap too) convenience build libraries, or to save disk space?
Not just to save disk space but to avoid duplicate symbols in multiple modules.
Simo.
I am sending rebased patch and patch where libsss_krb5_common.so was added into dlopen-tests.
LS
On Tue, Nov 12, 2013 at 08:23:04AM +0100, Lukas Slebodnik wrote:
On (16/10/13 08:56), Simo Sorce wrote:
On Wed, 2013-10-16 at 14:49 +0200, Benjamin Franzke wrote:
Hi List,
The symbol add_key is used by src/providers/krb5/krb5_delayed_online_authentication.c which is part of libsss_krb5_common.so
Fixes following error: [sssd[be[default]]] [load_backend_module] (0x0010): Unable to load ad module with path (/usr/lib64/sssd/libsss_ad.so), error: /usr/lib64/sssd/libsss_krb5_common.so: undefined symbol: add_key
-lkeyutils was passed to the libraries libsss_{krb5,ipa,ad}.so, but when compiling with -Wl,--as-needed this flag will be ignored, since it is not used directly. So it was unavailable to libsss_krb5_common.so which actually needs it.
This patch removes $(KEYUTILS_LIBS) from those libraries and adds it to libsss_krb5_common.so
Maybe libsss_krb5_common.so should be added to dlopen-tests?
Yes please.
But then other libraries and functions are needed as well, which it currently inherits from libsss_{krb5,ipa,ad}.so.
You can just open a chain of libraries.
BTW: are these common libraries (i mean ldap too) convenience build libraries, or to save disk space?
Not just to save disk space but to avoid duplicate symbols in multiple modules.
Simo.
I am sending rebased patch and patch where libsss_krb5_common.so was added into dlopen-tests.
LS
From 4318c59a8fc327fbc6013c8b424a35a2717fe838 Mon Sep 17 00:00:00 2001 From: Benjamin Franzke benjaminfranzke@googlemail.com Date: Wed, 16 Oct 2013 13:23:48 +0200 Subject: [PATCH 1/3] BUILD: Link libsss_krb5_common.so to libkeyutils.so
ACK
From e1639ee19549d645d84bf3206b2bd1f20964b555 Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik lslebodn@redhat.com Date: Mon, 11 Nov 2013 19:35:13 +0100 Subject: [PATCH 2/3] BUILD: move file find_uid.c into libsss_util.so
Makefile.am | 30 ++++++++---------------------- 1 file changed, 8 insertions(+), 22 deletions(-)
diff --git a/Makefile.am b/Makefile.am index 102797cec02161b0553c2adb3cbf19e83fd50be0..62e31a6fe4a22e8d98f73a6fb3c3e39ae3710555 100644 --- a/Makefile.am +++ b/Makefile.am @@ -590,9 +590,13 @@ libsss_util_la_SOURCES = \ src/util/util_errors.c \ src/util/sss_ini.c \ src/util/io.c \
- src/util/util_sss_idmap.c
- src/util/util_sss_idmap.c \
- src/util/find_uid.c
+libsss_util_la_CFLAGS = \
- $(SYSTEMD_LOGIN_CFLAGS)
libsss_util_la_LIBADD = \ $(SSSD_LIBS) \
- $(SYSTEMD_LOGIN_LIBS) \ $(UNICODE_LIBS)
if BUILD_SUDO libsss_util_la_SOURCES += src/db/sysdb_sudo.c @@ -784,16 +788,13 @@ sss_useradd_LDADD = \
sss_userdel_SOURCES = \ src/tools/sss_userdel.c \
- src/util/find_uid.c \ $(SSSD_LCL_TOOLS_OBJ)
sss_userdel_LDADD = \ $(TOOLS_LIBS) \
- $(SYSTEMD_LOGIN_LIBS) \ $(SSSD_INTERNAL_LTLIBS) \ $(CLIENT_LIBS)
sss_userdel_CFLAGS = \
- $(AM_CFLAGS) \
- $(SYSTEMD_LOGIN_CFLAGS)
- $(AM_CFLAGS)
sss_groupadd_SOURCES = \ src/tools/sss_groupadd.c \ @@ -987,21 +988,18 @@ krb5_utils_tests_SOURCES = \ src/providers/krb5/krb5_become_user.c \ src/providers/krb5/krb5_common.c \ src/util/sss_krb5.c \
- src/util/find_uid.c \ src/providers/data_provider_fo.c \ src/providers/data_provider_opts.c \ src/providers/data_provider_callbacks.c \ $(SSSD_FAILOVER_OBJ)
krb5_utils_tests_CFLAGS = \ $(AM_CFLAGS) \
- $(CHECK_CFLAGS) \
- $(SYSTEMD_LOGIN_CFLAGS)
- $(CHECK_CFLAGS)
krb5_utils_tests_LDADD = \ $(SSSD_LIBS)\ $(CARES_LIBS) \ $(KRB5_LIBS) \ $(CHECK_LIBS) \
- $(SYSTEMD_LOGIN_LIBS) \ $(SSSD_INTERNAL_LTLIBS) \ libsss_test_common.la
@@ -1251,7 +1249,6 @@ krb5_child_test_SOURCES = \ src/providers/krb5/krb5_become_user.c \ src/providers/krb5/krb5_common.c \ src/util/sss_krb5.c \
- src/util/find_uid.c \ src/providers/data_provider_fo.c \ src/providers/data_provider_opts.c \ src/providers/data_provider_callbacks.c \
@@ -1259,14 +1256,12 @@ krb5_child_test_SOURCES = \ krb5_child_test_CFLAGS = \ $(AM_CFLAGS) \ -DKRB5_CHILD_DIR="$(builddir)" \
- $(CHECK_CFLAGS) \
- $(SYSTEMD_LOGIN_CFLAGS)
- $(CHECK_CFLAGS)
krb5_child_test_LDADD = \ $(SSSD_LIBS) \ $(CARES_LIBS) \ $(KRB5_LIBS) \ $(CHECK_LIBS) \
- $(SYSTEMD_LOGIN_LIBS) \ $(SSSD_INTERNAL_LTLIBS) \ libsss_test_common.la
@@ -1392,12 +1387,10 @@ ad_access_filter_tests_SOURCES = \ $(sssd_be_SOURCES) \ src/util/sss_ldap.c \ src/util/sss_krb5.c \
- src/util/find_uid.c \ src/util/user_info_msg.c \ src/tests/cmocka/test_ad_access_filter.c
ad_access_filter_tests_CFLAGS = \ $(AM_CFLAGS) \
- $(SYSTEMD_LOGIN_CFLAGS) \ -DUNIT_TESTING
ad_access_filter_tests_LDADD = \ $(PAM_LIBS) \ @@ -1406,7 +1399,6 @@ ad_access_filter_tests_LDADD = \ $(CARES_LIBS) \ $(KRB5_LIBS) \ $(SSSD_INTERNAL_LTLIBS) \
- $(SYSTEMD_LOGIN_LIBS) \ libsss_ldap_common.la \ libsss_idmap.la \ libsss_krb5_common.la \
@@ -1606,14 +1598,12 @@ libsss_ldap_la_SOURCES = \ src/util/sss_krb5.c libsss_ldap_la_CFLAGS = \ $(AM_CFLAGS) \
- $(SYSTEMD_LOGIN_CFLAGS) \ $(OPENLDAP_CFLAGS) \ $(KRB5_CFLAGS)
libsss_ldap_la_LIBADD = \ $(OPENLDAP_LIBS) \ $(DHASH_LIBS) \ $(KRB5_LIBS) \
- $(SYSTEMD_LOGIN_LIBS) \ libsss_ldap_common.la \ libsss_idmap.la
libsss_ldap_la_LDFLAGS = \ @@ -1749,21 +1739,17 @@ libsss_ad_la_SOURCES = \ src/providers/ad/ad_subdomains.h \ src/providers/ad/ad_domain_info.c \ src/providers/ad/ad_domain_info.h \
- src/util/find_uid.c \ src/util/user_info_msg.c \ src/util/sss_krb5.c \ src/util/sss_ldap.c
libsss_ad_la_CFLAGS = \ $(AM_CFLAGS) \
- $(SYSTEMD_LOGIN_CFLAGS) \ $(OPENLDAP_CFLAGS) \
- $(SASL_CFLAGS) \
Why was SASL_CFLAGS removed here?
$(DHASH_CFLAGS) \ $(KRB5_CFLAGS) \ $(NDR_NBT_CFLAGS)libsss_ad_la_LIBADD = \
- $(SYSTEMD_LOGIN_LIBS) \ $(OPENLDAP_LIBS) \ $(SASL_LIBS) \ $(DHASH_LIBS) \
-- 1.8.3.1
From b044b85ac073ed068796a9365715155c6edd8fd8 Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik lslebodn@redhat.com Date: Mon, 11 Nov 2013 19:39:17 +0100 Subject: [PATCH 3/3] TEST: Add libsss_krb5_common.so to dlopen test
Makefile.am | 6 ++++-- src/tests/dlopen-tests.c | 2 ++ 2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/Makefile.am b/Makefile.am index 62e31a6fe4a22e8d98f73a6fb3c3e39ae3710555..cb23fe4764d7aa831359e21b59a033c32c3a25fa 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1580,9 +1580,11 @@ libsss_krb5_common_la_SOURCES = \ src/providers/krb5/krb5_auth.c \ src/providers/krb5/krb5_access.c \ src/providers/krb5/krb5_child_handler.c \
- src/providers/krb5/krb5_init_shared.c
- src/providers/krb5/krb5_init_shared.c \
- src/util/sss_krb5.c
Why was sss_krb5.c and KRB5_LIBS added in this patch?
libsss_krb5_common_la_LIBADD = \
- $(KEYUTILS_LIBS)
- $(KEYUTILS_LIBS) \
- $(KRB5_LIBS)
libsss_krb5_common_la_LDFLAGS = \ -avoid-version
diff --git a/src/tests/dlopen-tests.c b/src/tests/dlopen-tests.c index dd4cc754ac37621b953a462b5e73696d4556770c..9c09e3334005fdac03be84489777c0e96e84cf4e 100644 --- a/src/tests/dlopen-tests.c +++ b/src/tests/dlopen-tests.c @@ -80,6 +80,8 @@ struct so { LIBPFX"libsss_ipa.so", NULL } }, { "libsss_krb5.so", { LIBPFX"libdlopen_test_providers.so", LIBPFX"libsss_krb5.so", NULL } },
- { "libsss_krb5_common.so", { LIBPFX"libdlopen_test_providers.so",
{ "libsss_ldap.so", { LIBPFX"libdlopen_test_providers.so", LIBPFX"libsss_ldap.so", NULL } }, { "libsss_proxy.so", { LIBPFX"libdlopen_test_providers.so",LIBPFX"libsss_krb5_common.so", NULL } },-- 1.8.3.1
On (12/11/13 11:15), Jakub Hrozek wrote:
On Tue, Nov 12, 2013 at 08:23:04AM +0100, Lukas Slebodnik wrote:
On (16/10/13 08:56), Simo Sorce wrote:
On Wed, 2013-10-16 at 14:49 +0200, Benjamin Franzke wrote:
Hi List,
The symbol add_key is used by src/providers/krb5/krb5_delayed_online_authentication.c which is part of libsss_krb5_common.so
Fixes following error: [sssd[be[default]]] [load_backend_module] (0x0010): Unable to load ad module with path (/usr/lib64/sssd/libsss_ad.so), error: /usr/lib64/sssd/libsss_krb5_common.so: undefined symbol: add_key
-lkeyutils was passed to the libraries libsss_{krb5,ipa,ad}.so, but when compiling with -Wl,--as-needed this flag will be ignored, since it is not used directly. So it was unavailable to libsss_krb5_common.so which actually needs it.
This patch removes $(KEYUTILS_LIBS) from those libraries and adds it to libsss_krb5_common.so
Maybe libsss_krb5_common.so should be added to dlopen-tests?
Yes please.
But then other libraries and functions are needed as well, which it currently inherits from libsss_{krb5,ipa,ad}.so.
You can just open a chain of libraries.
BTW: are these common libraries (i mean ldap too) convenience build libraries, or to save disk space?
Not just to save disk space but to avoid duplicate symbols in multiple modules.
Simo.
I am sending rebased patch and patch where libsss_krb5_common.so was added into dlopen-tests.
LS
From 4318c59a8fc327fbc6013c8b424a35a2717fe838 Mon Sep 17 00:00:00 2001 From: Benjamin Franzke benjaminfranzke@googlemail.com Date: Wed, 16 Oct 2013 13:23:48 +0200 Subject: [PATCH 1/3] BUILD: Link libsss_krb5_common.so to libkeyutils.so
ACK
From e1639ee19549d645d84bf3206b2bd1f20964b555 Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik lslebodn@redhat.com Date: Mon, 11 Nov 2013 19:35:13 +0100 Subject: [PATCH 2/3] BUILD: move file find_uid.c into libsss_util.so
Makefile.am | 30 ++++++++---------------------- 1 file changed, 8 insertions(+), 22 deletions(-)
diff --git a/Makefile.am b/Makefile.am index 102797cec02161b0553c2adb3cbf19e83fd50be0..62e31a6fe4a22e8d98f73a6fb3c3e39ae3710555 100644 --- a/Makefile.am +++ b/Makefile.am @@ -590,9 +590,13 @@ libsss_util_la_SOURCES = \ src/util/util_errors.c \ src/util/sss_ini.c \ src/util/io.c \
- src/util/util_sss_idmap.c
- src/util/util_sss_idmap.c \
- src/util/find_uid.c
+libsss_util_la_CFLAGS = \
- $(SYSTEMD_LOGIN_CFLAGS)
libsss_util_la_LIBADD = \ $(SSSD_LIBS) \
- $(SYSTEMD_LOGIN_LIBS) \ $(UNICODE_LIBS)
if BUILD_SUDO libsss_util_la_SOURCES += src/db/sysdb_sudo.c @@ -784,16 +788,13 @@ sss_useradd_LDADD = \
sss_userdel_SOURCES = \ src/tools/sss_userdel.c \
- src/util/find_uid.c \ $(SSSD_LCL_TOOLS_OBJ)
sss_userdel_LDADD = \ $(TOOLS_LIBS) \
- $(SYSTEMD_LOGIN_LIBS) \ $(SSSD_INTERNAL_LTLIBS) \ $(CLIENT_LIBS)
sss_userdel_CFLAGS = \
- $(AM_CFLAGS) \
- $(SYSTEMD_LOGIN_CFLAGS)
- $(AM_CFLAGS)
sss_groupadd_SOURCES = \ src/tools/sss_groupadd.c \ @@ -987,21 +988,18 @@ krb5_utils_tests_SOURCES = \ src/providers/krb5/krb5_become_user.c \ src/providers/krb5/krb5_common.c \ src/util/sss_krb5.c \
- src/util/find_uid.c \ src/providers/data_provider_fo.c \ src/providers/data_provider_opts.c \ src/providers/data_provider_callbacks.c \ $(SSSD_FAILOVER_OBJ)
krb5_utils_tests_CFLAGS = \ $(AM_CFLAGS) \
- $(CHECK_CFLAGS) \
- $(SYSTEMD_LOGIN_CFLAGS)
- $(CHECK_CFLAGS)
krb5_utils_tests_LDADD = \ $(SSSD_LIBS)\ $(CARES_LIBS) \ $(KRB5_LIBS) \ $(CHECK_LIBS) \
- $(SYSTEMD_LOGIN_LIBS) \ $(SSSD_INTERNAL_LTLIBS) \ libsss_test_common.la
@@ -1251,7 +1249,6 @@ krb5_child_test_SOURCES = \ src/providers/krb5/krb5_become_user.c \ src/providers/krb5/krb5_common.c \ src/util/sss_krb5.c \
- src/util/find_uid.c \ src/providers/data_provider_fo.c \ src/providers/data_provider_opts.c \ src/providers/data_provider_callbacks.c \
@@ -1259,14 +1256,12 @@ krb5_child_test_SOURCES = \ krb5_child_test_CFLAGS = \ $(AM_CFLAGS) \ -DKRB5_CHILD_DIR="$(builddir)" \
- $(CHECK_CFLAGS) \
- $(SYSTEMD_LOGIN_CFLAGS)
- $(CHECK_CFLAGS)
krb5_child_test_LDADD = \ $(SSSD_LIBS) \ $(CARES_LIBS) \ $(KRB5_LIBS) \ $(CHECK_LIBS) \
- $(SYSTEMD_LOGIN_LIBS) \ $(SSSD_INTERNAL_LTLIBS) \ libsss_test_common.la
@@ -1392,12 +1387,10 @@ ad_access_filter_tests_SOURCES = \ $(sssd_be_SOURCES) \ src/util/sss_ldap.c \ src/util/sss_krb5.c \
- src/util/find_uid.c \ src/util/user_info_msg.c \ src/tests/cmocka/test_ad_access_filter.c
ad_access_filter_tests_CFLAGS = \ $(AM_CFLAGS) \
- $(SYSTEMD_LOGIN_CFLAGS) \ -DUNIT_TESTING
ad_access_filter_tests_LDADD = \ $(PAM_LIBS) \ @@ -1406,7 +1399,6 @@ ad_access_filter_tests_LDADD = \ $(CARES_LIBS) \ $(KRB5_LIBS) \ $(SSSD_INTERNAL_LTLIBS) \
- $(SYSTEMD_LOGIN_LIBS) \ libsss_ldap_common.la \ libsss_idmap.la \ libsss_krb5_common.la \
@@ -1606,14 +1598,12 @@ libsss_ldap_la_SOURCES = \ src/util/sss_krb5.c libsss_ldap_la_CFLAGS = \ $(AM_CFLAGS) \
- $(SYSTEMD_LOGIN_CFLAGS) \ $(OPENLDAP_CFLAGS) \ $(KRB5_CFLAGS)
libsss_ldap_la_LIBADD = \ $(OPENLDAP_LIBS) \ $(DHASH_LIBS) \ $(KRB5_LIBS) \
- $(SYSTEMD_LOGIN_LIBS) \ libsss_ldap_common.la \ libsss_idmap.la
libsss_ldap_la_LDFLAGS = \ @@ -1749,21 +1739,17 @@ libsss_ad_la_SOURCES = \ src/providers/ad/ad_subdomains.h \ src/providers/ad/ad_domain_info.c \ src/providers/ad/ad_domain_info.h \
- src/util/find_uid.c \ src/util/user_info_msg.c \ src/util/sss_krb5.c \ src/util/sss_ldap.c
libsss_ad_la_CFLAGS = \ $(AM_CFLAGS) \
- $(SYSTEMD_LOGIN_CFLAGS) \ $(OPENLDAP_CFLAGS) \
- $(SASL_CFLAGS) \
Why was SASL_CFLAGS removed here?
It was a mistake.
$(DHASH_CFLAGS) \ $(KRB5_CFLAGS) \ $(NDR_NBT_CFLAGS)libsss_ad_la_LIBADD = \
- $(SYSTEMD_LOGIN_LIBS) \ $(OPENLDAP_LIBS) \ $(SASL_LIBS) \ $(DHASH_LIBS) \
-- 1.8.3.1
From b044b85ac073ed068796a9365715155c6edd8fd8 Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik lslebodn@redhat.com Date: Mon, 11 Nov 2013 19:39:17 +0100 Subject: [PATCH 3/3] TEST: Add libsss_krb5_common.so to dlopen test
Makefile.am | 6 ++++-- src/tests/dlopen-tests.c | 2 ++ 2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/Makefile.am b/Makefile.am index 62e31a6fe4a22e8d98f73a6fb3c3e39ae3710555..cb23fe4764d7aa831359e21b59a033c32c3a25fa 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1580,9 +1580,11 @@ libsss_krb5_common_la_SOURCES = \ src/providers/krb5/krb5_auth.c \ src/providers/krb5/krb5_access.c \ src/providers/krb5/krb5_child_handler.c \
- src/providers/krb5/krb5_init_shared.c
- src/providers/krb5/krb5_init_shared.c \
- src/util/sss_krb5.c
Why was sss_krb5.c and KRB5_LIBS added in this patch?
Variable KRB5_LIBS was added because libsss_krb5_common.so directly call krb5 functions and dlopen test failed without it.
bash$ objdump -T .libs/libsss_krb5_common.so | grep -E "UND.*krb5_" | tail -n5 0000000000000000 DF *UND* 0000000000000000 krb5_3_MIT krb5_cc_destroy 0000000000000000 DF *UND* 0000000000000000 krb5_3_MIT krb5_set_trace_callback 0000000000000000 DF *UND* 0000000000000000 krb5_3_MIT krb5_parse_name_flags 0000000000000000 DF *UND* 0000000000000000 krb5_3_MIT krb5_kt_resolve 0000000000000000 DF *UND* 0000000000000000 krb5_3_MIT krb5_cc_set_default_name
dlopen test failed without file sss_krb5.c
bash$ ./dlopen-tests Running suite(s): dlopen 0%: Checks: 1, Failures: 1, Errors: 0 ./src/tests/dlopen-tests.c:131:F:dlopen:test_dlopen_base:0: Error opening libsss_krb5_common.so: [dlopen() failed: ./sssd_build/.libs/libsss_krb5_common.so: undefined symbol: check_fast]
Updated patches are attached.
LS
On Tue, 2013-11-12 at 16:23 +0100, Lukas Slebodnik wrote:
On (12/11/13 11:15), Jakub Hrozek wrote:
From b044b85ac073ed068796a9365715155c6edd8fd8 Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik lslebodn@redhat.com Date: Mon, 11 Nov 2013 19:39:17 +0100 Subject: [PATCH 3/3] TEST: Add libsss_krb5_common.so to dlopen test
Makefile.am | 6 ++++-- src/tests/dlopen-tests.c | 2 ++ 2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/Makefile.am b/Makefile.am index 62e31a6fe4a22e8d98f73a6fb3c3e39ae3710555..cb23fe4764d7aa831359e21b59a033c32c3a25fa 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1580,9 +1580,11 @@ libsss_krb5_common_la_SOURCES = \ src/providers/krb5/krb5_auth.c \ src/providers/krb5/krb5_access.c \ src/providers/krb5/krb5_child_handler.c \
- src/providers/krb5/krb5_init_shared.c
- src/providers/krb5/krb5_init_shared.c \
- src/util/sss_krb5.c
Why was sss_krb5.c and KRB5_LIBS added in this patch?
Variable KRB5_LIBS was added because libsss_krb5_common.so directly call krb5 functions and dlopen test failed without it.
bash$ objdump -T .libs/libsss_krb5_common.so | grep -E "UND.*krb5_" | tail -n5 0000000000000000 DF *UND* 0000000000000000 krb5_3_MIT krb5_cc_destroy 0000000000000000 DF *UND* 0000000000000000 krb5_3_MIT krb5_set_trace_callback 0000000000000000 DF *UND* 0000000000000000 krb5_3_MIT krb5_parse_name_flags 0000000000000000 DF *UND* 0000000000000000 krb5_3_MIT krb5_kt_resolve 0000000000000000 DF *UND* 0000000000000000 krb5_3_MIT krb5_cc_set_default_name
dlopen test failed without file sss_krb5.c
bash$ ./dlopen-tests Running suite(s): dlopen 0%: Checks: 1, Failures: 1, Errors: 0 ./src/tests/dlopen-tests.c:131:F:dlopen:test_dlopen_base:0: Error opening libsss_krb5_common.so: [dlopen() failed: ./sssd_build/.libs/libsss_krb5_common.so: undefined symbol: check_fast]
Updated patches are attached.
This is not the right way to fix the failure.
That library *knowingly* has missing symbols because it is an internal library.
You need to build or link to an outer program that have only the necessary symbols you know are going to be available.
In this case those symbols are in libsss_krb5.so, so all you should nee to do is to have this in tests:
+ { "libsss_krb5_common.so", { LIBPFX"libdlopen_test_providers.so", + LIBPFX"libsss_krb5_common.so", + LIBPFX"libsss_krb5.so", + NULL } },
Adding c files to librraies to "make them pass" the dl-open test is wrong in 99% of the cases. you are just making the test pass not testing that the libraries have the right dependencies. In particular for internal modules you really need to understand the hierarchy of the modules and link in the test all the necessary dependencies before thinking of adding any file to the module itself.
In this case what you did was to duplicate symblos in the resulting Text file, and that can potentially cause very serious consequencies if any of those symbols is a static symbols that is meant to have a share de value all code rely on.
Simo.
On Tue, 2013-11-12 at 11:03 -0500, Simo Sorce wrote:
On Tue, 2013-11-12 at 16:23 +0100, Lukas Slebodnik wrote:
Updated patches are attached.
This is not the right way to fix the failure.
That library *knowingly* has missing symbols because it is an internal library.
You need to build or link to an outer program that have only the necessary symbols you know are going to be available.
In this case those symbols are in libsss_krb5.so, so all you should nee to do is to have this in tests:
- { "libsss_krb5_common.so", { LIBPFX"libdlopen_test_providers.so",
LIBPFX"libsss_krb5_common.so",LIBPFX"libsss_krb5.so",NULL } },Adding c files to librraies to "make them pass" the dl-open test is wrong in 99% of the cases. you are just making the test pass not testing that the libraries have the right dependencies. In particular for internal modules you really need to understand the hierarchy of the modules and link in the test all the necessary dependencies before thinking of adding any file to the module itself.
In this case what you did was to duplicate symblos in the resulting Text file, and that can potentially cause very serious consequencies if any of those symbols is a static symbols that is meant to have a share de value all code rely on.
Oh let me add. I did my check very quickly so it is possible that my version is not the final correct approach, I was just describing how you should pass in multiple libs for dependency.
It is possible that you need those symbols directly in libsss_krb5_common.so, but if that is actually the case, then it means you have to remove those symbols from libsss_krb5.so and add them libsss_krb5_common.so as dependency for libsss_krb5.so in makefile and tests. This may even require moving functions from one file to another in the extreme case.
HTH, Simo.
On (12/11/13 11:15), Simo Sorce wrote:
On Tue, 2013-11-12 at 11:03 -0500, Simo Sorce wrote:
On Tue, 2013-11-12 at 16:23 +0100, Lukas Slebodnik wrote:
Updated patches are attached.
This is not the right way to fix the failure.
That library *knowingly* has missing symbols because it is an internal library.
You need to build or link to an outer program that have only the necessary symbols you know are going to be available.
In this case those symbols are in libsss_krb5.so, so all you should nee to do is to have this in tests:
- { "libsss_krb5_common.so", { LIBPFX"libdlopen_test_providers.so",
LIBPFX"libsss_krb5_common.so",LIBPFX"libsss_krb5.so",NULL } },Adding c files to librraies to "make them pass" the dl-open test is wrong in 99% of the cases. you are just making the test pass not testing that the libraries have the right dependencies. In particular for internal modules you really need to understand the hierarchy of the modules and link in the test all the necessary dependencies before thinking of adding any file to the module itself.
In this case what you did was to duplicate symblos in the resulting Text file, and that can potentially cause very serious consequencies if any of those symbols is a static symbols that is meant to have a share de value all code rely on.
Oh let me add. I did my check very quickly so it is possible that my version is not the final correct approach, I was just describing how you should pass in multiple libs for dependency.
It is possible that you need those symbols directly in libsss_krb5_common.so, but if that is actually the case, then it means you have to remove those symbols from libsss_krb5.so and add them libsss_krb5_common.so as dependency for libsss_krb5.so in makefile and tests. This may even require moving functions from one file to another in the extreme case.
zombie patches are returning :-)
I hope all commit messages are self descripting.
They were lot of changes in Makefile.am sh-4.2$ git diff --stat HEAD~6..HEAD Makefile.am Makefile.am | 99 +++++++++++++++++++------------------------------------------ 1 file changed, 31 insertions(+), 68 deletions(-)
LS
On (15/03/14 17:57), Lukas Slebodnik wrote:
On (12/11/13 11:15), Simo Sorce wrote:
On Tue, 2013-11-12 at 11:03 -0500, Simo Sorce wrote:
On Tue, 2013-11-12 at 16:23 +0100, Lukas Slebodnik wrote:
Updated patches are attached.
This is not the right way to fix the failure.
That library *knowingly* has missing symbols because it is an internal library.
You need to build or link to an outer program that have only the necessary symbols you know are going to be available.
In this case those symbols are in libsss_krb5.so, so all you should nee to do is to have this in tests:
- { "libsss_krb5_common.so", { LIBPFX"libdlopen_test_providers.so",
LIBPFX"libsss_krb5_common.so",LIBPFX"libsss_krb5.so",NULL } },Adding c files to librraies to "make them pass" the dl-open test is wrong in 99% of the cases. you are just making the test pass not testing that the libraries have the right dependencies. In particular for internal modules you really need to understand the hierarchy of the modules and link in the test all the necessary dependencies before thinking of adding any file to the module itself.
In this case what you did was to duplicate symblos in the resulting Text file, and that can potentially cause very serious consequencies if any of those symbols is a static symbols that is meant to have a share de value all code rely on.
Oh let me add. I did my check very quickly so it is possible that my version is not the final correct approach, I was just describing how you should pass in multiple libs for dependency.
It is possible that you need those symbols directly in libsss_krb5_common.so, but if that is actually the case, then it means you have to remove those symbols from libsss_krb5.so and add them libsss_krb5_common.so as dependency for libsss_krb5.so in makefile and tests. This may even require moving functions from one file to another in the extreme case.
zombie patches are returning :-)
I hope all commit messages are self descripting.
They were lot of changes in Makefile.am sh-4.2$ git diff --stat HEAD~6..HEAD Makefile.am Makefile.am | 99 +++++++++++++++++++------------------------------------------ 1 file changed, 31 insertions(+), 68 deletions(-)
LS
From 457eaee99fb5afba5f6dd3752efcf78ce19d723c Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik lslebodn@redhat.com Date: Sat, 15 Mar 2014 17:29:25 +0100 Subject: [PATCH 5/6] BUILD: Move duplicated files from providers to libsss_ldap_common.so
self-NACK
make distcheck fails with 5th patch.
LS
On (17/03/14 07:53), Lukas Slebodnik wrote:
On (15/03/14 17:57), Lukas Slebodnik wrote:
On (12/11/13 11:15), Simo Sorce wrote:
On Tue, 2013-11-12 at 11:03 -0500, Simo Sorce wrote:
On Tue, 2013-11-12 at 16:23 +0100, Lukas Slebodnik wrote:
Updated patches are attached.
This is not the right way to fix the failure.
That library *knowingly* has missing symbols because it is an internal library.
You need to build or link to an outer program that have only the necessary symbols you know are going to be available.
In this case those symbols are in libsss_krb5.so, so all you should nee to do is to have this in tests:
- { "libsss_krb5_common.so", { LIBPFX"libdlopen_test_providers.so",
LIBPFX"libsss_krb5_common.so",LIBPFX"libsss_krb5.so",NULL } },Adding c files to librraies to "make them pass" the dl-open test is wrong in 99% of the cases. you are just making the test pass not testing that the libraries have the right dependencies. In particular for internal modules you really need to understand the hierarchy of the modules and link in the test all the necessary dependencies before thinking of adding any file to the module itself.
In this case what you did was to duplicate symblos in the resulting Text file, and that can potentially cause very serious consequencies if any of those symbols is a static symbols that is meant to have a share de value all code rely on.
Oh let me add. I did my check very quickly so it is possible that my version is not the final correct approach, I was just describing how you should pass in multiple libs for dependency.
It is possible that you need those symbols directly in libsss_krb5_common.so, but if that is actually the case, then it means you have to remove those symbols from libsss_krb5.so and add them libsss_krb5_common.so as dependency for libsss_krb5.so in makefile and tests. This may even require moving functions from one file to another in the extreme case.
zombie patches are returning :-)
I hope all commit messages are self descripting.
They were lot of changes in Makefile.am sh-4.2$ git diff --stat HEAD~6..HEAD Makefile.am Makefile.am | 99 +++++++++++++++++++------------------------------------------ 1 file changed, 31 insertions(+), 68 deletions(-)
LS
From 457eaee99fb5afba5f6dd3752efcf78ce19d723c Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik lslebodn@redhat.com Date: Sat, 15 Mar 2014 17:29:25 +0100 Subject: [PATCH 5/6] BUILD: Move duplicated files from providers to libsss_ldap_common.so
self-NACK
make distcheck fails with 5th patch.
Interdiff for 5th patch:
diff --git a/Makefile.am b/Makefile.am index 2d0a295..a65369b 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1742,6 +1742,10 @@ endif # Plugin Libraries # ####################
+# libsss_krb5_common must be installed before libsss_ldap_common +# because libtool tries to relink libsss_ldap_common when installing +# libsss_ldap_common and therefore make distcheck fails +pkglib_LTLIBRARIES += libsss_krb5_common.la pkglib_LTLIBRARIES += libsss_ldap_common.la libsss_ldap_common_la_SOURCES = \ src/providers/ldap/ldap_id.c \ @@ -1778,6 +1782,8 @@ libsss_ldap_common_la_SOURCES = \ src/providers/ldap/sdap.c \ src/util/user_info_msg.c \ src/util/sss_ldap.c libsss_ldap_common_la_LIBADD = \ $(KRB5_LIBS) \ libsss_krb5_common.la \ @@ -1800,11 +1806,7 @@ libsss_ldap_common_la_SOURCES += \ src/providers/ldap/sdap_async_autofs.c endif
libsss_ldap_common_la_CFLAGS = \ $(KRB5_CFLAGS)
-pkglib_LTLIBRARIES += libsss_krb5_common.la libsss_krb5_common_la_SOURCES = \ src/providers/krb5/krb5_utils.c \ src/providers/krb5/krb5_become_user.c \
Updated patches are attached
LS
On Mon, Mar 17, 2014 at 09:34:04AM +0100, Lukas Slebodnik wrote:
On (17/03/14 07:53), Lukas Slebodnik wrote:
On (15/03/14 17:57), Lukas Slebodnik wrote:
On (12/11/13 11:15), Simo Sorce wrote:
On Tue, 2013-11-12 at 11:03 -0500, Simo Sorce wrote:
On Tue, 2013-11-12 at 16:23 +0100, Lukas Slebodnik wrote:
Updated patches are attached.
This is not the right way to fix the failure.
That library *knowingly* has missing symbols because it is an internal library.
You need to build or link to an outer program that have only the necessary symbols you know are going to be available.
In this case those symbols are in libsss_krb5.so, so all you should nee to do is to have this in tests:
- { "libsss_krb5_common.so", { LIBPFX"libdlopen_test_providers.so",
LIBPFX"libsss_krb5_common.so",LIBPFX"libsss_krb5.so",NULL } },Adding c files to librraies to "make them pass" the dl-open test is wrong in 99% of the cases. you are just making the test pass not testing that the libraries have the right dependencies. In particular for internal modules you really need to understand the hierarchy of the modules and link in the test all the necessary dependencies before thinking of adding any file to the module itself.
In this case what you did was to duplicate symblos in the resulting Text file, and that can potentially cause very serious consequencies if any of those symbols is a static symbols that is meant to have a share de value all code rely on.
Oh let me add. I did my check very quickly so it is possible that my version is not the final correct approach, I was just describing how you should pass in multiple libs for dependency.
It is possible that you need those symbols directly in libsss_krb5_common.so, but if that is actually the case, then it means you have to remove those symbols from libsss_krb5.so and add them libsss_krb5_common.so as dependency for libsss_krb5.so in makefile and tests. This may even require moving functions from one file to another in the extreme case.
zombie patches are returning :-)
I hope all commit messages are self descripting.
They were lot of changes in Makefile.am sh-4.2$ git diff --stat HEAD~6..HEAD Makefile.am Makefile.am | 99 +++++++++++++++++++------------------------------------------ 1 file changed, 31 insertions(+), 68 deletions(-)
LS
From 457eaee99fb5afba5f6dd3752efcf78ce19d723c Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik lslebodn@redhat.com Date: Sat, 15 Mar 2014 17:29:25 +0100 Subject: [PATCH 5/6] BUILD: Move duplicated files from providers to libsss_ldap_common.so
self-NACK
make distcheck fails with 5th patch.
Interdiff for 5th patch:
diff --git a/Makefile.am b/Makefile.am index 2d0a295..a65369b 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1742,6 +1742,10 @@ endif # Plugin Libraries # ####################
+# libsss_krb5_common must be installed before libsss_ldap_common +# because libtool tries to relink libsss_ldap_common when installing +# libsss_ldap_common and therefore make distcheck fails +pkglib_LTLIBRARIES += libsss_krb5_common.la pkglib_LTLIBRARIES += libsss_ldap_common.la libsss_ldap_common_la_SOURCES = \ src/providers/ldap/ldap_id.c \ @@ -1778,6 +1782,8 @@ libsss_ldap_common_la_SOURCES = \ src/providers/ldap/sdap.c \ src/util/user_info_msg.c \ src/util/sss_ldap.c libsss_ldap_common_la_LIBADD = \ $(KRB5_LIBS) \ libsss_krb5_common.la \ @@ -1800,11 +1806,7 @@ libsss_ldap_common_la_SOURCES += \ src/providers/ldap/sdap_async_autofs.c endif
libsss_ldap_common_la_CFLAGS = \ $(KRB5_CFLAGS)
-pkglib_LTLIBRARIES += libsss_krb5_common.la libsss_krb5_common_la_SOURCES = \ src/providers/krb5/krb5_utils.c \ src/providers/krb5/krb5_become_user.c \
Updated patches are attached
LS
Sorry for the delay in reviewing the packages.
I think these packages are good and I'd like to ACK them.
The testing consisted of: * visual inspection of the patches. The commit messages help, thanks! * applying the dlopen-test patch on master. The dlopen test then didn't pass. After aplying the other patches, the dlopen test passed as well. * starting IPA, AD and LDAP (with and without ID mapping to be sure) went fine
On Tue, Apr 15, 2014 at 11:02:15AM +0200, Jakub Hrozek wrote:
On Mon, Mar 17, 2014 at 09:34:04AM +0100, Lukas Slebodnik wrote:
On (17/03/14 07:53), Lukas Slebodnik wrote:
On (15/03/14 17:57), Lukas Slebodnik wrote:
On (12/11/13 11:15), Simo Sorce wrote:
On Tue, 2013-11-12 at 11:03 -0500, Simo Sorce wrote:
On Tue, 2013-11-12 at 16:23 +0100, Lukas Slebodnik wrote:
> Updated patches are attached.
This is not the right way to fix the failure.
That library *knowingly* has missing symbols because it is an internal library.
You need to build or link to an outer program that have only the necessary symbols you know are going to be available.
In this case those symbols are in libsss_krb5.so, so all you should nee to do is to have this in tests:
- { "libsss_krb5_common.so", { LIBPFX"libdlopen_test_providers.so",
LIBPFX"libsss_krb5_common.so",LIBPFX"libsss_krb5.so",NULL } },Adding c files to librraies to "make them pass" the dl-open test is wrong in 99% of the cases. you are just making the test pass not testing that the libraries have the right dependencies. In particular for internal modules you really need to understand the hierarchy of the modules and link in the test all the necessary dependencies before thinking of adding any file to the module itself.
In this case what you did was to duplicate symblos in the resulting Text file, and that can potentially cause very serious consequencies if any of those symbols is a static symbols that is meant to have a share de value all code rely on.
Oh let me add. I did my check very quickly so it is possible that my version is not the final correct approach, I was just describing how you should pass in multiple libs for dependency.
It is possible that you need those symbols directly in libsss_krb5_common.so, but if that is actually the case, then it means you have to remove those symbols from libsss_krb5.so and add them libsss_krb5_common.so as dependency for libsss_krb5.so in makefile and tests. This may even require moving functions from one file to another in the extreme case.
zombie patches are returning :-)
I hope all commit messages are self descripting.
They were lot of changes in Makefile.am sh-4.2$ git diff --stat HEAD~6..HEAD Makefile.am Makefile.am | 99 +++++++++++++++++++------------------------------------------ 1 file changed, 31 insertions(+), 68 deletions(-)
LS
From 457eaee99fb5afba5f6dd3752efcf78ce19d723c Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik lslebodn@redhat.com Date: Sat, 15 Mar 2014 17:29:25 +0100 Subject: [PATCH 5/6] BUILD: Move duplicated files from providers to libsss_ldap_common.so
self-NACK
make distcheck fails with 5th patch.
Interdiff for 5th patch:
diff --git a/Makefile.am b/Makefile.am index 2d0a295..a65369b 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1742,6 +1742,10 @@ endif # Plugin Libraries # ####################
+# libsss_krb5_common must be installed before libsss_ldap_common +# because libtool tries to relink libsss_ldap_common when installing +# libsss_ldap_common and therefore make distcheck fails +pkglib_LTLIBRARIES += libsss_krb5_common.la pkglib_LTLIBRARIES += libsss_ldap_common.la libsss_ldap_common_la_SOURCES = \ src/providers/ldap/ldap_id.c \ @@ -1778,6 +1782,8 @@ libsss_ldap_common_la_SOURCES = \ src/providers/ldap/sdap.c \ src/util/user_info_msg.c \ src/util/sss_ldap.c libsss_ldap_common_la_LIBADD = \ $(KRB5_LIBS) \ libsss_krb5_common.la \ @@ -1800,11 +1806,7 @@ libsss_ldap_common_la_SOURCES += \ src/providers/ldap/sdap_async_autofs.c endif
libsss_ldap_common_la_CFLAGS = \ $(KRB5_CFLAGS)
-pkglib_LTLIBRARIES += libsss_krb5_common.la libsss_krb5_common_la_SOURCES = \ src/providers/krb5/krb5_utils.c \ src/providers/krb5/krb5_become_user.c \
Updated patches are attached
LS
Sorry for the delay in reviewing the packages.
I think these packages are good and I'd like to ACK them.
The testing consisted of: * visual inspection of the patches. The commit messages help, thanks! * applying the dlopen-test patch on master. The dlopen test then didn't pass. After aplying the other patches, the dlopen test passed as well. * starting IPA, AD and LDAP (with and without ID mapping to be sure) went fine
Pushed all patches to master in: a25942bc2f6ac9b3b2817ede35fa2b445373c6e4 cc1c033c34b5f816b633d27a21aefbf811a7cf72 6261893e00bd14fdd192ffc9a1379cb9c647d326 12805da52a93c268290cec7b8fbbdbd4ea8abc3e 7fc27c7a3ccbb6aecb8cf4a4a5f91962028cb897 717008c8c3f29f3a1a77266cc72a6cfa616bf295
On (16/04/14 11:12), Jakub Hrozek wrote:
On Tue, Apr 15, 2014 at 11:02:15AM +0200, Jakub Hrozek wrote:
On Mon, Mar 17, 2014 at 09:34:04AM +0100, Lukas Slebodnik wrote:
On (17/03/14 07:53), Lukas Slebodnik wrote:
On (15/03/14 17:57), Lukas Slebodnik wrote:
On (12/11/13 11:15), Simo Sorce wrote:
On Tue, 2013-11-12 at 11:03 -0500, Simo Sorce wrote: > On Tue, 2013-11-12 at 16:23 +0100, Lukas Slebodnik wrote:
> > Updated patches are attached. > > This is not the right way to fix the failure. > > That library *knowingly* has missing symbols because it is an internal > library. > > You need to build or link to an outer program that have only the > necessary symbols you know are going to be available. > > In this case those symbols are in libsss_krb5.so, so all you should nee > to do is to have this in tests: > > + { "libsss_krb5_common.so", { LIBPFX"libdlopen_test_providers.so", > + LIBPFX"libsss_krb5_common.so", > + LIBPFX"libsss_krb5.so", > + NULL } }, > > Adding c files to librraies to "make them pass" the dl-open test is > wrong in 99% of the cases. you are just making the test pass not testing > that the libraries have the right dependencies. In particular for > internal modules you really need to understand the hierarchy of the > modules and link in the test all the necessary dependencies before > thinking of adding any file to the module itself. > > In this case what you did was to duplicate symblos in the resulting Text > file, and that can potentially cause very serious consequencies if any > of those symbols is a static symbols that is meant to have a share de > value all code rely on.
Oh let me add. I did my check very quickly so it is possible that my version is not the final correct approach, I was just describing how you should pass in multiple libs for dependency.
It is possible that you need those symbols directly in libsss_krb5_common.so, but if that is actually the case, then it means you have to remove those symbols from libsss_krb5.so and add them libsss_krb5_common.so as dependency for libsss_krb5.so in makefile and tests. This may even require moving functions from one file to another in the extreme case.
zombie patches are returning :-)
I hope all commit messages are self descripting.
They were lot of changes in Makefile.am sh-4.2$ git diff --stat HEAD~6..HEAD Makefile.am Makefile.am | 99 +++++++++++++++++++------------------------------------------ 1 file changed, 31 insertions(+), 68 deletions(-)
LS
From 457eaee99fb5afba5f6dd3752efcf78ce19d723c Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik lslebodn@redhat.com Date: Sat, 15 Mar 2014 17:29:25 +0100 Subject: [PATCH 5/6] BUILD: Move duplicated files from providers to libsss_ldap_common.so
self-NACK
make distcheck fails with 5th patch.
Interdiff for 5th patch:
diff --git a/Makefile.am b/Makefile.am index 2d0a295..a65369b 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1742,6 +1742,10 @@ endif # Plugin Libraries # ####################
+# libsss_krb5_common must be installed before libsss_ldap_common +# because libtool tries to relink libsss_ldap_common when installing +# libsss_ldap_common and therefore make distcheck fails +pkglib_LTLIBRARIES += libsss_krb5_common.la pkglib_LTLIBRARIES += libsss_ldap_common.la libsss_ldap_common_la_SOURCES = \ src/providers/ldap/ldap_id.c \ @@ -1778,6 +1782,8 @@ libsss_ldap_common_la_SOURCES = \ src/providers/ldap/sdap.c \ src/util/user_info_msg.c \ src/util/sss_ldap.c libsss_ldap_common_la_LIBADD = \ $(KRB5_LIBS) \ libsss_krb5_common.la \ @@ -1800,11 +1806,7 @@ libsss_ldap_common_la_SOURCES += \ src/providers/ldap/sdap_async_autofs.c endif
libsss_ldap_common_la_CFLAGS = \ $(KRB5_CFLAGS)
-pkglib_LTLIBRARIES += libsss_krb5_common.la libsss_krb5_common_la_SOURCES = \ src/providers/krb5/krb5_utils.c \ src/providers/krb5/krb5_become_user.c \
Updated patches are attached
LS
Sorry for the delay in reviewing the packages.
I think these packages are good and I'd like to ACK them.
The testing consisted of: * visual inspection of the patches. The commit messages help, thanks! * applying the dlopen-test patch on master. The dlopen test then didn't pass. After aplying the other patches, the dlopen test passed as well. * starting IPA, AD and LDAP (with and without ID mapping to be sure) went fine
Pushed all patches to master in: a25942bc2f6ac9b3b2817ede35fa2b445373c6e4 cc1c033c34b5f816b633d27a21aefbf811a7cf72 6261893e00bd14fdd192ffc9a1379cb9c647d326 12805da52a93c268290cec7b8fbbdbd4ea8abc3e 7fc27c7a3ccbb6aecb8cf4a4a5f91962028cb897 717008c8c3f29f3a1a77266cc72a6cfa616bf295
Could we push Benjamin's patch to 1.11 branch?
LS
On Wed, Apr 23, 2014 at 11:04:54PM +0200, Lukas Slebodnik wrote:
On (16/04/14 11:12), Jakub Hrozek wrote:
On Tue, Apr 15, 2014 at 11:02:15AM +0200, Jakub Hrozek wrote:
On Mon, Mar 17, 2014 at 09:34:04AM +0100, Lukas Slebodnik wrote:
On (17/03/14 07:53), Lukas Slebodnik wrote:
On (15/03/14 17:57), Lukas Slebodnik wrote:
On (12/11/13 11:15), Simo Sorce wrote: >On Tue, 2013-11-12 at 11:03 -0500, Simo Sorce wrote: >> On Tue, 2013-11-12 at 16:23 +0100, Lukas Slebodnik wrote: > >> > Updated patches are attached. >> >> This is not the right way to fix the failure. >> >> That library *knowingly* has missing symbols because it is an internal >> library. >> >> You need to build or link to an outer program that have only the >> necessary symbols you know are going to be available. >> >> In this case those symbols are in libsss_krb5.so, so all you should nee >> to do is to have this in tests: >> >> + { "libsss_krb5_common.so", { LIBPFX"libdlopen_test_providers.so", >> + LIBPFX"libsss_krb5_common.so", >> + LIBPFX"libsss_krb5.so", >> + NULL } }, >> >> Adding c files to librraies to "make them pass" the dl-open test is >> wrong in 99% of the cases. you are just making the test pass not testing >> that the libraries have the right dependencies. In particular for >> internal modules you really need to understand the hierarchy of the >> modules and link in the test all the necessary dependencies before >> thinking of adding any file to the module itself. >> >> In this case what you did was to duplicate symblos in the resulting Text >> file, and that can potentially cause very serious consequencies if any >> of those symbols is a static symbols that is meant to have a share de >> value all code rely on. > >Oh let me add. >I did my check very quickly so it is possible that my version is not the >final correct approach, I was just describing how you should pass in >multiple libs for dependency. > >It is possible that you need those symbols directly in >libsss_krb5_common.so, but if that is actually the case, then it means >you have to remove those symbols from libsss_krb5.so and add them >libsss_krb5_common.so as dependency for libsss_krb5.so in makefile and >tests. This may even require moving functions from one file to another >in the extreme case. >
zombie patches are returning :-)
I hope all commit messages are self descripting.
They were lot of changes in Makefile.am sh-4.2$ git diff --stat HEAD~6..HEAD Makefile.am Makefile.am | 99 +++++++++++++++++++------------------------------------------ 1 file changed, 31 insertions(+), 68 deletions(-)
LS
From 457eaee99fb5afba5f6dd3752efcf78ce19d723c Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik lslebodn@redhat.com Date: Sat, 15 Mar 2014 17:29:25 +0100 Subject: [PATCH 5/6] BUILD: Move duplicated files from providers to libsss_ldap_common.so
self-NACK
make distcheck fails with 5th patch.
Interdiff for 5th patch:
diff --git a/Makefile.am b/Makefile.am index 2d0a295..a65369b 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1742,6 +1742,10 @@ endif # Plugin Libraries # ####################
+# libsss_krb5_common must be installed before libsss_ldap_common +# because libtool tries to relink libsss_ldap_common when installing +# libsss_ldap_common and therefore make distcheck fails +pkglib_LTLIBRARIES += libsss_krb5_common.la pkglib_LTLIBRARIES += libsss_ldap_common.la libsss_ldap_common_la_SOURCES = \ src/providers/ldap/ldap_id.c \ @@ -1778,6 +1782,8 @@ libsss_ldap_common_la_SOURCES = \ src/providers/ldap/sdap.c \ src/util/user_info_msg.c \ src/util/sss_ldap.c libsss_ldap_common_la_LIBADD = \ $(KRB5_LIBS) \ libsss_krb5_common.la \ @@ -1800,11 +1806,7 @@ libsss_ldap_common_la_SOURCES += \ src/providers/ldap/sdap_async_autofs.c endif
libsss_ldap_common_la_CFLAGS = \ $(KRB5_CFLAGS)
-pkglib_LTLIBRARIES += libsss_krb5_common.la libsss_krb5_common_la_SOURCES = \ src/providers/krb5/krb5_utils.c \ src/providers/krb5/krb5_become_user.c \
Updated patches are attached
LS
Sorry for the delay in reviewing the packages.
I think these packages are good and I'd like to ACK them.
The testing consisted of: * visual inspection of the patches. The commit messages help, thanks! * applying the dlopen-test patch on master. The dlopen test then didn't pass. After aplying the other patches, the dlopen test passed as well. * starting IPA, AD and LDAP (with and without ID mapping to be sure) went fine
Pushed all patches to master in: a25942bc2f6ac9b3b2817ede35fa2b445373c6e4 cc1c033c34b5f816b633d27a21aefbf811a7cf72 6261893e00bd14fdd192ffc9a1379cb9c647d326 12805da52a93c268290cec7b8fbbdbd4ea8abc3e 7fc27c7a3ccbb6aecb8cf4a4a5f91962028cb897 717008c8c3f29f3a1a77266cc72a6cfa616bf295
Could we push Benjamin's patch to 1.11 branch?
LS
Sure, did you only want to push Benjamin's patch (717008c8c3f29f3a1a77266cc72a6cfa616bf295 in master) or the whole set?
On (25/04/14 11:43), Jakub Hrozek wrote:
On Wed, Apr 23, 2014 at 11:04:54PM +0200, Lukas Slebodnik wrote:
On (16/04/14 11:12), Jakub Hrozek wrote:
On Tue, Apr 15, 2014 at 11:02:15AM +0200, Jakub Hrozek wrote:
On Mon, Mar 17, 2014 at 09:34:04AM +0100, Lukas Slebodnik wrote:
On (17/03/14 07:53), Lukas Slebodnik wrote:
On (15/03/14 17:57), Lukas Slebodnik wrote: >On (12/11/13 11:15), Simo Sorce wrote: >>On Tue, 2013-11-12 at 11:03 -0500, Simo Sorce wrote: >>> On Tue, 2013-11-12 at 16:23 +0100, Lukas Slebodnik wrote: >> >>> > Updated patches are attached. >>> >>> This is not the right way to fix the failure. >>> >>> That library *knowingly* has missing symbols because it is an internal >>> library. >>> >>> You need to build or link to an outer program that have only the >>> necessary symbols you know are going to be available. >>> >>> In this case those symbols are in libsss_krb5.so, so all you should nee >>> to do is to have this in tests: >>> >>> + { "libsss_krb5_common.so", { LIBPFX"libdlopen_test_providers.so", >>> + LIBPFX"libsss_krb5_common.so", >>> + LIBPFX"libsss_krb5.so", >>> + NULL } }, >>> >>> Adding c files to librraies to "make them pass" the dl-open test is >>> wrong in 99% of the cases. you are just making the test pass not testing >>> that the libraries have the right dependencies. In particular for >>> internal modules you really need to understand the hierarchy of the >>> modules and link in the test all the necessary dependencies before >>> thinking of adding any file to the module itself. >>> >>> In this case what you did was to duplicate symblos in the resulting Text >>> file, and that can potentially cause very serious consequencies if any >>> of those symbols is a static symbols that is meant to have a share de >>> value all code rely on. >> >>Oh let me add. >>I did my check very quickly so it is possible that my version is not the >>final correct approach, I was just describing how you should pass in >>multiple libs for dependency. >> >>It is possible that you need those symbols directly in >>libsss_krb5_common.so, but if that is actually the case, then it means >>you have to remove those symbols from libsss_krb5.so and add them >>libsss_krb5_common.so as dependency for libsss_krb5.so in makefile and >>tests. This may even require moving functions from one file to another >>in the extreme case. >> > >zombie patches are returning :-) > >I hope all commit messages are self descripting. > >They were lot of changes in Makefile.am >sh-4.2$ git diff --stat HEAD~6..HEAD Makefile.am > Makefile.am | 99 +++++++++++++++++++------------------------------------------ > 1 file changed, 31 insertions(+), 68 deletions(-) > >LS
>From 457eaee99fb5afba5f6dd3752efcf78ce19d723c Mon Sep 17 00:00:00 2001 >From: Lukas Slebodnik lslebodn@redhat.com >Date: Sat, 15 Mar 2014 17:29:25 +0100 >Subject: [PATCH 5/6] BUILD: Move duplicated files from providers to > libsss_ldap_common.so >
self-NACK
make distcheck fails with 5th patch.
Interdiff for 5th patch:
diff --git a/Makefile.am b/Makefile.am index 2d0a295..a65369b 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1742,6 +1742,10 @@ endif # Plugin Libraries # ####################
+# libsss_krb5_common must be installed before libsss_ldap_common +# because libtool tries to relink libsss_ldap_common when installing +# libsss_ldap_common and therefore make distcheck fails +pkglib_LTLIBRARIES += libsss_krb5_common.la pkglib_LTLIBRARIES += libsss_ldap_common.la libsss_ldap_common_la_SOURCES = \ src/providers/ldap/ldap_id.c \ @@ -1778,6 +1782,8 @@ libsss_ldap_common_la_SOURCES = \ src/providers/ldap/sdap.c \ src/util/user_info_msg.c \ src/util/sss_ldap.c libsss_ldap_common_la_LIBADD = \ $(KRB5_LIBS) \ libsss_krb5_common.la \ @@ -1800,11 +1806,7 @@ libsss_ldap_common_la_SOURCES += \ src/providers/ldap/sdap_async_autofs.c endif
libsss_ldap_common_la_CFLAGS = \ $(KRB5_CFLAGS)
-pkglib_LTLIBRARIES += libsss_krb5_common.la libsss_krb5_common_la_SOURCES = \ src/providers/krb5/krb5_utils.c \ src/providers/krb5/krb5_become_user.c \
Updated patches are attached
LS
Sorry for the delay in reviewing the packages.
I think these packages are good and I'd like to ACK them.
The testing consisted of: * visual inspection of the patches. The commit messages help, thanks! * applying the dlopen-test patch on master. The dlopen test then didn't pass. After aplying the other patches, the dlopen test passed as well. * starting IPA, AD and LDAP (with and without ID mapping to be sure) went fine
Pushed all patches to master in: a25942bc2f6ac9b3b2817ede35fa2b445373c6e4 cc1c033c34b5f816b633d27a21aefbf811a7cf72 6261893e00bd14fdd192ffc9a1379cb9c647d326 12805da52a93c268290cec7b8fbbdbd4ea8abc3e 7fc27c7a3ccbb6aecb8cf4a4a5f91962028cb897 717008c8c3f29f3a1a77266cc72a6cfa616bf295
Could we push Benjamin's patch to 1.11 branch?
LS
Sure, did you only want to push Benjamin's patch (717008c8c3f29f3a1a77266cc72a6cfa616bf295 in master) or the whole set?
just Benjamin's patch. Other patches will probably not apply to 1.11 branch and they were necessary mostly for dlopen test.
LS
On Fri, Apr 25, 2014 at 11:56:38AM +0200, Lukas Slebodnik wrote:
Could we push Benjamin's patch to 1.11 branch?
LS
Sure, did you only want to push Benjamin's patch (717008c8c3f29f3a1a77266cc72a6cfa616bf295 in master) or the whole set?
just Benjamin's patch. Other patches will probably not apply to 1.11 branch and they were necessary mostly for dlopen test.
OK, no problem. Pushed to sssd-1-11: 917ff0aa43d3858df1b82f75608601622c12ba57
On (28/04/14 22:31), Jakub Hrozek wrote:
On Fri, Apr 25, 2014 at 11:56:38AM +0200, Lukas Slebodnik wrote:
Could we push Benjamin's patch to 1.11 branch?
LS
Sure, did you only want to push Benjamin's patch (717008c8c3f29f3a1a77266cc72a6cfa616bf295 in master) or the whole set?
just Benjamin's patch. Other patches will probably not apply to 1.11 branch and they were necessary mostly for dlopen test.
OK, no problem. Pushed to sssd-1-11: 917ff0aa43d3858df1b82f75608601622c12ba57
Thank you
LS
sssd-devel@lists.fedorahosted.org