Lior Vernia has posted comments on this change. Change subject: common: adding maskValidator util ......................................................................
Patch Set 2: (8 comments) Haven't gone over the tests yet as I didn't like the code itself... :) 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])$"; This should probably be called something like SUBNET_PREFIX and look more like: ^3[0-2]|[1-2]\\d|\\d$ 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) { This doesn't seem like the right access modifier, if the method is to be used by other utilities then it should be public. It also probably belongs in the other class (which is more general and probably shouldn't be called MaskValidator, see comments there). 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 { This class could be called IpAddressValidator/Helper, as it provides general information about IP addresses. It can be used by CidrValidator, see below comments. 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)) { This method aggregates too much functionality in my opinion - I would make it more atomic, e.g. representsSubnet(). In its documentation you could demand a pre-condition that the passed String should already be of a valid IP address format. This can have an overload that accepts an int prefix "hint" as argument, to be used by CidrValidator (move some logic here from CidrValidator). Line 40: return false; Line 41: } Line 42: Line 43: if (mask.matches(ValidationUtils.SUBNET_PATTERN_AS_NETMASK)) { Line 43: if (mask.matches(ValidationUtils.SUBNET_PATTERN_AS_NETMASK)) { Line 44: return true; Line 45: } Line 46: Line 47: long maskAsInt = CidrValidator.covnertIpToInt(mask); The naming of the method here is confusing, as it evidently converts to long and not to int. Line 48: long flag = 1; Line 49: flag <<= 32; Line 50: Line 51: boolean isFirstZeroFound = false; Line 44: return true; Line 45: } Line 46: Line 47: long maskAsInt = CidrValidator.covnertIpToInt(mask); Line 48: long flag = 1; I would construct the logic differently. Firstly, "mask" usually refers not to the tested value, but to what you & or | with. Secondly, typically the mask doesn't change and the value tested against it is bit-shifted. Thirdly, I would use a mask for the right-most bit and look first for zeros and then start looking for ones (simple and you only shift in one direction). Line 49: flag <<= 32; Line 50: Line 51: boolean isFirstZeroFound = false; Line 52: for (int i = 0; i < 32; i++) { Line 49: flag <<= 32; Line 50: Line 51: boolean isFirstZeroFound = false; Line 52: for (int i = 0; i < 32; i++) { Line 53: flag >>= 1; If you'll be starting with a one mask, you should only shift the passed int right at the end of the loop, rather than at the beginning. Line 54: if (!isFirstZeroFound && (flag & maskAsInt) == 0) { Line 55: isFirstZeroFound = true; Line 56: continue; Line 57: } Line 50: Line 51: boolean isFirstZeroFound = false; Line 52: for (int i = 0; i < 32; i++) { Line 53: flag >>= 1; Line 54: if (!isFirstZeroFound && (flag & maskAsInt) == 0) { This doesn't depend on whether zero/one has already been found, and it doesn't add any performance gain to add this check. Instead of using continue, you can put the next clause in an "else" statement. More elegant in my opinion. Line 55: isFirstZeroFound = true; Line 56: continue; Line 57: } Line 58: -- 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 <el...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@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
