<div dir="ltr">Hi Dmitri,<br><div class="gmail_extra"><br><br><div class="gmail_quote">2013/10/11 Dmitri Pal <span dir="ltr"><<a href="mailto:dpal@redhat.com" target="_blank">dpal@redhat.com</a>></span><br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">On 10/11/2013 07:47 AM, Sumit Bose wrote:<br>
> On Fri, Oct 11, 2013 at 12:53:48PM +0200, Benjamin Franzke wrote:<br>
>> 2013/10/11 Sumit Bose <<a href="mailto:sbose@redhat.com">sbose@redhat.com</a>><br>
>><br>
>>> On Thu, Oct 10, 2013 at 02:41:54PM +0200, Benjamin Franzke wrote:<br>
>>>> Thanks for your review!<br>
>>>><br>
>>> <snip><br>
>>>> The patch with your concers fixed is attached.<br>
>>> Thank you for the new version. I only have two comments left:<br>
>>><br>
>>> + if (handle == NULL || errmsg == NULL) {<br>
>>> + *errmsg = strerror(EINVAL);<br>
>>><br>
>>> you cannot return an error message if errmsg == NULL.<br>
>>><br>
>> yes, of course not :)<br>
>> had to leave house and wanted to get the patch out before, that dumb<br>
>> mistake was the result :\<br>
>><br>
>>> + return -EINVAL;<br>
<br>
<br>
<br>
</div>Why -EINVAL?<br></blockquote><div><br></div><div>I did that basicly because the winbind plugin did that as well.</div><div>Of course, that doenst mean we have to do that,</div><div>since the cifsidmap.h header just says "non-zero on error".</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I do not remember us turning errors into negative numbers.<br>
Is this a convention that I missed?<br></blockquote><div><br></div><div><br></div><div>Yeah, i see from a quick git grep, that this is really unusual in sssd.</div><div>Changed to positive values in the attached patch.</div>
<div><br></div><div>Thanks!</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im HOEnZb"><br>
<br>
<br>
>>> + }<br>
>>><br>
>>><br>
>>> + err = sss_nss_getsidbyname(name, &sid, &id_type);<br>
>>> + if (err != 0) {<br>
>>> + /* Might be a raw string representation of SID,<br>
>>> + * try converting that before returning an error. */<br>
>>> + if (sid_to_cifs_sid(ctx, name, csid) == 0)<br>
>>> + return 0;<br>
>>> +<br>
>>><br>
>>> ^^^^^^^^ trailing whitespace<br>
>>><br>
>>> Oops, again.. have them highlighted in my editor now ;)<br>
>>> + ctx_set_error(ctx, strerror(err));<br>
>>> + return -err;<br>
>>> + }<br>
>>><br>
>>><br>
>>> bye,<br>
>>> Sumit<br>
>>><br>
>> New patch attached, with these two things fixed.<br>
> Code looks good, tests are working as expected, ACK.<br>
><br>
> bye,<br>
> Sumit<br>
><br>
</div><div class="im HOEnZb">> _______________________________________________<br>
> sssd-devel mailing list<br>
> <a href="mailto:sssd-devel@lists.fedorahosted.org">sssd-devel@lists.fedorahosted.org</a><br>
> <a href="https://lists.fedorahosted.org/mailman/listinfo/sssd-devel" target="_blank">https://lists.fedorahosted.org/mailman/listinfo/sssd-devel</a><br>
<br>
<br>
</div><span class="HOEnZb"><font color="#888888">--<br>
Thank you,<br>
Dmitri Pal<br>
<br>
Sr. Engineering Manager for IdM portfolio<br>
Red Hat Inc.<br>
<br>
<br>
-------------------------------<br>
Looking to carve out IT costs?<br>
<a href="http://www.redhat.com/carveoutcosts/" target="_blank">www.redhat.com/carveoutcosts/</a><br>
</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
<br>
_______________________________________________<br>
sssd-devel mailing list<br>
<a href="mailto:sssd-devel@lists.fedorahosted.org">sssd-devel@lists.fedorahosted.org</a><br>
<a href="https://lists.fedorahosted.org/mailman/listinfo/sssd-devel" target="_blank">https://lists.fedorahosted.org/mailman/listinfo/sssd-devel</a><br>
</div></div></blockquote></div><br></div></div>