On 01/20/2014 07:15 AM, Chen Hanxiao wrote:
> From: Chen Hanxiao <chenhanxiao@xxxxxxxxxxxxxx>
>
> We could modify scsi controller model
> between 'default' and 'virtio-scsi'.
>
> Signed-off-by: Chen Hanxiao <chenhanxiao@xxxxxxxxxxxxxx>
> ---
> 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
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
