search

xrmx / bootchart

merge of bootchart-collector and pybootchartgui
GNU General Public License v2.0
233 stars 88 forks source link

Make bootchart-collector clean up after itself #22

Closed Xake closed 13 years ago

Xake commented 13 years ago

These commits in short does the following:

  1. change logic so that bootchart-collector unmounts with detach, so that we may unmount directories bootchart-collector still holds active (fixes tmpfs left behind from --dump).
  2. change logic so that --dump terminates the background running collector instead of killing it (removes a scary message, and makes point 3 possible).
  3. adds logic so that bootchart-collector on termination runs the previous mentioned unmount logic (fixes tmpfs left behind by killed process).
xrmx commented 13 years ago

This does not seem to work here on debian in qemu arm, tried with or without e8fe096.

Xake commented 13 years ago

What does not seem to work? The patches does not apply? bootchart-collector fails to terminate? It does not unmount /dev/.bootchart/tmpfs* of /lib/bootchart/tmpfs*? It makes bootchart-collector not work at all?

e8fe096 should for "bootchart-collector --dump" work without the other two, leaving at most one instance of the tmpfs.

I have tested these patches on Gentoo amd64, running both with bootchart in a dracut generated ramdisk, and without. I can try it out on a Fedora installation too, but I want to know what to look for.

xrmx commented 13 years ago

Yeah sorry for the vague report, i've got mixed results in my tests, a couple of time i got bootchart-collector stuck in T and one time it exited nicely. But since i'm running the stuff under qemu it well maybe an emulation bug and not an issue with your code. And on top of that i have the habit to check the result with mount instead of /proc/mounts so i see wrong results.

A couple of things we have to fix for sure in term_handler as pointed out by mmeeks on IRC are that per man 7 signal, "Async-signal-safe functions" chapter we cannot use fprintf and replace exit with _exit. So if you can fix that i will pull your tree.

Thanks a lot.

xrmx commented 13 years ago

Also why not EXIT_SUCCESS ?

Xake commented 13 years ago

That stuck could be from fprintf I guess? I have made a new commit taken the feedback into consideration. Since it looks like the way github works, I am going to close this pull request and send a new one.

Xake commented 13 years ago

Oh, did not see that the commit was added automagically.:) Sorry for the noise.

I took the feedback into consideration and made term_handler do the unmounting itself, removing any traces of fprintf from it. I also changed the logic, so _exit() should reflect with EXIT_SUCCESS or EXIT_FAILURE depeding on the unlinking/unmounting. Why I had EXIT_FAILURE in all cases before I cannot answer, I blame a thinko.

I think we should consider look into the possibility of make the functions mounting/unmounting also update mtab for all distributions not being Fedora15, however since that is not a regression in these commits I can not see that block these merges.

Thank for the feedback.

xrmx commented 13 years ago

Pushed, i had to fix latest patch though.

Xake commented 13 years ago

Thank you for the pull.

And if the fix you refer to was moving fprintf from clean_enviroment to main() then it did not matter. I stopped using clean_environment, since I could not find out the safety of perror(). It was not listed in man 7 signal, and stdio.h seems to be using fprintf for it.

xrmx commented 13 years ago

The fix was if (ret = 0) vs ret == 0 to decide what pass to _exit

Xake commented 13 years ago

Ah, thanks. Did not see that you made the change within the commit.

xrmx commented 13 years ago

git commit --amend ftw :)

Xake commented 13 years ago

Hehe, yeah. Did just learn about that, but have yet to explore what exactly is possible with it.