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 <jhrozek@redhat.com> 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!