Martin Mucha has posted comments on this change. Change subject: engine: Dual mode shouldn't be sent to vdsm on hotplug nic ......................................................................
Patch Set 7: (3 comments) I think it's ok from functional perspective, yet VmInfoBuilder can be made more readable. http://gerrit.ovirt.org/#/c/26380/7/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VmInfoBuilder.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VmInfoBuilder.java: Line 344: } Line 345: Line 346: ArchStrategyFactory.getStrategy(vm.getClusterArch()).run(new CreateAdditionalControllers(devices)); Line 347: } Line 348: I think this method is overgrown. * devicesByDeviceId map should be created in separate method * each vmInterface in for loop should be processed in separate method * ifaceType has 'default value' (value from enum) for no reason -- I'd like to see method which returns ifaceType which should be used. Line 349: @Override Line 350: protected void buildVmNetworkInterfaces() { Line 351: Map<VmDeviceId, VmDevice> devicesByDeviceId = Line 352: Entities.businessEntitiesById(DbFacade.getInstance() Line 367: Line 368: if (vmInterface.getType() != null) { Line 369: ifaceType = VmInterfaceType.forValue(vmInterface.getType()); Line 370: } Line 371: this method should create 'struct' itself and return it as a result Line 372: addNetworkInterfaceProperties(struct, Line 373: vmInterface, Line 374: vmDevice, Line 375: VmInfoBuilder.evaluateInterfaceType(ifaceType, vm.getHasAgent()), Line 378: addToManagedDevices(vmDevice); Line 379: } Line 380: } Line 381: } Line 382: nested ternaries are not very readable. Line 383: public static String evaluateInterfaceType(VmInterfaceType ifaceType, boolean vmHasAgent) { Line 384: return ifaceType == VmInterfaceType.rtl8139_pv Line 385: ? (vmHasAgent ? VmInterfaceType.pv.name() : VmInterfaceType.rtl8139.name()) Line 386: : ifaceType.getInternalName(); -- To view, visit http://gerrit.ovirt.org/26380 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifdc878a5f8391659a58cf25f2f9137c944689ce4 Gerrit-PatchSet: 7 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Yevgeny Zaspitsky <yzasp...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
