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
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,
sss_idmap_init() can set sensible default values, then range calculation
is available after sss_idmap_init() but can be tuned e.g. by
sss_idmap_set_autorange_opts().
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.
bye,
Sumit
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.
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
