IETF ForCES(Forwarding and Control Element Separation) dissector

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

Original bug information:

Reporter: Fenggen Jia
Status: RESOLVED FIXED
Product: Wireshark
Component: GTK+ UI
OS: All
Platform: All
Version: Git

Attachments:

forces-dissector.tar.gz: The tarbar includes a patch for create the packet-forces.c and update epan/dissectors/Makefile.common and a ForCES packets capture file using tshark
packet-forces.c: INCOMPLETE changes I was making to the dissector as I was reviewing it. This could be used as a starting point if someone wants to pick up on it, or just as ideas of changes to be made.
forCES_dissector.patch: Updated forCES dissector
forCES_dissector.patch: Updated forCES dissector 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.

This a ForCES protocol dissector that conforms to the newest protocol draft:http://www.ietf.org/internet-drafts/draft-ietf-forces-protocol-22.txt,the protocol has completed and now is waiting for its RFC number to be assigned.The dissector has the following features: 1 Support for two types of TMLs:TCP+UDP and SCTP 2 TCP+UDP TML: the default port is TCP port 3000 and UDP port 3001. 3 SCTP TML: the default port is SCTP port 6700,6701 and 6702. 4 Users can change the default ForCES TML port setting in the preference menu. 5 Full dissecting of ForCES main header,all kinds of the ForCES messages types and TLVs. 6 Multiple LFB selectors and recursive PATH DATA TLV are supported. Using the attatched ForCES capture file,fuzz test has been done.

(In reply to comment #2) > (In reply to comment #0) > > 2 TCP+UDP TML: the default port is TCP port 3000 and UDP port 3001. > > 3 SCTP TML: the default port is SCTP port 6700,6701 and 6702. > > From IANA: > Port 3000 is assigned hbci and 3001 is unassigned. > Port 6700 is unassigned, 6701 is assigned kti-icad-srvr and 6702 is assigned > e-design-net. > Where is this port numbers defined? TCP+UDP TML: The ports are defined for local test purposes,we can change the port to unused ports.FYI,TCP+UDP TML is now not a standard IETF ForCES draft,but it used to be,we just want to support it for some reasons,if asked,we can remove this support. SCTP TML:The ports are assigned in IETF draft http://www.ietf.org/internet-drafts/draft-ietf-forces-sctptml-03.txt,note that it's still a draft and not approved by IETF,we can recomended them to change the default port numbers,I will do this work.

(In reply to comment #3) > (In reply to comment #2) > > (In reply to comment #0) > > > 2 TCP+UDP TML: the default port is TCP port 3000 and UDP port 3001. > > > 3 SCTP TML: the default port is SCTP port 6700,6701 and 6702. > > > > From IANA: > > Port 3000 is assigned hbci and 3001 is unassigned. > > Port 6700 is unassigned, 6701 is assigned kti-icad-srvr and 6702 is assigned > > e-design-net. > > Where is this port numbers defined? > > TCP+UDP TML: The ports are defined for local test purposes,we can change the > port to unused ports.FYI,TCP+UDP TML is now not a standard IETF ForCES > draft,but it used to be,we just want to support it for some reasons,if asked,we > can remove this support. > SCTP TML:The ports are assigned in IETF draft > http://www.ietf.org/internet-drafts/draft-ietf-forces-sctptml-03.txt,note that > it's still a draft and not approved by IETF,we can recomended them to change > the default port numbers,I will do this work. Have confirmed that SCTP ports 6701 6702 and 6703 are not occupied. http://www.iana.org/assignments/sctp-parameters

Should num2messagetype, num2tlvtype, and num2operationtype be replaced with value strings? There are a lot of places where the dissector adds a uint32 formatted as an IPv4 address: temp=tvb_get_ntohl(tvb, offset+4); sid=tvb_get_ptr(tvb,offset+4 ,4); proto_tree_add_uint_format( forces_main_header_tree,hf_forces_sid, tvb, 4, 4, temp,"Source ID:%s",ip_to_str((guint8 *) sid)); Is there a reason for doing that instead of using proto_tree_add_item with FT_IPv4?

FWIW, this is now RFC 5810. Yes, the num2* functions can and should easily be replaced with value_strings. Other comments: - Ethereal has been Wireshark for several years now (applies to the copyright notice) - all the tvb_get_xxx() calls followed by immediately by proto_tree_add_xxx_format() should be replaced with proto_tree_add_item(). Adding value strings to the hf fields will make the displays come out right. - all (or certainly most of) the length checks should be removed: there's no need. Wireshark will work better if you just keep trying to access malformed packet--eventually an exception will be thrown and the packet will show up as malformed. This will save a lot of code and complexity (like the 'back' variables + returns). A reasonable goal would be the elimination of error_report_length(), though there may be some places where it would still be useful. - the check_col() calls can all come out (they're no longer necessary) - all of the functions should be static (so they don't pollute the global name space) - next_tvb can be a function-local variable (instead of a non-static global) - I think (but I'm not sure) that all of the function-static variables don't need to be static - Don't use temp, temp1, and temp2 over and over again through a function. Define variables for each thing so the variable name tells you what it is (this alleviates the need for a comment to say what's in the variable). If a function starts to have too many variables, that's a good sign it's time to make another function. - Instead of doing this: if(!abs(strcmp(num2tlvtype(type), "PATH DATA-TLV"))) or: if(abs(strcmp(num2operationtype(type), "Unknown Operation Type")) just use some macros (like TLV_PATH_DATA) and do an integer comparison. Or, for the latter, use the return of match_strval() to determine if it's a known operation. - The days of 80-column code are probably gone, but 300+ character lines are too long--there need to be some line wraps in here. (Eliminating those length checks and/or making sub-functions would help.)

(In reply to comment #13) > (In reply to comment #11) > > col_set_str(pinfo->cinfo, expert_add_info_format, ... must be also called when > > tree == NULL. > > I don't understand this comment. I thought most tree == NULL checks where for > still processing a packet even though you didn't need it displayed (like for > converstations, states, etc). This dissector is strictly for "display", which > is why the tree == NULL check is so early. Only if you want to display tree (heh, it's quite obvious). If you want to display only columns info, *sometimes* tree is NULL, check: 1/ packet_list_dissect_and_cache_record() (ui/gtk/packet_list_store.c) 1186 create_proto_tree = (dissect_color && color_filters_used()) || 1187 (dissect_columns && have_custom_cols(cinfo)); 1188 1189 epan_dissect_init(&edt, 1190 create_proto_tree, 1191 FALSE /* proto_tree_visible */); 2/ process_packet (tshark.c) 3066 if (cf->rfcode || verbose || filtering_tap_listeners || 3067 (tap_flags & TL_REQUIRES_PROTO_TREE) || have_custom_cols(&cf->cinfo)) 3068 create_proto_tree = TRUE; 3069 else 3070 create_proto_tree = FALSE; /* ... */ 3076 epan_dissect_init(&edt, create_proto_tree, print_packet_info && verbose); In gtk create_proto_tree depends on colors, so tree == NULL is unlikely. But in tshark it should be common, just don't pass -V. Similar case for expert info, 'tshark -q -z expert' don't require tree creation. From ui/cli/tap-expert.c: 255 error_string = register_tap_listener("expert", hs, 256 filter, ***0***, Compare with ui/gtk/expert_comp_dlg.c: 885 error_string=register_tap_listener("expert", etd, NULL /* fstring */, 886 ***TL_REQUIRES_PROTO_TREE***, So checking for tree == NULL, and not setting column info/expert is mostly ok in wireshark, in other application using libwireshark it might be not. AFAIK we care about other applications, quoting README.developer: "Note, however, that you must fill in column information, (...) do calls to "expert" functions, (...) regardless of whether 'tree' is NULL or not." > Should I just move the > col_set_str(pinfo->cinfo, COL_PROTOCOL, "ForCES"); > col_clear(pinfo->cinfo, COL_INFO); > > lines into dissect_forces_tcp() and dissect_forces_not_tcp()? I'd rather remove tree == NULL check, but it's your patch.

(In reply to comment #14) > So checking for tree == NULL, and not setting column info/expert is mostly ok > in wireshark, in other application using libwireshark it might be not. > AFAIK we care about other applications, quoting README.developer: > "Note, however, that you must fill in column information, (...) > do calls to "expert" functions, (...) regardless of whether 'tree' > > I'd rather remove tree == NULL check, but it's your patch. So why ever have the tree == NULL check? I have noticed certain dissectors seem to cater more towards Wireshark or tshark (whichever the developer is more used to) in how "fields" are populated (with some, IMO overpopulating the COL_INFO column for example). While it may be my patch, I would like its use to be maximized. To me the tree == NULL check is for faster processing (Wireshark only?). If I have to dissect the protocol for column info, "expert" functions, etc, I'm not saving much processing time (because the fields have to be dissected anyway for those items). And the code would be unnecessarily ugly with more tree == NULL checks sprinkled about the code to avoid missing column info, "expert" functions, etc. For "simple" protocols like this one, I thought it was common practice to put the tree == NULL near the top of the protocol dissection (but maybe it should be just after the setting column info)

(In reply to comment #15) > (In reply to comment #14) > > So checking for tree == NULL, and not setting column info/expert is mostly ok > > in wireshark, in other application using libwireshark it might be not. > > AFAIK we care about other applications, quoting README.developer: > > "Note, however, that you must fill in column information, (...) > > do calls to "expert" functions, (...) regardless of whether 'tree' > > > > I'd rather remove tree == NULL check, but it's your patch. > > So why ever have the tree == NULL check? I have noticed certain dissectors > seem to cater more towards Wireshark or tshark (whichever the developer is more > used to) in how "fields" are populated (with some, IMO overpopulating the > COL_INFO column for example). While it may be my patch, I would like its use > to be maximized. > > To me the tree == NULL check is for faster processing (Wireshark only?). If I > have to dissect the protocol for column info, "expert" functions, etc, I'm not > saving much processing time (because the fields have to be dissected anyway for > those items). And the code would be unnecessarily ugly with more tree == NULL > checks sprinkled about the code to avoid missing column info, "expert" > functions, etc. For "simple" protocols like this one, I thought it was common > practice to put the tree == NULL near the top of the protocol dissection (but > maybe it should be just after the setting column info) [This discussion might be better suited for the -dev list.] Yes, the tree==NULL check is for faster processing. Some people care very much about speed [probably because they often work with larger files]. Wireshark with no coloring rules (how I normally work) or tshark (without "-V") both stand to benefit from if(tree) checks. But it's all a question of effort vs. reward. If you've got a function with 100 proto_tree_add_item()s that don't affect the columns or expert info then you could easily trade one if(tree) for 100 function calls which then end up taking the shortcut out when tree==NULL. OTOH if you've got column updates and expert infos hiding in there (or if you're tracking the offset into the tvb) then you're probably better off without the if(tree) check.

IETF ForCES(Forwarding and Control Element Separation) dissector

Activity