Moti Asayag has posted comments on this change. Change subject: tools: CidrValidation Utils ......................................................................
Patch Set 1: Code-Review-1 (10 comments) nice work! http://gerrit.ovirt.org/#/c/32539/1/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 23: public static final String NO_SPECIAL_CHARACTERS_WITH_DOT = "[0-9a-zA-Z-_\\.]+"; Line 24: public static final String NO_TRIMMING_WHITE_SPACES_PATTERN = "^$|\\S.*\\S"; Line 25: public static final String IP_PATTERN = Line 26: "^\\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 27: //TODO elevi consider name - could be miss leading why miss leading ? 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$"; http://gerrit.ovirt.org/#/c/32539/1/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 2: Line 3: import org.ovirt.engine.core.common.utils.ValidationUtils; Line 4: Line 5: public class CidrValidator { Line 6: private static String getCidrRegex() { why do you need this method instead of accessing ValidationUtils.CIDR_FORMAT_PATTERN directly ? Line 7: return ValidationUtils.CIDR_FORMAT_PATTERN; Line 8: } Line 9: Line 10: /*** Line 6: private static String getCidrRegex() { Line 7: return ValidationUtils.CIDR_FORMAT_PATTERN; Line 8: } Line 9: Line 10: /*** javadoc can be formatted to ease the reading, for example: /*** * Check if CIDR is in correct format: x.x.x.x/y where: * <ul> * <li>x belongs to [0,255] * <li>y belongs to [0,32] * <li>both inclusive * </ul> * <p> * <b>Note!</b> the function is not validating that IP and mask match to a network address, please see @see * {@link CidrValidator#isCidrFormatValid(String)} * @param cidr * @return true if correct format, false otherwise. */ Line 11: * check if CIDR is in correct format - x.x.x.x/y where: x belongs to [0,255] y belongs to [0,32] both inclusive Line 12: * note! the function is not validating that ip and mask match to a network address, please see @see Line 13: * {@link CidrValidator#isCidrFormatValid(String)} Line 14: * @param cidr Line 14: * @param cidr Line 15: * @return true if correct format, false otherwise. Line 16: */ Line 17: public static boolean isCidrFormatValid(String cidr) { Line 18: return (cidr == null || !cidr.matches(getCidrRegex())) ? doesn't it mean that a user which sends an empty cidr (i.e. wasn't supplied in the parameters) will fail with this validation ? Line 19: false : true; Line 20: } Line 21: Line 22: /*** Line 25: * - in correct format, please use the following function first: @see Line 26: * {@link CidrValidator#isCidrFormatValid(String)} Line 27: * @return true if valid CIDR ,false otherwise Line 28: */ Line 29: public static boolean isCidrValidNetworkAddress(String cidr) { maybe the name should be isCidrNetworkAddressValid ? Line 30: String ipAsString = getIpFromCidr(cidr); Line 31: int ipAsInteger = covnertIpToInt(ipAsString); Line 32: int mask = getMaskFromCidr(cidr); Line 33: return isNetworkAddress(ipAsInteger, mask); Line 34: } Line 35: Line 36: private static int getMaskFromCidr(String cidr) { Line 37: String[] temp = cidr.split("/"); Line 38: return Integer.parseInt(temp[1]); why there is needs to parse the string twice ? here and in getIpFromCidr ? Line 39: } Line 40: Line 41: private static String getIpFromCidr(String cidr) { Line 42: String[] temp = cidr.split("/"); Line 50: for (int index = 3; index > -1; index--) { Line 51: temp = Integer.parseInt(subAdd[3 - index]); Line 52: temp <<= (index * 8); Line 53: output |= temp; Line 54: } please add a space line after a closing curly bracket Line 55: return output; Line 56: } Line 57: Line 58: private static boolean isNetworkAddress(int ip, int mask) { Line 58: private static boolean isNetworkAddress(int ip, int mask) { Line 59: int temp = 1; Line 60: int check = 0; Line 61: for (int i = 0; i < mask; i++) { Line 62: check = temp & ip; wouldn't you can avoid 'check' variable here ? if (temp & ip) { return false; } temp <<= 1; ... Line 63: if (check != 0) { Line 64: return false; Line 65: } Line 66: temp <<= 1; Line 61: for (int i = 0; i < mask; i++) { Line 62: check = temp & ip; Line 63: if (check != 0) { Line 64: return false; Line 65: } same about space line Line 66: temp <<= 1; Line 67: } Line 68: return true; Line 69: } http://gerrit.ovirt.org/#/c/32539/1/backend/manager/modules/common/src/test/java/org/ovirt/engine/core/common/validation/CidrValidatorTest.java File backend/manager/modules/common/src/test/java/org/ovirt/engine/core/common/validation/CidrValidatorTest.java: Line 21: } Line 22: Line 23: @Test Line 24: public void checkCidrValidation() { Line 25: boolean result = CidrValidator.isCidrFormatValid(cidr); why not to separate into two tests ? Line 26: if (result) { Line 27: assertEquals(expectedResult, CidrValidator.isCidrValidNetworkAddress(cidr)); Line 28: } Line 29: else { -- To view, visit http://gerrit.ovirt.org/32539 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib1277dbc815953926fe1648350cd55cb75e1084a Gerrit-PatchSet: 1 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: 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
