On 05/06/2010 02:45 PM, Stephen Gallagher wrote:
On 05/06/2010 02:32 PM, Stephen Gallagher wrote:
> On 05/06/2010 02:11 PM, Dmitri Pal wrote:
>> Stephen Gallagher wrote:
>>> ctx->gsh is sometimes NULL here, so dereferencing it without checking
>>> is bad.
>>> ------------------------------------------------------------------------
>>>
>>>
>>> _______________________________________________
>>> sssd-devel mailing list
>>> sssd-devel(a)lists.fedorahosted.org
>>>
https://fedorahosted.org/mailman/listinfo/sssd-devel
>> Through I do not have a problem with this specific patch I do have
>> several concerns about the underlaying code.
>>
>> 1) The code seems to be duplicated multiple times. Why common code is
>> not segmented into a common function?
>
> It is now :)
>
>> 2) What happens if the ctx->gsh is actually null? Is it safe to fall
>> through. May be a comment would help.
>
> Comment added.
>
>> 3) The code around sdap_account_info_restart(breq); is not consistent.
>> In first two cases we return in case of success. What do we do in case
>> of failure?
>> In the last function we on the contrary do not check the error and
>> return right away regardless of the outcome of the
>> sdap_account_info_restart(breq). This does not look right.
>>
>
> The third case was wrong. Good catch! I have now fixed this.
>
> New patch attached. Thanks for the review.
>
Sorry, accidentally attached the old patch. This should be the right one
now.
Wow, I really need sleep.
Maybe I'll get it right this time?
--
Stephen Gallagher
RHCE 804006346421761
Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/