[Libguestfs] [PATCH] v2v: add --in-place mode

[Roman Kagan]

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.

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.

> > +  let overlays = (
> > +    if not in_place then (
[...]
> > +    ) else []
> 
> The overlays array shouldn't be an empty list in the case where we are
> doing an in_place conversion.  It should be None (and the other case
> should be Some overlays).
> 
> However this makes me wonder if there is a better way to structure
> this code.  As it stands, when using --in-place, the output object is
> set (pretty much randomly) to Output_libvirt.output_libvirt.  So a
> bunch of methods on Output_libvirt.output_libvirt are called which
> don't make sense.

Right.  Actually when developing the patch I hesitated between better
structure and less changes, and chose the latter as I wasn't sure the
feature would be welcome at all.  That's why I used empty lists here
(and all over the place): as a result I didn't need multiple extra
conditionals, because the code which wasn't supposed to execute in
in_place case didn't as it was passed an empty list to work on.

That said, if we're up to restructuring (which is IMO desirable here
anyway since the main function is now about 400 lines) I'll try to
make it look more natural.

> In virt-sparsify, --in-place is handled essentially by a completely
> different main function in a different module, and that might be a
> less error-prone way to do things here too.

Thanks for the suggestion, I'll have a look.

> Change the if statements around so that
> 
>   if not in_place then
>     (* old code *)
>   else
>     (* new code *)
> 
> I envisage that the not in_place route will remain the usual way to
> use virt-v2v.

OK

> > @@ -518,6 +523,18 @@ C<root>.
> >  You will get an error if virt-v2v is unable to mount/write to the
> >  Export Storage Domain.
> >  
> > +=item B<--in-place>
> 
> This appears in the wrong place in the list of options (sorted by 'p'
> rather than 'i').  I don't know if this was intentional, but I think
> it's wrong.

I didn't realize the options went in a strict sorting order.  Besides,
at first I thought I'd make it a new -o suboption; then I changed my
mind but didn't bother to adjust the position in the list.

Thanks,
Roman.