Reply-To-All :) ---------- Forwarded message ---------- From: Iskren Chernev <[email protected]> Date: 2011/3/13 Subject: Re: minor fixes in event-loop.c To: Kristian Høgsberg <[email protected]>
I fixed the "close fd" ones, but its not perfect: -- in what order should we do the epoll_ctl, close and free? -- what should we do if epoll_ctl is first and fails, or if close is first and fails (call the next one or return)? -- should we free if epoll_ctl of close fail? -- should we loop on close while it returns EINTR - the man page doesn't say weather SA_RESTART works for close. I think that close is unlikely to fail, because it is a special system fd. I'm not sure how often does epoll_ctl fail and what can be done about it -- there is no error returned from epoll_ctl that we might get unless there is something very wrong. It should be good enough for now :) Regards, Iskren PS.: I'll make a github account in the near future. So should I also send the patches with send-mail or only somehow send links to the revision in github? 2011/3/13 Kristian Høgsberg <[email protected]> > On Sun, Mar 13, 2011 at 11:24 AM, Iskren Chernev > <[email protected]> wrote: > > Some patches for event-loop.c -- the signal & timer fds were never > closed. > > In the future do you prefer patches standalone or archived? > > Looks like more good patches. Patches 1 and 4 close the fd before > passing it to epoll_ctl(), and I don't think that's how it's supposed > to work. It may or may not work depending on the implementation and > the man page doesn't state one way or the other. However, the obvious > safe approach is to just call epoll_ctl() before closing the fd. > > I prefer patches sent by git send-email, since they're easy to review > and discuss on the list, but for more than a couple of patches I also > would like a branch somewhere (github, gitorious etc) that I can just > pull if it all looks ok. > > Kristian >
From 42f651ad56e3b961468dfdcda40c755972540011 Mon Sep 17 00:00:00 2001 From: Iskren Chernev <[email protected]> Date: Sun, 13 Mar 2011 21:05:14 +0200 Subject: [PATCH 1/4] Close timer file descriptors in event loop on remove and failure. When the timer is removed we always close the fd and free the memory regardless of the fact that epoll_ctl may fail. I don't think we can do much better. --- wayland/event-loop.c | 9 +++++---- 1 files changed, 5 insertions(+), 4 deletions(-) diff --git a/wayland/event-loop.c b/wayland/event-loop.c index bf2a9aa..b3f2d17 100644 --- a/wayland/event-loop.c +++ b/wayland/event-loop.c @@ -172,12 +172,12 @@ wl_event_source_timer_remove(struct wl_event_source *source) struct wl_event_source_timer *timer_source = (struct wl_event_source_timer *) source; struct wl_event_loop *loop = source->loop; - int fd; + int result; - fd = timer_source->fd; + result = epoll_ctl(loop->epoll_fd, EPOLL_CTL_DEL, timer_source->fd, NULL); + close(timer_source->fd); free(source); - - return epoll_ctl(loop->epoll_fd, EPOLL_CTL_DEL, fd, NULL); + return result; } struct wl_event_source_interface timer_source_interface = { @@ -215,6 +215,7 @@ wl_event_loop_add_timer(struct wl_event_loop *loop, ep.data.ptr = source; if (epoll_ctl(loop->epoll_fd, EPOLL_CTL_ADD, source->fd, &ep) < 0) { + close(source->fd); free(source); return NULL; } -- 1.7.4
From 5aaae6423840cc045eed488fecd987b4287299de Mon Sep 17 00:00:00 2001 From: Iskren Chernev <[email protected]> Date: Sun, 13 Mar 2011 21:08:37 +0200 Subject: [PATCH 4/4] Close signal file descriptor in event loop on remove and failure. When the signal handler is removed we always close the fd and free the memory regardless of the fact that epoll_ctl may fail. I don't think we can do much better. --- wayland/event-loop.c | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-) diff --git a/wayland/event-loop.c b/wayland/event-loop.c index 9dd5ca9..9032f56 100644 --- a/wayland/event-loop.c +++ b/wayland/event-loop.c @@ -269,12 +269,13 @@ wl_event_source_signal_remove(struct wl_event_source *source) struct wl_event_source_signal *signal_source = (struct wl_event_source_signal *) source; struct wl_event_loop *loop = source->loop; - int fd; + int result; - fd = signal_source->fd; + result = epoll_ctl(loop->epoll_fd, EPOLL_CTL_DEL, signal_source->fd, NULL); + close(signal_source->fd); free(source); - return epoll_ctl(loop->epoll_fd, EPOLL_CTL_DEL, fd, NULL); + return result; } struct wl_event_source_interface signal_source_interface = { @@ -318,6 +319,7 @@ wl_event_loop_add_signal(struct wl_event_loop *loop, ep.data.ptr = source; if (epoll_ctl(loop->epoll_fd, EPOLL_CTL_ADD, source->fd, &ep) < 0) { + close(source->fd); free(source); return NULL; } -- 1.7.4
_______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
