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