On 10/29/2012 03:53 PM, Simo Sorce wrote:
Codewise looks ok, but I still see duplication of the code used to lock the file. I was wondering, would it make sense to split this patch in 2 and put the lock_mc_file functions as a more generic function in the common utils as a separate patch first ? I would see it added in util/util_lock.c so it is available to both tools and responders (avoids the duplication you still have). The prototype should be something like: static errno_t sss_br_lock_file(int fd, size_t start, size_t len, int retries, int wait); Where wait is expressed in milliseconds.
I was thinking about this too, but there is one thing I do not like about it. Maybe I am wrong, but IMO functions related to memory cache should be as private to sssd_nss as possible. That is why the function for locking the files is static (sssd_nss part). Function for memory cache is invalidation is an exception to this rule, because it is used by sss_cache, but it should also keep the code that manipulates memory cache private (and it uses it only on one place, so static function was not needed that much).
I do not know about any other place, where we would like to lock the mc files except of sssd_nss and the sss_memcache_invalidate used by sss_cache and it should stay like this. I consider the usage of mc files in sss_cache as necessary pollution, but providing non-static API to spread this pollution does not sound good to me.
But if you really want the util_lock.c, I can send it as a separate patch on top of this one (it is only a small refactor). But still... I am not sure if this is good idea.
Also you do not really need to unlock the file if you are going to close() it. Posix semantics require the OS to drop all locks on a file if you close it, so you can safely drop the logic around unlocking in tools_util.c (however add a comment before the close like: /* closing the file will drop the lock */ ).
I thought closing the file explicitly is more readable. But yes, I'll remove it, adding the comment as you suggested is better.
Simo.
New patch is attached. Thanks Michal
>From a9f653ff67a61bd8e68016592023c9fdcfdacf42 Mon Sep 17 00:00:00 2001 From: Michal Zidek <[email protected]> Date: Fri, 26 Oct 2012 17:36:51 +0200 Subject: [PATCH] sss_cache: Remove fastcache even if sssd is not running. https://fedorahosted.org/sssd/ticket/1584 --- src/responder/nss/nsssrv_mmap_cache.c | 66 ++++++++++++++++++- src/tools/sss_cache.c | 78 ++++++++++++++++------ src/tools/tools_util.c | 119 ++++++++++++++++++++++++++++++++++ src/tools/tools_util.h | 2 + 4 files changed, 242 insertions(+), 23 deletions(-) diff --git a/src/responder/nss/nsssrv_mmap_cache.c b/src/responder/nss/nsssrv_mmap_cache.c index f402564..159e099 100644 --- a/src/responder/nss/nsssrv_mmap_cache.c +++ b/src/responder/nss/nsssrv_mmap_cache.c @@ -527,6 +527,48 @@ static errno_t sss_mc_set_recycled(int fd) return EOK; } +static errno_t lock_mc_file(int mc_fd) +{ + int ret; + useconds_t t = 50000; + struct flock mc_lock; + int retries_left; + + mc_lock.l_type = F_WRLCK; + mc_lock.l_whence = SEEK_SET; + mc_lock.l_start = 0; + mc_lock.l_len = 1; + mc_lock.l_pid = 0; + + for (retries_left = 3; retries_left > 0; retries_left--) { + ret = fcntl(mc_fd, F_SETLK, &mc_lock); + if (ret == EACCES || ret == EAGAIN || ret == EINTR) { + /* File is locked by someone else */ + DEBUG(SSSDBG_TRACE_FUNC, + ("Failed to lock mmap cache file. Retries left: %d\n", + retries_left - 1)); + ret = usleep(t); + if (ret == -1) { + DEBUG(SSSDBG_MINOR_FAILURE, + ("usleep() failed -> ignoring\n")); + } + } else if (ret == 0) { + /* File successfuly locked */ + break; + } else { + /* Error occurred */ + DEBUG(SSSDBG_CRIT_FAILURE, ("Unable to lock mmap cache file.\n")); + return ret; + } + } + if (retries_left < 0) { + DEBUG(SSSDBG_CRIT_FAILURE, ("Unable to lock mmap cache file.\n")); + return ret; + } + + return EOK; +} + /* * When we (re)create a new file we must mark the current file as recycled * so active clients will abandon its use asap. @@ -540,11 +582,16 @@ static errno_t sss_mc_create_file(struct sss_mc_ctx *mc_ctx) ofd = open(mc_ctx->file, O_RDWR); if (ofd != -1) { + ret = lock_mc_file(ofd); + if (ret != EOK) { + DEBUG(SSSDBG_FATAL_FAILURE, + ("Failed to lock file %s.\n", mc_ctx->file)); + } ret = sss_mc_set_recycled(ofd); if (ret) { - DEBUG(SSSDBG_TRACE_FUNC, ("Failed to mark mmap file %s as" - " recycled: %d(%s)\n", - mc_ctx->file, ret, strerror(ret))); + DEBUG(SSSDBG_FATAL_FAILURE, ("Failed to mark mmap file %s as" + " recycled: %d(%s)\n", + mc_ctx->file, ret, strerror(ret))); } close(ofd); @@ -568,11 +615,24 @@ static errno_t sss_mc_create_file(struct sss_mc_ctx *mc_ctx) ret = errno; DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to open mmap file %s: %d(%s)\n", mc_ctx->file, ret, strerror(ret))); + goto done; } + ret = lock_mc_file(mc_ctx->fd); + if (ret != EOK) { + DEBUG(SSSDBG_FATAL_FAILURE, + ("Failed to lock file %s.\n", mc_ctx->file)); + goto done; + } + +done: /* reset mask back */ umask(old_mask); + if (ret) { + close(mc_ctx->fd); + } + return ret; } diff --git a/src/tools/sss_cache.c b/src/tools/sss_cache.c index 36e425c..a647294 100644 --- a/src/tools/sss_cache.c +++ b/src/tools/sss_cache.c @@ -92,6 +92,7 @@ errno_t invalidate_entry(TALLOC_CTX *ctx, struct sysdb_ctx *sysdb, bool invalidate_entries(TALLOC_CTX *ctx, struct sysdb_ctx *sysdb, enum sss_cache_entry entry_type, const char *filter, const char *name); +static int clear_fastcache(bool *sssd_nss_is_off); int main(int argc, const char *argv[]) { @@ -99,6 +100,7 @@ int main(int argc, const char *argv[]) struct cache_tool_ctx *tctx = NULL; struct sysdb_ctx *sysdb; int i; + bool sssd_nss_is_off; bool skipped = true; FILE *clear_mc_flag; @@ -143,29 +145,36 @@ int main(int argc, const char *argv[]) ret = ENOENT; goto done; } else { - /*Local cache changed -> signal monitor to invalidate fastcache */ - clear_mc_flag = fopen(SSS_NSS_MCACHE_DIR"/"CLEAR_MC_FLAG, "w"); - if (clear_mc_flag == NULL) { - DEBUG(SSSDBG_CRIT_FAILURE, - ("Failed to create clear_mc_flag file. " - "Memory cache will not be cleared.\n")); + ret = clear_fastcache(&sssd_nss_is_off); + if (ret != EOK) { + DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to clear caches.\n")); goto done; } - ret = fclose(clear_mc_flag); - if (ret != 0) { - ret = errno; - DEBUG(SSSDBG_CRIT_FAILURE, - ("Unable to close file descriptor: %s\n", - strerror(ret))); - goto done; - } + if (!sssd_nss_is_off) { + /* sssd_nss is running -> signal monitor to invalidate fastcache */ + clear_mc_flag = fopen(SSS_NSS_MCACHE_DIR"/"CLEAR_MC_FLAG, "w"); + if (clear_mc_flag == NULL) { + DEBUG(SSSDBG_CRIT_FAILURE, + ("Failed to create clear_mc_flag file. " + "Memory cache will not be cleared.\n")); + goto done; + } + ret = fclose(clear_mc_flag); + if (ret != 0) { + ret = errno; + DEBUG(SSSDBG_CRIT_FAILURE, + ("Unable to close file descriptor: %s\n", + strerror(ret))); + goto done; + } - DEBUG(SSSDBG_TRACE_FUNC, ("Sending SIGHUP to monitor.\n")); - ret = signal_sssd(SIGHUP); - if (ret != EOK) { - DEBUG(SSSDBG_CRIT_FAILURE, - ("Failed to send SIGHUP to monitor.\n")); - goto done; + DEBUG(SSSDBG_TRACE_FUNC, ("Sending SIGHUP to monitor.\n")); + ret = signal_sssd(SIGHUP); + if (ret != EOK) { + DEBUG(SSSDBG_CRIT_FAILURE, + ("Failed to send SIGHUP to monitor.\n")); + goto done; + } } } @@ -174,6 +183,33 @@ done: return ret; } +static int clear_fastcache(bool *sssd_nss_is_off) +{ + int ret; + ret = sss_memcache_invalidate(SSS_NSS_MCACHE_DIR"/passwd"); + if (ret != EOK) { + if (ret == EACCES) { + *sssd_nss_is_off = false; + return EOK; + } else { + return ret; + } + } + + ret = sss_memcache_invalidate(SSS_NSS_MCACHE_DIR"/group"); + if (ret != EOK) { + if (ret == EACCES) { + *sssd_nss_is_off = false; + return EOK; + } else { + return ret; + } + } + + *sssd_nss_is_off = true; + return EOK; +} + bool invalidate_entries(TALLOC_CTX *ctx, struct sysdb_ctx *sysdb, enum sss_cache_entry entry_type, const char *filter, const char *name) @@ -549,3 +585,5 @@ search_autofsmaps(TALLOC_CTX *mem_ctx, struct sysdb_ctx *sysdb, return ENOSYS; #endif /* BUILD_AUTOFS */ } + + diff --git a/src/tools/tools_util.c b/src/tools/tools_util.c index 049a4f5..f729da6 100644 --- a/src/tools/tools_util.c +++ b/src/tools/tools_util.c @@ -35,6 +35,7 @@ #include "db/sysdb.h" #include "tools/tools_util.h" #include "tools/sss_sync_ops.h" +#include "util/mmap_cache.h" static int setup_db(struct tools_ctx *ctx) { @@ -671,3 +672,121 @@ errno_t signal_sssd(int signum) return EOK; } + +static errno_t sss_mc_set_recycled(int fd) +{ + uint32_t w = SSS_MC_HEADER_RECYCLED; + struct sss_mc_header h; + off_t offset; + off_t pos; + int ret; + + + offset = MC_PTR_DIFF(&h.status, &h); + + pos = lseek(fd, offset, SEEK_SET); + if (pos == -1) { + /* What do we do now ? */ + return errno; + } + + errno = 0; + ret = sss_atomic_write_s(fd, (uint8_t *)&w, sizeof(h.status)); + if (ret == -1) { + return errno; + } + + if (ret != sizeof(h.status)) { + /* Write error */ + return EIO; + } + + return EOK; +} + +errno_t sss_memcache_invalidate(const char *mc_filename) +{ + int mc_fd = -1; + struct flock mc_lock; + errno_t ret; + useconds_t t = 50000; + int retries_left; + + if (!mc_filename) { + return EINVAL; + } + + mc_fd = open(mc_filename, O_RDWR); + if (mc_fd == -1) { + ret = errno; + if (ret == ENOENT) { + DEBUG(SSSDBG_TRACE_FUNC,("Memory cache file %s " + "does not exist.\n", mc_filename)); + return EOK; + } else { + DEBUG(SSSDBG_CRIT_FAILURE, ("Unable to open file %s: %s\n", + mc_filename, strerror(ret))); + return ret; + } + } + + mc_lock.l_type = F_WRLCK; + mc_lock.l_whence = SEEK_SET; + mc_lock.l_start = 0; + mc_lock.l_len = 1; + mc_lock.l_pid = 0; + + for (retries_left = 2; retries_left > 0; retries_left--) { + ret = fcntl(mc_fd, F_SETLK, &mc_lock); + if (ret == 0) { + /* File locked */ + break; + } else if (ret == -1) { + ret = errno; + if (ret == EACCES || ret == EAGAIN) { + /* File already locked by sssd_nss */ + ret = EACCES; + goto done; + } else if (EINTR) { + /* Try again in 50 ms */ + DEBUG(SSSDBG_TRACE_FUNC, + ("Failed to lock file %s. Retries left %d\n", + mc_filename, retries_left - 1)); + ret = usleep(t); + if (ret == -1) { + DEBUG(SSSDBG_MINOR_FAILURE, + ("usleep() failed -> ignoring\n")); + } + } else { + DEBUG(SSSDBG_CRIT_FAILURE, + ("Failed to lock memory cache file %s: %s\n", + mc_filename, strerror(ret))); + goto done; + } + } + } + + /* Mark the mc file as recycled. */ + ret = sss_mc_set_recycled(mc_fd); + if (ret != EOK) { + DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to mark memory cache file %s " + "as recycled.\n", mc_filename)); + goto done; + } + + ret = EOK; +done: + if (mc_fd != -1) { + /* Closing the file also releases the lock */ + close(mc_fd); + ret = unlink(mc_filename); + if (ret == -1) { + DEBUG(SSSDBG_MINOR_FAILURE, + ("Failed to unlink file %s. " + "Will be unlinked later by sssd_nss.\n")); + ret = EOK; + } + } + return ret; +} + diff --git a/src/tools/tools_util.h b/src/tools/tools_util.h index 1be17e8..a83c8ee 100644 --- a/src/tools/tools_util.h +++ b/src/tools/tools_util.h @@ -104,6 +104,8 @@ int run_userdel_cmd(struct tools_ctx *tctx); errno_t signal_sssd(int signum); +errno_t sss_memcache_invalidate(const char *mc_filename); + /* from files.c */ int remove_tree(const char *root); -- 1.7.11.2
_______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
