https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=7654
--- Comment #6 from Loránd Jakab <ljakab@xxxxxxxxxx> 2012-08-20 09:09:00 PDT ---
(In reply to comment #3)
> Hi Lorand,
>
> a few comments regarding your patch:
> - in line 1302
> proto_tree_add_item(lisp_tree, hf_lisp_xtrid, tvb, offset, LISP_XTRID_LEN,
> ENC_BIG_ENDIAN);
> should be replaced by
> proto_tree_add_item(lisp_tree, hf_lisp_xtrid, tvb, offset, LISP_XTRID_LEN,
> ENC_BIG_ENDIAN);
> as hf_lisp_xtrid is FT_BYTES
True, I missed that.
> - why not use expert_add_info_format instead of proto_tree_add_text for
> warnings/errors? It would permit you to benefit from the existing expert system
To be honest, simply because of my ignorance. I started developing the patch in
the 1.2.x days, I read the developer documentation back then, and haven't been
keeping up with new functions. Thanks for pointing this out, I'll look into it
and update all such calls. Can this patch be applied as-is for now, or should I
fix this first and resubmit?
> - the proto_tree_add_text calls you added in the new functions
> (dissect_lcaf_natt_rloc, dissect_lcaf_afi_list, dissect_lisp_mapping...) will
> make the items (like IP address) unfilterable. Is this desirable? Will you
> enhance it in the future?
Text items in the disector contain usually more than one protocol field and
make reading all the details easier. I would like to keep them as is, and add
subtrees in the future with the individual and filterable fields (which I agree
is very good to have!). I haven't implemented that yet, and I prefer to have
this out soon, since it's useable for a lot of people.
> BTW, I saw that hf_lisp_mrep_flags and hf_lisp_mreg_flags are unused. Were they
> supposed to be used so as to create a subtree? Can we remove them or should we
> add this subtree?
Let's have a clean patch, please remove them, and I will keep them around in my
tree, as for now I'm unsure if adding a 'flags' field and having the individual
flags in a subtree would worsen usability. I tend to prefer not adding them
ATM.
Thanks for the quick review!
-Lori
>
> Regards,
> Pascal.
--
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching all bug changes.