[Patch] Cmocka unit test for negcache.c module

[Pallavi Jha]

Hi,

I have attached the test patch for negcache.c module along with the mail.

I could not write test for the last function sss_ncache_prepopulate(). I
have doubt regarding the initialization of the arguments to be passed to
sss_ncache_prepopulate(). Please review the patch.

Thanks,
Pallavi

[Jakub Hrozek]

On Thu, Jan 02, 2014 at 11:48:48PM +0545, Pallavi Jha wrote:
> Hi,
>
> I have attached the test patch for negcache.c module along with the mail.
>
> I could not write test for the last function sss_ncache_prepopulate(). I
> have doubt regarding the initialization of the arguments to be passed to
> sss_ncache_prepopulate(). Please review the patch.
>
> Thanks,
> Pallavi

Hi Pallavi,

this is a great start. I have some comments on improving the test, see
below.

First, the patch needs rebasing on top of origin/master as it conflicts
with your authtok patch.
> From c1cc80745496c08ff31cd2fade0b306606199801 Mon Sep 17 00:00:00 2001
> From: Pallavi Jha 
> Date: Tue, 31 Dec 2013 18:04:16 +0530
> Subject: [PATCH] cmocka unit test for negcache module added

[snip]
> +/*
> SSSD
> NSS Responder
> Authors:
>     Pallavi Jha <[e-mail]>
>     Copyright (C) 2013 Red Hat
>     This program is free software; you can redistribute it and/or modify
>     it under the terms of the GNU General Public License as published by
>     the Free Software Foundation; either version 3 of the License, or
>     (at your option) any later version.
>     This program is distributed in the hope that it will be useful,
>     but WITHOUT ANY WARRANTY; without even the implied warranty of
>     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>     GNU General Public License for more details.
>     You should have received a copy of the GNU General Public License
>     along with this program.  If not, see <http://www.gnu.org/licenses/>.
>
> +*/
>
> +#include <stdarg.h>
> +#include <stdlib.h>
> +#include <stddef.h>
> +#include <setjmp.h>
> +#include <errno.h>
> +#include <unistd.h>
> +#include <sys/types.h>
> +#include <inttypes.h>
> +#include <cmocka.h>
>
> +#include "util/util.h"
> +#include "responder/common/responder.h"
> +#include "responder/common/negcache.h"
>
> +#define PORT 21
> +#define SID "10001"

It doesn't really matter in the context of the test, but maybe it would
be more readable to use something that resembles a SID more. sysdb tests
use 'S-1-2-3-4-5', feel free to re-use that identifier.
> +#define PROTO "TCP"
> +#define LIFETIME 200
> +#define NAME "foo_name"
>
> +struct cli_protocol_version *register_cli_protocol_version(void)
> +{
> static struct cli_protocol_version responder_test_cli_protocol_version[] = {
>    {0, NULL, NULL}
> };
> return responder_test_cli_protocol_version;
>
> +}

I don't think the register_cli_protocol_version function is needed at
all. It might be needed in some of the later tests, though, but this one
should do fine without the function, right?
> +struct test_state {
> struct sss_nc_ctx *ctx;
>
> +};
>
> +static void setup(void **state)
> +{
> int ret;
> struct test_state *ts;
> ts = talloc(NULL, struct test_state);
> assert_non_null(ts);
> ret  = sss_ncache_init(ts, &ts->ctx);
> assert_int_equal(ret, EOK);
> assert_non_null(ts->ctx);
> *state = (void *)ts;
>
> +}
>
> +static void teardown(void **state)
> +{
> struct test_state *ts = talloc_get_type_abort(*state, struct test_state);
> talloc_free(ts);
>
> +}
>
> +static void test_sss_ncache_init(void **state)
> +{
> int ret;
> struct test_state *ts;
> struct sss_nc_ctx *ctx;
> ts = talloc_get_type_abort(*state, struct test_state);
> ret = sss_ncache_init(ts, &ctx);
> assert_int_equal(ret, EOK);
> assert_non_null(ctx);

This test is fine, but I don't think it should use the setup/teardown
functions. That way, if this test is run without fixtures and would be
the first to run, you would verify that the fixtures are correct and can
be used for the rest of the tests.
> +}
>
> +/* @test_sss_ncache_uid : test following functions
> sss_ncache_set_uid
> sss_ncache_check_uid
> */
>
> +static void test_sss_ncache_uid(void **state)
> +{
> uid_t uid;
> int ret, ttl;
> bool permanent;
> TALLOC_CTX *memctx;
> struct test_state *ts;

I think it would be nice to add one more logic into the test. Make the
lifetime really short (say, 2 seconds) and in the test, verify that
right after setting the entry into negcache, the entry is there. Then
sleep for 2 seconds and verify that the entry is gone already.

Perhaps it would be sufficient to do this just in one test to not
prolong the time it takes to run 'make check'.
> ttl = LIFETIME;
> uid = getuid();
> ts = talloc_get_type_abort(*state, struct test_state);
> /* test when uid not present in database */
> ret = sss_ncache_check_uid(ts->ctx, ttl, uid);
> assert_int_equal(ret, ENOENT);
> /* test when uid is present in database */
> permanent = true;

The 'permanent' test case should make sure the entry stays in the
negative cache even after the TTL passes.
> ret = sss_ncache_set_uid(ts->ctx, permanent, uid);
> assert_int_equal(ret, EOK);
> ret = sss_ncache_check_uid(ts->ctx, ttl, uid);
> assert_int_equal(ret, EEXIST);
> permanent = false;

On the contrary, the 'not permanent' tests should make sure that the
entry goes away, as I noted above.
> ret = sss_ncache_set_uid(ts->ctx, permanent, uid);
> assert_int_equal(ret, EOK);
> ret = sss_ncache_check_uid(ts->ctx, ttl, uid);
> assert_int_equal(ret, EEXIST);
> /* test when ttl is -1 with uid present in database*/
> ttl = -1;
> ret = sss_ncache_check_uid(ts->ctx, ttl, uid);
> assert_int_equal(ret, EEXIST);
>
> +}

[snip]
> +static void test_sss_ncache_reset_permanent(void **state)
> +{
> int ret;
> struct test_state *ts;
> ts = talloc_get_type_abort(*state, struct test_state);
> ret = sss_ncache_reset_permament(ts->ctx);

^^ here is a typo, this should read 'permanent'. I suspect you just need
to rebase on top of your own rename patch :-)
> assert_int_equal(ret, EOK);
>
> +}
>
> +/*
> +static void test_sss_ncache_prepopulate(void **state)
> +{
> int ret;
> struct resp_ctx *rctx;
> struct confdb_ctx *cdb;
> struct sss_nc_ctx *ncache;
> // initialization need to be done
> ret = sss_ncache_prepopulate(ncache, cdb, rctx);o

You can use the mock function mock_rctx() from
src/tests/cmocka/common_mock_resp.c to initialize a dummy responder
context. Then you'd have to create a fake 'confdb' database using a call
to create_dom_test_ctx(). The configuration to be passed is then simply
an array of "struct sss_test_conf_param". For you case the array would
look something like:
    struct sss_test_conf_param params[] = {
            { "filter_users", "testuser1" },
            { "filter_groups", "testgroup1" },
            { NULL, NULL },             /* Sentinel */
    };

Check out how the fake database is created at
src/tests/cmocka/test_nss_srv.c:599
> assert_int_equal(ret, EOK);
>
> +}
> +*/
>
> +int main(void)
> +{
> const UnitTest tests[] = {
>    unit_test_setup_teardown(test_sss_ncache_init, setup, teardown),

As noted above, this test shouldn't use the setup and teardown routines,
because the test does the same as the fixtures. It can be called simply
with unit_test(test_sss_ncache_init)

(obviously it can't use the state then, but its own local state)
>    unit_test_setup_teardown(test_sss_ncache_uid, setup, teardown),
>    unit_test_setup_teardown(test_sss_ncache_gid, setup, teardown),
>    unit_test_setup_teardown(test_sss_ncache_sid, setup, teardown),
>    unit_test_setup_teardown(test_sss_ncache_user, setup, teardown),
>    unit_test_setup_teardown(test_sss_ncache_group, setup, teardown),
>    unit_test_setup_teardown(test_sss_ncache_netgr, setup, teardown),
>    unit_test_setup_teardown(test_sss_ncache_service_name, setup,
>                             teardown),
>    unit_test_setup_teardown(test_sss_ncache_service_port, setup,
>                              teardown),
>    unit_test_setup_teardown(test_sss_ncache_reset_permanent, setup,
>                              teardown)
>    //unit_test(test_sss_ncache_prepopulate)
> };
> return run_tests(tests);
> +}
>
> 1.8.1.4

Thanks for the patch!

[Pallavi Jha]

Hi,

I have tried to make all the required changes as mentioned in the above
mail. The patch for the same is attached along with this mail. While
writing test "test_sss_ncache_prepopulate" I came across a bug, as
mentioned below. Let me know if I am going wrong anywhere:

*In negcache.c*

line 676  ret = sss_parse_name_for_domains(tmpctx, domain_list,
                                         rctx->default_domain,
filter_list[i],
                                         &domainname, &name);

line 676 calls sss_parse_name() which in return executes line no 395 in
src/util/usertools.c  which is  ret = sss_parse_name(tmp_ctx, dom->names,
orig, &dmatch, &nmatch);

Now look at 2nd argument of sss_parse_namae it is of type char * but if you
check sss_parse_name defn at line no 304:

int sss_parse_name(TALLOC_CTX *memctx,
                   struct sss_names_ctx *snctx,
                   const char *orig, char **domain, char **name)

so line no 308 : pcre *re = snctx->re;  is the reason for
crash(segmentation fault).

Please review the patch.

Thanks!
Pallavi

On 12 January 2014 21:55, Jakub Hrozek  wrote:
> On Thu, Jan 02, 2014 at 11:48:48PM +0545, Pallavi Jha wrote:
> > Hi,
> >
> > I have attached the test patch for negcache.c module along with the mail.
> >
> > I could not write test for the last function sss_ncache_prepopulate(). I
> > have doubt regarding the initialization of the arguments to be passed to
> > sss_ncache_prepopulate(). Please review the patch.
> >
> > Thanks,
> > Pallavi
>
> Hi Pallavi,
>
> this is a great start. I have some comments on improving the test, see
> below.
>
> First, the patch needs rebasing on top of origin/master as it conflicts
> with your authtok patch.
> > From c1cc80745496c08ff31cd2fade0b306606199801 Mon Sep 17 00:00:00 2001
> > From: Pallavi Jha 
> > Date: Tue, 31 Dec 2013 18:04:16 +0530
> > Subject: [PATCH] cmocka unit test for negcache module added
>
> [snip]
> > +/*
> > SSSD
> > NSS Responder
> > Authors:
> >     Pallavi Jha <[e-mail]>
> >     Copyright (C) 2013 Red Hat
> >     This program is free software; you can redistribute it and/or
>
> modify
> >     it under the terms of the GNU General Public License as
>
> published by
> >     the Free Software Foundation; either version 3 of the License,
>
> or
> >     (at your option) any later version.
> >     This program is distributed in the hope that it will be useful,
> >     but WITHOUT ANY WARRANTY; without even the implied warranty of
> >     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> >     GNU General Public License for more details.
> >     You should have received a copy of the GNU General Public
>
> License
> >     along with this program.  If not, see <
>
> http://www.gnu.org/licenses/%3E.
> > +*/
> >
> > +#include <stdarg.h>
> > +#include <stdlib.h>
> > +#include <stddef.h>
> > +#include <setjmp.h>
> > +#include <errno.h>
> > +#include <unistd.h>
> > +#include <sys/types.h>
> > +#include <inttypes.h>
> > +#include <cmocka.h>
> >
> > +#include "util/util.h"
> > +#include "responder/common/responder.h"
> > +#include "responder/common/negcache.h"
> >
> > +#define PORT 21
> > +#define SID "10001"
>
> It doesn't really matter in the context of the test, but maybe it would
> be more readable to use something that resembles a SID more. sysdb tests
> use 'S-1-2-3-4-5', feel free to re-use that identifier.
> > +#define PROTO "TCP"
> > +#define LIFETIME 200
> > +#define NAME "foo_name"
> >
> > +struct cli_protocol_version *register_cli_protocol_version(void)
> > +{
> > static struct cli_protocol_version
>
> responder_test_cli_protocol_version[] = {
> >    {0, NULL, NULL}
> > };
> > return responder_test_cli_protocol_version;
> >
> > +}
>
> I don't think the register_cli_protocol_version function is needed at
> all. It might be needed in some of the later tests, though, but this one
> should do fine without the function, right?
> > +struct test_state {
> > struct sss_nc_ctx *ctx;
> >
> > +};
> >
> > +static void setup(void **state)
> > +{
> > int ret;
> > struct test_state *ts;
> > ts = talloc(NULL, struct test_state);
> > assert_non_null(ts);
> > ret  = sss_ncache_init(ts, &ts->ctx);
> > assert_int_equal(ret, EOK);
> > assert_non_null(ts->ctx);
> > *state = (void *)ts;
> >
> > +}
> >
> > +static void teardown(void **state)
> > +{
> > struct test_state *ts = talloc_get_type_abort(*state, struct
>
> test_state);
> > talloc_free(ts);
> >
> > +}
> >
> > +static void test_sss_ncache_init(void **state)
> > +{
> > int ret;
> > struct test_state *ts;
> > struct sss_nc_ctx *ctx;
> > ts = talloc_get_type_abort(*state, struct test_state);
> > ret = sss_ncache_init(ts, &ctx);
> > assert_int_equal(ret, EOK);
> > assert_non_null(ctx);
>
> This test is fine, but I don't think it should use the setup/teardown
> functions. That way, if this test is run without fixtures and would be
> the first to run, you would verify that the fixtures are correct and can
> be used for the rest of the tests.
> > +}
> >
> > +/* @test_sss_ncache_uid : test following functions
> > sss_ncache_set_uid
> > sss_ncache_check_uid
> > */
> >
> > +static void test_sss_ncache_uid(void **state)
> > +{
> > uid_t uid;
> > int ret, ttl;
> > bool permanent;
> > TALLOC_CTX *memctx;
> > struct test_state *ts;
>
> I think it would be nice to add one more logic into the test. Make the
> lifetime really short (say, 2 seconds) and in the test, verify that
> right after setting the entry into negcache, the entry is there. Then
> sleep for 2 seconds and verify that the entry is gone already.
>
> Perhaps it would be sufficient to do this just in one test to not
> prolong the time it takes to run 'make check'.
> > ttl = LIFETIME;
> > uid = getuid();
> > ts = talloc_get_type_abort(*state, struct test_state);
> > /* test when uid not present in database */
> > ret = sss_ncache_check_uid(ts->ctx, ttl, uid);
> > assert_int_equal(ret, ENOENT);
> > /* test when uid is present in database */
> > permanent = true;
>
> The 'permanent' test case should make sure the entry stays in the
> negative cache even after the TTL passes.
> > ret = sss_ncache_set_uid(ts->ctx, permanent, uid);
> > assert_int_equal(ret, EOK);
> > ret = sss_ncache_check_uid(ts->ctx, ttl, uid);
> > assert_int_equal(ret, EEXIST);
> > permanent = false;
>
> On the contrary, the 'not permanent' tests should make sure that the
> entry goes away, as I noted above.
> > ret = sss_ncache_set_uid(ts->ctx, permanent, uid);
> > assert_int_equal(ret, EOK);
> > ret = sss_ncache_check_uid(ts->ctx, ttl, uid);
> > assert_int_equal(ret, EEXIST);
> > /* test when ttl is -1 with uid present in database*/
> > ttl = -1;
> > ret = sss_ncache_check_uid(ts->ctx, ttl, uid);
> > assert_int_equal(ret, EEXIST);
> >
> > +}
>
> [snip]
> > +static void test_sss_ncache_reset_permanent(void **state)
> > +{
> > int ret;
> > struct test_state *ts;
> > ts = talloc_get_type_abort(*state, struct test_state);
> > ret = sss_ncache_reset_permament(ts->ctx);
>
> ^^ here is a typo, this should read 'permanent'. I suspect you just need
> to rebase on top of your own rename patch :-)
> > assert_int_equal(ret, EOK);
> >
> > +}
> >
> > +/*
> > +static void test_sss_ncache_prepopulate(void **state)
> > +{
> > int ret;
> > struct resp_ctx *rctx;
> > struct confdb_ctx *cdb;
> > struct sss_nc_ctx *ncache;
> > // initialization need to be done
> > ret = sss_ncache_prepopulate(ncache, cdb, rctx);o
>
> You can use the mock function mock_rctx() from
> src/tests/cmocka/common_mock_resp.c to initialize a dummy responder
> context. Then you'd have to create a fake 'confdb' database using a call
> to create_dom_test_ctx(). The configuration to be passed is then simply
> an array of "struct sss_test_conf_param". For you case the array would
> look something like:
>     struct sss_test_conf_param params[] = {
>             { "filter_users", "testuser1" },
>             { "filter_groups", "testgroup1" },
>             { NULL, NULL },             /* Sentinel */
>     };
>
> Check out how the fake database is created at
> src/tests/cmocka/test_nss_srv.c:599
> > assert_int_equal(ret, EOK);
> >
> > +}
> > +*/
> >
> > +int main(void)
> > +{
> > const UnitTest tests[] = {
> >    unit_test_setup_teardown(test_sss_ncache_init, setup, teardown),
>
> As noted above, this test shouldn't use the setup and teardown routines,
> because the test does the same as the fixtures. It can be called simply
> with unit_test(test_sss_ncache_init)
>
> (obviously it can't use the state then, but its own local state)
> >    unit_test_setup_teardown(test_sss_ncache_uid, setup, teardown),
> >    unit_test_setup_teardown(test_sss_ncache_gid, setup, teardown),
> >    unit_test_setup_teardown(test_sss_ncache_sid, setup, teardown),
> >    unit_test_setup_teardown(test_sss_ncache_user, setup, teardown),
> >    unit_test_setup_teardown(test_sss_ncache_group, setup,
>
> teardown),
> >    unit_test_setup_teardown(test_sss_ncache_netgr, setup,
>
> teardown),
> >    unit_test_setup_teardown(test_sss_ncache_service_name, setup,
> >                             teardown),
> >    unit_test_setup_teardown(test_sss_ncache_service_port, setup,
> >                              teardown),
> >    unit_test_setup_teardown(test_sss_ncache_reset_permanent, setup,
> >                              teardown)
> >    //unit_test(test_sss_ncache_prepopulate)
> > };
> > return run_tests(tests);
> > +}
> >
> > 1.8.1.4
>
> Thanks for the patch!

[Jakub Hrozek]

On Tue, Jan 28, 2014 at 04:50:44AM +0545, Pallavi Jha wrote:
> Hi,
>
> I have tried to make all the required changes as mentioned in the above
> mail. The patch for the same is attached along with this mail. While
> writing test "test_sss_ncache_prepopulate" I came across a bug, as
> mentioned below. Let me know if I am going wrong anywhere:
>
> *In negcache.c*
>
> line 676  ret = sss_parse_name_for_domains(tmpctx, domain_list,
>                                          rctx->default_domain,
> filter_list[i],
>                                          &domainname, &name);
>
> line 676 calls sss_parse_name() which in return executes line no 395 in
> src/util/usertools.c  which is  ret = sss_parse_name(tmp_ctx, dom->names,
> orig, &dmatch, &nmatch);
>
> Now look at 2nd argument of sss_parse_namae it is of type char * but if you
> check sss_parse_name defn at line no 304:
>
> int sss_parse_name(TALLOC_CTX *memctx,
>                    struct sss_names_ctx *snctx,
>                    const char *orig, char **domain, char **name)
>
> so line no 308 : pcre *re = snctx->re;  is the reason for
> crash(segmentation fault).
>
> Please review the patch.

Hi,

you just need to initialize the snctx, that can be done with a similar
call in the prepopulate test:

ret = sss_names_init(ts, tc->confdb, TEST_DOM_NAME, &dom->names);
 assert_int_equal(ret, EOK);

See some minor comments about the patch inline.
> +#define SHORTSPAN 2

I know I suggested the sleep of 2 seconds, but when I ran the test now
it looked like too much, the test was paused for too long. Maybe just 1s
would be better, I tried running the test in a loop and didn't find any
bugs the short span would cause

[snip]
> +static void test_sss_ncache_prepopulate(void **state)
> +{
> int ret;
> struct test_state *ts;
> struct tevent_context *ev;
> struct sss_nc_ctx *ncache;
> struct sss_test_ctx *tc;
> struct sss_domain_info *dom;
> ts = talloc_get_type_abort(*state, struct test_state);
> ev = tevent_context_init(ts);

You should check if ev is not NULL.
> dom = talloc(ts, struct sss_domain_info);

Same here for dom.
> dom->name = NAME;

It's a very good idea to define the NAME, but with my (admittedly
strict) CFLAGS, I'm getting a warning because the domain name is "char
*" but the string constant in NAME is "const char *".

You can get around this warning by using a special 'discard_const' macro
like this:
    dom->name = discard_const(NAME);
> ts->nctx = mock_nctx(ts);
> assert_non_null(ts->nctx);
> struct sss_test_conf_param params[] = {
>    { "filter_users", "testuser1" },
>    { "filter_groups", "testgroup1" },
>    { NULL, NULL },
> };

I prefer if variable definitions are kept at the function beginning and
not interleave with code.
> tc = create_dom_test_ctx(ts, TESTS_PATH, TEST_CONF_DB,
>                         TEST_SYSDB_FILE, TEST_DOM_NAME,
>                         TEST_ID_PROVIDER, params);
> assert_non_null(tc);
> ncache = ts->ctx;
> ts->rctx = mock_rctx(ts, ev, dom, ts->nctx);
> assert_non_null(ts->rctx);
> assert_non_null(tc->confdb);

Why do you check confdb here separately? I think create_dom_test_ctx can
be trusted to create it on its own.
> ret = sss_ncache_prepopulate(ncache, tc->confdb, ts->rctx);
> assert_int_equal(ret, EOK);

I assume the continuation of this test would be to check that testuser1
and testgroup1 will be present in the cache even after the short sleep?
> +}

The test is looking good, nice work!

[Pallavi Jha]

Hi,

I have tried making all the changes suggested to the unit test of negcache
patch. The patch is attached with the mail.

All is working fine except when I add the check for the existence of
"testuser1" and "testgroup1" in test_sss_ncache_prepopulate(). Test fails
there. Please review the patch and kindly let me know where I am going
wrong.

Thanks!

On 29 January 2014 03:26, Jakub Hrozek  wrote:
> On Tue, Jan 28, 2014 at 04:50:44AM +0545, Pallavi Jha wrote:
> > Hi,
> >
> > I have tried to make all the required changes as mentioned in the above
> > mail. The patch for the same is attached along with this mail. While
> > writing test "test_sss_ncache_prepopulate" I came across a bug, as
> > mentioned below. Let me know if I am going wrong anywhere:
> >
> > *In negcache.c*
> >
> > line 676  ret = sss_parse_name_for_domains(tmpctx, domain_list,
> >                                          rctx->default_domain,
> > filter_list[i],
> >                                          &domainname, &name);
> >
> > line 676 calls sss_parse_name() which in return executes line no 395 in
> > src/util/usertools.c  which is  ret = sss_parse_name(tmp_ctx, dom->names,
> > orig, &dmatch, &nmatch);
> >
> > Now look at 2nd argument of sss_parse_namae it is of type char * but if
>
> you
> > check sss_parse_name defn at line no 304:
> >
> > int sss_parse_name(TALLOC_CTX *memctx,
> >                    struct sss_names_ctx *snctx,
> >                    const char *orig, char **domain, char **name)
> >
> > so line no 308 : pcre *re = snctx->re;  is the reason for
> > crash(segmentation fault).
> >
> > Please review the patch.
>
> Hi,
>
> you just need to initialize the snctx, that can be done with a similar
> call in the prepopulate test:
>
> ret = sss_names_init(ts, tc->confdb, TEST_DOM_NAME, &dom->names);
>  assert_int_equal(ret, EOK);
>
> See some minor comments about the patch inline.
> > +#define SHORTSPAN 2
>
> I know I suggested the sleep of 2 seconds, but when I ran the test now
> it looked like too much, the test was paused for too long. Maybe just 1s
> would be better, I tried running the test in a loop and didn't find any
> bugs the short span would cause
>
> [snip]
> > +static void test_sss_ncache_prepopulate(void **state)
> > +{
> > int ret;
> > struct test_state *ts;
> > struct tevent_context *ev;
> > struct sss_nc_ctx *ncache;
> > struct sss_test_ctx *tc;
> > struct sss_domain_info *dom;
> > ts = talloc_get_type_abort(*state, struct test_state);
> > ev = tevent_context_init(ts);
>
> You should check if ev is not NULL.
> > dom = talloc(ts, struct sss_domain_info);
>
> Same here for dom.
> > dom->name = NAME;
>
> It's a very good idea to define the NAME, but with my (admittedly
> strict) CFLAGS, I'm getting a warning because the domain name is "char
> *" but the string constant in NAME is "const char *".
>
> You can get around this warning by using a special 'discard_const' macro
> like this:
>     dom->name = discard_const(NAME);
> > ts->nctx = mock_nctx(ts);
> > assert_non_null(ts->nctx);
> > struct sss_test_conf_param params[] = {
> >    { "filter_users", "testuser1" },
> >    { "filter_groups", "testgroup1" },
> >    { NULL, NULL },
> > };
>
> I prefer if variable definitions are kept at the function beginning and
> not interleave with code.
> > tc = create_dom_test_ctx(ts, TESTS_PATH, TEST_CONF_DB,
> >                         TEST_SYSDB_FILE, TEST_DOM_NAME,
> >                         TEST_ID_PROVIDER, params);
> > assert_non_null(tc);
> > ncache = ts->ctx;
> > ts->rctx = mock_rctx(ts, ev, dom, ts->nctx);
> > assert_non_null(ts->rctx);
> > assert_non_null(tc->confdb);
>
> Why do you check confdb here separately? I think create_dom_test_ctx can
> be trusted to create it on its own.
> > ret = sss_ncache_prepopulate(ncache, tc->confdb, ts->rctx);
> > assert_int_equal(ret, EOK);
>
> I assume the continuation of this test would be to check that testuser1
> and testgroup1 will be present in the cache even after the short sleep?
> > +}
>
> The test is looking good, nice work!

[Lukas Slebodnik]

On (15/02/14 01:43), Pallavi Jha wrote:
> Hi,
>
> I have tried making all the changes suggested to the unit test of negcache
> patch. The patch is attached with the mail.
>
> All is working fine except when I add the check for the existence of
> "testuser1" and "testgroup1" in test_sss_ncache_prepopulate(). Test fails
> there. Please review the patch and kindly let me know where I am going
> wrong.
>
> Thanks!
>
> On 29 January 2014 03:26, Jakub Hrozek  wrote:
> > On Tue, Jan 28, 2014 at 04:50:44AM +0545, Pallavi Jha wrote:
> > > Hi,
> > >
> > > I have tried to make all the required changes as mentioned in the above
> > > mail. The patch for the same is attached along with this mail. While
> > > writing test "test_sss_ncache_prepopulate" I came across a bug, as
> > > mentioned below. Let me know if I am going wrong anywhere:
> > >
> > > *In negcache.c*
> > >
> > > line 676  ret = sss_parse_name_for_domains(tmpctx, domain_list,
> > >                                          rctx->default_domain,
> > > filter_list[i],
> > >                                          &domainname, &name);
> > >
> > > line 676 calls sss_parse_name() which in return executes line no 395 in
> > > src/util/usertools.c  which is  ret = sss_parse_name(tmp_ctx, dom->names,
> > > orig, &dmatch, &nmatch);
> > >
> > > Now look at 2nd argument of sss_parse_namae it is of type char * but if
> >
> > you
> > > check sss_parse_name defn at line no 304:
> > >
> > > int sss_parse_name(TALLOC_CTX *memctx,
> > >                    struct sss_names_ctx *snctx,
> > >                    const char *orig, char **domain, char **name)
> > >
> > > so line no 308 : pcre *re = snctx->re;  is the reason for
> > > crash(segmentation fault).
> > >
> > > Please review the patch.
> >
> > Hi,
> >
> > you just need to initialize the snctx, that can be done with a similar
> > call in the prepopulate test:
> >
> > ret = sss_names_init(ts, tc->confdb, TEST_DOM_NAME, &dom->names);
> >  assert_int_equal(ret, EOK);
> >
> > See some minor comments about the patch inline.
> > > +#define SHORTSPAN 2
> >
> > I know I suggested the sleep of 2 seconds, but when I ran the test now
> > it looked like too much, the test was paused for too long. Maybe just 1s
> > would be better, I tried running the test in a loop and didn't find any
> > bugs the short span would cause
> >
> > [snip]
> > > +static void test_sss_ncache_prepopulate(void **state)
> > > +{
> > > int ret;
> > > struct test_state *ts;
> > > struct tevent_context *ev;
> > > struct sss_nc_ctx *ncache;
> > > struct sss_test_ctx *tc;
> > > struct sss_domain_info *dom;
> > > ts = talloc_get_type_abort(*state, struct test_state);
> > > ev = tevent_context_init(ts);
> >
> > You should check if ev is not NULL.
> > > dom = talloc(ts, struct sss_domain_info);
> >
> > Same here for dom.
> > > dom->name = NAME;
> >
> > It's a very good idea to define the NAME, but with my (admittedly
> > strict) CFLAGS, I'm getting a warning because the domain name is "char
> > *" but the string constant in NAME is "const char *".
> >
> > You can get around this warning by using a special 'discard_const' macro
> > like this:
> >     dom->name = discard_const(NAME);
> > > ts->nctx = mock_nctx(ts);
> > > assert_non_null(ts->nctx);
> > > struct sss_test_conf_param params[] = {
> > >    { "filter_users", "testuser1" },
> > >    { "filter_groups", "testgroup1" },
> > >    { NULL, NULL },
> > > };
> >
> > I prefer if variable definitions are kept at the function beginning and
> > not interleave with code.
> > > tc = create_dom_test_ctx(ts, TESTS_PATH, TEST_CONF_DB,
> > >                         TEST_SYSDB_FILE, TEST_DOM_NAME,
> > >                         TEST_ID_PROVIDER, params);
> > > assert_non_null(tc);
> > > ncache = ts->ctx;
> > > ts->rctx = mock_rctx(ts, ev, dom, ts->nctx);
> > > assert_non_null(ts->rctx);
> > > assert_non_null(tc->confdb);
> >
> > Why do you check confdb here separately? I think create_dom_test_ctx can
> > be trusted to create it on its own.
> > > ret = sss_ncache_prepopulate(ncache, tc->confdb, ts->rctx);
> > > assert_int_equal(ret, EOK);
> >
> > I assume the continuation of this test would be to check that testuser1
> > and testgroup1 will be present in the cache even after the short sleep?
> > > +}
> >
> > The test is looking good, nice work!
> From 55b61c6ab1b0578a6ccb1f88c010c56cda6e16ec Mon Sep 17 00:00:00 2001
> From: Pallavi Jha 
> Date: Sat, 18 Jan 2014 14:36:25 +0545
> Subject: [PATCH] Unit-test-for-negcache-module-added
>
> Makefile.am                      |  17 +
> src/tests/cmocka/test_negcache.c | 648 +++++++++++++++++++++++++++++++++++++++
> 2 files changed, 665 insertions(+)
> create mode 100644 src/tests/cmocka/test_negcache.c
>
> diff --git a/Makefile.am b/Makefile.am
> index 9c155d68c7f452bb02a4da154992fa2fca6af273..3ef6c3f57fd1ca31b57745cc068f918a135ae3a9 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -150,6 +150,7 @@ if HAVE_CMOCKA
>         nss-srv-tests \
>         test-find-uid \
>         test-io \
>    test-negcache \
>   test-authtok \
>   sss_nss_idmap-tests \
>   dyndns-tests \
>
> @@ -1347,6 +1348,22 @@ test_io_CFLAGS = \
> test_io_LDADD = \
>     $(CMOCKA_LIBS)
>
> +EXTRA_test_negcache_DEPENDENCIES = \
> $(ldblib_LTLIBRARIES)
>
> +test_negcache_SOURCES = \
> $(SSSD_RESPONDER_OBJ) \
> src/tests/cmocka/test_negcache.c
>
> +test_negcache_CFLAGS = \
> $(AM_CFLAGS) \
> $(TALLOC_CFLAGS) \
> $(DHASH_CFLAGS)
>
> +test_negcache_LDADD = \
> $(CMOCKA_LIBS) \
> $(SSSD_LIBS) \
> $(SSSD_INTERNAL_LTLIBS) \
> libsss_test_common.la \
> libsss_idmap.la
>
> test_authtok_SOURCES = \
>     src/tests/cmocka/test_authtok.c \
>     src/util/authtok.c \
> diff --git a/src/tests/cmocka/test_negcache.c b/src/tests/cmocka/test_negcache.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..85903a3be7900e39a3bebac438d6e7d977f177d3
> --- /dev/null
> +++ b/src/tests/cmocka/test_negcache.c
> @@ -0,0 +1,648 @@
> +/*
> SSSD
> NSS Responder
> Authors:
>     Pallavi Jha <[e-mail]>
>     Copyright (C) 2013 Red Hat
>     This program is free software; you can redistribute it and/or modify
>     it under the terms of the GNU General Public License as published by
>     the Free Software Foundation; either version 3 of the License, or
>     (at your option) any later version.
>     This program is distributed in the hope that it will be useful,
>     but WITHOUT ANY WARRANTY; without even the implied warranty of
>     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>     GNU General Public License for more details.
>     You should have received a copy of the GNU General Public License
>     along with this program.  If not, see <http://www.gnu.org/licenses/>.
>
> +*/
>
> +#include <stdarg.h>
> +#include <stdlib.h>
> +#include <stddef.h>
> +#include <setjmp.h>
> +#include <errno.h>
> +#include <unistd.h>
> +#include <sys/types.h>
> +#include <inttypes.h>
> +#include <cmocka.h>
>
> +#include "tests/cmocka/common_mock.h"
> +#include "tests/cmocka/common_mock_resp.h"
> +#include "responder/nss/nsssrv.h"
> +#include "responder/nss/nsssrv_private.h"
> +#include "sss_client/idmap/sss_nss_idmap.h"
> +#include "util/util_sss_idmap.h"
> +#include "lib/idmap/sss_idmap.h"
> +#include "util/util.h"
> +#include "util/util_sss_idmap.h"
> +#include "responder/common/responder.h"
> +#include "responder/common/negcache.h"
>
> +#define PORT 21
> +#define SID "S-1-2-3-4-5"
> +#define PROTO "TCP"
> +#define LIFETIME 200
> +#define SHORTSPAN 1
> +#define NAME "foo_name"
> +#define UID "U-1-2-3-4-5"
> +#define TESTS_PATH "tests_nss"
> +#define TEST_CONF_DB "test_nss_conf.ldb"
> +#define TEST_SYSDB_FILE "cache_nss_test.ldb"
> +#define TEST_DOM_NAME "nss_test"
> +#define TEST_SUBDOM_NAME "test.sub"
> +#define TEST_ID_PROVIDER "ldap"
>
> +/* register_cli_protocol_version is required in test since it links with
> responder_common.c module
> */
>
> +struct cli_protocol_version *register_cli_protocol_version(void)
> +{
> static struct cli_protocol_version responder_test_cli_protocol_version[] = {
>    {0, NULL, NULL}
> };
> return responder_test_cli_protocol_version;
>
> +}
>
> +/* Mock NSS structure */
> +struct nss_ctx *
> +mock_nctx(TALLOC_CTX *mem_ctx)
> +{
> struct nss_ctx *nctx;
> errno_t ret;
> enum idmap_error_code err;
> nctx = talloc_zero(mem_ctx, struct nss_ctx);
> if (!nctx) {
>    return NULL;
> }
> ret = sss_ncache_init(nctx, &nctx->ncache);
> if (ret != EOK) {
>    talloc_free(nctx);
>    return NULL;
> }
> nctx->neg_timeout = 10;
> nctx->pwfield = discard_const("*");
> err = sss_idmap_init(sss_idmap_talloc, nctx, sss_idmap_talloc_free,
>                     &nctx->idmap_ctx);
> if (err != IDMAP_SUCCESS) {
>    DEBUG(SSSDBG_FATAL_FAILURE, ("sss_idmap_init failed.\n"));
>    talloc_free(nctx);
>    return NULL;
> }
> return nctx;
>
> +}
>
> +/* Mock a responder context */
> +struct resp_ctx *
> +mock_rctx(TALLOC_CTX *mem_ctx,
>      struct tevent_context *ev,
>      struct sss_domain_info *domains,
>      void *pvt_ctx)
>
> +{
> struct resp_ctx *rctx;
> errno_t ret;
> rctx = talloc_zero(mem_ctx, struct resp_ctx);
> if (!rctx) return NULL;
> ret = sss_hash_create(rctx, 30, &rctx->dp_request_table);
> if (ret != EOK) {
>    talloc_free(rctx);
>    return NULL;
> }
> rctx->ev = ev;
> rctx->domains = domains;
> rctx->pvt_ctx = pvt_ctx;
> return rctx;
>
> +}
>
> +struct test_state {
> struct sss_nc_ctx *ctx;
> struct nss_ctx *nctx;
> struct resp_ctx *rctx;
>
> +};
>
> +static void setup(void **state)
> +{
> int ret;
> struct test_state *ts;
> ts = talloc(NULL, struct test_state);
> assert_non_null(ts);
> ret  = sss_ncache_init(ts, &ts->ctx);
> assert_int_equal(ret, EOK);
> assert_non_null(ts->ctx);
> *state = (void *)ts;
>
> +}
>
> +static void teardown(void **state)
> +{
> struct test_state *ts = talloc_get_type_abort(*state, struct test_state);
> talloc_free(ts);
>
> +}
>
> +static void test_sss_ncache_init(void **state)
> +{
> int ret;
> TALLOC_CTX *memctx;
> struct sss_nc_ctx *ctx;
> memctx = talloc_new(NULL);
> assert_non_null(memctx);
> ret = sss_ncache_init(memctx, &ctx );
> assert_int_equal(ret, errno);
> assert_int_equal(ret, EOK);
> assert_non_null(ctx);
> talloc_free(memctx);
>
> +}
>
> +/* @test_sss_ncache_uid : test following functions
> sss_ncache_set_uid
> sss_ncache_check_uid
> */
>
> +static void test_sss_ncache_uid(void **state)
> +{
> uid_t uid;
> int ret, ttl;
> bool permanent;
> struct test_state *ts;
> ttl = LIFETIME;
> uid = getuid();
> ts = talloc_get_type_abort(*state, struct test_state);
> /* test when uid not present in database */
> ret = sss_ncache_check_uid(ts->ctx, ttl, uid);
> assert_int_equal(ret, ENOENT);
> /* test when uid is present in database */
> permanent = true;
> ret = sss_ncache_reset_permanent(ts->ctx);
> assert_int_equal(ret, EOK);
> ret = sss_ncache_set_uid(ts->ctx, permanent, uid);
> assert_int_equal(ret, EOK);
> ret = sss_ncache_check_uid(ts->ctx, ttl, uid);
> assert_int_equal(ret, EEXIST);
> ttl = SHORTSPAN;
> ret = sss_ncache_set_uid(ts->ctx, permanent, uid);
> assert_int_equal(ret, EOK);
> ret = sss_ncache_check_uid(ts->ctx, ttl, uid);
> assert_int_equal(ret, EEXIST);
> sleep(SHORTSPAN + 1);
> ret = sss_ncache_check_uid(ts->ctx, ttl, uid);
> assert_int_equal(ret, EEXIST);
> permanent = false;
> ret = sss_ncache_set_uid(ts->ctx, permanent, uid);
> assert_int_equal(ret, EOK);
> ret = sss_ncache_check_uid(ts->ctx, ttl, uid);
> assert_int_equal(ret, EEXIST);
> sleep(SHORTSPAN + 1);
> ret = sss_ncache_check_uid(ts->ctx, ttl, uid);
> assert_int_equal(ret, ENOENT);
> ret = sss_ncache_set_uid(ts->ctx, permanent, uid);
> assert_int_equal(ret, EOK);
> /* test when ttl is -1 with uid present in database*/
> ttl = -1;
> ret = sss_ncache_check_uid(ts->ctx, ttl, uid);
> assert_int_equal(ret, EEXIST);
>
> +}
>
> +/* @test_sss_ncache_gid : test following functions
> sss_ncache_set_gid
> sss_ncache_check_gid
> */
>
> +static void test_sss_ncache_gid(void **state)
> +{
> gid_t gid;
> int ret, ttl;
> bool permanent;
> struct test_state *ts;
> ttl = LIFETIME;
> gid = getgid();
> ts = talloc_get_type_abort(*state, struct test_state);
> /* test when gid is not present in database */
> ret = sss_ncache_check_gid(ts->ctx, ttl, gid);
> assert_int_equal(ret, ENOENT);
> /* test when gid is present in database */
> permanent = true;
> ret = sss_ncache_set_gid(ts->ctx, permanent, gid);
> assert_int_equal(ret, EOK);
> ret = sss_ncache_check_gid(ts->ctx, ttl, gid);
> assert_int_equal(ret, EEXIST);
> permanent = false;
> ret = sss_ncache_set_uid(ts->ctx, permanent, gid);
> assert_int_equal(ret, EOK);
> ret = sss_ncache_check_uid(ts->ctx, ttl, gid);
> assert_int_equal(ret, EEXIST);
> /* test when ttl is -1 with gid present in database*/
> ttl = -1;
> ret = sss_ncache_check_gid(ts->ctx, ttl, gid);
> assert_int_equal(ret, EEXIST);
>
> +}
>
> +/* @test_sss_ncache_sid : test following functions
> sss_ncache_set_sid
> sss_ncache_check_sid
> */
>
> +static void test_sss_ncache_sid(void **state)
> +{
> int ret, ttl;
> bool permanent;
> const char *sid = NULL;
> struct test_state *ts;
> ttl = LIFETIME;
> sid = SID;
> ts = talloc_get_type_abort(*state, struct test_state);
> /*test when sid in not present in database */
> ret = sss_ncache_check_sid(ts->ctx, ttl, sid);
> assert_int_equal(ret, ENOENT);
> /* test when sid is present in database */
> permanent = true;
> ret = sss_ncache_set_sid(ts->ctx, permanent, sid);
> assert_int_equal(ret, EOK);
> ret = sss_ncache_check_sid(ts->ctx, ttl, sid);
> assert_int_equal(ret, EEXIST);
> permanent = false;
> ret = sss_ncache_set_sid(ts->ctx, permanent, sid);
> assert_int_equal(ret, EOK);
> ret = sss_ncache_check_sid(ts->ctx, ttl, sid);
> assert_int_equal(ret, EEXIST);
> /* test when ttl is -1 with sid present in database*/
> ttl = -1;
> ret = sss_ncache_check_sid(ts->ctx, ttl, sid);
> assert_int_equal(ret, EEXIST);
>
> +}
>
> +/* @test_sss_ncache_user : test following functions
> sss_ncache_check_user
> sss_ncache_set_user
> */
>
> +static void test_sss_ncache_user(void **state)
> +{
> int ret, ttl;
> bool permanent;
> const char *name = NAME;
> struct test_state *ts;
> struct sss_domain_info *dom;
> ttl = LIFETIME;
> ts = talloc_get_type_abort(*state, struct test_state);
> dom = talloc(ts, struct sss_domain_info);
> dom->name = discard_const(NAME);
> /* test when domain name is not present in database */
> dom->case_sensitive = false;
> ret = sss_ncache_check_user(ts->ctx, ttl, dom, name);
> assert_int_equal(ret, ENOENT);
> dom->case_sensitive = true;
> ret = sss_ncache_check_user(ts->ctx, ttl, dom, name);
> assert_int_equal(ret, ENOENT);
> /* test when domain name is present in database */
> permanent = true;
> ret = sss_ncache_set_user(ts->ctx, permanent, dom, name);
> assert_int_equal(ret, EOK);
> ret = sss_ncache_check_user(ts->ctx, ttl, dom, name);
> assert_int_equal(ret, EEXIST);
> permanent = false;
> ret = sss_ncache_set_user(ts->ctx, permanent, dom, name);
> assert_int_equal(ret, EOK);
> ret = sss_ncache_check_user(ts->ctx, ttl, dom, name);
> assert_int_equal(ret, EEXIST);
> /* test when ttl is -1 with domain name present in database */
> ttl = -1;
> ret = sss_ncache_check_user(ts->ctx, ttl, dom, name);
> assert_int_equal(ret, EEXIST);
>
> +}
>
> +/* @test_sss_ncache_group : test following functions
> sss_ncache_check_group
> sss_ncache_set_group
> */
>
> +static void test_sss_ncache_group(void **state)
> +{
> int ret, ttl;
> bool permanent;
> const char *name = NAME;
> struct test_state *ts;
> struct sss_domain_info *dom;
> ttl = LIFETIME;
> ts = talloc_get_type_abort(*state, struct test_state);
> dom = talloc(ts, struct sss_domain_info);
> dom->name = discard_const(NAME);
> /* test when domain name is not present in database */
> dom->case_sensitive = false;
> ret = sss_ncache_check_group(ts->ctx, ttl, dom, name);
> assert_int_equal(ret, ENOENT);
> dom->case_sensitive = true;
> ret = sss_ncache_check_group(ts->ctx, ttl, dom, name);
> assert_int_equal(ret, ENOENT);
> /* test when domain name is present in database */
> permanent = true;
> ret = sss_ncache_set_group(ts->ctx, permanent, dom, name);
> assert_int_equal(ret, EOK);
> ret = sss_ncache_check_group(ts->ctx, ttl, dom, name);
> assert_int_equal(ret, EEXIST);
> permanent = false;
> ret = sss_ncache_set_group(ts->ctx, permanent, dom, name);
> assert_int_equal(ret, EOK);
> ret = sss_ncache_check_group(ts->ctx, ttl, dom, name);
> assert_int_equal(ret, EEXIST);
> /* test when ttl is -1 with domain name present in database */
> ttl = -1;
> ret = sss_ncache_check_group(ts->ctx, ttl, dom, name);
> assert_int_equal(ret, EEXIST);
>
> +}
>
> +/* @test_sss_ncache_netgr : test following functions
> sss_ncache_check_netgr
> sss_ncache_set_netgr
> */
>
> +static void test_sss_ncache_netgr(void **state)
> +{
> int ret, ttl;
> bool permanent;
> const char *name = NAME;
> struct test_state *ts;
> struct sss_domain_info *dom;
> ttl = LIFETIME;
> ts = talloc_get_type_abort(*state, struct test_state);
> dom = talloc(ts, struct sss_domain_info);
> dom->name = discard_const(NAME);
> /* test when domain name is not present in database */
> dom->case_sensitive = false;
> ret = sss_ncache_check_netgr(ts->ctx, ttl, dom, name);
> assert_int_equal(ret, ENOENT);
> dom->case_sensitive = true;
> ret = sss_ncache_check_netgr(ts->ctx, ttl, dom, name);
> assert_int_equal(ret, ENOENT);
> /* test when domain name is present in database */
> permanent = true;
> ret = sss_ncache_set_netgr(ts->ctx, permanent, dom, name);
> assert_int_equal(ret, EOK);
> ret = sss_ncache_check_netgr(ts->ctx, ttl, dom, name);
> assert_int_equal(ret, EEXIST);
> permanent = false;
> ret = sss_ncache_set_netgr(ts->ctx, permanent, dom, name);
> assert_int_equal(ret, EOK);
> ret = sss_ncache_check_netgr(ts->ctx, ttl, dom, name);
> assert_int_equal(ret, EEXIST);
> /* test when ttl is -1 with domain name present in database */
> ttl = -1;
> ret = sss_ncache_check_netgr(ts->ctx, ttl, dom, name);
> assert_int_equal(ret, EEXIST);
>
> +}
>
> +/* @test_sss_ncache_service_name : test following functions
> sss_ncache_check_service
> sss_ncache_set_service_name
> */
>
> +static void test_sss_ncache_service_name(void **state)
> +{
> int ret, ttl;
> bool permanent;
> const char *name = NAME;
> struct test_state *ts;
> struct sss_domain_info *dom;
> ttl = LIFETIME;
> ts = talloc_get_type_abort(*state, struct test_state);
> dom = talloc(ts, struct sss_domain_info);
> dom->name = discard_const(NAME);
> /* test when domain name and protocol are not present in database */
> dom->case_sensitive = false;
> ret = sss_ncache_check_service(ts->ctx, ttl, dom, name, PROTO);
> assert_int_equal(ret, ENOENT);
> dom->case_sensitive = true;
> ret = sss_ncache_check_service(ts->ctx, ttl, dom, name, PROTO);
> assert_int_equal(ret, ENOENT);
> /* test when domain name and protocol are present in database */
> permanent = true;
> ret = sss_ncache_set_service_name(ts->ctx, permanent, dom, name, PROTO);
> assert_int_equal(ret, EOK);
> ret = sss_ncache_check_service(ts->ctx, ttl, dom, name, PROTO);
> assert_int_equal(ret, EEXIST);
> permanent = false;
> ret = sss_ncache_set_service_name(ts->ctx, permanent, dom, name, PROTO);
> assert_int_equal(ret, EOK);
> ret = sss_ncache_check_service(ts->ctx, ttl, dom, name, PROTO);
> assert_int_equal(ret, EEXIST);
> /* test when ttl is -1 with domain name present in database */
> ttl = -1;
> ret = sss_ncache_check_service(ts->ctx, ttl, dom, name, PROTO);
> assert_int_equal(ret, EEXIST);
>
> +}
>
> +/* @test_sss_ncache_service_port : test following functions
> sss_ncache_check_service_port
> sss_ncache_set_service_port
> */
>
> +static void test_sss_ncache_service_port(void **state)
> +{
> int ret, ttl;
> bool permanent;
> struct test_state *ts;
> struct sss_domain_info *dom;
> ttl = LIFETIME;
> ts = talloc_get_type_abort(*state, struct test_state);
> dom = talloc(ts, struct sss_domain_info);
> dom->name = discard_const(NAME);
> /* test when domain name, port and protocol are not present in database */
> dom->case_sensitive = false;
> ret = sss_ncache_check_service_port(ts->ctx, ttl, dom, (uint16_t)PORT,
>                                    PROTO);
> assert_int_equal(ret, ENOENT);
> dom->case_sensitive = true;
> ret = sss_ncache_check_service_port(ts->ctx, ttl, dom, (uint16_t)PORT,
>                                    PROTO);
> assert_int_equal(ret, ENOENT);
> /* test when domain name, port and protocol are present in database */
> permanent = true;
> ret = sss_ncache_set_service_port(ts->ctx, permanent, dom, (uint16_t)PORT,
>                                  PROTO);
> assert_int_equal(ret, EOK);
> ret = sss_ncache_check_service_port(ts->ctx, ttl, dom, (uint16_t)PORT,
>                                    PROTO);
> assert_int_equal(ret, EEXIST);
> permanent = false;
> ret = sss_ncache_set_service_port(ts->ctx, permanent, dom, (uint16_t)PORT,
>                                  PROTO);
> assert_int_equal(ret, EOK);
> ret = sss_ncache_check_service_port(ts->ctx, ttl, dom, (uint16_t)PORT,
>                                    PROTO);
> assert_int_equal(ret, EEXIST);
> /* test when ttl is -1 with domain name present in database */
> ttl = -1;
> ret = sss_ncache_check_service_port(ts->ctx, ttl, dom, (uint16_t)PORT,
>                                    PROTO);
> assert_int_equal(ret, EEXIST);
>
> +}
>
> +#if 0
> +static void test_sss_ncache_reset_permanent(void **state)
> +{
> int ret;
> struct test_state *ts;
> ts = talloc_get_type_abort(*state, struct test_state);
> ret = sss_ncache_reset_permanent(ts->ctx);
> assert_int_equal(ret, EOK);
>
> +}
> +#endif
>
> +static void test_sss_ncache_prepopulate(void **state)
> +{
> int ret;
> struct test_state *ts;
> struct tevent_context *ev;
> struct sss_nc_ctx *ncache;
> struct sss_test_ctx *tc;
> struct sss_domain_info *dom;
> struct sss_test_conf_param params[] = {
>    { "filter_users", "testuser1" },
>    { "filter_groups", "testgroup1" },
>    { NULL, NULL },
> };
> ts = talloc_get_type_abort(*state, struct test_state);
> ev = tevent_context_init(ts);
> assert_non_null(ev);
> dom = talloc(ts, struct sss_domain_info);
> assert_non_null(dom);
> dom->name = discard_const(NAME);
> ts->nctx = mock_nctx(ts);
> assert_non_null(ts->nctx);
> tc = create_dom_test_ctx(ts, TESTS_PATH, TEST_CONF_DB,
>                         TEST_SYSDB_FILE, TEST_DOM_NAME,
>                         TEST_ID_PROVIDER, params);
> assert_non_null(tc);
> ncache = ts->ctx;
> ts->rctx = mock_rctx(ts, ev, dom, ts->nctx);
> assert_non_null(ts->rctx);
> ret = sss_names_init(ts, tc->confdb, TEST_DOM_NAME, &dom->names);
> assert_int_equal(ret, EOK);
> ret = sss_ncache_prepopulate(ncache, tc->confdb, ts->rctx);
> assert_int_equal(ret, EOK);
> sleep(SHORTSPAN);
> ret = sss_ncache_check_user(ncache, 1, dom, "testuser1");
> assert_int_equal(ret, EEXIST);
> ret = sss_ncache_check_group(ncache, 1, dom, "testgroup1");
> assert_int_equal(ret, EEXIST);

The test(s) don't pass because they cannot pass.
You used function create_dom_test_ctx and param with filter_groups and
filter_users and result of this is configuration:

[domain/nss_test]
filter_users = testuser1
filter_groups = testgroup1

But negative cache reads configuration options from [nss] section.

[nss]
filter_users = testuser1
filter_groups = testgroup1

You will need to write new auxiliary function create_service_test_ctx.
It should not be very difficult, because it will be very similar to
create_dom_test_ctx. SSSD internally uses ldb to read configutration options.
I will try to explain what shoul you change.

struct sss_test_conf_param params[] = {
        { "filter_users", "testuser1" },
        { "filter_groups", "testgroup1" },
        { NULL, NULL },
    };
    <snip>
    tc = create_dom_test_ctx(ts, TESTS_PATH, TEST_CONF_DB,
                             TEST_SYSDB_FILE, TEST_DOM_NAME,
                             TEST_ID_PROVIDER, params);

Previous code snippet will generate:
    sh-4.2$ ldbsearch -H ./test_nss_conf.ldb
    # record 1
    dn: cn=sssd,cn=config
    cn: sssd
    domains: nss_test
    distinguishedName: cn=sssd,cn=config

# record 2
    dn: cn=nss_test,cn=domain,cn=config
    cn: nss_test
    id_provider: ldap
    filter_users: testuser1
    filter_groups: testgroup1
    distinguishedName: cn=nss_test,cn=domain,cn=config

And the proposed function create_service_test_ctx should generate file:
    struct sss_test_conf_param params[] = {
        { "filter_users", "testuser1" },
        { "filter_groups", "testgroup1" },
        { NULL, NULL },
    };
    <snip>
    tc = create_service_test_ctx(ts, TESTS_PATH, TEST_CONF_DB,
                                 TEST_SYSDB_FILE, TEST_DOM_NAME,
                                 TEST_ID_PROVIDER, params);

sh-4.2$ ldbsearch -H ./test_nss_conf.ldb
    # record 1
    dn: cn=sssd,cn=config
    cn: sssd
    services: nss
    distinguishedName: cn=sssd,cn=config

# record 2
    dn: cn=nss,cn=config
    cn: nss
    filter_groups: testgroup1
    filter_users: testuser1
    distinguishedName: cn=nss,cn=config

Command line utility ldbsearch is part of package ldb-tools (in Fedora)

LS

[Jakub Hrozek]

On Mon, Feb 17, 2014 at 04:47:09PM +0100, Lukas Slebodnik wrote:
> The test(s) don't pass because they cannot pass.
> You used function create_dom_test_ctx and param with filter_groups and
> filter_users and result of this is configuration:
>
> [domain/nss_test]
> filter_users = testuser1
> filter_groups = testgroup1
>
> But negative cache reads configuration options from [nss] section.

No, it reads from both actually and domain takes precedence.
> [nss]
> filter_users = testuser1
> filter_groups = testgroup1

There was two different bugs in the test. Because the domain structure
was created with talloc, the ->next pointer was random data and a for
loop that walked through the list of domains crashed.

The second bug was that the test used different domain name for the
domain structure (NAME) and a different name when generating the
configuration (TEST_DOM_NAME) so the config was never read actually.

Attached is a snippet that I used to fix the test. Pallavi, if you
agree, please resubmit your patch with the snippet squashed in.

btw I didn't know what was test_sss_ncache_reset_permanent() about, so I
removed it during my testing..feel free to retain it in the final patch,
but please don't leave if-ed code in the resulting test.

[Pallavi Jha]

Hi,

Updated patch is attached. All tests are passing. Please see the attachment
and let me know if any further changes are to be made.

Thanks!
Pallavi

On 25 February 2014 21:25, Jakub Hrozek  wrote:
> On Mon, Feb 17, 2014 at 04:47:09PM +0100, Lukas Slebodnik wrote:
> > The test(s) don't pass because they cannot pass.
> > You used function create_dom_test_ctx and param with filter_groups and
> > filter_users and result of this is configuration:
> >
> > [domain/nss_test]
> > filter_users = testuser1
> > filter_groups = testgroup1
> >
> > But negative cache reads configuration options from [nss] section.
>
> No, it reads from both actually and domain takes precedence.
> > [nss]
> > filter_users = testuser1
> > filter_groups = testgroup1
>
> There was two different bugs in the test. Because the domain structure
> was created with talloc, the ->next pointer was random data and a for
> loop that walked through the list of domains crashed.
>
> The second bug was that the test used different domain name for the
> domain structure (NAME) and a different name when generating the
> configuration (TEST_DOM_NAME) so the config was never read actually.
>
> Attached is a snippet that I used to fix the test. Pallavi, if you
> agree, please resubmit your patch with the snippet squashed in.
>
> btw I didn't know what was test_sss_ncache_reset_permanent() about, so I
> removed it during my testing..feel free to retain it in the final patch,
> but please don't leave if-ed code in the resulting test.

[Jakub Hrozek]

On Wed, Feb 26, 2014 at 02:14:56AM +0545, Pallavi Jha wrote:
> Hi,
>
> Updated patch is attached. All tests are passing. Please see the attachment
> and let me know if any further changes are to be made.

I found some more smallish bugs. Sorry I haven't seen them sooner. See
inline, I think this would be the last iteration :-)
> Thanks!
> Pallavi
>
> On 25 February 2014 21:25, Jakub Hrozek  wrote:
> > On Mon, Feb 17, 2014 at 04:47:09PM +0100, Lukas Slebodnik wrote:
> > > The test(s) don't pass because they cannot pass.
> > > You used function create_dom_test_ctx and param with filter_groups and
> > > filter_users and result of this is configuration:
> > >
> > > [domain/nss_test]
> > > filter_users = testuser1
> > > filter_groups = testgroup1
> > >
> > > But negative cache reads configuration options from [nss] section.
> >
> > No, it reads from both actually and domain takes precedence.
> > > [nss]
> > > filter_users = testuser1
> > > filter_groups = testgroup1
> >
> > There was two different bugs in the test. Because the domain structure
> > was created with talloc, the ->next pointer was random data and a for
> > loop that walked through the list of domains crashed.
> >
> > The second bug was that the test used different domain name for the
> > domain structure (NAME) and a different name when generating the
> > configuration (TEST_DOM_NAME) so the config was never read actually.
> >
> > Attached is a snippet that I used to fix the test. Pallavi, if you
> > agree, please resubmit your patch with the snippet squashed in.
> >
> > btw I didn't know what was test_sss_ncache_reset_permanent() about, so I
> > removed it during my testing..feel free to retain it in the final patch,
> > but please don't leave if-ed code in the resulting test.
> From 579d8a60cb1e950afa9de811adc13be53fa8ba40 Mon Sep 17 00:00:00 2001
> From: Pallavi Jha 
> Date: Sat, 18 Jan 2014 14:36:25 +0545
> Subject: [PATCH] Unit-test-for-negcache-module-added

[snip]
> +/* Mock NSS structure */
> +struct nss_ctx *
> +mock_nctx(TALLOC_CTX *mem_ctx)

Please make this function static.
> +{
> struct nss_ctx *nctx;
> errno_t ret;
> enum idmap_error_code err;
> nctx = talloc_zero(mem_ctx, struct nss_ctx);
> if (!nctx) {
>    return NULL;
> }
> ret = sss_ncache_init(nctx, &nctx->ncache);
> if (ret != EOK) {
>    talloc_free(nctx);
>    return NULL;
> }
> nctx->neg_timeout = 10;
> nctx->pwfield = discard_const("*");
> err = sss_idmap_init(sss_idmap_talloc, nctx, sss_idmap_talloc_free,
>                     &nctx->idmap_ctx);
> if (err != IDMAP_SUCCESS) {
>    DEBUG(SSSDBG_FATAL_FAILURE, ("sss_idmap_init failed.\n"));
>    talloc_free(nctx);
>    return NULL;
> }
> return nctx;
>
> +}
>
> +/* Mock a responder context */
> +struct resp_ctx *
> +mock_rctx(TALLOC_CTX *mem_ctx,
>      struct tevent_context *ev,
>      struct sss_domain_info *domains,
>      void *pvt_ctx)

This function is the same as mock_rctx() from common_mock.c. I think you
can simply remove this one and keep using the shared one.
> +{
> struct resp_ctx *rctx;
> errno_t ret;
> rctx = talloc_zero(mem_ctx, struct resp_ctx);
> if (!rctx) return NULL;
> ret = sss_hash_create(rctx, 30, &rctx->dp_request_table);
> if (ret != EOK) {
>    talloc_free(rctx);
>    return NULL;
> }
> rctx->ev = ev;
> rctx->domains = domains;
> rctx->pvt_ctx = pvt_ctx;
> return rctx;
>
> +}
>
> +struct test_state {
> struct sss_nc_ctx *ctx;
> struct nss_ctx *nctx;
> struct resp_ctx *rctx;
>
> +};
>
> +static void setup(void **state)
> +{
> int ret;
> struct test_state *ts;
> ts = talloc(NULL, struct test_state);
> assert_non_null(ts);
> ret  = sss_ncache_init(ts, &ts->ctx);
> assert_int_equal(ret, EOK);
> assert_non_null(ts->ctx);
> *state = (void *)ts;
>
> +}
>
> +static void teardown(void **state)
> +{
> struct test_state *ts = talloc_get_type_abort(*state, struct test_state);
> talloc_free(ts);
>
> +}
>
> +static void test_sss_ncache_init(void **state)
> +{
> int ret;
> TALLOC_CTX *memctx;
> struct sss_nc_ctx *ctx;
> memctx = talloc_new(NULL);
> assert_non_null(memctx);
> ret = sss_ncache_init(memctx, &ctx );
> assert_int_equal(ret, errno);

I think this comparison with errno is not needed, as the next one checks
for EOK. I think the first one mostly works only by accident because
errno happens to be zero.
> assert_int_equal(ret, EOK);
> assert_non_null(ctx);
> talloc_free(memctx);
>
> +}

That's all, the rest looks good to me.

[Pallavi Jha]

Hi,

Few changes mentioned in earlier mail are added and the required comment is
also added for mock_rctx(), as was discussed with Jakub. Kindly review the
patch.

Thanks!
Pallavi

On 28 February 2014 15:28, Jakub Hrozek  wrote:
> On Wed, Feb 26, 2014 at 02:14:56AM +0545, Pallavi Jha wrote:
> > Hi,
> >
> > Updated patch is attached. All tests are passing. Please see the
>
> attachment
> > and let me know if any further changes are to be made.
>
> I found some more smallish bugs. Sorry I haven't seen them sooner. See
> inline, I think this would be the last iteration :-)
> > Thanks!
> > Pallavi
> >
> > On 25 February 2014 21:25, Jakub Hrozek  wrote:
> > > On Mon, Feb 17, 2014 at 04:47:09PM +0100, Lukas Slebodnik wrote:
> > > > The test(s) don't pass because they cannot pass.
> > > > You used function create_dom_test_ctx and param with filter_groups
>
> and
> > > > filter_users and result of this is configuration:
> > > >
> > > > [domain/nss_test]
> > > > filter_users = testuser1
> > > > filter_groups = testgroup1
> > > >
> > > > But negative cache reads configuration options from [nss] section.
> > >
> > > No, it reads from both actually and domain takes precedence.
> > > > [nss]
> > > > filter_users = testuser1
> > > > filter_groups = testgroup1
> > >
> > > There was two different bugs in the test. Because the domain structure
> > > was created with talloc, the ->next pointer was random data and a for
> > > loop that walked through the list of domains crashed.
> > >
> > > The second bug was that the test used different domain name for the
> > > domain structure (NAME) and a different name when generating the
> > > configuration (TEST_DOM_NAME) so the config was never read actually.
> > >
> > > Attached is a snippet that I used to fix the test. Pallavi, if you
> > > agree, please resubmit your patch with the snippet squashed in.
> > >
> > > btw I didn't know what was test_sss_ncache_reset_permanent() about, so
>
> I
> > > removed it during my testing..feel free to retain it in the final
>
> patch,
> > > but please don't leave if-ed code in the resulting test.
> > From 579d8a60cb1e950afa9de811adc13be53fa8ba40 Mon Sep 17 00:00:00 2001
> > From: Pallavi Jha 
> > Date: Sat, 18 Jan 2014 14:36:25 +0545
> > Subject: [PATCH] Unit-test-for-negcache-module-added
>
> [snip]
> > +/* Mock NSS structure */
> > +struct nss_ctx *
> > +mock_nctx(TALLOC_CTX *mem_ctx)
>
> Please make this function static.
> > +{
> > struct nss_ctx *nctx;
> > errno_t ret;
> > enum idmap_error_code err;
> > nctx = talloc_zero(mem_ctx, struct nss_ctx);
> > if (!nctx) {
> >    return NULL;
> > }
> > ret = sss_ncache_init(nctx, &nctx->ncache);
> > if (ret != EOK) {
> >    talloc_free(nctx);
> >    return NULL;
> > }
> > nctx->neg_timeout = 10;
> > nctx->pwfield = discard_const("*");
> > err = sss_idmap_init(sss_idmap_talloc, nctx, sss_idmap_talloc_free,
> >                     &nctx->idmap_ctx);
> > if (err != IDMAP_SUCCESS) {
> >    DEBUG(SSSDBG_FATAL_FAILURE, ("sss_idmap_init failed.\n"));
> >    talloc_free(nctx);
> >    return NULL;
> > }
> > return nctx;
> >
> > +}
> >
> > +/* Mock a responder context */
> > +struct resp_ctx *
> > +mock_rctx(TALLOC_CTX *mem_ctx,
> >      struct tevent_context *ev,
> >      struct sss_domain_info *domains,
> >      void *pvt_ctx)
>
> This function is the same as mock_rctx() from common_mock.c. I think you
> can simply remove this one and keep using the shared one.
> > +{
> > struct resp_ctx *rctx;
> > errno_t ret;
> > rctx = talloc_zero(mem_ctx, struct resp_ctx);
> > if (!rctx) return NULL;
> > ret = sss_hash_create(rctx, 30, &rctx->dp_request_table);
> > if (ret != EOK) {
> >    talloc_free(rctx);
> >    return NULL;
> > }
> > rctx->ev = ev;
> > rctx->domains = domains;
> > rctx->pvt_ctx = pvt_ctx;
> > return rctx;
> >
> > +}
> >
> > +struct test_state {
> > struct sss_nc_ctx *ctx;
> > struct nss_ctx *nctx;
> > struct resp_ctx *rctx;
> >
> > +};
> >
> > +static void setup(void **state)
> > +{
> > int ret;
> > struct test_state *ts;
> > ts = talloc(NULL, struct test_state);
> > assert_non_null(ts);
> > ret  = sss_ncache_init(ts, &ts->ctx);
> > assert_int_equal(ret, EOK);
> > assert_non_null(ts->ctx);
> > *state = (void *)ts;
> >
> > +}
> >
> > +static void teardown(void **state)
> > +{
> > struct test_state *ts = talloc_get_type_abort(*state, struct
>
> test_state);
> > talloc_free(ts);
> >
> > +}
> >
> > +static void test_sss_ncache_init(void **state)
> > +{
> > int ret;
> > TALLOC_CTX *memctx;
> > struct sss_nc_ctx *ctx;
> > memctx = talloc_new(NULL);
> > assert_non_null(memctx);
> > ret = sss_ncache_init(memctx, &ctx );
> > assert_int_equal(ret, errno);
>
> I think this comparison with errno is not needed, as the next one checks
> for EOK. I think the first one mostly works only by accident because
> errno happens to be zero.
> > assert_int_equal(ret, EOK);
> > assert_non_null(ctx);
> > talloc_free(memctx);
> >
> > +}
>
> That's all, the rest looks good to me.

[Jakub Hrozek]

On Wed, Mar 05, 2014 at 04:24:19PM +0545, Pallavi Jha wrote:
> Hi,
>
> Few changes mentioned in earlier mail are added and the required comment is
> also added for mock_rctx(), as was discussed with Jakub. Kindly review the
> patch.
>
> Thanks!
> Pallavi

The patch now looks good and the test passes, so I'll ACK it.

Please note that when I applied the patch, the test failed, complaining
about a failure on line 603, which would mean create_dom_test_ctx()
failed. But I haven't been able to reproduce the error again, subsequent
runs went OK for me. If someone else sees the failure, please holler.

[Jakub Hrozek]

On Wed, Mar 05, 2014 at 10:33:23PM +0100, Jakub Hrozek wrote:
> On Wed, Mar 05, 2014 at 04:24:19PM +0545, Pallavi Jha wrote:
> > Hi,
> >
> > Few changes mentioned in earlier mail are added and the required comment is
> > also added for mock_rctx(), as was discussed with Jakub. Kindly review the
> > patch.
> >
> > Thanks!
> > Pallavi
>
> The patch now looks good and the test passes, so I'll ACK it.
>
> Please note that when I applied the patch, the test failed, complaining
> about a failure on line 603, which would mean create_dom_test_ctx()
> failed. But I haven't been able to reproduce the error again, subsequent
> runs went OK for me. If someone else sees the failure, please holler.

Pushed to master.

[Lukas Slebodnik]

On (05/03/14 22:33), Jakub Hrozek wrote:
> On Wed, Mar 05, 2014 at 04:24:19PM +0545, Pallavi Jha wrote:
> > Hi,
> >
> > Few changes mentioned in earlier mail are added and the required comment is
> > also added for mock_rctx(), as was discussed with Jakub. Kindly review the
> > patch.
> >
> > Thanks!
> > Pallavi
>
> The patch now looks good and the test passes, so I'll ACK it.
>
> Please note that when I applied the patch, the test failed, complaining
> about a failure on line 603, which would mean create_dom_test_ctx()
> failed. But I haven't been able to reproduce the error again, subsequent
> runs went OK for me. If someone else sees the failure, please holler.

I don't know why, but I am able to reproduce it :-(

(Thu Mar  6 09:23:04:051345 2014) [sssd] [ldb] (0x0020): Unable to open tdb 'tests_nss/test_nss_conf.ldb'
(Thu Mar  6 09:23:04:051412 2014) [sssd] [ldb] (0x0020): Failed to connect to 'tests_nss/test_nss_conf.ldb' with backend 'tdb': Unable to open tdb 'tests_nss/test_nss_conf.ldb'
(Thu Mar  6 09:23:04:051425 2014) [sssd] [confdb_init] (0x0010): Unable to open config database [tests_nss/test_nss_conf.ldb]
(Thu Mar  6 09:23:04:051446 2014) [sssd] [create_dom_test_ctx] (0x0020): confdb_init failed: 5
tc
../sssd/src/tests/cmocka/test_negcache.c:603: error: Failure!
[  FAILED  ] test_sss_ncache_prepopulate

BTW The test will not fail if I run "make rpms"

LS

[Jakub Hrozek]

On Thu, Mar 06, 2014 at 09:26:40AM +0100, Lukas Slebodnik wrote:
> On (05/03/14 22:33), Jakub Hrozek wrote:
> > On Wed, Mar 05, 2014 at 04:24:19PM +0545, Pallavi Jha wrote:
> > > Hi,
> > >
> > > Few changes mentioned in earlier mail are added and the required comment is
> > > also added for mock_rctx(), as was discussed with Jakub. Kindly review the
> > > patch.
> > >
> > > Thanks!
> > > Pallavi
> >
> > The patch now looks good and the test passes, so I'll ACK it.
> >
> > Please note that when I applied the patch, the test failed, complaining
> > about a failure on line 603, which would mean create_dom_test_ctx()
> > failed. But I haven't been able to reproduce the error again, subsequent
> > runs went OK for me. If someone else sees the failure, please holler.
>
> I don't know why, but I am able to reproduce it :-(
>
> (Thu Mar  6 09:23:04:051345 2014) [sssd] [ldb] (0x0020): Unable to open tdb 'tests_nss/test_nss_conf.ldb'
> (Thu Mar  6 09:23:04:051412 2014) [sssd] [ldb] (0x0020): Failed to connect to 'tests_nss/test_nss_conf.ldb' with backend 'tdb': Unable to open tdb 'tests_nss/test_nss_conf.ldb'
> (Thu Mar  6 09:23:04:051425 2014) [sssd] [confdb_init] (0x0010): Unable to open config database [tests_nss/test_nss_conf.ldb]
> (Thu Mar  6 09:23:04:051446 2014) [sssd] [create_dom_test_ctx] (0x0020): confdb_init failed: 5
> tc
> ../sssd/src/tests/cmocka/test_negcache.c:603: error: Failure!
> [  FAILED  ] test_sss_ncache_prepopulate
>
> BTW The test will not fail if I run "make rpms"
>
> LS

I was actually hoping you could reproduce this, you have a way with
strange bugs :-)

But on my machine, all tests still pass. Provided you can reproduce
this, can you also send a patch?

[Lukas Slebodnik]

On (06/03/14 09:50), Jakub Hrozek wrote:
> On Thu, Mar 06, 2014 at 09:26:40AM +0100, Lukas Slebodnik wrote:
> > On (05/03/14 22:33), Jakub Hrozek wrote:
> > > On Wed, Mar 05, 2014 at 04:24:19PM +0545, Pallavi Jha wrote:
> > > > Hi,
> > > >
> > > > Few changes mentioned in earlier mail are added and the required comment is
> > > > also added for mock_rctx(), as was discussed with Jakub. Kindly review the
> > > > patch.
> > > >
> > > > Thanks!
> > > > Pallavi
> > >
> > > The patch now looks good and the test passes, so I'll ACK it.
> > >
> > > Please note that when I applied the patch, the test failed, complaining
> > > about a failure on line 603, which would mean create_dom_test_ctx()
> > > failed. But I haven't been able to reproduce the error again, subsequent
> > > runs went OK for me. If someone else sees the failure, please holler.
> >
> > I don't know why, but I am able to reproduce it :-(
> >
> > (Thu Mar  6 09:23:04:051345 2014) [sssd] [ldb] (0x0020): Unable to open tdb 'tests_nss/test_nss_conf.ldb'
> > (Thu Mar  6 09:23:04:051412 2014) [sssd] [ldb] (0x0020): Failed to connect to 'tests_nss/test_nss_conf.ldb' with backend 'tdb': Unable to open tdb 'tests_nss/test_nss_conf.ldb'
> > (Thu Mar  6 09:23:04:051425 2014) [sssd] [confdb_init] (0x0010): Unable to open config database [tests_nss/test_nss_conf.ldb]
> > (Thu Mar  6 09:23:04:051446 2014) [sssd] [create_dom_test_ctx] (0x0020): confdb_init failed: 5
> > tc
> > ../sssd/src/tests/cmocka/test_negcache.c:603: error: Failure!
> > [  FAILED  ] test_sss_ncache_prepopulate
> >
> > BTW The test will not fail if I run "make rpms"
> >
> > LS
>
> I was actually hoping you could reproduce this, you have a way with
> strange bugs :-)
>
> But on my machine, all tests still pass. Provided you can reproduce
> this, can you also send a patch?

done.

LS

[Michal Židek]

On 03/06/2014 01:24 PM, Lukas Slebodnik wrote:
> On (06/03/14 09:50), Jakub Hrozek wrote:
> > On Thu, Mar 06, 2014 at 09:26:40AM +0100, Lukas Slebodnik wrote:
> > > On (05/03/14 22:33), Jakub Hrozek wrote:
> > > > On Wed, Mar 05, 2014 at 04:24:19PM +0545, Pallavi Jha wrote:
> > > > > Hi,
> > > > >
> > > > > Few changes mentioned in earlier mail are added and the required comment is
> > > > > also added for mock_rctx(), as was discussed with Jakub. Kindly review the
> > > > > patch.
> > > > >
> > > > > Thanks!
> > > > > Pallavi
> > > >
> > > > The patch now looks good and the test passes, so I'll ACK it.
> > > >
> > > > Please note that when I applied the patch, the test failed, complaining
> > > > about a failure on line 603, which would mean create_dom_test_ctx()
> > > > failed. But I haven't been able to reproduce the error again, subsequent
> > > > runs went OK for me. If someone else sees the failure, please holler.
> > >
> > > I don't know why, but I am able to reproduce it :-(
> > >
> > > (Thu Mar  6 09:23:04:051345 2014) [sssd] [ldb] (0x0020): Unable to open tdb 'tests_nss/test_nss_conf.ldb'
> > > (Thu Mar  6 09:23:04:051412 2014) [sssd] [ldb] (0x0020): Failed to connect to 'tests_nss/test_nss_conf.ldb' with backend 'tdb': Unable to open tdb 'tests_nss/test_nss_conf.ldb'
> > > (Thu Mar  6 09:23:04:051425 2014) [sssd] [confdb_init] (0x0010): Unable to open config database [tests_nss/test_nss_conf.ldb]
> > > (Thu Mar  6 09:23:04:051446 2014) [sssd] [create_dom_test_ctx] (0x0020): confdb_init failed: 5
> > > tc
> > > ../sssd/src/tests/cmocka/test_negcache.c:603: error: Failure!
> > > [  FAILED  ] test_sss_ncache_prepopulate
> > >
> > > BTW The test will not fail if I run "make rpms"
> > >
> > > LS
> >
> > I was actually hoping you could reproduce this, you have a way with
> > strange bugs :-)
> >
> > But on my machine, all tests still pass. Provided you can reproduce
> > this, can you also send a patch?
>
> done.
>
> LS

ACK.

There are no more TESTS_PATH collisions in the code now.

Michal

[Jakub Hrozek]

On Thu, Mar 06, 2014 at 05:25:47PM +0100, Michal Židek wrote:
> On 03/06/2014 01:24 PM, Lukas Slebodnik wrote:
> > On (06/03/14 09:50), Jakub Hrozek wrote:
> > > On Thu, Mar 06, 2014 at 09:26:40AM +0100, Lukas Slebodnik wrote:
> > > > On (05/03/14 22:33), Jakub Hrozek wrote:
> > > > > On Wed, Mar 05, 2014 at 04:24:19PM +0545, Pallavi Jha wrote:
> > > > > > Hi,
> > > > > >
> > > > > > Few changes mentioned in earlier mail are added and the required comment is
> > > > > > also added for mock_rctx(), as was discussed with Jakub. Kindly review the
> > > > > > patch.
> > > > > >
> > > > > > Thanks!
> > > > > > Pallavi
> > > > >
> > > > > The patch now looks good and the test passes, so I'll ACK it.
> > > > >
> > > > > Please note that when I applied the patch, the test failed, complaining
> > > > > about a failure on line 603, which would mean create_dom_test_ctx()
> > > > > failed. But I haven't been able to reproduce the error again, subsequent
> > > > > runs went OK for me. If someone else sees the failure, please holler.
> > > >
> > > > I don't know why, but I am able to reproduce it :-(
> > > >
> > > > (Thu Mar  6 09:23:04:051345 2014) [sssd] [ldb] (0x0020): Unable to open tdb 'tests_nss/test_nss_conf.ldb'
> > > > (Thu Mar  6 09:23:04:051412 2014) [sssd] [ldb] (0x0020): Failed to connect to 'tests_nss/test_nss_conf.ldb' with backend 'tdb': Unable to open tdb 'tests_nss/test_nss_conf.ldb'
> > > > (Thu Mar  6 09:23:04:051425 2014) [sssd] [confdb_init] (0x0010): Unable to open config database [tests_nss/test_nss_conf.ldb]
> > > > (Thu Mar  6 09:23:04:051446 2014) [sssd] [create_dom_test_ctx] (0x0020): confdb_init failed: 5
> > > > tc
> > > > ../sssd/src/tests/cmocka/test_negcache.c:603: error: Failure!
> > > > [  FAILED  ] test_sss_ncache_prepopulate
> > > >
> > > > BTW The test will not fail if I run "make rpms"
> > > >
> > > > LS
> > >
> > > I was actually hoping you could reproduce this, you have a way with
> > > strange bugs :-)
> > >
> > > But on my machine, all tests still pass. Provided you can reproduce
> > > this, can you also send a patch?
> >
> > done.
> >
> > LS
>
> ACK.
>
> There are no more TESTS_PATH collisions in the code now.
>
> Michal

Thanks for the patch. I'm still not able to reproduce the failure
anymore, but the patch makes sense and works fine.

Pushed to master.

[Lukas Slebodnik]

On (25/02/14 16:40), Jakub Hrozek wrote:
> On Mon, Feb 17, 2014 at 04:47:09PM +0100, Lukas Slebodnik wrote:
> > The test(s) don't pass because they cannot pass.
> > You used function create_dom_test_ctx and param with filter_groups and
> > filter_users and result of this is configuration:
> >
> > [domain/nss_test]
> > filter_users = testuser1
> > filter_groups = testgroup1
> >
> > But negative cache reads configuration options from [nss] section.
>
> No, it reads from both actually and domain takes precedence.

In this case, we should update manual pages.

man sssd.conf
    -> NSS configuration options
       filter_users, filter_groups (string)
           Exclude certain users from being fetched from the sss NSS database.
           This is particularly useful for system accounts. This option can
           also be set per-domain or include fully-qualified names
            to filter only users from the particular domain.

Default: root

I apologize for confusion.
> > [nss]
> > filter_users = testuser1
> > filter_groups = testgroup1
>
> There was two different bugs in the test. Because the domain structure
> was created with talloc, the ->next pointer was random data and a for
> loop that walked through the list of domains crashed.

Nice catch.
I was lucky, because test din't crash in my case.

LS

[Jakub Hrozek]

On Tue, Feb 25, 2014 at 10:00:00PM +0100, Lukas Slebodnik wrote:
> On (25/02/14 16:40), Jakub Hrozek wrote:
> > On Mon, Feb 17, 2014 at 04:47:09PM +0100, Lukas Slebodnik wrote:
> > > The test(s) don't pass because they cannot pass.
> > > You used function create_dom_test_ctx and param with filter_groups and
> > > filter_users and result of this is configuration:
> > >
> > > [domain/nss_test]
> > > filter_users = testuser1
> > > filter_groups = testgroup1
> > >
> > > But negative cache reads configuration options from [nss] section.
> >
> > No, it reads from both actually and domain takes precedence.
>
> In this case, we should update manual pages.
>
> man sssd.conf
>     -> NSS configuration options
>        filter_users, filter_groups (string)
>            Exclude certain users from being fetched from the sss NSS database.
>            This is particularly useful for system accounts. This option can
>            also be set per-domain or include fully-qualified names
>             to filter only users from the particular domain.
>        Default: root
>
> I apologize for confusion.

No problem, I didn't realize that either until I checked out the backtrace
from the failing test.

Can you file a ticket (or just send a patch :-)) ?

[Jakub Hrozek]

On Tue, Feb 25, 2014 at 10:31:50PM +0100, Jakub Hrozek wrote:
> On Tue, Feb 25, 2014 at 10:00:00PM +0100, Lukas Slebodnik wrote:
> > On (25/02/14 16:40), Jakub Hrozek wrote:
> > > On Mon, Feb 17, 2014 at 04:47:09PM +0100, Lukas Slebodnik wrote:
> > > > The test(s) don't pass because they cannot pass.
> > > > You used function create_dom_test_ctx and param with filter_groups and
> > > > filter_users and result of this is configuration:
> > > >
> > > > [domain/nss_test]
> > > > filter_users = testuser1
> > > > filter_groups = testgroup1
> > > >
> > > > But negative cache reads configuration options from [nss] section.
> > >
> > > No, it reads from both actually and domain takes precedence.
> >
> > In this case, we should update manual pages.
> >
> > man sssd.conf
> >     -> NSS configuration options
> >        filter_users, filter_groups (string)
> >            Exclude certain users from being fetched from the sss NSS database.
> >            This is particularly useful for system accounts. This option can
> >            also be set per-domain or include fully-qualified names
> >             to filter only users from the particular domain.
> >        Default: root
> >
> > I apologize for confusion.
>
> No problem, I didn't realize that either until I checked out the backtrace
> from the failing test.
>
> Can you file a ticket (or just send a patch :-)) ?

https://fedorahosted.org/sssd/ticket/2259