[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