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

[Richard W.M. Jones]

On Thu, Aug 27, 2015 at 09:39:30PM +0300, Roman Kagan wrote:
> 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?

Because the point of using the language is to use its type safety to
ensure there are no errors, now or in the future.  I reproduced your
new main() function after the patch series at the end of this email so
we have something concrete to look at.

At the moment, it's (sort of) obvious that

  in_place => overlays = targets = []

but we're not using the compiler to enforce that, so it could break in
future.

It can be made type-safe without a huge amount of work.  Something
like this.  First define a new type:

type conversion_mode =
  | Copying of overlay list * target list
  | In_place

Then change the code to look like below (I didn't change all of it,
but it should be fairly obvious from the examples here):

  let conversion_mode =
    if not in_place then
      Copying (
       let overlays = create_overlays source.s_disks in
       let targets = init_targets overlays source output output_format in
       overlays, targets)
    else In_place in

...

  (match conversion_mode with
    Copying (overlays, _) -> populate_overlays g overlays
    In_place ->  populate_disks g source.s_disks
  );

...

  match conversion_mode with
  | In_place ->
      message (f_"Finishing off");
      exit 0
  | Copying (overlays, targets) ->
      (* rest of main function follows here ... *)



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

OK so you've convinced me that a Common code module isn't
necessary and would lead to a lot of duplication.

However - I still don't like the bottom-up reworking of the code, and
changing that means having fewer, simpler patches in this series.  And
I think the in-place/copying stuff can be made type-safe as above.

Rich.


----------------------------------------------------------------------

let main () =
  (* Handle the command line. *)
  let input, output,
    debug_gc, debug_overlays, do_copy, in_place, network_map, no_trim,
    output_alloc, output_format, output_name, print_source, root_choice =
    Cmdline.parse_cmdline () in

  (* Print the version, easier than asking users to tell us. *)
  if verbose () then
    printf "%s: %s %s (%s)\n%!"
      prog Config.package_name Config.package_version Config.host_cpu;

  if debug_gc then
    at_exit (fun () -> Gc.compact());

  let source = open_source input print_source in
  let source = amend_source source output_name network_map in
  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

  let guestfs_kind =
    if not in_place then "overlay" else "source VM" in
  message (f_"Opening the %s") guestfs_kind;
  let g = open_guestfs () in

  if not in_place then populate_overlays g overlays
  else populate_disks g source.s_disks;

  g#launch ();

  (* Inspection - this also mounts up the filesystems. *)
  message (f_"Inspecting the %s") guestfs_kind;
  let inspect = inspect_source g root_choice in

  let mpstats = get_mpstats g in
  check_free_space mpstats;
  if not in_place then
    check_target_free_space mpstats source targets output;

  let keep_serial_console =
    if not in_place then output#keep_serial_console
    else true in
  let guestcaps = do_convert g inspect source keep_serial_console in

  if no_trim <> ["*"] && (do_copy || debug_overlays) then (
    (* Doing fstrim on all the filesystems reduces the transfer size
     * because unused blocks are marked in the overlay and thus do
     * not have to be copied.
     *)
    message (f_"Mapping filesystem data to avoid copying unused and blank areas");
    do_fstrim g no_trim inspect;
  );

  message (f_"Closing the %s") guestfs_kind;
  g#close ();

  if in_place then (
    message (f_"Finishing off");
    exit 0
  );

  let target_firmware = get_target_firmware inspect guestcaps source output in
  let target_buses = target_bus_assignment source targets guestcaps in
  let targets =
    if not do_copy then targets
    else copy_targets targets input output output_alloc in

  (* Create output metadata. *)
  message (f_"Creating output metadata");
  output#create_metadata source targets target_buses guestcaps inspect
                         target_firmware;

  if debug_overlays then preserve_overlays overlays source.s_name;

  message (f_"Finishing off");
  delete_target_on_exit := false  (* Don't delete target on exit. *)
----------------------------------------------------------------------


-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top