Alona Kaplan has posted comments on this change. Change subject: core: refactored HostSetupNetworksValidator to use HostInterfaceValidator ......................................................................
Patch Set 32: Code-Review+2 (7 comments) https://gerrit.ovirt.org/#/c/35978/32/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksValidator.java: Line 237: candidateAttachments.addAll(newAttachments); Line 238: return candidateAttachments; Line 239: } Line 240: Line 241: private boolean validModifiedBonds() { s/validModifiedBonds/validModifiedOrNewBonds Line 242: boolean passed = true; Line 243: for (Bond modifiedOrNewBond : params.getBonds()) { Line 244: String bondName = modifiedOrNewBond.getName(); Line 245: Line 255: continue; Line 256: } Line 257: Line 258: //count of bond slaves must be at least two. Line 259: if (modifiedOrNewBond.getSlaves().size() < 2) { Why don't you use the HostInterfaceValidator.isValidBond validation? Line 260: addViolation(VdcBllMessages.NETWORK_BONDS_INVALID_SLAVE_COUNT, bondName); Line 261: passed = false; Line 262: continue; Line 263: } Line 286: Why did you add the spaces? Line 283: /* we're creating new bond, and it's definition contains reference to slave already assigned Line 284: to a different bond. */ Line 285: (!slaveBondName.equals(bondName) Line 286: //…but this bond is also removed in this request, so it's ok. Line 287: || !slaveUsedByRemovedBond(slaveBondName))) { Why not allowing move slave form one bond to another (just make sure it was actually removed from the previous bond)? Line 288: addViolation(VdcBllMessages.NETWORK_INTERFACE_ALREADY_IN_BOND, slaveName); Line 289: passed = false; Line 290: continue; Line 291: } Line 290: continue; Line 291: } Line 292: Line 293: /* slave has network assigned and there isn't request for unassigning it; Line 294: so this probably check, that nic is part of newly crated bond, and any previously attached network has Please remove the word 'probably' from the comment. Line 295: to be unattached. */ Line 296: if (slave.getNetworkName() != null && !nicUsedByRemovedNetwork(slave)) { Line 297: addViolation(VdcBllMessages.NETWORK_INTERFACE_ATTACHED_TO_NETWORK_CANNOT_BE_SLAVE, slave.getName()); Line 298: passed = false; Line 292: Line 293: /* slave has network assigned and there isn't request for unassigning it; Line 294: so this probably check, that nic is part of newly crated bond, and any previously attached network has Line 295: to be unattached. */ Line 296: if (slave.getNetworkName() != null && !nicUsedByRemovedNetwork(slave)) { 1. What if the slave has vlan network attached to it? slave.getNetworkName() != null will return true, you have to check all the vlan devices of the slave. 2. If you add a slave to a bond and move it's network to another nic it should be valid, isn't it? (Your code here will block it. It refers to a question I asked you in a previous patch- how do you move a network from nic to nic- removing the old attachment and creating a new one or just changing the nic on the attachment). Line 297: addViolation(VdcBllMessages.NETWORK_INTERFACE_ATTACHED_TO_NETWORK_CANNOT_BE_SLAVE, slave.getName()); Line 298: passed = false; Line 299: continue; Line 300: } https://gerrit.ovirt.org/#/c/35978/32/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/HostInterfaceValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/HostInterfaceValidator.java: Line 43: interfaceIsBond please rename to interfaceIsBondOrNull -- To view, visit https://gerrit.ovirt.org/35978 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I070bb5c75d635d28911f7f367051c71dbd0127d2 Gerrit-PatchSet: 32 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
