Wireshark-bugs: [Wireshark-bugs] [Bug 8594] Adding support for IEEE-802.11ad protocol at the cur
Comment # 7
on bug 8594
from Jalil Moraney
Hi guys,
Fixes as requested had been done, please see notes inline:
(In reply to comment #5)
> (In reply to comment #4)
> > Hi Jalil, in general it looks fine.
> >
> > However, I noticed you do a lot of proto_tree_add_bool and
> > proto_tree_add_uint with data retrieved unmodified from the tvb. In this
> > case it is faster and more efficient to simply use proto_tree_add_item and
> > let Wireshark handle fetching the data. For those fields that are only a few
> > bits you can add a bitmask to the hf array entries instead of manually
> > masking in the dissector code.
> >
> + 1
We Changed it in all places where we hadn't modified the data.
> Also I have some warning when i launch Wireshark with your sample
> Warn Extended value string frame_type_subtype_vals forced to fall back to
> linear search: entry 24, value 23 < previous entry, value 362
> Warn Extended value string tag_num_vals forced to fall back to linear
> search: entry 129, value 143 < previous entry, value 221
> Warn Extended value string category_codes forced to fall back to linear
> search: entry 19, value 16 < previous entry, value 127
Fixed.
> Also checkhf is not happy
> Unused entry: epan/dissectors/packet-ieee80211.c,
> hf_ieee80211_ff_sswf_dmg_select
> Unused entry: epan/dissectors/packet-ieee80211.c,
> hf_ieee80211_ff_sswf_sector_select
> Unused entry: epan/dissectors/packet-ieee80211.c,
> hf_ieee80211_ff_sswf_snr_report
Fixed.
> Also why readd some static const true_false_string (dsss_ofdm_flags,
> cf_apsd_flags... )?
Please further explain of what you think the proper behaviour should be.
> I have some warning also with clang analyser tools
> packet-ieee80211.c:13843:11: warning: Value stored to 'offset' is never read
> packet-ieee80211.c:13832:17: warning: Value stored to 'offset' is never read
> packet-ieee80211.c:13848:4: warning: Value stored to 'offset' is never read
> packet-ieee80211.c:13862:11: warning: Value stored to 'offset' is never read
> packet-ieee80211.c:13855:4: warning: Value stored to 'offset' is never read
We didn't get the warnings after the changes, however please verify they are
gone since we don't have any prior experience with clang tools.
> Check also your indent (the file use 2 spaces indent) and there is some
> trailing whitespaces...
> (From Git : warning: squelched 114 whitespace errors, warning: 119 lines add
> whitespace errors. )
Fixed as far as we could tell.
> About allocation_duration_base_custom function, use
> beacon_interval_base_custom function (rename if it is needed)
The allocation_duration_base_custom behaves differently than
beacon_interval_base_custom (there is a constant of 1024). We couldn't find a
way to merge them to one function, due to lacking the possibilty of passing
different constant to the calls (is it possible?).
Since we changed to proto_tree_add_item, we needed 2 extra (very) simple custom
base function that before was covered by direct manuplation of the data
retrived from the tvb. Any other suggestion?
> Also there is some typo error : Changing control frame extension for
> extesion frames */ or + {"Low Power SC PHY Supported",
> "wlan.dmg_capa.low_power_suuported", or +
> add_tag_relay_capableities(tag_len, tree, tvb, &offset);
Fixed.
> About /* causes a segmentation fault when called. */, you need to
> "initialize" g_pinfo in dissect_dmg_beacon
> @@ -7569,6 +7568,8 @@ dissect_dmg_beacon(packet_info *pinfo, proto_tree
> *tree, tvbuff_t *t
> int tagged_parameter_tree_len;
> int current_offset = offset;
>
> + g_pinfo = pinfo;
No further use or need of g_pinfo in dissector.
Thanks,
Majd & Jalil
You are receiving this mail because:
- You are watching all bug changes.
