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

Jakub Hrozek jhrozek at redhat.com
Mon Sep 24 11:13:47 UTC 2012


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



More information about the sssd-devel mailing list