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

[Roman Kagan]

On Wed, Apr 13, 2016 at 06:39:55PM +0100, Richard W.M. Jones wrote:
> On Wed, Apr 13, 2016 at 07:45:42PM +0300, Roman Kagan wrote:
> > 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:
> > > > +  (* 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.
> > 
> > On a second look, I'm not sure I got your comment right.
> > 
> > AFAICS there's no way in libvirt xml to define multiple scsi buses,
> 
> I think this is possible, although massively unlikely in practice.  I
> think the different controllers would have different
> index="0"/index="1"/... attributes.

How the binding of drives to controllers is done, then?  Is it specified
in drive's "address"?

And indeed we probably don't have to worry about this case.

> > so all the matching I can do is just to notice the presence of
> > virtio-scsi controller, and attribute all SCSI drives to it.  This
> > is exactly what happens in the patch.
> >
> > Am I missing something?
> 
> So first off I think this part of the patch and your assumption is
> fine, there's no need to change the code.
> 
> However there is a possibility that the source guest might have
> multiple mixed virtio-blk + virtio-scsi disks.  In that case the disks
> would look like this:
> 
>     <disk type='file' device='disk'>
>       <driver name='qemu' type='raw'/>
>       <source file=...>
>       <target dev='vda' bus='virtio'/>
>     </disk>
>     <disk type='file' device='disk'>
>       <driver name='qemu' type='raw'/>
>       <source file=...>
>       <target dev='sda' bus='scsi'/>
>     </disk>
>     <controller type="scsi" index="0" model="virtio-scsi"/>
> 
> It's not something that we should worry about, and a simple comment
> with "XXX" in it is fine, if you agree with my analysis.

This case is handled fine in the patch, isn't it?  has_virtio_scsi is a
local variable which is only consulted when populating per-drive
s_controller field: for drives with bus='scsi' it differentiates between
Source_virtio_SCSI and Source_SCSI.

The place where we make up a single disk type is
v2v.ml:rcaps_from_source, but there I explicitly error out if the guest
has multiple disk types (which may be too harsh but simplifies things
for now).

Roman.