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 <s...@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 <pbrez...@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 <s...@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 <s...@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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
