Hi, On Sat, Mar 3, 2012 at 9:47 AM, DR <[email protected]> wrote: > On Sat, Mar 3, 2012 at 8:29 PM, DR <[email protected]> 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,
