On Fri, Aug 28, 2015 at 03:44:07PM +0300, Roman Kagan wrote: > 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?
If you can be bothered to do (4), then adding the at_exit hook in the command line parsing of all virt tools seems like the way to go here. If you can't be bothered with all that, just drop it. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html _______________________________________________ Libguestfs mailing list [email protected] https://www.redhat.com/mailman/listinfo/libguestfs
