Re: [libvirt] [PATCH] util: avoid fds leak on virEventPollInit

2011/7/19 <ajia(a)redhat.com&gt;: > * 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; }