First I'd like to point out that I could not try full functionality since there are some pieces missing on the server side. That said, I have couple minor comments for some patches.
Patch #0001: In pac_responder.m4 you are checking for the right version of kerberos. Would it be possible to make the check more generic, like "if the version is 1.9 or higher"? Otherwise we would need to update this every time new Kerberos is out. The error message should be corrected accordingly (right now it only specifies Kerberos 1.9 is necessary) idmap_talloc() and idmap_free(): I just noticed that we use practically the same functions in ldap provider (sdap_idmap.c). How about moving both functions to util/ and utilize them both in ldap provider and pac responder? Patch #0002: Ack Patch #0003: Nitpick: pac_store_memberships() should be more accuratelly called pac_store_membership() > > You are not freeing pr_ctx in some error cases. The only place pr_ctx is > > free'd is pac_save_memberships_done(), i.e. only when everything went ok. > > > > Please add some talloc_free() calls where necessary. My first guess is > > that pac_get_domains_done() and pac_add_pac_user() are the only two > > places where it is necessary. > > I have added a talloc_free() at both places. But since pr_ctx is a child > of the client context which is freed shortly afterwards one might argue > if the talloc_free() is even needed in pac_save_memberships_done(). But > I would agree with you that it is a good idea to remove pr_ctx when it > is not needed anymore. Just for the record, I don't think the cctx context is freed before responder starts shutting down. That's why I asked you to free those pr_ctx contexts. Patch #0004: Please add build dependencies (conditionally of course) to our example spec file. Patch #0005: Ack Patch #0006: Ack Patch #0007: Are there any changes that should be done in spec file for this? Nitpick: the "if" statement on line 361 of the patch is missing curly braces. The same on line 425. Thanks Jan _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel