-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1

On 04/22/2014 05: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.

...

From 29b998ed4b98b43a815023f32413442c01ecf92a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Fri, 18 Apr 2014 17:23:16 +0200 Subject: [PATCH 1/3] sss_config: the code

--- src/util/sss_config.c | 507 ++++++++++++++++++++++++++++++++++++++++++++++++++ src/util/sss_config.h | 71 +++++++ 2 files changed, 578 insertions(+) create mode 100644 src/util/sss_config.c create mode 100644 src/util/sss_config.h

diff --git a/src/util/sss_config.c b/src/util/sss_config.c new file mode 100644 index 0000000000000000000000000000000000000000..6a6216da1b90adc2628cbdae692228121162fb3e

- --- /dev/null

...

+++ b/src/util/sss_config.c @@ -0,0 +1,507 @@ +/* + Authors: + Pavel Březina pbrezina@redhat.com + + Copyright (C) 2014 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/. +*/ + +#include <augeas.h> +#include <talloc.h> +#include <string.h> +#include "util/sss_config.h" + +#define PATH_SECTION "/files/%s/target[. = "%s"]" +#define PATH_OPTION PATH_SECTION "/%s" + +#define build_section_path(mem_ctx, config_ctx, section) \ + talloc_asprintf(mem_ctx, PATH_SECTION, config_ctx->file, section) + +#define build_option_path(mem_ctx, config_ctx, section, option) \ + talloc_asprintf(mem_ctx, PATH_OPTION, config_ctx->file, section, option) + +struct sss_config_ctx +{ + augeas *auges_ctx; + const char *file; +}; + +static errno_t +sss_config_set_option(struct sss_config_ctx *ctx, + const char *section, + const char *option, + const char *value) +{ + TALLOC_CTX *tmp_ctx = NULL; + char *target_path = NULL; + char *option_path = NULL; + errno_t ret; + int aug_ret; + + tmp_ctx = talloc_new(NULL); + if (tmp_ctx == NULL) { + return ENOMEM; + } + + target_path = build_section_path(tmp_ctx, ctx, section); + if (target_path == NULL) { + ret = ENOMEM; + goto done; + } + + option_path = build_option_path(tmp_ctx, ctx, section, option); + if (option_path == NULL) { + ret = ENOMEM; + goto done; + } + + /* Set configuration option: + * + * # make sure the section exists + * set /files/$file/target[. = "$section"] $section + * + * # set value + * set /files/$file/target[. = "$section"]/$option $value + */ + + aug_ret = aug_set(ctx->auges_ctx, target_path, section); + if (aug_ret != 0) { + ret = EIO; + goto done; + } + + aug_ret = aug_set(ctx->auges_ctx, option_path, value); + if (aug_ret != 0) { + ret = EIO; + goto done; + }

• • ret = EOK; + +done: + talloc_free(tmp_ctx); + return

ret; +} + +static errno_t +sss_config_rm_option(struct sss_config_ctx *ctx, + const char *section,

•                  const char *option) +{ +    TALLOC_CTX
  

*tmp_ctx = NULL; + char *option_path = NULL; + errno_t ret; + int aug_ret; + + tmp_ctx = talloc_new(NULL); + if (tmp_ctx == NULL) { + return ENOMEM; + } + + option_path = build_option_path(tmp_ctx, ctx, section, option); + if (option_path == NULL) { + ret = ENOMEM; + goto done; + } + + /* Remove configuration option: + * +

• rm /files/$file/target[. = "$section"]/$option + */ + +

aug_ret = aug_rm(ctx->auges_ctx, option_path); + if (aug_ret != 1) { + ret = EIO; + goto done; + } + + ret = EOK; + +done: + talloc_free(tmp_ctx); + return ret; +} + +static errno_t +sss_config_set_list(struct sss_config_ctx *ctx,

•                const char *section, +
  

const char *option, + char **list) +{ + TALLOC_CTX *tmp_ctx = NULL; + char *value = NULL; + errno_t ret; + int i; + + if (list == NULL) { + return EOK;

• } + + tmp_ctx = talloc_new(NULL); + if (tmp_ctx ==

NULL) { + return ENOMEM; + } + + if (list[0] == NULL) { + ret = sss_config_rm_option(ctx, section, option); + goto done; + } + + value = talloc_strdup(tmp_ctx, list[0]); + if (value == NULL) { + ret = ENOMEM; + goto done; + } + + for (i = 1; list[i] != NULL; i++) { + value = talloc_asprintf_append(value, ", %s", list[i]); + if (value == NULL) { + ret = ENOMEM; + goto done; + } + } + + ret = sss_config_set_option(ctx, section, option, value); + +done: + talloc_free(tmp_ctx); + return ret; +} + +static errno_t +sss_config_get_list(TALLOC_CTX *mem_ctx, + struct sss_config_ctx *ctx, + const char *section, + const char *option, + char ***_list) +{ + TALLOC_CTX *tmp_ctx = NULL; + char *option_path = NULL; + const char *value = NULL; + char **list = NULL; + errno_t ret; + int aug_ret; + + tmp_ctx = talloc_new(NULL); + if (tmp_ctx == NULL) { + return ENOMEM; + } + + 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; ^~~

...

•    goto done; +    } + +    aug_ret =
  

aug_get(ctx->auges_ctx, option_path, &value); + if (aug_ret == 0 || (aug_ret == 1 && (value == NULL || *value == '\0'))) { + /* option is not present, return empty list */ + list = talloc_zero_array(tmp_ctx, char*, 1); + if (list == NULL) { + ret = ENOMEM; + } + + ret = EOK; + goto done; + } else if (aug_ret != 1) { + /* error: more than one value found */ + ret = EINVAL; + goto done; + } + + ret = split_on_separator(tmp_ctx, value, ',', true, true, &list, NULL); + if (ret != EOK) { + goto done; + } + + *_list = talloc_steal(mem_ctx, list); + ret = EOK; + +done: + talloc_free(tmp_ctx); + return ret; +} + +static errno_t +sss_config_is_in_list(struct sss_config_ctx *ctx, + const char *section, + const char *option, + const char *value, + bool *_result) +{ + char **list = NULL; + errno_t ret; + + ret = sss_config_get_list(ctx, ctx, section, option, &list); + if (ret != EOK) { + goto done; + } + + *_result = string_in_list(value, list, true); + +done: + talloc_free(list); + return ret; +} + +static errno_t +sss_config_add_to_list(struct sss_config_ctx *ctx, + const char *section, + const char *option,

•                   const char *value) +{ +    TALLOC_CTX
  

*tmp_ctx = NULL; + char **list = NULL; + errno_t ret; + bool result = false; + + tmp_ctx = talloc_new(NULL); + if (tmp_ctx == NULL) { + return ENOMEM; + } + + ret = sss_config_get_list(tmp_ctx, ctx, section, option, &list); + if (ret != EOK) { + goto done; + } + + result = string_in_list(value, list, true); + if (result == true) { + ret = EOK; + goto done; + } + + ret = add_string_to_list(tmp_ctx, value, &list); + if (ret != EOK) { + goto done; + } + + ret = sss_config_set_list(ctx, section, option, list); + +done: + talloc_free(tmp_ctx); + return ret; +} + +static errno_t +sss_config_del_from_list(struct sss_config_ctx *ctx, + const char *section, + const char *option, + const char *value) +{ + TALLOC_CTX *tmp_ctx = NULL; + char **list = NULL; + errno_t ret; + bool found; + int i; + + tmp_ctx = talloc_new(NULL); + if (tmp_ctx == NULL) { + return ENOMEM; + } + + ret = sss_config_get_list(tmp_ctx, ctx, section, option, &list); + if (ret != EOK) { + goto done; + } + + if (list == NULL) { + goto done; + } + + found = false; + for (i = 0; list[i] != NULL; i++) {

•    if (strcmp(list[i], value) == 0) { +            found =
  

true; + } + + if (found) { + list[i] = list[i + 1]; + } + } + + ret = sss_config_set_list(ctx, section, option, list); + +done: + talloc_free(tmp_ctx); + return ret; +} + +static int sss_config_ctx_destructor(struct sss_config_ctx *ctx) +{ + if (ctx->auges_ctx != NULL) { + aug_close(ctx->auges_ctx); + ctx->auges_ctx = NULL; + } + + return 0; +} + +struct sss_config_ctx * +sss_config_open(TALLOC_CTX *mem_ctx, + const char *root, + const char *file) +{ + struct sss_config_ctx *ctx = NULL; + errno_t ret; + int aug_ret; + + ctx = talloc_zero(mem_ctx, struct sss_config_ctx); + if (ctx == NULL) { + return NULL; + } + + talloc_set_destructor(ctx, sss_config_ctx_destructor);

• • ctx->auges_ctx = aug_init(root, NULL, AUG_NO_LOAD |

AUG_NO_MODL_AUTOLOAD + | AUG_SAVE_BACKUP); + if (ctx->auges_ctx == NULL) { + ret = ENOMEM; + goto done; + } + + ctx->file = talloc_strdup(ctx, file); + if (ctx->file == NULL) { + ret = ENOMEM; + goto done; + } + + /* Load configuration file + * + * set /augeas/load/sssd/lens sssd.lns + * set /augeas/load/sssd/incl $file + * load + */ + + aug_ret = aug_set(ctx->auges_ctx, "/augeas/load/sssd/lens", "sssd.lns"); + if (aug_ret != 0) { + ret = EIO; + goto done; + } + + aug_ret = aug_set(ctx->auges_ctx, "/augeas/load/sssd/incl", ctx->file); + if (aug_ret != 0) { + ret = EIO; + goto done; + } + + aug_ret = aug_load(ctx->auges_ctx); + if (aug_ret != 0) { + ret = EIO; + goto done; + } + + ret = EOK; + +done: + if (ret != EOK) { + talloc_free(ctx); + } + + return ctx; +} + +errno_t +sss_config_save(struct sss_config_ctx *ctx) +{ + int aug_ret; + + aug_ret = aug_save(ctx->auges_ctx); + if (aug_ret != 0) { + return EIO; + } + + return EOK; +} + +void +sss_config_close(struct sss_config_ctx **_ctx) +{ + if (_ctx == NULL || *_ctx == NULL) { + return; + } + + talloc_free(*_ctx); + *_ctx = NULL; +} + +errno_t +sss_config_set_debug_level(struct sss_config_ctx *ctx, + const char *section, + uint32_t level) +{ + char *level_str = NULL; + errno_t ret; + + level_str = talloc_asprintf(ctx, "%#.4x", level); + if (level_str == NULL) { + return ENOMEM; + } + + ret = sss_config_set_option(ctx, section, CONFDB_SERVICE_DEBUG_LEVEL, + level_str); + + talloc_free(level_str); + return ret; +} + +errno_t +sss_config_service_is_enabled(struct sss_config_ctx *ctx, + const char *service, + bool *_result) +{ + return sss_config_is_in_list(ctx, "sssd", CONFDB_MONITOR_ACTIVE_SERVICES, + service, _result); +} + +errno_t +sss_config_service_enable(struct sss_config_ctx *ctx, + const char *service) +{ + return sss_config_add_to_list(ctx, "sssd", CONFDB_MONITOR_ACTIVE_SERVICES, + service); +} + +errno_t +sss_config_service_disable(struct sss_config_ctx *ctx, + const char *service) +{ + return sss_config_del_from_list(ctx, "sssd", CONFDB_MONITOR_ACTIVE_SERVICES, + service); +} + +errno_t +sss_config_domain_is_enabled(struct sss_config_ctx *ctx, + const char *domain, + bool *_result) +{ + return sss_config_is_in_list(ctx, "sssd", CONFDB_MONITOR_ACTIVE_DOMAINS, + domain, _result); +} + +errno_t +sss_config_domain_enable(struct sss_config_ctx *ctx, + const char *domain) +{ + return sss_config_add_to_list(ctx, "sssd", CONFDB_MONITOR_ACTIVE_DOMAINS, + domain); +} + +errno_t +sss_config_domain_disable(struct sss_config_ctx *ctx, + const char *domain) +{ + return sss_config_del_from_list(ctx, "sssd", CONFDB_MONITOR_ACTIVE_DOMAINS, + domain); +} diff --git a/src/util/sss_config.h b/src/util/sss_config.h new file mode 100644 index 0000000000000000000000000000000000000000..b59f5fcf0e23e7a6c8543346c52fa06ae6041a3d

- --- /dev/null

...

+++ b/src/util/sss_config.h @@ -0,0 +1,71 @@ +/* + Authors: + Pavel Březina pbrezina@redhat.com + + Copyright (C) 2014 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/. +*/ + +#ifndef SSS_CONFIG_H_ +#define SSS_CONFIG_H_ + +#include <talloc.h> +#include "util/util.h" + +struct sss_config_ctx; + +struct sss_config_ctx

• +sss_config_open(TALLOC_CTX *mem_ctx, + const

char *root, + const char *file); + +errno_t +sss_config_save(struct sss_config_ctx *ctx); + +void +sss_config_close(struct sss_config_ctx **_ctx); + +errno_t +sss_config_set_debug_level(struct sss_config_ctx *ctx, + const char *section, + uint32_t level); + +errno_t +sss_config_service_is_enabled(struct sss_config_ctx *ctx, + const char *service, + bool *_result); + +errno_t +sss_config_service_enable(struct sss_config_ctx *ctx, + const char *service); + +errno_t +sss_config_service_disable(struct sss_config_ctx *ctx, + const char *service); + +errno_t +sss_config_domain_is_enabled(struct sss_config_ctx *ctx, + const char *domain, + bool *_result); + +errno_t +sss_config_domain_enable(struct sss_config_ctx *ctx, + const char *domain); + +errno_t +sss_config_domain_disable(struct sss_config_ctx *ctx, + const char *domain); + +#endif /* SSS_CONFIG_H_ */ -- 1.7.11.7

...

From 38a199b55608cf42ff3879a9aaa168ef619a42d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Mon, 21 Apr 2014 12:17:53 +0200 Subject: [PATCH 2/3] sss_config: build

--- Makefile.am | 12 ++++++++++++ configure.ac | 1 + src/external/libaugeas.m4 | 10 ++++++++++ 3 files changed, 23 insertions(+) create mode 100644 src/external/libaugeas.m4

diff --git a/Makefile.am b/Makefile.am index 0ce377a03f0b606f18b4e1524de375744096eb09..58b4159b0d1a47ef98a2b88dd751f75f5dc54e18 100644 --- a/Makefile.am +++ b/Makefile.am @@ -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.

...

+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.

...

+libsss_config_la_SOURCES = \ + src/util/sss_config.c +libsss_config_la_CFLAGS = \ + $(AM_CFLAGS) \

^^^^ '\t' instead of spaces.

...

• $(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

...

+libsss_config_la_LDFLAGS = \ + -avoid-version + pkglib_LTLIBRARIES += libsss_util.la libsss_util_la_SOURCES = \ src/confdb/confdb.c \ diff --git a/configure.ac b/configure.ac index 41fa6496553336c73338459eaff639f10fde74f1..11bb0874bdf913070fc2f7509523a3f0b3982c42 100644 --- a/configure.ac +++ b/configure.ac @@ -129,6 +129,7 @@ WITH_CRYPTO WITH_SYSLOG

m4_include([src/external/pkg.m4]) +m4_include([src/external/libaugeas.m4]) m4_include([src/external/libpopt.m4]) m4_include([src/external/libtalloc.m4]) m4_include([src/external/libtdb.m4]) diff --git a/src/external/libaugeas.m4 b/src/external/libaugeas.m4 new file mode 100644 index 0000000000000000000000000000000000000000..813d1067804f45b2b6f593f3bb2c3bc32112bb24

- --- /dev/null

...

+++ 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

You added new build time dependency, but spec file was not change.

The spec file needs the new BuildRequires and also the new %files for the internal shared library. We also need to ask ourselves whether we need to make this library multiarch.

Could be this optional dependency like a cifs-idmap-plugin? src/external/cifsidmap.m4

You created new dynamic library(module) but it was not added into dlopen tests.

LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

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

On (01/05/14 19:20), 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.

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; ^~~

...

•    goto done;
  

• }

You added new build time dependency, but spec file was not change.

Could be this optional dependency like a cifs-idmap-plugin? src/external/cifsidmap.m4

You created new dynamic library(module) but it was not added into dlopen tests.

LS

Hi, I'm sending new version with augeas as optional dependency. The config lib is build by default and can be disabled by --disable-config-lib.

From 06a8e861c28b311e629bc425bb603b9f3205256d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Fri, 18 Apr 2014 17:23:16 +0200 Subject: [PATCH 1/3] sss_config: the code

---

src/util/sss_config.c | 509 ++++++++++++++++++++++++++++++++++++++++++++++++++ src/util/sss_config.h | 71 +++++++ 2 files changed, 580 insertions(+) create mode 100644 src/util/sss_config.c create mode 100644 src/util/sss_config.h

diff --git a/src/util/sss_config.c b/src/util/sss_config.c new file mode 100644 index 0000000000000000000000000000000000000000..9654db535cf50e2bc6ec39c66b04af757318d345 --- /dev/null +++ b/src/util/sss_config.c @@ -0,0 +1,509 @@ +/*

• Authors:

•    Pavel Březina <pbrezina@redhat.com>
  

• Copyright (C) 2014 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/.

+*/

+#include <augeas.h> +#include <talloc.h> +#include <string.h> +#include "util/sss_config.h"

+#define PATH_SECTION "/files/%s/target[. = "%s"]" +#define PATH_OPTION PATH_SECTION "/%s"

+#define build_section_path(mem_ctx, config_ctx, section) \

• talloc_asprintf(mem_ctx, PATH_SECTION, config_ctx->file, section)

+#define build_option_path(mem_ctx, config_ctx, section, option) \

• talloc_asprintf(mem_ctx, PATH_OPTION, config_ctx->file, section, option)

+struct sss_config_ctx +{

• augeas *auges_ctx;
• const char *file;

+};

+static errno_t +sss_config_set_option(struct sss_config_ctx *ctx,

•                  const char *section,
  

•                  const char *option,
  

•                  const char *value)
  

+{

• TALLOC_CTX *tmp_ctx = NULL;
• char *target_path = NULL;
• char *option_path = NULL;
• errno_t ret;
• int aug_ret;
• tmp_ctx = talloc_new(NULL);
• if (tmp_ctx == NULL) {

•    return ENOMEM;
  

• }
• target_path = build_section_path(tmp_ctx, ctx, section);
• if (target_path == NULL) {

•    ret = ENOMEM;
  

•    goto done;
  

• }
• option_path = build_option_path(tmp_ctx, ctx, section, option);
• if (option_path == NULL) {

•    ret = ENOMEM;
  

•    goto done;
  

• }
• /* Set configuration option:

• *
  

• * # make sure the section exists
  

• * set /files/$file/target[. = "$section"] $section
  

• *
  

• * # set value
  

• * set /files/$file/target[. = "$section"]/$option $value
  

• */
  

• aug_ret = aug_set(ctx->auges_ctx, target_path, section);
• if (aug_ret != 0) {

•    ret = EIO;
  

•    goto done;
  

• }
• aug_ret = aug_set(ctx->auges_ctx, option_path, value);
• if (aug_ret != 0) {

•    ret = EIO;
  

•    goto done;
  

• }
• ret = EOK;

+done:

• talloc_free(tmp_ctx);
• return ret;

+}

+static errno_t +sss_config_rm_option(struct sss_config_ctx *ctx,

•                  const char *section,
  

•                  const char *option)
  

+{

• TALLOC_CTX *tmp_ctx = NULL;
• char *option_path = NULL;
• errno_t ret;
• int aug_ret;
• tmp_ctx = talloc_new(NULL);
• if (tmp_ctx == NULL) {

•    return ENOMEM;
  

• }
• option_path = build_option_path(tmp_ctx, ctx, section, option);
• if (option_path == NULL) {

•    ret = ENOMEM;
  

•    goto done;
  

• }
• /* Remove configuration option:

• *
  

• * rm /files/$file/target[. = "$section"]/$option
  

• */
  

• aug_ret = aug_rm(ctx->auges_ctx, option_path);
• if (aug_ret != 1) {

•    ret = EIO;
  

•    goto done;
  

• }
• ret = EOK;

+done:

• talloc_free(tmp_ctx);
• return ret;

+}

+static errno_t +sss_config_set_list(struct sss_config_ctx *ctx,

•                const char *section,
  

•                const char *option,
  

•                char **list)
  

+{

• TALLOC_CTX *tmp_ctx = NULL;
• char *value = NULL;
• errno_t ret;
• int i;
• if (list == NULL) {

•    return EOK;
  

Could you explain why is EOK returned?

• }
• tmp_ctx = talloc_new(NULL);
• if (tmp_ctx == NULL) {

•    return ENOMEM;
  

• }
• if (list[0] == NULL) {

•    ret = sss_config_rm_option(ctx, section, option);
  

•    goto done;
  

• }
• value = talloc_strdup(tmp_ctx, list[0]);
• if (value == NULL) {

•    ret = ENOMEM;
  

•    goto done;
  

• }
• for (i = 1; list[i] != NULL; i++) {

•    value = talloc_asprintf_append(value, ", %s", list[i]);
  

•    if (value == NULL) {
  

•        ret = ENOMEM;
  

•        goto done;
  

•    }
  

• }
• ret = sss_config_set_option(ctx, section, option, value);

+done:

• talloc_free(tmp_ctx);
• return ret;

+}

+static errno_t +sss_config_get_list(TALLOC_CTX *mem_ctx,

•                struct sss_config_ctx *ctx,
  

•                const char *section,
  

•                const char *option,
  

•                char ***_list)
  

+{

• TALLOC_CTX *tmp_ctx = NULL;
• char *option_path = NULL;
• const char *value = NULL;
• char **list = NULL;
• errno_t ret;
• int aug_ret;
• tmp_ctx = talloc_new(NULL);
• if (tmp_ctx == NULL) {

•    return ENOMEM;
  

• }
• option_path = build_option_path(tmp_ctx, ctx, section, option);
• if (option_path == NULL) {

•    ret = ENOMEM;
  

•    goto done;
  

• }
• aug_ret = aug_get(ctx->auges_ctx, option_path, &value);
• if (aug_ret == 0 || (aug_ret == 1 && (value == NULL || *value == '\0'))) {

•    /* option is not present, return empty list */
  

•    list = talloc_zero_array(tmp_ctx, char*, 1);
  

•    if (list == NULL) {
  

•        ret = ENOMEM;
  

•        goto done;
  

•    }
  

•    ret = EOK;
  

•    goto done;
  

• } else if (aug_ret != 1) {

•    /* error: more than one value found */
  

•    ret = EINVAL;
  

•    goto done;
  

• }
• ret = split_on_separator(tmp_ctx, value, ',', true, true, &list, NULL);
• if (ret != EOK) {

•    goto done;
  

• }
• *_list = talloc_steal(mem_ctx, list);
• ret = EOK;

+done:

• talloc_free(tmp_ctx);
• return ret;

+}

+static errno_t +sss_config_is_in_list(struct sss_config_ctx *ctx,

•                  const char *section,
  

•                  const char *option,
  

•                  const char *value,
  

•                  bool *_result)
  

+{

• char **list = NULL;
• errno_t ret;
• ret = sss_config_get_list(ctx, ctx, section, option, &list);
• if (ret != EOK) {

•    goto done;
  

• }
• *_result = string_in_list(value, list, true);

+done:

• talloc_free(list);
• return ret;

+}

+static errno_t +sss_config_add_to_list(struct sss_config_ctx *ctx,

•                   const char *section,
  

•                   const char *option,
  

•                   const char *value)
  

+{

• TALLOC_CTX *tmp_ctx = NULL;
• char **list = NULL;
• errno_t ret;
• bool result = false;
• tmp_ctx = talloc_new(NULL);
• if (tmp_ctx == NULL) {

•    return ENOMEM;
  

• }
• ret = sss_config_get_list(tmp_ctx, ctx, section, option, &list);
• if (ret != EOK) {

•    goto done;
  

• }
• result = string_in_list(value, list, true);
• if (result == true) {

•    ret = EOK;
  

•    goto done;
  

• }
• ret = add_string_to_list(tmp_ctx, value, &list);
• if (ret != EOK) {

•    goto done;
  

• }
• ret = sss_config_set_list(ctx, section, option, list);

+done:

• talloc_free(tmp_ctx);
• return ret;

+}

+static errno_t +sss_config_del_from_list(struct sss_config_ctx *ctx,

•                     const char *section,
  

•                     const char *option,
  

•                     const char *value)
  

+{

• TALLOC_CTX *tmp_ctx = NULL;
• char **list = NULL;
• errno_t ret;
• bool found;
• int i;
• tmp_ctx = talloc_new(NULL);
• if (tmp_ctx == NULL) {

•    return ENOMEM;
  

• }
• ret = sss_config_get_list(tmp_ctx, ctx, section, option, &list);
• if (ret != EOK) {

•    goto done;
  

• }
• if (list == NULL) {

•    goto done;
  

• }
• found = false;
• for (i = 0; list[i] != NULL; i++) {

•    if (strcmp(list[i], value) == 0) {
  

•        found = true;
  

•    }
  

•    if (found) {
  

•        list[i] = list[i + 1];
  

•    }
  

• }
• ret = sss_config_set_list(ctx, section, option, list);

+done:

• talloc_free(tmp_ctx);
• return ret;

+}

+static int sss_config_ctx_destructor(struct sss_config_ctx *ctx) +{

• if (ctx->auges_ctx != NULL) {

•    aug_close(ctx->auges_ctx);
  

•    ctx->auges_ctx = NULL;
  

• }
• return 0;

+}

+struct sss_config_ctx * +sss_config_open(TALLOC_CTX *mem_ctx,

•            const char *root,
  

•            const char *file)
  

+{

• struct sss_config_ctx *ctx = NULL;
• errno_t ret;
• int aug_ret;
• ctx = talloc_zero(mem_ctx, struct sss_config_ctx);
• if (ctx == NULL) {

•    return NULL;
  

• }
• talloc_set_destructor(ctx, sss_config_ctx_destructor);
• ctx->auges_ctx = aug_init(root, NULL, AUG_NO_LOAD | AUG_NO_MODL_AUTOLOAD

•                          | AUG_SAVE_BACKUP);
  

• if (ctx->auges_ctx == NULL) {

•    ret = ENOMEM;
  

•    goto done;
  

• }
• ctx->file = talloc_strdup(ctx, file);
• if (ctx->file == NULL) {

•    ret = ENOMEM;
  

•    goto done;
  

• }
• /* Load configuration file

• *
  

• * set /augeas/load/sssd/lens sssd.lns
  

• * set /augeas/load/sssd/incl $file
  

• * load
  

• */
  

• aug_ret = aug_set(ctx->auges_ctx, "/augeas/load/sssd/lens", "sssd.lns");
• if (aug_ret != 0) {

•    ret = EIO;
  

•    goto done;
  

• }
• aug_ret = aug_set(ctx->auges_ctx, "/augeas/load/sssd/incl", ctx->file);
• if (aug_ret != 0) {

•    ret = EIO;
  

•    goto done;
  

• }
• aug_ret = aug_load(ctx->auges_ctx);
• if (aug_ret != 0) {

•    ret = EIO;
  

•    goto done;
  

• }
• ret = EOK;

+done:

• if (ret != EOK) {

•    talloc_free(ctx);
  

• }
• return ctx;

+}

+errno_t +sss_config_save(struct sss_config_ctx *ctx) +{

• int aug_ret;
• aug_ret = aug_save(ctx->auges_ctx);
• if (aug_ret != 0) {

•    return EIO;
  

• }
• return EOK;

+}

+void +sss_config_close(struct sss_config_ctx **_ctx) +{

• if (_ctx == NULL || *_ctx == NULL) {

•    return;
  

• }
• talloc_free(*_ctx);
• *_ctx = NULL;

+}

+errno_t +sss_config_set_debug_level(struct sss_config_ctx *ctx,

•                       const char *section,
  

•                       uint32_t level)
  

+{

• char *level_str = NULL;
• errno_t ret;
• level_str = talloc_asprintf(ctx, "%#.4x", level);
• if (level_str == NULL) {

•    return ENOMEM;
  

• }
• ret = sss_config_set_option(ctx, section, CONFDB_SERVICE_DEBUG_LEVEL,

•                            level_str);
  

• talloc_free(level_str);
• return ret;

+}

+errno_t +sss_config_service_is_enabled(struct sss_config_ctx *ctx,

•                          const char *service,
  

•                          bool *_result)
  

+{

• return sss_config_is_in_list(ctx, "sssd", CONFDB_MONITOR_ACTIVE_SERVICES,

•                             service, _result);
  

+}

+errno_t +sss_config_service_enable(struct sss_config_ctx *ctx,

•                      const char *service)
  

+{

• return sss_config_add_to_list(ctx, "sssd", CONFDB_MONITOR_ACTIVE_SERVICES,

•                              service);
  

+}

+errno_t +sss_config_service_disable(struct sss_config_ctx *ctx,

•                       const char *service)
  

+{

• return sss_config_del_from_list(ctx, "sssd", CONFDB_MONITOR_ACTIVE_SERVICES,

•                                service);
  

+}

+errno_t +sss_config_domain_is_enabled(struct sss_config_ctx *ctx,

•                         const char *domain,
  

•                         bool *_result)
  

+{

• return sss_config_is_in_list(ctx, "sssd", CONFDB_MONITOR_ACTIVE_DOMAINS,

•                             domain, _result);
  

+}

+errno_t +sss_config_domain_enable(struct sss_config_ctx *ctx,

•                     const char *domain)
  

+{

• return sss_config_add_to_list(ctx, "sssd", CONFDB_MONITOR_ACTIVE_DOMAINS,

•                              domain);
  

+}

+errno_t +sss_config_domain_disable(struct sss_config_ctx *ctx,

•                      const char *domain)
  

+{

• return sss_config_del_from_list(ctx, "sssd", CONFDB_MONITOR_ACTIVE_DOMAINS,

•                                domain);
  

+} diff --git a/src/util/sss_config.h b/src/util/sss_config.h new file mode 100644 index 0000000000000000000000000000000000000000..b59f5fcf0e23e7a6c8543346c52fa06ae6041a3d --- /dev/null +++ b/src/util/sss_config.h @@ -0,0 +1,71 @@ +/*

• Authors:

•    Pavel Březina <pbrezina@redhat.com>
  

• Copyright (C) 2014 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/.

+*/

+#ifndef SSS_CONFIG_H_ +#define SSS_CONFIG_H_

+#include <talloc.h> +#include "util/util.h"

+struct sss_config_ctx;

+struct sss_config_ctx * +sss_config_open(TALLOC_CTX *mem_ctx,

•            const char *root,
  

•            const char *file);
  

+errno_t +sss_config_save(struct sss_config_ctx *ctx);

+void +sss_config_close(struct sss_config_ctx **_ctx);

+errno_t +sss_config_set_debug_level(struct sss_config_ctx *ctx,

•                       const char *section,
  

•                       uint32_t level);
  

+errno_t +sss_config_service_is_enabled(struct sss_config_ctx *ctx,

•                          const char *service,
  

•                          bool *_result);
  

+errno_t +sss_config_service_enable(struct sss_config_ctx *ctx,

•                      const char *service);
  

+errno_t +sss_config_service_disable(struct sss_config_ctx *ctx,

•                       const char *service);
  

+errno_t +sss_config_domain_is_enabled(struct sss_config_ctx *ctx,

•                         const char *domain,
  

•                         bool *_result);
  

+errno_t +sss_config_domain_enable(struct sss_config_ctx *ctx,

•                     const char *domain);
  

+errno_t +sss_config_domain_disable(struct sss_config_ctx *ctx,

•                      const char *domain);
  

+#endif /* SSS_CONFIG_H_ */

1.7.11.7

From 9dbc0dee60bff938fe03993c4953f9b76834815a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Mon, 21 Apr 2014 12:17:53 +0200 Subject: [PATCH 2/3] sss_config: build

---

Makefile.am | 15 +++++++++++++++ configure.ac | 5 +++++ contrib/sssd.spec.in | 2 ++ src/external/configlib.m4 | 12 ++++++++++++ src/external/libaugeas.m4 | 9 +++++++++ 5 files changed, 43 insertions(+) create mode 100644 src/external/configlib.m4 create mode 100644 src/external/libaugeas.m4

diff --git a/Makefile.am b/Makefile.am index e58b9afd26bdc660b57039148de4bc2f56fde5d7..bdc399e17ec2961ac5c5d4f1e42cf5fc0f09711a 100644 --- a/Makefile.am +++ b/Makefile.am @@ -437,6 +437,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 \

@@ -651,6 +652,20 @@ SSSD_INTERNAL_LTLIBS = \ libsss_debug.la \ libsss_child.la

+if BUILD_CONFIG_LIB +pkglib_LTLIBRARIES += libsss_config.la +libsss_config_la_SOURCES = \

• src/util/sss_config.c

+libsss_config_la_CFLAGS = \

• $(AM_CFLAGS) \
• $(AUGEAS_CFLAGS)

+libsss_config_la_LIBADD = \

• $(AUGEAS_LIBS) \
• $(SSSD_INTERNAL_LTLIBS)

This library use talloc, it would be nice to link with talloc directly and do not rely on other libraries (libsss_util.so)

+libsss_config_la_LDFLAGS = \

• avoid-version

+endif

lib_LTLIBRARIES = libipa_hbac.la libsss_idmap.la libsss_nss_idmap.la pkgconfig_DATA += src/providers/ipa/ipa_hbac.pc libipa_hbac_la_SOURCES = \ diff --git a/configure.ac b/configure.ac index 41fa6496553336c73338459eaff639f10fde74f1..65008d41f55aa4e3c6a4b374ec907e729ab92c2e 100644 --- a/configure.ac +++ b/configure.ac @@ -159,6 +159,11 @@ m4_include([src/external/signal.m4]) m4_include([src/external/inotify.m4]) m4_include([src/external/libndr_nbt.m4]) m4_include([src/external/sasl.m4]) +m4_include([src/external/configlib.m4])

+if test x$build_config_lib = xyes; then

• m4_include([src/external/libaugeas.m4])

+fi

WITH_UNICODE_LIB if test x$unicode_lib = xlibunistring; then diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in index ef29970d08cc212eee58392a04f73bbaa5734d2d..7ad67acf2f29d498831ea71ef7fa0324bc2e9d5b 100644 --- a/contrib/sssd.spec.in +++ b/contrib/sssd.spec.in @@ -138,6 +138,7 @@ BuildRequires: systemd-devel %if (0%{?with_cifs_utils_plugin} == 1) BuildRequires: cifs-utils-devel %endif +BuildRequires: augeas-devel

# RHEL 5 is too old to support samba4 and the PAC responder %if !0%{?is_rhel5} @@ -574,6 +575,7 @@ rm -rf $RPM_BUILD_ROOT %{_libdir}/%{name}/libsss_debug.so %{_libdir}/%{name}/libsss_ldap_common.so %{_libdir}/%{name}/libsss_util.so +%{_libdir}/%{name}/libsss_config.so

# 3rd party application libraries %{_libdir}/sssd/modules/libsss_autofs.so diff --git a/src/external/configlib.m4 b/src/external/configlib.m4 new file mode 100644 index 0000000000000000000000000000000000000000..ad6c1a943e8d7bf0c38e9163241f0eb11792d82f --- /dev/null +++ b/src/external/configlib.m4 @@ -0,0 +1,12 @@ +AC_ARG_ENABLE([config-lib],

•          [AS_HELP_STRING([--disable-config-lib],
  

•                          [do not build internal config library])],
  

•          [build_config_lib=$enableval],
  

•          [build_config_lib=yes])
  

+AM_CONDITIONAL([BUILD_CONFIG_LIB],

•           [test x$build_config_lib = xyes])
  

+AM_COND_IF([BUILD_CONFIG_LIB],

•       [AC_DEFINE_UNQUOTED(HAVE_CONFIG_LIB, 1,
  

•        [Build with internal config library])])
  

\ No newline at end of file diff --git a/src/external/libaugeas.m4 b/src/external/libaugeas.m4 new file mode 100644 index 0000000000000000000000000000000000000000..6f8072e9ac2ad9ee968b4b935430dd73ac7e48f6 --- /dev/null +++ b/src/external/libaugeas.m4 @@ -0,0 +1,9 @@ +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")
• )

Could you change error message. It is not clear that this dependency is optional. You should write some hint that this chcek can be disabled with argument --disable-config-lib

checking for NDR_NBT... yes checking for SASL... yes checking for AUGEAS... no configure: error: "Please install augeas-devel"

-- 1.7.11.7

From 3bf71bd1f89a2888ceb23e63193f0cee2787a09d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Mon, 21 Apr 2014 19:44:52 +0200 Subject: [PATCH 3/3] sss_config: unit tests

---

Makefile.am | 18 +- src/tests/sss_config-tests.c | 806 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 823 insertions(+), 1 deletion(-) create mode 100644 src/tests/sss_config-tests.c

diff --git a/Makefile.am b/Makefile.am index bdc399e17ec2961ac5c5d4f1e42cf5fc0f09711a..47b6b8e4f2e805c489ce4793946cda15c265ce5e 100644 --- a/Makefile.am +++ b/Makefile.am @@ -149,7 +149,8 @@ if HAVE_CHECK responder_socket_access-tests \ safe-format-tests \ sbus_tests \

•    sbus_codegen_tests
  

•    sbus_codegen_tests \
  

•    sss_config-tests
  
  

if BUILD_SSH non_interactive_check_based_tests += sysdb_ssh-tests @@ -1414,6 +1415,21 @@ sbus_codegen_tests_LDADD = \ $(SSSD_LIBS) \ $(CHECK_LIBS)

+sss_config_tests_SOURCES = \

• src/tests/sss_config-tests.c \
• src/tests/common.c

+sss_config_tests_CFLAGS = \

• $(AM_CFLAGS) \
• $(CHECK_CFLAGS) \

this test do not use any augeas header files

• $(AUGEAS_CFLAGS)

+sss_config_tests_LDADD = \

• $(SSSD_LIBS) \
• $(CHECK_LIBS) \
• $(AUGEAS_LIBS) \

the same here

• $(SSSD_INTERNAL_LTLIBS) \
• libsss_config.la \
• libsss_test_common.la

if HAVE_CMOCKA

TEST_MOCK_RESP_OBJ = \ diff --git a/src/tests/sss_config-tests.c b/src/tests/sss_config-tests.c new file mode 100644 index 0000000000000000000000000000000000000000..c0c80591e1441c2c5cc7fe3b181d94eee78e5ec8 --- /dev/null +++ b/src/tests/sss_config-tests.c @@ -0,0 +1,806 @@ +/*

• Authors:

•    Pavel Březina <pbrezina@redhat.com>
  

• Copyright (C) 2014 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/.

+*/

+#include <check.h> +#include <stdio.h> +#include <talloc.h> +#include <errno.h> +#include <string.h> +#include "util/util.h" +#include "util/sss_config.h" +#include "tests/common.h" +#include "tests/common_check.h"

+#define TEST_FILE "sss_config_test.conf" +#define TEST_FILE_BACKUP TEST_FILE ".augsave"

+/* input files */

+const char *test_orig = +"[sssd]\n\ +services = nss, pam\n\ +domains = LDAP\n\ +debug_level = 0x0ff0\n\ +[domain/LDAP]\n\ +debug_level = 0x0ff0\n\ +[domain/IPA]\n\ +debug_level = 0x0ff0\n";

+const char *test_svc_one = +"[sssd]\n\ +services = nss\n\ +domains = LDAP\n\ +debug_level = 0x0ff0\n\ +[domain/LDAP]\n\ +debug_level = 0x0ff0\n\ +[domain/IPA]\n\ +debug_level = 0x0ff0\n";

+const char *test_svc_empty = +"[sssd]\n\ +services =\n\ +domains = LDAP\n\ +debug_level = 0x0ff0\n\ +[domain/LDAP]\n\ +debug_level = 0x0ff0\n\ +[domain/IPA]\n\ +debug_level = 0x0ff0\n";

+const char *test_svc_missing = +"[sssd]\n\ +domains = LDAP\n\ +debug_level = 0x0ff0\n\ +[domain/LDAP]\n\ +debug_level = 0x0ff0\n\ +[domain/IPA]\n\ +debug_level = 0x0ff0\n";

+const char *test_dom_empty = +"[sssd]\n\ +services = nss, pam\n\ +domains =\n\ +debug_level = 0x0ff0\n\ +[domain/LDAP]\n\ +debug_level = 0x0ff0\n\ +[domain/IPA]\n\ +debug_level = 0x0ff0\n";

+const char *test_dom_missing = +"[sssd]\n\ +services = nss, pam\n\ +debug_level = 0x0ff0\n\ +[domain/LDAP]\n\ +debug_level = 0x0ff0\n\ +[domain/IPA]\n\ +debug_level = 0x0ff0\n";

+const char *test_dom_two = +"[sssd]\n\ +services = nss, pam\n\ +domains = LDAP, IPA\n\ +debug_level = 0x0ff0\n\ +[domain/LDAP]\n\ +debug_level = 0x0ff0\n\ +[domain/IPA]\n\ +debug_level = 0x0ff0\n";

+/* expected */

+const char *exp_debug_level_exist = +"[sssd]\n\ +services = nss, pam\n\ +domains = LDAP\n\ +debug_level = 0x0ff0\n\ +[domain/LDAP]\n\ +debug_level = 0x0330\n\ +[domain/IPA]\n\ +debug_level = 0x0ff0\n";

+const char *exp_debug_level_notexist = +"[sssd]\n\ +services = nss, pam\n\ +domains = LDAP\n\ +debug_level = 0x0ff0\n\ +[domain/LDAP]\n\ +debug_level = 0x0ff0\n\ +[domain/IPA]\n\ +debug_level = 0x0ff0\n\ +[nss]\n\ +debug_level=0x0330\n";

+const char *exp_svc = +"[sssd]\n\ +services = nss, pam, pac\n\ +domains = LDAP\n\ +debug_level = 0x0ff0\n\ +[domain/LDAP]\n\ +debug_level = 0x0ff0\n\ +[domain/IPA]\n\ +debug_level = 0x0ff0\n";

+const char *exp_svc_empty = +"[sssd]\n\ +services =pac\n\ +domains = LDAP\n\ +debug_level = 0x0ff0\n\ +[domain/LDAP]\n\ +debug_level = 0x0ff0\n\ +[domain/IPA]\n\ +debug_level = 0x0ff0\n";

+const char *exp_svc_missing = +"[sssd]\n\ +domains = LDAP\n\ +debug_level = 0x0ff0\n\ +services=pac\n\ +[domain/LDAP]\n\ +debug_level = 0x0ff0\n\ +[domain/IPA]\n\ +debug_level = 0x0ff0\n";

+const char *exp_svc_disable = +"[sssd]\n\ +services = pam\n\ +domains = LDAP\n\ +debug_level = 0x0ff0\n\ +[domain/LDAP]\n\ +debug_level = 0x0ff0\n\ +[domain/IPA]\n\ +debug_level = 0x0ff0\n";

+const char *exp_svc_disable_one = +"[sssd]\n\ +domains = LDAP\n\ +debug_level = 0x0ff0\n\ +[domain/LDAP]\n\ +debug_level = 0x0ff0\n\ +[domain/IPA]\n\ +debug_level = 0x0ff0\n";

+const char *exp_svc_disable_empty = +"[sssd]\n\ +services =\n\ +domains = LDAP\n\ +debug_level = 0x0ff0\n\ +[domain/LDAP]\n\ +debug_level = 0x0ff0\n\ +[domain/IPA]\n\ +debug_level = 0x0ff0\n";

+const char *exp_svc_disable_missing = +"[sssd]\n\ +domains = LDAP\n\ +debug_level = 0x0ff0\n\ +[domain/LDAP]\n\ +debug_level = 0x0ff0\n\ +[domain/IPA]\n\ +debug_level = 0x0ff0\n";

+const char *exp_dom = +"[sssd]\n\ +services = nss, pam\n\ +domains = LDAP, IPA\n\ +debug_level = 0x0ff0\n\ +[domain/LDAP]\n\ +debug_level = 0x0ff0\n\ +[domain/IPA]\n\ +debug_level = 0x0ff0\n";

+const char *exp_dom_empty = +"[sssd]\n\ +services = nss, pam\n\ +domains =IPA\n\ +debug_level = 0x0ff0\n\ +[domain/LDAP]\n\ +debug_level = 0x0ff0\n\ +[domain/IPA]\n\ +debug_level = 0x0ff0\n";

+const char *exp_dom_missing = +"[sssd]\n\ +services = nss, pam\n\ +debug_level = 0x0ff0\n\ +domains=IPA\n\ +[domain/LDAP]\n\ +debug_level = 0x0ff0\n\ +[domain/IPA]\n\ +debug_level = 0x0ff0\n";

+const char *exp_dom_disable = +"[sssd]\n\ +services = nss, pam\n\ +debug_level = 0x0ff0\n\ +[domain/LDAP]\n\ +debug_level = 0x0ff0\n\ +[domain/IPA]\n\ +debug_level = 0x0ff0\n";

+const char *exp_dom_disable_two = +"[sssd]\n\ +services = nss, pam\n\ +domains = IPA\n\ +debug_level = 0x0ff0\n\ +[domain/LDAP]\n\ +debug_level = 0x0ff0\n\ +[domain/IPA]\n\ +debug_level = 0x0ff0\n";

+const char *exp_dom_disable_empty = +"[sssd]\n\ +services = nss, pam\n\ +domains =\n\ +debug_level = 0x0ff0\n\ +[domain/LDAP]\n\ +debug_level = 0x0ff0\n\ +[domain/IPA]\n\ +debug_level = 0x0ff0\n";

+struct sss_config_ctx *config_ctx;

+static bool +check_file_content(const char *filename, const char *expected) +{

• FILE *file = NULL;
• size_t i;
• char c;
• bool result;
• file = fopen(filename, "r");
• fail_if(file == NULL, "unable to open test file");
• i = 0;
• while ((c = fgetc(file)) != EOF) {

•    if (c != expected[i]) {
  

•        printf("\nnot match: %d %c == %d %c\n", c, c, expected[i], expected[i]);
  

•        result = false;
  

•        goto done;
  

•    }
  

•    i++;
  

• }
• if (expected[i] != '\0') {

•    printf("\nnot end: %d %c == %d %c\n", c, c, expected[i], expected[i]);
  

•    result = false;
  

•    goto done;
  

• }
• result = true;

+done:

• fclose(file);
• return result;

+}

+static void test_setup(const char *configuration) +{

• FILE *file = NULL;
• size_t ret;
• file = fopen(TEST_FILE, "w+");
• fail_if(file == NULL, "unable to create test file");
• ret = fputs(configuration, file);
• fail_if(ret == EOF, "unable to write test file");
• fail_if(fclose(file) != 0, "unable to close test file");
• config_ctx = sss_config_open(NULL, TEST_DIR, "sss_config_test.conf");

^^^^^^^^^^^^^^^^^^^^^^ Could you use defined macro here? It think you should call function sss_config_open in main function and here you should use some subdirectory (test_sss_config?)

• fail_if(config_ctx == NULL, "config_ctx is NULL");

+}

+static void setup(void) +{

• ck_leak_check_setup();

+}

+static void teardown(void) +{

• sss_config_close(&config_ctx);
• fail_if(config_ctx != NULL, "config_ctx is not NULL");
• //unlink(TEST_FILE);

Why is this line commented out?

• unlink(TEST_FILE_BACKUP);
• ck_leak_check_teardown();

+}

+START_TEST(test_sss_config_set_debug_level_exist) +{

• errno_t ret;
• bool result;
• test_setup(test_orig);
• ret = sss_config_set_debug_level(config_ctx, "domain/LDAP", 0x0330);
• fail_if(ret != EOK, "unable change configuration");
• ret = sss_config_save(config_ctx);
• fail_if(ret != EOK, "unable save configuration");
• result = check_file_content(TEST_FILE, exp_debug_level_exist);
• fail_if(result == false, "file does not match");
• result = check_file_content(TEST_FILE_BACKUP, test_orig);
• fail_if(result == false, "backup file does not match");

+} +END_TEST

+START_TEST(test_sss_config_set_debug_level_notexist) +{

• errno_t ret;
• bool result;
• test_setup(test_orig);
• ret = sss_config_set_debug_level(config_ctx, "nss", 0x0330);
• fail_if(ret != EOK, "unable change configuration [%d]: %s",

•                    ret, strerror(ret));
  

• ret = sss_config_save(config_ctx);
• fail_if(ret != EOK, "unable save configuration [%d]: %s",

•                    ret, strerror(ret));
  

• result = check_file_content(TEST_FILE, exp_debug_level_notexist);
• fail_if(result == false, "file does not match");
• result = check_file_content(TEST_FILE_BACKUP, test_orig);
• fail_if(result == false, "backup file does not match");

+} +END_TEST

+START_TEST(test_sss_config_service_enabled) +{

• errno_t ret;
• bool result;
• test_setup(test_orig);
• ret = sss_config_service_is_enabled(config_ctx, "nss", &result);
• fail_if(ret != EOK, "unable to read configuration [%d]: %s",

•                    ret, strerror(ret));
  

• fail_if(result == false, "wrong result");

+} +END_TEST

+START_TEST(test_sss_config_service_disabled) +{

• errno_t ret;
• bool result;
• test_setup(test_orig);
• ret = sss_config_service_is_enabled(config_ctx, "pac", &result);
• fail_if(ret != EOK, "unable to read configuration [%d]: %s",

•                    ret, strerror(ret));
  

• fail_if(result == true, "wrong result");

+} +END_TEST

+START_TEST(test_sss_config_service_disabled_empty) +{

• errno_t ret;
• bool result;
• test_setup(test_svc_empty);
• ret = sss_config_service_is_enabled(config_ctx, "pac", &result);
• fail_if(ret != EOK, "unable to read configuration [%d]: %s",

•                    ret, strerror(ret));
  

• fail_if(result == true, "wrong result");

+} +END_TEST

+START_TEST(test_sss_config_service_disabled_missing) +{

• errno_t ret;
• bool result;
• test_setup(test_svc_missing);
• ret = sss_config_service_is_enabled(config_ctx, "pac", &result);
• fail_if(ret != EOK, "unable to read configuration [%d]: %s",

•                    ret, strerror(ret));
  

• fail_if(result == true, "wrong result");

+} +END_TEST

+START_TEST(test_sss_config_service_enable) +{

• errno_t ret;
• bool result;
• test_setup(test_orig);
• ret = sss_config_service_enable(config_ctx, "pac");
• fail_if(ret != EOK, "unable change configuration [%d]: %s",

•                    ret, strerror(ret));
  

• ret = sss_config_save(config_ctx);
• fail_if(ret != EOK, "unable save configuration [%d]: %s",

•                    ret, strerror(ret));
  

• result = check_file_content(TEST_FILE, exp_svc);
• fail_if(result == false, "file does not match");
• result = check_file_content(TEST_FILE_BACKUP, test_orig);
• fail_if(result == false, "backup file does not match");

+} +END_TEST

+START_TEST(test_sss_config_service_enable_empty) +{

• errno_t ret;
• bool result;
• test_setup(test_svc_empty);
• ret = sss_config_service_enable(config_ctx, "pac");
• fail_if(ret != EOK, "unable change configuration [%d]: %s",

•                    ret, strerror(ret));
  

• ret = sss_config_save(config_ctx);
• fail_if(ret != EOK, "unable save configuration [%d]: %s",

•                    ret, strerror(ret));
  

• result = check_file_content(TEST_FILE, exp_svc_empty);
• fail_if(result == false, "file does not match");
• result = check_file_content(TEST_FILE_BACKUP, test_svc_empty);
• fail_if(result == false, "backup file does not match");

+} +END_TEST

+START_TEST(test_sss_config_service_enable_missing) +{

• errno_t ret;
• bool result;
• test_setup(test_svc_missing);
• ret = sss_config_service_enable(config_ctx, "pac");
• fail_if(ret != EOK, "unable change configuration [%d]: %s",

•                    ret, strerror(ret));
  

• ret = sss_config_save(config_ctx);
• fail_if(ret != EOK, "unable save configuration [%d]: %s",

•                    ret, strerror(ret));
  

• result = check_file_content(TEST_FILE, exp_svc_missing);
• fail_if(result == false, "file does not match");
• result = check_file_content(TEST_FILE_BACKUP, test_svc_missing);
• fail_if(result == false, "backup file does not match");

+} +END_TEST

+START_TEST(test_sss_config_service_disable) +{

• errno_t ret;
• bool result;
• test_setup(test_orig);
• ret = sss_config_service_disable(config_ctx, "nss");
• fail_if(ret != EOK, "unable change configuration [%d]: %s",

•                    ret, strerror(ret));
  

• ret = sss_config_save(config_ctx);
• fail_if(ret != EOK, "unable save configuration [%d]: %s",

•                    ret, strerror(ret));
  

• result = check_file_content(TEST_FILE, exp_svc_disable);
• fail_if(result == false, "file does not match");
• result = check_file_content(TEST_FILE_BACKUP, test_orig);
• fail_if(result == false, "backup file does not match");

+} +END_TEST

+START_TEST(test_sss_config_service_disable_one) +{

• return;

Why is this test disabled? It passed on my machine.

• errno_t ret;
• bool result;
• test_setup(test_svc_one);
• ret = sss_config_service_disable(config_ctx, "nss");
• fail_if(ret != EOK, "unable change configuration [%d]: %s",

•                    ret, strerror(ret));
  

• ret = sss_config_save(config_ctx);
• fail_if(ret != EOK, "unable save configuration [%d]: %s",

•                    ret, strerror(ret));
  

• result = check_file_content(TEST_FILE, exp_svc_disable_one);
• fail_if(result == false, "file does not match");
• result = check_file_content(TEST_FILE_BACKUP, test_svc_one);
• fail_if(result == false, "backup file does not match");

+} +END_TEST

+START_TEST(test_sss_config_service_disable_empty) +{

• errno_t ret;
• bool result;
• test_setup(test_svc_empty);
• ret = sss_config_service_disable(config_ctx, "nss");
• fail_if(ret != EOK, "unable change configuration [%d]: %s",

•                    ret, strerror(ret));
  

• ret = sss_config_save(config_ctx);
• fail_if(ret != EOK, "unable save configuration [%d]: %s",

•                    ret, strerror(ret));
  

• result = check_file_content(TEST_FILE, exp_svc_disable_empty);
• fail_if(result == false, "file does not match");
• /* no backup file created */

+} +END_TEST

+START_TEST(test_sss_config_service_disable_missing) +{

• errno_t ret;
• bool result;
• test_setup(test_svc_missing);
• ret = sss_config_service_disable(config_ctx, "nss");
• fail_if(ret != EOK, "unable change configuration [%d]: %s",

•                    ret, strerror(ret));
  

• ret = sss_config_save(config_ctx);
• fail_if(ret != EOK, "unable save configuration [%d]: %s",

•                    ret, strerror(ret));
  

• result = check_file_content(TEST_FILE, exp_svc_disable_missing);
• fail_if(result == false, "file does not match");
• /* no backup file created */

+} +END_TEST

+START_TEST(test_sss_config_domain_enable) +{

• errno_t ret;
• bool result;
• test_setup(test_orig);
• ret = sss_config_domain_enable(config_ctx, "IPA");
• fail_if(ret != EOK, "unable change configuration [%d]: %s",

•                    ret, strerror(ret));
  

• ret = sss_config_save(config_ctx);
• fail_if(ret != EOK, "unable save configuration [%d]: %s",

•                    ret, strerror(ret));
  

• result = check_file_content(TEST_FILE, exp_dom);
• fail_if(result == false, "file does not match");
• result = check_file_content(TEST_FILE_BACKUP, test_orig);
• fail_if(result == false, "backup file does not match");

+} +END_TEST

+START_TEST(test_sss_config_domain_enable_empty) +{

• errno_t ret;
• bool result;
• test_setup(test_dom_empty);
• ret = sss_config_domain_enable(config_ctx, "IPA");
• fail_if(ret != EOK, "unable change configuration [%d]: %s",

•                    ret, strerror(ret));
  

• ret = sss_config_save(config_ctx);
• fail_if(ret != EOK, "unable save configuration [%d]: %s",

•                    ret, strerror(ret));
  

• result = check_file_content(TEST_FILE, exp_dom_empty);
• fail_if(result == false, "file does not match");
• result = check_file_content(TEST_FILE_BACKUP, test_dom_empty);
• fail_if(result == false, "backup file does not match");

+} +END_TEST

+START_TEST(test_sss_config_domain_enable_missing) +{

• errno_t ret;
• bool result;
• test_setup(test_dom_missing);
• ret = sss_config_domain_enable(config_ctx, "IPA");
• fail_if(ret != EOK, "unable change configuration [%d]: %s",

•                    ret, strerror(ret));
  

• ret = sss_config_save(config_ctx);
• fail_if(ret != EOK, "unable save configuration [%d]: %s",

•                    ret, strerror(ret));
  

• result = check_file_content(TEST_FILE, exp_dom_missing);
• fail_if(result == false, "file does not match");
• result = check_file_content(TEST_FILE_BACKUP, test_dom_missing);
• fail_if(result == false, "backup file does not match");

+} +END_TEST

+START_TEST(test_sss_config_domain_disable) +{

• errno_t ret;
• bool result;
• test_setup(test_orig);
• ret = sss_config_domain_disable(config_ctx, "LDAP");
• fail_if(ret != EOK, "unable change configuration [%d]: %s",

•                    ret, strerror(ret));
  

• ret = sss_config_save(config_ctx);
• fail_if(ret != EOK, "unable save configuration [%d]: %s",

•                    ret, strerror(ret));
  

• result = check_file_content(TEST_FILE, exp_dom_disable);
• fail_if(result == false, "file does not match");
• result = check_file_content(TEST_FILE_BACKUP, test_orig);
• fail_if(result == false, "backup file does not match");

+} +END_TEST

+START_TEST(test_sss_config_domain_disable_two) +{

• return;

The same here.

• errno_t ret;
• bool result;
• test_setup(test_dom_two);
• ret = sss_config_domain_disable(config_ctx, "LDAP");
• fail_if(ret != EOK, "unable change configuration [%d]: %s",

•                    ret, strerror(ret));
  

• ret = sss_config_save(config_ctx);
• fail_if(ret != EOK, "unable save configuration [%d]: %s",

•                    ret, strerror(ret));
  

• result = check_file_content(TEST_FILE, exp_dom_disable_two);
• fail_if(result == false, "file does not match");
• result = check_file_content(TEST_FILE_BACKUP, test_dom_two);
• fail_if(result == false, "backup file does not match");

+} +END_TEST

+START_TEST(test_sss_config_domain_disable_empty) +{

• errno_t ret;
• bool result;
• test_setup(test_dom_empty);
• ret = sss_config_domain_disable(config_ctx, "LDAP");
• fail_if(ret != EOK, "unable change configuration [%d]: %s",

•                    ret, strerror(ret));
  

• ret = sss_config_save(config_ctx);
• fail_if(ret != EOK, "unable save configuration [%d]: %s",

•                    ret, strerror(ret));
  

• result = check_file_content(TEST_FILE, exp_dom_disable_empty);
• fail_if(result == false, "file does not match");
• /* no backup file created */

+} +END_TEST

+START_TEST(test_sss_config_domain_disable_missing) +{

• errno_t ret;
• bool result;
• test_setup(test_dom_missing);
• ret = sss_config_domain_disable(config_ctx, "LDAP");
• fail_if(ret != EOK, "unable change configuration [%d]: %s",

•                    ret, strerror(ret));
  

• ret = sss_config_save(config_ctx);
• fail_if(ret != EOK, "unable save configuration [%d]: %s",

•                    ret, strerror(ret));
  

• result = check_file_content(TEST_FILE, exp_dom_disable);
• fail_if(result == false, "file does not match");
• /* no backup file created */

+} +END_TEST

+Suite *sss_config_suite(void) +{

• Suite *s = suite_create("sss_config");
• TCase *tc = tcase_create("sss_config");
• tcase_add_checked_fixture(tc, setup, teardown);
• tcase_add_test(tc, test_sss_config_set_debug_level_exist);
• tcase_add_test(tc, test_sss_config_set_debug_level_notexist);
• tcase_add_test(tc, test_sss_config_service_enabled);
• tcase_add_test(tc, test_sss_config_service_disabled);
• tcase_add_test(tc, test_sss_config_service_disabled_empty);
• tcase_add_test(tc, test_sss_config_service_disabled_missing);
• tcase_add_test(tc, test_sss_config_service_enable);
• tcase_add_test(tc, test_sss_config_service_enable_empty);
• tcase_add_test(tc, test_sss_config_service_enable_missing);
• tcase_add_test(tc, test_sss_config_service_disable);
• tcase_add_test(tc, test_sss_config_service_disable_one);
• tcase_add_test(tc, test_sss_config_service_disable_empty);
• tcase_add_test(tc, test_sss_config_service_disable_missing);
• tcase_add_test(tc, test_sss_config_domain_enable);
• tcase_add_test(tc, test_sss_config_domain_enable_empty);
• tcase_add_test(tc, test_sss_config_domain_enable_missing);
• tcase_add_test(tc, test_sss_config_domain_disable);
• tcase_add_test(tc, test_sss_config_domain_disable_two);
• tcase_add_test(tc, test_sss_config_domain_disable_empty);
• tcase_add_test(tc, test_sss_config_domain_disable_missing);
• tcase_set_timeout(tc, 60);
• suite_add_tcase(s, tc);
• return s;

+}

+int main(int argc, const char *argv[]) +{

• int number_failed;
• tests_set_cwd();
• Suite *s = sss_config_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.7.11.7

Could you also add simple test for function sss_config_domain_is_enabled. It will be very simillar to the test test_sss_config_service_disabled

Add line to dlopen test with some if condition HAVE_CONFIG_LIB + { "libsss_config.so", { LIBPFX"libsss_config.so", NULL } },

LS

On (05/05/14 11:36), Pavel Březina wrote:

Hi, thank you for review. Can you please remove unneeded quotation next time?

sure :-)

...

...

+static errno_t +sss_config_set_list(struct sss_config_ctx *ctx,

•                const char *section,
  

•                const char *option,
  

•                char **list)
  

+{

• TALLOC_CTX *tmp_ctx = NULL;
• char *value = NULL;
• errno_t ret;
• int i;
• if (list == NULL) {

•    return EOK;
  

Could you explain why is EOK returned?

Right, I should return EINVAL here.

...

...

+if BUILD_CONFIG_LIB +pkglib_LTLIBRARIES += libsss_config.la +libsss_config_la_SOURCES = \

• src/util/sss_config.c

+libsss_config_la_CFLAGS = \

• $(AM_CFLAGS) \
• $(AUGEAS_CFLAGS)

+libsss_config_la_LIBADD = \

• $(AUGEAS_LIBS) \
• $(SSSD_INTERNAL_LTLIBS)

This library use talloc, it would be nice to link with talloc directly and do not rely on other libraries (libsss_util.so)

Done.

...

...

+++ b/src/external/libaugeas.m4 @@ -0,0 +1,9 @@ +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")
• )

Could you change error message. It is not clear that this dependency is optional. You should write some hint that this chcek can be disabled with argument --disable-config-lib

Done.

...

...

+sss_config_tests_SOURCES = \

• src/tests/sss_config-tests.c \
• src/tests/common.c

+sss_config_tests_CFLAGS = \

• $(AM_CFLAGS) \
• $(CHECK_CFLAGS) \

this test do not use any augeas header files

Done.

...

...

• $(AUGEAS_CFLAGS)

+sss_config_tests_LDADD = \

• $(SSSD_LIBS) \
• $(CHECK_LIBS) \
• $(AUGEAS_LIBS) \

the same here

Done.

...

...

• $(SSSD_INTERNAL_LTLIBS) \
• libsss_config.la \
• libsss_test_common.la

+static void test_setup(const char *configuration) +{

• FILE *file = NULL;
• size_t ret;
• file = fopen(TEST_FILE, "w+");
• fail_if(file == NULL, "unable to create test file");
• ret = fputs(configuration, file);
• fail_if(ret == EOF, "unable to write test file");
• fail_if(fclose(file) != 0, "unable to close test file");
• config_ctx = sss_config_open(NULL, TEST_DIR, "sss_config_test.conf");

                                               ^^^^^^^^^^^^^^^^^^^^^^
                                        Could you use defined macro here?

Done.

...

It think you should call function sss_config_open in main function and here you

No, the context has to be new for each test.

...

should use some subdirectory (test_sss_config?)

Done.

...

...

• fail_if(config_ctx == NULL, "config_ctx is NULL");

+}

+static void setup(void) +{

• ck_leak_check_setup();

+}

+static void teardown(void) +{

• sss_config_close(&config_ctx);
• fail_if(config_ctx != NULL, "config_ctx is not NULL");
• //unlink(TEST_FILE);

Why is this line commented out?

Damn, I forgot to remove some debug changes. The same applies for the disabled tests.

Fixed.

...

...

+START_TEST(test_sss_config_service_disable_one) +{

• return;

Why is this test disabled? It passed on my machine.

Fixed.

...

...

+START_TEST(test_sss_config_domain_disable_two) +{

• return;

The same here.

Fixed.

...

Could you also add simple test for function sss_config_domain_is_enabled. It will be very simillar to the test test_sss_config_service_disabled

Done.

...

Add line to dlopen test with some if condition HAVE_CONFIG_LIB

• { "libsss_config.so", { LIBPFX"libsss_config.so", NULL } },

LS

I also make the test build conditional.

New patches are attached, together with diff for easier review.

From 4c79b451b09646c3f8d691a3836893d09102dd16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Fri, 18 Apr 2014 17:23:16 +0200 Subject: [PATCH 1/3] sss_config: the code

ACK

From 7dd94ea81eea1782e38e3b4964f10c220a3a109d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Mon, 21 Apr 2014 12:17:53 +0200 Subject: [PATCH 2/3] sss_config: build

---

diff --git a/Makefile.am b/Makefile.am index 56d8e1df648822a1fe8b3ca5fc8995f3c78a8566..536a1dc5f52f8d720e923eb27e3bd6fcf22bfa87 100644 --- a/Makefile.am +++ b/Makefile.am @@ -651,6 +652,22 @@ SSSD_INTERNAL_LTLIBS = \ libsss_debug.la \ libsss_child.la

+if BUILD_CONFIG_LIB +pkglib_LTLIBRARIES += libsss_config.la +libsss_config_la_SOURCES = \

• src/util/sss_config.c

+libsss_config_la_CFLAGS = \

• $(AM_CFLAGS) \
• $(AUGEAS_CFLAGS) \
• $(TALLOC_CFLAGS)

+libsss_config_la_LIBADD = \

• $(AUGEAS_LIBS) \
• $(TALLOC_LIBS) \
• $(SSSD_INTERNAL_LTLIBS)

+libsss_config_la_LDFLAGS = \

• avoid-version

ACK with small change - avoid-version + -avoid-version

I noticed this when I tried to build rpms for static analysis. I should have catched it earlier, but it can be changed before pushing patches to master.

RPM build errors: error: Installed (but unpackaged) file(s) found: /usr/lib64/sssd/libsss_config.so.0 /usr/lib64/sssd/libsss_config.so.0.0.0

From 1f0207a2ea5503b8e592d8b698e4a50da11a076a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Mon, 21 Apr 2014 19:44:52 +0200 Subject: [PATCH 3/3] sss_config: unit tests

ACK

There are not any warnigs from static analysers.

LS

On (12/05/14 11:16), Pavel Březina wrote:

On 05/05/2014 02:21 PM, Lukas Slebodnik wrote:

...

On (05/05/14 11:36), Pavel Březina wrote:

...

Hi, thank you for review. Can you please remove unneeded quotation next time?

sure :-)

...

...

...

+static errno_t +sss_config_set_list(struct sss_config_ctx *ctx,

•                const char *section,
  

•                const char *option,
  

•                char **list)
  

+{

• TALLOC_CTX *tmp_ctx = NULL;
• char *value = NULL;
• errno_t ret;
• int i;
• if (list == NULL) {

•    return EOK;
  

Could you explain why is EOK returned?

Right, I should return EINVAL here.

...

...

+if BUILD_CONFIG_LIB +pkglib_LTLIBRARIES += libsss_config.la +libsss_config_la_SOURCES = \

• src/util/sss_config.c

+libsss_config_la_CFLAGS = \

• $(AM_CFLAGS) \
• $(AUGEAS_CFLAGS)

+libsss_config_la_LIBADD = \

• $(AUGEAS_LIBS) \
• $(SSSD_INTERNAL_LTLIBS)

This library use talloc, it would be nice to link with talloc directly and do not rely on other libraries (libsss_util.so)

Done.

...

...

+++ b/src/external/libaugeas.m4 @@ -0,0 +1,9 @@ +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")
• )

Could you change error message. It is not clear that this dependency is optional. You should write some hint that this chcek can be disabled with argument --disable-config-lib

Done.

...

...

+sss_config_tests_SOURCES = \

• src/tests/sss_config-tests.c \
• src/tests/common.c

+sss_config_tests_CFLAGS = \

• $(AM_CFLAGS) \
• $(CHECK_CFLAGS) \

this test do not use any augeas header files

Done.

...

...

• $(AUGEAS_CFLAGS)

+sss_config_tests_LDADD = \

• $(SSSD_LIBS) \
• $(CHECK_LIBS) \
• $(AUGEAS_LIBS) \

the same here

Done.

...

...

• $(SSSD_INTERNAL_LTLIBS) \
• libsss_config.la \
• libsss_test_common.la

+static void test_setup(const char *configuration) +{

• FILE *file = NULL;
• size_t ret;
• file = fopen(TEST_FILE, "w+");
• fail_if(file == NULL, "unable to create test file");
• ret = fputs(configuration, file);
• fail_if(ret == EOF, "unable to write test file");
• fail_if(fclose(file) != 0, "unable to close test file");
• config_ctx = sss_config_open(NULL, TEST_DIR, "sss_config_test.conf");

                                               ^^^^^^^^^^^^^^^^^^^^^^
                                        Could you use defined macro here?

Done.

...

It think you should call function sss_config_open in main function and here you

No, the context has to be new for each test.

...

should use some subdirectory (test_sss_config?)

Done.

...

...

• fail_if(config_ctx == NULL, "config_ctx is NULL");

+}

+static void setup(void) +{

• ck_leak_check_setup();

+}

+static void teardown(void) +{

• sss_config_close(&config_ctx);
• fail_if(config_ctx != NULL, "config_ctx is not NULL");
• //unlink(TEST_FILE);

Why is this line commented out?

Damn, I forgot to remove some debug changes. The same applies for the disabled tests.

Fixed.

...

...

+START_TEST(test_sss_config_service_disable_one) +{

• return;

Why is this test disabled? It passed on my machine.

Fixed.

...

...

+START_TEST(test_sss_config_domain_disable_two) +{

• return;

The same here.

Fixed.

...

Could you also add simple test for function sss_config_domain_is_enabled. It will be very simillar to the test test_sss_config_service_disabled

Done.

...

Add line to dlopen test with some if condition HAVE_CONFIG_LIB

• { "libsss_config.so", { LIBPFX"libsss_config.so", NULL } },

LS

I also make the test build conditional.

New patches are attached, together with diff for easier review.

...

From 4c79b451b09646c3f8d691a3836893d09102dd16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Fri, 18 Apr 2014 17:23:16 +0200 Subject: [PATCH 1/3] sss_config: the code

ACK

...

From 7dd94ea81eea1782e38e3b4964f10c220a3a109d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Mon, 21 Apr 2014 12:17:53 +0200 Subject: [PATCH 2/3] sss_config: build

---

diff --git a/Makefile.am b/Makefile.am index 56d8e1df648822a1fe8b3ca5fc8995f3c78a8566..536a1dc5f52f8d720e923eb27e3bd6fcf22bfa87 100644 --- a/Makefile.am +++ b/Makefile.am @@ -651,6 +652,22 @@ SSSD_INTERNAL_LTLIBS = \ libsss_debug.la \ libsss_child.la

+if BUILD_CONFIG_LIB +pkglib_LTLIBRARIES += libsss_config.la +libsss_config_la_SOURCES = \

• src/util/sss_config.c

+libsss_config_la_CFLAGS = \

• $(AM_CFLAGS) \
• $(AUGEAS_CFLAGS) \
• $(TALLOC_CFLAGS)

+libsss_config_la_LIBADD = \

• $(AUGEAS_LIBS) \
• $(TALLOC_LIBS) \
• $(SSSD_INTERNAL_LTLIBS)

+libsss_config_la_LDFLAGS = \

• avoid-version

ACK with small change

• avoid-version

• -avoid-version

I noticed this when I tried to build rpms for static analysis. I should have catched it earlier, but it can be changed before pushing patches to master.

RPM build errors: error: Installed (but unpackaged) file(s) found: /usr/lib64/sssd/libsss_config.so.0 /usr/lib64/sssd/libsss_config.so.0.0.0

...

From 1f0207a2ea5503b8e592d8b698e4a50da11a076a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Mon, 21 Apr 2014 19:44:52 +0200 Subject: [PATCH 3/3] sss_config: unit tests

ACK

There are not any warnigs from static analysers.

LS

Hi, thank you for the review, the typo is fixed in this patch set.

I have already ACK-ed these patches. (Sorry that I didn't catch problem with avoid-version earlier). On more time.

ACK

LS

On (13/05/14 23:01), Pavel Březina wrote:

On 05/13/2014 07:45 AM, Lukas Slebodnik wrote:

...

On (12/05/14 11:16), Pavel Březina wrote:

...

On 05/05/2014 02:21 PM, Lukas Slebodnik wrote:

...

On (05/05/14 11:36), Pavel Březina wrote:

...

Hi, thank you for review. Can you please remove unneeded quotation next time?

sure :-)

...

...

>+ >+static errno_t >+sss_config_set_list(struct sss_config_ctx *ctx, >+ const char *section, >+ const char *option, >+ char **list) >+{ >+ TALLOC_CTX *tmp_ctx = NULL; >+ char *value = NULL; >+ errno_t ret; >+ int i; >+ >+ if (list == NULL) { >+ return EOK; Could you explain why is EOK returned?

Right, I should return EINVAL here.

...

>+if BUILD_CONFIG_LIB >+pkglib_LTLIBRARIES += libsss_config.la >+libsss_config_la_SOURCES = \ >+ src/util/sss_config.c >+libsss_config_la_CFLAGS = \ >+ $(AM_CFLAGS) \ >+ $(AUGEAS_CFLAGS) >+libsss_config_la_LIBADD = \ >+ $(AUGEAS_LIBS) \ >+ $(SSSD_INTERNAL_LTLIBS) This library use talloc, it would be nice to link with talloc directly and do not rely on other libraries (libsss_util.so)

Done.

...

>+++ b/src/external/libaugeas.m4 >@@ -0,0 +1,9 @@ >+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") >+ ) Could you change error message. It is not clear that this dependency is optional. You should write some hint that this chcek can be disabled with argument --disable-config-lib

Done.

...

>+sss_config_tests_SOURCES = \ >+ src/tests/sss_config-tests.c \ >+ src/tests/common.c >+sss_config_tests_CFLAGS = \ >+ $(AM_CFLAGS) \ >+ $(CHECK_CFLAGS) \ this test do not use any augeas header files

Done.

...

>+ $(AUGEAS_CFLAGS) >+sss_config_tests_LDADD = \ >+ $(SSSD_LIBS) \ >+ $(CHECK_LIBS) \ >+ $(AUGEAS_LIBS) \ the same here

Done.

...

>+ $(SSSD_INTERNAL_LTLIBS) \ >+ libsss_config.la \ >+ libsss_test_common.la >+ >+static void test_setup(const char *configuration) >+{ >+ FILE *file = NULL; >+ size_t ret; >+ >+ file = fopen(TEST_FILE, "w+"); >+ fail_if(file == NULL, "unable to create test file"); >+ >+ ret = fputs(configuration, file); >+ fail_if(ret == EOF, "unable to write test file"); >+ >+ fail_if(fclose(file) != 0, "unable to close test file"); >+ >+ config_ctx = sss_config_open(NULL, TEST_DIR, "sss_config_test.conf"); ^^^^^^^^^^^^^^^^^^^^^^ Could you use defined macro here?

Done.

...

It think you should call function sss_config_open in main function and here you

No, the context has to be new for each test.

...

should use some subdirectory (test_sss_config?)

Done.

...

>+ fail_if(config_ctx == NULL, "config_ctx is NULL"); >+} >+ >+static void setup(void) >+{ >+ ck_leak_check_setup(); >+} >+ >+static void teardown(void) >+{ >+ sss_config_close(&config_ctx); >+ fail_if(config_ctx != NULL, "config_ctx is not NULL"); >+ >+ //unlink(TEST_FILE); Why is this line commented out?

Damn, I forgot to remove some debug changes. The same applies for the disabled tests.

Fixed.

...

>+ >+START_TEST(test_sss_config_service_disable_one) >+{ >+ return; Why is this test disabled? It passed on my machine.

Fixed.

...

>+ >+START_TEST(test_sss_config_domain_disable_two) >+{ >+ return; The same here.

Fixed.

...

Could you also add simple test for function sss_config_domain_is_enabled. It will be very simillar to the test test_sss_config_service_disabled

Done.

...

Add line to dlopen test with some if condition HAVE_CONFIG_LIB

• { "libsss_config.so", { LIBPFX"libsss_config.so", NULL } },

LS

I also make the test build conditional.

New patches are attached, together with diff for easier review.

...

From 4c79b451b09646c3f8d691a3836893d09102dd16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Fri, 18 Apr 2014 17:23:16 +0200 Subject: [PATCH 1/3] sss_config: the code

ACK

...

From 7dd94ea81eea1782e38e3b4964f10c220a3a109d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Mon, 21 Apr 2014 12:17:53 +0200 Subject: [PATCH 2/3] sss_config: build

---

diff --git a/Makefile.am b/Makefile.am index 56d8e1df648822a1fe8b3ca5fc8995f3c78a8566..536a1dc5f52f8d720e923eb27e3bd6fcf22bfa87 100644 --- a/Makefile.am +++ b/Makefile.am @@ -651,6 +652,22 @@ SSSD_INTERNAL_LTLIBS = \ libsss_debug.la \ libsss_child.la

+if BUILD_CONFIG_LIB +pkglib_LTLIBRARIES += libsss_config.la +libsss_config_la_SOURCES = \

• src/util/sss_config.c

+libsss_config_la_CFLAGS = \

• $(AM_CFLAGS) \
• $(AUGEAS_CFLAGS) \
• $(TALLOC_CFLAGS)

+libsss_config_la_LIBADD = \

• $(AUGEAS_LIBS) \
• $(TALLOC_LIBS) \
• $(SSSD_INTERNAL_LTLIBS)

+libsss_config_la_LDFLAGS = \

• avoid-version

ACK with small change

• avoid-version

• -avoid-version

I noticed this when I tried to build rpms for static analysis. I should have catched it earlier, but it can be changed before pushing patches to master.

RPM build errors: error: Installed (but unpackaged) file(s) found: /usr/lib64/sssd/libsss_config.so.0 /usr/lib64/sssd/libsss_config.so.0.0.0

...

From 1f0207a2ea5503b8e592d8b698e4a50da11a076a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Mon, 21 Apr 2014 19:44:52 +0200 Subject: [PATCH 3/3] sss_config: unit tests

ACK

There are not any warnigs from static analysers.

LS

Hi, thank you for the review, the typo is fixed in this patch set.

I have already ACK-ed these patches. (Sorry that I didn't catch problem with avoid-version earlier). On more time.

ACK

LS

I'm sending one more patch set after a discussion with Jakub. The last patch moves the lib into the ifp package and builds it by default only if the IFP responder is built.

From 4dbfc32d1d5ea08e7675e72debd1fb9d4caed7e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Tue, 13 May 2014 23:00:01 +0200 Subject: [PATCH 4/4] sss_config: build only when IFP is allowed

since the IFP responder is currently the only planned consumer.

Makefile.am | 12 +++++++++--- contrib/sssd.spec.in | 5 +++-- 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in index ad2a1cddab2b07af91d5fb9e3c72cdca90352640..2cb436875fd473e6766c0ea7304aa87f95a965b1 100644 --- a/contrib/sssd.spec.in +++ b/contrib/sssd.spec.in @@ -138,7 +138,6 @@ BuildRequires: systemd-devel %if (0%{?with_cifs_utils_plugin} == 1) BuildRequires: cifs-utils-devel %endif -BuildRequires: augeas-devel

# RHEL 5 is too old to support samba4 and the PAC responder %if !0%{?is_rhel5} @@ -398,7 +397,9 @@ be used by Python applications. Summary: The D-Bus responder of the SSSD Group: Applications/System License: GPLv3+ +BuildRequires: augeas-devel Requires: sssd-common = %{version}-%{release} +Requires: augeas-libs

It is not necessary, but it does not hurt :-) rpm can detect libuaugeas without it. I am fine with adding dependency explicitly

sh$ rpm -q -p sssd-common-1.11.90-0.fc20.x86_64.rpm --requires | grep aug sh$ rpm -q -p sssd-dbus-1.11.90-0.fc20.x86_64.rpm --requires | grep aug libaugeas.so.0()(64bit) libaugeas.so.0(AUGEAS_0.1.0)(64bit) libaugeas.so.0(AUGEAS_0.8.0)(64bit)

%description dbus Provides the D-Bus responder of the SSSD, called the InfoPipe, that allows @@ -574,7 +575,6 @@ rm -rf $RPM_BUILD_ROOT %{_libdir}/%{name}/libsss_debug.so %{_libdir}/%{name}/libsss_ldap_common.so %{_libdir}/%{name}/libsss_util.so -%{_libdir}/%{name}/libsss_config.so

# 3rd party application libraries %{_libdir}/sssd/modules/libsss_autofs.so @@ -670,6 +670,7 @@ rm -rf $RPM_BUILD_ROOT %{_mandir}/man5/sssd-ifp.5* # InfoPipe DBus plumbing %{_sysconfdir}/dbus-1/system.d/org.freedesktop.sssd.infopipe.conf +%{_libdir}/%{name}/libsss_config.so

%files client -f sssd_client.lang %defattr(-,root,root,-) -- 1.7.11.7

I think Jakub's comments are addresed.

ACK

LS

On 05/14/2014 10:48 AM, Jakub Hrozek wrote:

On Wed, May 14, 2014 at 09:42:53AM +0200, Lukas Slebodnik wrote:

...

...

@@ -398,7 +397,9 @@ be used by Python applications. Summary: The D-Bus responder of the SSSD Group: Applications/System License: GPLv3+ +BuildRequires: augeas-devel Requires: sssd-common = %{version}-%{release} +Requires: augeas-libs

It is not necessary, but it does not hurt :-) rpm can detect libuaugeas without it. I am fine with adding dependency explicitly

sh$ rpm -q -p sssd-common-1.11.90-0.fc20.x86_64.rpm --requires | grep aug sh$ rpm -q -p sssd-dbus-1.11.90-0.fc20.x86_64.rpm --requires | grep aug libaugeas.so.0()(64bit) libaugeas.so.0(AUGEAS_0.1.0)(64bit) libaugeas.so.0(AUGEAS_0.8.0)(64bit)

...

%description dbus Provides the D-Bus responder of the SSSD, called the InfoPipe, that allows @@ -574,7 +575,6 @@ rm -rf $RPM_BUILD_ROOT %{_libdir}/%{name}/libsss_debug.so %{_libdir}/%{name}/libsss_ldap_common.so %{_libdir}/%{name}/libsss_util.so -%{_libdir}/%{name}/libsss_config.so

# 3rd party application libraries %{_libdir}/sssd/modules/libsss_autofs.so @@ -670,6 +670,7 @@ rm -rf $RPM_BUILD_ROOT %{_mandir}/man5/sssd-ifp.5* # InfoPipe DBus plumbing %{_sysconfdir}/dbus-1/system.d/org.freedesktop.sssd.infopipe.conf +%{_libdir}/%{name}/libsss_config.so

%files client -f sssd_client.lang %defattr(-,root,root,-) -- 1.7.11.7

I think Jakub's comments are addresed.

ACK

LS

I would actually prefer to omit the explicit dependency, that's what the Fedora Packaging Guidelines suggest as well. I verified that even without the explicit deps, RPM does its job:

rpm -q --requires -p /dev/shm/sssd/rpmbuild/RPMS/x86_64/sssd-dbus-1.11.90-0.fc20.x86_64.rpm | grep aug libaugeas.so.0()(64bit) libaugeas.so.0(AUGEAS_0.1.0)(64bit) libaugeas.so.0(AUGEAS_0.8.0)(64bit)

OK if I push the attached patchset (just the dependency is removed) ?

Sure, go ahead.