I'm attaching an initial draft of the implementation of ignore_group_members per ticket #1376. I still need to update the documentation (and some python code in SSSDConfig it looks like), but functionality wise this prevents ldap from requesting the member attribute and sssd from returning any cached members that might be left in the local db. As this is my first attempt at working with sssd, I wanted to get early feedback in case I'm doing something silly ;).
Also, regarding the comment on the ticket:
"The trickiest piece of this functionality would be ensuring that we don't delete existing member/memberOf linkages from the cache during group lookups that were put there by previous initgroups() requests. Thus, when this option is in play, member/memberOf should only be managed by initgroups() calls."
My understanding of this is that an initgroups call will set up some state in the cache regarding members of groups, and a getgrnam or getgrgid call that skips retrieving the member attribute will wipe these out of the cache. However, it's also my understanding that *every* initgroups call hits ldap directly to make sure stale data isn't used for authorization purposes. If so, why do we care that the data in the cache, which isn't going to be used, gets wiped out? When ignore_group_members is enabled, the only thing that cares about group members is initgroups, correct?
Thanks...
On Fri, Nov 09, 2012 at 08:05:02PM -0800, Paul B. Henson wrote:
I'm attaching an initial draft of the implementation of ignore_group_members per ticket #1376. I still need to update the documentation (and some python code in SSSDConfig it looks like), but functionality wise this prevents ldap from requesting the member attribute and sssd from returning any cached members that might be left in the local db. As this is my first attempt at working with sssd, I wanted to get early feedback in case I'm doing something silly ;).
Also, regarding the comment on the ticket:
Thank you for the contribution! I will take a look at the patch..can you just send the patch as output of git format-patch in the future? It's easier for us to handle that way.
As per the documentation and the configAPI -- you can either let us worry about that part as we already know which parts need to be modified or you can check out commit 1542b85f13d72329685bdd97aa879c36d11f81be, for example. I think that patch added/changed all the correct pieces at the correct places.
"The trickiest piece of this functionality would be ensuring that we don't delete existing member/memberOf linkages from the cache during group lookups that were put there by previous initgroups() requests. Thus, when this option is in play, member/memberOf should only be managed by initgroups() calls."
My understanding of this is that an initgroups call will set up some state in the cache regarding members of groups, and a getgrnam or getgrgid call that skips retrieving the member attribute will wipe these out of the cache.
Yes, that's my understanding, too. I think the way the code is built now would treat the attributes as missing and delete them during a getgrnam or getgrgid call.
However, it's also my understanding that *every* initgroups call hits ldap directly to make sure stale data isn't used for authorization purposes. If so, why do we care that the data in the cache, which isn't going to be used, gets wiped out? When ignore_group_members is enabled, the only thing that cares about group members is initgroups, correct?
The SSSD always retrieves initgroups data during an authentication request that comes through the PAM stack for exactly the reason you cited. For lookups that only occur through the name-service-switch module, the SSSD caches the data for entry_cache_user_timeout by default.
So maybe the code could be modifed to read the existing group memberships and fake the results it got from LDAP to also include these memberships?
On 11/12/2012 4:12 AM, Jakub Hrozek wrote:
Thank you for the contribution! I will take a look at the patch..can you just send the patch as output of git format-patch in the future? It's easier for us to handle that way.
As per the documentation and the configAPI -- you can either let us
Sorry; the diff was just a quickie to get some initial feedback. I've attached a patch that should include everything except possible changes to cache handling.
Yes, that's my understanding, too. I think the way the code is built now would treat the attributes as missing and delete them during a getgrnam or getgrgid call.
[...]
The SSSD always retrieves initgroups data during an authentication request that comes through the PAM stack for exactly the reason you cited. For lookups that only occur through the name-service-switch module, the SSSD caches the data for entry_cache_user_timeout by default.
I'm not completely clear on what you mean about retrieving initgroups data via the PAM stack. My understanding is that the supplementary groups are set via the initgroups(3) call, which uses nss to look up what groups a particular user belongs to and then calls setgroups to set them. Or are you talking about access control in the pam stack provided for example by simple_allow_groups/simple_deny_groups?
So maybe the code could be modifed to read the existing group memberships and fake the results it got from LDAP to also include these memberships?
Hmm, look up the current cached groups and spoof them in the response so the cache handling code won't delete them? That sounds a bit kludgy :). The mechanism mentioned in the original ticket of having the cache update code only activate for initgroups lookups and not getgr lookups seems cleaner.
I guess I need to dig a bit deeper in the code and try to understand the scenario where not adding this additional cache handling exception will cause a problem. With the new option enabled, it would seem the only time group membership is relevant is for initgroups() calls.
Hmm, okay, I found an anomaly:
# id -a henson uid=1000(henson) gid=1000(henson) groups=1000(henson),1866(cppnet-admin),1402(iit),3285(iit_staff),14988(iit_operations),18445(nt-mgr),22212(iit_ex_spam_out-admin),1865(cppnet),1020(intranet),14528(iit_systems),17730(unxadmin),21016(eoc_essential),23306(iit_vmware_admin),23380(netadmin),1406(staff),15379(noc),23358(employees),23359(members),19289(idm_sysadmin),19295(idm_restricted),19317(iit_systems_staff),20795(campus_techs),23668(iit_ops_systems)
# id -a henson uid=1000(henson) gid=1000(henson) groups=1000(henson)
With the new option enabled, the first time I do an 'id -a', it lists all of my groups; the next time (and subsequent times), they're gone 8-/. Presumably the getgr lookups of each individual group after my group list was obtained wiped out the memberships as warned about in the ticket.
I'll see about fixing it up so this doesn't happen.
Thanks
"The trickiest piece of this functionality would be ensuring that we don't delete existing member/memberOf linkages from the cache during group lookups that were put there by previous initgroups() requests. Thus, when this option is in play, member/memberOf should only be managed by initgroups() calls."
I think I've tweaked the code such that this is true, proposed patch attached. Review would be good :), I'm not completely sure I understood the request flow, so while my initial testing seemed to indicate this was working correctly there could be edge cases that aren't handled right.
Thanks...
On Tue, 2012-11-13 at 13:40 -0800, Paul B. Henson wrote:
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c index 036e88f..1182532 100644 --- a/src/responder/nss/nsssrv_cmd.c +++ b/src/responder/nss/nsssrv_cmd.c @@ -2035,24 +2035,25 @@ static int fill_grent(struct sss_packet *packet, pwfield.str, pwfield.len);
memnum = 0;
el = ldb_msg_find_element(msg, SYSDB_MEMBERUID);
if (el) {
ret = fill_members(packet, dom, nctx, el, &rzero, &rsize,
&memnum);
if (ret != EOK) {
num = 0;
goto done;
if (!dom->ignore_group_members) {
el = ldb_msg_find_element(msg, SYSDB_MEMBERUID);
if (el) {
ret = fill_members(packet, dom, nctx, el, &rzero,
&rsize, &memnum);
if (ret != EOK) {
num = 0;
goto done;
}
sss_packet_get_body(packet, &body, &blen); }
sss_packet_get_body(packet, &body, &blen);
}
el = ldb_msg_find_element(msg, SYSDB_GHOST);
if (el) {
ret = fill_members(packet, dom, nctx, el, &rzero, &rsize,
&memnum);
if (ret != EOK) {
num = 0;
goto done;
el = ldb_msg_find_element(msg, SYSDB_GHOST);
if (el) {
ret = fill_members(packet, dom, nctx, el, &rzero,
&rsize, &memnum);
if (ret != EOK) {
num = 0;
goto done;
}
sss_packet_get_body(packet, &body, &blen); }
sss_packet_get_body(packet, &body, &blen); } if (memnum) {
Is this part really necessary ?
If you do not fetch members from LDAP then memberuid will usually be empty anyway. In any case even if there is something (initgroups ?) then what you have there is only members that actually have logged in and that *is* information you may want to have so that applications that may (naively) use a getgrnam() call in order to check membership of a specific user will not fail at least for logged in users.
IMO it looks like you should simply drop the changes to this file.
Simo.
On Tue, Nov 13, 2012 at 08:08:20PM -0500, Simo Sorce wrote:
Is this part really necessary ?
If you do not fetch members from LDAP then memberuid will usually be empty anyway. In any case even if there is something (initgroups ?) then what you have there is only members that actually have logged in and that *is* information you may want to have so that applications that may (naively) use a getgrnam() call in order to check membership of a specific user will not fail at least for logged in users.
Without this part, getgr calls will sometimes return members, and sometimes not, depending on the state of the local cache and what else has been looked up, as well as how long the ignore_group_members option has been set (if you look up a group without that option set, set it and restart sssd, you'll get all members of the group as they're still in the cache).
It seems a lot more consistent to simply never return any members than to sporadically return some members sometimes. However, my main goal for the feature is to avoid the (relatively) expensive LDAP lookup, so functionally it doesn't really matter too much to me. You guys surely have a better idea of how you want the software to operate than a drive-by contributor ;), so I won't argue against dropping that piece if that's what the concensus is among primary developers.
Thanks...
On Tue, 2012-11-13 at 17:59 -0800, Paul B. Henson wrote:
On Tue, Nov 13, 2012 at 08:08:20PM -0500, Simo Sorce wrote:
Is this part really necessary ?
If you do not fetch members from LDAP then memberuid will usually be empty anyway. In any case even if there is something (initgroups ?) then what you have there is only members that actually have logged in and that *is* information you may want to have so that applications that may (naively) use a getgrnam() call in order to check membership of a specific user will not fail at least for logged in users.
Without this part, getgr calls will sometimes return members, and sometimes not, depending on the state of the local cache and what else has been looked up, as well as how long the ignore_group_members option has been set (if you look up a group without that option set, set it and restart sssd, you'll get all members of the group as they're still in the cache).
It seems a lot more consistent to simply never return any members than to sporadically return some members sometimes. However, my main goal for the feature is to avoid the (relatively) expensive LDAP lookup, so functionally it doesn't really matter too much to me. You guys surely have a better idea of how you want the software to operate than a drive-by contributor ;), so I won't argue against dropping that piece if that's what the concensus is among primary developers.
Thanks...
Well my concern is allowing people to get the perf. benefit you need, as you may not be the only one who needs it, w/o causing issues for those apps that will use getgrnam() or getgrgid() to check stuff.
Maybe we can make this second part optional by changing your parameter into a tristate, where you can go from normal, to no ldap lookups but return what we have in cache, to never return anything even from the cache.
I am not dead set on this, because there is value in simplicity as well as better predictability too, so I'll let other give a thought and comment as well.
Simo.
On Nov 13, 2012, at 9:06 PM, Simo Sorce simo@redhat.com wrote:
Well my concern is allowing people to get the perf. benefit you need, as you may not be the only one who needs it, w/o causing issues for those apps that will use getgrnam() or getgrgid() to check stuff.
If you have a reliance on an app using getgr lookups, you probably shouldn't enable an option called "ignore_group_members" ;). The trade off you're making for that increased performance is that you're not going to get group members via that interface.
I just don't see a mechanism that returns a subset of members depending on who's happened to log in recently as desirable. From a sysadmin perspective, I'd rather it reproducibly returns no members than return a potentially different subset on each call. "Hmm, the app seems to work right after bob logs in but then stops working a few hours later"...
On Wed 14 Nov 2012 01:24:15 AM EST, Paul B. Henson wrote:
On Nov 13, 2012, at 9:06 PM, Simo Sorce simo@redhat.com wrote:
Well my concern is allowing people to get the perf. benefit you need, as you may not be the only one who needs it, w/o causing issues for those apps that will use getgrnam() or getgrgid() to check stuff.
If you have a reliance on an app using getgr lookups, you probably shouldn't enable an option called "ignore_group_members" ;). The trade off you're making for that increased performance is that you're not going to get group members via that interface.
I just don't see a mechanism that returns a subset of members depending on who's happened to log in recently as desirable. From a sysadmin perspective, I'd rather it reproducibly returns no members than return a potentially different subset on each call. "Hmm, the app seems to work right after bob logs in but then stops working a few hours later"...
For what it's worth, I agree with Paul here. If someone is setting 'ignore_group_members', they're already explicitly stating that they don't want to see group members come back from 'getgrnam()'. We should be guaranteed consistency here.
On Wed, 2012-11-14 at 08:48 -0500, Stephen Gallagher wrote:
On Wed 14 Nov 2012 01:24:15 AM EST, Paul B. Henson wrote:
On Nov 13, 2012, at 9:06 PM, Simo Sorce simo@redhat.com wrote:
Well my concern is allowing people to get the perf. benefit you
need, as
you may not be the only one who needs it, w/o causing issues for
those
apps that will use getgrnam() or getgrgid() to check stuff.
If you have a reliance on an app using getgr lookups, you probably
shouldn't enable an option called "ignore_group_members" ;). The trade off you're making for that increased performance is that you're not going to get group members via that interface.
I just don't see a mechanism that returns a subset of members
depending on who's happened to log in recently as desirable. From a sysadmin perspective, I'd rather it reproducibly returns no members than return a potentially different subset on each call. "Hmm, the app seems to work right after bob logs in but then stops working a few hours later"...
For what it's worth, I agree with Paul here. If someone is setting 'ignore_group_members', they're already explicitly stating that they don't want to see group members come back from 'getgrnam()'. We should be guaranteed consistency here.
Ok, put down this way it tips my opinion toward the currently proposed patch.
Simo.
On Wed, 2012-11-14 at 10:59 -0800, Paul B. Henson wrote:
On Wed, Nov 14, 2012 at 09:04:24AM -0500, Simo Sorce wrote:
Ok, put down this way it tips my opinion toward the currently proposed patch.
Cool. Is there anything else I need to do to move this forward?
No we just need someone to test and ack your patch.
Simo.
On Wed 14 Nov 2012 02:28:07 PM EST, Simo Sorce wrote:
On Wed, 2012-11-14 at 10:59 -0800, Paul B. Henson wrote:
On Wed, Nov 14, 2012 at 09:04:24AM -0500, Simo Sorce wrote:
Ok, put down this way it tips my opinion toward the currently proposed patch.
Cool. Is there anything else I need to do to move this forward?
No we just need someone to test and ack your patch.
Simo.
First of all, thank you very much for the contribution. It's a great start. I've got a few bits and pieces for you to clean up before we commit it upstream.
Minor: Please use the new SSSDBG macros in confdb_get_domain_internal(). You don't need to update the existing code, but all new code should use the macros. See util.h for a listing of them.
Build issue: ../src/providers/ldap/ldap_id.c: In function 'groups_get_send': ../src/providers/ldap/ldap_id.c:448:32: error: passing argument 4 of 'build_attrs_from_map' from incompatible pointer type [-Werror] In file included from ../src/providers/ldap/ldap_common.h:26:0,its o from ../src/providers/ldap/ldap_id.c:32: ../src/providers/ldap/sdap.h:481:5: note: expected 'const char **' but argument is of type 'char **' cc1: all warnings being treated as errors
I recommend building with: make CFLAGS+="-ggdb3 -O0 -Wall -Wextra -Wno-unused-parameter -Wno-sign-compare -Wformat-security -Werror"
Test issue: .....F...F... ====================================================================== FAIL: testListOptions (__main__.SSSDConfigTestSSSDDomain) ---------------------------------------------------------------------- Traceback (most recent call last): File "./../src/config/SSSDConfigTest.py", line 539, in testListOptions option) AssertionError: Option [ignore_group_members] unexpectedly found
====================================================================== FAIL: testRemoveProvider (__main__.SSSDConfigTestSSSDDomain) ---------------------------------------------------------------------- Traceback (most recent call last): File "./../src/config/SSSDConfigTest.py", line 879, in testRemoveProvider option) AssertionError: Option [ignore_group_members] unexpectedly found
---------------------------------------------------------------------- Ran 13 tests in 0.094s
FAILED (failures=2)
You need to add 'ignore_group_members' to the tuple of expected values in the test in SSSDConfigTest.py
Style issue: /* TODO: handle attrs_type */ ret = build_attrs_from_map(state, ctx->opts->group_map, SDAP_OPTS_GROUP, - NULL, &state->attrs, NULL); + state->domain->ignore_group_members ? + member_filter : NULL, &state->attrs, NULL);
Please indent the rest of the ?: structure so that it's clear that member_filter : NULL is part of the above. &state->attrs, NULL should be on the next line. It'll read easier that way.
In sdap_get_groups_done(), wouldn't it just be easier to use !state->dom->ignore_group_members with a comment as to what that means? The if-then-else construct is a bit confusing there.
nsssrv_cmd.c has added a tab at the beginning of the line if (!dom->ignore_group_members) { SSSD is standardized on four-space indentation. (And my editor lights it up like neon when tabs are added)
Other than that, the code looks good. I haven't had a chance to test it yet, but I'll try to get to that in the next two days.
On 11/14/2012 1:41 PM, Stephen Gallagher wrote:
Minor: Please use the new SSSDBG macros in confdb_get_domain_internal(). You don't need to update the existing code, but all new code should use the macros. See util.h for a listing of them.
Done.
../src/providers/ldap/sdap.h:481:5: note: expected 'const char **' but argument is of type 'char **'
Fixed.
You need to add 'ignore_group_members' to the tuple of expected values in the test in SSSDConfigTest.py
D'oh, I actually had an open editor buffer with that change in it that it seems I lost track of and never saved 8-/.
Please indent the rest of the ?: structure so that it's clear that member_filter : NULL is part of the above. &state->attrs, NULL should be on the next line. It'll read easier that way.
Done.
!state->dom->ignore_group_members with a comment as to what that means? The if-then-else construct is a bit confusing there.
Good suggestion, I guess I had just had ternaries on my mind from the previous use :).
nsssrv_cmd.c has added a tab at the beginning of the line if (!dom->ignore_group_members) {
Oops, I try to play chameleon when working on other people's code bases but I guess my editor slipped that one past me.
I've attached an updated patch that I believe addresses all of your issues, let me know if I missed something or anything else needs a tuneup.
Thanks...
On Wed 14 Nov 2012 06:39:06 PM EST, Paul B. Henson wrote:
On 11/14/2012 1:41 PM, Stephen Gallagher wrote:
Minor: Please use the new SSSDBG macros in confdb_get_domain_internal(). You don't need to update the existing code, but all new code should use the macros. See util.h for a listing of them.
Done.
../src/providers/ldap/sdap.h:481:5: note: expected 'const char **' but argument is of type 'char **'
Fixed.
You need to add 'ignore_group_members' to the tuple of expected values in the test in SSSDConfigTest.py
D'oh, I actually had an open editor buffer with that change in it that it seems I lost track of and never saved 8-/.
Please indent the rest of the ?: structure so that it's clear that member_filter : NULL is part of the above. &state->attrs, NULL should be on the next line. It'll read easier that way.
Done.
!state->dom->ignore_group_members with a comment as to what that means? The if-then-else construct is a bit confusing there.
Good suggestion, I guess I had just had ternaries on my mind from the previous use :).
nsssrv_cmd.c has added a tab at the beginning of the line if (!dom->ignore_group_members) {
Oops, I try to play chameleon when working on other people's code bases but I guess my editor slipped that one past me.
I've attached an updated patch that I believe addresses all of your issues, let me know if I missed something or anything else needs a tuneup.
Thanks...
I've run some tests on this today. With the option unset or False, behavior appears to be properly unmodified from the existing code (Good). I ran a simple performance test on a moderately complex AD environment I already had set up.
That environment has 3000 users that are a members of a series of nested groups. I have the following baseline for performance (on a completely empty cache):
[root@sgallagh520 db]# time id kurashima@adtest uid=201145(kurashima) gid=200513(domain users) groups=200513(domain users),204697(abovetopgroup),201108(topgroup),201109(nestedgroup),204699(aboveeventhat),204695(uppergroup),201107(sssdgroup),204698(aboveabovetop)
real 0m1.795s user 0m0.002s sys 0m0.009s
Changing only the ignore_group_members output, this becomes:
[root@sgallagh520 db]# time id kurashima@adtest uid=201145(kurashima) gid=200513(domain users) groups=200513(domain users),204697(abovetopgroup),201108(topgroup),201109(nestedgroup),204699(aboveeventhat),204695(uppergroup),201107(sssdgroup),204698(aboveabovetop)
real 0m0.441s user 0m0.002s sys 0m0.007s
This is obviously already a significant enhancement, and of course the difference will be more pronounced for much larger environments. I'm prepared to give this an ack, with one comment to whoever pushes the patch upstream: please reflow the changes in nsssrv_cmd.c's fill_grent() that are now exceeding the 80-character line limit due to the new indentation.
On 11/15/2012 5:45 AM, Stephen Gallagher wrote:
This is obviously already a significant enhancement, and of course the difference will be more pronounced for much larger environments. I'm prepared to give this an ack, with one comment to whoever pushes the patch upstream: please reflow the changes in nsssrv_cmd.c's fill_grent() that are now exceeding the 80-character line limit due to the new indentation.
Hmm, I see a few other lines in that file that exceed 80 characters; are there scenarios where there is an exception to that limit or were those just not caught ;)?
It looks like there were only two lines in my diff for that file that exceeded 80 characters, the attached updated patch should fix that.
Thanks for the ack. Will this just go into head or will it be pushed back to the 1.9 branch? I've got an open RHEL support ticket requesting this be back ported to RHEL 6, given they're supposed to be shipping 1.9 in RHEL 6.4, it might be easier to get them to include it if it becomes part of the 1.9 branch :). Or if whoever commits this "accidentally" slips it into the RHEL 6 repo while they're twiddling bits ;)...
On Thu, Nov 15, 2012 at 10:47:10AM -0800, Paul B. Henson wrote:
On 11/15/2012 5:45 AM, Stephen Gallagher wrote:
This is obviously already a significant enhancement, and of course the difference will be more pronounced for much larger environments. I'm prepared to give this an ack, with one comment to whoever pushes the patch upstream: please reflow the changes in nsssrv_cmd.c's fill_grent() that are now exceeding the 80-character line limit due to the new indentation.
Hmm, I see a few other lines in that file that exceed 80 characters; are there scenarios where there is an exception to that limit or were those just not caught ;)?
They slipped through the review, most probably. There's no hard rule, the 80-char limit just makes the code more readable for people like me who like to split their terminals vertically.
It looks like there were only two lines in my diff for that file that exceeded 80 characters, the attached updated patch should fix that.
Looks good to me, now. There's one more log line in groups_get_send but that one is a) hard to split, so it's a reasonable exception and b) that whole function doesn't respect the limit.
I tested the patch as well, works fine, the code looks great to me, to..
Thank you very much for the contribution!
Thanks for the ack. Will this just go into head or will it be pushed back to the 1.9 branch?
Currently just head, we might backport it later. I try to keep the 1.9 brach quite close to the RHEL6.4 codebase.
I've got an open RHEL support ticket requesting this be back ported to RHEL 6, given they're supposed to be shipping 1.9 in RHEL 6.4, it might be easier to get them to include it if it becomes part of the 1.9 branch :).
Yes, this is in fact the best way to go. Bugzillas coming from paying customers tend to get higher priority.
Or if whoever commits this "accidentally" slips it into the RHEL 6 repo while they're twiddling bits ;)...
Then my boss would "accidentally" fire me, I'm afraid :-)
On Thu, Nov 15, 2012 at 08:01:34PM +0100, Jakub Hrozek wrote:
On Thu, Nov 15, 2012 at 10:47:10AM -0800, Paul B. Henson wrote:
On 11/15/2012 5:45 AM, Stephen Gallagher wrote:
This is obviously already a significant enhancement, and of course the difference will be more pronounced for much larger environments. I'm prepared to give this an ack, with one comment to whoever pushes the patch upstream: please reflow the changes in nsssrv_cmd.c's fill_grent() that are now exceeding the 80-character line limit due to the new indentation.
Hmm, I see a few other lines in that file that exceed 80 characters; are there scenarios where there is an exception to that limit or were those just not caught ;)?
They slipped through the review, most probably. There's no hard rule, the 80-char limit just makes the code more readable for people like me who like to split their terminals vertically.
It looks like there were only two lines in my diff for that file that exceeded 80 characters, the attached updated patch should fix that.
Looks good to me, now. There's one more log line in groups_get_send but that one is a) hard to split, so it's a reasonable exception and b) that whole function doesn't respect the limit.
I tested the patch as well, works fine, the code looks great to me, to..
Thank you very much for the contribution!
I included the ticket URL in the commit message and pushed the patch to master.
On Thu, Nov 15, 2012 at 08:01:34PM +0100, Jakub Hrozek wrote:
Yes, this is in fact the best way to go. Bugzillas coming from paying customers tend to get higher priority.
Thanks for pushing the commit. I've started making noise on my support case and the redhat bugzilla (looks like you saw the latter ;) ), and will follow up through those channels...
Sorry for the OT ...
On Thu, 2012-11-15 at 20:01 +0100, Jakub Hrozek wrote:
They slipped through the review, most probably. There's no hard rule, the 80-char limit just makes the code more readable for people like me who like to split their terminals vertically.
Although I am not an emacs user I find this page exaplains very well why 80 columns are used and gives you a better understanding of why I pushed hard to have this rule in the coding style: http://www.emacswiki.org/emacs/EightyColumnRule
It's a bit more than "because I like it" :-)
Simo.
On Thu, Nov 15, 2012 at 08:01:34PM +0100, Jakub Hrozek wrote:
Currently just head, we might backport it later. I try to keep the 1.9 brach quite close to the RHEL6.4 codebase.
This option is really handy and used by many users. Any opposition against pushing it to the 1.9 branch?
On 05/31/2013 06:14 AM, Jakub Hrozek wrote:
On Thu, Nov 15, 2012 at 08:01:34PM +0100, Jakub Hrozek wrote:
Currently just head, we might backport it later. I try to keep the 1.9 brach quite close to the RHEL6.4 codebase.
This option is really handy and used by many users. Any opposition against pushing it to the 1.9 branch? _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
As long as we do not create too much overhead for us I am fine.
On Fri, May 31, 2013 at 10:23:00AM -0400, Dmitri Pal wrote:
On 05/31/2013 06:14 AM, Jakub Hrozek wrote:
On Thu, Nov 15, 2012 at 08:01:34PM +0100, Jakub Hrozek wrote:
Currently just head, we might backport it later. I try to keep the 1.9 brach quite close to the RHEL6.4 codebase.
This option is really handy and used by many users. Any opposition against pushing it to the 1.9 branch? _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
As long as we do not create too much overhead for us I am fine.
I think it's fine, the patch is quite simple and wouldn't cause the 1.9 branch and RHEL6 codebase to diverge. Keeping them close is still my goal.
On Fri, 2013-05-31 at 16:27 +0200, Jakub Hrozek wrote:
On Fri, May 31, 2013 at 10:23:00AM -0400, Dmitri Pal wrote:
On 05/31/2013 06:14 AM, Jakub Hrozek wrote:
On Thu, Nov 15, 2012 at 08:01:34PM +0100, Jakub Hrozek wrote:
Currently just head, we might backport it later. I try to keep the 1.9 brach quite close to the RHEL6.4 codebase.
This option is really handy and used by many users. Any opposition against pushing it to the 1.9 branch? _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
As long as we do not create too much overhead for us I am fine.
I think it's fine, the patch is quite simple and wouldn't cause the 1.9 branch and RHEL6 codebase to diverge. Keeping them close is still my goal.
Pushed to sssd-1-9
sssd-devel@lists.fedorahosted.org