On 04/24/2013 02:15 PM, Sumit Bose wrote:
On Wed, Apr 24, 2013 at 01:34:07PM +0200, Michal Židek wrote:
> 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
>> 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
> 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,
sss_idmap_init() can set sensible default values, then range calculation
is available after sss_idmap_init() but can be tuned e.g. by
Setting some default values in sss_idmap_init() is very good idea. And
with this approach it also makes sense to create individual setters, as
you suggested, instead of one function to set them all (in the case
someone wants to change only several default values).
> 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.
From my point of view there are two reasons. The major one is that I
want to try to avoid to break FreeIPA before the release. The second is
that I would like to wait to get some more experience how the library
will be used and what functions are needed or useful before setting the
version to 1. E.g. there are plans to use this library in winsync so
that winsync can assign the same IDs as SSSD does, maybe we get some
feedback from the developers which would help to improve the public API
even further. Trying to not break the API would make this learning phase
easier because the dependent projects are not affected by the changes.
>> 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.