Mike Kolesnik has posted comments on this change.
Change subject: engine: Prevent empty MAC Address range
......................................................................
Patch Set 2:
(4 comments)
I know it's presumably a refactor, however seeing as this is a continuation of
the previous work in this area, I wrote some comments around some areas which I
think can be greatly simplified and have much better performance with lower
overhead.
Since you start using this code in config tool, I think performance becomes
more critical as user expects the tool to work in reasonable time.
Also we need to consider memory utilization in case people try to use a very
big range.
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/MacAddressRangeUtils.java
Line 10:
Line 11: private static final String MAC_ADDRESS_MULTICAST_LSB =
"13579bBdDfF";
Line 12: private static final int HEX_RADIX = 16;
Line 13:
Line 14: public static Set<String> initRange(String start, String end) {
I'd expect this to return a List<String> since by definition in a range you
can't have duplicates, so it doesn't make much sense to return Set it's just
over protection (at a price of slower, more memory hogging implementation)
Also, can you please add javadoc?
Line 15:
Line 16: String parsedRangeStart = parseRangePart(start);
Line 17: String parsedRangeEnd = parseRangePart(end);
Line 18: if (parsedRangeEnd == null || parsedRangeStart == null) {
Line 27:
Line 28: Set<String> macAddresses = new HashSet<String>();
Line 29:
Line 30: for (long i = startNum; i <= endNum; i++) {
Line 31: String value = String.format("%x", i);
I think it would be good to check at this point:
if (0x01000000 & value != 0) {
continue;
}
This will be moch more efficient to check LSB of the MAC for multicast address
Line 32: if (value.length() > 12) {
Line 33: return macAddresses;
Line 34: } else if (value.length() < 12) {
Line 35: value = StringUtils.leftPad(value, 12, '0');
Line 45:
Line 46: return macAddresses;
Line 47: }
Line 48:
Line 49: private static String parseRangePart(String start) {
This can all be changed by something like:
"aa:aa:aa".replaceAll(":", "")
Line 50: StringBuilder builder = new StringBuilder();
Line 51: for (String part : start.split("[:]", -1)) {
Line 52: String tempPart = part.trim();
Line 53: if (tempPart.length() == 1) {
Line 61:
Line 62: return builder.toString();
Line 63: }
Line 64:
Line 65: private static String createMacAddress(String value) {
You could replace the code by nicer code such as:
StringUtils.join("aabbccddeeff".split(".."), ":")
Line 66: StringBuilder builder = new StringBuilder();
Line 67:
Line 68: for (int j = 0; j < value.length(); j += 2) {
Line 69: String group = value.substring(j, j + 2);
--
To view, visit http://gerrit.ovirt.org/20155
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I24113379e8fbbc15a63bdb1be4e6c719d7cb764f
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Moti Asayag <masa...@redhat.com>
Gerrit-Reviewer: Mike Kolesnik <mkole...@redhat.com>
Gerrit-Reviewer: Moti Asayag <masa...@redhat.com>
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
