[SSSD] [PATCH] CIFS idmap Plugin using SSSD

Sumit Bose sbose at redhat.com
Tue Oct 15 09:41:49 UTC 2013


On Mon, Oct 14, 2013 at 01:05:22PM +0200, Sumit Bose wrote:
> 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

pushed to master.

bye,
Sumit

> 
> bye,
> Sumit
> 
> > 
> > 
> > 2013/10/14 Sumit Bose <sbose at redhat.com>
> > 
> > > On Sat, Oct 12, 2013 at 06:21:26AM +0200, Benjamin Franzke wrote:
> > > > Hi Dmitri,
> > > >
> > > >
> > > > 2013/10/11 Dmitri Pal <dpal at 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 at 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 at 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 at lists.fedorahosted.org
> > > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > > >
> > >
> > >
> > > > _______________________________________________
> > > > sssd-devel mailing list
> > > > sssd-devel at lists.fedorahosted.org
> > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > >
> > >
> 
> 
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel



More information about the sssd-devel mailing list