[bugzilla-daemon@xxxxxxxxxxxxx]

https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=6355

--- Comment #5 from Harald Welte <laforge@xxxxxxxxxxxx> 2011-12-10 05:57:13 PST ---
(In reply to comment #1)

> 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.
[...]
>    I've attached a patch file for your use to update your packet-gsm_sim.c
>    source file with the encoding parameter changes..

thanks, I've merged your patch.

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

No, this is really just a hack.  What we need to do in order to resolve
this properly: Track the currently selected directory.  ISO7816-4 is a
filesystem access protocol, and SELECT can use relative path names.

I'm not entirely sure how we should track this.  Can I just use a static global
variable to represent the current directory?  This would mean that there is
always only one PCAP file being processed.  Is that a valid assumption to make?

In any case, I suggest that this be merged in an incremental change after the
original dissector goes in.

> In addition there are ~35 dups in the combined array.

That may be the case, but the FID space is not a global namespace.  The #if0
hack to create one large array is just a workaround


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

agreed, was a copy+paste left-over.

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

I've done that now in the most recent version of the patch

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

removed.

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

done.

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

not sure if it's really worth it?  what is the general rule here?

> 3. See item #3a above.
> 4. See item #4 above.
> 5. #include <stdio.h> not needed.

done.

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

done.

I've also done the rename to packet-etsi_card_app_toolkit.c as requested.
"etsi_cap" might be a mis-nomer, as there is also CAP == "CAMEL Application
Part".

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