[SSSD] [PATCH] sss_config: alter configuration file

Pavel Březina pbrezina at redhat.com
Tue May 13 21:01:58 UTC 2014


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

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.



-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-sss_config-the-code.patch
Type: text/x-patch
Size: 15247 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140513/16d8bc9d/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-sss_config-build.patch
Type: text/x-patch
Size: 4656 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140513/16d8bc9d/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-sss_config-unit-tests.patch
Type: text/x-patch
Size: 24835 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140513/16d8bc9d/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-sss_config-build-only-when-IFP-is-allowed.patch
Type: text/x-patch
Size: 3082 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140513/16d8bc9d/attachment-0003.bin>


More information about the sssd-devel mailing list