[Libguestfs] [PATCH] v2v: add support for virtio-scsi

[Richard W.M. Jones]

On Tue, Apr 12, 2016 at 06:46:31PM +0300, Roman Kagan wrote:
>            (match guestcaps.gcaps_block_bus with
> -          | Virtio_blk -> "VirtIO" | IDE -> "IDE");
> +          | Virtio_blk -> "VirtIO"
> +          | Virtio_SCSI -> "SCSI"

I believe this should be "VirtIO_SCSI", although it's difficult to get
a straight answer from the massive and convoluted Java sources of
ovirt-engine ...

> @@ -1148,14 +1149,15 @@ let rec convert ~keep_serial_console (g : G.guestfs) inspect source rcaps =
>          List.iter (fun path -> ignore (g#aug_rm path))
>            (List.rev paths_to_delete);
>  
> -        g#aug_set (path ^ "/modulename") "virtio_blk"
> +        g#aug_set (path ^ "/modulename")
> +                  (string_of_block_type block_type)

Here ...

>        ) else (
>          (* We have to add a scsi_hostadapter. *)
>          let modpath = discover_modpath () in
>          g#aug_set (sprintf "/files%s/alias[last()+1]" modpath)
>            "scsi_hostadapter";
>          g#aug_set (sprintf "/files%s/alias[last()]/modulename" modpath)
> -          "virtio_blk"
> +                  (string_of_block_type block_type)
>        )

... and here don't seem right.

Firstly, string_of_block_type returns "virtio-blk" (not "virtio_blk").
Maybe for this configuration file it doesn't matter.

But, the real problem is that string_of_block_type is essentially an
internal debugging function.

What we should really have is a new function (eg.
"linux_module_of_block_type") which converts the block type to a Linux
module name.

> +  (* Presence of virtio-scsi controller. *)
> +  let has_virtio_scsi =
> +    let obj = Xml.xpath_eval_expression xpathctx
> +                "/domain/devices/controller[@model='virtio-scsi']" in

I guess this short cut is OK.  A true test would involve checking the
<target bus="scsi"> on each disk and matching it back to the
controller.  In other words, a huge pain!  Maybe you can add an "XXX"
comment in the source about this.

> +    (Xml.xpathobj_nr_nodes obj) > 0 in
       ^                         ^
You don't need these parentheses.  In functional languages, function
application always binds tightest of all, eg:

  f x + 1

is the same as:

  (f x) + 1

> diff --git a/v2v/output_glance.ml b/v2v/output_glance.ml
> index 9487f0b..2572de6 100644
> --- a/v2v/output_glance.ml
> +++ b/v2v/output_glance.ml
> @@ -86,6 +86,7 @@ object
>            "hw_disk_bus",
>            (match guestcaps.gcaps_block_bus with
>             | Virtio_blk -> "virtio"
> +           | Virtio_SCSI -> "scsi"
>             | IDE -> "ide");
>            "hw_vif_model",
>            (match guestcaps.gcaps_net_bus with

This is wrong, or at least, incomplete.

Apparently you need:

  hw_disk_bus=scsi     (as above)
  hw_scsi_model=virtio-scsi  (additional property)

See:
http://www.sebastien-han.fr/blog/2015/02/02/openstack-and-ceph-rbd-discard/

>    if not (copy_drivers g inspect driverdir) then (
>      match rcaps with
> -    | { rcaps_block_bus = Some Virtio_blk }
> +    | { rcaps_block_bus = (Some Virtio_blk | Some Virtio_SCSI) }

I don't think you need parens there.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html