Thanks for the review. New patch will be posted soon. Regards > -----Original Message----- > From: Cole Robinson [mailto:crobinso@xxxxxxxxxx] > Sent: Wednesday, December 05, 2012 11:23 PM > To: Chen Hanxiao > Cc: virt-tools-list@xxxxxxxxxx > Subject: Re: [virt-manager v3]Add virtio-scsi disk bus option > > On 12/03/2012 05:25 AM, Chen Hanxiao wrote: > > From: ChenHanxiao <chenhanxiao@xxxxxxxxxxxxxx> > > > > 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@xxxxxxxxxxxxxx> > > > > 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 _______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
