Hi Thanks for the review. I have posted v2 patch a few days ago: https://www.redhat.com/archives/virt-tools-list/2012-November/msg00157.html But lots of codes are same as v1. So I answer some of your comments inline. > -----Original Message----- > From: Cole Robinson [mailto:crobinso@xxxxxxxxxx] > Sent: Tuesday, November 27, 2012 11:52 PM > To: Chen Hanxiao > Cc: virt-tools-list@xxxxxxxxxx > Subject: Re: [virt-manager] Add virtio-scsi disk bus option > > Hi Chen, thanks for the patch! Sorry for the review delay, comments inline. > > On 11/12/2012 03:12 AM, Chen Hanxiao wrote: > > From: ChenHanxiao <chenhanxiao@xxxxxxxxxxxxxx> > > > > This patch will add virtio-scsi bus option on "Add New Virtual > > Hardware" GUI page. It will support users to add a virtual disk > > using SCSI bus with a controller model virtio-scsi. > > In virt-manager, we will provide virtio-scsi disk XML for libvirt: > > > > <disk type='block' device='disk'> > > <driver name='qemu'/> > > <source dev='/dev/sdb'/> > > <target dev='sda' bus='scsi' model='virtio-scsi'/> > > </disk> > > > > We provided tag 'model' for libvirt to determine which SCSI > > controller model to use. > > > > Signed-off-by: ChenHanxiao <chenhanxiao@xxxxxxxxxxxxxx> > > --- > > src/virtManager/addhardware.py | 7 +++++++ > > src/virtManager/details.py | 6 +++++- > > 2 files changed, 12 insertions(+), 1 deletions(-) > > > > diff --git a/src/virtManager/addhardware.py > b/src/virtManager/addhardware.py > > index 4d894eb..9a7602c 100644 > > --- a/src/virtManager/addhardware.py > > +++ b/src/virtManager/addhardware.py > > @@ -539,6 +539,8 @@ class vmmAddHardware(vmmGObjectUI): > > if self.vm.get_hv_type() == "kvm": > > add_dev("sata", virtinst.VirtualDisk.DEVICE_DISK, "SATA disk") > > add_dev("virtio", virtinst.VirtualDisk.DEVICE_DISK, "Virtio > disk") > > + add_dev("virtio-scsi", virtinst.VirtualDisk.DEVICE_DISK, > > + "Virtio-scsi disk") > > That last string is a disk label, so please make it something like _('Virtio > SCSI disk') Will change in next version. > > > if self.conn.is_xen(): > > add_dev("xen", virtinst.VirtualDisk.DEVICE_DISK, "Virtual > disk") > > > > @@ -1222,6 +1224,10 @@ class vmmAddHardware(vmmGObjectUI): > > bus, device = self.get_config_disk_target() > > cache = self.get_config_disk_cache() > > fmt = self.get_config_disk_format() > > + model = None > > + if bus == "virtio-scsi": > > + bus = "scsi" > > + model = "virtio-scsi" > > > > # Make sure default pool is running > > if self.is_default_storage(): > > @@ -1271,6 +1277,7 @@ class vmmAddHardware(vmmGObjectUI): > > device=device, > > bus=bus, > > driverCache=cache, > > + model=model, > > format=fmt) > > > > if not fmt: > > diff --git a/src/virtManager/details.py b/src/virtManager/details.py > > index d20e748..0e86648 100644 > > --- a/src/virtManager/details.py > > +++ b/src/virtManager/details.py > > @@ -2219,7 +2219,11 @@ class vmmDetails(vmmGObjectUI): > > if bus == "spapr-vscsi": > > bus = "scsi" > > addr = "spapr-vio" > > - add_define(self.vm.define_disk_bus, dev_id_info, bus, addr) > > + if bus == "virtio-scsi": > > + bus = "scsi" > > + model = "virtio-scsi" > > + add_define(self.vm.define_disk_bus, dev_id_info, bus, > > + addr, model) > > > > return self._change_config_helper(df, da, hf, ha) > > > > > > Does this actually work? By my reading this breaks all disk adding because > model is only defined for bus == "virtio-scsi", and define_disk_bus in > domain.py wasn't changed to accept the model parameter. > In this version, I leave this job of adding controllers to libvirt (with new patchs). But Daniel says apps should add it explicitly. So at v2: https://www.redhat.com/archives/virt-tools-list/2012-November/msg00157.html I use model parameter just as helper to add controllers needed by virtio-disks and It could be work without modifying current libvirt. > An easy way to test all this stuff is to do > > virt-manager --connect test:///default V2 looks fine with this command without changing domain.py. > > That uses the libvirt stub driver which will allow you to add all manner of > devices to a fake guest and nothing will affect the local machine. > > I've added a few commits to help with that, so make sure you rebase your > changes on latest virt-manager git. > > - Cole Regards. _______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
