Hello Sumit, thanks for comments. I attached rebased patch with fixes as you proposed.
On 12/10/2015 01:23 PM, Sumit Bose wrote:
Some unit-test where failing for me because of an issue in cleaning
up
the secondary slices. 'it = it->next' does not work because it was just
freed in the body of the loop. I fixed it with the change below, but
maybe a do-while loop would be even nicer here?
Sorry to hear that. I fixed that in attached patch set.
Which test failed for you? make check is always passing in my environment.
diff --git a/src/lib/idmap/sss_idmap.c b/src/lib/idmap/sss_idmap.c
index c4ae811..7ab5c66 100644
--- a/src/lib/idmap/sss_idmap.c
+++ b/src/lib/idmap/sss_idmap.c
@@ -210,6 +210,7 @@ static void sss_idmap_free_domain(struct
sss_idmap_ctx *ctx,
struct idmap_domain_info *dom)
{
struct idmap_range_params *it;
+ struct idmap_range_params *next;
if (ctx == NULL || dom == NULL) {
return;
@@ -217,7 +218,8 @@ static void sss_idmap_free_domain(struct
sss_idmap_ctx *ctx,
ctx->free_func(dom->range_params.range_id, ctx->alloc_pvt);
/* Free list of secondary ranges */
- for (it = dom->sec_ranges; it != NULL; it = it->next) {
+ for (it = dom->sec_ranges; it != NULL; it = next) {
+ next = it->next;
ctx->free_func(it->range_id, ctx->alloc_pvt);
ctx->free_func(it, ctx->alloc_pvt);
}
While checking and playing with the test I came across
+ /* Todo - Add tests */
+ /* Add size of primary slice for first_rid of secondary slices. */
+ rid += ctx->idmap_opts.rangesize;
in sss_idmap_add_auto_domain_ex(). Does the 'Todo' only refer to 'Add
tests' or to 'Add size of primary slice for first_rid of secondary
slices.' as well? I'm asking because you really have to use the size of
the primary slice and not ctx->idmap_opts.rangesize here because the
primary range is given as a parameter to sss_idmap_add_auto_domain_ex()
and the size might be different then ctx->idmap_opts.rangesize. E.g. in
the unit-test the range is [1234,9876].
I removed the misleading comment about adding tests. I have no intention of changing how
'rid' is set currently.
About sss_idmap_add_auto_domain_ex(), I think having auto-generated
secondary ranges with external_mapping==true make no sense. I would
suggest to just create the primary range in this case.
OK, done.
I'll continue reviewing and testing ...
Thanks!