[SSSD] [PATCH] Check our libraries properly resolve symbols
Simo Sorce
simo at redhat.com
Fri Sep 6 13:02:27 UTC 2013
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 at 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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-tests-Add-dlopen-test-to-make-sure-modules-works.patch
Type: text/x-patch
Size: 4569 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130906/75110a91/attachment.bin>
More information about the sssd-devel
mailing list