On 5/16/07, Sebastien Tandel <sebastien@xxxxxxxxx> wrote:
Hi,
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?
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.
Continuing the answer to question 1, this unfortunately means the
flags field can't be treated in a byte order agnostic fashion anymore.
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?
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,
- 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 ;-)
- Have you fuzz tested your changes?
No, sorry. I haven't had the time or resources to do that. The only
things that to my understanding could mess things up are malformed
(i.e. wrong length) messages or message extensions. I've tested for
these and haven't at least been able to crash wireshark.
It's just been complaining about malformed messages when it's
encountered messages it hasn't been able to parse properly.
Regards,
Ville