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