Lior Vernia has posted comments on this change. Change subject: common: adding maskValidator util ......................................................................
Patch Set 7: (21 comments) http://gerrit.ovirt.org/#/c/35144/7/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/IpAddressConverter.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/IpAddressConverter.java: Line 2: Line 3: Line 4: public class IpAddressConverter { Line 5: Line 6: private static IpAddressConverter ipAddConverter = new IpAddressConverter (); Is this formatted? There are multiple space characters here. Line 7: Line 8: private IpAddressConverter (){ Line 9: } Line 10: Line 4: public class IpAddressConverter { Line 5: Line 6: private static IpAddressConverter ipAddConverter = new IpAddressConverter (); Line 7: Line 8: private IpAddressConverter (){ And here one is missing. Line 9: } Line 10: Line 11: public static IpAddressConverter getInstance(){ Line 12: return ipAddConverter; Line 7: Line 8: private IpAddressConverter (){ Line 9: } Line 10: Line 11: public static IpAddressConverter getInstance(){ And here again too many. Line 12: return ipAddConverter; Line 13: } Line 14: Line 15: /*** Line 21: * @param ipV4Add Line 22: * in valid x1.x2.x3.x4 format Line 23: * @return the IPv4 value, represented as long, where x4 is the LSB of the returned long Line 24: */ Line 25: public long covnertIpToLong(String ipV4Add) { Typo: covnert? Line 26: String[] subAdd = ipV4Add.split("\\."); Line 27: long output = 0; Line 28: long temp; Line 29: for (int index = 3; index > -1; index--) { Line 22: * in valid x1.x2.x3.x4 format Line 23: * @return the IPv4 value, represented as long, where x4 is the LSB of the returned long Line 24: */ Line 25: public long covnertIpToLong(String ipV4Add) { Line 26: String[] subAdd = ipV4Add.split("\\."); Please don't use shorthand naming, it's not clear what "subAdd" means. If it's supposed to be "subnetAddress", then that's not an appropriate name either - it doesn't really describe what the variable holds. I would call it "octets" or "octateArray". Line 27: long output = 0; Line 28: long temp; Line 29: for (int index = 3; index > -1; index--) { Line 30: temp = Integer.parseInt(subAdd[3 - index]); Line 25: public long covnertIpToLong(String ipV4Add) { Line 26: String[] subAdd = ipV4Add.split("\\."); Line 27: long output = 0; Line 28: long temp; Line 29: for (int index = 3; index > -1; index--) { This could be made more elegant. I think the following code would do the same: for (String octet : subAdd) { output = 256 * output + Integer.parseInt(octet); } What do you think? Line 30: temp = Integer.parseInt(subAdd[3 - index]); Line 31: temp <<= (index * 8); Line 32: output |= temp; Line 33: } http://gerrit.ovirt.org/#/c/35144/7/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 22: public static final String ONLY_I18N_ASCII_OR_NONE = "[\\p{ASCII}\\p{L}]*"; Line 23: public static final String ONLY_ASCII_OR_NONE = "[\\p{ASCII}]*"; Line 24: public static final String NO_SPECIAL_CHARACTERS_WITH_DOT = "[0-9a-zA-Z-_\\.]+"; Line 25: public static final String NO_TRIMMING_WHITE_SPACES_PATTERN = "^$|\\S.*\\S"; Line 26: // TODO elevi "" return true for this/.. ask if the one did it, wanted this functionalty What is this comment about? Please make sure you understand what you need and remove it. Line 27: public static final String IP_PATTERN = Line 28: "^\\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 29: public static final String CIDR_FORMAT_PATTERN = Line 30: "^\\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 25: public static final String NO_TRIMMING_WHITE_SPACES_PATTERN = "^$|\\S.*\\S"; Line 26: // TODO elevi "" return true for this/.. ask if the one did it, wanted this functionalty Line 27: public static final String IP_PATTERN = Line 28: "^\\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 29: public static final String CIDR_FORMAT_PATTERN = Now that you have the separate variables, this should just be IP_PATTERN_NON_EMPTY + SUBNET_PREFIX_PATTERN (up to the '^' and '$' characters - perhaps you could leave them out of the pattern and concatenate them as appropriate in using classes). Line 30: "^\\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 31: public static final String ISO_SUFFIX = ".iso"; Line 32: public static final String ISO_SUFFIX_PATTERN = "^$|^.+\\.iso$"; Line 33: public static final String SUBNET_PATTERN_AS_NETMASK_WITH_OPTIONAL_PREFIX_SLASH = "^(?:/)?(3[0-2]|[0-2]\\d|\\d)$"; Line 29: public static final String CIDR_FORMAT_PATTERN = Line 30: "^\\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 31: public static final String ISO_SUFFIX = ".iso"; Line 32: public static final String ISO_SUFFIX_PATTERN = "^$|^.+\\.iso$"; Line 33: public static final String SUBNET_PATTERN_AS_NETMASK_WITH_OPTIONAL_PREFIX_SLASH = "^(?:/)?(3[0-2]|[0-2]\\d|\\d)$"; The variable name is really long and not anymore helpful than SUBNET_PREFIX_PATTERN. Also, the regex should be changed to (see that you understand why): "^/?(?:3[0-2]|[1-2]\\d|\\d)$" Line 34: public static final String NETMASK_PATTERN = Line 35: "(^\\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 36: Line 37: /** Line 30: "^\\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 31: public static final String ISO_SUFFIX = ".iso"; Line 32: public static final String ISO_SUFFIX_PATTERN = "^$|^.+\\.iso$"; Line 33: public static final String SUBNET_PATTERN_AS_NETMASK_WITH_OPTIONAL_PREFIX_SLASH = "^(?:/)?(3[0-2]|[0-2]\\d|\\d)$"; Line 34: public static final String NETMASK_PATTERN = This isn't special to netmask, it's just an IP address, isn't it? The only difference I see from IP_PATTERN is that you don't accept empty strings here. So this should be named IP_PATTERN_NON_EMPTY. Line 35: "(^\\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 36: Line 37: /** Line 38: * the mask will be replaced with zero-padded number in the generated names of the VMs in the pool, see http://gerrit.ovirt.org/#/c/35144/7/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 58: * function will return false Line 59: * Line 60: * @return true if mask is in IPv4 format ,false otherwise. Line 61: */ Line 62: public boolean isNetmaskFormatValid(String mask) { The name of this method isn't good. How is isNetmaskFormatValid() different from isMaskFormatValid()? How would someone understand from their names what they do differently? Maybe should be unified to one method that accepts as a boolean argument whether a prefix is allowed. Line 63: return mask != null && mask.matches(ValidationUtils.IP_PATTERN); Line 64: } Line 65: Line 66: /*** Line 59: * Line 60: * @return true if mask is in IPv4 format ,false otherwise. Line 61: */ Line 62: public boolean isNetmaskFormatValid(String mask) { Line 63: return mask != null && mask.matches(ValidationUtils.IP_PATTERN); IP_PATTERN accepts an empty string, was that your intention? Line 64: } Line 65: Line 66: /*** Line 67: * check if mask is valid and netmasked formated and return true if does Line 68: * Line 69: * @param netmask Line 70: * @return true if the netmask is in IPv4 format and valid, false otherwise Line 71: */ Line 72: public boolean isNetmaskValid(String netmask) { Same issue with naming as above, compared to isMaskValid(). Line 73: long addressInBits = IpAddressConverter.getInstance().covnertIpToLong(netmask); Line 74: long mask = 1; Line 75: boolean isFirstOneFound = false; Line 76: http://gerrit.ovirt.org/#/c/35144/7/backend/manager/modules/common/src/test/java/org/ovirt/engine/core/common/utils/IpAddressConverterTest.java File backend/manager/modules/common/src/test/java/org/ovirt/engine/core/common/utils/IpAddressConverterTest.java: Line 14: Line 15: final private String ipAdd; Line 16: final private long ipAsLong; Line 17: Line 18: public IpAddressConverterTest(String ipAdd, long ipAsLong){ This file doesn't look formatted either. Line 19: this.ipAdd = ipAdd; Line 20: this.ipAsLong = ipAsLong; Line 21: } Line 22: Line 36: {"1.255.4.255", 33490175 }, Line 37: {"0.128.0.7", 8388615 }, Line 38: {"1.2.3.4", 16909060 }, Line 39: {"127.63.31.7", 2134843143 }, Line 40: {"0.0.0.1", 1 }, }); These tests aren't very readable - I have no idea whether they make sense or not (other than the last one). Consider: * Using values that are easy to translate, such as 1.1.1.1. * Using hexadecimal notation to specify the expected result (e.g for 255.255.255.255, 0xffffffff). Line 41: } Line 42: Line 43: } http://gerrit.ovirt.org/#/c/35144/7/backend/manager/modules/common/src/test/java/org/ovirt/engine/core/common/validation/MaskValidatorTest.java File backend/manager/modules/common/src/test/java/org/ovirt/engine/core/common/validation/MaskValidatorTest.java: Line 31: Line 32: @Test Line 33: public void checkMaskValidValue() { Line 34: Line 35: if (!isMaskValidFormat) { You shouldn't depend on the expected value passed by the developer - you should go by MaskValidator.getInstance().isMaskFormatValid(mask). Line 36: return; Line 37: } Line 38: Line 39: assertEquals("Failed to validate mask value" + mask, Line 42: Line 43: } Line 44: Line 45: @Test Line 46: public void checkNetmaskValidFormat() { This and the next method, if I understand correctly, are related to the naming issue of the methods in MaskValidator. Tests should probably be changed so another parameter is passed, whether a prefix should be allowed or not, and expected results modified correspondingly. Line 47: final String maskValidFormat = "255.255.0.0"; Line 48: final String maskInvlidPrefixFreeFormat = "31"; Line 49: Line 50: assertEquals("Failed to validate mask format prefixFree" + maskValidFormat, Line 71: @Parameterized.Parameters Line 72: public static Collection<Object[]> namesParams() { Line 73: return Arrays.asList(new Object[][] { Line 74: Line 75: // Bad Format prefix format These aren't prefixes. Line 76: { null, false, false }, //$NON-NLS-1$ Line 77: { "a.a.a.a", false, false }, //$NON-NLS-1$ Line 78: { "255.255.0", false, false }, //$NON-NLS-1$ Line 79: { "255.255.0.0.0", false, false }, //$NON-NLS-1$ Line 78: { "255.255.0", false, false }, //$NON-NLS-1$ Line 79: { "255.255.0.0.0", false, false }, //$NON-NLS-1$ Line 80: { "255.255.0.0.", false, false }, //$NON-NLS-1$ Line 81: Line 82: // Prefix not valid These neither. Line 83: { "255.255.0.1", true, false }, //$NON-NLS-1$ Line 84: { "255.0.255.0", true, false }, //$NON-NLS-1$ Line 85: { "255.0.0.255", true, false }, //$NON-NLS-1$ Line 86: { "224.0.255.0", true, false }, //$NON-NLS-1$ Line 84: { "255.0.255.0", true, false }, //$NON-NLS-1$ Line 85: { "255.0.0.255", true, false }, //$NON-NLS-1$ Line 86: { "224.0.255.0", true, false }, //$NON-NLS-1$ Line 87: Line 88: // Valid prefix These neither. Line 89: { "255.255.0.0", true, true }, //$NON-NLS-1$ Line 90: { "255.255.255.255", true, true }, //$NON-NLS-1$ Line 91: Line 92: // 0-32 bad format format Line 94: { "33", false, false }, //$NON-NLS-1$ Line 95: { "31/", false, false }, //$NON-NLS-1$ Line 96: { "31*", false, false }, //$NON-NLS-1$ Line 97: { "//31 ", false, false }, //$NON-NLS-1$ Line 98: { "/31 ", false, false }, //$NON-NLS-1$ More invalid formats: "01" "/01" "33" "/33" Line 99: Line 100: // 0-32 valid Line 101: { "31", true, true }, //$NON-NLS-1$ Line 102: { "/31", true, true }, //$NON-NLS-1$ -- 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: 7 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Eliraz Levi <el...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Eliraz Levi <el...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Yevgeny Zaspitsky <yzasp...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
