On Wed, 2013-03-06 at 23:19 +0100, Michal Židek wrote:
> On 03/06/2013 08:13 PM, Simo Sorce wrote:
>> On Wed, 2013-03-06 at 19:33 +0100, Michal Židek wrote:
>>> On 03/06/2013 07:27 PM, Michal Židek wrote:
>>>> On 03/06/2013 07:18 PM, Michal Židek wrote:
>>>>> On 03/06/2013 06:33 PM, Simo Sorce wrote:
>>>>>> On Wed, 2013-03-06 at 17:09 +0100, Michal Židek wrote:
>>>>>>>
https://fedorahosted.org/sssd/ticket/1826
>>>>>>>
>>>>>>> See commit message.
>>>>>>
>>>>>> It would be better if you can use a destructor attached to the
mc_ctx
>>>>>> so any other path where we need to free it is automatically
covered.
>>>>>>
>>>>>> Simo.
>>>>>>
>>>>>
>>>>> New patch attached.
>>>>>
>>>>> Thanks
>>>>> Michal
>>>>>
>>>>>
>>>> There should be goto done, not ret... will send another patch soon.
>>>>
>>>
>>> Here it is.
>>
>> The destructors can return only 0 or -1.
>>
>> And they shoul return -1 exclusively when they want to make the
>> talloc_free() operation fail.
>>
>> In this case we should probably never return anything but 0, because
>> even on failure we want to still free the mc_ctx.
>>
>
> OK.
>
>> Also I think failing to close the file or unmapping should be major
>> failures (much higher debug level) as they cause memory/resource leaks
>> that will quickly make the process unusuable.
>
> I changed it to critical failure.
The destructor looks fine now, however now that you have created it I
would reverse munmap and close of file descriptor operations order and
add unlink(mc_ctx->file); after you close the fd.
I would also add checks that fd is not already -1 and that mmap_base is
not NULL aqnd file is not NULL.
In sss_mmap_cache_init() I would add the destructor much earlier, right
after mc_ctx is allocated and fd is asigned -1. Then remove
munmap/close/unlink operations from the exception handling (done label)
and leave there just the talloc_free().
If you do this both code paths will use the same destructor to free
these resources and we do not risk a change in one path to make the 2
diverge.
Simo.
Ok. I also added the the sss_mc_set_recycled call to the destructor,
because the destruction is called as part of memory cache
reinitialisation, but I left the logic to handle this in the init
(sss_mc_create_file) part too, for the case if mc files were left on
disk after SSSD crashed and destructor failed or was not called when
SSSD was running last time. The unlink() call in the init function is
now called only if the call to open the old mc file succeeded (so the
file really exists).
The second patch adds debug message for the case if opening old
instance of mc file fails. It has not much to do with the destructor but
it applies on top of the first patch so I added it to this thread.
New version of the patch is attached (along with the second patch).
Thanks
Michal