Hi Dmitri,


2013/10/11 Dmitri Pal <dpal@redhat.com>
On 10/11/2013 07:47 AM, Sumit Bose wrote:
> On Fri, Oct 11, 2013 at 12:53:48PM +0200, Benjamin Franzke wrote:
>> 2013/10/11 Sumit Bose <sbose@redhat.com>
>>
>>> On Thu, Oct 10, 2013 at 02:41:54PM +0200, Benjamin Franzke wrote:
>>>> Thanks for your review!
>>>>
>>> <snip>
>>>> The patch with your concers fixed is attached.
>>> Thank you for the new version. I only have two comments left:
>>>
>>> +    if (handle == NULL || errmsg == NULL) {
>>> +        *errmsg = strerror(EINVAL);
>>>
>>> you cannot return an error message if errmsg == NULL.
>>>
>> yes, of course not :)
>> had to leave house and wanted to get the patch out before, that dumb
>> mistake was the result :\
>>
>>> +        return -EINVAL;



Why -EINVAL?

I did that basicly because the winbind plugin did that as well.
Of course, that doenst mean we have to do that,
since the cifsidmap.h header just says "non-zero on error".

I do not remember us turning errors into negative numbers.
Is this a convention that I missed?


Yeah, i see from a quick git grep, that this is really unusual in sssd.
Changed to positive values in the attached patch.

Thanks!




>>> +    }
>>>
>>>
>>> +    err = sss_nss_getsidbyname(name, &sid, &id_type);
>>> +    if (err != 0)  {
>>> +        /* Might be a raw string representation of SID,
>>> +         * try converting that before returning an error. */
>>> +        if (sid_to_cifs_sid(ctx, name, csid) == 0)
>>> +            return 0;
>>> +
>>>
>>>  ^^^^^^^^ trailing whitespace
>>>
>>> Oops, again.. have them highlighted in my editor now ;)
>>> +        ctx_set_error(ctx, strerror(err));
>>> +        return -err;
>>> +    }
>>>
>>>
>>> bye,
>>> Sumit
>>>
>> New patch attached, with these two things fixed.
> Code looks good, tests are working as expected, ACK.
>
> bye,
> Sumit
>
> _______________________________________________
> sssd-devel mailing list
> sssd-devel@lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel


--
Thank you,
Dmitri Pal

Sr. Engineering Manager for IdM portfolio
Red Hat Inc.


-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/



_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel