Vered Volansky has posted comments on this change.
Change subject: engine: Support Duplicate Mac Addresses
......................................................................
Patch Set 4: I would prefer that you didn't submit this
(4 inline comments)
You should also test the new functionality.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java
Line 49: import org.ovirt.engine.core.common.businessentities.VmTemplate;
Line 50: import org.ovirt.engine.core.common.businessentities.VmTemplateStatus;
Line 51: import org.ovirt.engine.core.common.businessentities.storage_domains;
Line 52: import org.ovirt.engine.core.common.businessentities.network.Network;
Line 53: import
org.ovirt.engine.core.common.businessentities.network.VmNetworkInterface;
This is the only change in the file - why submit?
Line 54: import org.ovirt.engine.core.common.errors.VdcBLLException;
Line 55: import
org.ovirt.engine.core.common.queries.GetAllFromExportDomainQueryParameters;
Line 56: import
org.ovirt.engine.core.common.queries.GetStorageDomainsByVmTemplateIdQueryParameters;
Line 57: import
org.ovirt.engine.core.common.queries.IsVmWithSameNameExistParameters;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/MacPoolManager.java
Line 198: private boolean otherIfaceWithSameMacExists(VmNetworkInterface
iface) {
Line 199: NGuid ifaceId = iface.getId();
Line 200: if (ifaceId != null) {
Line 201: for (Guid ifaceFromList :
getVmNetworkInterfaceDao().getVmInterfacesByMac(iface.getMacAddress())) {
Line 202: if (!ifaceId.equals(ifaceFromList)) {
You're returning true if they're NOT the same. You'd want to remove the !
Line 203: return true;
Line 204: }
Line 205: }
Line 206: }
Line 230: * @return
Line 231: */
Line 232: public boolean addMac(String mac) {
Line 233: boolean retVal = true;
Line 234: boolean allowDuplicate = getAllowDuplicate();
You'd better just check for getAllowDuplicate directly and return true
immediately, also save the lock that is only left to be unlocked in this
scenario.
Then you can leave the try-catch block untouched, unless you'd like to take
this opportunity and make it nicer.
Line 235: lockObj.writeLock().lock();
Line 236: try {
Line 237: if (allocatedMacs.contains(mac)) {
Line 238: retVal = allowDuplicate ? true : false;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/VmInterfaceManager.java
Line 78: List<VmNetworkInterface> interfaces =
getVmNetworkInterfaceDao().getAllForVm(vmId);
Line 79: if (interfaces != null) {
Line 80: for (VmNetworkInterface iface : interfaces) {
Line 81: getMacPoolManager().freeMac(iface);
Line 82: getVmNetworkInterfaceDao().remove(iface.getId());
If iface can be nul in freeMac it can also be null here. I suggest you move the
check here and remove it from freeMac.
Line 83: getVmNetworkStatisticsDao().remove(iface.getId());
Line 84: }
Line 85: }
Line 86: }
--
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: 4
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
