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