On Mon, 2012-01-09 at 16:30 -0500, Stephen Gallagher wrote:
On Tue, 2012-01-03 at 18:00 -0500, Simo Sorce wrote:
> >From [PATCH 0/0] A shared memory cache to perform better:
>
> 0/4: Actual memory cache implementation
> These is the bulk of the work, these patches are still a bit rough at
> the edges, grep for FIXMEs and TODOs and you'll see some plumbing (for
> example configure options in sssd to set expiration time and cache sizes
> are missing and are still harcoded).
Patch 0008: Ack
Patch 0009: Nack
As the fixmes say, please parameterize the cache sizes and timeout
period.
You have a time-of-check/time-of-use bug with the stat of mc_ctx->file.
Please open it first and then use fstat() instead, to avoid a
race-condition exploit.
Not sure what race condition is there.
sssd_nss is the only thing that can create/manipulate that file, in what
case do you see a race condition ?
The problem of using fstat() is that it unnecessarily(IMO) complicates
the code for this case.
Please use errno_t for the return code of functions that return
errno
values.
Ok.
I think the check_header pieces are unnecessary. When the NSS
provider
is started, it should always remove an existing fast cache. This will
make the behavior better match users' expectations.
Not sure about this, it unnecessarily causes cache lookups to go though
the pipe to re-populate the fast cache. Perhaps we need to discuss a bit
more pros-cons of either approach.
Don't use ftruncate() to generate the cache file. It's not
safe on all
platforms or filesystems. Probably better to use fseek() (even though
it's a bit slower. It's a rare operation).
I asked our filesystem gurus and they think ftruncate() is the best way
to go. (and the kernel sources confirm it works with every local file
systems even VFAT :-)
I will review the other patches tomorrow.
Thanks!
Simo.
--
Simo Sorce * Red Hat, Inc * New York