On 10/26/2012 07:53 PM, Simo Sorce wrote:
On Fri, 2012-10-26 at 18:33 +0200, Michal Židek wrote:
--- a/src/responder/nss/nsssrv_mmap_cache.c
+++ b/src/responder/nss/nsssrv_mmap_cache.c
@@ -537,9 +537,46 @@ static errno_t sss_mc_create_file(struct
sss_mc_ctx *mc_ctx)
mode_t old_mask;
int ofd;
int ret;
+ int retries_left;
+ struct flock mc_lock;
+ useconds_t t = 50000;
+
+ mc_lock.l_type = F_WRLCK;
+ mc_lock.l_whence = SEEK_SET;
+ mc_lock.l_start = 0;
+ mc_lock.l_start = 1;
Something wrong here ^ this should be l_len
+ mc_lock.l_pid = getpid();
You do not need to set this, it's a read-out variable, and should be
initialized to 0.
ofd = open(mc_ctx->file, O_RDWR);
if (ofd != -1) {
+ for (retries_left = 2; retries_left >= 0; retries_left--) {
nitpick but this would be more readable as:
for (retries_left = 3; retries_left > 0; retries_left--) {
however I see it would require (retries_left - 1) in the debug message,
so if you prefer the current form add a comment that says /* try 3 times
*/ before the for loop.
@@ -570,6 +607,34 @@ static errno_t sss_mc_create_file(struct
sss_mc_ctx *mc_ctx)
mc_ctx->file, ret,
strerror(ret)));
}
+ for (retries_left = 2; retries_left >= 0; retries_left--) {
+ ret = fcntl(mc_ctx->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 mc file %s. Retries left: %d\n",
+ mc_ctx->file, retries_left));
+ 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 mc file %s\n", mc_ctx->file));
+ return ret;
+ }
+ }
+ if (retries_left < 0) {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ ("Unable to lock mc file %s\n", mc_ctx->file));
+ return ret;
+ }
+
This block is the same as above, please do not repeat blocks of code,
create a static helper function instead and use it in both places.
+errno_t sss_memcache_invalidate(const char *mc_filename)
+{
+ int mc_fd = -1;
+ struct flock mc_lock;
+ errno_t ret;
+ bool locked = false;
+
+ 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_start = 1;
+ mc_lock.l_pid = getpid();
Same comments as per sssd_nss code.
+ ret = fcntl(mc_fd, F_SETLK, &mc_lock);
+ if (ret == -1) {
+ ret = errno;
+ if (ret == EACCES || ret == EAGAIN || ret == EINTR) {
+ /* File already locked by sssd_nss */
+ ret = EACCES;
+ goto done;
EINTR must be handled differently here, I would retry at least once on
EINTR as it doesn't mean the file is locked, only that the syscall was
interrupted before the lock could be attempted.
(The server case is different because it makes no difference for what
reason you fail, there you have to retry anyway, here only for EINTR).
+ } else {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ ("Failed to lock memory cache file %s: %s\n",
+ mc_filename, strerror(ret)));
+ goto done;
+ }
+ }
+ locked = true;
+
+ /* 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 (locked) {
+ mc_lock.l_type = F_UNLCK;
+ fcntl(mc_fd, F_SETLK, &mc_lock);
+ }
+
+ if (mc_fd != -1) {
+ close(mc_fd);
+ }
Shouldn't you also unlink the file here ?
That is not necessary, the file will be deleted by sssd_nss. But I added
the unlink here in the new patch, so it is more obvious, that the file
is no longer needed.
+ return ret;
+}
In general the approach is correct and the patch is ok, please fix the
bugs and will get in master right away.
Simo.
I also changed one debug message in the new patch. In
sss_mc_create_file, if we fail to mark the old cache as recycled, we
simply continue (failing here would not prevent the processes that use
the old cache from using it). I think the message that informs about
this failure should be level 0.
Thanks for the review.
New Patch is attached.
NOTE: Just want to remind that this patch applies on top of the [SSSD]
[PATCH] sss_cache: Multiple domains not handled properly
Michal
>From d5ef27bd966787772e85d61830789cd0727234e8 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 | 62 ++++++++++++++++-
src/tools/sss_cache.c | 78 +++++++++++++++------
src/tools/tools_util.c | 125 ++++++++++++++++++++++++++++++++++
src/tools/tools_util.h | 2 +
4 files changed, 246 insertions(+), 21 deletions(-)
diff --git a/src/responder/nss/nsssrv_mmap_cache.c b/src/responder/nss/nsssrv_mmap_cache.c
index f402564..8a4f537 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,9 +582,14 @@ 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"
+ DEBUG(SSSDBG_FATAL_FAILURE, ("Failed to mark mmap file %s as"
" recycled: %d(%s)\n",
mc_ctx->file, ret, strerror(ret)));
}
@@ -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..f8bdce6 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,127 @@ 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;
+ bool locked = false;
+ 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;
+ }
+ }
+ }
+ locked = true;
+
+ /* 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 (locked) {
+ mc_lock.l_type = F_UNLCK;
+ fcntl(mc_fd, F_SETLK, &mc_lock);
+ }
+
+ if (mc_fd != -1) {
+ 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
