Some quick comments :
1) please use proto_tree_add_item whenever possible. Don't use
tvb_get_* if you don't intend to use the retrieved value for another
purpose than inserting it in the tree.
Ok. I just used it to convert some multi-byte values to host byte
order. Is there a better way of doing this, or are the items added
through proto_tree_add_item somehow automatically displayed is host
byte order?
Yes, in fact, the last argument of proto_tree_add_item() let you
choose the byte order.
It's a boolean field which tells that the value is a little endian
representation if TRUE and big endian if FALSE.
2) Why have you changed the length type of some fields
(icmp.mip.flags, icmp.mip.r, icmp.mip.h, ...)? Do we really want to
be unable to parse correctly mip captures which contains these fields
but of previous lengths?
I haven't really changed the length of anything. As you see the
reserved field is now part of the 16-bit flags field. Originally the
ICMP mobility agent advertisement extension just had 8 1-bit flags and
8 reserved bits. Now, as of RFCs 3519 and 3543, there are 10 1-bit
flags and 6 bits of reserved space. It's still 16-bits, it's just
divided in 10 + 6 bits instead of 8 + 8.
OK. Do you have a mean to distinguish between mip captures from the
previous norm and the new ones? Or is it really useless?
Whatever the answer is, please propagate the changes when
inserting
in the tree. At least icmp.mip.flags is inserted once in the tree
with a display length field of 1.
Sorry? I didn't quite understand this. The flags field is added using
proto_tree_add_uint with length 2 and the corresponding
hf_register_info entry has type FT_UINT16. What else is needed?
Forget about it. I've missed the - sign in the patch for the line I
mentioned.
Maybe I was not wearing my glasses when doing this review ;)
Some questions :
- Do you have traces you can share? If so, can you put them on
the captures wiki page?
Sure, I can generate some if needed,
That will be great. Thanks.
- Is there a description on the wiki page?
No, but I noticed there doesn't seem to be anything at all about
Mobile IP on the wiki ;-)
Maybe it's time to put something then :)
- Have you fuzz tested your changes?
No, sorry. I haven't had the time or resources to do that.
That's not too much resources ... simply run for a few passes
tools/fuzztest.sh mip1.cap mip2.cap mip3.cap
Regards,
Sebastien Tandel
