[SSSD] [PATCHES] sss_cache tool invalidates records in memory cache

Jakub Hrozek jhrozek at redhat.com
Mon Sep 24 11:32:36 UTC 2012


On Mon, Sep 24, 2012 at 01:13:47PM +0200, Jakub Hrozek wrote:
> On Mon, Sep 24, 2012 at 12:15:06PM +0200, Michal Židek wrote:
> > On 09/20/2012 07:12 PM, Jakub Hrozek wrote:
> > >On Tue, Sep 18, 2012 at 01:26:20PM +0200, Michal Židek wrote:
> > >>On 09/17/2012 06:02 PM, Simo Sorce wrote:
> > >>>
> > >>>Hi Michal,
> > >>>you bring up a good point about the race with startup, but I am not
> > >>>totally convinced about the approach you used to address it.
> > >>>
> > >>>The race you point out could be easily resolved by having the init code
> > >>>unlink() the reset file w/o acting on it. That would address the problem
> > >>>of cleaning up stale file.
> > >>
> > >>unlink() is added to the nss_process_init in the new patch, to avoid
> > >>unnecessary cache cleaning when not needed.
> > >>
> > >>>
> > >>>Your solution though has another race, if someone runs sss_cache and at
> > >>>the same time the system wants to rotate logs you may get the monitor to
> > >>>ignore the legit SIGHUP and only clean the cache but not rotate the logs.
> > >>>
> > >>>This is why I planned to have the check in sssd_nss and always do log
> > >>>rotation (monitor always sends the SIGHUP around).
> > >>
> > >>You are right. I changed it to always do log rotation.
> > >>
> > >>Btw. monitor does not send SIGHUP to other process. Monitor
> > >>receives SIGHUP and tells other services what to do (via D-Bus).
> > >>Right now, 'rotate logs' and 'clear memory cache'(NSS only) messages
> > >>are sent from monitor_hup function. I've changed the comment in
> > >>the code to make it more obvious.
> > >>
> > >>>
> > >>>The other 'problem' with the current code is that you are 'leaking'
> > >>>information specific to the sssd_nss backend into the monitor code.
> > >>>This is not really a major issue, but I prefer to avoid these 'shortcuts'
> > >>>and keep the monitor dumber wrt frontend or provider specific features.
> > >>
> > >>Everything I considered unrelated to monitor is removed in the new
> > >>patch, but it still needs to know the NSS process name (nsssrv.h), to
> > >>be able to send it the request for cache clean up. It would not need it
> > >>if we had a single NSS specific function to handle the logrotate
> > >>request, which would do both logrotation and memcache clean up
> > >>(I had it this way before and I did not like it myself). I thing if
> > >>monitor knows that on SIGHUP (sent to monitor) all processes rotate
> > >>logs, it should also know that NSS needs to clear caches.
> > >>
> > >
> > >I think "leaking" the service name is OK or perhaps the lesser of all
> > >evils. I remember we even had a check in the past that would test if the
> > >list of configured services contains "nss" and "pam".
> > >
> > >>>
> > >>>Other nitpicks: in the current code if you get a fatal error do not do
> > >>>anything if the stat fails with an error different from ENOENT. I think
> > >>>the SIGHUP should always be sent (as I said above) and the failure should
> > >>>only be logged, not be fatal (of course this is not a problem if we move
> > >>>back checks into sssd_nss).
> > >>
> > >>I made the first possible unlink() error (the one that can happen in
> > >>nss_process_init) not fatal. I think sssd should at least try to
> > >>continue even if the caches might be removed on next log rotation
> > >>request.
> > >>
> > >>New patches are in attachment.
> > >>
> > >>Thanks
> > >>Michal
> > >>
> > >
> > >The code works as advertized.
> > >
> > >I only have some minor nitpicks style-wise:
> > >
> > >+static int nss_clear_memcache(DBusMessage *message,
> > >+                   struct sbus_connection *conn)
> > >
> > >Please align the line after the linebreak.
> > >
> > 
> > Fixed.
> > 
> > >+    struct nss_ctx *nctx = (struct nss_ctx*)rctx->pvt_ctx;
> > >(and elsewhere) Please put whitespace after the cast.
> > >
> > 
> > Fixed.
> > 
> > >sss_mmap_cache_reinit:
> > >We have a convention to call our "local" talloc contexts tmp_ctx. We usually
> > >also allocate them on NULL so that a run with valgrind would immediatelly
> > >reveal them as leaks[*]
> > >
> > 
> > Fixed. It is now allocated as new top level ctx.
> > 
> > >I'm not sure about this check:
> > >     +    ret = talloc_free(*mc_ctx);
> > >Is there actually a descructor? Wouldn't we want to continue and try to
> > >init the cache again instead of just failing?
> > >
> > 
> > I removed the goto statement, so we get only debug message and the
> > function continues normally.
> > 
> > >Why the whitespace change in server_setup() ?
> > >
> > 
> > Fixed.
> > 
> > >Otherwise I'm fine with the patches. Good work, Michal!
> 
> Ack

Pushed to master.



More information about the sssd-devel mailing list