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