<div dir="ltr">Hi Dmitri,<br><div class="gmail_extra"><br><br><div class="gmail_quote">2013/10/11 Dmitri Pal <span dir="ltr">&lt;<a href="mailto:dpal@redhat.com" target="_blank">dpal@redhat.com</a>&gt;</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>
&gt; On Fri, Oct 11, 2013 at 12:53:48PM +0200, Benjamin Franzke wrote:<br>
&gt;&gt; 2013/10/11 Sumit Bose &lt;<a href="mailto:sbose@redhat.com">sbose@redhat.com</a>&gt;<br>
&gt;&gt;<br>
&gt;&gt;&gt; On Thu, Oct 10, 2013 at 02:41:54PM +0200, Benjamin Franzke wrote:<br>
&gt;&gt;&gt;&gt; Thanks for your review!<br>
&gt;&gt;&gt;&gt;<br>
&gt;&gt;&gt; &lt;snip&gt;<br>
&gt;&gt;&gt;&gt; The patch with your concers fixed is attached.<br>
&gt;&gt;&gt; Thank you for the new version. I only have two comments left:<br>
&gt;&gt;&gt;<br>
&gt;&gt;&gt; +    if (handle == NULL || errmsg == NULL) {<br>
&gt;&gt;&gt; +        *errmsg = strerror(EINVAL);<br>
&gt;&gt;&gt;<br>
&gt;&gt;&gt; you cannot return an error message if errmsg == NULL.<br>
&gt;&gt;&gt;<br>
&gt;&gt; yes, of course not :)<br>
&gt;&gt; had to leave house and wanted to get the patch out before, that dumb<br>
&gt;&gt; mistake was the result :\<br>
&gt;&gt;<br>
&gt;&gt;&gt; +        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 &quot;non-zero on error&quot;.</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>
&gt;&gt;&gt; +    }<br>
&gt;&gt;&gt;<br>
&gt;&gt;&gt;<br>
&gt;&gt;&gt; +    err = sss_nss_getsidbyname(name, &amp;sid, &amp;id_type);<br>
&gt;&gt;&gt; +    if (err != 0)  {<br>
&gt;&gt;&gt; +        /* Might be a raw string representation of SID,<br>
&gt;&gt;&gt; +         * try converting that before returning an error. */<br>
&gt;&gt;&gt; +        if (sid_to_cifs_sid(ctx, name, csid) == 0)<br>
&gt;&gt;&gt; +            return 0;<br>
&gt;&gt;&gt; +<br>
&gt;&gt;&gt;<br>
&gt;&gt;&gt;  ^^^^^^^^ trailing whitespace<br>
&gt;&gt;&gt;<br>
&gt;&gt;&gt; Oops, again.. have them highlighted in my editor now ;)<br>
&gt;&gt;&gt; +        ctx_set_error(ctx, strerror(err));<br>
&gt;&gt;&gt; +        return -err;<br>
&gt;&gt;&gt; +    }<br>
&gt;&gt;&gt;<br>
&gt;&gt;&gt;<br>
&gt;&gt;&gt; bye,<br>
&gt;&gt;&gt; Sumit<br>
&gt;&gt;&gt;<br>
&gt;&gt; New patch attached, with these two things fixed.<br>
&gt; Code looks good, tests are working as expected, ACK.<br>
&gt;<br>
&gt; bye,<br>
&gt; Sumit<br>
&gt;<br>
</div><div class="im HOEnZb">&gt; _______________________________________________<br>
&gt; sssd-devel mailing list<br>
&gt; <a href="mailto:sssd-devel@lists.fedorahosted.org">sssd-devel@lists.fedorahosted.org</a><br>
&gt; <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>