Re: [i3] [PATCH] Use ev_break() rather than exit() in i3bar.
- From: Fernando Lemos <fernandotcl@xxxxxxxx>
- To: "Discussions/Questions about the i3 window manager" <i3-discuss@xxxxxxxxxxxxx>
- Date: Sat, 3 Mar 2012 17:43:08 -0300
Hi,
On Sat, Mar 3, 2012 at 9:47 AM, DR <drdarkraven@xxxxxxxxx> wrote:
On Sat, Mar 3, 2012 at 8:29 PM, DR <drdarkraven@xxxxxxxxx> wrote:
If i3bar is running in hide mode, when i3wm exit, i3bar will exit
without killing its child process, leaving a couple of processes in
'T' mode (i.e. Stopped).
With this patch, when i3bar needs to quit, it will call ev_break()
rather than exit(), thus gives the code after ev_loop() a chance to
clean things up.
I'm not very sure I've done things right. I might change some exit()s
called before ev_loop() into ev_break(), or leave some exit()s
unconverted. Someone please review this patch.
Sorry, sorry. I made a mistake, I leave out a file in this patch.
New patch attached.
Sounds great, but I think it would be cleaner to avoid exporting so
many symbols. For example, for at least some of those calls to exit(),
can't we just make the function signal to the caller through a return
code that it has failed? The caller would then signal to his parent
and so on, until we get to the event handlers. Those handlers know all
state involved and can cleanly break out of the loop.
In case the last sentence became a bit confusing, consider this (->
indicates a function call):
Event loop -> Event handler -> A -> B -> C
If C called exit, you're now calling ev_break in C. But then you have
to export the symbols of stuff that needs to be cleaned up in the
event handler, in A and in B, so that you can clean up for them. If
you instead propagate the error across the call stack, C would clean
after itself and pass the error code to B, which would do the same and
pass the error code to A, and so on. That way no function has to clean
up after its parent.
That might require some refactoring (e.g. some functions might already
return something) but it's probably worth it.
Another option may be only clean up what needs to be cleaned up.
Memory usually doesn't need to be free'd if the process is going down
anyways. I'd consider this last resort, but we already do that in the
signal handler anyways, so it's not like it would be unprecedented.
You could also call kill_child in an atexit() handler (child_pid would
have to be initialized to 0), but it's ugly too.
I also noticed that you call ev_break, but the signal handler calls
ev_unloop. Is that intentional?
Finally I'd recommend that you split part of the commit message into
the description. In Git, the first line of the commit message is
special and it must be short. How about something like this:
---
Use ev_break() instead of exit().
Calling exit() directly would mean we would never clean up. So now we
break out of the event loop instead. Now we can be sure the child killed
killed when i3bar exits, for example.
---
Regards,
Other related posts: