On Wed, Apr 13, 2016 at 10:11:54AM +0100, Richard W.M. Jones wrote: > 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.
Thanks a lot for the review, I'll fix the comments and respin. Roman. _______________________________________________ Libguestfs mailing list [email protected] https://www.redhat.com/mailman/listinfo/libguestfs
