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. -- Simo Sorce * Red Hat, Inc * New York _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel