Eliraz Levi has posted comments on this change. Change subject: common: adding maskValidator util ......................................................................
Patch Set 2: (8 comments) 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 l Done 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 us Done 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 genera we decided that the validation between CIDR and Mask is different 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 we need to separate between two cases: cidr: 64.0.0.0/8 is valid subnetwork 01 0^30 but not a valid mask - we count from MSB the number of 1's 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 lon Done 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. Done 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 Done 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 does not using continue 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 <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Eliraz Levi <[email protected]> Gerrit-Reviewer: Lior Vernia <[email protected]> Gerrit-Reviewer: Martin Mucha <[email protected]> Gerrit-Reviewer: Yevgeny Zaspitsky <[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
