r111829 MediaWiki - Code Review archive

/36 is not a valid IPv4 mask since an IP address is 32 bits.

Instead of randomly generated IP, you need to make a list that would actually stress the code. For example making sure 100.0.0.0 is after 10.0.0.0. Or 10.0.0.1 after 10.0.0.0 but before 10.0.0.2.

How would you sort 10.0.0.0/24 vs 10.0.0.0/16. And 10.0.0.0/24 vs 10.0.0.0 ?

I guess an IP should be normalized to use a /32 mask then sort using that.

I'd sort 10.0.0.0/16, 10.0.0.0/24, 10.0.0.0 (from greater nets to smaller ones)

I think that is a rather technical assumption. In my mind the users will care more about the IP order, and not so much about the technical meaning of the CIDR mask.

Also note this is the IP sorter, supporting IP/CIDR notation, not an IP/CIDR sorter.

It is not reason to make false assumptions and write tests based on random content. We need obvious test cases and make sure the code does respect them. The code will sort a single IP by appending 000. Thus we could well have a sort output like: 10.0.0.0 10.0.0.0/0 10.0.0.0/32 which does not make sense since the first (10.0.0.0) is strictly the same as 10.0.0.0/32.

If we add CIDR notation support, we might as well have the sorter properly use it.

i'm not sure how to do this. I've been tinkering with a base2 version of this formatter, but higher ip addresses flip the sign whenever I try to get a numeric or string value out of this.

			var	a = s.split( '.' ),
				r,
				u;
			if( a.length == 4 ) {
				var cidr = a[3].split('/');
				if (cidr.length > 1 ) {
					a[3] = cidr[0];
					a[4] = cidr[1];
				} else a[4] = '32';
			}
			for ( var i = 0; i < a.length; i++ ) {
				var item = parseInt(a[i]);
				item &= 255;
				switch(i) {
					case 0:
						r = (item << 24);
						break;
					case 1:
						r |= (item << 16);
						break;
					case 2:
						r |= (item << 8);
						break;
					case 3:
						r |= item;
						break;
					case 4:
						u = r; /* preserve unmasked value */
						if( item <= 32 ) {
							item = 32 - item;
							if( item > 0 ) {
								r = r >> item;
								r = r << item;
							}
						}
						break;
				}
			}

This does not work for instance with "192.168.31.2" . If anyone has more experience with bit operations in JS, then I welcome the feedback.

OK, i might have found a way, but it's becoming a rather complex function now... It will also require testing on IE. BTW, what do you guys expect to sort higher ? So what is the expectation regarding sort order ? I currently have:

Ascending: 10.0.0.0/8 10.5.5.5/8 10.0.0.0/16 10.0.0.0/32, 10.0.0.0 10.5.5.5 192.168.0.0/8 192.168.0.0/32 192.168.0.0

That is masked value from big to small, then for equal subsets, subordered by unmasked value. A /32 is also 'smaller' than an unmasked IP address.

I would keep the host sort this way but sort the CIDR in reverse order. A /8 represents roughly 16Millions IP, way more IP than a /32:

Ascending:

10.0.0.0
10.0.0.0/32
10.0.0.0/16
10.0.0.0/8
10.5.5.5
10.5.5.5/8
192.168.0.0
192.168.0.0/32
192.168.0.0/8

The next funny case is were to insert 10.255.0.0/16 in the above list ? It should be between 10.0.0.0/16 and 10.0.0.0/8 since it has the same size has 10.0.0.0/16 but it is contained by 10.0.0.0/8.

10.0.0.0
10.0.0.0/32
10.0.0.0/16
10.255.0.0/16 <-----
10.0.0.0/8

That is not going to help you devising a smart CIDR sorter :-]

Since you normalize an IP to /32, you can not guarantee yet that an IP will be placed before its /32 representation. One way to verify it is to try sorting:

10.0.0.0
10.0.0.0/32

And sort

10.0.0.0/32
10.0.0.0

And verify that both give the same output for consistence.