On Tue, Jul 28, 2015 at 02:51:39PM +0300, Roman Kagan wrote:
First off, thanks for the review. IIUC you are not opposed to the idea of adding the in-place operation mode in general, are you? I'll work through your comments to make the patch fit better with the rest of the code.
I'd definitely want to see the final patch, but I'm not opposed in principle.
On Tue, Jul 28, 2015 at 11:54:59AM +0100, Richard W.M. Jones wrote:
On Mon, Jul 27, 2015 at 07:29:57PM +0300, Roman Kagan wrote:
--- a/tests/guests/guests.xml.in +++ b/tests/guests/guests.xml.in
Let's not add this to tests/guests, since this disk image is only used for a single test in the v2v/ subdirectory.
You can change the new test in the v2v/ subdirectory so it creates (or is supplied with) the XML. There are various tests which work like that already - eg: v2v/test-v2v-cdrom.*
OK, makes sense, thanks for the suggestion.
While at this: do you think adding the test for the newly added option should go in the same commit with the option itself? It may be easier to review separately, but YMMV, so please let me know which way you'd like it better.
Either way you like. Sometimes we add tests as separate commits, and sometimes in the same commit. The only hard rule for commits is: don't break bisection. 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