Wireshark-bugs: [Wireshark-bugs] [Bug 7806] Enhance to support P2MP-RSVP related TLVs of MPLS OA
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=7806
--- Comment #6 from Tomofumi Hayashi <s1061123@xxxxxxxxx> 2012-10-07 08:34:27 PDT ---
Hi Evan,
Thank you for your valuable comments.
Please see my comments inline [TomH].
(In reply to comment #5)
> (In reply to comment #3)
> > Hi Evan,
> >
> > Thank you for your so-much-quick reply!
> >
> > (In reply to comment #2)
> > > Hi Tomofumi, thanks for the patch. Two quick questions:
> > >
> > > - Why do you register hf_mpls_echo_tlv_fec_rsvp_p2mp_ipv4_ext_tunnel_id as a
> > > uint32 and then add it to the tree with proto_tree_add_text as an IPv4 address?
> > > Wouldn't it be easier to just register it as an IPv4 field?
> > From RFC, _ext_tunnel_id is described just as "unique identifier", not ip
> > address,
> > hence I keep uint32_t.
> > http://tools.ietf.org/html/rfc4875#section-19.1.1
> >
> > But _ext_tunnel_id depends on address type, i.e. IPv4-rspv has 32bit for ext
> > tunnel and ipv6 has 128bit as well.
> >
> > So I will change to ipv4 addr soon and update the patch.
>
> If I understand correctly then, it is defined in the standard as a unique
> identifier that happens to have the correct length to store an IP address (v4
> or v6 depending), with the result that everybody just stores an IP address in
> it, even though the standard doesn't technically specify that?
>
> If that's the case then the existing code that adds it to the tree with text
> and adds a hidden item for filtering is probably fine - it really depends if
> people are more likely to want to specify it as a uint or an IP address in the
> filter bar.
[TomH]
You're correct. I keep to text as same as
hf_mpls_echo_tlv_fec_rsvp_ipv4_ext_tunnel_id.
> > > - There are a few places where you use proto_tree_add_text to display an error
> > > condition - is there a particular reason you're not using
> > > expert_add_info_format?
> > This is why packet-mpls-echo.c uses only proto_tree_add_text() for the error.
> > I wrote this code looking packet-mpls-echo.c only, so I don't recognize
> > expert_add_info().
> >
> > I can rewrite my diff to using expert_add_info(). Should I rewrite it?
>
> expert_add_info_format is the new way to indicate possibly malformed packets
> and other things of that nature. It probably didn't exist when the dissector
> was originally written, and nobody's bothered to update it since, but new code
> should be using it. packet-moldudp.c is a nice short dissector that uses it a
> few times if you're looking for an example.
[TomH]
I've roughly looked packet-moldudp.c and using expert_add_info()
packet-mpls-echo.c needs a little big change. If possible, I would like to keep
it this time because it needs more review and additional test.
I will modify entire file in next time in a separate bug ticket, e.g. "change
packet-mpls-echo.c to use new dissector API". This makes more clear bug ticket
and easy to review. Of course, it is not so far from now ;)
--
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching all bug changes.
