On 08/22/2013 01:20 AM, Michal Židek wrote:
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
Ack.
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
