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

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?  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.

Anyway, I'd just drop this patch from the series and if it's needed
post it as another series.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

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.

So I went ahead and replaced them all with two lines at the beginning,
setting up an at_exit hook.
> 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.
> Anyway, I'd just drop this patch from the series and if it's needed
> post it as another series.
I tend to think it's appropriate here as it's approaching the goal of
the refactoring.  Well, unless you think the idea is so good that it
should be done in all tools which accept --debug-gc, in which case a
separate patchset is due of course ;)

Roman.

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.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v