[SSSD] [PATCHES] HBAC access control rewrite

Jakub Hrozek jhrozek at redhat.com
Tue Jun 28 13:57:08 UTC 2011


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.



More information about the sssd-devel mailing list