Re: Extending PCLI payload decoding

[mmann78 () netscape net]

Do you want to post your current progress to the existing review and I can take a look at it from there?  That's 
probably the easiest way to look at the "rough edges".  You definitely want a dissector table with type FT_UINT32 (not 
a string)
 
 
 
 
-----Original Message-----
From: Luke Mewburn <luke () mewburn net>
To: Developer support list for Wireshark <wireshark-dev () wireshark org>
Sent: Tue, May 26, 2015 10:16 am
Subject: Re: [Wireshark-dev] Extending PCLI payload decoding


On Mon, May 25, 2015 at 12:18:17AM -0400, mmann78 () netscape net wrote:
  |   
Certainly fine to move the discussion here IMO.  I get better "text
  |   
formatting" in email form than I do from Gerrit anyway.
  | 
  |    Some
comments:
  |    1. I'm of the opinion that no NEW development should have
enum
  |    preferences for choosing subdissectors.  Dissectors that need to
give
  |    user an option to change subdissection should do it through Decode
As
  |    functionality.

Ok.

I've made a first pass at converting my
changeset from providing a
preference to implementing decode_as, with both
Ethernet and IP setup.

I'm running into a couple of rough edges.

I first
tried registering the subdissector table "pcli.payload"
as FT_UINT32, BASE_DEC
and using that with dissector_try_uint(table,0,...),
and registering IP & Eth
with:
   ip: dissector_add_for_decode_as("pcli.payload", ip_handle);
   eth:
dissector_add_for_decode_as("pcli.payload", eth_withoutfcs_handle);

It
"works" when I "decode as", but the UI window in "Decode As" has warty bits:
-
When I select a decode, I get
  GLib-CRITICAL **: g_hash_table_foreach:
assertion 'version == hash_table->version' failed
  Then wireshark locks up. I
think this is bug 11124 and not specifically
  related to my changes.  I
restart and continue.
- The Value column is 0, the Type is Integer, and the
Current menu pop-down 
  contains "", "(none)", "Ethernet", "IPv4".

I've
tried changing the subdissector to FT_STRING, BASE_NONE,
decoding with
dissector_try_string(table, "data",...)
and registering the sub dissectors
with:
  ip: dissector_add_string("pcli.payload", "ip", ip_handle);
  eth:
dissector_add_string("pcli.payload", "eth", eth_withoutfcs_handle);
but that
doesn't even decode.

I'm missing something that's probably obvious here
:-|

Do you have any specific suggestions which decode_as pattern
to use for
a dissector that doesn't have a particular identifier
in the payload and
instead relies upon the user to select a specific
decode_as pattern?

I can
provide my diff of the decode_as using FT_UINT32,
or upload a newer changeset
to my original change?


  |    2. I was unaware of all of the different PCLI
headers.  Guy's review
  |    comments make a little more sense now.  I think I
would take the route
  |    of creating 5 separate dissectors (all with unique
"protocol names" to
  |    help distinguish in Decode As dialog)  The
dissectors would dissect the
  |    fields you describe up to the payload and
then the payload would be
  |    controlled by Decode As.  I'm taking the
(uneducated) stance that the
  |    payload types are not fixed for any of the
different header types and
  |    that you could see any protocol as a payload.
If you think there's a
  |    good chance the payload types are fixed, you can
"hardcode" the
  |    dissectors as needed (for say header types 3-5) like for
what is
  |    currently done in the dissector.

I agree that 5 separate
dissector variations of PCLI is the way to go,
and the user selects which
variation that they want with "Decode As".
This is an orthogonal fix to how
pcli payload is decoded per (1) above,
so I'd address that in a separate
changeset.



  |    3. Not sure how you want to handle the UDP port
registration if you do
  |    have the 5 protocols.  They don't appear
"heuristically strong enough"
  |    to distinguish.  Perhaps the current
preference of picking a UDP port
  |    (which defaults to 0 anyway) should
just be removed in favor of
  |    complete Decode As functionality (with all 5
dissectors registered with
  |    dissector_add_for_decode_as).

Sounds like
that could be investigated as part of addressing (2)
above.



thanks,
Luke.

 
___________________________________________________________________________
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