On Wed, Dec 09, 2015 at 04:27:29PM +0100, Pavel Reichl wrote:
On 12/08/2015 05:27 PM, Pavel Reichl wrote:
>On 12/02/2015 02:18 PM, Pavel Reichl wrote:
>>Hello,
>>
>>I decided to share this design document although it still a work in progress.
Attached patches are just prove of concept and are very much work in progress. So far
patches also defers from design in order in which secondary slices are generated.
>>
>>Thanks for feedback on this early state of effort.
>>
>>Bye.
>>
>>https://fedorahosted.org/sssd/wiki/IdmapAutoAssignNewSlices
>>
>>
>>_______________________________________________
>>sssd-devel mailing list
>>sssd-devel(a)lists.fedorahosted.org
>>https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
>>
>Hello, attached patches are still work in progress - I mainly addressed Sumit's
comments focused on:
>1) Changing secondary slices from arrays to link list (to make adding new slices easy
in sid_to_unix()).
>2) Added new function sss_idmap_add_auto_domain_ex() which adds secondary slices -
it's not a good idea to add secondary slices in sss_idmap_add_domain_ex() because
it's called by IPA which won't use them.
>3) Changing sss_idmap_sid_to_unix() to always generate new secondary slice if RID is
out of scope of currently allocated secondary slices.
>4) And other smaller fixes.
>
>Also, option ldap_idmap_extra_slice_max was added and patch was refactored to remove
code duplication.
>
>I still need to increase code coverage and do more testing.
>
>Bye.
>
>
>_______________________________________________
>sssd-devel mailing list
>sssd-devel(a)lists.fedorahosted.org
>https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
>
I did some testing and amended the patch a little.
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?
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].
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.
I'll continue reviewing and testing ...
bye,
Sumit
>
> You can see diff here:
>
> >diff --git a/src/lib/idmap/sss_idmap.c b/src/lib/idmap/sss_idmap.c
> >index a92066b..c4ae811 100644
> >--- a/src/lib/idmap/sss_idmap.c
> >+++ b/src/lib/idmap/sss_idmap.c
> >@@ -562,6 +562,7 @@ generate_slice(struct sss_idmap_ctx *ctx, char *slice_name,
uint32_t first_rid,
> >
> > err = sss_idmap_calculate_range(ctx, slice_name, NULL, &tmp_range);
> > if (err != IDMAP_SUCCESS) {
> >+ ctx->free_func(slice, ctx->alloc_pvt);
> > return err;
> > }
> >
> >@@ -718,17 +719,18 @@ sss_idmap_add_auto_domain_ex(struct sss_idmap_ctx *ctx,
> > rid += ctx->idmap_opts.rangesize;
> > err = get_sec_slices(ctx, domain_name, rid,
> > &ctx->idmap_domain_info->sec_ranges);
> >- if (err != IDMAP_SUCCESS) {
> >+ if (err == IDMAP_SUCCESS) {
> >+ ctx->idmap_domain_info->use_sec_ranges = true;
> >+ } else {
> > /* Running out of slices for secondary mapping is a non-fatal
> > * problem. */
> >- if (err != IDMAP_OUT_OF_SLICES) {
> >- return err;
> >+ if (err == IDMAP_OUT_OF_SLICES) {
> >+ err = IDMAP_SUCCESS;
> > }
> >+ ctx->idmap_domain_info->use_sec_ranges = false;
> > }
> >
> >- ctx->idmap_domain_info->use_sec_ranges = true;
> >-
> >- return IDMAP_SUCCESS;
> >+ return err;
> > }
> >
> > enum idmap_error_code sss_idmap_add_domain(struct sss_idmap_ctx *ctx,
> >@@ -874,7 +876,7 @@ enum idmap_error_code sss_idmap_sid_to_unix(struct
sss_idmap_ctx *ctx,
> >
> > /* Try secondary slices */
> > if (matched_dom != NULL && matched_dom->use_sec_ranges) {
> >- struct idmap_range_params *last_slide;
> >+ struct idmap_range_params *last_slide = NULL;
> > struct idmap_range_params *new_slice;
> > enum idmap_error_code err;
> >
> >@@ -898,7 +900,11 @@ enum idmap_error_code sss_idmap_sid_to_unix(struct
sss_idmap_ctx *ctx,
> > return err;
> > }
> >
> >- last_slide->next = new_slice;
> >+ if (last_slide == NULL) {
> >+ matched_dom->sec_ranges = new_slice;
> >+ } else {
> >+ last_slide->next = new_slice;
> >+ }
> >
> > if (comp_id(new_slice, rid, _id)) {
> > return IDMAP_SUCCESS;
>