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?
>+ 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.
>+
>+ 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