On Sun, Mar 4, 2012 at 4:43 AM, Fernando Lemos <[email protected]> wrote: > 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. If my understanding of ev_break() is right, my patch works quite the same way as you described. Event handler will return, and all the clean up is done in main(), no extra symbols need to be exported.
> > > 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? The ev_unloop is from the old code. I just didn't notice that. > > > 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: Thank you for point that out, I think I'm just too careless about the commit message.... > > --- > 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,
