[SSSD] [Patch] Cmocka unit test for negcache.c module

Pallavi Jha pallavikumarijha at gmail.com
Fri Feb 14 19:58:33 UTC 2014


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 at 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!
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140215/9917b6c3/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Unit-test-for-negcache-module-added(1).patch
Type: text/x-patch
Size: 20864 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140215/9917b6c3/attachment.bin>


More information about the sssd-devel mailing list