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, 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.