On Thu, May 24, 2012 at 07:17:31AM -0400, Stephen Gallagher wrote:
> On Thu, 2012-05-24 at 10:47 +0200, Jakub Hrozek wrote:
> > On Wed, May 23, 2012 at 01:42:58PM -0400, Stephen Gallagher wrote:
> > > On Wed, 2012-05-23 at 16:52 +0200, Jakub Hrozek wrote:
> > > > On Wed, May 23, 2012 at 09:03:16AM -0400, Stephen Gallagher wrote:
> > > > > If the mmap cache cannot be initialized (such as insufficient
> > > > > permissions or SELinux/AppArmor denial), we are supposed to fall
> > > > > our 1.8 behavior of LDB cache only. However, we weren't
> > > > > checking that the cache had been set up and we were always
> > > > > dereference the mmap context in fill_pwent() and fill_grent().
> > > > >
> > > > > Fixes https://fedorahosted.org/sssd/ticket/1346
> > > >
> > > > I think we should do the same for sss_mmap_cache_gr_store, too. And
> > > > perhaps do the check inside the functions themselves and not the
> > > > so that we don't forget to add the check when we add a new call
> > > > functions.
> > >
> > > How do you think we should handle the return code if we add it to the
> > > functions themselves? EOK would seem to be hiding things, but ENOMEM
> > > (insufficient space in the cache) seems a little hackish.
> > I was thinking logging a MINOR_FAILURE and returning EOK.
> Logging a minor failure would be much too noisy. We'd be polluting the
> log with an error they may not be able to fix.
> > But checking if the context exists in the caller is fine, too, we just
> > need to do it for both _pw_store and _gr_store.
> Can you point out where I missed one? My patch fixes both fill_pwent()
> and fill_grent(). Grepping through the sources shows me only those two
> locations using sss_mmap_cache_*_store().
You're right, you fixed both. I don't know why I didn't see that. Sorry
for the noise.