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

Richard W.M. Jones rjones at redhat.com
Fri Aug 28 13:15:34 UTC 2015


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




More information about the Libguestfs mailing list