Lior Vernia has posted comments on this change. Change subject: common: adding maskValidator util ......................................................................
Patch Set 2: (4 comments) http://gerrit.ovirt.org/#/c/35144/2/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/ValidationUtils.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/ValidationUtils.java: Line 28: public static final String CIDR_FORMAT_PATTERN = Line 29: "^\\b((25[0-5]|2[0-4]\\d|[01]\\d\\d|\\d?\\d)\\.){3}(25[0-5]|2[0-4]\\d|[01]\\d\\d|\\d?\\d)(?:/(?:3[0-2]|[12]?[0-9]))$"; Line 30: public static final String ISO_SUFFIX = ".iso"; Line 31: public static final String ISO_SUFFIX_PATTERN = "^$|^.+\\.iso$"; Line 32: public static final String SUBNET_PATTERN_AS_NETMASK = "^([3][0-2]|[0-2][0-9]|[0-9])$"; > Done This still accepts "0" as prefix - have we decided that's a valid netmask? Line 33: public static final String NETMASK_PATTERN = Line 34: "(^\\b((25[0-5]|2[0-4]\\d|[01]\\d\\d|\\d?\\d)\\.){3}(25[0-5]|2[0-4]\\d|[01]\\d\\d|\\d?\\d)\\b$)|(" Line 35: + SUBNET_PATTERN_AS_NETMASK + ")+"; Line 36: http://gerrit.ovirt.org/#/c/35144/2/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/validation/CidrValidator.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/validation/CidrValidator.java: Line 44: int mask = Integer.parseInt(temp[1]); Line 45: return isNetworkAddress(ipAsInteger, mask); Line 46: } Line 47: Line 48: protected static long covnertIpToInt(String ipAdd) { > Done I still don't think this should sit inside a class called "CidrValidator". It has very little to do with the CIDR format, and everything to do with IP address format. Line 49: String[] subAdd = ipAdd.split("\\."); Line 50: long output = 0; Line 51: long temp; Line 52: for (int index = 3; index > -1; index--) { http://gerrit.ovirt.org/#/c/35144/2/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/validation/MaskValidator.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/validation/MaskValidator.java: Line 1: package org.ovirt.engine.core.common.validation; Line 2: Line 3: import org.ovirt.engine.core.common.utils.ValidationUtils; Line 4: Line 5: public class MaskValidator { > we decided that the validation between CIDR and Mask is different Indeed, but the static method in CidrValidator doesn't belong there. I suggest to turn this into an IP address utility class. Line 6: Line 7: private static MaskValidator validator = new MaskValidator(); Line 8: Line 9: private MaskValidator() { Line 35: * @param mask Line 36: * @return true if valid mask ,false otherwise Line 37: */ Line 38: public boolean isMaskValid(String mask) { Line 39: if (!isMaskFormatValid(mask)) { > we need to separate between two cases: This still doesn't explain why you need to check that the format is valid here. This method should be called "isValidNetmask()" and only deal with the actual value. And assume as a pre-requisite that the format is valid (otherwise it is meaningless to call this method). Line 40: return false; Line 41: } Line 42: Line 43: if (mask.matches(ValidationUtils.SUBNET_PATTERN_AS_NETMASK)) { -- To view, visit http://gerrit.ovirt.org/35144 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I04fb0aa2193801688f1bfc5dc7684cabdfa2827d Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Eliraz Levi <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Eliraz Levi <[email protected]> Gerrit-Reviewer: Lior Vernia <[email protected]> Gerrit-Reviewer: Martin Mucha <[email protected]> Gerrit-Reviewer: Yevgeny Zaspitsky <[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
