[SSSD] [PATCH] sss_config: alter configuration file

Pavel Březina pbrezina at redhat.com
Mon May 5 09:36:42 UTC 2014


On 05/05/2014 09:32 AM, Lukas Slebodnik wrote:
> 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.
>>
>

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

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-diff.patch
Type: text/x-patch
Size: 7611 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140505/882bc2c0/attachment.bin>
-------------- 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/20140505/882bc2c0/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-sss_config-build.patch
Type: text/x-patch
Size: 4655 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140505/882bc2c0/attachment-0002.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/20140505/882bc2c0/attachment-0003.bin>


More information about the sssd-devel mailing list