Mike Kolesnik has posted comments on this change.
Change subject: core: create UpdateVmInterfaceVDSCommand
......................................................................
Patch Set 11: (6 inline comments)
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/UpdateVmInterfaceVDSParameters.java
Line 4: import org.ovirt.engine.core.common.businessentities.VmDevice;
Line 5: import org.ovirt.engine.core.common.businessentities.VmNetworkInterface;
Line 6: import org.ovirt.engine.core.compat.Guid;
Line 7:
Line 8: public class UpdateVmInterfaceVDSParameters extends
VmUpdateDeviceVDSParameters {
I would prefer that you didn't add a new parameters class.
Instead, you could rename HotPlugUnplgNicVDSParameters (to something like
VmNicDeviceVDSParameters) and reuse it.
Line 9:
Line 10: private final VmNetworkInterface nic;
Line 11:
Line 12: public UpdateVmInterfaceVDSParameters(Guid vdsId, VM vm,
VmNetworkInterface nic, VmDevice vmDevice) {
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/VmUpdateDeviceVDSParameters.java
Line 3: import org.ovirt.engine.core.common.businessentities.VM;
Line 4: import org.ovirt.engine.core.common.businessentities.VmDevice;
Line 5: import org.ovirt.engine.core.compat.Guid;
Line 6:
Line 7: public class VmUpdateDeviceVDSParameters extends
VdsIdVDSCommandParametersBase {
Why split this class?
Line 8:
Line 9: private final VM vm;
Line 10: private final VmDevice vmDevice;
Line 11:
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/UpdateVmInterfaceVDSCommand.java
Line 16: protected void initDeviceStructure() {
Line 17: super.initDeviceStructure();
Line 18: VmNetworkInterface nic = getParameters().getNic();
Line 19:
Line 20: if (nic.getNetworkName() != null) {
This could be inlined as:
deviceStruct.add(VdsProperties.network,
StringUtils.defaultString(nic.getNetworkName()));
Line 21: deviceStruct.add(VdsProperties.network,
nic.getNetworkName());
Line 22: } else {
Line 23: deviceStruct.add(VdsProperties.network, "");
Line 24: }
Line 25:
Line 26: deviceStruct.add(VdsProperties.linkActive,
String.valueOf(nic.isLinked()));
Line 27:
Line 28: if (nic.isPortMirroring() && nic.getNetworkName() != null) {
Line 29: deviceStruct.add(VdsProperties.portMirroring,
Collections.singletonList(nic.getNetworkName()));
This could be inlined as:
if (nic.isPortMirroring()) {
deviceStruct.add(VdsProperties.portMirroring, nic.getNetworkName() == null
? Collections.emptyList() : Collections.singletonList(nic.getNetworkNa
me()));
}
Line 30: } else {
Line 31: deviceStruct.add(VdsProperties.portMirroring, new
ArrayList<String>());
Line 32: }
Line 33: }
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VdsProperties.java
Line 207: public static final String Custom = "custom";
Line 208: public static final String Type = "type";
Line 209: public static final String DeviceId = "deviceId";
Line 210: public static final String Device = "device";
Line 211: public static final String DeviceType = "deviceType";
I don't understand why VDSM API didn't use "type" property that they already
defined.
Line 212: public static final String Devices = "devices";
Line 213: public static final String Index = "index";
Line 214: public static final String Iface = "iface";
Line 215: public static final String PoolId = "poolID";
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VmUpdateDeviceVDSCommand.java
Line 2:
Line 3: import
org.ovirt.engine.core.common.vdscommands.VmUpdateDeviceVDSParameters;
Line 4: import org.ovirt.engine.core.vdsbroker.xmlrpc.XmlRpcStruct;
Line 5:
Line 6: public abstract class VmUpdateDeviceVDSCommand<P extends
VmUpdateDeviceVDSParameters> extends VdsBrokerCommand<P> {
Why split this class?
Only your class using it, I see no need to split this off if so.
Line 7:
Line 8: protected XmlRpcStruct deviceStruct = new XmlRpcStruct();
Line 9:
Line 10: public VmUpdateDeviceVDSCommand(P parameters) {
--
To view, visit http://gerrit.ovirt.org/9669
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Idc54fedde0fdf6ea9694fc19977bf1178e73efdf
Gerrit-PatchSet: 11
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alona Kaplan <[email protected]>
Gerrit-Reviewer: Alona Kaplan <[email protected]>
Gerrit-Reviewer: Mike Kolesnik <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
