[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [virt-tools-list] [PATCH] virt-manager:Add "vSCSI disk" and "vSCSI cdrom" option into Device type in "add hardware" page



On 03/29/2012 10:21 PM, Cole Robinson wrote:
On 03/29/2012 03:44 AM, Li Zhang wrote:
From: Qing Lin<qinglbj linux vnet ibm com>

1.Add "vSCSI disk" and "vSCSI cdrom" option into device type on
"add storage page".
2.Add sPAPR-vSCSI controller when it's needed.
When vSCSI controller doesn't exist,there are two situations to
add it:one is when adding a vSCSI disk, another is when
an existing disk is changed into sPAPR-vSCSI bus type.
3.Fix Disk Lable text  on details page.

Signed-off-by: Qing Lin<qinglbj linux vnet ibm com>
Signed-off-by: Li Zhang<zhlcindy linux vnet ibm com>
---
  src/virtManager/addhardware.py |   30 ++++++++++++++++++++++++++++++
  src/virtManager/details.py     |   33 +++++++++++++++++++++++++++------
  2 files changed, 57 insertions(+), 6 deletions(-)

diff --git a/src/virtManager/addhardware.py b/src/virtManager/addhardware.py
index 50852ed..f7b3b3f 100644
--- a/src/virtManager/addhardware.py
+++ b/src/virtManager/addhardware.py
@@ -535,6 +535,9 @@ class vmmAddHardware(vmmGObjectUI):
              add_dev("ide", virtinst.VirtualDisk.DEVICE_CDROM, "IDE cdrom")
              add_dev("fdc", virtinst.VirtualDisk.DEVICE_FLOPPY, "Floppy disk")

+            if (self.vm.get_machtype() == "pseries"):
+                add_dev("spapr-vscsi", virtinst.VirtualDisk.DEVICE_DISK, "vSCSI disk")
+                add_dev("spapr-vscsi", virtinst.VirtualDisk.DEVICE_CDROM, "vSCSI cdrom")

Does a pseries machine really support IDE and all these other options? If not,
might as well limit the device list to the actually supported values while you
are here.

No, a pseries machine doesn't support IDE bus.
we will limit the device list. Thanks :)


Also, I think you should add an extra value to the disk type combo, storing
the disk address. So for these spapr devices, you'd have an add_dev call like

add_dev("scsi", virtinst.VirtualDisk.DEVICE_DISK, "vSCSI disk",
         address="spapr-vscsi")

All other devices wouldn't set address, and itll just default to None. Then
you can avoid all the conditionalizing later on, and just do

OK. Good suggestion. I will consider to modify it.

if address:
   dev.set_address(address)

We should have done that in details.py as well
It also needs to set the address here.
Because this is another device which is added by "add hardware".


              if self.vm.rhel6_defaults():
                  add_dev("scsi", virtinst.VirtualDisk.DEVICE_DISK, "SCSI disk")
                  add_dev("usb", virtinst.VirtualDisk.DEVICE_DISK, "USB disk")
@@ -1176,6 +1179,25 @@ class vmmAddHardware(vmmGObjectUI):
              if not res:
                  return (False, None)

+        if self.vm._guest:

Why this check? The fact that you are accessing a private variable (as denoted
by the leading underscore) is an indication something is wrong here.

Oh, you are right. I didn't realize it.
I will review whether this check is necessary.
We will modify it. :)


+            if (self._dev.get_virtual_device_type() == VirtualDevice.VIRTUAL_DEV_DISK and
+                self._dev.address.type == "spapr-vio"):
+                vscsi_need_add = True
+                for dev in self.vm._guest._devices:
+                    if (dev.get_virtual_device_type() == VirtualDevice.VIRTUAL_DEV_CONTROLLER and
+                        dev.address.type == "spapr-vio"):
+                        vscsi_need_add = False
+                        break
+                if vscsi_need_add:
+                    ctrl = virtinst.VirtualController.get_class_for_type(
+                               virtinst.VirtualController.CONTROLLER_TYPE_SCSI)(self.vm._guest.conn)
+                    ctrl.set_address("spapr-vio")
+                    try:
+                        self.vm.add_device(ctrl)
+                    except Exception, e:
+                        self.err.show_err(_("Error adding device: %s" % str(e)))
+                        return (True, None)
+

libvirt needs to handle this for us, as I mentioned in the virtinst patch
review. Until then, NACK to this bit.
OK. We can have some discussion about it.


          # Alter persistent config
          try:
              self.vm.add_device(self._dev)
@@ -1264,6 +1286,11 @@ class vmmAddHardware(vmmGObjectUI):
                      if do_use:
                          diskpath = ideal

+            is_vscsi = False
+            if bus == "spapr-vscsi":
+                bus = "scsi"
+                is_vscsi = True
+
              disk = virtinst.VirtualDisk(conn=self.conn.vmm,
                                          path=diskpath,
                                          size=disksize,
@@ -1274,6 +1301,9 @@ class vmmAddHardware(vmmGObjectUI):
                                          driverCache=cache,
                                          format=fmt)

+            if is_vscsi:
+                disk.set_address("spapr-vio")
+
              if not fmt:
                  fmt = self.config.get_storage_format()
                  if (self.is_default_storage() and
diff --git a/src/virtManager/details.py b/src/virtManager/details.py
index 5395087..368b444 100644
--- a/src/virtManager/details.py
+++ b/src/virtManager/details.py
@@ -2217,6 +2217,22 @@ class vmmDetails(vmmGObjectUI):
              if bus == "spapr-vscsi":
                  bus = "scsi"
                  addr = "spapr-vio"
+                vscsi_need_add = True
+                if self.vm._guest:
+                    for dev in self.vm._guest._devices:
+                        if (dev.get_virtual_device_type() == virtinst.VirtualDevice.VIRTUAL_DEV_CONTROLLER  and
+                            dev.address.type == "spapr-vio"):
+                            vscsi_need_add = False
+                            break
+                    if vscsi_need_add:
+                        ctrl = virtinst.VirtualController.get_class_for_type(
+                                   virtinst.VirtualController.CONTROLLER_TYPE_SCSI)(self.vm._guest.conn)
+                        ctrl.set_address("spapr-vio")
+                        try:
+                            self.vm.add_device(ctrl)
+                        except Exception, e:
+                            self.err.show_err(_("Error adding device: %s" % str(e)))
+                            return (True, None)
              add_define(self.vm.define_disk_bus, dev_id_info, bus, addr)

          return self._change_config_helper(df, da, hf, ha)
@@ -2806,7 +2822,6 @@ class vmmDetails(vmmGObjectUI):
          ro = disk.read_only
          share = disk.shareable
          bus = disk.bus
-        addr = disk.address.type
          idx = disk.disk_bus_index
          cache = disk.driver_cache
          io = disk.driver_io
@@ -2830,9 +2845,11 @@ class vmmDetails(vmmGObjectUI):
          is_cdrom = (devtype == virtinst.VirtualDisk.DEVICE_CDROM)
          is_floppy = (devtype == virtinst.VirtualDisk.DEVICE_FLOPPY)

-        if addr == "spapr-vio":
-            bus = "spapr-vscsi"

+        if (self.vm.get_hv_type() == "kvm" and
+                    self.vm.get_machtype() == "pseries"):
+            if bus == "scsi":
+                bus = "spapr-vscsi"
          pretty_name = prettyify_disk(devtype, bus, idx)

          self.widget("disk-source-path").set_text(path or "-")
@@ -3289,6 +3306,9 @@ class vmmDetails(vmmGObjectUI):
              buses.append(["ide", "IDE"])
              if self.vm.rhel6_defaults():
                  buses.append(["scsi", "SCSI"])
+            if (self.vm.get_hv_type() == "kvm" and
+                    self.vm.get_machtype() == "pseries"):
+                buses.append(["spapr-vscsi", "sPAPR-vSCSI"])
          else:
              if self.vm.is_hvm():
                  buses.append(["ide", "IDE"])
@@ -3386,9 +3406,10 @@ class vmmDetails(vmmGObjectUI):
              elif devtype == "floppy":
                  icon = "media-floppy"

-            if disk.address.type == "spapr-vio":
-                bus = "spapr-vscsi"
-
+            if (self.vm.get_hv_type() == "kvm" and
+                    self.vm.get_machtype() == "pseries"):
+                if bus == "scsi":
+                    bus = "spapr-vscsi"
              label = prettyify_disk(devtype, bus, idx)

              update_hwlist(HW_LIST_TYPE_DISK, disk, label, icon)

This should all be changed to explicity pass the address type to prettify_disk
as well. Then it doesn't even matter if we are a pseries machine, if we see
bus == scsi and addresstype == spapr-vscsi, we know its a vscsi disk.
As I mentioned in the mail for virinst,

There is a little tricky here, we pass the disk's address type as
spapr-vio from virt-manager GUI, in fact the address type of the
disk is "drive". So here the address type we get, it is not
spapr-vio.
So we use pseries machine to specify that it is vscsi.


Thanks,
Cole



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]