On Thu, Aug 22, 2013 at 10:45:40AM +0200, Pavel Březina wrote:
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.
Pushed to master.