Huge thanks to our Platinum Members Endace and LiveAction,
and our Silver Member Veeam, for supporting the Wireshark Foundation and project.

Wireshark-dev: Re: [Wireshark-dev] Extending PCLI payload decoding

From: Luke Mewburn <luke@xxxxxxxxxxx>
Date: Wed, 27 May 2015 00:16:24 +1000
On Mon, May 25, 2015 at 12:18:17AM -0400, mmann78@xxxxxxxxxxxx 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.

Attachment: pgp8xGYQ2_g5_.pgp
Description: PGP signature