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.