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

[Chen HanXiao]

Thanks for the review. New patch will be posted soon.

Regards


> -----Original Message-----
> From: Cole Robinson [mailto:crobinso at redhat.com]
> Sent: Wednesday, December 05, 2012 11:23 PM
> To: Chen Hanxiao
> Cc: virt-tools-list at redhat.com
> Subject: Re: [virt-tools-list][virt-manager v3]Add virtio-scsi disk bus
option
> 
> On 12/03/2012 05:25 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.
> > If there is no SCSI controller existed, a new SCSI controller by model
> > 'virtio-scsi' will be added automatically.
> >
> > Signed-off-by: ChenHanxiao <chenhanxiao at cn.fujitsu.com>
> >
> > diff --git a/src/virtManager/addhardware.py
> > b/src/virtManager/addhardware.py index 4d894eb..f0b8319 100644
> > --- a/src/virtManager/addhardware.py
> > +++ b/src/virtManager/addhardware.py
> > @@ -34,6 +34,7 @@ import virtManager.uihelpers as uihelpers  from
> > virtManager.asyncjob import vmmAsyncJob  from
> > virtManager.storagebrowse import vmmStorageBrowser  from
> > virtManager.baseclass import vmmGObjectUI
> > +from virtinst.VirtualController import VirtualControllerSCSI
> >
> >  PAGE_ERROR = 0
> >  PAGE_DISK = 1
> > @@ -539,6 +540,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")
> >          if self.conn.is_xen():
> >              add_dev("xen", virtinst.VirtualDisk.DEVICE_DISK, "Virtual
> > disk")
> >
> > @@ -1151,10 +1154,16 @@ class vmmAddHardware(vmmGObjectUI):
> >
> >          self._dev.get_xml_config()
> >          logging.debug("Adding device:\n" +
> > self._dev.get_xml_config())
> > +        if self.has_controller():
> > +            if self._controller is not None:
> > +                logging.debug("Adding controller:\n"
> > +                            + self._controller.get_xml_config())
> >
> >          # Hotplug device
> >          attach_err = False
> >          try:
> > +            if self.has_controller() and self._controller is not None:
> > +                self.vm.attach_device(self._controller)
> >              self.vm.attach_device(self._dev)
> >          except Exception, e:
> >              logging.debug("Device could not be hotplugged: %s",
> > str(e)) @@ -1176,6 +1185,13 @@ class vmmAddHardware(vmmGObjectUI):
> >                  return (False, None)
> >
> >          # Alter persistent config
> > +        if self.has_controller() and self._controller is not None:
> > +            try:
> > +                self.vm.add_device(self._controller)
> > +            except Excpetion, e:
> > +                self.err.show_err(_("Error adding device: %s" %
str(e)))
> > +                return (True, None)
> > +
> 
> Similar to the hotplug case, move the has_controller() bit into the same
> exception block as the add_device() call below.
> 
> >          try:
> >              self.vm.add_device(self._dev)
> >          except Exception, e:
> > @@ -1184,6 +1200,14 @@ class vmmAddHardware(vmmGObjectUI):
> >
> >          return (False, None)
> >
> > +    #check whether device has attribute '_controller'
> > +    def has_controller(self):
> > +        try:
> > +            self._controller
> > +        except AttributeError:
> > +            return False
> > +        else:
> > +            return True
> >
> 
> This is overkill, just replace instances of has_controller() with
getattr(self,
> "_controller", None)
> 
> Though I have another recommendation below
> 
> >      ###########################
> >      # Page validation methods #
> > @@ -1222,6 +1246,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 +1299,7 @@ class vmmAddHardware(vmmGObjectUI):
> >                                          device=device,
> >                                          bus=bus,
> >                                          driverCache=cache,
> > +                                        model=model,
> >                                          format=fmt)
> >
> >              if not fmt:
> > @@ -1316,6 +1345,15 @@ class vmmAddHardware(vmmGObjectUI):
> >          uihelpers.check_path_search_for_qemu(self.topwin,
> >                                               self.conn, disk.path)
> >
> > +        if disk.model == "virtio-scsi":
> > +            controllers = self.vm.get_controller_devices()
> > +            controller = VirtualControllerSCSI(conn = self.conn.vmm)
> > +            controller.set_model(disk.model)
> > +            self._controller = controller
> > +            for d in controllers:
> > +                if disk.model == d.model:
> > +                   self._controller = None
> > +
> >          self._dev = disk
> >          return True
> >
> >
> 
> If we drop the virtinst patch, you can still make all this work. Just
stick the
> 'model' value in disk.vmm__model and the controller in
disk.vmm__controller.
> It's kinda hacky, but it's also one of the nice things about python is
that you
> can just monkey patch everything.
> 
> This will actually solve a real bug: if someone adds a virtio-scsi disk,
> self._controller is set. Then if the user later tries to add an audio
device,
> self._controller is still set and we will attempt to add it again.
> 
> Thanks,
> Cole