On (23/04/14 11:03), Pavel Březina wrote:
On 04/22/2014 11:21 AM, Lukas Slebodnik wrote:
>On (22/04/14 10:29), Pavel Březina wrote:
>>Hi,
>>I'm sending some patches that I'll use for OpenLMI provider. It
>>supports few modifications of sssd.conf through augeas.
>>
>>For the moment, I think we should not bound to any particular API so
>>even though I made it a separate object, I don't have any intentions
>>to make it publicly usable library.
>>
>>This code will be used from D-Bus responder. I may extend the API if needed.
>>
>>Unit tests are attached.
>
>>+ option_path = build_option_path(tmp_ctx, ctx, section, option);
>>+ if (option_path == NULL) {
> ret = EINVAL???
>
>src/util/sss_config.c:203:9: error: variable 'ret' is used uninitialized
whenever 'if' condition
> is true [-Werror,-Wsometimes-uninitialized]
> if (option_path == NULL) {
> ^~~~~~~~~~~~~~~~~~~
>src/util/sss_config.c:233:12: note: uninitialized use occurs here
> return ret;
> ^~~
Thanks, fixed.
>>@@ -435,6 +435,7 @@ dist_noinst_HEADERS = \
>> src/util/sss_ssh.h \
>> src/util/sss_ini.h \
>> src/util/sss_format.h \
>>+ src/util/sss_config.h \
>> src/util/refcount.h \
>> src/util/find_uid.h \
>> src/util/user_info_msg.h \
>>@@ -577,6 +578,17 @@ pkglib_LTLIBRARIES += libsss_child.la
>>libsss_child_la_SOURCES = src/util/child_common.c
>>libsss_child_la_LDFLAGS = -avoid-version
>>
>^^^^
>trailing spaces.
>
>sh$ git am ~/000*
>Applying: sss_config: the code
>Applying: sss_config: build
>/dev/shm/sssd/.git/rebase-apply/patch:65: new blank line at EOF.
>+
>warning: 1 line adds whitespace errors.
>Applying: sss_config: unit tests
>/dev/shm/sssd/.git/rebase-apply/patch:25: trailing whitespace.
>
>warning: 1 line adds whitespace errors.
Fixed.
>
>>+pkglib_LTLIBRARIES += libsss_config.la
> ^^^^^^
> libsss_config.so will be installed in private directory /usr/lib64/sssd
> If augeas opens this library with dlopen it should be installed in different
> directory.
It is not dlopened. We use augeas libs, not the other way around.
>>+libsss_config_la_SOURCES = \
>>+ src/util/sss_config.c
>>+libsss_config_la_CFLAGS = \
>>+ $(AM_CFLAGS) \
> ^^^^
> '\t' instead of spaces.
Fixed.
Was not fixed. '\t' is still there.
s/\t/ /g
>>+ $(AUGEAS_CFLAGS)
>>+libsss_config_la_LIBADD = \
>>+ $(AUGEAS_LIBS)
>
>./.libs/libsss_config.so: undefined reference to `string_in_list'
>./.libs/libsss_config.so: undefined reference to `split_on_separator'
>./.libs/libsss_config.so: undefined reference to `add_string_to_list'
>
>add $(SSSD_INTERNAL_LTLIBS) to LIBADD
You should move this part behind the
library libsss_util.la
or better behind the definition of make-variable SSSD_INTERNAL_LTLIBS.
Because there is a link failure in make distcheck
libtool: install: warning: relinking `libsss_config.la'
libtool: install: (cd sssd_build/sssd-1.11.90/_build; /bin/sh
sssd_build/sssd-1.11.90/_build/libtool --silent --tag CC
--mode=relink gcc -Wall -Wshadow -Wstrict-prototypes -Wpointer-arith
-Wcast-qual -Wcast-align -Wwrite-strings -Wundef
-Werror-implicit-function-declaration -Winit-self -fno-strict-aliasing
-std=gnu99 -I/usr/include/libxml2 -g -O2 -D_FILE_OFFSET_BITS=64
-D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -avoid-version -o
libsss_config.la -rpath sssd_build/sssd-1.11.90/_inst/lib/sssd
src/util/libsss_config_la-sss_config.lo -laugeas libsss_util.la
libsss_crypt.la libsss_debug.la libsss_child.la -ldl )
/usr/bin/ld: cannot find -lsss_util
collect2: error: ld returned 1 exit status
libtool: install: error: relink `libsss_config.la' with the above command
before installing it
make[4]: *** [install-pkglibLTLIBRARIES] Error 1
Fixed.
>>+++ b/src/external/libaugeas.m4
>>@@ -0,0 +1,10 @@
>>+AC_SUBST(AUGEAS_OBJ)
>>+AC_SUBST(AUGEAS_CFLAGS)
>>+AC_SUBST(AUGEAS_LIBS)
>>+
>>+PKG_CHECK_MODULES(AUGEAS,
>>+ augeas >= 1.0.0,
>>+ ,
>>+ AC_MSG_ERROR("Please install augeas-devel")
>>+ )
>>+
>>--
>>1.7.11.7
>>
>
>Could be this optional dependency like a cifs-idmap-plugin?
> src/external/cifsidmap.m4
This library is intended to be used from dbus responder only, for the
time being.
If it is library only for dbus responder it can be installed
in pkglibdir. (current state)
Do we want to build it by default and add an option to disable it?
I would prefer this one.
Do we want to build it only when dbus responder is enabled?
Will
dbus responder work without this library? (I expect answer: yes)
Do we want to allow the possibility to build dbus responder without
support of configuration changes?
libaugeas is the new dependency and some people may want to have minimal
dependencies for sssd. (minimal installed size on ARM based board)
I would expect dbus responder will not add new dependencies.
sssd has already dependency on libdbus. It would be nice to have an option
to build sssd without libaugeas.
> You added new build time dependency, but spec file was not
change.
>You created new dynamic library(module) but it was not added into dlopen tests.
Will do, once we decide the build condition.
Please wait for replies from other developrers :-)
Thanks for the review.
LS