Mike Kolesnik has posted comments on this change.
Change subject: engine: Support Duplicate Mac Addresses
......................................................................
Patch Set 4: (4 inline comments)
If you do plan on adding methods to the DAO, please add tests too.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/MacPoolManager.java
Line 173: return availableMacs.size();
Line 174: }
Line 175:
Line 176: public void freeMac(VmNetworkInterface iface) {
Line 177: if (iface == null || iface.getMacAddress() == null) {
Why handle this case? looks like a bug if someone sends null in either of these
so i'd expect the method to fail
Line 178: log.infoFormat("MacPoolManager::freeMac(iface or mac =
null, exiting without freeing mac)");
Line 179: return;
Line 180: }
Line 181:
Line 194: lockObj.writeLock().unlock();
Line 195: }
Line 196: }
Line 197:
Line 198: private boolean otherIfaceWithSameMacExists(VmNetworkInterface
iface) {
I think implementation can be simpler..
Instead of querying a list of nics, and also needing to change the interface of
freeMac method to receive interface [1], you can just change the logic so that
you have a "reference count" [2] and when it reaches 0 you free the mac from
memory.
[1] This is wrong by itself, since the manager now becomes aware of this entity
type instead of just mac which is the actual type it manages.
[2] Either in memory, or fetch it from DB is up to you..
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)) {
Line 277: @SuppressWarnings("serial")
Line 278: private class MacPoolExceededMaxException extends
RuntimeException {
Line 279: }
Line 280:
Line 281: private boolean getAllowDuplicate() {
Probably better call it just allowDuplicate since this is not a quite getter..
However, if it were a getter it would start with 'is'.
Line 282: return Config.<Boolean>
GetValue(ConfigValues.AllowDuplicateMacAddresses);
Line 283: }
Line 284:
Line 285: private VmNetworkInterfaceDao getVmNetworkInterfaceDao() {
....................................................
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());
I don't understand your comment.
The DAO method returns a list of interfaces, how can one be null?
Also as I wrote earlier, if something like this happens it's a bug, not
expected behavior.
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
