[SSSD] [PATCH] sss_config: alter configuration file

Lukas Slebodnik lslebodn at redhat.com
Tue May 13 05:45:04 UTC 2014


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



More information about the sssd-devel mailing list