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(a)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(a)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(a)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(a)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