Mike Kolesnik has posted comments on this change.
Change subject: engine: Support Duplicate Mac Addresses
......................................................................
Patch Set 5: (13 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/MacPoolManager.java
Line 29: private static final MacPoolManager INSTANCE = new
MacPoolManager();
Line 30:
Line 31: private static Log log = LogFactory.getLog(MacPoolManager.class);
Line 32:
Line 33: // Maps that hold the allocated Mac addresses as keys, and
counters as values
Can you please do this as javadoc for each of the fields?
Line 34: private final Map<String, Integer> allocatedMacs = new
HashMap<String, Integer>();
Line 35: private final Map<String, Integer> allocatedCustomMacs = new
HashMap<String, Integer>();
Line 36:
Line 37: private final Set<String> availableMacs = new HashSet<String>();
Line 196: AuditLogDirector.log(logable,
AuditLogType.MAC_ADDRESSES_POOL_NOT_INITIALIZED);
Line 197: }
Line 198:
Line 199: private void internalFreeMac(String mac) {
Line 200: if (allocatedMacs.get(mac) != null) {
Why not check if contains key?
Line 201: removeMacFromMap(allocatedMacs, mac);
Line 202: availableMacs.add(mac);
Line 203: } else if (allocatedCustomMacs.get(mac) != null) {
Line 204: removeMacFromMap(allocatedCustomMacs, mac);
Line 199: private void internalFreeMac(String mac) {
Line 200: if (allocatedMacs.get(mac) != null) {
Line 201: removeMacFromMap(allocatedMacs, mac);
Line 202: availableMacs.add(mac);
Line 203: } else if (allocatedCustomMacs.get(mac) != null) {
Same question
Line 204: removeMacFromMap(allocatedCustomMacs, mac);
Line 205: }
Line 206: }
Line 207:
Line 264: @SuppressWarnings("serial")
Line 265: private class MacPoolExceededMaxException extends
RuntimeException {
Line 266: }
Line 267:
Line 268: private boolean allowDuplicate() {
I wouldn't put this in a separate method since it's only used in 1 place and
the logic doesn't merit it's own method.
Line 269: return Config.<Boolean>
GetValue(ConfigValues.AllowDuplicateMacAddresses);
Line 270: }
Line 271:
Line 272: private VmNetworkInterfaceDao getVmNetworkInterfaceDao() {
Line 268: private boolean allowDuplicate() {
Line 269: return Config.<Boolean>
GetValue(ConfigValues.AllowDuplicateMacAddresses);
Line 270: }
Line 271:
Line 272: private VmNetworkInterfaceDao getVmNetworkInterfaceDao() {
I wouldn't put this in a separate method since it's only used in 1 place and
the logic doesn't merit it's own method.
Unless you're doing this for tests..
Line 273: return DbFacade.getInstance().getVmNetworkInterfaceDao();
Line 274: }
Line 275:
Line 276: /*
Line 274: }
Line 275:
Line 276: /*
Line 277: * Adds the given Mac to the given Map, either by putting it in
the Map, or incrementing its counter. If Mac is
Line 278: * already taken and the ConfigValue AllowDuplicateMacAddresses
is false, return false and does not add Mac to map
Please document parameters & return value.
Additionally you can use {@link ConfigValue#AllowDuplicateMacAddresses} which
is a more robust way to document types/methods/etc
Line 279: */
Line 280: private boolean addMacToMap(Map<String, Integer> macMap, String
mac) {
Line 281: boolean returnValue = false;
Line 282: if (!macMap.containsKey(mac)) {
Line 277: * Adds the given Mac to the given Map, either by putting it in
the Map, or incrementing its counter. If Mac is
Line 278: * already taken and the ConfigValue AllowDuplicateMacAddresses
is false, return false and does not add Mac to map
Line 279: */
Line 280: private boolean addMacToMap(Map<String, Integer> macMap, String
mac) {
Line 281: boolean returnValue = false;
I wouldn't use a variable for this but just return on the spot, this method is
not that complicated that you would need a variable to track it's return value.
Line 282: if (!macMap.containsKey(mac)) {
Line 283: macMap.put(mac, 1); // First time this mac is used
Line 284: returnValue = true;
Line 285: } else if (allowDuplicate()) {
Line 279: */
Line 280: private boolean addMacToMap(Map<String, Integer> macMap, String
mac) {
Line 281: boolean returnValue = false;
Line 282: if (!macMap.containsKey(mac)) {
Line 283: macMap.put(mac, 1); // First time this mac is used
I don't think there's need for a comment here, it's pretty obvious
Line 284: returnValue = true;
Line 285: } else if (allowDuplicate()) {
Line 286: incrementMacInMap(macMap, mac);
Line 287: returnValue = true;
Line 288: }
Line 289: return returnValue;
Line 290: }
Line 291:
Line 292: private void incrementMacInMap(Map<String, Integer> macMap,
String mac) {
I wouldn't put this in a separate method since it's only used in 1 place and
the logic doesn't merit it's own method.
Line 293: macMap.put(mac, macMap.get(mac) + 1);
Line 294: }
Line 295:
Line 296: private void decrementMacInMap(Map<String, Integer> macMap,
String mac) {
Line 292: private void incrementMacInMap(Map<String, Integer> macMap,
String mac) {
Line 293: macMap.put(mac, macMap.get(mac) + 1);
Line 294: }
Line 295:
Line 296: private void decrementMacInMap(Map<String, Integer> macMap,
String mac) {
Same here
Line 297: macMap.put(mac, macMap.get(mac) - 1);
Line 298: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/vm/AddVmInterfaceCommand.java
Line 84: }
Line 85: } finally {
Line 86: setSucceeded(succeeded);
Line 87: if (!succeeded) {
Line 88:
MacPoolManager.getInstance().freeMac(getParameters().getInterface().getMacAddress());
Why this change?
Line 89: }
Line 90: }
Line 91: }
Line 92:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/vm/UpdateVmInterfaceCommand.java
Line 108: } finally {
Line 109: setSucceeded(succeeded);
Line 110: if (!succeeded && macAddressChanged) {
Line 111:
MacPoolManager.getInstance().addMac(oldIface.getMacAddress());
Line 112:
MacPoolManager.getInstance().freeMac(getInterface().getMacAddress());
getMacAddress() -> getInterface().getMacAddress()
Why this change?
Line 113: }
Line 114: }
Line 115: }
Line 116:
Line 155: public void rollback() {
Line 156: super.rollback();
Line 157: if (macAddressChanged) {
Line 158:
MacPoolManager.getInstance().addMac(oldIface.getMacAddress());
Line 159:
MacPoolManager.getInstance().freeMac(getInterface().getMacAddress());
getMacAddress() -> getInterface().getMacAddress()
Why this change?
Line 160: }
Line 161: }
Line 162:
Line 163: @Override
--
To view, visit http://gerrit.ovirt.org/10698
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie96eda19de2d3a44e24806095fb690e4eba41165
Gerrit-PatchSet: 5
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Muli Salem <msa...@redhat.com>
Gerrit-Reviewer: Mike Kolesnik <mkole...@redhat.com>
Gerrit-Reviewer: Moti Asayag <masa...@redhat.com>
Gerrit-Reviewer: Muli Salem <msa...@redhat.com>
Gerrit-Reviewer: Vered Volansky <vvola...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
