On (21/08/13 14:01), 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?
> >
> >>+ 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.
sensible
argument.
In this case, I agree too.
LS