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

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

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?

Thanks,
Roman.

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