On 07/19/2011 05:00 PM, Matthias Bolte wrote:
* src/util/event_poll.c: fix file descriptors leak on virEventPollInit.
Detected in valgrind run: ==1254== ==1254== FILE DESCRIPTORS: 6 open at exit. ==1254== Open file descriptor 5: ==1254== at 0x30736D9D47: pipe2 (syscall-template.S:82) ==1254== by 0x4DD6267: rpl_pipe2 (pipe2.c:61) ==1254== by 0x4C4C1C5: virEventPollInit (event_poll.c:648) ==1254== by 0x4C4AA94: virEventRegisterDefaultImpl (event.c:208) ==1254== by 0x42150C: main (virsh.c:13790) ==1254== ==1254== Open file descriptor 4: ==1254== at 0x30736D9D47: pipe2 (syscall-template.S:82) ==1254== by 0x4DD6267: rpl_pipe2 (pipe2.c:61) ==1254== by 0x4C4C1C5: virEventPollInit (event_poll.c:648) ==1254== by 0x4C4AA94: virEventRegisterDefaultImpl (event.c:208) ==1254== by 0x42150C: main (virsh.c:13790) ==1254==
* how to reproduce? % valgrind -v --track-fds=yes virsh list
--- src/util/event_poll.c | 13 ++++++++++--- 1 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/src/util/event_poll.c b/src/util/event_poll.c index 285ba50..1e4ef96 100644 --- a/src/util/event_poll.c +++ b/src/util/event_poll.c @@ -639,6 +639,8 @@ static void virEventPollHandleWakeup(int watch ATTRIBUTE_UNUSED,
int virEventPollInit(void) { + int ret = -1; + if (virMutexInit(&eventLoop.lock)< 0) { virReportSystemError(errno, "%s", _("Unable to initialize mutex")); @@ -648,7 +650,7 @@ int virEventPollInit(void) if (pipe2(eventLoop.wakeupfd, O_CLOEXEC | O_NONBLOCK)< 0) { virReportSystemError(errno, "%s", _("Unable to setup wakeup pipe")); - return -1; + goto cleanup; }
if (virEventPollAddHandle(eventLoop.wakeupfd[0], @@ -657,10 +659,15 @@ int virEventPollInit(void) virEventError(VIR_ERR_INTERNAL_ERROR, _("Unable to add handle %d to event loop"), eventLoop.wakeupfd[0]); - return -1; + goto cleanup; }
- return 0; + ret = 0; + +cleanup: + close(eventLoop.wakeupfd[0]); + close(eventLoop.wakeupfd[1]); + return ret; } NACK, as this is wrong. You're closing the pipe in all cases in virEventPollInit, but the pipe is supposed to be used afterwards. As
2011/7/19<ajia@redhat.com>: there is no virEventPollDeinit this FD leak has to be considered as a static leak that's expected and not to be fixed. Also close() is forbidden and make syntax-check should have told you that.
What can be done here is closing the pipe in case virEventPollAddHandle fails, for example like this:
diff --git a/src/util/event_poll.c b/src/util/event_poll.c index 285ba50..a62f960 100644 --- a/src/util/event_poll.c +++ b/src/util/event_poll.c @@ -657,6 +657,8 @@ int virEventPollInit(void) virEventError(VIR_ERR_INTERNAL_ERROR, _("Unable to add handle %d to event loop"), eventLoop.wakeupfd[0]); + VIR_CLOSE(eventLoop.wakeupfd[0]); + VIR_CLOSE(eventLoop.wakeupfd[1]); return -1; }
Yeah, only fix the above section you mentioned instead. Thanks, Alex