[Bro-Dev] new IPv6 code

Siwek, Jonathan Luke jsiwek at illinois.edu
Wed Feb 15 14:33:42 PST 2012


> - in BroValUnion, 'addr_val' is of type pointer type IPAddr*. I
>  suppose that's to keep the union size as small as possible, which
>  makes sense. I'd still be curious though how allocating the
>  addresses dynamically compares against storing the instance directly
>  inside the union. Have you tried that? I realize it's not an easy
>  change to do that, so fine if not.

You mean having a IPAddr versus IPAddr* member in the union?  I had started out that way, until the compiler told me union members cannot have non-trivial constructors.  And I figured going with a pointer is better for size anyway as you mentioned, although the largest member in the old --enable-brov6 code was the subnets with five uint32's and that did not seem to increase memory as much in the tests as I expected.

> 
> - The DNS binpac analyzer generates both dns_a6_reply and
>  dns_aaaa_reply, but the standard analyzer only the latter (for both
>  resource record types). What is correct?

My thought was to generate both depending on the type, but I guess I forgot to do it for the non-binpac one.  I'll fix it.

> 
> - In the RPC code, there's this:
> 
>    is_mapped_dce_rpc_endpoint(const ConnID* id, TransportProto proto)
> ········{
> ········if ( id->dst_addr.family() == IPAddr::IPv6 )
> ················return false;
>        ...
>        }
> 
>   Does the protocol not support IPv6 at all or is that a "todo"?

I'm not sure -- the old --enable-brov6 switch also bailed out for IPv6.  So yeah, I suppose the todo is to determine how much more of a todo there is.

> 
> - in DPM.cc, the ExpectedConn class: should it now store IPAddr
>  instead of uint32[4] for orig and resp?

Yes, seems like it could do that.

> 
> - Expr.cc, BinaryExpr::AddrFold: why still the uint32[4] here? If it's
>  just to make the macro work, I'd just remove that and use
>  comparision operators instead.
> 

Ok, will do that.

> - IP.h, IP_Hdr: Do we need the new src_addr/dst_addr attributes? Why
>  not construct on the fly out of ip4/ip6?

I guess not.  Hadn't thought of that as I was just replicating the face that the old --enable-brov6 code also added those.  Will fix.

+Jon


More information about the bro-dev mailing list