[Libguestfs] [PATCH v3 11/13] v2v: add --in-place mode

[Richard W.M. Jones]

Can you repost the final 3 patches, rebased on top of current master?

More comments below ...

On Tue, Oct 20, 2015 at 04:08:19PM +0300, Roman Kagan wrote:
> +  let guestfs_kind = (match conversion_mode with
> +                      | Copying (_, _) -> "overlay"
> +                      | In_place -> "source VM"
> +                     ) in
> +
> +  message (f_"Opening the %s") guestfs_kind;

This is a bit cleaner if written as:
  (match conversion_mode with
   | Copying _ -> message (f_"Opening the overlay")
   | In_place -> message (f_"Opening the source VM")
  );

It's obviously shorter, but there's another reason for doing this
which is it allows the translators to see the complete message for
better translation (although in this case it doesn't make a huge
difference).

You'll have to do the same change with the "Closing ..." message,
since guestfs_kind is no longer defined.

> -  let keep_serial_console = output#keep_serial_console in
> +  let keep_serial_console = (match conversion_mode with
> +                             | Copying (_, _) -> output#keep_serial_console
> +                             | In_place -> true
> +                            ) in

I don't think this change is correct.  I think you should call
#keep_serial_console in both cases, since this is controlled by the
guest type, not the conversion mode.

> -  message (f_"Closing the overlay");
> +  message (f_"Closing the %s") guestfs_kind;

See above.

> +and populate_disks (g:G.guestfs) src_disks =

As far as I know, the type annotation here should not be necessary.
But if it is necessary, then put some spaces in, ie:

                 ...  (g : G.guestfs) ...

> +  List.iter (
> +    fun ({s_qemu_uri = qemu_uri; s_format = format}) ->
> +      match format with
> +      | None ->
> +        g#add_drive_opts qemu_uri ~cachemode:"unsafe" ~discard:"besteffort"
> +      | Some fmt ->
> +        g#add_drive_opts qemu_uri ~format:fmt ~cachemode:"unsafe"
> +                          ~discard:"besteffort"

You don't need the match statement here, just use ?format
as a parameter, ie:

    fun ({s_qemu_uri = qemu_uri; s_format = format}) ->
        g#add_drive_opts qemu_uri ?format
                ~cachemode:"unsafe" ~discard:"besteffort"

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/