On Mon, Oct 14, 2013 at 11:00:44AM +0200, Benjamin Franzke wrote: > Oh, sorry, I shared an old branch between my devel machine and the testing > virtual machine, where i did some other changes, which i merged later on, > which caused this. Sorry.. > I've merge the changes again.. And gone through the notes in this thread, > to ensure they are all included now.
Thanks a lot. ACK bye, Sumit > > > 2013/10/14 Sumit Bose <sb...@redhat.com> > > > On Sat, Oct 12, 2013 at 06:21:26AM +0200, Benjamin Franzke wrote: > > > Hi Dmitri, > > > > > > > > > 2013/10/11 Dmitri Pal <d...@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 <sb...@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@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 > > > > > > > > > > > _______________________________________________ > > > sssd-devel mailing list > > > sssd-devel@lists.fedorahosted.org > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel > > > > _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel