Wireshark-bugs: [Wireshark-bugs] [Bug 6355] add dissectors for tracing SIM <-> Mobile Equipment
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.
