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

[Roman Kagan]

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.