On Sat, Oct 12, 2013 at 06:21:26AM +0200, Benjamin Franzke wrote:
Hi Dmitri,
2013/10/11 Dmitri Pal <dpal(a)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(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 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.
I'm sorry but I have to ask for another revision. I think you added the
changes Lukas and Dmitri asked for to your original patch and not to the
one where you e.g. already changed uint8_t -> const uint8_t.
bye,
Sumit
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(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/
>
>
>
> _______________________________________________
> sssd-devel mailing list
> sssd-devel(a)lists.fedorahosted.org
>
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel