Re: [Libguestfs] [PATCH v2 01/17] v2v: debug gc via at_exit hook

On Thu, Aug 27, 2015 at 07:17:20PM +0100, Richard W.M. Jones wrote: > On Thu, Aug 27, 2015 at 08:12:11PM +0300, Roman Kagan wrote: > > On Thu, Aug 27, 2015 at 03:50:11PM +0100, Richard W.M. Jones wrote: > > > On Tue, Aug 11, 2015 at 08:00:20PM +0300, Roman Kagan wrote: > > > > debub_gc (coming from the command line) indicates that gc should be > > > > forced on program exit. Instead of sticking it on every exit path, > > > > register it as an at_exit hook once. > > > > > > Was this change necessary as part of this patch series? > > > > I think so. > > > > The goal of the refactoring part of the series was to reduce the amount > > of detail in main(), and only leave coarse steps, making it easy to see > > the whole scenario. > > > > Originally every exit path had a clause > > > > if debug_gc then > > Gc.compact () > > > > There were two of them, and I was about to add another one. > > OK - I see, although only every non-error exit path. But adding an > atexit handler ought to be safe, since the garbage collector should > never be in an inconsistent state, and also the GC should never itself > call exit. > > > > The --debug-gc option is used across most of the virt-* tools in the > > > internal tests, and those tests shouldn't exit with an error when the > > > option is used. > > > > This patch doesn't affect any other virt-* tool. Also I'm failing to > > see why it would cause tests to fail: it doesn't change the behavior on > > regular exit paths. Needless to say that I run (the relevant subset of) > > the tests and they passed. > > I don't mean your patch would cause tests to fail, I mean the tests > should only use --debug-gc when the virt-* tool is expected to use a > non-failing path. Anyway this is going down a rabbit hole of detail > that doesn't matter for the virt-v2v --in-place patch series. So what is your verdict regarding this patch? Do you want me to 1) drop it at all 2) leave it as a part of this series 3) submit it separately 4) submit it separately together with similar changes to other tools for uniformity Also (unless you want to drop the patch at all) I'm tempted to make one step further and install an at_exit hook right in the command-line parsing function, right where --debug-gc is handled, without passing it on to the caller. Would it be OK to do so?