Eliraz Levi has posted comments on this change. Change subject: common: adding maskValidator util ......................................................................
Patch Set 4: (8 comments) http://gerrit.ovirt.org/#/c/35144/4/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]\\d|\\d)$"; > Why is this called SUBNET_PATTERN_AS_NETMASK? Why not something like NETMAS Done Line 33: public static final String SUBNET_PATTERN_AS_NETMASK_WITH_SLASH = "^(?:/)(3[0-2]|[0-2]\\d|\\d)$" Line 34: + "|^3[0-2]|[0-2]\\d|\\d$"; Line 35: public static final String NETMASK_PATTERN = Line 36: "(^\\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: "^\\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]\\d|\\d)$"; Line 33: public static final String SUBNET_PATTERN_AS_NETMASK_WITH_SLASH = "^(?:/)(3[0-2]|[0-2]\\d|\\d)$" > Why is this needed? Why not just modify SUBNET_PATTERN_AS_NETMASK to: Done Line 34: + "|^3[0-2]|[0-2]\\d|\\d$"; Line 35: public static final String NETMASK_PATTERN = Line 36: "(^\\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 37: + SUBNET_PATTERN_AS_NETMASK_WITH_SLASH + ")+"; 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]\\d|\\d)$"; Line 33: public static final String SUBNET_PATTERN_AS_NETMASK_WITH_SLASH = "^(?:/)(3[0-2]|[0-2]\\d|\\d)$" Line 34: + "|^3[0-2]|[0-2]\\d|\\d$"; Line 35: public static final String NETMASK_PATTERN = > Why are you changing this pattern? It is evidently still useful to only che Done Line 36: "(^\\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 37: + SUBNET_PATTERN_AS_NETMASK_WITH_SLASH + ")+"; Line 38: Line 39: /** http://gerrit.ovirt.org/#/c/35144/4/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 45: return isNetworkAddress(ipAsInteger, mask); Line 46: } Line 47: Line 48: /*** Line 49: * convert a given valid address IPv4 xx.yy.zz.ww to long, ww represent the LSB > This documentation is confusing, why two characters in each byte? Done Line 50: * Line 51: * @param ipAdd Line 52: * @return Line 53: */ Line 47: Line 48: /*** Line 49: * convert a given valid address IPv4 xx.yy.zz.ww to long, ww represent the LSB Line 50: * Line 51: * @param ipAdd > It is customary to explain what the parameters mean, same for return value Done Line 52: * @return Line 53: */ Line 54: public static long covnertIpToLong(String ipAdd) { Line 55: String[] subAdd = ipAdd.split("\\."); http://gerrit.ovirt.org/#/c/35144/4/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 34: * Line 35: * @param address Line 36: * @return Line 37: */ Line 38: public boolean isMaskValid(String netmask) { > Again, the method in its current form looks completely redundant, as I don' Done Line 39: if (!isMaskFormatValid(netmask)) { Line 40: return false; Line 41: } Line 42: Line 58: * {@link MaskValidator#isMaskValid(String)} Line 59: * Line 60: * @return true if correct IP format or string [0-32], false otherwise. Line 61: */ Line 62: public boolean isMaskValidFormatPrefixFree(String mask) { > Not a very comprehensible name, why not "isValidNetmaskFormat()" or "repres Done Line 63: return mask != null && mask.matches(ValidationUtils.IP_PATTERN); Line 64: } Line 65: Line 66: /*** Line 69: * @param address Line 70: * @return Line 71: */ Line 72: public boolean isMaskValidPrefixFree(String netmask) { Line 73: if (!isMaskValidFormatPrefixFree(netmask)) { > Again, why is this checked in this validation? It's a completely independen Done Line 74: return false; Line 75: } Line 76: Line 77: long addressInBits = CidrValidator.covnertIpToLong(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: 4 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
