On 08/21/2013 04:47 PM, Michal Židek wrote:
On 08/21/2013 02:01 PM, Jakub Hrozek wrote:
On Wed, Aug 21, 2013 at 10:27:38AM +0200, Lukas Slebodnik wrote:
On (21/08/13 09:50), Pavel Březina wrote:
On 08/19/2013 09:56 PM, Michal Židek wrote:
+static void sss_mc_save_corrupted(struct sss_mc_ctx *mc_ctx)
+{
+ int err;
+ int fd = -1;
+ ssize_t written;
+ char *file = NULL;
+ TALLOC_CTX *tmp_ctx;
+
+ if (mc_ctx == NULL) {
+ DEBUG(SSSDBG_TRACE_FUNC,
+ ("Cannot store uninitialized cache. Nothing to
do.\n"));
+ return;
+ }
+
+ tmp_ctx = talloc_new(NULL);
+ if (tmp_ctx == NULL) {
+ DEBUG(SSSDBG_CRIT_FAILURE, ("Out of memory.\n"));
+ return;
+ }
+
+ file = talloc_asprintf(tmp_ctx, "%s_%s",
+ mc_ctx->file, "corrupted");
+ if (file == NULL) {
+ DEBUG(SSSDBG_CRIT_FAILURE, ("Out of memory.\n"));
+ goto done;
+ }
+
+ /* We will always store only the last problematic cache state */
+ fd = creat(file, 0600);
+ if (fd == -1) {
+ err = errno;
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ ("Failed to open file '%s' [%d]: %s\n",
+ file, err, strerror(err)));
+ goto done;
+ }
+
+ written = write(fd, mc_ctx->mmap_base, mc_ctx->mmap_size);
Shouldn't sss_atomic_write_s() be used?
sss_atomic_write_s is probably better. I though that the function
is only useful for non-blocking sockets, but after reading it's code I
see it is good to use it here too.
+ if (written != mc_ctx->mmap_size) {
+ if (written == -1) {
+ err = errno;
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ ("write() failed [%d]: %s\n", err, strerror(err)));
+ } else {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ ("write() returned %zd (expected (%zd))\n",
+ written, mc_ctx->mmap_size));
+ }
+ goto done;
+ }
I think the file should be unlinked in case of error.
Ok. I added unlink() call for the case if write returns -1. If the
file was only partially written (for whatever reason) it can still
hold some useful information, so I would prefer to keep it.
+
+ sss_log(SSS_LOG_NOTICE,
+ "Stored copy of corrupted mmap cache in file '%s\n'",
file);
+done:
+ if (fd != -1) {
+ close(fd);
+ }
+ talloc_free(tmp_ctx);
+}
I think that function sss_mc_save_corrupted will never be called with
patches
from thread "[PATCHES] mmap_cache: Skip records which doesn't have
same hash"
It is very likely that it is a dead code. I would prefer to ignore this
nitpicks, because patches have already been pushed.
LS
No, sorry, I would prefer to always fix and improve code. At the very
least, existing code acts as a reference when developing new code.
Since the original patch was already pushed, this one just adds the
requested changes.
Thanks
Michal
Since this was not yet pushed I did one more cosmetic change. I think
calling the close() before unlink() is more readable. If the fd != -1 we
already know that we want to close it, so the conditional check for
unlink() just makes noise before it. And this order is probably more
natural for most cases.
Michal
>From cae96ca65899af8b75e952ca5c66b9c08d18026f Mon Sep 17 00:00:00 2001
From: Michal Zidek <[email protected]>
Date: Wed, 21 Aug 2013 15:26:36 +0200
Subject: [PATCH] mmap_cache: Use sss_atomic_write_s instead of write.
Use sss_atomic_write_s() instead of write() in
sss_mc_save_corrupted(). Also unlink() the file if no data
were written.
It is better to use sss_atomic_write_s instead of write
---
src/responder/nss/nsssrv_mmap_cache.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/src/responder/nss/nsssrv_mmap_cache.c b/src/responder/nss/nsssrv_mmap_cache.c
index fced018..1f584a2 100644
--- a/src/responder/nss/nsssrv_mmap_cache.c
+++ b/src/responder/nss/nsssrv_mmap_cache.c
@@ -99,7 +99,7 @@ static void sss_mc_save_corrupted(struct sss_mc_ctx *mc_ctx)
{
int err;
int fd = -1;
- ssize_t written;
+ ssize_t written = -1;
char *file = NULL;
TALLOC_CTX *tmp_ctx;
@@ -132,7 +132,7 @@ static void sss_mc_save_corrupted(struct sss_mc_ctx *mc_ctx)
goto done;
}
- written = write(fd, mc_ctx->mmap_base, mc_ctx->mmap_size);
+ written = sss_atomic_write_s(fd, mc_ctx->mmap_base, mc_ctx->mmap_size);
if (written != mc_ctx->mmap_size) {
if (written == -1) {
err = errno;
@@ -151,6 +151,15 @@ static void sss_mc_save_corrupted(struct sss_mc_ctx *mc_ctx)
done:
if (fd != -1) {
close(fd);
+ if (written == -1) {
+ err = unlink(file);
+ if (err != 0) {
+ err = errno;
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ ("Failed to remove file '%s': %s.\n", file,
+ strerror(err)));
+ }
+ }
}
talloc_free(tmp_ctx);
}
--
1.7.11.2
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
