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(a)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 do not remember us turning errors into negative numbers.
Is this a convention that I missed?
>> + }
>>
>>
>> + 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(a)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/