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