[Libguestfs] [PATCH v2 15/17] v2v: add --in-place mode

[Roman Kagan]

On Thu, Aug 27, 2015 at 04:08:43PM +0100, Richard W.M. Jones wrote:
> On Tue, Aug 11, 2015 at 08:00:34PM +0300, Roman Kagan wrote:
> > +  let overlays =
> > +    if not in_place then create_overlays source.s_disks
> > +    else [] in
> > +  let targets =
> > +    if not in_place then init_targets overlays source output output_format
> > +    else [] in
> 
> This doesn't solve the problem I raised before which is that overlays
> and targets should never be empty lists.

Why shouldn't they?

I thought the problem was that it was hard to keep track of whether the
empty lists made sense deep inside the main()'s guts; now that main()
fits in a screen it's easy to see that there are just two scenarios
sharing some steps but not others, and overlays and targets are empty
just when those steps aren't taken.

Moreover the steps that operated on overlays or targets are made
explicitly conditional on !in_place, even though they are tolerant of
empty lists.

So what's the problem with empty lists, please?

> I think really what should be happening here is something like this:
> 
> (1) Create extra modules:
> 
>   common.ml - for common functions shared; patches 1-14 will end up
>    moving a lot of functions into this file
> 
>   copying.ml - for copying conversion (ie. what virt-v2v does now)
> 
>   in_place.ml - for in-place conversion
> 
> (2) v2v.ml calls out to either Copying or In_place -- see how
>   virt-sparsify works.

I actually followed the advice you gave in the previous review and did
look into virt-sparsify.  I must admit I didn't appreciate the amount of
code duplication between the two usage scenarios.  That was exactly what
I wanted to avoid in my submission.

The posted code covers two usage scenarios like this:

  - common steps (command line, initialization)
  - conditional steps (creation of overlays, guestfs handle with either
    overlays or original disks)
  - common steps (inspection, space estimate, conversion)
  - conditional steps (creation of destination VM)

That's basically all to it.  How can this conveniently be split into
independent scenarios without code duplication and the risk of
forgetting to update one of the two when the common steps are being
modified?

Roman.