Re: The ieee802.11 dissector is a steaming pile of ordure

[Richard Sharpe <realrichardsharpe () gmail com>]

On Thu, Sep 10, 2015 at 12:42 AM, Alexis La Goutte
<alexis.lagoutte () gmail com> wrote:
> Hi Richard (and Bill)
>
> It is historic... at the first, there is only limited fixed field and tagged
> field...
> and on last 802.11 spec, there is > 100 tagged field...
>
> If you have a solution to avoid a big switch...
> i have already think to put each case on separate function... but for some
> case, there is only 5 lines of code...

I think the switch statement is OK, however, I would break out all the
open coded stuff into separate subroutines. I estimate that it would
reduce that subroutine to about 200-300 lines then.

I have vague thoughts about cleaning that dissector up, but I need to
find time. However, being aware of more problems makes the scope of
the effort clearer.

I also want to pass the phdr on to all the subdissectors so the
current crazy mechanism can be removed.

> Regards,
>
> On Wed, Sep 9, 2015 at 6:11 PM, Bill Meier <wmeier () newsguy com> wrote:
> > On 9/9/2015 12:03 PM, Bill Meier wrote:
> > > On 9/9/2015 11:23 AM, Richard Sharpe wrote:
> > > > Take a look at epan/dissectors/packet-ieee80211.c!
> > > >
> > > > Specifically, add_tagged_field.
> > > >
> > > > That function is approximately 2,300 lines long and it consists of one
> > > > big switch statement with every arm containing open-coded statements
> > > > to add things to the proto tree.
> > >
> > > It's even worse:
> > >
> > > add_fixed_field() given a "fixed field number" does a linear search thru
> > > a (large) table to to find the number (and the associated function
> > > address) and then calls the function ...
> > >
> > > One side effect: there are functions which aren't used but since they're
> > > in the table, they're not flagged as unused by the compiler.
> > >
> > > In several cases there is (or was) duplicate code elsewhere doing a
> > > dissection similar to the unused "fixed field functions".
> > >
> > > (I was working to fix all this but got a bit bored because I had to
> > > spend time delving thru the 802211 spec trying to understand the code.
> > > I guess I should at least do that work (unless you have a broader
> > > solution in mind to handle both tagged and fixed fields ?)
> >
> >
> > Actually, I guess add_tagged_field and add_fixed_field are sort of doing
> > the same thing (just in different ways) with respect to the lookup.
> >
> >
> >
> >
> >
> > ___________________________________________________________________________
> > Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
> > Archives:    https://www.wireshark.org/lists/wireshark-dev
> > Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
> >             mailto:wireshark-dev-request () wireshark org?subject=unsubscribe
>
>
>
> ___________________________________________________________________________
> Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
> Archives:    https://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
>              mailto:wireshark-dev-request () wireshark org?subject=unsubscribe



-- 
Regards,
Richard Sharpe
(何以解憂？唯有杜康。--曹操)
___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
Archives:    https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-request () wireshark org?subject=unsubscribe