On 06/28/2011 08:14 AM, Stephen Gallagher wrote: > On Mon, 2011-06-27 at 18:31 -0400, Jakub Hrozek wrote: >> 0002: ACK >> >> 0003: NACK >> The defattr in "%files -n libipa_hbac-devel" is missing a comma before >> the last parameter, it says "%defattr(-,root,root-)". >> > > Whoops, I could have sworn I fixed that... > >> hbac_free_error(): I wonder if it makes sense to add an explicit "if >> (!error) return". I think it is expected that most free functions accept >> NULL as a noop and it is quite useful for goto-cleanup cases. >> > > Yeah, that was an oversight. I converted it from talloc_free() (while > eliminating the talloc requirement) and didn't think about the NULL > case. >
ACK >> 0004: ACK >> >> 0005: ACK >> >> 0006: ACK >> >> 0007: ACK, >> altough the description of ipa_hbac_refresh was added to >> config/SSSDConfig.py in patch 0008. Feel free to fix or not, I don't >> think this should be a nack as the patchset will be applied as a whole >> anyway. >> > > Oops. Looks like I squashed that into the wrong patch. Fixed. ACK > >> >> For the rest of the patches - the code seems to be OK sans the one >> little issue below. I will defer the "architectural" review to Simo as >> the original comments were based on his PAC development experience. >> >> 0008: NACK >> Please free tmp_ctx in the "immediate:" label in >> ipa_hbac_update_groups_send() >> > > Thanks, I missed that. > >> The "/* Not found as a user; Check Groups*/" seems to be misleading, the >> code never checks users. >> > > That was a relic from before I decided to live with IPA-isms. Originally > I was doing a sysdb_search_user() to eliminate users from consideration. > I've removed this comment. > ACK >> 0009: ACK >> >> 0010: ACK >> >> 0011: ACK >> >> 0012: ACK > > > Updated patches attached. > > I also tried applying the python bindings on top of the latest patches and running the testsuite - it passed. So unless there are any specific requirements on changing the bindings, I think they are ready for review as well. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel