[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 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.

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

if address:
  dev.set_address(address)

We should have done that in details.py as well

>              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.

> +            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.

>          # 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.

Thanks,
Cole


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