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

Sumit Bose sbose at redhat.com
Wed Apr 24 08:08:27 UTC 2013


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.

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

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.

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.
> 
> -- 
> Simo Sorce * Red Hat, Inc * New York
> 
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel



More information about the sssd-devel mailing list