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(a)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!