Arik Hadas has posted comments on this change. Change subject: core: Add CRUD commands and queries for QoS objects (storage) ......................................................................
Patch Set 20: (5 comments) http://gerrit.ovirt.org/#/c/27096/20/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/qos/RemoveQosCommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/qos/RemoveQosCommandBase.java: Line 16: protected boolean canDoAction() { Line 17: if (validateParameters()) { Line 18: QosValidator<T> validator = getQosValidator(getQos()); Line 19: return super.canDoAction() && validate(validator.qosExists()); Line 20: } if validateParameters returns false, the result will be true.. Line 21: return true; Line 22: } Line 23: Line 24: @Override Line 26: if (getQosId() == null) { Line 27: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_QOS_NOT_FOUND); Line 28: } Line 29: return true; Line 30: } why do we need to override this method? Line 31: Line 32: @Override Line 33: protected void executeCommand() { Line 34: getQosDao().remove(getQos().getId()); Line 30: } Line 31: Line 32: @Override Line 33: protected void executeCommand() { Line 34: getQosDao().remove(getQos().getId()); if you keep validateParameters as is, I think there might be NPE here (getQos() == null) Line 35: getReturnValue().setActionReturnValue(getQos().getId()); Line 36: setSucceeded(true); Line 37: } Line 38: http://gerrit.ovirt.org/#/c/27096/20/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/StorageQosValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/StorageQosValidator.java: Line 38: } Line 39: Line 40: private boolean missingCategoryValues(Integer total, Integer read, Integer write) { Line 41: return (isPositive(total) && (isPositive(read) || isPositive(write))) Line 42: || (isPositive(total) && (isPositive(read) || isPositive(write))); you check the same thing twice.. Line 43: } Line 44: Line 45: private boolean isPositive(Integer value) { Line 46: if (value == null || value <= 0) { Line 45: private boolean isPositive(Integer value) { Line 46: if (value == null || value <= 0) { Line 47: return false; Line 48: } Line 49: return true; how about changing it to: return value != null && value > 0 Line 50: } -- To view, visit http://gerrit.ovirt.org/27096 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1a9af59277b5055453159f002f19046c00522ddb Gerrit-PatchSet: 20 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Arik Hadas <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Doron Fediuck <[email protected]> Gerrit-Reviewer: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Kobi Ianko <[email protected]> Gerrit-Reviewer: Liron Aravot <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
