2011/7/19 Daniel Veillard <veill...@redhat.com>:
> On Tue, Jul 19, 2011 at 11:00:21AM +0200, Matthias Bolte wrote:
>> 2011/7/19 <a...@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
