[SSSD] [PATCH 1/2] libsss_idmap: function to calculate range

Michal Židek mzidek at redhat.com
Wed Apr 24 11:34:07 UTC 2013


On 04/24/2013 10:08 AM, Sumit Bose wrote:
> On Tue, Apr 23, 2013 at 01:50:35PM -0400, Simo Sorce wrote:
>> On Tue, 2013-04-23 at 11:26 -0400, Stephen Gallagher wrote:
>>> -----BEGIN PGP SIGNED MESSAGE-----
>>> Hash: SHA1
>>>
>>> On 04/23/2013 10:37 AM, Michal Židek wrote:
>>>> Also one more question. How should I amend the -version-info value
>>>> for libsss_idmap in Makefile.am? Currently it is 0:1:0. Should it
>>>> be 1:0:0 now?
>>>>
>>>
>>> With your patch as written, yes you will need to change it to 1:0:0
>>> (because you modified a public function and two public data
>>> structures). This will effect a soname bump and require that consumers
>>> rebuild against this.
>>>
>>> It also means that you will need to make an announcement on
>>> Fedora-devel about the soname bump and make sure to coordinate the
>>> update so that all projects relying on this library rebuild together.
>>> Please do not do this in Fedora 18.
>>>
>>> A quick repoquery suggests that only SSSD and FreeIPA are consuming
>>> this library in Fedora (as expected), but it's not impossible that
>>> there are third-party tools out there consuming the library for their
>>> own purposes, so please be sure to announce.
>>>
>>> Unfortunately, I don't really see any way of avoiding the bump.
>>
>> Except, do we really need to make this incompatible change ?
>
> I also would recommend to only add new functions for now.
>>
>> If I read the patch correctly idmap_opts are only needed in
>> sss_idmap_calculate_range() which is a new function, so we could avoid
>> passing idmap_opts to ss_idmap_init and instead pass the struct directly
>> to sss_idmap_calculate_range().
>
> This would be possible, but I think it would be nice if all options can
> be set during the initialization phase. With this we can avoid to carry
> around the idmap_opts in some other context or to read the global
> configuration data whenever a new range must be found. So my suggestion
> would be to add a call like sss_idmap_set_autorange_opts() or similar
> which can be used after sss_idmap_init() to add the options to the idmap
> context (or individual setters, see below). A trac ticket can be opened
> to enhance sss_idmap_init() for some later release.
>
>>
>> The other change is slice_num stored in the range, but do we really need
>> to store it ? Or could we implement overrides as options on idmap_opts ?
>> After all I do not really see why a "range" would include a slice
>> number.
>
> I agree with Simo here as well. I think the slice_num is only used to
> check if another range is already using this slice. I think this check
> should be changes to check if the selected slice overlaps with any
> existing range/slice. Because otherwise this check would be limited to
> the automatic generated ranges. But we might need to support
> pre-configured ranges, which might not related to any specific slice in
> order to support some migration scenarios.

Yes, this make sense and should be changed.

>
>>
>> I think we can make a change that only adds functionality w/o changing
>> existing functions.
>>
>> Another question I have is, why do we have getters if the idmap_opts
>> structure is public ?
>>
>> I would either make it opaque and have a creation function or not have
>> setters.
>


Well, the idmap_opts is public, but it is only used to reduce the number 
of parameters to the init function. I wanted to keep the configurable 
options at one place, so the number of parameters to the init function 
will not grow every time we add new configurable parameter. But the 
values are later NOT accessible (they are part of opaque structure 
sss_idmap_ctx). As it is now, changing the rangesize, would cause some 
slices to overlap. This will not happen if we check the bounds as Sumit 
suggested, but are these values something we want to change after the 
initialization? Sudden change to autorid compatibility mode makes no 
sense, for example. I did not add individual setters for this reason. If 
we later add options that make sense to change after initialization, 
then we can add setters for them.

> If you think that a call like the suggested
> sss_idmap_set_autorange_opts() makes sense, you can see this as the
> setter for all options at once. Then you can remove idmap_opts from the
> public interface as Simo suggested.

It is a way to keep the API backward compatible, but I do not like much. 
This way the sss_idmap_init() function would actually init only part of 
the sss_idmap_ctx. The rest of the ctx would be initialized with the 
sss_idmap_set_autorange_opts(). The function to calculate range would 
not be usable until the second part of the initialization was finished. 
In my opinion it is not nice and something we might want to change in 
the future. As you suggested, we can file a ticket to track this, but is 
there a specific reason for not breaking the backward compatibility now? 
I think now is better than later.

>
> Individual setters would work as well and are easier to add if more
> options are needed later. And since the options have to be read one by
> one from the global configuration it might make not much difference for
> the caller to set all options in one call or with individual setters.
>
> bye,
> Sumit
>>
>> Simo.
>>





More information about the sssd-devel mailing list