[virt-tools-list] [virt-manager PATCH v2 2/2] add support for modifying scsi controller model

[Cole Robinson]

On 01/20/2014 07:15 AM, Chen Hanxiao wrote:
> From: Chen Hanxiao <chenhanxiao at cn.fujitsu.com>
> 
> We could modify scsi controller model
> between 'default' and 'virtio-scsi'.
> 
> Signed-off-by: Chen Hanxiao <chenhanxiao at cn.fujitsu.com>
> ---
> v2: fix an issue if no scsi controller existed
> 

I would just combine these two patches, patch 1 isn't much use with this bit.

>  virtManager/domain.py | 39 +++++++++++++++++++++++++++++++++------
>  1 file changed, 33 insertions(+), 6 deletions(-)
> 
> diff --git a/virtManager/domain.py b/virtManager/domain.py
> index ada404b..f590c5e 100644
> --- a/virtManager/domain.py
> +++ b/virtManager/domain.py
> @@ -822,27 +822,54 @@ class vmmDomain(vmmLibvirtObject):
>  
>      # Controller define methods
>      def define_controller_model(self, devobj, newmodel):
> +        #model list from libvirt

Please put a space after the # in comments

> +        usb_controller_model = ["default", "piix3-uhci", "piix4-uhci",
> +                "ehci", "ich9-ehci1", "ich9-uhci1", "ich9-uhci2",
> +                "ich9-uhci3", "vt82c686b-uhci", "pci-ohci", "nec-xhci"]
> +        scsi_controller_model = ["default_virtio_scsi", "auto", "buslogic",
> +                "lsilogic", "lsisas1068", "vmpvscsi", "ibmvscsi", "virtio-scsi",
> +                "lsisas1078"]
> +

Doing things this was is a bit overkill and prone to future breakage. You can
just check devobj.type == TYPE_USB or TYPE_SCSI to make the change.

>          def change(editdev):
>              ignore = editdev
>  
>              guest = self._get_xmlobj_to_define()
>              ctrls = guest.get_devices("controller")
> -            ctrls = [x for x in ctrls if (x.type ==
> -                     VirtualController.TYPE_USB)]
> -            for dev in ctrls:
> -                guest.remove_device(dev)
> -
> +            ctrls_usb = [x for x in ctrls if
> +                    (x.type == VirtualController.TYPE_USB)]
> +            ctrls_scsi = [x for x in ctrls if
> +                    (x.type == VirtualController.TYPE_SCSI)]
> +
> +            for dev in ctrls_usb:
> +                if newmodel in usb_controller_model:
> +                    guest.remove_device(dev)
>              if newmodel == "ich9-ehci1":
>                  for dev in VirtualController.get_usb2_controllers(
>                          guest.conn):
>                      guest.add_device(dev)
> -            else:
> +            elif newmodel in usb_controller_model:
>                  dev = VirtualController(guest.conn)
>                  dev.type = "usb"
>                  if newmodel != "default":
>                      dev.model = newmodel
>                  guest.add_device(dev)
>  
> +            if len(ctrls_scsi) > 0:
> +                index_new = max([x.index for x in ctrls_scsi]) + 1
> +
> +            for dev in ctrls_scsi:
> +                if newmodel in scsi_controller_model:
> +                    guest.remove_device(dev)
> +            for dev in ctrls_scsi:
> +                if newmodel == "virtio-scsi":
> +                    dev = VirtualController(guest.conn)
> +                    dev.type = "scsi"
> +                    dev.index = index_new
> +                    if newmodel != "default":
> +                        dev.model = newmodel
> +                    guest.add_device(dev)
> +                    break
> +
>          return self._redefine_device(change, devobj)
>  
>  
> 

This is a little hard to follow. Can you separate things into two functions, like:

def _change_usb(editdev):
   ....
def _change_scsi(editdev):
   ....

def change(editdev):
    if editdev.type == TYPE_USB:
        return _change_usb(editdev)
    elif editdev.type == TYPE_SCSI:
        return _change_scsi(editdev)

Thanks,
Cole