Add features to the Locator/ID Separation Protocol (LISP) dissector

This issue was migrated from bug 7654 in our old bug tracker.

Original bug information:

Reporter: Loránd Jakab
Status: RESOLVED FIXED
Product: Wireshark
Component: GTK+ UI
OS: All
Platform: All
Version: Git

Attachments:

lisp_control_packet_dissector_updates.patch: LISP control packet dissector updates
lori-instanceid.pcap: LISP control packets with Instance ID
elp-compatibility_mode.pcap: LISP control packets with AFI List and ELP LCAF types
lisp_control_packet_dissector_updates.v2.patch: LISP control packet dissector updates (v2)

To upload designs, you'll need to enable LFS and have an admin enable hashed storage. More information

Child items 0

No child items are currently assigned. Use child items to break down this issue into smaller parts.

Originally submitted as bug #5854, this patch significantly enhances the LISP dissector: * adds dissector code for several new packet subtypes * more information is shown the Info column * code cleanup I will try to make smaller incremental patches in the future, with higher frequency, to make reviewing easier. Please consider adding this update to the tree, as the LISP user base has significantly increased. * checkAPIs: PASS * Fuzz testing: PASS (1000 passes with all of my capture files)

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 - 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 - 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? 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? Regards, Pascal.

(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 I believe you meant change to: proto_tree_add_item(lisp_tree, hf_lisp_xtrid, tvb, offset, LISP_XTRID_LEN, ENC_NA); I would also like to see some of the "error messages" placed in proto_tree_add_text to be replaced with expert_add_info_format

(In reply to comment #4) > I believe you meant change to: > proto_tree_add_item(lisp_tree, hf_lisp_xtrid, tvb, offset, LISP_XTRID_LEN, > ENC_NA); Doh, that's exactly what I meant. Thanks for spotting my typo :)

(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.

(In reply to comment #6) > 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? If you have the time to add the expert info quickly (should be rather easy, you can have a look at other dissectors for examples) then I would prefer to have this before commit. Would it match your time frame?

(In reply to comment #9) > Created attachment 8989 [details] > LISP control packet dissector updates (v2) > > Please consider adding to the tree this updated version of the patch. Looks good to me with two small things: - ENC_BIG_ENDIAN -> ENC_NA change for hf_lisp_xtrid (already changed locally) - does Instance ID dissection in dissect_lcaf_iid deserves a real hf variable instead of a proto_tree_add_text? Maybe being able to filter is is completely useless I'm waiting your feedback on Instance ID before committing.

(In reply to comment #10) > Looks good to me with two small things: > - ENC_BIG_ENDIAN -> ENC_NA change for hf_lisp_xtrid (already changed locally) Right. I missed that. Again! :) > - does Instance ID dissection in dissect_lcaf_iid deserves a real hf variable > instead of a proto_tree_add_text? Maybe being able to filter is is completely > useless Since the IID is similar in function to a VLAN ID, except it accomodates a larger ID space, it is expected to be widely used in data center deployments of LISP, to isolate tenants (who may use overlapping IP space). Admittedly, the dissector could handle IID with more detail (and it probably will, as time permits), but I think filtering on it can be useful. > > I'm waiting your feedback on Instance ID before committing. Good to go! :)

Add features to the Locator/ID Separation Protocol (LISP) dissector

Activity