> On Aug. 3, 2012, 10:03 p.m., Chiradeep Vittal wrote: > > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVifDriver.java, > > line 35 > > <https://reviews.apache.org/r/6285/diff/3/?file=133630#file133630line35> > > > > This should be an Interface rather than an abstract class > > The interface should belong in com.cloud.resource in core and should be > > called VifDriver. You can have a subclass that is abstract (VifDriverBase) > > as well. The constructor should not throw exceptions, instead you should > > have an abstract configure() method that throws a ConfigurationException > > Tomoe Sugihara wrote: > Thanks for the review. Sure, I can do that. > Note that VifDriver will be hypervisor specific (signature differes), so > maybe I should create a interface com.cloud.hypervisor.kvm.VifDriver for KVM, > com.cloud.hypervisor.xen.VifDriver for Xen. What do you think?
<chiradeep> Hopefully you can make it more generic than that. It seems that the main problem is the plug method. If not, that is OK </chiradeep> I agree, hopefully we can make it more generic. For now, I'll create a interface at /plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/VifDriver.java. On KVM, different vif implementation creates different Libvirt xml, so the simplest way is that plug() method return LibvirtVMDef.InterfaceDef, which isn't/shouldn't be in core module. And this is not the case on other hypervisors, meaning VifDrive is hypervisor specific. - Tomoe ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6285/#review9837 ----------------------------------------------------------- On Aug. 2, 2012, 11:18 p.m., Tomoe Sugihara wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6285/ > ----------------------------------------------------------- > > (Updated Aug. 2, 2012, 11:18 p.m.) > > > Review request for cloudstack, edison su and Chiradeep Vittal. > > > Description > ------- > > Remove methods that are no longer needed in LibvirtComputingResource.java > > Signed-off-by: Tomoe Sugihara <to...@midokura.com> > > Add unplug() hook > > Signed-off-by: Tomoe Sugihara <to...@midokura.com> > > Add vif driver support > > - define abstract class named LibvirtVifDriver > - add LibvirtBridgeVifDriver to support current network implementation > - unplug() will be added > > Signed-off-by: Tomoe Sugihara <to...@midokura.com> > > > Diffs > ----- > > > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtBridgeVifDriver.java > PRE-CREATION > > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java > 73101a9896c4863bc75502ff8b0b6daceff52dc4 > > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVifDriver.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/6285/diff/ > > > Testing > ------- > > Manually tested launching VM on all-in-one KVM box with Advanced networking. > > > Thanks, > > Tomoe Sugihara > >
