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

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.