11:21 a.m.
On Tue, Feb 09, 2016 at 07:02:06PM +0000, Richard W.M. Jones wrote:
On Tue, Feb 09, 2016 at 05:53:57PM +0300, Roman Kagan wrote:
> diff --git a/v2v/windows_virtio.ml b/v2v/windows_virtio.ml
> index bdce038..8e0430c 100644
> --- a/v2v/windows_virtio.ml
> +++ b/v2v/windows_virtio.ml
> @@ -33,57 +33,105 @@ let virtio_win =
> with Not_found ->
> Guestfs_config.datadir // "virtio-win"
>
> -let rec install_drivers g inspect systemroot root current_cs =
> +let rec install_drivers g inspect systemroot root current_cs rcaps =
> (* Copy the virtio drivers to the guest. *)
> let driverdir = sprintf "%s/Drivers/VirtIO" systemroot in
> g#mkdir_p driverdir;
>
> if not (copy_drivers g inspect driverdir) then (
> + let block_type = match rcaps.rcaps_block_bus with
> + | None -> IDE
> + | Some block_type -> block_type in
> + let net_type = match rcaps.rcaps_net_bus with
> + | None -> RTL8139
> + | Some net_type -> net_type in
> + let video = match rcaps.rcaps_video with
> + | None -> Cirrus
> + | Some video -> video in
> +
> + if block_type = Virtio_blk || net_type = Virtio_net || video = QXL then
> + error (f_"there are no virtio drivers available for this version of
Windows (%d.%d %s %s). virt-v2v looks for drivers in %s")
> + inspect.i_major_version inspect.i_minor_version inspect.i_arch
> + inspect.i_product_variant virtio_win;
This is a bit obscure, and type unsafe.
I think it's better to cover all the cases by a big match statement.
The code would look something like this:
if not (copy_drivers g inspect driverdir) then (
match rcaps with
| { rcaps_block_bus = Some Virtio_blk }
| { rcaps_net_bus = Some Virtio_net }
| { rcaps_video = Some QXL } ->
(* User requested virtio, but we have no virtio-win drivers. *)
error [...]
| { rcaps_block_bus = (Some IDE | None);
rcaps_net_bus = ((Some E1000 | Some RTL8139 | None) as net_type);
rcaps_video = (Some Cirrus | None) } ->
warning [...];
let net_type =
match net_type with
| Some model -> model
| None -> RTL8139 in
(IDE, model, Cirrus)
)
else (
...
> else (
> (* Can we install the block driver? *)
> let block : guestcaps_block_type =
> - let source = driverdir // "viostor.sys" in
> - if g#exists source then (
> + let has_viostor = g#exists (driverdir // "viostor.inf") in
> + match rcaps.rcaps_block_bus with
> + | None ->
> + if not has_viostor then (
> + warning (f_"there is no viostor (virtio block device) driver for
this version of Windows (%d.%d %s). virt-v2v looks for this driver in %s\n\nThe guest
will be configured to use a slower emulated device.")
> + inspect.i_major_version inspect.i_minor_version
> + inspect.i_arch virtio_win;
> + IDE
> + )
> + else
> + Virtio_blk
> + | Some blk_type ->
> + if blk_type = Virtio_blk && not has_viostor then
> + error (f_"there is no viostor (virtio block device) driver for
this version of Windows (%d.%d %s). virt-v2v looks for this driver in %s\n\nThe guest
will be configured to use a slower emulated device.")
> + inspect.i_major_version inspect.i_minor_version
> + inspect.i_arch virtio_win;
> + blk_type
> + in
> +
> + (* Block driver needs tweaks to allow booting; the rest is set up by PnP
> + * manager *)
> + if block = Virtio_blk then (
> + let source = driverdir // "viostor.sys" in
> let target = sprintf "%s/system32/drivers/viostor.sys" systemroot
in
> let target = g#case_sensitive_path target in
> g#cp source target;
> - add_viostor_to_registry g inspect root current_cs;
> - Virtio_blk
> - ) else (
> - warning (f_"there is no viostor (virtio block device) driver for this
version of Windows (%d.%d %s). virt-v2v looks for this driver in %s\n\nThe guest will be
configured to use a slower emulated device.")
> - inspect.i_major_version inspect.i_minor_version
> - inspect.i_arch virtio_win;
> - IDE
> - ) in
> + add_viostor_to_registry g inspect root current_cs
> + );
Again, I found this code (and the following code for net/video) to be
very confusing. Any time you've got multiple levels of match + if,
you're probably doing it wrong, and it's better to do the whole lot
with pattern matching (especially since the compiler helps you to find
missing cases this way). For example:
match rcaps.rcaps_block_bus, has_viostor with
| { None, false }
(* Warn the user things could be better with virtio-win, but continue
* with IDE.
*)
warning [...];
IDE
| { Some IDE, false } ->
(* User requested IDE, so no warning is required. *)
IDE
| { Some Virtio_blk, false } ->
(* Error: request cannot be satisfied *)
error [...];
| { None, true } ->
(* Good news, we can configure virtio-win. *)
etc etc.
I'm going to have to think about this some more.
OK while you're still at it, let me add a bit to this discussion, as
I'm not happy about this patchset, too, because it doesn't fit well with
the overall existing v2v structure.
I think v2v in general falls into the following steps:
1) collecting information about the source VM (config, r/o inspection)
2) deciding on the target VM configuration, depending on the data from
step (1) and on the availability of the necessary stuff like virtio
drivers, etc.
3) creating the target VM with overlays over original disks
4) tuning up the guest to work in the new virtual hardware (installing
drivers, fixing up boot, etc.)
5) finalizing the VM's disks
Note that
- step (1) may be done by another tool which has better knowledge of the
source hypervisor (especially in the case of proprietary hypervisors)
- step (2) may be done with user interaction, e.g. in a UI. Besides,
the user may have reasons not to switch to virtio even when it's
supported, or only do so for some of the VM's disks, or prefer
virtio-scsi over virtio-blk, etc.
- step (4) is not supposed to take any decisions, but follow precisely
the VM configuration and fail if it can't. This makes up for a
sensible self-contained tool; one of its use cases apart from v2v
would be adjusting the guest to virtual hardware changes done by the
user on an existing VM, without migrating to another hypervisor (e.g.
switching from IDE to virtio-blk or from virtio-blk to virtio-scsi).
[This was approximately what I was trying to achieve with --in-place
but apparently failed so far.]
- step (5) may take different forms, e.g. the user may want to start
using the VM immediately and merge the data from the original images
in the background, or just leave them as backing images for good, or
do an explicit copy to the final images as is currently done
Does this make sense? If so, what would be the best way to proceed?
With my Virtuozzo hat on I'm interested only in step (4) because other
steps are easier to do in our own toolset; however I'm willing to do the
legwork to keep the things consistent.
Thanks,
Roman.