The first patch solves ticket https://fedorahosted.org/sssd/ticket/1844 The second modifies the test suite to reflect these changes.
The patch moves calculation of UID range for a domain from sdap_idmap code to sss_idmap. Some changes are:
- Structure sdap_idmap_slice has been removed. The slice number is now stored in the sss_idmap_range structure.
- The sss_idmap_init() function takes a new parameter: structure sss_idmap_opts. It holds configurable options (UID bounds, rangesize, use autorid mode). These values are also accessible outside sss_idmap (simple getters have been added).
- Options dp_opt_get_int for SDAP_IDMAP_LOWER/UPPER/AUTORID_COMPAT/RANGESIZE was moved from sdap_idmap_add_domain to sdap_idmap_init. These values are shared among subdomains, so there is no need to call it in each sdap_idmap_add_domain call.
- sss_idmap_calculate_range function is mostly copy of the range calculation processing in sdap_idmap_add_domain(), but it uses values stored in sss_idmap_opts and the slice number is taken from the rnge structure (which is completely created by this function now).
Sumit, could you please review this patch? I think you are most familiar with the id mapping code.
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?
Thanks Michal
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 04/23/2013 10:37 AM, Michal Židek wrote:
The first patch solves ticket https://fedorahosted.org/sssd/ticket/1844 The second modifies the test suite to reflect these changes.
The patch moves calculation of UID range for a domain from sdap_idmap code to sss_idmap. Some changes are:
- Structure sdap_idmap_slice has been removed. The slice number is
now stored in the sss_idmap_range structure.
- The sss_idmap_init() function takes a new parameter: structure
sss_idmap_opts. It holds configurable options (UID bounds, rangesize, use autorid mode). These values are also accessible outside sss_idmap (simple getters have been added).
- Options dp_opt_get_int for
SDAP_IDMAP_LOWER/UPPER/AUTORID_COMPAT/RANGESIZE was moved from sdap_idmap_add_domain to sdap_idmap_init. These values are shared among subdomains, so there is no need to call it in each sdap_idmap_add_domain call.
- sss_idmap_calculate_range function is mostly copy of the range
calculation processing in sdap_idmap_add_domain(), but it uses values stored in sss_idmap_opts and the slice number is taken from the rnge structure (which is completely created by this function now).
Sumit, could you please review this patch? I think you are most familiar with the id mapping code.
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?
Nack.
First, please don't do the shallow copy in sss_idmap_init(). Just because the idmap_opts object doesn't have any pointers today doesn't mean we aren't going to be introducing a hard-to-catch bug later on. Please make a copy routine (that just does a shallow copy today).
I think I just noticed a bug in my original design of the hash collision code (though it's hopefully small enough that no one has hit it). The comparison for the loop around back to slice 0 on collision is off-by-one. If the new_slice == max_slices, it would be choosing a slice number one past upper range. We should fix that here. Please double-check my thought process to make sure I'm not getting it wrong.
Build fails: CC src/responder/ssh/sshsrv_dp.o CC src/responder/ssh/sshsrv_cmd.o CC src/responder/pac/sssd_pac-pacsrv.o CC src/responder/pac/sssd_pac-pacsrv_cmd.o ../src/responder/pac/pacsrv.c: In function ‘pac_process_init’: ../src/responder/pac/pacsrv.c:190:26: warning: passing argument 4 of ‘sss_idmap_init’ from incompatible pointer type [enabled by default] In file included from ../src/responder/pac/pacsrv.h:41:0, from ../src/responder/pac/pacsrv.c:35: ../src/lib/idmap/sss_idmap.h:151:23: note: expected ‘struct sss_idmap_opts *’ but argument is of type ‘struct sss_idmap_ctx **’ ../src/responder/pac/pacsrv.c:190:26: error: too few arguments to function ‘sss_idmap_init’ In file included from ../src/responder/pac/pacsrv.h:41:0, from ../src/responder/pac/pacsrv.c:35: ../src/lib/idmap/sss_idmap.h:151:23: note: declared here make[1]: *** [src/responder/pac/sssd_pac-pacsrv.o] Error 1 make[1]: *** Waiting for unfinished jobs.... make[1]: Leaving directory `/home/sgallagh/workspace/sssd/x86_64' make: *** [check-recursive] Error 1
On 04/23/2013 10:59 AM, Stephen Gallagher wrote:
On 04/23/2013 10:37 AM, Michal Židek wrote:
The first patch solves ticket https://fedorahosted.org/sssd/ticket/1844 The second modifies the test suite to reflect these changes.
The patch moves calculation of UID range for a domain from sdap_idmap code to sss_idmap. Some changes are:
- Structure sdap_idmap_slice has been removed. The slice number is
now stored in the sss_idmap_range structure.
- The sss_idmap_init() function takes a new parameter: structure
sss_idmap_opts. It holds configurable options (UID bounds, rangesize, use autorid mode). These values are also accessible outside sss_idmap (simple getters have been added).
- Options dp_opt_get_int for
SDAP_IDMAP_LOWER/UPPER/AUTORID_COMPAT/RANGESIZE was moved from sdap_idmap_add_domain to sdap_idmap_init. These values are shared among subdomains, so there is no need to call it in each sdap_idmap_add_domain call.
- sss_idmap_calculate_range function is mostly copy of the range
calculation processing in sdap_idmap_add_domain(), but it uses values stored in sss_idmap_opts and the slice number is taken from the rnge structure (which is completely created by this function now).
Sumit, could you please review this patch? I think you are most familiar with the id mapping code.
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?
Nack.
First, please don't do the shallow copy in sss_idmap_init(). Just because the idmap_opts object doesn't have any pointers today doesn't mean we aren't going to be introducing a hard-to-catch bug later on. Please make a copy routine (that just does a shallow copy today).
I think I just noticed a bug in my original design of the hash collision code (though it's hopefully small enough that no one has hit it). The comparison for the loop around back to slice 0 on collision is off-by-one. If the new_slice == max_slices, it would be choosing a slice number one past upper range. We should fix that here. Please double-check my thought process to make sure I'm not getting it wrong.
If we confirm that a fix is needed please file a ticket.
Build fails: CC src/responder/ssh/sshsrv_dp.o CC src/responder/ssh/sshsrv_cmd.o CC src/responder/pac/sssd_pac-pacsrv.o CC src/responder/pac/sssd_pac-pacsrv_cmd.o ../src/responder/pac/pacsrv.c: In function ‘pac_process_init’: ../src/responder/pac/pacsrv.c:190:26: warning: passing argument 4 of ‘sss_idmap_init’ from incompatible pointer type [enabled by default] In file included from ../src/responder/pac/pacsrv.h:41:0, from ../src/responder/pac/pacsrv.c:35: ../src/lib/idmap/sss_idmap.h:151:23: note: expected ‘struct sss_idmap_opts *’ but argument is of type ‘struct sss_idmap_ctx **’ ../src/responder/pac/pacsrv.c:190:26: error: too few arguments to function ‘sss_idmap_init’ In file included from ../src/responder/pac/pacsrv.h:41:0, from ../src/responder/pac/pacsrv.c:35: ../src/lib/idmap/sss_idmap.h:151:23: note: declared here make[1]: *** [src/responder/pac/sssd_pac-pacsrv.o] Error 1 make[1]: *** Waiting for unfinished jobs.... make[1]: Leaving directory `/home/sgallagh/workspace/sssd/x86_64' make: *** [check-recursive] Error 1
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
-----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.
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 ?
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().
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 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.
Simo.
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@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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.
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().
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
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.
New version of the patch is attached.
Changes from previous iteration: - version-info set to 0:2:0 - init function remains backward compatible, but it sets default oadptions needed for range calculation. Non default values can be set via ded setters. - sss_idmap_opts moved to private header - both setters and getters check for sss_idmap_ctx validity - slice_num removed from range structure - min and max id are used to check for collisions with existing domains when calculating range for new domain - off by one error mentioned by Stephen was fixed (s/>/>=) - the parameter slice in sss_idmap_calculate_range() is now in/out parameter. We do not store the slice number in any context (it was only used to check for collisions and is no longer needed), but we currently store the slice number in sysdb, so it must be somehow returned from sss_idmap_calculate_range(). I think that in the future versions of sysdb, we will store the min/max id values instead of slice numbers (to support custom ranges), but it is better to be able to obtain the slice number (right now it is needed for sysdb, but might be needed for something else in the future).
Thanks Michal
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On Wed 24 Apr 2013 02:05:55 PM EDT, Michal Židek wrote:
New version of the patch is attached.
Changes from previous iteration: - version-info set to 0:2:0 - init function remains backward compatible, but it sets default oadptions needed for range calculation. Non default values can be set via ded setters. - sss_idmap_opts moved to private header
This is still a backwards-incompatible break. If we were presenting a data structure in a public header and no longer are, it's an ABI break. Is there any way to avoid this?
- both setters and getters check for sss_idmap_ctx validity -
slice_num removed from range structure - min and max id are used to check for collisions with existing domains when calculating range for new domain - off by one error mentioned by Stephen was fixed (s/>/>=) - the parameter slice in sss_idmap_calculate_range() is now in/out parameter. We do not store the slice number in any context (it was only used to check for collisions and is no longer needed), but we currently store the slice number in sysdb, so it must be somehow returned from sss_idmap_calculate_range(). I think that in the future versions of sysdb, we will store the min/max id values instead of slice numbers (to support custom ranges), but it is better to be able to obtain the slice number (right now it is needed for sysdb, but might be needed for something else in the future).
Thanks Michal
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Wed, Apr 24, 2013 at 02:22:42PM -0400, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On Wed 24 Apr 2013 02:05:55 PM EDT, Michal Židek wrote:
New version of the patch is attached.
Changes from previous iteration: - version-info set to 0:2:0 - init function remains backward compatible, but it sets default oadptions needed for range calculation. Non default values can be set via ded setters. - sss_idmap_opts moved to private header
This is still a backwards-incompatible break. If we were presenting a data structure in a public header and no longer are, it's an ABI break. Is there any way to avoid this?
sss_idmap_opts is introduced with this patch, it was in the public header in the first version and Michal moved it into the private one in the new version.
bye, Sumit
- both setters and getters check for sss_idmap_ctx validity -
slice_num removed from range structure - min and max id are used to check for collisions with existing domains when calculating range for new domain - off by one error mentioned by Stephen was fixed (s/>/>=) - the parameter slice in sss_idmap_calculate_range() is now in/out parameter. We do not store the slice number in any context (it was only used to check for collisions and is no longer needed), but we currently store the slice number in sysdb, so it must be somehow returned from sss_idmap_calculate_range(). I think that in the future versions of sysdb, we will store the min/max id values instead of slice numbers (to support custom ranges), but it is better to be able to obtain the slice number (right now it is needed for sysdb, but might be needed for something else in the future).
Thanks Michal
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
-----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.13 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iEYEARECAAYFAlF4IvIACgkQeiVVYja6o6PzxQCeJQncpfsNpK+GUEgoONZ6tD85 pPYAn0mqU/3RSEwnFHb6O2JezgtuIY4h =0jBk -----END PGP SIGNATURE----- _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Wed, 2013-04-24 at 20:55 +0200, Sumit Bose wrote:
On Wed, Apr 24, 2013 at 02:22:42PM -0400, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On Wed 24 Apr 2013 02:05:55 PM EDT, Michal Židek wrote:
New version of the patch is attached.
Changes from previous iteration: - version-info set to 0:2:0 - init function remains backward compatible, but it sets default oadptions needed for range calculation. Non default values can be set via ded setters. - sss_idmap_opts moved to private header
This is still a backwards-incompatible break. If we were presenting a data structure in a public header and no longer are, it's an ABI break. Is there any way to avoid this?
sss_idmap_opts is introduced with this patch, it was in the public header in the first version and Michal moved it into the private one in the new version.
I think Steven was referring to struct sdap_idmap_slice disappearing from src/providers/ldap/sdap_idmap.h maybe ?
However that should not be a problem as it is not a public header.
The new approach looks ok to me, anyway. Good turn around Michal.
Simo.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 04/24/2013 06:11 PM, Simo Sorce wrote:
On Wed, 2013-04-24 at 20:55 +0200, Sumit Bose wrote:
On Wed, Apr 24, 2013 at 02:22:42PM -0400, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On Wed 24 Apr 2013 02:05:55 PM EDT, Michal Židek wrote:
New version of the patch is attached.
Changes from previous iteration: - version-info set to 0:2:0
- init function remains backward compatible, but it sets
default oadptions needed for range calculation. Non default values can be set via ded setters. - sss_idmap_opts moved to private header
This is still a backwards-incompatible break. If we were presenting a data structure in a public header and no longer are, it's an ABI break. Is there any way to avoid this?
sss_idmap_opts is introduced with this patch, it was in the public header in the first version and Michal moved it into the private one in the new version.
I think Steven was referring to struct sdap_idmap_slice disappearing from src/providers/ldap/sdap_idmap.h maybe ?
Actually, I hadn't looked at the patch. I saw that he had moved a structure from a public to a private header and didn't realize it was a newly-added structure.
One nack: the version-info string needs to be 1:0:1 according to http://www.gnu.org/software/libtool/manual/html_node/Updating-version-info.h...
This is because you have added the getters and setters to the public interface, which means that all of steps 3, 4 and 5 in that guideline must be followed, which results in a value of 1:0:1 for version-info. Note: this arrangement does NOT result in a SOname bump, because the "age" is equal to the "current" (Which effectively means that we're saying that this interface is a proper superset of all interfaces that have preceded it).
On 04/25/2013 05:18 PM, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 04/24/2013 06:11 PM, Simo Sorce wrote:
On Wed, 2013-04-24 at 20:55 +0200, Sumit Bose wrote:
On Wed, Apr 24, 2013 at 02:22:42PM -0400, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On Wed 24 Apr 2013 02:05:55 PM EDT, Michal Židek wrote:
New version of the patch is attached.
Changes from previous iteration: - version-info set to 0:2:0
- init function remains backward compatible, but it sets
default oadptions needed for range calculation. Non default values can be set via ded setters. - sss_idmap_opts moved to private header
This is still a backwards-incompatible break. If we were presenting a data structure in a public header and no longer are, it's an ABI break. Is there any way to avoid this?
sss_idmap_opts is introduced with this patch, it was in the public header in the first version and Michal moved it into the private one in the new version.
I think Steven was referring to struct sdap_idmap_slice disappearing from src/providers/ldap/sdap_idmap.h maybe ?
Actually, I hadn't looked at the patch. I saw that he had moved a structure from a public to a private header and didn't realize it was a newly-added structure.
One nack: the version-info string needs to be 1:0:1 according to http://www.gnu.org/software/libtool/manual/html_node/Updating-version-info.h...
This is because you have added the getters and setters to the public interface, which means that all of steps 3, 4 and 5 in that guideline must be followed, which results in a value of 1:0:1 for version-info. Note: this arrangement does NOT result in a SOname bump, because the "age" is equal to the "current" (Which effectively means that we're saying that this interface is a proper superset of all interfaces that have preceded it).
Thanks a lot for the explanation Stephen. I was actually following the same guide, but I thought that only one rule is applied each time version-info is changed, which did not make much sense in some situations. Now it finally makes sense to me.
New patch is attached.
Thanks Michal
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 04/29/2013 06:46 AM, Michal Židek wrote:
On 04/25/2013 05:18 PM, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 04/24/2013 06:11 PM, Simo Sorce wrote:
On Wed, 2013-04-24 at 20:55 +0200, Sumit Bose wrote:
On Wed, Apr 24, 2013 at 02:22:42PM -0400, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On Wed 24 Apr 2013 02:05:55 PM EDT, Michal Židek wrote:
New version of the patch is attached.
Changes from previous iteration: - version-info set to 0:2:0 - init function remains backward compatible, but it sets default oadptions needed for range calculation. Non default values can be set via ded setters. - sss_idmap_opts moved to private header
This is still a backwards-incompatible break. If we were presenting a data structure in a public header and no longer are, it's an ABI break. Is there any way to avoid this?
sss_idmap_opts is introduced with this patch, it was in the public header in the first version and Michal moved it into the private one in the new version.
I think Steven was referring to struct sdap_idmap_slice disappearing from src/providers/ldap/sdap_idmap.h maybe ?
Actually, I hadn't looked at the patch. I saw that he had moved a structure from a public to a private header and didn't realize it was a newly-added structure.
One nack: the version-info string needs to be 1:0:1 according to http://www.gnu.org/software/libtool/manual/html_node/Updating-version-info.h...
This is because you have added the getters and setters to the public
interface, which means that all of steps 3, 4 and 5 in that guideline must be followed, which results in a value of 1:0:1 for version-info. Note: this arrangement does NOT result in a SOname bump, because the "age" is equal to the "current" (Which effectively means that we're saying that this interface is a proper superset of all interfaces that have preceded it).
Thanks a lot for the explanation Stephen. I was actually following the same guide, but I thought that only one rule is applied each time version-info is changed, which did not make much sense in some situations. Now it finally makes sense to me.
New patch is attached.
Ack!
On Mon, Apr 29, 2013 at 08:13:21AM -0400, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 04/29/2013 06:46 AM, Michal Židek wrote:
On 04/25/2013 05:18 PM, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 04/24/2013 06:11 PM, Simo Sorce wrote:
On Wed, 2013-04-24 at 20:55 +0200, Sumit Bose wrote:
On Wed, Apr 24, 2013 at 02:22:42PM -0400, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On Wed 24 Apr 2013 02:05:55 PM EDT, Michal Židek wrote: > New version of the patch is attached. > > Changes from previous iteration: - version-info set to > 0:2:0 - init function remains backward compatible, but it > sets default oadptions needed for range calculation. Non > default values can be set via ded setters. - > sss_idmap_opts moved to private header
This is still a backwards-incompatible break. If we were presenting a data structure in a public header and no longer are, it's an ABI break. Is there any way to avoid this?
sss_idmap_opts is introduced with this patch, it was in the public header in the first version and Michal moved it into the private one in the new version.
I think Steven was referring to struct sdap_idmap_slice disappearing from src/providers/ldap/sdap_idmap.h maybe ?
Actually, I hadn't looked at the patch. I saw that he had moved a structure from a public to a private header and didn't realize it was a newly-added structure.
One nack: the version-info string needs to be 1:0:1 according to http://www.gnu.org/software/libtool/manual/html_node/Updating-version-info.h...
This is because you have added the getters and setters to the public
interface, which means that all of steps 3, 4 and 5 in that guideline must be followed, which results in a value of 1:0:1 for version-info. Note: this arrangement does NOT result in a SOname bump, because the "age" is equal to the "current" (Which effectively means that we're saying that this interface is a proper superset of all interfaces that have preceded it).
Thanks a lot for the explanation Stephen. I was actually following the same guide, but I thought that only one rule is applied each time version-info is changed, which did not make much sense in some situations. Now it finally makes sense to me.
New patch is attached.
Ack!
Pushed to master.
sssd-devel@lists.fedorahosted.org
-
Dmitri Pal -
Jakub Hrozek -
Michal Židek -
Simo Sorce -
Stephen Gallagher -
Sumit Bose