Hi,
this series of patches fixes https://fedorahosted.org/sssd/ticket/1666 and https://fedorahosted.org/sssd/ticket/1672. Details can be found in the commit messages. Just as overview:
0001-0006 add new structs and call to make handling groups from different domains easier.
0007 and 0008 add the evaluation of the group memberships from the domain the user belongs to (#1666).
0009 just changes an error code
0010 fixes #1672 by removing duplicated groups
0011 adds a new unit test
bye, Sumit
On Wed, 2012-12-05 at 13:36 +0100, Sumit Bose wrote:
Hi,
this series of patches fixes https://fedorahosted.org/sssd/ticket/1666 and https://fedorahosted.org/sssd/ticket/1672. Details can be found in the commit messages. Just as overview:
0001-0006 add new structs and call to make handling groups from different domains easier.
Sorry Sumit but I do not really agree with patch 1, it is extremely wasteful of space, and ugly.
I think you should create a structure for the container and not for each element, where you have:
container { domain1 { domain_info array_of_gids } domain2 { ... ... }
Patch 4 sounds fishy, I distinctly recall we do not touch domain_id because that is the domain identifier. It can't change because if it 'changes' it means it is a different domain. I am not even sure you can ever match that for updates because IIRC domains are searched by domain_id. So where is my reasoning wrong ? Tentatively nack if I am correct.
Patch 5 is a step in the right direction but it sounds wrong that you search the user in the db and just skip saving if it exist. The user info you have in the PAC may be updated and contain information that differs from what's in the cache. At the very least you need to test that nothing needs to change if you do not want to unconditionally overwrite.
0007 and 0008 add the evaluation of the group memberships from the domain the user belongs to (#1666).
The domain sid retrieval looks extremely inefficient, we have a neat optimization where you can look up the domain once and then just go rid by rid and you are going back to build a full sid, which will then be turned into a string and matched once again to find the domain. I think this should be improved to use the fact you already have the sids split out with a modicum of caching. You can uses alexander's patch that just landed in freeipa to do binary sid comparison w/o converting to string, and a simple array in the function to keep a list of all dom_sid -> domaiin structs found so far, they wont be many just a handful in most cases so a linear search there is also fine.
0009 just changes an error code
ACK
0010 fixes #1672 by removing duplicated groups
I think using a hash table like function is fine, but I wonder if we can save a lot of cycles here by using a smarter hash algorithm, namely just truncate the gid and use the lower N bits. This would be orders of magnitude more efficient and you pretty much are guaranteed to not have many collisions because gids are unique and tend to all start from a common base so if you take the least N significant bits you basically have an automatic index already built in.
Building a static table on the stack like the following and only allocating (using talloc) only on collisions would also save tons of allocations.
struct gid_el { gid_t gid; struct gid_el *next; } gid_table[1024] = { 0 };
the 'hash/index' is (gid & 0x2FF)
0011 adds a new unit test
Simo.
On Wed, Dec 05, 2012 at 09:04:46AM -0500, Simo Sorce wrote:
On Wed, 2012-12-05 at 13:36 +0100, Sumit Bose wrote:
Hi,
this series of patches fixes https://fedorahosted.org/sssd/ticket/1666 and https://fedorahosted.org/sssd/ticket/1672. Details can be found in the commit messages. Just as overview:
0001-0006 add new structs and call to make handling groups from different domains easier.
Sorry Sumit but I do not really agree with patch 1, it is extremely wasteful of space, and ugly.
I think you should create a structure for the container and not for each element, where you have:
container { domain1 { domain_info array_of_gids } domain2 { ... ... }
ok, I'll change it accordingly
Patch 4 sounds fishy, I distinctly recall we do not touch domain_id because that is the domain identifier. It can't change because if it 'changes' it means it is a different domain. I am not even sure you can ever match that for updates because IIRC domains are searched by domain_id. So where is my reasoning wrong ? Tentatively nack if I am correct.
no, as said in the commit message, it is only changed if it is not already set and that it just covers the case where ipa-adtrust-install is called on the server and the local domain gets a SID.
Patch 5 is a step in the right direction but it sounds wrong that you search the user in the db and just skip saving if it exist. The user info you have in the PAC may be updated and contain information that differs from what's in the cache. At the very least you need to test that nothing needs to change if you do not want to unconditionally overwrite.
yes, there is still a todo in the original code for this, but since it is not related to the missing groups I haven't touched it yet.
0007 and 0008 add the evaluation of the group memberships from the domain the user belongs to (#1666).
The domain sid retrieval looks extremely inefficient, we have a neat optimization where you can look up the domain once and then just go rid by rid and you are going back to build a full sid, which will then be turned into a string and matched once again to find the domain. I think this should be improved to use the fact you already have the sids split out with a modicum of caching. You can uses alexander's patch that just landed in freeipa to do binary sid comparison w/o converting to string, and a simple array in the function to keep a list of all dom_sid -> domaiin structs found so far, they wont be many just a handful in most cases so a linear search there is also fine.
I cannot follow your comment, I'll ping you on irc about it.
0009 just changes an error code
ACK
0010 fixes #1672 by removing duplicated groups
I think using a hash table like function is fine, but I wonder if we can save a lot of cycles here by using a smarter hash algorithm, namely just truncate the gid and use the lower N bits. This would be orders of magnitude more efficient and you pretty much are guaranteed to not have many collisions because gids are unique and tend to all start from a common base so if you take the least N significant bits you basically have an automatic index already built in.
Building a static table on the stack like the following and only allocating (using talloc) only on collisions would also save tons of allocations.
struct gid_el { gid_t gid; struct gid_el *next; } gid_table[1024] = { 0 };
the 'hash/index' is (gid & 0x2FF)
Currently I get to following timing data for get_gids_from_pac():
Running suite(s): PAC responder Testcase [0], number of groups [1], duration [0s 10us] Testcase [1], number of groups [10], duration [0s 44us] Testcase [2], number of groups [100], duration [0s 197us] Testcase [3], number of groups [1000], duration [0s 2307us] Testcase [4], number of groups [10000], duration [0s 15636us] Testcase [5], number of groups [100000], duration [0s 167523us] Testcase [6], number of groups [1000000], duration [1s 688983us]
I think this is not too bad and I would prefer to use the common hash. If you disagree I can implement your suggestion.
bye, Sumit
0011 adds a new unit test
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 Wed, 2012-12-05 at 18:23 +0100, Sumit Bose wrote:
On Wed, Dec 05, 2012 at 09:04:46AM -0500, Simo Sorce wrote:
On Wed, 2012-12-05 at 13:36 +0100, Sumit Bose wrote:
Hi,
this series of patches fixes https://fedorahosted.org/sssd/ticket/1666 and https://fedorahosted.org/sssd/ticket/1672. Details can be found in the commit messages. Just as overview:
0001-0006 add new structs and call to make handling groups from different domains easier.
Sorry Sumit but I do not really agree with patch 1, it is extremely wasteful of space, and ugly.
I think you should create a structure for the container and not for each element, where you have:
container { domain1 { domain_info array_of_gids } domain2 { ... ... }
ok, I'll change it accordingly
Patch 4 sounds fishy, I distinctly recall we do not touch domain_id because that is the domain identifier. It can't change because if it 'changes' it means it is a different domain. I am not even sure you can ever match that for updates because IIRC domains are searched by domain_id. So where is my reasoning wrong ? Tentatively nack if I am correct.
no, as said in the commit message, it is only changed if it is not already set and that it just covers the case where ipa-adtrust-install is called on the server and the local domain gets a SID.
I guess I cannot see how you can have an entry without domain_id, maybe I missed how that can happen.
Patch 5 is a step in the right direction but it sounds wrong that you search the user in the db and just skip saving if it exist. The user info you have in the PAC may be updated and contain information that differs from what's in the cache. At the very least you need to test that nothing needs to change if you do not want to unconditionally overwrite.
yes, there is still a todo in the original code for this, but since it is not related to the missing groups I haven't touched it yet.
ok, do we have a ticket open as well ?
0007 and 0008 add the evaluation of the group memberships from the domain the user belongs to (#1666).
The domain sid retrieval looks extremely inefficient, we have a neat optimization where you can look up the domain once and then just go rid by rid and you are going back to build a full sid, which will then be turned into a string and matched once again to find the domain. I think this should be improved to use the fact you already have the sids split out with a modicum of caching. You can uses alexander's patch that just landed in freeipa to do binary sid comparison w/o converting to string, and a simple array in the function to keep a list of all dom_sid -> domaiin structs found so far, they wont be many just a handful in most cases so a linear search there is also fine.
I cannot follow your comment, I'll ping you on irc about it.
ok
0009 just changes an error code
ACK
0010 fixes #1672 by removing duplicated groups
I think using a hash table like function is fine, but I wonder if we can save a lot of cycles here by using a smarter hash algorithm, namely just truncate the gid and use the lower N bits. This would be orders of magnitude more efficient and you pretty much are guaranteed to not have many collisions because gids are unique and tend to all start from a common base so if you take the least N significant bits you basically have an automatic index already built in.
Building a static table on the stack like the following and only allocating (using talloc) only on collisions would also save tons of allocations.
struct gid_el { gid_t gid; struct gid_el *next; } gid_table[1024] = { 0 };
the 'hash/index' is (gid & 0x2FF)
Currently I get to following timing data for get_gids_from_pac():
Running suite(s): PAC responder Testcase [0], number of groups [1], duration [0s 10us] Testcase [1], number of groups [10], duration [0s 44us] Testcase [2], number of groups [100], duration [0s 197us] Testcase [3], number of groups [1000], duration [0s 2307us] Testcase [4], number of groups [10000], duration [0s 15636us] Testcase [5], number of groups [100000], duration [0s 167523us] Testcase [6], number of groups [1000000], duration [1s 688983us]
I think this is not too bad and I would prefer to use the common hash. If you disagree I can implement your suggestion.
No I think my solution was clever, but as much as I like it on paper, the numbers do not warrant wasting time on it right now.
Simo.
Hi,
here comes a slightly modified version of the patches, see comments inline.
bye, Sumit
On Wed, Dec 05, 2012 at 12:50:11PM -0500, Simo Sorce wrote:
On Wed, 2012-12-05 at 18:23 +0100, Sumit Bose wrote:
On Wed, Dec 05, 2012 at 09:04:46AM -0500, Simo Sorce wrote:
On Wed, 2012-12-05 at 13:36 +0100, Sumit Bose wrote:
Hi,
this series of patches fixes https://fedorahosted.org/sssd/ticket/1666 and https://fedorahosted.org/sssd/ticket/1672. Details can be found in the commit messages. Just as overview:
0001-0006 add new structs and call to make handling groups from different domains easier.
Sorry Sumit but I do not really agree with patch 1, it is extremely wasteful of space, and ugly.
I think you should create a structure for the container and not for each element, where you have:
container { domain1 { domain_info array_of_gids } domain2 { ... ... }
ok, I'll change it accordingly
I started to change it but didn't like the resulting code, because it turned out to be complex and hard to follow. Since the original struct only user 8 or 12 bytes per group (depending if your are on 32bits or on 64bits) I would prefer to keep the old struct for this round of patches.
Patch 4 sounds fishy, I distinctly recall we do not touch domain_id because that is the domain identifier. It can't change because if it 'changes' it means it is a different domain. I am not even sure you can ever match that for updates because IIRC domains are searched by domain_id. So where is my reasoning wrong ? Tentatively nack if I am correct.
no, as said in the commit message, it is only changed if it is not already set and that it just covers the case where ipa-adtrust-install is called on the server and the local domain gets a SID.
I guess I cannot see how you can have an entry without domain_id, maybe I missed how that can happen.
For the local domain the domain_id is added by ipa-adtrust-install, if it wasn't run before there is no domain_id for the local domain.
Patch 5 is a step in the right direction but it sounds wrong that you search the user in the db and just skip saving if it exist. The user info you have in the PAC may be updated and contain information that differs from what's in the cache. At the very least you need to test that nothing needs to change if you do not want to unconditionally overwrite.
yes, there is still a todo in the original code for this, but since it is not related to the missing groups I haven't touched it yet.
ok, do we have a ticket open as well ?
I added a fix with patch 0012.
0007 and 0008 add the evaluation of the group memberships from the domain the user belongs to (#1666).
The domain sid retrieval looks extremely inefficient, we have a neat optimization where you can look up the domain once and then just go rid by rid and you are going back to build a full sid, which will then be turned into a string and matched once again to find the domain. I think this should be improved to use the fact you already have the sids split out with a modicum of caching. You can uses alexander's patch that just landed in freeipa to do binary sid comparison w/o converting to string, and a simple array in the function to keep a list of all dom_sid -> domaiin structs found so far, they wont be many just a handful in most cases so a linear search there is also fine.
I cannot follow your comment, I'll ping you on irc about it.
ok
we talked on irc and found that code works efficient here.
0009 just changes an error code
ACK
0010 fixes #1672 by removing duplicated groups
I think using a hash table like function is fine, but I wonder if we can save a lot of cycles here by using a smarter hash algorithm, namely just truncate the gid and use the lower N bits. This would be orders of magnitude more efficient and you pretty much are guaranteed to not have many collisions because gids are unique and tend to all start from a common base so if you take the least N significant bits you basically have an automatic index already built in.
Building a static table on the stack like the following and only allocating (using talloc) only on collisions would also save tons of allocations.
struct gid_el { gid_t gid; struct gid_el *next; } gid_table[1024] = { 0 };
the 'hash/index' is (gid & 0x2FF)
Currently I get to following timing data for get_gids_from_pac():
Running suite(s): PAC responder Testcase [0], number of groups [1], duration [0s 10us] Testcase [1], number of groups [10], duration [0s 44us] Testcase [2], number of groups [100], duration [0s 197us] Testcase [3], number of groups [1000], duration [0s 2307us] Testcase [4], number of groups [10000], duration [0s 15636us] Testcase [5], number of groups [100000], duration [0s 167523us] Testcase [6], number of groups [1000000], duration [1s 688983us]
I think this is not too bad and I would prefer to use the common hash. If you disagree I can implement your suggestion.
No I think my solution was clever, but as much as I like it on paper, the numbers do not warrant wasting time on it right now.
I added the new test to patch 0011.
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 Tue, 2012-12-18 at 21:43 +0100, Sumit Bose wrote:
I started to change it but didn't like the resulting code, because it turned out to be complex and hard to follow. Since the original struct only user 8 or 12 bytes per group (depending if your are on 32bits or on 64bits) I would prefer to keep the old struct for this round of patches.
Sorry can't agree, space is only one of the factors, although completely unnecessarily using 2x or 3x number of groups space is ugly enough, But the main issues are really in the code this structure allows/produces.
Still a nack for me.
Simo.
On Wed, Dec 19, 2012 at 08:10:02AM -0500, Simo Sorce wrote:
On Tue, 2012-12-18 at 21:43 +0100, Sumit Bose wrote:
I started to change it but didn't like the resulting code, because it turned out to be complex and hard to follow. Since the original struct only user 8 or 12 bytes per group (depending if your are on 32bits or on 64bits) I would prefer to keep the old struct for this round of patches.
Sorry can't agree, space is only one of the factors, although completely unnecessarily using 2x or 3x number of groups space is ugly enough, But the main issues are really in the code this structure allows/produces.
Still a nack for me.
The attached patch should be applied on top of the other patches and changes the struct as you suggested in your first comment.
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 Fri, Dec 21, 2012 at 12:31:32PM +0100, Sumit Bose wrote:
On Wed, Dec 19, 2012 at 08:10:02AM -0500, Simo Sorce wrote:
On Tue, 2012-12-18 at 21:43 +0100, Sumit Bose wrote:
I started to change it but didn't like the resulting code, because it turned out to be complex and hard to follow. Since the original struct only user 8 or 12 bytes per group (depending if your are on 32bits or on 64bits) I would prefer to keep the old struct for this round of patches.
Sorry can't agree, space is only one of the factors, although completely unnecessarily using 2x or 3x number of groups space is ugly enough, But the main issues are really in the code this structure allows/produces.
Still a nack for me.
The attached patch should be applied on top of the other patches and changes the struct as you suggested in your first comment.
bye, Sumit
Why not squash the patch into the patch that introduces the structure?
On Fri, Jan 04, 2013 at 04:31:03PM +0100, Jakub Hrozek wrote:
On Fri, Dec 21, 2012 at 12:31:32PM +0100, Sumit Bose wrote:
On Wed, Dec 19, 2012 at 08:10:02AM -0500, Simo Sorce wrote:
On Tue, 2012-12-18 at 21:43 +0100, Sumit Bose wrote:
I started to change it but didn't like the resulting code, because it turned out to be complex and hard to follow. Since the original struct only user 8 or 12 bytes per group (depending if your are on 32bits or on 64bits) I would prefer to keep the old struct for this round of patches.
Sorry can't agree, space is only one of the factors, although completely unnecessarily using 2x or 3x number of groups space is ugly enough, But the main issues are really in the code this structure allows/produces.
Still a nack for me.
The attached patch should be applied on top of the other patches and changes the struct as you suggested in your first comment.
bye, Sumit
Why not squash the patch into the patch that introduces the structure?
The structure was introduced in the first patch and then used in some of the other patches. I didn't squash the changes into the related patches to not invalidate the reviews of the others and to allow a better comparison between the old and the new structure.
bye, Sumit
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Fri, 2013-01-04 at 17:15 +0100, Sumit Bose wrote:
On Fri, Jan 04, 2013 at 04:31:03PM +0100, Jakub Hrozek wrote:
On Fri, Dec 21, 2012 at 12:31:32PM +0100, Sumit Bose wrote:
On Wed, Dec 19, 2012 at 08:10:02AM -0500, Simo Sorce wrote:
On Tue, 2012-12-18 at 21:43 +0100, Sumit Bose wrote:
I started to change it but didn't like the resulting code, because it turned out to be complex and hard to follow. Since the original struct only user 8 or 12 bytes per group (depending if your are on 32bits or on 64bits) I would prefer to keep the old struct for this round of patches.
Sorry can't agree, space is only one of the factors, although completely unnecessarily using 2x or 3x number of groups space is ugly enough, But the main issues are really in the code this structure allows/produces.
Still a nack for me.
The attached patch should be applied on top of the other patches and changes the struct as you suggested in your first comment.
bye, Sumit
Why not squash the patch into the patch that introduces the structure?
The structure was introduced in the first patch and then used in some of the other patches. I didn't squash the changes into the related patches to not invalidate the reviews of the others and to allow a better comparison between the old and the new structure.
Ok I reviewd all patches except the last, which actually makes it harder as I would have to wrap my head again around 3/4 patches I already reviewed, so it would be really nice if it were folded in. I'll let Jakub decide what to do, maybe he will review the patch on ti's own and just ack it :)
I have one comment though, in patch 6 you use the term 'local domain' and later on you also renamed a variable from domain_sid to loc_domain_sid. I think this is confusing because we have the LOCAL domain in sssd and we never used the term 'local domain' to describe a generic domain. I think you should not call it local at all. The 'domain' is just fine, the subdomains are already differentiated by calling them 'sub'.
I would like to see 0006 respun to remove the 'local' reference from the commit, comments and the variable.
Also the first commit line of patch 0007 has 2 typos group -> groups, attributre -> attribute.
Beyond this I think it's an ACK for the first 12 patches, provided someone either review the 13th or fold the 13th back into the first.
Simo.
On Fri, Jan 04, 2013 at 05:15:22PM -0500, Simo Sorce wrote:
On Fri, 2013-01-04 at 17:15 +0100, Sumit Bose wrote:
On Fri, Jan 04, 2013 at 04:31:03PM +0100, Jakub Hrozek wrote:
On Fri, Dec 21, 2012 at 12:31:32PM +0100, Sumit Bose wrote:
On Wed, Dec 19, 2012 at 08:10:02AM -0500, Simo Sorce wrote:
On Tue, 2012-12-18 at 21:43 +0100, Sumit Bose wrote:
I started to change it but didn't like the resulting code, because it turned out to be complex and hard to follow. Since the original struct only user 8 or 12 bytes per group (depending if your are on 32bits or on 64bits) I would prefer to keep the old struct for this round of patches.
Sorry can't agree, space is only one of the factors, although completely unnecessarily using 2x or 3x number of groups space is ugly enough, But the main issues are really in the code this structure allows/produces.
Still a nack for me.
The attached patch should be applied on top of the other patches and changes the struct as you suggested in your first comment.
bye, Sumit
Why not squash the patch into the patch that introduces the structure?
The structure was introduced in the first patch and then used in some of the other patches. I didn't squash the changes into the related patches to not invalidate the reviews of the others and to allow a better comparison between the old and the new structure.
Ok I reviewd all patches except the last, which actually makes it harder as I would have to wrap my head again around 3/4 patches I already reviewed, so it would be really nice if it were folded in. I'll let Jakub decide what to do, maybe he will review the patch on ti's own and just ack it :)
Sumit explained it would be difficult to squash the patch into the previous set. In general, it would be nicer to have only a single set, but given our current constraints, I'm fine as long as the resulting code works and looks sane.
I'll review the patch separately.
On Mon, Jan 07, 2013 at 10:30:41AM +0100, Jakub Hrozek wrote:
On Fri, Jan 04, 2013 at 05:15:22PM -0500, Simo Sorce wrote:
On Fri, 2013-01-04 at 17:15 +0100, Sumit Bose wrote:
On Fri, Jan 04, 2013 at 04:31:03PM +0100, Jakub Hrozek wrote:
On Fri, Dec 21, 2012 at 12:31:32PM +0100, Sumit Bose wrote:
On Wed, Dec 19, 2012 at 08:10:02AM -0500, Simo Sorce wrote:
On Tue, 2012-12-18 at 21:43 +0100, Sumit Bose wrote: > > I started to change it but didn't like the resulting code, because it > turned out to be complex and hard to follow. Since the original struct > only user 8 or 12 bytes per group (depending if your are on 32bits or > on > 64bits) I would prefer to keep the old struct for this round of > patches. > Sorry can't agree, space is only one of the factors, although completely unnecessarily using 2x or 3x number of groups space is ugly enough, But the main issues are really in the code this structure allows/produces.
Still a nack for me.
The attached patch should be applied on top of the other patches and changes the struct as you suggested in your first comment.
bye, Sumit
Why not squash the patch into the patch that introduces the structure?
The structure was introduced in the first patch and then used in some of the other patches. I didn't squash the changes into the related patches to not invalidate the reviews of the others and to allow a better comparison between the old and the new structure.
Ok I reviewd all patches except the last, which actually makes it harder as I would have to wrap my head again around 3/4 patches I already reviewed, so it would be really nice if it were folded in. I'll let Jakub decide what to do, maybe he will review the patch on ti's own and just ack it :)
Sumit explained it would be difficult to squash the patch into the previous set. In general, it would be nicer to have only a single set, but given our current constraints, I'm fine as long as the resulting code works and looks sane.
I'll review the patch separately.
The last patch looks OK to me, I only asked Sumit to add some comments to place that wasn't so obvious in get_gids_from_pac().
I haven't read the previous patches too carefully, but I trust Simo's review.
On Mon, Jan 07, 2013 at 01:22:26PM +0100, Jakub Hrozek wrote:
On Mon, Jan 07, 2013 at 10:30:41AM +0100, Jakub Hrozek wrote:
On Fri, Jan 04, 2013 at 05:15:22PM -0500, Simo Sorce wrote:
On Fri, 2013-01-04 at 17:15 +0100, Sumit Bose wrote:
On Fri, Jan 04, 2013 at 04:31:03PM +0100, Jakub Hrozek wrote:
On Fri, Dec 21, 2012 at 12:31:32PM +0100, Sumit Bose wrote:
On Wed, Dec 19, 2012 at 08:10:02AM -0500, Simo Sorce wrote: > On Tue, 2012-12-18 at 21:43 +0100, Sumit Bose wrote: > > > > I started to change it but didn't like the resulting code, because it > > turned out to be complex and hard to follow. Since the original struct > > only user 8 or 12 bytes per group (depending if your are on 32bits or > > on > > 64bits) I would prefer to keep the old struct for this round of > > patches. > > > Sorry can't agree, space is only one of the factors, although completely > unnecessarily using 2x or 3x number of groups space is ugly enough, > But the main issues are really in the code this structure > allows/produces. > > Still a nack for me.
The attached patch should be applied on top of the other patches and changes the struct as you suggested in your first comment.
bye, Sumit
Why not squash the patch into the patch that introduces the structure?
The structure was introduced in the first patch and then used in some of the other patches. I didn't squash the changes into the related patches to not invalidate the reviews of the others and to allow a better comparison between the old and the new structure.
Ok I reviewd all patches except the last, which actually makes it harder as I would have to wrap my head again around 3/4 patches I already reviewed, so it would be really nice if it were folded in. I'll let Jakub decide what to do, maybe he will review the patch on ti's own and just ack it :)
Sumit explained it would be difficult to squash the patch into the previous set. In general, it would be nicer to have only a single set, but given our current constraints, I'm fine as long as the resulting code works and looks sane.
I'll review the patch separately.
The last patch looks OK to me, I only asked Sumit to add some comments to place that wasn't so obvious in get_gids_from_pac().
I haven't read the previous patches too carefully, but I trust Simo's review.
Simo, Jakub, thank you very much for the review. The new set of patches address all your latest comments.
bye, Sumit
On Tue, Jan 08, 2013 at 09:29:46AM +0100, Sumit Bose wrote:
On Mon, Jan 07, 2013 at 01:22:26PM +0100, Jakub Hrozek wrote:
On Mon, Jan 07, 2013 at 10:30:41AM +0100, Jakub Hrozek wrote:
On Fri, Jan 04, 2013 at 05:15:22PM -0500, Simo Sorce wrote:
On Fri, 2013-01-04 at 17:15 +0100, Sumit Bose wrote:
On Fri, Jan 04, 2013 at 04:31:03PM +0100, Jakub Hrozek wrote:
On Fri, Dec 21, 2012 at 12:31:32PM +0100, Sumit Bose wrote: > On Wed, Dec 19, 2012 at 08:10:02AM -0500, Simo Sorce wrote: > > On Tue, 2012-12-18 at 21:43 +0100, Sumit Bose wrote: > > > > > > I started to change it but didn't like the resulting code, because it > > > turned out to be complex and hard to follow. Since the original struct > > > only user 8 or 12 bytes per group (depending if your are on 32bits or > > > on > > > 64bits) I would prefer to keep the old struct for this round of > > > patches. > > > > > Sorry can't agree, space is only one of the factors, although completely > > unnecessarily using 2x or 3x number of groups space is ugly enough, > > But the main issues are really in the code this structure > > allows/produces. > > > > Still a nack for me. > > The attached patch should be applied on top of the other patches and > changes the struct as you suggested in your first comment. > > bye, > Sumit
Why not squash the patch into the patch that introduces the structure?
The structure was introduced in the first patch and then used in some of the other patches. I didn't squash the changes into the related patches to not invalidate the reviews of the others and to allow a better comparison between the old and the new structure.
Ok I reviewd all patches except the last, which actually makes it harder as I would have to wrap my head again around 3/4 patches I already reviewed, so it would be really nice if it were folded in. I'll let Jakub decide what to do, maybe he will review the patch on ti's own and just ack it :)
Sumit explained it would be difficult to squash the patch into the previous set. In general, it would be nicer to have only a single set, but given our current constraints, I'm fine as long as the resulting code works and looks sane.
I'll review the patch separately.
The last patch looks OK to me, I only asked Sumit to add some comments to place that wasn't so obvious in get_gids_from_pac().
I haven't read the previous patches too carefully, but I trust Simo's review.
Simo, Jakub, thank you very much for the review. The new set of patches address all your latest comments.
bye, Sumit
Both mine and Simo's comments were addressed, the code looks good to me and basic sanity testing went fine.
Ack
On Tue, Jan 08, 2013 at 02:11:47PM +0100, Jakub Hrozek wrote:
Simo, Jakub, thank you very much for the review. The new set of patches address all your latest comments.
bye, Sumit
Both mine and Simo's comments were addressed, the code looks good to me and basic sanity testing went fine.
Ack
Pushed all patches to master and sssd-1-9
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Simo Sorce -
Sumit Bose