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