[libvirt] [PATCH] util: avoid fds leak on virEventPollInit
* 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; } static int virEventPollInterruptLocked(void) -- 1.7.1
于 2011年07月19日 16:25, Alex Jia 写道:
* 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]);
s/close/VIR_FORCE_CLOSE/
+ return ret; }
static int virEventPollInterruptLocked(void)
于 2011年07月19日 16:25, Alex Jia 写道:
* 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]);
s/close/VIR_FORCE_CLOSE/ If do this, I need also include 'files.h', VIR_FORCE_CLOSE is defined in
On 07/19/2011 05:00 PM, Osier Yang wrote: this header. Thanks, Alex
+ return ret; }
static int virEventPollInterruptLocked(void)
2011/7/19 <ajia@redhat.com>:
* 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 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; } -- Matthias Bolte http://photron.blogspot.com
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
On 07/19/2011 05:00 PM, Matthias Bolte wrote:
2011/7/19<ajia@redhat.com>:
* 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== Hi Matthias, In fact, if everything is okay, we will always open the above 2 fds, I
On 07/19/2011 05:12 PM, Alex Jia wrote: think it should be a expected result based on your comments, right? Thanks, Alex
* 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 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
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Jul 19, 2011 at 11:00:21AM +0200, Matthias Bolte wrote:
2011/7/19 <ajia@redhat.com>:
* 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 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; }
The only problem I have is why is a simple "virsh list" exhibiting one of those 2 errors, they sounds like hard cornercases, and not something to be triggered in such a simple operation. Valgrind had to emulate one of those code path to raise the error, and that sounds weird. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
2011/7/19 Daniel Veillard <veillard@redhat.com>:
On Tue, Jul 19, 2011 at 11:00:21AM +0200, Matthias Bolte wrote:
2011/7/19 <ajia@redhat.com>:
* 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 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; }
The only problem I have is why is a simple "virsh list" exhibiting one of those 2 errors, they sounds like hard cornercases, and not something to be triggered in such a simple operation. Valgrind had to emulate one of those code path to raise the error, and that sounds weird.
Daniel
I'm not sure that I understand what you mean. The FD leak that valgrind detected is there and expected as virsh (indirectly) calls virEventPollInit, but there is not counterpart to virEventPollInit to close those FDs again. This is an expected static leak, you're supposed to call virEventPollInit at most once in your program. So there is actually nothing to fix here. The patch I suggested is unrelated to the valfrind detected leak. In case virEventPollAddHandle fails we know that virEventPollInit will fail and that the calling app should fail too. In that case we can close the FDs. This is just an 'improvement' that's not strictly necessary. It just avoids the static leak in an error case. -- Matthias Bolte http://photron.blogspot.com
On Tue, Jul 19, 2011 at 12:19:34PM +0200, Matthias Bolte wrote:
2011/7/19 Daniel Veillard <veillard@redhat.com>:
On Tue, Jul 19, 2011 at 11:00:21AM +0200, Matthias Bolte wrote:
2011/7/19 <ajia@redhat.com>:
* 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 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; }
The only problem I have is why is a simple "virsh list" exhibiting one of those 2 errors, they sounds like hard cornercases, and not something to be triggered in such a simple operation. Valgrind had to emulate one of those code path to raise the error, and that sounds weird.
Daniel
I'm not sure that I understand what you mean. The FD leak that valgrind detected is there and expected as virsh (indirectly) calls virEventPollInit, but there is not counterpart to virEventPollInit to close those FDs again. This is an expected static leak, you're supposed to call virEventPollInit at most once in your program. So there is actually nothing to fix here.
Yes, just that the patch was about the error paths, if we didn't go through the error paths then fine, and yes a static leak is fine, libxml2 has one too
The patch I suggested is unrelated to the valfrind detected leak. In case virEventPollAddHandle fails we know that virEventPollInit will fail and that the calling app should fail too. In that case we can close the FDs. This is just an 'improvement' that's not strictly necessary. It just avoids the static leak in an error case.
yeah, the program is likely to exit anyway, but cleaning this up makes sense, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (5)
-
ajia＠redhat.com -
Alex Jia
-
Daniel Veillard -
Matthias Bolte -
Osier Yang