Add IPv6 support #20
Add IPv6 support #20
Conversation
This is my first stab at adding IPv6 support to IPTV-Analyzer. I've tried to keep down the amount of duplication for IPv4 and IPv6 and instead use as much common code as possible but it's somewhat difficult. I've verified it's working for both IPv4 and IPv6 streams but there might be corner cases that aren't covered.
| @@ -271,8 +273,9 @@ struct pid_data_t { | |||
|
|
|||
| /* Data to match a stream / connection */ | |||
| struct mpeg2ts_stream_match { /* Like xt_hashlimit: dsthash_dst */ | |||
| __be32 dst_addr; /* MC addr first */ | |||
| __be32 src_addr; | |||
| int version; | |||
There was a problem hiding this comment.
This structure were explicitly kept small, as a memcmp() is done on each packet, thus this change would hurt performance...
There was a problem hiding this comment.
I see.
I wasn't sure how to determine whether the nf_inet_addr union was currently holding an IPv4 or IPv6 address and therefore added this version variable.
The version variable is pretty much only used to control how the values are printed, ie using NIP6 or %pI4. If there is a NIP46() that could adapt to nf_inet_addr that would significantly reduce the amount of code around print statements.
There was a problem hiding this comment.
According to http://stackoverflow.com/questions/9479424/how-to-determine-what-type-is-used-in-a-union and other similar posts, it's not possible to determine what type is actually used in a union.
We could use a fixed size instead, like in6 and do IPv4 in IPv6 mapping, but that would worsen the case for IPv4 as it consumes four times more memory than necessary.
Another approach might be to pass an extra argument along to all the functions saying whether it's an IPv4 or IPv6 match object being passed.
Third, we could shrink version to a char, saving one byte (I'm presuming int is normally 16 bits).
|
NIP6 seems to be deprecated in favour of %pI6 that was introduced with 2.6.29. Do we care about backwards compatibiltiy that far? Should I use %pI6 instead? |
|
I think we/you should use %pI6, I don't think we need backward compat with 2.6.29. I'm thinking of doing a release, before we start adding IPv6 support. This will give users a more clear picture, and allow us to relax on compat change in development git tree. |
|
On 2015-02-13 00:24, Jesper Dangaard Brouer wrote:
|
In 2.6.29 %pI6 was introduced and is favored over the older NIP6 format. In addition, there is %pI6c, which I'm using, that will format a compressed IPv6 addresses, greatly increasing readability.
|
Is there anything I can do to help get this merged? You mentioned doing a pre-IPv6 release, perhaps I can help out with that somehow? Are there other things you want me to fix with this patch before we can merge it? |
|
I did a release (0.9.4), so it will be easier to distinguish when IPv6 support gets added. We/you still have to fix up the match struct (mpeg2ts_stream_match), so we don't introduce a potential performance regression for IPv4, before I'm going to take these changes. |
|
Thanks for putting together that release! :) I suppose the fix up refers to the introduction of the new "version" in the match struct, correct? As I mentioned before, I don't see how one can determine the type that is currently stored in a union, so short of adding "version" in the struct I suppose the only other method would be to pass an argument through all the function calls to notify each function if an IPv4 or IPv6 struct is being passed. Would this align with your view or do you see another way of doing it? |
|
Look at how the "xt_hashlimit" module does this split, I think we should use that model. See code at: https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/tree/net/netfilter/xt_hashlimit.c |
This is my first stab at adding IPv6 support to IPTV-Analyzer. I've
tried to keep down the amount of duplication for IPv4 and IPv6 and
instead use as much common code as possible but it's somewhat difficult.
I've verified it's working for both IPv4 and IPv6 streams but there
might be corner cases that aren't covered.