While reviewing the recent big patchset I sent Sumit found out I did not add some .c files to the ldap provider (which I didn't test as I was working on the krb5 provider), and that broke the provider's shared library as one symbols was missing.
We allow modules to have unresolved symbols because a number of the symbols they use come from our sssd_be binary so they can't be fully resolved at build time, only at runtime.
The attached patch adds a test that simulates loading most of the shared modules we build in order to verify that there are no unresolved symbols, that is done by manually dlopen()ing eash .so library with RTLD_NOW so all symbols are immediately resolved.
I tested that this test properly caught the issue Sumit found before fixing it.
Simo.
On Thu, Sep 05, 2013 at 09:48:27PM -0400, Simo Sorce wrote:
While reviewing the recent big patchset I sent Sumit found out I did not add some .c files to the ldap provider (which I didn't test as I was working on the krb5 provider), and that broke the provider's shared library as one symbols was missing.
We allow modules to have unresolved symbols because a number of the symbols they use come from our sssd_be binary so they can't be fully resolved at build time, only at runtime.
The attached patch adds a test that simulates loading most of the shared modules we build in order to verify that there are no unresolved symbols, that is done by manually dlopen()ing eash .so library with RTLD_NOW so all symbols are immediately resolved.
I tested that this test properly caught the issue Sumit found before fixing it.
Simo.
Hi,
I think in general this is a great test to have, thank you. See some minor comments below:
-- Simo Sorce * Red Hat, Inc * New York
From 53d8be3d2b543fec3aa83a7cfa86dcd87c7425d7 Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Thu, 5 Sep 2013 11:52:08 -0400 Subject: [PATCH] tests: Add dlopen test to make sure modules works
This tests dlopens and resolves all symbols to make sure there are no missing symbols in our provider modules.
Makefile.am | 23 ++++++++++ src/tests/dlopen-tests.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 135 insertions(+) create mode 100644 src/tests/dlopen-tests.c
diff --git a/Makefile.am b/Makefile.am index 05da9a14d82d4d0c11ec2eb888e6af46c6e48ff7..6aad7530120e783c9a1c937110b22b49cf6139eb 100644 --- a/Makefile.am +++ b/Makefile.am @@ -114,6 +114,7 @@ endif
if HAVE_CHECK non_interactive_check_based_tests = \
- dlopen-tests \
Mixed tab and space
sysdb-tests \ strtonum-tests \ resolv-tests \
index 0000000000000000000000000000000000000000..4067162eb63feb0ebdf21b250f83bdfcb4654182 --- /dev/null +++ b/src/tests/dlopen-tests.c @@ -0,0 +1,112 @@ +/*
- SSSD
- debug-tests.c
- Authors:
Pavel Březina <pbrezina@redhat.com>
You should credit yourself :)
- Copyright (C) 2011 Red Hat
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 3 of the License, or
- (at your option) any later version.
- This program 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 General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+#define _GNU_SOURCE +#include <stdbool.h> +#include <dlfcn.h> +#include <stdio.h> +#include <stdlib.h> +#include <limits.h> +#include <check.h> +#include "tests/common.h"
+#define LIBPFX DLOPEN_TESTS_BUILDDIR"/.libs/"
We talked about this hardcoded directory on IRC and I can't see another way to autodetect this either.
+const char *sonames[] = {
- "libipa_hbac.so",
- "libsss_ad.so",
- "libsss_child.so",
- "libsss_debug.so",
- "libsss_ipa.so",
- "libsss_krb5.so",
- "libsss_ldap.so",
- "libsss_proxy.so",
- "libsss_sudo.so",
- "memberof.so",
- "pyhbac.so",
- "pysss_nss_idmap.so",
- "sssd_krb5_locator_plugin.so",
- "libnss_sss.so",
- "libsss_autofs.so",
- "libsss_crypt.so",
- "libsss_idmap.so",
- "libsss_nss_idmap.so",
- "libsss_simple.so",
- "libsss_util.so",
- "pam_sss.so",
- "sssd_pac_plugin.so",
- NULL
+};
I wonder if there's any way of autogenerating this maybe from LTLIBRARIES? But definitely not something worth fixing in this patch instance.
+START_TEST(test_dlopen_base) +{
- void *handle;
- char *soname;
- int ret, i;
- for (i = 0; sonames[i] != NULL; i++) {
ret = asprintf(&soname, LIBPFX"%s", sonames[i]);fail_unless(ret != -1, "Failed to construct soname");handle = dlopen(soname, RTLD_NOW);fail_unless(handle != NULL,"dlopen() failed for %s: [%s]", soname, dlerror());dlclose(handle);free(soname);- }
+} +END_TEST
+Suite *dlopen_suite(void) +{
- Suite *s = suite_create("dlopen");
- TCase *tc_dlopen = tcase_create("dlopen");
- tcase_add_test(tc_dlopen, test_dlopen_base);
- tcase_set_timeout(tc_dlopen, 10);
- suite_add_tcase(s, tc_dlopen);
- return s;
+}
+int main(int argc, const char *argv[]) +{
- int number_failed;
- tests_set_cwd();
I think this test is a special case where we actually should not call tests_set_cwd(). Calling it breaks the parallel build for me.
- Suite *s = dlopen_suite();
- SRunner *sr = srunner_create(s);
- srunner_run_all(sr, CK_NORMAL);
- number_failed = srunner_ntests_failed(sr);
- srunner_free(sr);
- if (number_failed == 0)
return EXIT_SUCCESS;- return EXIT_FAILURE;
+}
1.8.3.1
Otherwise looks good to me!
On Fri, 2013-09-06 at 13:50 +0200, Jakub Hrozek wrote:
On Thu, Sep 05, 2013 at 09:48:27PM -0400, Simo Sorce wrote:
While reviewing the recent big patchset I sent Sumit found out I did not add some .c files to the ldap provider (which I didn't test as I was working on the krb5 provider), and that broke the provider's shared library as one symbols was missing.
We allow modules to have unresolved symbols because a number of the symbols they use come from our sssd_be binary so they can't be fully resolved at build time, only at runtime.
The attached patch adds a test that simulates loading most of the shared modules we build in order to verify that there are no unresolved symbols, that is done by manually dlopen()ing eash .so library with RTLD_NOW so all symbols are immediately resolved.
I tested that this test properly caught the issue Sumit found before fixing it.
Simo.
Hi,
I think in general this is a great test to have, thank you. See some minor comments below:
-- Simo Sorce * Red Hat, Inc * New York
From 53d8be3d2b543fec3aa83a7cfa86dcd87c7425d7 Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Thu, 5 Sep 2013 11:52:08 -0400 Subject: [PATCH] tests: Add dlopen test to make sure modules works
This tests dlopens and resolves all symbols to make sure there are no missing symbols in our provider modules.
Makefile.am | 23 ++++++++++ src/tests/dlopen-tests.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 135 insertions(+) create mode 100644 src/tests/dlopen-tests.c
diff --git a/Makefile.am b/Makefile.am index 05da9a14d82d4d0c11ec2eb888e6af46c6e48ff7..6aad7530120e783c9a1c937110b22b49cf6139eb 100644 --- a/Makefile.am +++ b/Makefile.am @@ -114,6 +114,7 @@ endif
if HAVE_CHECK non_interactive_check_based_tests = \
- dlopen-tests \
Mixed tab and space
Fixed.
sysdb-tests \ strtonum-tests \ resolv-tests \index 0000000000000000000000000000000000000000..4067162eb63feb0ebdf21b250f83bdfcb4654182 --- /dev/null +++ b/src/tests/dlopen-tests.c @@ -0,0 +1,112 @@ +/*
- SSSD
- debug-tests.c
- Authors:
Pavel Březina <pbrezina@redhat.com>You should credit yourself :)
Indeed
- Copyright (C) 2011 Red Hat
And changed year too ^^
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 3 of the License, or
- (at your option) any later version.
- This program 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 General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+#define _GNU_SOURCE +#include <stdbool.h> +#include <dlfcn.h> +#include <stdio.h> +#include <stdlib.h> +#include <limits.h> +#include <check.h> +#include "tests/common.h"
+#define LIBPFX DLOPEN_TESTS_BUILDDIR"/.libs/"
We talked about this hardcoded directory on IRC and I can't see another way to autodetect this either.
So far no good way indeed, the joys of libtool ...
+const char *sonames[] = {
- "libipa_hbac.so",
- "libsss_ad.so",
- "libsss_child.so",
- "libsss_debug.so",
- "libsss_ipa.so",
- "libsss_krb5.so",
- "libsss_ldap.so",
- "libsss_proxy.so",
- "libsss_sudo.so",
- "memberof.so",
- "pyhbac.so",
- "pysss_nss_idmap.so",
- "sssd_krb5_locator_plugin.so",
- "libnss_sss.so",
- "libsss_autofs.so",
- "libsss_crypt.so",
- "libsss_idmap.so",
- "libsss_nss_idmap.so",
- "libsss_simple.so",
- "libsss_util.so",
- "pam_sss.so",
- "sssd_pac_plugin.so",
- NULL
+};
I wonder if there's any way of autogenerating this maybe from LTLIBRARIES? But definitely not something worth fixing in this patch instance.
Not sure, if you look at the list carefully you'll see 2 .so objects are missing (sss_krb5_common.so and sss_ldap_common.so) because they would fail the test on their own and they are already implicitly tested by provider modules which have these libraries as dependencies (see output of ldd .libs/libss_ldap.so for example).
I actually initially wanted to restrict to only the providers librraries given all others are built with dependency checking, but given it cost nothing to test them all and I used a sed command to go from ls -> list and it all worked I left most of them in there.
+START_TEST(test_dlopen_base) +{
- void *handle;
- char *soname;
- int ret, i;
- for (i = 0; sonames[i] != NULL; i++) {
ret = asprintf(&soname, LIBPFX"%s", sonames[i]);fail_unless(ret != -1, "Failed to construct soname");handle = dlopen(soname, RTLD_NOW);fail_unless(handle != NULL,"dlopen() failed for %s: [%s]", soname, dlerror());dlclose(handle);free(soname);- }
+} +END_TEST
+Suite *dlopen_suite(void) +{
- Suite *s = suite_create("dlopen");
- TCase *tc_dlopen = tcase_create("dlopen");
- tcase_add_test(tc_dlopen, test_dlopen_base);
- tcase_set_timeout(tc_dlopen, 10);
- suite_add_tcase(s, tc_dlopen);
- return s;
+}
+int main(int argc, const char *argv[]) +{
- int number_failed;
- tests_set_cwd();
I think this test is a special case where we actually should not call tests_set_cwd(). Calling it breaks the parallel build for me.
Removed.
- Suite *s = dlopen_suite();
- SRunner *sr = srunner_create(s);
- srunner_run_all(sr, CK_NORMAL);
- number_failed = srunner_ntests_failed(sr);
- srunner_free(sr);
- if (number_failed == 0)
return EXIT_SUCCESS;- return EXIT_FAILURE;
+}
1.8.3.1
Otherwise looks good to me!
Ok, fixed patch attached.
Simo.
On (06/09/13 09:02), Simo Sorce wrote:
On Fri, 2013-09-06 at 13:50 +0200, Jakub Hrozek wrote:
On Thu, Sep 05, 2013 at 09:48:27PM -0400, Simo Sorce wrote:
While reviewing the recent big patchset I sent Sumit found out I did not add some .c files to the ldap provider (which I didn't test as I was working on the krb5 provider), and that broke the provider's shared library as one symbols was missing.
We allow modules to have unresolved symbols because a number of the symbols they use come from our sssd_be binary so they can't be fully resolved at build time, only at runtime.
The attached patch adds a test that simulates loading most of the shared modules we build in order to verify that there are no unresolved symbols, that is done by manually dlopen()ing eash .so library with RTLD_NOW so all symbols are immediately resolved.
I tested that this test properly caught the issue Sumit found before fixing it.
Simo.
Hi,
I think in general this is a great test to have, thank you. See some minor comments below:
-- Simo Sorce * Red Hat, Inc * New York
From 53d8be3d2b543fec3aa83a7cfa86dcd87c7425d7 Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Thu, 5 Sep 2013 11:52:08 -0400 Subject: [PATCH] tests: Add dlopen test to make sure modules works
This tests dlopens and resolves all symbols to make sure there are no missing symbols in our provider modules.
Makefile.am | 23 ++++++++++ src/tests/dlopen-tests.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 135 insertions(+) create mode 100644 src/tests/dlopen-tests.c
diff --git a/Makefile.am b/Makefile.am index 05da9a14d82d4d0c11ec2eb888e6af46c6e48ff7..6aad7530120e783c9a1c937110b22b49cf6139eb 100644 --- a/Makefile.am +++ b/Makefile.am @@ -114,6 +114,7 @@ endif
if HAVE_CHECK non_interactive_check_based_tests = \
- dlopen-tests \
Mixed tab and space
Fixed.
sysdb-tests \ strtonum-tests \ resolv-tests \index 0000000000000000000000000000000000000000..4067162eb63feb0ebdf21b250f83bdfcb4654182 --- /dev/null +++ b/src/tests/dlopen-tests.c @@ -0,0 +1,112 @@ +/*
- SSSD
- debug-tests.c
- Authors:
Pavel Březina <pbrezina@redhat.com>You should credit yourself :)
Indeed
- Copyright (C) 2011 Red Hat
And changed year too ^^
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 3 of the License, or
- (at your option) any later version.
- This program 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 General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+#define _GNU_SOURCE +#include <stdbool.h> +#include <dlfcn.h> +#include <stdio.h> +#include <stdlib.h> +#include <limits.h> +#include <check.h> +#include "tests/common.h"
+#define LIBPFX DLOPEN_TESTS_BUILDDIR"/.libs/"
We talked about this hardcoded directory on IRC and I can't see another way to autodetect this either.
So far no good way indeed, the joys of libtool ...
+const char *sonames[] = {
- "libipa_hbac.so",
- "libsss_ad.so",
- "libsss_child.so",
- "libsss_debug.so",
- "libsss_ipa.so",
- "libsss_krb5.so",
- "libsss_ldap.so",
- "libsss_proxy.so",
- "libsss_sudo.so",
- "memberof.so",
- "pyhbac.so",
- "pysss_nss_idmap.so",
- "sssd_krb5_locator_plugin.so",
- "libnss_sss.so",
- "libsss_autofs.so",
- "libsss_crypt.so",
- "libsss_idmap.so",
- "libsss_nss_idmap.so",
- "libsss_simple.so",
- "libsss_util.so",
- "pam_sss.so",
- "sssd_pac_plugin.so",
- NULL
+};
I wonder if there's any way of autogenerating this maybe from LTLIBRARIES? But definitely not something worth fixing in this patch instance.
Not sure, if you look at the list carefully you'll see 2 .so objects are missing (sss_krb5_common.so and sss_ldap_common.so) because they would fail the test on their own and they are already implicitly tested by provider modules which have these libraries as dependencies (see output of ldd .libs/libss_ldap.so for example).
I actually initially wanted to restrict to only the providers librraries given all others are built with dependency checking, but given it cost nothing to test them all and I used a sed command to go from ls -> list and it all worked I left most of them in there.
+START_TEST(test_dlopen_base) +{
- void *handle;
- char *soname;
- int ret, i;
- for (i = 0; sonames[i] != NULL; i++) {
ret = asprintf(&soname, LIBPFX"%s", sonames[i]);fail_unless(ret != -1, "Failed to construct soname");handle = dlopen(soname, RTLD_NOW);fail_unless(handle != NULL,"dlopen() failed for %s: [%s]", soname, dlerror());dlclose(handle);free(soname);- }
+} +END_TEST
+Suite *dlopen_suite(void) +{
- Suite *s = suite_create("dlopen");
- TCase *tc_dlopen = tcase_create("dlopen");
- tcase_add_test(tc_dlopen, test_dlopen_base);
- tcase_set_timeout(tc_dlopen, 10);
- suite_add_tcase(s, tc_dlopen);
- return s;
+}
+int main(int argc, const char *argv[]) +{
- int number_failed;
- tests_set_cwd();
I think this test is a special case where we actually should not call tests_set_cwd(). Calling it breaks the parallel build for me.
Removed.
- Suite *s = dlopen_suite();
- SRunner *sr = srunner_create(s);
- srunner_run_all(sr, CK_NORMAL);
- number_failed = srunner_ntests_failed(sr);
- srunner_free(sr);
- if (number_failed == 0)
return EXIT_SUCCESS;- return EXIT_FAILURE;
+}
1.8.3.1
Otherwise looks good to me!
Ok, fixed patch attached.
Simo.
-- Simo Sorce * Red Hat, Inc * New York
From 62545b25aa24c8a1eb109aadbf1a3973c7b7d01b Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Thu, 5 Sep 2013 11:52:08 -0400 Subject: [PATCH] tests: Add dlopen test to make sure modules works
This tests dlopens and resolves all symbols to make sure there are no missing symbols in our provider modules.
Makefile.am | 23 ++++++++++ src/tests/dlopen-tests.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 133 insertions(+) create mode 100644 src/tests/dlopen-tests.c
diff --git a/Makefile.am b/Makefile.am index 05da9a14d82d4d0c11ec2eb888e6af46c6e48ff7..060cf15c1e347eab75a5d6e28c8df40672e7db94 100644 --- a/Makefile.am +++ b/Makefile.am @@ -114,6 +114,7 @@ endif
if HAVE_CHECK non_interactive_check_based_tests = \
dlopen-tests \ sysdb-tests \ strtonum-tests \ resolv-tests \@@ -886,6 +887,28 @@ if HAVE_CHECK libsss_test_common_la_SOURCES += \ src/tests/common_check.c
+dlopen_tests_DEPENDENCIES = \
- $(ldblib_LTLIBRARIES)
+dlopen_tests_SOURCES = \
- src/tests/dlopen-tests.c \
- $(sssd_be_SOURCES)
+dlopen_tests_CFLAGS = \
- $(AM_CFLAGS) \
- $(CHECK_CFLAGS) \
- -DDLOPEN_TESTS_BUILDDIR="$(builddir)" \
- -DUNIT_TESTING
+dlopen_tests_LDADD = \
- -ldl \
- $(SSSD_LIBS) \
- $(CARES_LIBS) \
- $(CHECK_LIBS) \
- $(PAM_LIBS) \
- $(SSSD_INTERNAL_LTLIBS) \
- libsss_test_common.la
+dlopen_tests_LDFLAGS = \
- -Wl,--version-script,$(srcdir)/src/providers/sssd_be.exports \
- -export-dynamic
sysdb_tests_DEPENDENCIES = \ $(ldblib_LTLIBRARIES) sysdb_tests_SOURCES = \ diff --git a/src/tests/dlopen-tests.c b/src/tests/dlopen-tests.c new file mode 100644 index 0000000000000000000000000000000000000000..c2ac7a10364a53cb00375ebd6a573d7b4a6920cb --- /dev/null +++ b/src/tests/dlopen-tests.c @@ -0,0 +1,110 @@ +/*
- SSSD
- debug-tests.c
- Authors:
Simo Sorce <simo@redhat.com>- Copyright (C) 2013 Red Hat
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 3 of the License, or
- (at your option) any later version.
- This program 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 General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+#define _GNU_SOURCE +#include <stdbool.h> +#include <dlfcn.h> +#include <stdio.h> +#include <stdlib.h> +#include <limits.h> +#include <check.h> +#include "tests/common.h"
+#define LIBPFX DLOPEN_TESTS_BUILDDIR"/.libs/"
+const char *sonames[] = {
- "libipa_hbac.so",
- "libsss_ad.so",
- "libsss_child.so",
- "libsss_debug.so",
- "libsss_ipa.so",
- "libsss_krb5.so",
- "libsss_ldap.so",
- "libsss_proxy.so",
- "libsss_sudo.so",
- "memberof.so",
- "pyhbac.so",
- "pysss_nss_idmap.so",
- "sssd_krb5_locator_plugin.so",
- "libnss_sss.so",
- "libsss_autofs.so",
- "libsss_crypt.so",
- "libsss_idmap.so",
- "libsss_nss_idmap.so",
- "libsss_simple.so",
- "libsss_util.so",
- "pam_sss.so",
- "sssd_pac_plugin.so",
- NULL
+};
+START_TEST(test_dlopen_base) +{
- void *handle;
- char *soname;
- int ret, i;
- for (i = 0; sonames[i] != NULL; i++) {
ret = asprintf(&soname, LIBPFX"%s", sonames[i]);fail_unless(ret != -1, "Failed to construct soname");handle = dlopen(soname, RTLD_NOW);fail_unless(handle != NULL,"dlopen() failed for %s: [%s]", soname, dlerror());dlclose(handle);free(soname);- }
+} +END_TEST
+Suite *dlopen_suite(void) +{
- Suite *s = suite_create("dlopen");
- TCase *tc_dlopen = tcase_create("dlopen");
- tcase_add_test(tc_dlopen, test_dlopen_base);
- tcase_set_timeout(tc_dlopen, 10);
- suite_add_tcase(s, tc_dlopen);
- return s;
+}
+int main(int argc, const char *argv[]) +{
- int number_failed;
- Suite *s = dlopen_suite();
- SRunner *sr = srunner_create(s);
- srunner_run_all(sr, CK_NORMAL);
- number_failed = srunner_ntests_failed(sr);
- srunner_free(sr);
- if (number_failed == 0)
return EXIT_SUCCESS;- return EXIT_FAILURE;
+}
1.8.3.1
This version works well.
I tried to remove some files from library: diff ------------------ @@ -1366,7 +1366,6 @@ endif
nsslib_LTLIBRARIES = libnss_sss.la libnss_sss_la_SOURCES = \ - src/sss_client/common.c \ src/sss_client/nss_passwd.c \ src/sss_client/nss_group.c \ src/sss_client/nss_netgroup.c \
And test failed. Running suite(s): dlopen 0%: Checks: 1, Failures: 1, Errors: 0 ../sssd/src/tests/dlopen-tests.c:74:F:dlopen:test_dlopen_base:0: dlopen() failed for ./.libs/libnss_sss.so: [./.libs/libnss_sss.so: undefined symbol: sss_nss_lock]
Then I tried to remove another file.
@@ -1374,8 +1374,6 @@ libnss_sss_la_SOURCES = \ src/sss_client/sss_cli.h \ src/sss_client/nss_compat.h \ src/sss_client/nss_mc_common.c \ - src/util/io.c \ src/util/murmurhash3.c \ src/sss_client/nss_mc_passwd.c \ src/sss_client/nss_mc_group.c \ src/sss_client/nss_mc.h
And test did not fail. I don't know why, because this missing file caused crash in past. https://fedorahosted.org/sssd/ticket/1838
LS
On Fri, 2013-09-06 at 15:27 +0200, Lukas Slebodnik wrote:
This version works well.
I tried to remove some files from library: diff
@@ -1366,7 +1366,6 @@ endif
nsslib_LTLIBRARIES = libnss_sss.la libnss_sss_la_SOURCES = \
- src/sss_client/common.c \ src/sss_client/nss_passwd.c \ src/sss_client/nss_group.c \ src/sss_client/nss_netgroup.c \
And test failed. Running suite(s): dlopen 0%: Checks: 1, Failures: 1, Errors: 0 ../sssd/src/tests/dlopen-tests.c:74:F:dlopen:test_dlopen_base:0: dlopen() failed for ./.libs/libnss_sss.so: [./.libs/libnss_sss.so: undefined symbol: sss_nss_lock]
Then I tried to remove another file.
@@ -1374,8 +1374,6 @@ libnss_sss_la_SOURCES = \ src/sss_client/sss_cli.h \ src/sss_client/nss_compat.h \ src/sss_client/nss_mc_common.c \
- src/util/io.c \ src/util/murmurhash3.c \ src/sss_client/nss_mc_passwd.c \ src/sss_client/nss_mc_group.c \ src/sss_client/nss_mc.h
And test did not fail. I don't know why, because this missing file caused crash in past. https://fedorahosted.org/sssd/ticket/1838
Ok the reason why the second issue was not caught is that in the original patch I had to link in a bunch of binaries for the provider module case, and those contained the symbol you tried to test.
I have a new patch attached, that spins off the symbols needed by the provider modules in their own .so library, so that we load the library only when testing the provider modules but not for other libraries.
Now libnss_sss.so fails the dlopen-tests when the io.c file is omitted.
2 points on the new code: * I had to add libdlopen_test_providers to lib_LTLIBRARY instead of noinst_LTLIBRARY or check_LTLIBRARY because with these last 2 targets libtool never builds .so files :-/ Therefore I had to add an intsall-exec-hook to remove libdlopen_test_providers.so file at install time, as we use it only for testing.
* I am not sure why libsss_child.so, libsss_crypt.so, libsss_util.so need some libraries to be explicitly loaded instead of having them encoded as their dependencies, might be a bug in our makefile. If it is I think we should still commit this patch as is and then open a bug to resolve the issue and adjust the test in the same commit the resolve the issue.
Simo.
On (06/09/13 17:13), Simo Sorce wrote:
On Fri, 2013-09-06 at 15:27 +0200, Lukas Slebodnik wrote:
This version works well.
I tried to remove some files from library: diff
@@ -1366,7 +1366,6 @@ endif
nsslib_LTLIBRARIES = libnss_sss.la libnss_sss_la_SOURCES = \
- src/sss_client/common.c \ src/sss_client/nss_passwd.c \ src/sss_client/nss_group.c \ src/sss_client/nss_netgroup.c \
And test failed. Running suite(s): dlopen 0%: Checks: 1, Failures: 1, Errors: 0 ../sssd/src/tests/dlopen-tests.c:74:F:dlopen:test_dlopen_base:0: dlopen() failed for ./.libs/libnss_sss.so: [./.libs/libnss_sss.so: undefined symbol: sss_nss_lock]
Then I tried to remove another file.
@@ -1374,8 +1374,6 @@ libnss_sss_la_SOURCES = \ src/sss_client/sss_cli.h \ src/sss_client/nss_compat.h \ src/sss_client/nss_mc_common.c \
- src/util/io.c \ src/util/murmurhash3.c \ src/sss_client/nss_mc_passwd.c \ src/sss_client/nss_mc_group.c \ src/sss_client/nss_mc.h
And test did not fail. I don't know why, because this missing file caused crash in past. https://fedorahosted.org/sssd/ticket/1838
Ok the reason why the second issue was not caught is that in the original patch I had to link in a bunch of binaries for the provider module case, and those contained the symbol you tried to test.
I have a new patch attached, that spins off the symbols needed by the provider modules in their own .so library, so that we load the library only when testing the provider modules but not for other libraries.
Now libnss_sss.so fails the dlopen-tests when the io.c file is omitted.
2 points on the new code:
- I had to add libdlopen_test_providers to lib_LTLIBRARY instead of
noinst_LTLIBRARY or check_LTLIBRARY because with these last 2 targets libtool never builds .so files :-/ Therefore I had to add an intsall-exec-hook to remove libdlopen_test_providers.so file at install time, as we use it only for testing.
I found a way how to build dynamic library with check_LTLIBRARY. comments inline
- I am not sure why libsss_child.so, libsss_crypt.so, libsss_util.so
need some libraries to be explicitly loaded instead of having them encoded as their dependencies, might be a bug in our makefile. If it is I think we should still commit this patch as is and then open a bug to resolve the issue and adjust the test in the same commit the resolve the issue.
It was a problem to build sssd on ubuntu with dynamic libraries. https://lists.fedorahosted.org/pipermail/sssd-devel/2013-May/015053.html Problem was with link_all_deplibs="no" on debian/ubuntu. Here is an explanation: https://lists.fedorahosted.org/pipermail/sssd-devel/2013-May/015053.html
Simo.
-- Simo Sorce * Red Hat, Inc * New York
From be6d1c97c36dd9345358e270ffb00dc89640e288 Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Thu, 5 Sep 2013 11:52:08 -0400 Subject: [PATCH] tests: Add dlopen test to make sure modules works
This tests dlopens and resolves all symbols to make sure there are no missing symbols in our provider modules.
Makefile.am | 35 +++++++++++ src/tests/dlopen-tests.c | 147 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 182 insertions(+) create mode 100644 src/tests/dlopen-tests.c
diff --git a/Makefile.am b/Makefile.am index b913a12b895d68f1f3e23c185e493e576641d0e2..4d32b174264fb5c61fddd18e13263fe20e045b1f 100644 --- a/Makefile.am +++ b/Makefile.am @@ -114,6 +114,7 @@ endif
if HAVE_CHECK non_interactive_check_based_tests = \
dlopen-tests \ sysdb-tests \ strtonum-tests \ resolv-tests \@@ -886,6 +887,39 @@ if HAVE_CHECK libsss_test_common_la_SOURCES += \ src/tests/common_check.c
+lib_LTLIBRARIES += \
- libdlopen_test_providers.la
Please use "check_LTLIBRARIES = "
+libdlopen_test_providers_la_DEPENDENCIES = \
- $(ldblib_LTLIBRARIES) \
- $(SSSD_INTERNAL_LTLIBS)
^^^^^ These lines can be removed with check_LTLIBRARIES =
+libdlopen_test_providers_la_SOURCES = \
- $(sssd_be_SOURCES)
+libdlopen_test_providers_la_CFLAGS = \
- $(AM_CFLAGS) \
- $(CHECK_CFLAGS) \
- -DUNIT_TESTING
+libdlopen_test_providers_la_LIBADD = \
- $(PAM_LIBS) \
- $(SSSD_LIBS) \
- $(CARES_LIBS) \
- $(SSSD_INTERNAL_LTLIBS)
+libdlopen_test_providers_la_LDFLAGS = \
- -module \
- -avoid-version \
- -Wl,--version-script,$(srcdir)/src/providers/sssd_be.exports \
add "-rpath $(abs_builddir) " libdlopen_test_providers will be dynamic library with this line.
- -export-dynamic
+dlopen_tests_SOURCES = \
- src/tests/dlopen-tests.c
+dlopen_tests_CFLAGS = \
- $(AM_CFLAGS) \
- $(CHECK_CFLAGS) \
- -DDLOPEN_TESTS_BUILDDIR="$(builddir)"
+dlopen_tests_LDADD = \
- -ldl \
- $(CHECK_LIBS)
sysdb_tests_DEPENDENCIES = \ $(ldblib_LTLIBRARIES) sysdb_tests_SOURCES = \ @@ -1911,6 +1945,7 @@ if BUILD_PYTHON_BINDINGS endif
install-exec-hook: installsssddirs
- rm -f $(DESTDIR)$(libdir)/libdlopen_test_providers.*
^^^^^^^^ This like can be removed.
if BUILD_PYTHON_BINDINGS if [ "$(DESTDIR)" = "" ]; then \ cd $(builddir)/src/config; $(PYTHON) setup.py build --build-base $(abs_builddir)/src/config install $(DISTSETUPOPTS) --prefix=$(PYTHON_PREFIX) --record=$(abs_builddir)/src/config/.files; \ diff --git a/src/tests/dlopen-tests.c b/src/tests/dlopen-tests.c new file mode 100644 index 0000000000000000000000000000000000000000..5d4e77df74cb80dcdebd7b4b34a594d1f6a66de7 --- /dev/null +++ b/src/tests/dlopen-tests.c @@ -0,0 +1,147 @@ +/*
- SSSD
- debug-tests.c
- Authors:
Simo Sorce <simo@redhat.com>- Copyright (C) 2013 Red Hat
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 3 of the License, or
- (at your option) any later version.
- This program 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 General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+#define _GNU_SOURCE +#include <stdbool.h> +#include <dlfcn.h> +#include <stdio.h> +#include <stdlib.h> +#include <limits.h> +#include <check.h> +#include "tests/common.h"
+#define LIBPFX DLOPEN_TESTS_BUILDDIR"/.libs/"
+struct so {
- const char *name;
- const char *libs[6];
+} so[] = {
- { "libsss_debug.so", { LIBPFX"libsss_debug.so", NULL } },
- { "libsss_sudo.so", { LIBPFX"libsss_sudo.so", NULL } },
- { "libipa_hbac.so", { LIBPFX"libipa_hbac.so", NULL } },
- { "libsss_autofs.so", { LIBPFX"libsss_autofs.so", NULL } },
- { "libsss_idmap.so", { LIBPFX"libsss_idmap.so", NULL } },
- { "libsss_nss_idmap.so", { LIBPFX"libsss_nss_idmap.so", NULL } },
- { "libnss_sss.so", { LIBPFX"libnss_sss.so", NULL } },
- { "pam_sss.so", { LIBPFX"pam_sss.so", NULL } },
- { "sssd_pac_plugin.so", { LIBPFX"sssd_pac_plugin.so", NULL } },
- { "sssd_krb5_locator_plugin.so", { LIBPFX"sssd_krb5_locator_plugin.so",
NULL } },- { "memberof.so", { LIBPFX"memberof.so", NULL } },
- { "pyhbac.so", { LIBPFX"pyhbac.so", NULL } },
- { "pysss_nss_idmap.so", { LIBPFX"pysss_nss_idmap.so", NULL } },
{ "pysss_murmur.so", { LIBPFX"pysss_murmur.so", NULL } }, { "pysss.so", { LIBPFX"pysss.so", NULL } }, I missed these libraries.
Thank you very much for this test.
LS
On Sat, 2013-09-07 at 11:42 +0200, Lukas Slebodnik wrote:
+libdlopen_test_providers_la_LDFLAGS = \
- -module \
- -avoid-version \
- -Wl,--version-script,$(srcdir)/src/providers/sssd_be.exports \
add "-rpath $(abs_builddir) " libdlopen_test_providers will be dynamic library with this line.
I had already tried this trick and it did not work, but the exact line I used was slightly different.
I'll retry and if it works I'll send an updated patch I do preferer check_LTLIBRARY as well, rather than the current hack.
Simo.
On Sat, 2013-09-07 at 12:39 -0400, Simo Sorce wrote:
On Sat, 2013-09-07 at 11:42 +0200, Lukas Slebodnik wrote:
+libdlopen_test_providers_la_LDFLAGS = \
- -module \
- -avoid-version \
- -Wl,--version-script,$(srcdir)/src/providers/sssd_be.exports \
add "-rpath $(abs_builddir) " libdlopen_test_providers will be dynamic library with this line.
I had already tried this trick and it did not work, but the exact line I used was slightly different.
Ah it did work with this syntax, I had tried with -Wl,-rpath previously. Thanks a lot.
I'll retry and if it works I'll send an updated patch I do preferer check_LTLIBRARY as well, rather than the current hack.
so now check_LTLIBRARY is used instead, moreover I added guards around libraries that we compiled only conditionally, I had to add a couple of statements in m4 files to make stuff appear in config.in. Also I added the 2 missing python bindings.
This should be final, it all works here.
Simo.
On (07/09/13 13:17), Simo Sorce wrote:
On Sat, 2013-09-07 at 12:39 -0400, Simo Sorce wrote:
On Sat, 2013-09-07 at 11:42 +0200, Lukas Slebodnik wrote:
+libdlopen_test_providers_la_LDFLAGS = \
- -module \
- -avoid-version \
- -Wl,--version-script,$(srcdir)/src/providers/sssd_be.exports \
add "-rpath $(abs_builddir) " libdlopen_test_providers will be dynamic library with this line.
I had already tried this trick and it did not work, but the exact line I used was slightly different.
Ah it did work with this syntax, I had tried with -Wl,-rpath previously. Thanks a lot.
I'll retry and if it works I'll send an updated patch I do preferer check_LTLIBRARY as well, rather than the current hack.
so now check_LTLIBRARY is used instead, moreover I added guards around libraries that we compiled only conditionally, I had to add a couple of statements in m4 files to make stuff appear in config.in. Also I added the 2 missing python bindings.
This should be final, it all works here.
Simo.
-- Simo Sorce * Red Hat, Inc * New York
From a957e4642ec49a36831270c8007bcc8d7818db90 Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Thu, 5 Sep 2013 11:52:08 -0400 Subject: [PATCH] tests: Add dlopen test to make sure modules works
This tests dlopens and resolves all symbols to make sure there are no missing symbols in our provider modules.
Makefile.am | 31 ++++++++++ configure.ac | 1 + src/external/krb5.m4 | 3 +- src/tests/dlopen-tests.c | 155 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 189 insertions(+), 1 deletion(-) create mode 100644 src/tests/dlopen-tests.c
diff --git a/Makefile.am b/Makefile.am index b913a12b895d68f1f3e23c185e493e576641d0e2..bddb403b373ecee21f6ad41f36e69338b4cbb5e2 100644 --- a/Makefile.am +++ b/Makefile.am @@ -114,6 +114,7 @@ endif
if HAVE_CHECK non_interactive_check_based_tests = \
dlopen-tests \ sysdb-tests \ strtonum-tests \ resolv-tests \@@ -886,6 +887,36 @@ if HAVE_CHECK libsss_test_common_la_SOURCES += \ src/tests/common_check.c
+check_LTLIBRARIES = \
- libdlopen_test_providers.la
+libdlopen_test_providers_la_SOURCES = \
- $(sssd_be_SOURCES)
+libdlopen_test_providers_la_CFLAGS = \
- $(AM_CFLAGS) \
- $(CHECK_CFLAGS) \
- -DUNIT_TESTING
+libdlopen_test_providers_la_LIBADD = \
- $(PAM_LIBS) \
- $(SSSD_LIBS) \
- $(CARES_LIBS) \
- $(SSSD_INTERNAL_LTLIBS)
+libdlopen_test_providers_la_LDFLAGS = \
- -module \
- -avoid-version \
- -Wl,--version-script,$(srcdir)/src/providers/sssd_be.exports \
- -rpath $(abs_top_builddir) \
- -export-dynamic
+dlopen_tests_SOURCES = \
- src/tests/dlopen-tests.c
+dlopen_tests_CFLAGS = \
- $(AM_CFLAGS) \
- $(CHECK_CFLAGS)
+dlopen_tests_LDADD = \
- -ldl \
- $(CHECK_LIBS)
sysdb_tests_DEPENDENCIES = \ $(ldblib_LTLIBRARIES) sysdb_tests_SOURCES = \ diff --git a/configure.ac b/configure.ac index 511e8d6c8afbfb0cbf5aca076b460aa4c881e252..586fe0bf9dbda5c65175d491e0044f8f09650b39 100644 --- a/configure.ac +++ b/configure.ac @@ -227,6 +227,7 @@ if test x$HAVE_PYTHON_BINDINGS != x; then AM_PATH_PYTHON([2.4]) AM_CHECK_PYTHON_HEADERS([], AC_MSG_ERROR([Could not find python headers]))
- AC_DEFINE_UNQUOTED(HAVE_PYTHON_BINDINGS, 1, [Build with python bindings])
I would prefer to have this line in the file conf_macros.m4 directly after the line src/conf_macros.m4:333: AM_CONDITIONAL([BUILD_PYTHON_BINDINGS],
In this file, there is a lot of similar patterns
AM_PYTHON_CONFIG AM_CHECK_PYTHON_COMPATfi diff --git a/src/external/krb5.m4 b/src/external/krb5.m4 index bdbd71ba1f395eb4f78047f75841729e0289ae81..a0da44a887329fbb2173a5849497d0b2cf1729a7 100644 --- a/src/external/krb5.m4 +++ b/src/external/krb5.m4 @@ -90,4 +90,5 @@ AC_CHECK_HEADER([krb5/locate_plugin.h], ]) AM_CONDITIONAL([BUILD_KRB5_LOCATOR_PLUGIN], [test x$have_locate_plugin = xyes -a x$build_locator = xyes])
+AM_COND_IF([BUILD_KRB5_LOCATOR_PLUGIN],
[AC_DEFINE_UNQUOTED(HAVE_KRB5_LOCATOR_PLUGIN, 1, [Build with krb5 locator plugin])])diff --git a/src/tests/dlopen-tests.c b/src/tests/dlopen-tests.c new file mode 100644 index 0000000000000000000000000000000000000000..608b6106d6584a528a819cc2abdf1e21469d8d38 --- /dev/null +++ b/src/tests/dlopen-tests.c @@ -0,0 +1,155 @@ +/*
- SSSD
- debug-tests.c
- Authors:
Simo Sorce <simo@redhat.com>- Copyright (C) 2013 Red Hat
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 3 of the License, or
- (at your option) any later version.
- This program 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 General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+#define _GNU_SOURCE +#include <stdbool.h> +#include <dlfcn.h> +#include <stdio.h> +#include <stdlib.h> +#include <limits.h> +#include <check.h> +#include "tests/common.h"
+#define LIBPFX ABS_BUILD_DIR"/.libs/"
+struct so {
- const char *name;
- const char *libs[6];
+} so[] = {
- { "libsss_debug.so", { LIBPFX"libsss_debug.so", NULL } },
- { "libsss_sudo.so", { LIBPFX"libsss_sudo.so", NULL } },
add check for BUILD_SUDO
- { "libipa_hbac.so", { LIBPFX"libipa_hbac.so", NULL } },
- { "libsss_autofs.so", { LIBPFX"libsss_autofs.so", NULL } },
add check for BUILD_AUTOFS
- { "libsss_idmap.so", { LIBPFX"libsss_idmap.so", NULL } },
- { "libsss_nss_idmap.so", { LIBPFX"libsss_nss_idmap.so", NULL } },
- { "libnss_sss.so", { LIBPFX"libnss_sss.so", NULL } },
- { "pam_sss.so", { LIBPFX"pam_sss.so", NULL } },
+#ifdef HAVE_KRB5_LOCATOR_PLUGIN
- { "sssd_krb5_locator_plugin.so", { LIBPFX"sssd_krb5_locator_plugin.so",
NULL } },+#endif +#ifdef HAVE_PAC_RESPONDER
- { "sssd_pac_plugin.so", { LIBPFX"sssd_pac_plugin.so", NULL } },
+#endif
- { "memberof.so", { LIBPFX"memberof.so", NULL } },
- { "libsss_child.so", { "libtevent.so",
LIBPFX"libsss_debug.so",LIBPFX"libsss_crypt.so",LIBPFX"libsss_util.so",LIBPFX"libsss_child.so", NULL } },- { "libsss_crypt.so", { "libtalloc.so",
LIBPFX"libsss_debug.so",LIBPFX"libsss_crypt.so", NULL } },- { "libsss_util.so", { "libtalloc.so",
LIBPFX"libsss_debug.so",LIBPFX"libsss_crypt.so",LIBPFX"libsss_util.so", NULL } },- { "libsss_simple.so", { LIBPFX"libdlopen_test_providers.so",
LIBPFX"libsss_simple.so", NULL } },- { "libsss_ad.so", { LIBPFX"libdlopen_test_providers.so",
LIBPFX"libsss_ad.so", NULL } },- { "libsss_ipa.so", { LIBPFX"libdlopen_test_providers.so",
LIBPFX"libsss_ipa.so", NULL } },- { "libsss_krb5.so", { LIBPFX"libdlopen_test_providers.so",
LIBPFX"libsss_krb5.so", NULL } },- { "libsss_ldap.so", { LIBPFX"libdlopen_test_providers.so",
LIBPFX"libsss_ldap.so", NULL } },- { "libsss_proxy.so", { LIBPFX"libdlopen_test_providers.so",
LIBPFX"libsss_proxy.so", NULL } },+#ifdef BUILD_PYTHON_BINDINGS
You defined macro HAVE_PYTHON_BINDINGS, but BUILD_PYTHON_BINDINGS is used here. This is a dead code.
- { "pyhbac.so", { LIBPFX"pyhbac.so", NULL } },
- { "pysss.so", { LIBPFX"pysss.so", NULL } },
- { "pysss_murmur.so", { LIBPFX"pysss_murmur.so", NULL } },
- { "pysss_nss_idmap.so", { LIBPFX"pysss_nss_idmap.so", NULL } },
+#endif
btw, BUILD_AUTOFS is used elsewhere in code. src/tests/sysdb-tests.c:4707:#ifdef BUILD_AUTOFS src/tests/sysdb-tests.c:4913:#endif /* BUILD_AUTOFS */ src/tests/sysdb-tests.c:5224:#ifdef BUILD_AUTOFS
So we can use either BUILD_PYTHON_BINDINGS, BUILD_KRB5_LOCATOR_PLUGIN or we should file a ticket to be consistent.
LS
On Mon, Sep 09, 2013 at 09:52:34AM +0200, Lukas Slebodnik wrote:
On (07/09/13 13:17), Simo Sorce wrote:
On Sat, 2013-09-07 at 12:39 -0400, Simo Sorce wrote:
On Sat, 2013-09-07 at 11:42 +0200, Lukas Slebodnik wrote:
+libdlopen_test_providers_la_LDFLAGS = \
- -module \
- -avoid-version \
- -Wl,--version-script,$(srcdir)/src/providers/sssd_be.exports \
add "-rpath $(abs_builddir) " libdlopen_test_providers will be dynamic library with this line.
I had already tried this trick and it did not work, but the exact line I used was slightly different.
Ah it did work with this syntax, I had tried with -Wl,-rpath previously. Thanks a lot.
I'll retry and if it works I'll send an updated patch I do preferer check_LTLIBRARY as well, rather than the current hack.
so now check_LTLIBRARY is used instead, moreover I added guards around libraries that we compiled only conditionally, I had to add a couple of statements in m4 files to make stuff appear in config.in. Also I added the 2 missing python bindings.
This should be final, it all works here.
Simo.
-- Simo Sorce * Red Hat, Inc * New York
From a957e4642ec49a36831270c8007bcc8d7818db90 Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Thu, 5 Sep 2013 11:52:08 -0400 Subject: [PATCH] tests: Add dlopen test to make sure modules works
This tests dlopens and resolves all symbols to make sure there are no missing symbols in our provider modules.
Makefile.am | 31 ++++++++++ configure.ac | 1 + src/external/krb5.m4 | 3 +- src/tests/dlopen-tests.c | 155 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 189 insertions(+), 1 deletion(-) create mode 100644 src/tests/dlopen-tests.c
diff --git a/Makefile.am b/Makefile.am index b913a12b895d68f1f3e23c185e493e576641d0e2..bddb403b373ecee21f6ad41f36e69338b4cbb5e2 100644 --- a/Makefile.am +++ b/Makefile.am @@ -114,6 +114,7 @@ endif
if HAVE_CHECK non_interactive_check_based_tests = \
dlopen-tests \ sysdb-tests \ strtonum-tests \ resolv-tests \@@ -886,6 +887,36 @@ if HAVE_CHECK libsss_test_common_la_SOURCES += \ src/tests/common_check.c
+check_LTLIBRARIES = \
- libdlopen_test_providers.la
+libdlopen_test_providers_la_SOURCES = \
- $(sssd_be_SOURCES)
+libdlopen_test_providers_la_CFLAGS = \
- $(AM_CFLAGS) \
- $(CHECK_CFLAGS) \
- -DUNIT_TESTING
+libdlopen_test_providers_la_LIBADD = \
- $(PAM_LIBS) \
- $(SSSD_LIBS) \
- $(CARES_LIBS) \
- $(SSSD_INTERNAL_LTLIBS)
+libdlopen_test_providers_la_LDFLAGS = \
- -module \
- -avoid-version \
- -Wl,--version-script,$(srcdir)/src/providers/sssd_be.exports \
- -rpath $(abs_top_builddir) \
- -export-dynamic
+dlopen_tests_SOURCES = \
- src/tests/dlopen-tests.c
+dlopen_tests_CFLAGS = \
- $(AM_CFLAGS) \
- $(CHECK_CFLAGS)
+dlopen_tests_LDADD = \
- -ldl \
- $(CHECK_LIBS)
sysdb_tests_DEPENDENCIES = \ $(ldblib_LTLIBRARIES) sysdb_tests_SOURCES = \ diff --git a/configure.ac b/configure.ac index 511e8d6c8afbfb0cbf5aca076b460aa4c881e252..586fe0bf9dbda5c65175d491e0044f8f09650b39 100644 --- a/configure.ac +++ b/configure.ac @@ -227,6 +227,7 @@ if test x$HAVE_PYTHON_BINDINGS != x; then AM_PATH_PYTHON([2.4]) AM_CHECK_PYTHON_HEADERS([], AC_MSG_ERROR([Could not find python headers]))
- AC_DEFINE_UNQUOTED(HAVE_PYTHON_BINDINGS, 1, [Build with python bindings])
I would prefer to have this line in the file conf_macros.m4 directly after the line src/conf_macros.m4:333: AM_CONDITIONAL([BUILD_PYTHON_BINDINGS],
In this file, there is a lot of similar patterns
AM_PYTHON_CONFIG AM_CHECK_PYTHON_COMPATfi diff --git a/src/external/krb5.m4 b/src/external/krb5.m4 index bdbd71ba1f395eb4f78047f75841729e0289ae81..a0da44a887329fbb2173a5849497d0b2cf1729a7 100644 --- a/src/external/krb5.m4 +++ b/src/external/krb5.m4 @@ -90,4 +90,5 @@ AC_CHECK_HEADER([krb5/locate_plugin.h], ]) AM_CONDITIONAL([BUILD_KRB5_LOCATOR_PLUGIN], [test x$have_locate_plugin = xyes -a x$build_locator = xyes])
+AM_COND_IF([BUILD_KRB5_LOCATOR_PLUGIN],
[AC_DEFINE_UNQUOTED(HAVE_KRB5_LOCATOR_PLUGIN, 1, [Build with krb5 locator plugin])])diff --git a/src/tests/dlopen-tests.c b/src/tests/dlopen-tests.c new file mode 100644 index 0000000000000000000000000000000000000000..608b6106d6584a528a819cc2abdf1e21469d8d38 --- /dev/null +++ b/src/tests/dlopen-tests.c @@ -0,0 +1,155 @@ +/*
- SSSD
- debug-tests.c
- Authors:
Simo Sorce <simo@redhat.com>- Copyright (C) 2013 Red Hat
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 3 of the License, or
- (at your option) any later version.
- This program 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 General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+#define _GNU_SOURCE +#include <stdbool.h> +#include <dlfcn.h> +#include <stdio.h> +#include <stdlib.h> +#include <limits.h> +#include <check.h> +#include "tests/common.h"
+#define LIBPFX ABS_BUILD_DIR"/.libs/"
+struct so {
- const char *name;
- const char *libs[6];
+} so[] = {
- { "libsss_debug.so", { LIBPFX"libsss_debug.so", NULL } },
- { "libsss_sudo.so", { LIBPFX"libsss_sudo.so", NULL } },
add check for BUILD_SUDO
- { "libipa_hbac.so", { LIBPFX"libipa_hbac.so", NULL } },
- { "libsss_autofs.so", { LIBPFX"libsss_autofs.so", NULL } },
add check for BUILD_AUTOFS
- { "libsss_idmap.so", { LIBPFX"libsss_idmap.so", NULL } },
- { "libsss_nss_idmap.so", { LIBPFX"libsss_nss_idmap.so", NULL } },
- { "libnss_sss.so", { LIBPFX"libnss_sss.so", NULL } },
- { "pam_sss.so", { LIBPFX"pam_sss.so", NULL } },
+#ifdef HAVE_KRB5_LOCATOR_PLUGIN
- { "sssd_krb5_locator_plugin.so", { LIBPFX"sssd_krb5_locator_plugin.so",
NULL } },+#endif +#ifdef HAVE_PAC_RESPONDER
- { "sssd_pac_plugin.so", { LIBPFX"sssd_pac_plugin.so", NULL } },
+#endif
- { "memberof.so", { LIBPFX"memberof.so", NULL } },
- { "libsss_child.so", { "libtevent.so",
LIBPFX"libsss_debug.so",LIBPFX"libsss_crypt.so",LIBPFX"libsss_util.so",LIBPFX"libsss_child.so", NULL } },- { "libsss_crypt.so", { "libtalloc.so",
LIBPFX"libsss_debug.so",LIBPFX"libsss_crypt.so", NULL } },- { "libsss_util.so", { "libtalloc.so",
LIBPFX"libsss_debug.so",LIBPFX"libsss_crypt.so",LIBPFX"libsss_util.so", NULL } },- { "libsss_simple.so", { LIBPFX"libdlopen_test_providers.so",
LIBPFX"libsss_simple.so", NULL } },- { "libsss_ad.so", { LIBPFX"libdlopen_test_providers.so",
LIBPFX"libsss_ad.so", NULL } },- { "libsss_ipa.so", { LIBPFX"libdlopen_test_providers.so",
LIBPFX"libsss_ipa.so", NULL } },- { "libsss_krb5.so", { LIBPFX"libdlopen_test_providers.so",
LIBPFX"libsss_krb5.so", NULL } },- { "libsss_ldap.so", { LIBPFX"libdlopen_test_providers.so",
LIBPFX"libsss_ldap.so", NULL } },- { "libsss_proxy.so", { LIBPFX"libdlopen_test_providers.so",
LIBPFX"libsss_proxy.so", NULL } },+#ifdef BUILD_PYTHON_BINDINGS
You defined macro HAVE_PYTHON_BINDINGS, but BUILD_PYTHON_BINDINGS is used here. This is a dead code.
- { "pyhbac.so", { LIBPFX"pyhbac.so", NULL } },
- { "pysss.so", { LIBPFX"pysss.so", NULL } },
- { "pysss_murmur.so", { LIBPFX"pysss_murmur.so", NULL } },
- { "pysss_nss_idmap.so", { LIBPFX"pysss_nss_idmap.so", NULL } },
+#endif
btw, BUILD_AUTOFS is used elsewhere in code. src/tests/sysdb-tests.c:4707:#ifdef BUILD_AUTOFS src/tests/sysdb-tests.c:4913:#endif /* BUILD_AUTOFS */ src/tests/sysdb-tests.c:5224:#ifdef BUILD_AUTOFS
So we can use either BUILD_PYTHON_BINDINGS, BUILD_KRB5_LOCATOR_PLUGIN or we should file a ticket to be consistent.
LS
These are valid comments, but I wonder if we can remove the conditional altogether and always build the autofs responder? Then the distributions can choose not to package it. The downside would be some dead code in the LDAP provider.
The autofs code has no external dependencies, it just produces a shared library that can be dlopen()-ed.
On (09/09/13 09:52), Lukas Slebodnik wrote:
On (07/09/13 13:17), Simo Sorce wrote:
On Sat, 2013-09-07 at 12:39 -0400, Simo Sorce wrote:
On Sat, 2013-09-07 at 11:42 +0200, Lukas Slebodnik wrote:
+libdlopen_test_providers_la_LDFLAGS = \
- -module \
- -avoid-version \
- -Wl,--version-script,$(srcdir)/src/providers/sssd_be.exports \
add "-rpath $(abs_builddir) " libdlopen_test_providers will be dynamic library with this line.
I had already tried this trick and it did not work, but the exact line I used was slightly different.
Ah it did work with this syntax, I had tried with -Wl,-rpath previously. Thanks a lot.
I'll retry and if it works I'll send an updated patch I do preferer check_LTLIBRARY as well, rather than the current hack.
so now check_LTLIBRARY is used instead, moreover I added guards around libraries that we compiled only conditionally, I had to add a couple of statements in m4 files to make stuff appear in config.in. Also I added the 2 missing python bindings.
This should be final, it all works here.
Simo.
-- Simo Sorce * Red Hat, Inc * New York
From a957e4642ec49a36831270c8007bcc8d7818db90 Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Thu, 5 Sep 2013 11:52:08 -0400 Subject: [PATCH] tests: Add dlopen test to make sure modules works
This tests dlopens and resolves all symbols to make sure there are no missing symbols in our provider modules.
Makefile.am | 31 ++++++++++ configure.ac | 1 + src/external/krb5.m4 | 3 +- src/tests/dlopen-tests.c | 155 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 189 insertions(+), 1 deletion(-) create mode 100644 src/tests/dlopen-tests.c
diff --git a/Makefile.am b/Makefile.am index b913a12b895d68f1f3e23c185e493e576641d0e2..bddb403b373ecee21f6ad41f36e69338b4cbb5e2 100644 --- a/Makefile.am +++ b/Makefile.am @@ -114,6 +114,7 @@ endif
if HAVE_CHECK non_interactive_check_based_tests = \
dlopen-tests \ sysdb-tests \ strtonum-tests \ resolv-tests \@@ -886,6 +887,36 @@ if HAVE_CHECK libsss_test_common_la_SOURCES += \ src/tests/common_check.c
+check_LTLIBRARIES = \
- libdlopen_test_providers.la
+libdlopen_test_providers_la_SOURCES = \
- $(sssd_be_SOURCES)
+libdlopen_test_providers_la_CFLAGS = \
- $(AM_CFLAGS) \
- $(CHECK_CFLAGS) \
- -DUNIT_TESTING
+libdlopen_test_providers_la_LIBADD = \
- $(PAM_LIBS) \
- $(SSSD_LIBS) \
- $(CARES_LIBS) \
- $(SSSD_INTERNAL_LTLIBS)
+libdlopen_test_providers_la_LDFLAGS = \
- -module \
- -avoid-version \
- -Wl,--version-script,$(srcdir)/src/providers/sssd_be.exports \
- -rpath $(abs_top_builddir) \
- -export-dynamic
+dlopen_tests_SOURCES = \
- src/tests/dlopen-tests.c
+dlopen_tests_CFLAGS = \
- $(AM_CFLAGS) \
- $(CHECK_CFLAGS)
+dlopen_tests_LDADD = \
- -ldl \
Please use $(LIBADD_DL) instead of "-ldl" I did not noticed it first time.
"-ldl" was relaced in commit "AUTOMAKE: Use portable way to link with dlopen" ccf340e56364851f2e5b75e52d3d63701b662954
LS
- $(CHECK_LIBS)
$(ldblib_LTLIBRARIES)sysdb_tests_SOURCES = \ diff --git a/configure.ac b/configure.ac index 511e8d6c8afbfb0cbf5aca076b460aa4c881e252..586fe0bf9dbda5c65175d491e0044f8f09650b39 100644 --- a/configure.ac +++ b/configure.ac @@ -227,6 +227,7 @@ if test x$HAVE_PYTHON_BINDINGS != x; then AM_PATH_PYTHON([2.4]) AM_CHECK_PYTHON_HEADERS([], AC_MSG_ERROR([Could not find python headers]))
- AC_DEFINE_UNQUOTED(HAVE_PYTHON_BINDINGS, 1, [Build with python bindings])
I would prefer to have this line in the file conf_macros.m4 directly after the line src/conf_macros.m4:333: AM_CONDITIONAL([BUILD_PYTHON_BINDINGS],
In this file, there is a lot of similar patterns
AM_PYTHON_CONFIG AM_CHECK_PYTHON_COMPATfi diff --git a/src/external/krb5.m4 b/src/external/krb5.m4 index bdbd71ba1f395eb4f78047f75841729e0289ae81..a0da44a887329fbb2173a5849497d0b2cf1729a7 100644 --- a/src/external/krb5.m4 +++ b/src/external/krb5.m4 @@ -90,4 +90,5 @@ AC_CHECK_HEADER([krb5/locate_plugin.h], ]) AM_CONDITIONAL([BUILD_KRB5_LOCATOR_PLUGIN], [test x$have_locate_plugin = xyes -a x$build_locator = xyes])
+AM_COND_IF([BUILD_KRB5_LOCATOR_PLUGIN],
[AC_DEFINE_UNQUOTED(HAVE_KRB5_LOCATOR_PLUGIN, 1, [Build with krb5 locator plugin])])diff --git a/src/tests/dlopen-tests.c b/src/tests/dlopen-tests.c new file mode 100644 index 0000000000000000000000000000000000000000..608b6106d6584a528a819cc2abdf1e21469d8d38 --- /dev/null +++ b/src/tests/dlopen-tests.c @@ -0,0 +1,155 @@ +/*
- SSSD
- debug-tests.c
- Authors:
Simo Sorce <simo@redhat.com>- Copyright (C) 2013 Red Hat
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 3 of the License, or
- (at your option) any later version.
- This program 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 General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+#define _GNU_SOURCE +#include <stdbool.h> +#include <dlfcn.h> +#include <stdio.h> +#include <stdlib.h> +#include <limits.h> +#include <check.h> +#include "tests/common.h"
+#define LIBPFX ABS_BUILD_DIR"/.libs/"
+struct so {
- const char *name;
- const char *libs[6];
+} so[] = {
- { "libsss_debug.so", { LIBPFX"libsss_debug.so", NULL } },
- { "libsss_sudo.so", { LIBPFX"libsss_sudo.so", NULL } },
add check for BUILD_SUDO
- { "libipa_hbac.so", { LIBPFX"libipa_hbac.so", NULL } },
- { "libsss_autofs.so", { LIBPFX"libsss_autofs.so", NULL } },
add check for BUILD_AUTOFS
- { "libsss_idmap.so", { LIBPFX"libsss_idmap.so", NULL } },
- { "libsss_nss_idmap.so", { LIBPFX"libsss_nss_idmap.so", NULL } },
- { "libnss_sss.so", { LIBPFX"libnss_sss.so", NULL } },
- { "pam_sss.so", { LIBPFX"pam_sss.so", NULL } },
+#ifdef HAVE_KRB5_LOCATOR_PLUGIN
- { "sssd_krb5_locator_plugin.so", { LIBPFX"sssd_krb5_locator_plugin.so",
NULL } },+#endif +#ifdef HAVE_PAC_RESPONDER
- { "sssd_pac_plugin.so", { LIBPFX"sssd_pac_plugin.so", NULL } },
+#endif
- { "memberof.so", { LIBPFX"memberof.so", NULL } },
- { "libsss_child.so", { "libtevent.so",
LIBPFX"libsss_debug.so",LIBPFX"libsss_crypt.so",LIBPFX"libsss_util.so",LIBPFX"libsss_child.so", NULL } },- { "libsss_crypt.so", { "libtalloc.so",
LIBPFX"libsss_debug.so",LIBPFX"libsss_crypt.so", NULL } },- { "libsss_util.so", { "libtalloc.so",
LIBPFX"libsss_debug.so",LIBPFX"libsss_crypt.so",LIBPFX"libsss_util.so", NULL } },- { "libsss_simple.so", { LIBPFX"libdlopen_test_providers.so",
LIBPFX"libsss_simple.so", NULL } },- { "libsss_ad.so", { LIBPFX"libdlopen_test_providers.so",
LIBPFX"libsss_ad.so", NULL } },- { "libsss_ipa.so", { LIBPFX"libdlopen_test_providers.so",
LIBPFX"libsss_ipa.so", NULL } },- { "libsss_krb5.so", { LIBPFX"libdlopen_test_providers.so",
LIBPFX"libsss_krb5.so", NULL } },- { "libsss_ldap.so", { LIBPFX"libdlopen_test_providers.so",
LIBPFX"libsss_ldap.so", NULL } },- { "libsss_proxy.so", { LIBPFX"libdlopen_test_providers.so",
LIBPFX"libsss_proxy.so", NULL } },+#ifdef BUILD_PYTHON_BINDINGS
You defined macro HAVE_PYTHON_BINDINGS, but BUILD_PYTHON_BINDINGS is used here. This is a dead code.
- { "pyhbac.so", { LIBPFX"pyhbac.so", NULL } },
- { "pysss.so", { LIBPFX"pysss.so", NULL } },
- { "pysss_murmur.so", { LIBPFX"pysss_murmur.so", NULL } },
- { "pysss_nss_idmap.so", { LIBPFX"pysss_nss_idmap.so", NULL } },
+#endif
btw, BUILD_AUTOFS is used elsewhere in code. src/tests/sysdb-tests.c:4707:#ifdef BUILD_AUTOFS src/tests/sysdb-tests.c:4913:#endif /* BUILD_AUTOFS */ src/tests/sysdb-tests.c:5224:#ifdef BUILD_AUTOFS
So we can use either BUILD_PYTHON_BINDINGS, BUILD_KRB5_LOCATOR_PLUGIN or we should file a ticket to be consistent.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Mon, 2013-09-09 at 17:29 +0200, Lukas Slebodnik wrote:
+dlopen_tests_LDADD = \
- -ldl \
Please use $(LIBADD_DL) instead of "-ldl" I did not noticed it first time.
"-ldl" was relaced in commit "AUTOMAKE: Use portable way to link with dlopen" ccf340e56364851f2e5b75e52d3d63701b662954
LS
Ok fixed this one too.
Simo.
On (09/09/13 11:43), Simo Sorce wrote:
On Mon, 2013-09-09 at 17:29 +0200, Lukas Slebodnik wrote:
+dlopen_tests_LDADD = \
- -ldl \
Please use $(LIBADD_DL) instead of "-ldl" I did not noticed it first time.
"-ldl" was relaced in commit "AUTOMAKE: Use portable way to link with dlopen" ccf340e56364851f2e5b75e52d3d63701b662954
LS
Ok fixed this one too.
Simo.
-- Simo Sorce * Red Hat, Inc * New York
I checked two fixed issues: #2025 and #1838
https://fedorahosted.org/sssd/ticket/2025 fixed in 4b60fd691cc04455810d15da4f7f4044b92b07b3
Running suite(s): dlopen 0%: Checks: 1, Failures: 1, Errors: 0 src/tests/dlopen-tests.c:125:F:dlopen:test_dlopen_base:0: Error opening pysss.so: [dlopen() failed: /dev/shm/sssd_build/.libs/pysss.so: undefined symbol: debug_timestamps]
https://fedorahosted.org/sssd/ticket/1838 fixed in 3a4186ae40d0c3b7be46a4c973166f6048fcfe38
Running suite(s): dlopen 0%: Checks: 1, Failures: 1, Errors: 0 src/tests/dlopen-tests.c:125:F:dlopen:test_dlopen_base:0: Error opening libnss_sss.so: [dlopen() failed: /dev/shm/sssd_build/.libs/libnss_sss.so: undefined symbol: sss_open_cloexec]
dlopen test catched both problems.
ACK
LS
On Mon, 2013-09-09 at 19:30 +0200, Lukas Slebodnik wrote:
On (09/09/13 11:43), Simo Sorce wrote:
On Mon, 2013-09-09 at 17:29 +0200, Lukas Slebodnik wrote:
+dlopen_tests_LDADD = \
- -ldl \
Please use $(LIBADD_DL) instead of "-ldl" I did not noticed it first time.
"-ldl" was relaced in commit "AUTOMAKE: Use portable way to link with dlopen" ccf340e56364851f2e5b75e52d3d63701b662954
LS
Ok fixed this one too.
Simo.
-- Simo Sorce * Red Hat, Inc * New York
I checked two fixed issues: #2025 and #1838
https://fedorahosted.org/sssd/ticket/2025 fixed in 4b60fd691cc04455810d15da4f7f4044b92b07b3
Running suite(s): dlopen 0%: Checks: 1, Failures: 1, Errors: 0 src/tests/dlopen-tests.c:125:F:dlopen:test_dlopen_base:0: Error opening pysss.so: [dlopen() failed: /dev/shm/sssd_build/.libs/pysss.so: undefined symbol: debug_timestamps]
https://fedorahosted.org/sssd/ticket/1838 fixed in 3a4186ae40d0c3b7be46a4c973166f6048fcfe38
Running suite(s): dlopen 0%: Checks: 1, Failures: 1, Errors: 0 src/tests/dlopen-tests.c:125:F:dlopen:test_dlopen_base:0: Error opening libnss_sss.so: [dlopen() failed: /dev/shm/sssd_build/.libs/libnss_sss.so: undefined symbol: sss_open_cloexec]
dlopen test catched both problems.
ACK
Thank you, pushed to master only
Simo.
On Mon, 2013-09-09 at 09:52 +0200, Lukas Slebodnik wrote:
On (07/09/13 13:17), Simo Sorce wrote:
[..]
AM_CHECK_PYTHON_HEADERS([], AC_MSG_ERROR([Could not find python headers]))
- AC_DEFINE_UNQUOTED(HAVE_PYTHON_BINDINGS, 1, [Build with python bindings])
I would prefer to have this line in the file conf_macros.m4 directly after the line src/conf_macros.m4:333: AM_CONDITIONAL([BUILD_PYTHON_BINDINGS],
In this file, there is a lot of similar patterns
Ack, I looked for HAVE_PYTHON_BINDINGS but not hard enough evidently.
[..]
- { "libsss_debug.so", { LIBPFX"libsss_debug.so", NULL } },
- { "libsss_sudo.so", { LIBPFX"libsss_sudo.so", NULL } },
add check for BUILD_SUDO
- { "libipa_hbac.so", { LIBPFX"libipa_hbac.so", NULL } },
- { "libsss_autofs.so", { LIBPFX"libsss_autofs.so", NULL } },
add check for BUILD_AUTOFS
Good catch, I saw the defainitions and then forgot to add guards for these 2.
[..]
+#ifdef BUILD_PYTHON_BINDINGS
You defined macro HAVE_PYTHON_BINDINGS, but BUILD_PYTHON_BINDINGS is used here. This is a dead code.
Fixed
- { "pyhbac.so", { LIBPFX"pyhbac.so", NULL } },
- { "pysss.so", { LIBPFX"pysss.so", NULL } },
- { "pysss_murmur.so", { LIBPFX"pysss_murmur.so", NULL } },
- { "pysss_nss_idmap.so", { LIBPFX"pysss_nss_idmap.so", NULL } },
+#endif
btw, BUILD_AUTOFS is used elsewhere in code. src/tests/sysdb-tests.c:4707:#ifdef BUILD_AUTOFS src/tests/sysdb-tests.c:4913:#endif /* BUILD_AUTOFS */ src/tests/sysdb-tests.c:5224:#ifdef BUILD_AUTOFS
So we can use either BUILD_PYTHON_BINDINGS, BUILD_KRB5_LOCATOR_PLUGIN or we should file a ticket to be consistent.
I went back and forth a couple of times, then decided to settle on the HAVE_ pattern as that is the customery definition you find in config.h
I think it make sense to open a separate bug to change BUILD_SUDO and friends to HAVE_SUDO in config.h and in code ifdefs.
Attached new patch with fixes.
Simo.
On Fri, Sep 06, 2013 at 09:02:27AM -0400, Simo Sorce wrote:
On Fri, 2013-09-06 at 13:50 +0200, Jakub Hrozek wrote:
On Thu, Sep 05, 2013 at 09:48:27PM -0400, Simo Sorce wrote:
While reviewing the recent big patchset I sent Sumit found out I did not add some .c files to the ldap provider (which I didn't test as I was working on the krb5 provider), and that broke the provider's shared library as one symbols was missing.
We allow modules to have unresolved symbols because a number of the symbols they use come from our sssd_be binary so they can't be fully resolved at build time, only at runtime.
The attached patch adds a test that simulates loading most of the shared modules we build in order to verify that there are no unresolved symbols, that is done by manually dlopen()ing eash .so library with RTLD_NOW so all symbols are immediately resolved.
I tested that this test properly caught the issue Sumit found before fixing it.
Simo.
Hi,
I think in general this is a great test to have, thank you. See some minor comments below:
-- Simo Sorce * Red Hat, Inc * New York
From 53d8be3d2b543fec3aa83a7cfa86dcd87c7425d7 Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Thu, 5 Sep 2013 11:52:08 -0400 Subject: [PATCH] tests: Add dlopen test to make sure modules works
This tests dlopens and resolves all symbols to make sure there are no missing symbols in our provider modules.
Makefile.am | 23 ++++++++++ src/tests/dlopen-tests.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 135 insertions(+) create mode 100644 src/tests/dlopen-tests.c
diff --git a/Makefile.am b/Makefile.am index 05da9a14d82d4d0c11ec2eb888e6af46c6e48ff7..6aad7530120e783c9a1c937110b22b49cf6139eb 100644 --- a/Makefile.am +++ b/Makefile.am @@ -114,6 +114,7 @@ endif
if HAVE_CHECK non_interactive_check_based_tests = \
- dlopen-tests \
Mixed tab and space
Fixed.
sysdb-tests \ strtonum-tests \ resolv-tests \index 0000000000000000000000000000000000000000..4067162eb63feb0ebdf21b250f83bdfcb4654182 --- /dev/null +++ b/src/tests/dlopen-tests.c @@ -0,0 +1,112 @@ +/*
- SSSD
- debug-tests.c
- Authors:
Pavel Březina <pbrezina@redhat.com>You should credit yourself :)
Indeed
- Copyright (C) 2011 Red Hat
And changed year too ^^
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 3 of the License, or
- (at your option) any later version.
- This program 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 General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+#define _GNU_SOURCE +#include <stdbool.h> +#include <dlfcn.h> +#include <stdio.h> +#include <stdlib.h> +#include <limits.h> +#include <check.h> +#include "tests/common.h"
+#define LIBPFX DLOPEN_TESTS_BUILDDIR"/.libs/"
We talked about this hardcoded directory on IRC and I can't see another way to autodetect this either.
So far no good way indeed, the joys of libtool ...
+const char *sonames[] = {
- "libipa_hbac.so",
- "libsss_ad.so",
- "libsss_child.so",
- "libsss_debug.so",
- "libsss_ipa.so",
- "libsss_krb5.so",
- "libsss_ldap.so",
- "libsss_proxy.so",
- "libsss_sudo.so",
- "memberof.so",
- "pyhbac.so",
- "pysss_nss_idmap.so",
- "sssd_krb5_locator_plugin.so",
- "libnss_sss.so",
- "libsss_autofs.so",
- "libsss_crypt.so",
- "libsss_idmap.so",
- "libsss_nss_idmap.so",
- "libsss_simple.so",
- "libsss_util.so",
- "pam_sss.so",
- "sssd_pac_plugin.so",
- NULL
+};
I wonder if there's any way of autogenerating this maybe from LTLIBRARIES? But definitely not something worth fixing in this patch instance.
Not sure, if you look at the list carefully you'll see 2 .so objects are missing (sss_krb5_common.so and sss_ldap_common.so) because they would fail the test on their own and they are already implicitly tested by provider modules which have these libraries as dependencies (see output of ldd .libs/libss_ldap.so for example).
I actually initially wanted to restrict to only the providers librraries given all others are built with dependency checking, but given it cost nothing to test them all and I used a sed command to go from ls -> list and it all worked I left most of them in there.
That's fine, adding a new provider is not a common operation after all.
Maybe we could add a comment to the Makefile.am instructing the developer to watch out for this test when adding a new library?
+START_TEST(test_dlopen_base) +{
- void *handle;
- char *soname;
- int ret, i;
- for (i = 0; sonames[i] != NULL; i++) {
ret = asprintf(&soname, LIBPFX"%s", sonames[i]);fail_unless(ret != -1, "Failed to construct soname");handle = dlopen(soname, RTLD_NOW);fail_unless(handle != NULL,"dlopen() failed for %s: [%s]", soname, dlerror());dlclose(handle);free(soname);- }
+} +END_TEST
+Suite *dlopen_suite(void) +{
- Suite *s = suite_create("dlopen");
- TCase *tc_dlopen = tcase_create("dlopen");
- tcase_add_test(tc_dlopen, test_dlopen_base);
- tcase_set_timeout(tc_dlopen, 10);
- suite_add_tcase(s, tc_dlopen);
- return s;
+}
+int main(int argc, const char *argv[]) +{
- int number_failed;
- tests_set_cwd();
I think this test is a special case where we actually should not call tests_set_cwd(). Calling it breaks the parallel build for me.
Removed.
- Suite *s = dlopen_suite();
- SRunner *sr = srunner_create(s);
- srunner_run_all(sr, CK_NORMAL);
- number_failed = srunner_ntests_failed(sr);
- srunner_free(sr);
- if (number_failed == 0)
return EXIT_SUCCESS;- return EXIT_FAILURE;
+}
1.8.3.1
Otherwise looks good to me!
Ok, fixed patch attached.
Simo.
ACK from me. I'll wait until Lukas finishes his investigation before pushing.
btw the error I was seeing wasn't about parallel build per se but about my setup having different source dir, build dir and test dir..the cwd would change the working directory to test dir and then attempt to load objects from build dir.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Simo Sorce