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.