Eliraz Levi has posted comments on this change. Change subject: common: adding maskValidator util ......................................................................
Patch Set 7: (20 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. Done 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. Done 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. Done 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? Done 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 i Done 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 sa Done 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 a Done 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_NO Done 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 removed 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 removed 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 Done 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? Done 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(). Done 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. Done Line 19: this.ipAdd = ipAdd; Line 20: this.ipAsLong = ipAsLong; Line 21: } Line 22: 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 sh Done 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 nam Done 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. Done 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. Done 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. Done 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: Done 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
