https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=7615
--- Comment #6 from Mike Morrin <wireshark@xxxxxxxxxxxxxxx> 2012-08-12 11:57:14 PDT ---
(In reply to comment #5)
> Hi Mike,
>
> here are a few comments after a really quick review:
> - were the changes to packet-gsmtap.* approved by Osmocom guys? Please contact
> them and get their approval before proposing those changes
> - please do not put back Packet_Measurement_Order_Reduced_t structure: it was
> removed on purpose
> - I guess egprs_data_block_length array should return a guint16 (or other ones
> should return a guint8 to keep consistency)
> - ensure that filter names match the protocol names: we did an effort to clean
> this dissector so let's not introduce non valid filter names now
> (gsm_rlcmac.*). It can be checked with the tools/checkfiltername.pl script
> - in dissect_gsm_rlcmac_downlink(), data->frame_number does not get initialized
> if the private info is not present
> - you lack a test to avoid exceeding the maximum number of LI you defined when
> filling the array that can lead to a stack corruption
> - given their size, you could define dl_rlc_message_type_vals and
> ul_rlc_message_type_vals as extended value strings
>
> I'm running out of time right now but I will try to do a more in depth review
> in a few days.
Put this on hold until I have a chance to make some changes and test them
(maybe a couple of weeks).
I have realised that there is a structural error in the current version (I
coded 95% of this 6 months ago and made some mistakes while rebasing and
tidying up).
I also see that the current method of using gsmtap is unworkable, as the
gsm_rlcmac dissector needs information from the EGPRS UL header block (the PI
flag) to decode the accompanying EGPRS UL data block(s). I had done most of my
testing with a private plugin dissector instead of gsmtap, so had overlooked
this issue.
--
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching all bug changes.