On 14.5.2012 18:37, Jakub Hrozek wrote:
On Mon, May 14, 2012 at 05:55:09PM +0200, Pavel Březina wrote:On 11.5.2012 12:23, Pavel Březina wrote:On 05/07/2012 07:17 PM, Pavel Březina wrote:On 05/03/2012 10:30 PM, Pavel Březina wrote:On 3.5.2012 13:48, Jakub Hrozek wrote:On Fri, Apr 27, 2012 at 01:21:30PM +0200, Jakub Hrozek wrote:On Wed, Apr 25, 2012 at 05:05:36PM +0200, Pavel Březina wrote:The subdomains patches messed with one line which was causing troubles. Here are the rebased patches.On 04/23/2012 12:51 PM, Pavel Březina wrote:On 04/23/2012 10:03 AM, Jakub Hrozek wrote:On Fri, Apr 13, 2012 at 12:46:01PM +0200, Pavel Březina wrote:On 03/26/2012 09:47 PM, Jakub Hrozek wrote:On Mon, Mar 19, 2012 at 02:46:02PM +0100, Pavel Březina wrote:https://fedorahosted.org/sssd/ticket/1239 [PATCH 1/2] Finally removes EOK constant from sudo api header. It is not used in the SUDO code so it does not require their changes.Looks good to me, it'w weird to use EOK in an external header (even though it is correctly defined). (Note: this patch shouldn't be pushed even though I don't have any comments. It is an ABI break and we need to coordinate with Daniel K.)[PATCH 2/2] This does what is requested in the ticket. It seems to be very huge but in fact it is mainly changing the variable. Basically I tried to get rid of domain ctx where possible, leave it only in initgroups part and use command ctx elsewhere.Still, it is hard to review the huge patch. Can you split it into smaller ones? What about creating one that removes the duplication between _get_sudorules and _get_defaults, one that converts to using cmdctx and one that adds the uid support?The patches are attached. I want to get those patches acked because I need to deliver the updated interface to Dan so he can update the sudo binary. It does not implement the in-memory cache (yet? :-)) due to the discussion with simo. The patches expects "sudo api: check sss_status instead of errnop in sss_sudo_send_recv_generic()" (already acked on the list and waiting to be pushed).Patch 0001: sudo api: remove EOK Ack Patch 0002: sudo responder: remove code duplication in commands I think it would be a good idea to also check if rawname[rawname_len] == '\0' when parsing username, this would ensure that all the string checks work OK.Done.Now that several sudosrv_response_append_* functions were removed from header, can you make them static? In particular sudosrv_response_append_string(), sudosrv_response_append_uint32(), sudosrv_response_append_rule(), sudosrv_response_append_attr().Done.Patch 0003: sudo sysdb: make sysdb_get_sudo_user_info more configurable Ack Patch 0004: sudo api: send uid, username and domainname Please get rid of the C++ comments in sudosrv_cmd(). If you need to leave code in but disabled, please use #if 0/#endifThanks. I'm using keybord shortcut to comment out whole block with these slashes and I fortgot that we have this rule :) The whole hunk thatchanges sudosrv_cmd() seems like it belongs to patch #2.Not really. It requires the new protocol which is implemented in this patch. It makes the two command more similar than they was before.There's another // comment in sudosrv_get_sudorules_from_cache. The logic looks good to me, though.Thank you for the review. I'm attaching a 6th patch that removes all code related to the in-memory cache as it will not be used anymore.Patch 0001: Ack Patch 0002: Ack Patch 0003: Ack Patch 0004: Ack Patch 0005: Ack Patch 0006: Nack, does to apply on master: $ git reset --hard origin/master HEAD is now at 24ba5b8 NSS: Only return data from initgroups once $ git am *.patch Applying: sudo api: remove EOK Applying: sudo responder: remove code duplication in commands Applying: sudo responder: get rid of dctx where possible Applying: sudo sysdb: make sysdb_get_sudo_user_info more configurable Applying: sudo api: send uid, username and domainname Applying: sudo responder: discard in-memory cache error: patch failed: src/responder/sudo/sudosrv_cache.c:1I'm sorry, but it works for me. Are you sure you have the correct patches? What exactly fails to merge? $ git reset --hard origin/master HEAD is now at 24ba5b8 NSS: Only return data from initgroups once $ git am *.patch Applying: sudo api: remove EOK Applying: sudo responder: remove code duplication in commands Applying: sudo responder: get rid of dctx where possible Applying: sudo sysdb: make sysdb_get_sudo_user_info more configurable Applying: sudo api: send uid, username and domainname Applying: sudo responder: discard in-memory cache $I'm sending it again formatted via git format-patch -M -C --patience --full-index and tarred.Thank you, I'm not sure which one of the changes did it, but I'm able to apply the patches now.There was one big mistake in the fifth patch. I was passing wrong uid information into query_cache function.I have added a patch that increases protocol number. If client uses old protocol, responder will return error immediately.Sorry, one last change..can you change --version to --version-info and bump the "current" number in the version string to 1?
Patches are attached.
I know the sudo part is not yet ready. But I would like to get these patches pushed - I don't want to rebase them again :) It does not break sudo until sss is present in nsswitch.conf.FWIW, I don't like breaking running systems. Yes, there is probably very few people who use sudo and sssd and would be affected by this, but still.. I don't have any issues with the code, I'll ack it when the version is bumped.
patches.tar.gz
Description: GNU Zip compressed data
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
