Wireshark-bugs: [Wireshark-bugs] [Bug 6355] add dissectors for tracing SIM <-> Mobile Equipment

Date: Fri, 18 Nov 2011 07:32:08 -0800 (PST)
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=6355

--- Comment #1 from Bill Meier <wmeier@xxxxxxxxxxx> 2011-11-18 10:32:05 EST ---
Some comments:

packet-gsm_sim.c

1. We've recently changed the usage for the encoding parameter of 
   proto_tree_add_item().
   Instead of using TRUE/FALSE we're now using 
   ENC_NA/ENC_BIG_ENDIAN/ENC_LITTLE_ENDIAN depending upon the type.

   See README.developer (SVN) under proto_tree_add_item() for the rules.

   It appears to me that the protocol is big-endian so I 
   made a few changes by hand and then used a script
   (tools/fix-encoding-args.pl) to convert the encoding args as
   seems appropriate.

   I've attached a patch file for your use to update your packet-gsm_sim.c
   source file with the encoding parameter changes..

2. I'm not really a fan of the use of #if 0/#endif to combine a number 
   of value_strings into one quite large value_string. In addition there
   are ~35 dups in the combined array.

   How would you feel about just having one big sorted array with unique 
   entries ? If this is done, then an "extended" value string can
   be used which (in this case) will do a binary search thru the array.

   It would be also nice to sort some of the other value_string arrays (such as
   sw_vals[]) so that they can also accessed via an extended value string.

   See packet-rsl.c for a recently added example of the use of extended
   value strings.

3a. There's some #if 0'd code having to do with a preference; in addition the
   proto_reg_handoff...() code does nothing of any effect.

   I suggest removing both sections of code.

3b. However, there does need to be a proto_reg_handoff...() with the following 
   (moved from proto_register...())

    sub_handle_cap = find_dissector("etsi_cat");

   Looking up a dissector shouldn't be done until after all the
   dissectors have been registered since there's no real guarantee
   as to the order in which the dissectors are registered.

4. In general, tvb_reported_length() should be used instead of tvb_length().
   tvb_reported_length() returns the actual value of the packet length
   when it was captured while tvb_length() reports the amount of the packet
   saved.  These can differ if the capture was configured to limit the
   amount of data saved from a packet ("snapshot length").

   Wireshark will display an appropriate message (something about 
   "data not available due to snapshot length") if there is an attempt
   to dissect data not actually available.

5. #include <stdio.h> not needed.

packet-card_app_toolkit.c

1. All but one of the proto_tree_add_item() encoding args should be
   ENC_BIG_ENDIAN.
   proto_tree_add_item(tree, proto_cat,...) should use ENC_NA.

   (See item #1 bove).

2. There are several value_string arrays of some length for which it 
   probably makes sense to access them using an extended value string.

3. See item #3a above.

4. See item #4 above.

5. #include <stdio.h> not needed.


Other notes

1. epan/CmakeLists.txt also needs to be updated with the new filenames.

-- 
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching all bug changes.