On 10/30/2012 02:25 PM, Simo Sorce wrote:
On Tue, 2012-10-30 at 10:39 +0100, Michal Židek wrote:
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).
This is why I made it a generic function not strictly related to the
cache in the prototype.
I prefer to keep a single function to be used by all components, because
copy&paste is a source of pain in the long term.
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.
The function prototype I suggested is not mc specific. Look at it, it
can be used with any file to lock any part, we can also extend it to
take a lock type if you want it even more generic.
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.
I think it should be a patch that comes before the current one and just
provides the facility, then the current patch will build on top of it.
Ok. I did not add the parameter to select the type of lock (not all
types would fit to this function). But we can expand the util_lock.c
later with additional functions if more lock types are needed.
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.
Yup, thanks, this part looks good, I think we only need to split the
patch and move the locking function into util, then we are golden.
Simo.
New patches attached.
Thanks
Michal
>From 2cf78527d7f345fcf0121823289fbbf308087f68 Mon Sep 17 00:00:00 2001
From: Michal Zidek <[email protected]>
Date: Wed, 31 Oct 2012 11:05:31 +0100
Subject: [PATCH 1/2] util: Added new file util_lock.c
---
Makefile.am | 3 +-
src/util/util.h | 4 +++
src/util/util_lock.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 90 insertions(+), 1 deletion(-)
create mode 100644 src/util/util_lock.c
diff --git a/Makefile.am b/Makefile.am
index ee7e198..f2655ff 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -507,7 +507,8 @@ libsss_util_la_SOURCES = \
src/util/murmurhash3.c \
src/util/atomic_io.c \
src/util/sss_selinux.c \
- src/util/domain_info_utils.c
+ src/util/domain_info_utils.c \
+ src/util/util_lock.c
libsss_util_la_LIBADD = \
$(SSSD_LIBS) \
$(UNICODE_LIBS) \
diff --git a/src/util/util.h b/src/util/util.h
index a96f519..dbb5f14 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -552,6 +552,10 @@ struct sss_domain_info *new_subdomain(TALLOC_CTX *mem_ctx,
struct sss_domain_info *copy_subdomain(TALLOC_CTX *mem_ctx,
struct sss_domain_info *subdomain);
+/* from util_lock.c */
+errno_t sss_br_lock_file(int fd, size_t start, size_t len,
+ int retries, useconds_t wait);
+
/* Endianness-compatibility for systems running older versions of glibc */
#ifndef le32toh
diff --git a/src/util/util_lock.c b/src/util/util_lock.c
new file mode 100644
index 0000000..c9c43d8
--- /dev/null
+++ b/src/util/util_lock.c
@@ -0,0 +1,84 @@
+/*
+ SSSD
+
+ util_lock.c
+
+ Authors:
+ Michal Zidek <[email protected]>
+
+ Copyright (C) 2012 Red Hat
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>.
+*/
+
+#include <unistd.h>
+#include <fcntl.h>
+#include <errno.h>
+
+#include "util/util.h"
+
+errno_t sss_br_lock_file(int fd, size_t start, size_t len,
+ int retries, useconds_t wait)
+{
+ int ret;
+ struct flock lock;
+ int retries_left;
+
+ lock.l_type = F_WRLCK;
+ lock.l_whence = SEEK_SET;
+ lock.l_start = start;
+ lock.l_len = len;
+ lock.l_pid = 0;
+
+ for (retries_left = retries; retries_left > 0; retries_left--) {
+ ret = fcntl(fd, F_SETLK, &lock);
+ if (ret == -1) {
+ ret = errno;
+ if (ret == EACCES || ret == EAGAIN || ret == EINTR) {
+ DEBUG(SSSDBG_TRACE_FUNC,
+ ("Failed to lock mmap cache file. Retries left: %d\n",
+ retries_left - 1));
+
+ if ((ret == EACCES || ret == EAGAIN) && (retries_left <= 1)) {
+ /* File is locked by someone else. Return EACCESS
+ * if this is the last try. */
+ return EACCES;
+ }
+
+ if (retries_left - 1 > 0) {
+ ret = usleep(wait);
+ if (ret == -1) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ ("usleep() failed -> ignoring\n"));
+ }
+ }
+ } else {
+ /* Error occurred */
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ ("Unable to lock mmap cache file.\n"));
+ return ret;
+ }
+ } else if (ret == 0) {
+ /* File successfuly locked */
+ break;
+ }
+ }
+ if (retries_left == 0) {
+ DEBUG(SSSDBG_CRIT_FAILURE, ("Unable to lock mmap cache file.\n"));
+ return ret;
+ }
+
+ return EOK;
+}
+
--
1.7.11.2
>From dc99f5d7c3e1fb611881498255707070c0776d90 Mon Sep 17 00:00:00 2001
From: Michal Zidek <[email protected]>
Date: Fri, 26 Oct 2012 17:36:51 +0200
Subject: [PATCH 2/2] sss_cache: Remove fastcache even if sssd is not running.
https://fedorahosted.org/sssd/ticket/1584
---
src/responder/nss/nsssrv_mmap_cache.c | 26 ++++++++--
src/tools/sss_cache.c | 78 ++++++++++++++++++++--------
src/tools/tools_util.c | 95 +++++++++++++++++++++++++++++++++++
src/tools/tools_util.h | 2 +
4 files changed, 178 insertions(+), 23 deletions(-)
diff --git a/src/responder/nss/nsssrv_mmap_cache.c b/src/responder/nss/nsssrv_mmap_cache.c
index f402564..524aa47 100644
--- a/src/responder/nss/nsssrv_mmap_cache.c
+++ b/src/responder/nss/nsssrv_mmap_cache.c
@@ -537,14 +537,21 @@ static errno_t sss_mc_create_file(struct sss_mc_ctx *mc_ctx)
mode_t old_mask;
int ofd;
int ret;
+ useconds_t t = 50000;
+ int retries = 3;
ofd = open(mc_ctx->file, O_RDWR);
if (ofd != -1) {
+ ret = sss_br_lock_file(ofd, 0, 1, retries, t);
+ 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 +575,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 = sss_br_lock_file(mc_ctx->fd, 0, 1, retries, t);
+ 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..4827501 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,97 @@ 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;
+ errno_t ret;
+ errno_t pret;
+ useconds_t t = 50000;
+ int retries = 2;
+
+ 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;
+ }
+ }
+
+ ret = sss_br_lock_file(mc_fd, 0, 1, retries, t);
+ if (ret == EACCES) {
+ DEBUG(SSSDBG_TRACE_FUNC,
+ ("File %s already locked by someone else.\n", mc_filename));
+ goto done;
+ } else if (ret != EOK) {
+ DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to lock file %s.\n", mc_filename));
+ 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);
+
+ /* Only unlink the file if invalidation was succesful */
+ if (ret == EOK) {
+ pret = unlink(mc_filename);
+ if (pret == -1) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ ("Failed to unlink file %s. "
+ "Will be unlinked later by sssd_nss.\n"));
+ }
+ }
+ }
+ 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
