Moti Asayag has posted comments on this change.

Change subject: engine: vmDevice of 'passthrough' vnic should be 'hostdev' not 
'bridge'
......................................................................


Patch Set 4: Code-Review-1

(1 comment)

https://gerrit.ovirt.org/#/c/38260/4/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ovf/OvfReader.java
File 
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ovf/OvfReader.java:

Line 902:             }
Line 903:         } else if (Integer.valueOf(OvfHardware.Network) == 
resourceType) {
Line 904:             // handle interfaces with different sub types : we have 
0-3 as the VmInterfaceType enum
Line 905:             boolean isKnownType = false;
Line 906:             for (VmInterfaceType vmInterfaceType : 
VmInterfaceType.values()) {
there are so many wrongs in this for loop, which aren't related to this patch 
specifically, but please fix them before you inherit your code:

 * The loop iterates over the enum values, so search for a match for the given 
resourceSubType (type int). This is exactly what VmInterfaceType.forValue(int) 
does

 * Second, there is this code:
  Integer.valueOf(vmInterfaceType.getValue())
  which basically wraps an int value with an integer, just to unbox it back 
into an int for the sake of comparison.

so the entire for loop should be simplified to:
  VmInterfaceType nicType = VmInterfaceType.forValue(resourceSubType);
            if (nicType != null) {
    vmDevice.setDevice(VmDeviceType.BRIDGE.getName());
    vmDevice.setDevice(VmDeviceType.getoVirtDevice(resourceType).getName());
  }

which makes the isKnownType useless.

You can either send a patch specifically for that, or modify the current patch 
to include the above simplification.
Line 907:                 if (Integer.valueOf(vmInterfaceType.getValue()) == 
resourceSubType) {
Line 908:                     if (VmInterfaceType.pciPassthrough.getValue() == 
resourceSubType) {
Line 909:                         
vmDevice.setDevice(VmDeviceType.HOST_DEVICE.getName());
Line 910:                     } else {


-- 
To view, visit https://gerrit.ovirt.org/38260
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8c9739695887570d2b032b93af4d5835a35a0b88
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alona Kaplan <[email protected]>
Gerrit-Reviewer: Moti Asayag <[email protected]>
Gerrit-Reviewer: [email protected]
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to