[virt-tools-list] [virt-manager PATCH 7/7] scsi: Add controller index option and virtio-scsi bus option

Lin Ma lma at suse.com
Mon Nov 6 12:52:09 UTC 2017


When we change disk bus to 'scsi' or add scsi disks, virt-manager relies
on libvirt's rules to create addresses. Sometimes it works, sometimes not,
Espectially in multiple scsi controllers case, we can not exactly control
which virtual disks attach to which scsi controller, sometimes libvirt
creates unnecessary scsi controllers for us, attach virtual disks to the
scsi controllers which we don't want. It has confused users for a while.

This patch added two new option to solve the above issue:
1. add virtio-scsi bus option. users can decide which kind of scsi bus
they want to use, instead of guessing through libvirt.
2. add controller index option. when users configure disk bus, it allows
users to exactly choice which existed scsi controller they want to attach
or create a new one.

The patch changed some mechanism about adding/configuring scsi bus, and
generating device addrStr instead of libvirt did, it avoids some annoying
issues.

Signed-off-by: Lin Ma <lma at suse.com>
---
 ui/addhardware.ui          |  40 +++++++++++++++
 ui/details.ui              |  40 +++++++++++++++
 virtManager/addhardware.py | 125 +++++++++++++++++++++++++++++++++++----------
 virtManager/details.py     |  80 ++++++++++++++++++++++++++++-
 virtManager/domain.py      |   7 ++-
 virtinst/device.py         |  15 +++++-
 virtinst/devicedisk.py     |   2 +
 7 files changed, 278 insertions(+), 31 deletions(-)

diff --git a/ui/addhardware.ui b/ui/addhardware.ui
index eb476dab..e90d31e6 100644
--- a/ui/addhardware.ui
+++ b/ui/addhardware.ui
@@ -169,6 +169,45 @@
                                     <property name="top_attach">0</property>
                                   </packing>
                                 </child>
+                                <child>
+                                  <object class="GtkGrid" id="storage-scsi-idx">
+                                    <property name="visible">True</property>
+                                    <property name="can_focus">False</property>
+                                    <property name="row_spacing">3</property>
+                                    <property name="column_spacing">8</property>
+                                    <child>
+                                      <object class="GtkLabel" id="label37">
+                                        <property name="visible">True</property>
+                                        <property name="can_focus">False</property>
+                                        <property name="halign">start</property>
+                                        <property name="label" translatable="yes">Controlle_r:</property>
+                                        <property name="use_underline">True</property>
+                                        <property name="mnemonic_widget">storage-scsi-index</property>
+                                      </object>
+                                      <packing>
+                                        <property name="left_attach">0</property>
+                                        <property name="top_attach">0</property>
+                                      </packing>
+                                    </child>
+                                    <child>
+                                      <object class="GtkComboBox" id="storage-scsi-index">
+                                        <property name="visible">True</property>
+                                        <property name="can_focus">False</property>
+                                        <property name="halign">start</property>
+                                      </object>
+                                      <packing>
+                                        <property name="left_attach">1</property>
+                                        <property name="top_attach">0</property>
+                                      </packing>
+                                    </child>
+                                  </object>
+                                  <packing>
+                                    <property name="expand">False</property>
+                                    <property name="fill">True</property>
+                                    <property name="left_attach">2</property>
+                                    <property name="top_attach">1</property>
+                                  </packing>
+                                </child>
                                 <child>
                                   <object class="GtkComboBox" id="storage-devtype">
                                     <property name="visible">True</property>
@@ -205,6 +244,7 @@
                                     <property name="visible">True</property>
                                     <property name="can_focus">False</property>
                                     <property name="halign">start</property>
+                                    <signal name="changed" handler="on_storage_bustype_changed" swapped="no"/>
                                   </object>
                                   <packing>
                                     <property name="left_attach">1</property>
diff --git a/ui/details.ui b/ui/details.ui
index 1202db07..5f8cfe74 100644
--- a/ui/details.ui
+++ b/ui/details.ui
@@ -3559,6 +3559,46 @@ if you know what you are doing.</small></property>
                                                         <property name="top_attach">0</property>
                                                       </packing>
                                                     </child>
+                                                    <child>
+                                                      <object class="GtkGrid" id="controller-idx">
+                                                        <property name="visible">True</property>
+                                                        <property name="can_focus">False</property>
+                                                        <property name="row_spacing">3</property>
+                                                        <property name="column_spacing">8</property>
+                                                        <child>
+                                                          <object class="GtkLabel" id="label67">
+                                                            <property name="visible">True</property>
+                                                            <property name="can_focus">False</property>
+                                                            <property name="halign">start</property>
+                                                            <property name="label" translatable="yes">Controlle_r:</property>
+                                                            <property name="use_underline">True</property>
+                                                            <property name="mnemonic_widget">controller-index</property>
+                                                          </object>
+                                                          <packing>
+                                                            <property name="left_attach">0</property>
+                                                            <property name="top_attach">0</property>
+                                                          </packing>
+                                                        </child>
+                                                        <child>
+                                                          <object class="GtkComboBox" id="controller-index">
+                                                            <property name="visible">True</property>
+                                                            <property name="can_focus">False</property>
+                                                            <property name="halign">start</property>
+                                                            <signal name="changed" handler="on_controller_index_combo_changed" swapped="no"/>
+                                                          </object>
+                                                          <packing>
+                                                            <property name="left_attach">1</property>
+                                                            <property name="top_attach">0</property>
+                                                          </packing>
+                                                        </child>
+                                                      </object>
+                                                      <packing>
+                                                      <property name="expand">False</property>
+                                                      <property name="fill">True</property>
+                                                      <property name="left_attach">2</property>
+                                                      <property name="top_attach">0</property>
+                                                      </packing>
+                                                    </child>
                                                     <child>
                                                       <object class="GtkEntry" id="disk-serial">
                                                         <property name="visible">True</property>
diff --git a/virtManager/addhardware.py b/virtManager/addhardware.py
index d661602d..9205725f 100644
--- a/virtManager/addhardware.py
+++ b/virtManager/addhardware.py
@@ -97,6 +97,7 @@ class vmmAddHardware(vmmGObjectUI):
             "on_create_finish_clicked": self._finish,
             "on_hw_list_changed": self._hw_selected,
 
+            "on_storage_bustype_changed": self._change_storage_bustype,
             "on_storage_devtype_changed": self._change_storage_devtype,
 
             "on_mac_address_clicked": self._change_macaddr_use,
@@ -196,6 +197,10 @@ class vmmAddHardware(vmmGObjectUI):
         netmodel_list = self.widget("net-model")
         self.build_network_model_combo(self.vm, netmodel_list)
 
+        # Scsi index combo
+        self.build_controller_index_combo(self.vm,
+            self.widget("storage-scsi-index"))
+
         # Disk bus type
         self.build_disk_bus_combo(self.vm,
             self.widget("storage-bustype"))
@@ -678,6 +683,41 @@ class vmmAddHardware(vmmGObjectUI):
             model.append([None, _("Hypervisor default")])
         combo.set_active(0)
 
+    @staticmethod
+    def build_controller_index_combo(vm, combo):
+        ignore = vm
+        model = Gtk.ListStore(str, str)
+        combo.set_model(model)
+        uiutil.init_combo_text_column(combo, 1)
+        model.set_sort_column_id(1, Gtk.SortType.ASCENDING)
+        combo.set_active(-1)
+
+    @staticmethod
+    def has_available_scsi_unit(vm, controller_index):
+        used_unit = 0
+        for disk in vm.get_disk_devices(inactive=True):
+            if disk.bus == "scsi":
+                if disk.address.controller == controller_index:
+                    used_unit += 1
+        if used_unit < 7:
+            return True
+        else:
+            return False
+
+    @staticmethod
+    def find_available_scsi_unit(vm, controller_index):
+        max_unit = 7
+        avail_unit = 0
+        used_list = []
+        for disk in vm.get_disk_devices(inactive=True):
+            if disk.bus == "scsi":
+                if str(disk.address.controller) == controller_index:
+                    used_list.append(disk.address.unit)
+        while avail_unit < max_unit:
+            if avail_unit not in used_list:
+                return avail_unit
+            avail_unit += 1
+
     @staticmethod
     def build_disk_bus_combo(vm, combo):
         ignore = vm
@@ -718,6 +758,9 @@ class vmmAddHardware(vmmGObjectUI):
             if vm.conn.is_xen() or vm.conn.is_test():
                 disk_bus_types.append("xen")
 
+        if vm.get_hv_type() in ["qemu", "kvm", "test"]:
+            disk_bus_types.append("virtio-scsi")
+
         rows = []
         for bus in disk_bus_types:
             rows.append([bus, virtinst.VirtualDisk.pretty_disk_bus(bus)])
@@ -725,10 +768,11 @@ class vmmAddHardware(vmmGObjectUI):
         model.clear()
 
         bus_map = {
-            "disk": ["ide", "sata", "scsi", "sd", "usb", "virtio", "xen"],
+            "disk": ["ide", "sata", "scsi", "sd", "usb", "virtio", "xen",
+                     "virtio-scsi"],
             "floppy": ["fdc"],
-            "cdrom": ["ide", "sata", "scsi"],
-            "lun": ["scsi"],
+            "cdrom": ["ide", "sata", "scsi", "virtio-scsi"],
+            "lun": ["scsi", "virtio-scsi"],
         }
         for row in rows:
             if row[0] in bus_map[devtype]:
@@ -1132,6 +1176,30 @@ class vmmAddHardware(vmmGObjectUI):
     # Device page listeners #
     #########################
 
+    def _change_storage_bustype(self, ignore):
+        bustype = uiutil.get_list_selection(
+            self.widget("storage-bustype"))
+
+        controller_model = bustype
+        if controller_model in ["scsi", "virtio-scsi"]:
+            model = self.widget("storage-scsi-index").get_model()
+            model.clear()
+            if controller_model == "scsi":
+                controller_model = None
+            for controller in self.vm.get_controller_devices():
+                if (controller.type == "scsi" and
+                    controller.model == controller_model):
+                    if not vmmAddHardware.has_available_scsi_unit(
+                        self.vm, controller.index):
+                        continue
+                    model.append([str(controller.index),
+                                 str(controller.index)])
+            model.append(["new", "New"])
+            self.widget("storage-scsi-index").set_active(0)
+            self.widget("storage-scsi-idx").set_visible(True)
+        else:
+            self.widget("storage-scsi-idx").set_visible(False)
+
     def _change_storage_devtype(self, ignore):
         devtype = uiutil.get_list_selection(
             self.widget("storage-devtype"))
@@ -1429,10 +1497,9 @@ class vmmAddHardware(vmmGObjectUI):
             self._dev.validate()
         return ret
 
-    def _set_disk_controller(self, disk, controller_model, used_disks):
-        # Add a SCSI controller with model virtio-scsi if needed
+    def _set_disk_controller(self, disk, controller_model):
         disk.vmm_controller = None
-        if controller_model != "virtio-scsi":
+        if disk.bus != "scsi":
             return None
 
         # Get SCSI controllers
@@ -1440,7 +1507,7 @@ class vmmAddHardware(vmmGObjectUI):
         ctrls_scsi = [x for x in controllers if
                 (x.type == VirtualController.TYPE_SCSI)]
 
-        # Create possible new controller
+        # Create new controller
         controller = VirtualController(self.conn.get_backend())
         controller.type = "scsi"
         controller.model = controller_model
@@ -1450,23 +1517,7 @@ class vmmAddHardware(vmmGObjectUI):
         if ctrls_scsi:
             controller.index = max([x.index for x in ctrls_scsi]) + 1
 
-        # Take only virtio-scsi ones
-        ctrls_scsi = [x for x in ctrls_scsi
-                      if x.model == controller_model]
-
-        # Save occupied places per controller
-        occupied = collections.defaultdict(int)
-        for d in used_disks:
-            if (d.get_target_prefix() == disk.get_target_prefix() and
-                d.bus == "scsi"):
-                num = virtinst.VirtualDisk.target_to_num(d.target)
-                occupied[num / 7] += 1
-        for c in ctrls_scsi:
-            if occupied[c.index] < 7:
-                controller = c
-                break
-        else:
-            disk.vmm_controller = controller
+        disk.vmm_controller = controller
 
         return controller.index
 
@@ -1479,9 +1530,12 @@ class vmmAddHardware(vmmGObjectUI):
             self.widget("storage-cache"))
 
         controller_model = None
-        if (bus == "scsi" and
+        controller_index = None
+        scsi_unit = None
+        if (bus == "virtio-scsi" and
             self.vm.get_hv_type() in ["qemu", "kvm", "test"] and
             not self.vm.xmlobj.os.is_pseries()):
+            bus = "scsi"
             controller_model = "virtio-scsi"
 
         collidelist = [d.path for d in self.vm.get_disk_devices()]
@@ -1507,10 +1561,25 @@ class vmmAddHardware(vmmGObjectUI):
                 if d.target not in used:
                     used.append(d.target)
 
-            prefer_ctrl = self._set_disk_controller(
-                disk, controller_model, self.vm.get_disk_devices(inactive=True))
+            new_ctrl_idx = self._set_disk_controller(disk, controller_model)
+            disk.generate_target(used, new_ctrl_idx)
+
+            if bus == "scsi":
+                controller_index = uiutil.get_list_selection(
+                    self.widget("storage-scsi-index"))
+                if controller_index != "new":
+                    scsi_unit = vmmAddHardware.find_available_scsi_unit(
+                        self.vm, controller_index)
+                    disk.address.type = "drive"
+                    disk.address.controller = controller_index
+                    disk.address.bus = "0"
+                    disk.address.target = "0"
+                    disk.address.unit  = scsi_unit
+                    disk.vmm_controller = None
+                else:
+                    disk.address.type = "drive"
+                    disk.address.controller = new_ctrl_idx
 
-            disk.generate_target(used, prefer_ctrl)
         except Exception as e:
             return self.err.val_err(_("Storage parameter error."), e)
 
diff --git a/virtManager/details.py b/virtManager/details.py
index a9e0cb79..3c116787 100644
--- a/virtManager/details.py
+++ b/virtManager/details.py
@@ -30,6 +30,7 @@ import libvirt
 import virtinst
 from virtinst import util
 from virtinst import VirtualRNGDevice
+from virtinst import VirtualController
 
 from . import vmmenu
 from . import uiutil
@@ -538,7 +539,8 @@ class vmmDetails(vmmGObjectUI):
             "on_disk_removable_changed": lambda *x: self.enable_apply(x, EDIT_DISK_REMOVABLE),
             "on_disk_cache_combo_changed": lambda *x: self.enable_apply(x, EDIT_DISK_CACHE),
             "on_disk_io_combo_changed": lambda *x: self.enable_apply(x, EDIT_DISK_IO),
-            "on_disk_bus_combo_changed": lambda *x: self.enable_apply(x, EDIT_DISK_BUS),
+            "on_controller_index_combo_changed": lambda *x: self.enable_apply(x, EDIT_DISK_BUS),
+            "on_disk_bus_combo_changed": self.disk_bus_combo_changed,
             "on_disk_format_changed": self.disk_format_changed,
             "on_disk_serial_changed": lambda *x: self.enable_apply(x, EDIT_DISK_SERIAL),
             "on_disk_sgio_entry_changed": lambda *x: self.enable_apply(x, EDIT_DISK_SGIO),
@@ -1006,6 +1008,10 @@ class vmmDetails(vmmGObjectUI):
         disk_bus = self.widget("disk-bus")
         vmmAddHardware.build_disk_bus_combo(self.vm, disk_bus)
 
+        # Controller index combo
+        controller_index = self.widget("controller-index")
+        vmmAddHardware.build_controller_index_combo(self.vm, controller_index)
+
         # Network model
         net_model = self.widget("network-model")
         vmmAddHardware.build_network_model_combo(self.vm, net_model)
@@ -1822,6 +1828,41 @@ class vmmDetails(vmmGObjectUI):
         boot_list.get_selection().emit("changed")
         self.enable_apply(EDIT_BOOTORDER)
 
+    def disk_bus_combo_changed(self, ignore):
+        controller_model = uiutil.get_list_selection(self.widget("disk-bus"))
+
+        if controller_model in ["scsi", "virtio-scsi"]:
+            disk = self.get_hw_selection(HW_LIST_COL_DEVICE)
+            model = self.widget("controller-index").get_model()
+            model.clear()
+            current_controller = -1
+            i = -1
+            if controller_model == "scsi":
+                controller_model = None
+            for controller in self.vm.get_controller_devices():
+                if (controller.type == "scsi" and
+                    controller.model == controller_model):
+                    i += 1
+                    if (disk.bus == "scsi" and
+                        disk.address.controller == controller.index):
+                        current_controller = i
+                    if not vmmAddHardware.has_available_scsi_unit(
+                        self.vm, controller.index):
+                        if current_controller != i:
+                            continue
+                    model.append([str(controller.index),
+                                 str(controller.index)])
+            model.append(["new", "New"])
+            if current_controller >= 0:
+                self.widget("controller-index").set_active(current_controller)
+            else:
+                self.widget("controller-index").set_active(0)
+            self.widget("controller-idx").set_visible(True)
+        else:
+            self.widget("controller-idx").set_visible(False)
+
+        self.enable_apply(EDIT_DISK_BUS)
+
     def disk_format_changed(self, ignore):
         self.widget("disk-format-warn").show()
         self.enable_apply(EDIT_DISK_FORMAT)
@@ -2127,6 +2168,35 @@ class vmmDetails(vmmGObjectUI):
         if self.edited(EDIT_DISK_BUS):
             bus = uiutil.get_list_selection(self.widget("disk-bus"))
             addr = None
+            controller_model = None
+            if bus in ["scsi", "virtio-scsi"]:
+                controller_index = uiutil.get_list_selection(
+                    self.widget("controller-index"))
+                if bus == "virtio-scsi":
+                    bus = "scsi"
+                    controller_model = "virtio-scsi"
+                if controller_index != "new":
+                    scsi_bus = "0"
+                    scsi_target = "0"
+                    scsi_unit = vmmAddHardware.find_available_scsi_unit(
+                        self.vm, controller_index)
+                    addr = (controller_index + "/" + scsi_bus + "/" +
+                            scsi_target + "/" + str(scsi_unit))
+                else:
+                    controllers = self.vm.get_controller_devices()
+                    ctrls_scsi = [x for x in controllers if (x.type == "scsi")]
+
+                    new_ctrl = VirtualController(self.conn.get_backend())
+                    new_ctrl.type = "scsi"
+                    new_ctrl.model = controller_model
+                    new_ctrl.index = 0
+
+                    if ctrls_scsi:
+                        new_ctrl.index = max([x.index for x in ctrls_scsi]) + 1
+
+                    self.vm.add_device(new_ctrl)
+
+                    addr = str(new_ctrl.index) + "/"
 
             kwargs["bus"] = bus
             kwargs["addrstr"] = addr
@@ -2681,6 +2751,14 @@ class vmmDetails(vmmGObjectUI):
         vmmAddHardware.populate_disk_bus_combo(self.vm, devtype,
             self.widget("disk-bus").get_model())
         uiutil.set_list_selection(self.widget("disk-bus"), bus)
+        if bus == virtinst.VirtualController.TYPE_SCSI:
+            for controller in self.vm.get_controller_devices():
+                if (controller.type == "scsi" and
+                    controller.index == disk.address.controller):
+                        if controller.model == "virtio-scsi":
+                            uiutil.set_list_selection(self.widget("disk-bus"),
+                                                      "virtio-scsi")
+
         self.widget("disk-serial").set_text(serial or "")
 
         button = self.widget("disk-cdrom-connect")
diff --git a/virtManager/domain.py b/virtManager/domain.py
index a1f59e38..fb658d59 100644
--- a/virtManager/domain.py
+++ b/virtManager/domain.py
@@ -791,7 +791,12 @@ class vmmDomain(vmmLibvirtObject):
             editdev.bus = bus
 
             if oldbus == bus:
-                return
+                if oldbus == "scsi":
+                    controller_index = addrstr.split("/")[0]
+                    if editdev.address.controller == controller_index:
+                        return
+                else:
+                    return
 
             editdev.address.clear()
             editdev.address.set_addrstr(addrstr)
diff --git a/virtinst/device.py b/virtinst/device.py
index 4f9dcb2c..a8436177 100644
--- a/virtinst/device.py
+++ b/virtinst/device.py
@@ -36,7 +36,7 @@ class VirtualDeviceAddress(XMLBuilder):
     """
     Examples:
     <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
-    <address type='drive' controller='0' bus='0' unit='0'/>
+    <address type='drive' controller='0' bus='0' target='0' unit='0'/>
     <address type='ccid' controller='0' slot='0'/>
     <address type='virtio-serial' controller='1' bus='0' port='4'/>
     """
@@ -68,6 +68,19 @@ class VirtualDeviceAddress(XMLBuilder):
                 self.domain, self.bus = addrstr.split(":", 1)
         elif addrstr == "spapr-vio":
             self.type = self.ADDRESS_TYPE_SPAPR_VIO
+        elif addrstr.count("/") in [1, 2, 3]:
+            self.type = self.ADDRESS_TYPE_DRIVE
+            self.controller = addrstr.split("/")[0]
+            if addrstr.count("/") == 1:
+                if addrstr.split("/")[1] != "":
+                    self.bus = addrstr.split("/")[1]
+            elif addrstr.count("/") == 2:
+                self.bus = addrstr.split("/")[1]
+                self.target = addrstr.split("/")[2]
+            elif addrstr.count("/") == 3:
+                self.bus = addrstr.split("/")[1]
+                self.target = addrstr.split("/")[2]
+                self.unit = addrstr.split("/")[3]
         else:
             raise ValueError(_("Could not determine or unsupported "
                                "format of '%s'") % addrstr)
diff --git a/virtinst/devicedisk.py b/virtinst/devicedisk.py
index 73ccb5de..16f860f2 100644
--- a/virtinst/devicedisk.py
+++ b/virtinst/devicedisk.py
@@ -159,6 +159,8 @@ class VirtualDisk(VirtualDevice):
             return bus.capitalize()
         if bus == "virtio":
             return "VirtIO"
+        if bus == "virtio-scsi":
+            return "VirtIO SCSI"
         return bus
 
     @staticmethod
-- 
2.14.0




More information about the virt-tools-list mailing list