On Fri, Feb 8, 2019 at 10:20 AM Brandur Leach <bran...@mutelight.org> wrote: > Attached a V2 patch: identical to V1 except rebased and > with a new OID selected.
Attached is a revised version that I came up with, based on your v2. I found this part of your approach confusing: > + /* > + * Number of bits in subnet. e.g. An IPv4 that's /24 is 32 - 24 = 8. > + * > + * However, only some of the bits may have made it into the fixed sized > + * datum, so take the smallest number between bits in the subnet and bits > + * in the datum which are not part of the network. > + */ > + datum_subnet_size = Min(ip_maxbits(authoritative) - > ip_bits(authoritative), > + SIZEOF_DATUM * BITS_PER_BYTE - > ip_bits(authoritative)); The way that you put a Min() on the subnet size potentially constrains the size of the bitmask used for the network component of the abbreviated key (the component that comes immediately after the ipfamily status bit). Why not just let the bitmask be a bitmask, without bringing SIZEOF_DATUM into it? Doing it that way allowed for a more streamlined approach, with significantly fewer special cases. I'm not sure whether or not your approach had bugs, but I didn't like the way you sometimes did a straight "network = ipaddr_datum" assignment without masking. I really liked your diagrams, but much of the text that went with them either seemed redundant (it described established rules about how the underlying types sort), or seemed to discuss things that were better discussed next to the relevant network_abbrev_convert() code. Thoughts? -- Peter Geoghegan
v3-0001-Add-sort-support-for-inet-cidr-opfamily.patch
Description: Binary data
