On Fri, Feb 8, 2019 at 10:20 AM Brandur Leach <brandur@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