On 11/01/2012 02:45 PM, Simo Sorce wrote:
On Thu, 2012-11-01 at 09:14 -0400, Simo Sorce wrote:
On Thu, 2012-11-01 at 11:19 +0100, Michal Židek wrote:
On 11/01/2012 06:54 AM, Simo Sorce wrote:
On Wed, 2012-10-31 at 12:43 +0100, Michal Židek wrote:
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.
Michal,
the structure now is fine, only one minor nitpick is that it would be
probably better to remove references to mmap cache from sss_br_lock_file
debugging messages as it is not mmap cache specific anymore.
Sure, this was leftover from previous versions.
Anyway I was trying to test your patch on my system but on current
master, with your 3 patches on top I cannot get sss_cache to do anything
as far as I can see.
It always returns: "No cache object matched the specified search"
Looking at the code it seem to me that this always happen (skipped is
always true if you pass only one switch ?) unless all maps are full and
you pass -UGNSA, but I haven't used this tool much so I may just be
missing something.
What's the right way to test this patches ?
This is strange. It works fine for me. I test it for example this way:
1. start sssd
2. fill the cache with getent passwd user1
3. turn off sssd
4. getent passwd user1 again (result is returned from memory cache)
5. sss_cache -u user1 (or sss_cache -U) should remove the cache
6. getent passwd user1 should give no results
I also tested the sss_cache while sssd was running and it gives me
expected results (sended SIGHUP to monitor and next getent did not use
the mem cache).
I tested it on current master, but maybe there really is a bug. If it is
still not working for you, could you send how you test it?
I tired the same above commands, however I didn't make sure to
repopulate the cache after the first try, maybe I was indeed operating
on an empty one, I'll retry with your 2 new patches (btw it would be
nice if you could send them in separate emails in future, so patchwork
picks both them up :).
Ok I have found out what I was doing wrong.
I was trying with a user in a trusted domain. These users are apparently
not yet stored in the memory ccache (need to open a bug on that I
guess).
Once I started trying with an simple ipa users it worked fine.
ACK!
Simo.
Just re-sending these patches. They are the same as previous just
rebased so that they apply on top of the modified 'sss_cache: Multiple
domains not handled properly' patch.
Thanks
Michal
>From e35e886c7641e3b6ac2d42422093af2aad57bfbf Mon Sep 17 00:00:00 2001
From: Michal Zidek <mzi...@redhat.com>
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 07dcbd2..3306bba 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 df57a3d..1b979e1 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -557,6 +557,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..aa0d73c
--- /dev/null
+++ b/src/util/util_lock.c
@@ -0,0 +1,84 @@
+/*
+ SSSD
+
+ util_lock.c
+
+ Authors:
+ Michal Zidek <mzi...@redhat.com>
+
+ 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 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 file.\n"));
+ return ret;
+ }
+ } else if (ret == 0) {
+ /* File successfuly locked */
+ break;
+ }
+ }
+ if (retries_left == 0) {
+ DEBUG(SSSDBG_CRIT_FAILURE, ("Unable to lock file.\n"));
+ return ret;
+ }
+
+ return EOK;
+}
+
--
1.7.11.2
>From 4e924e742bbefba529ad1864691544d79617b5ef Mon Sep 17 00:00:00 2001
From: Michal Zidek <mzi...@redhat.com>
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 84a53dd..467d5c3 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)
@@ -550,3 +586,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
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
