On 03/07/2013 03:22 PM, Simo Sorce wrote:
On Thu, 2013-03-07 at 15:02 +0100, Michal Židek wrote:
> + /* Closing the file descriptor and ummaping the file
> + * from memory is done in the mc_ctx_destructor. */
> if (mc_ctx && mc_ctx->fd != -1) {
> - close(mc_ctx->fd);
> - ret = unlink(mc_ctx->file);
> - if (ret == -1) {
> - DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to rm mmap file %
> s: %d(%s)\n",
> - mc_ctx->file, ret,
> strerror(ret)));
> + dret = unlink(mc_ctx->file);
> + if (dret == -1) {
> + DEBUG(SSSDBG_CRIT_FAILURE,
> + ("Failed to rm mmap file %s: %d(%s)\n",
> mc_ctx->file,
> + dret, strerror(dret)));
> }
sorry I just also realized this is probably incorrect, you should test
mc_ctx->file not mc_ctx->fd
Not really. The flow is to allocate mc_ctx->file and after that call
open... so if fd is not -1 then the mc_ctx->file must be already
allocated (not null). Testing for mc_ctx->file would be incorrect,
because we might successfully create mc_ctx->file, but then fail during
open()... so checking for fd is correct here.
I also see we have a minor issue in sss_mc_create_file(), we should
probably remove close()/unlink() as well from the error condition in
case sss_br_lock_file() fails, or at the very least set fd = -1 and file
= NULL when close() and unlink() are called respectively.
Yes. But simply setting the fd = -1 should be enough (for the same
reason as described above).
I'll upload new patch soon.
Simo.