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