[virt-tools-list] [virt-manager] Add virtio-scsi disk bus option

[Chen HanXiao]

Hi
	Thanks for the review.
	I have posted v2 patch a few days ago:
https://www.redhat.com/archives/virt-tools-list/2012-November/msg00157.html
	But lots of codes are same as v1. 
	So I answer some of your comments inline.
> -----Original Message-----
> From: Cole Robinson [mailto:crobinso at redhat.com]
> Sent: Tuesday, November 27, 2012 11:52 PM
> To: Chen Hanxiao
> Cc: virt-tools-list at redhat.com
> Subject: Re: [virt-tools-list] [virt-manager] Add virtio-scsi disk bus
option
> 
> Hi Chen, thanks for the patch! Sorry for the review delay, comments
inline.
> 
> On 11/12/2012 03:12 AM, Chen Hanxiao wrote:
> > From: ChenHanxiao <chenhanxiao at cn.fujitsu.com>
> >
> > This patch will add virtio-scsi bus option on "Add New Virtual
> > Hardware" GUI page. It will support users to add a virtual disk
> > using SCSI bus with a controller model virtio-scsi.
> > In virt-manager, we will provide virtio-scsi disk XML for libvirt:
> >
> > <disk type='block' device='disk'>
> >   <driver name='qemu'/>
> >   <source dev='/dev/sdb'/>
> >   <target dev='sda' bus='scsi' model='virtio-scsi'/>
> > </disk>
> >
> > We provided tag 'model' for libvirt to determine which SCSI
> > controller model to use.
> >
> > Signed-off-by: ChenHanxiao <chenhanxiao at cn.fujitsu.com>
> > ---
> >  src/virtManager/addhardware.py |    7 +++++++
> >  src/virtManager/details.py     |    6 +++++-
> >  2 files changed, 12 insertions(+), 1 deletions(-)
> >
> > diff --git a/src/virtManager/addhardware.py
> b/src/virtManager/addhardware.py
> > index 4d894eb..9a7602c 100644
> > --- a/src/virtManager/addhardware.py
> > +++ b/src/virtManager/addhardware.py
> > @@ -539,6 +539,8 @@ class vmmAddHardware(vmmGObjectUI):
> >          if self.vm.get_hv_type() == "kvm":
> >              add_dev("sata", virtinst.VirtualDisk.DEVICE_DISK, "SATA
disk")
> >              add_dev("virtio", virtinst.VirtualDisk.DEVICE_DISK, "Virtio
> disk")
> > +            add_dev("virtio-scsi", virtinst.VirtualDisk.DEVICE_DISK,
> > +                "Virtio-scsi disk")
> 
> That last string is a disk label, so please make it something like
_('Virtio
> SCSI disk')
Will change in next version.

> 
> >          if self.conn.is_xen():
> >              add_dev("xen", virtinst.VirtualDisk.DEVICE_DISK, "Virtual
> disk")
> >
> > @@ -1222,6 +1224,10 @@ class vmmAddHardware(vmmGObjectUI):
> >          bus, device = self.get_config_disk_target()
> >          cache = self.get_config_disk_cache()
> >          fmt = self.get_config_disk_format()
> > +        model = None
> > +        if bus == "virtio-scsi":
> > +           bus = "scsi"
> > +           model = "virtio-scsi"
> >
> >          # Make sure default pool is running
> >          if self.is_default_storage():
> > @@ -1271,6 +1277,7 @@ class vmmAddHardware(vmmGObjectUI):
> >                                          device=device,
> >                                          bus=bus,
> >                                          driverCache=cache,
> > +                                        model=model,
> >                                          format=fmt)
> >
> >              if not fmt:
> > diff --git a/src/virtManager/details.py b/src/virtManager/details.py
> > index d20e748..0e86648 100644
> > --- a/src/virtManager/details.py
> > +++ b/src/virtManager/details.py
> > @@ -2219,7 +2219,11 @@ class vmmDetails(vmmGObjectUI):
> >              if bus == "spapr-vscsi":
> >                  bus = "scsi"
> >                  addr = "spapr-vio"
> > -            add_define(self.vm.define_disk_bus, dev_id_info, bus, addr)
> > +            if bus == "virtio-scsi":
> > +                bus = "scsi"
> > +                model = "virtio-scsi"
> > +            add_define(self.vm.define_disk_bus, dev_id_info, bus,
> > +                addr, model)
> >
> >          return self._change_config_helper(df, da, hf, ha)
> >
> >
> 
> Does this actually work? By my reading this breaks all disk adding because
> model is only defined for bus == "virtio-scsi", and define_disk_bus in
> domain.py wasn't changed to accept the model parameter.
> 
In this version, I leave this job of adding controllers to libvirt (with new
patchs).
But Daniel says apps should add it explicitly.
So at v2:
https://www.redhat.com/archives/virt-tools-list/2012-November/msg00157.html
I use model parameter just as helper to add controllers needed by
virtio-disks and 
It could be work without modifying current libvirt.

> An easy way to test all this stuff is to do
> 
>   virt-manager --connect test:///default
V2 looks fine with this command without changing domain.py.

> 
> That uses the libvirt stub driver which will allow you to add all manner
of
> devices to a fake guest and nothing will affect the local machine.
> 
> I've added a few commits to help with that, so make sure you rebase your
> changes on latest virt-manager git.
> 
> - Cole

Regards.