[bugzilla-daemon@xxxxxxxxxxxxx]

Jalil Moraney changed bug 8594

What	Removed	Added
Attachment #11900 Flags		review_for_checkin?

Comment # 19 on bug 8594 from Jalil Moraney

Created attachment 11900 [details]
802.11ad patch v4

(In reply to comment #10)
> (In reply to comment #7)
> 
> > 
> > > 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.
> > 
> Fixed by Michael

Fixed in previous version.

> > > 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.
> > 
> I have always this warning with Clang (the offset value is never read
> after...)

As far as I could tell there were fixed after the TAG_* default case with a
TODO, not by us.

> 
> (In reply to comment #9)
> > In addition:
> > Some of the TAG_DMG_* cases appear to have overlapping offsets.  Should the
> > boolean values really be 16 bits?
> > 
> > TAG_DMG_OPERATION (and some others) seems to have a bit field of 16 bytes,
> > which would make the items 2 bytes, not 1.
> 
> I confirm 
> It is not possible proto_tree_add_bitmask ? or use the some length for each
> hf ? (it is not easy to read)
> Example for Antenna Sector ID with your patch :
>         Tag: Antenna Sector ID
>             Tag Number: Antenna Sector ID (190)
>             Tag length: 4
>             .... 0000 = Type: 0x00
>             .... ..00 0000 .... = Tap 1: 0x0000
>             0000 00.. = State 1: 0x00
>             0000 0000 = Tap 2: 0x00
>             0000 0000 = State 2: 0x00
> 
> With patch (there is a bug in your bitmask i thinks...)
> 
> My Patch :
> --- a/epan/dissectors/packet-ieee80211.c
> +++ b/epan/dissectors/packet-ieee80211.c
> @@ -13451,11 +13451,11 @@ add_tagged_field(packet_info *pinfo, proto_tree
> *tree, tvbuff_t *tvb, int offset
>          break;
>        }
>        offset += 2;
> -      proto_tree_add_item(tree, hf_ieee80211_tag_type, tvb, offset, 1,
> ENC_NA);
> -      proto_tree_add_item(tree, hf_ieee80211_tag_tap1, tvb, offset, 2,
> ENC_NA);
> -      proto_tree_add_item(tree, hf_ieee80211_tag_state1, tvb, offset+1, 1,
> ENC_NA);
> -      proto_tree_add_item(tree, hf_ieee80211_tag_tap2, tvb, offset+2, 1,
> ENC_NA);
> -      proto_tree_add_item(tree, hf_ieee80211_tag_state2, tvb, offset+3, 1,
> ENC_NA);
> +      proto_tree_add_item(tree, hf_ieee80211_tag_type, tvb, offset, 4,
> ENC_NA);
> +      proto_tree_add_item(tree, hf_ieee80211_tag_tap1, tvb, offset, 4,
> ENC_NA);
> +      proto_tree_add_item(tree, hf_ieee80211_tag_state1, tvb, offset, 4,
> ENC_NA);
> +      proto_tree_add_item(tree, hf_ieee80211_tag_tap2, tvb, offset, 4,
> ENC_NA);
> +      proto_tree_add_item(tree, hf_ieee80211_tag_state2, tvb, offset, 4,
> ENC_NA);
>        offset += 4;
>        break;
>      }
> @@ -17041,27 +17041,27 @@ proto_register_ieee80211 (void)
>  
>      {&hf_ieee80211_tag_type,
>       {"Type", "wlan.sctor_id.type",
> -      FT_UINT8, BASE_HEX, NULL, 0x0f,
> +      FT_UINT32, BASE_HEX, NULL, 0x0f000000,
>        NULL, HFILL }},
>  
>      {&hf_ieee80211_tag_tap1,
>       {"Tap 1", "wlan.sctor_id.tap1",
> -      FT_UINT16, BASE_HEX, NULL, 0x03f0,
> +      FT_UINT32, BASE_HEX, NULL, 0x03f00000,
>        NULL, HFILL }},
>  
>      {&hf_ieee80211_tag_state1,
>       {"State 1", "wlan.sctor_id.state1",
> -      FT_UINT8, BASE_HEX, NULL, 0xfc,
> +      FT_UINT32, BASE_HEX, NULL, 0x00fc0000,
>        NULL, HFILL }},
>  
>      {&hf_ieee80211_tag_tap2,
>       {"Tap 2", "wlan.sctor_id.tap2",
> -      FT_UINT8, BASE_HEX, NULL, 0xff,
> +      FT_UINT32, BASE_HEX, NULL, 0x0000ff00,
>        NULL, HFILL }},
>  
>      {&hf_ieee80211_tag_state2,
>       {"State 2", "wlan.sctor_id.state2",
> -      FT_UINT8, BASE_HEX, NULL, 0xff,
> +      FT_UINT32, BASE_HEX, NULL, 0x000000ff,
>        NULL, HFILL }},
>  
>      {&hf_ieee80211_tag_allocation_id,
> -- 
> It should be applied to BRP_Request, SSW, SSWF, DMG Capa (octet by octet if
> needed), DMG Oper, Extended shcedule, sta availability, DMG_BEAM_REFINEMENT

BRP_Request - patched.
SSW - patched.
SSWF - patched.
DMG Capa - patched by splitting 64bit mask into 24bit + 24bit + 16bit different
masks (couldn't find an option to write 64bit unsigned integer constant mask in
C without receiving conversion errors).
DMG Oper - I think there is no need to such patch here.
Extended schedule - patched.
sta avail - patched.
DMG beam refinement - patched.

> Also always the typo about add_tag_relay_capableities

Fixed.

> +proto_tree_add_text(tree, tvb, offset, 1, "AID of NextPCP %d: %d", i,
> tvb_get_guint8(tvb, offset));
> [...]
> +      proto_tree_add_text(tree, tvb, offset, 1, "Remaining BI's: %d",
> tvb_get_guint8(tvb, offset));
> [...]
> +      proto_tree_add_text(tree, tvb, offset, 2, "Request Token: %d",
> tvb_get_letohs(tvb, offset));
> Use proto_tree_add_item

Fixed.

> Also why change the length of hf_ieee80211_fc_frame_type_subtype ? 

I mistakenly thought it is needed to show the new 802.11ad packet with the
extension field and not only the type/subtype. Changed it back to 8.

> +  if (tag_len < 2) {
> +    proto_tree_add_text (tree, tvb, *offset + 2, tag_len,
> +      "Relay Capabilities: Error: Tag length must be 2 bytes long");
> +    return;
> Use expert_info

Fixed.

(In reply to comment #18)
> Hi Evans,
> Need to replace some proto_tree_add_text by expert_info or
> proto_tree_add_item

Fixed.

> and there is some unknown about "wrong bitmask/field size see comment 10"

If the same as:
"It should be applied to BRP_Request, SSW, SSWF, DMG Capa (octet by octet if
needed), DMG Oper, Extended shcedule, sta availability, DMG_BEAM_REFINEMENT."
described in comment 10, then it is fixed as well.

I would like to point out, that I hadn't worked on any other portion of this
patch but the points mentioned above. So, as always, there might be other
places to enhance/fix that I hadn't treated yet so feel free

Last but not least, sorry for disappearing. Life had other plans.

Jalil

---

      You are receiving this mail because:

• You are watching all bug changes.