Wireshark-dev: Re: [Wireshark-dev] Another one with dissector questions
Date Index · Thread Index · Other Months · All Mailing Lists
Date Prev · Date Next · Thread Prev · Thread Next
From: Jaap Keuter <jaap.keuter@xxxxxxxxx>
Date: Thu, 24 Apr 2008 18:54:32 +0200
Hi,
A fix is already in, see http://anonsvn.wireshark.org/viewvc/index.py/trunk/epan/conversation.c?r1=24661&r2=25158 

Thanx,
Jaap

Jens Steinhauser wrote:

The attached patch fixes conversation_lookup_hashtable().

Thanks,
Jens

On Wed, 2008-04-23 at 22:17 +0200, Jaap Keuter wrote:

Hi,

Yes, I think you're on the right track here.
Concerning the conversation search, I think you've a point. When searching for a conversation along the time axis, you shouldn't get the a conversation before the first one is established.
I'm not aware if many dissectors use conversations that way and this is a corner case. That may be why it wasn't spotted before.
A simple fix for your code is to check the returned conversation frame number against the current frames' number and discard it when it's older. Of course that should be done by the search routine, for which a change will be committed later. 

Thanx,
Jaap

Jens Steinhauser wrote:

Ok, I changed my dissector to use a conversation. The dissector creates
a new conversation for every configuration frame it finds and uses
conversation_add_proto_data() to save the information that is needed to
dissect the data frames. When it dissects a data frame, it uses
find_conversation() and conversation_get_data() to get the information
from the config frame. Is that the proper way?

But strange stuff happens when I have data frames before the first
configuration frame:

README.developer says (line 2741): "The conversation returned is where
                          (frame_num >= conversation->setup_frame
                        && frame_num < conversation->next->setup_frame)"
and (line 2723): "If no conversation is found, the routine will return a
NULL value."

It is my understanding that find_conversation() should return NULL for
the data frames before the configuration frames.

find_conversation() calls conversation_lookup_hashtable() to search for
the conversation (conversation.c, line 632):

match = g_hash_table_lookup(hashtable, &key);
if (match) { 
   for (conversation = match->next; conversation; conversation =
conversation->next) {
      if ((conversation->setup_frame <= frame_num)
       && (conversation->setup_frame > match->setup_frame))
         match = conversation;
   }
}
return match; 

The code doesn't work when frame_num is smaller than setup_frame of all
conversation in the linked list. The "for (...; conversation; ...)"
doesn't execute the body of the loop because conversation is null and
returns a conversation with setup_frame bigger than frame_num passed to
find_conversation(). I think that's a bug.

Thanks,
Jens


On Tue, 2008-04-22 at 15:58 +0200, Jaap Keuter wrote:

Hi,

1) You better use a conversation. Read README.developer on what
conversations are and how they are designed for specifically this purpose.
Using globals is never a good way to do this, especially when we intend to
multithread/work with multiple files.

2) ett's are datastructures for subtrees. They hold the expanded/collapsed
state for instance. You can reuse them, but then all these subtrees will
have the same expanded/collapsed state.

3) Remove all display filters and *disable packet coloring* (which is also
display filter based) to get tree==NULL.

Thanx,
Jaap


Hi,

I'm working on a dissector for about a month and some questions came up
when doing more than just basic dissection of the packets:

1) The protocol consists mainly of two different types of packets:
configuration frames and data frames. Configuration frames are send at
the beginning of a transmission or when the configuration of the sending
device changes and describe the content and format of data frames.
Without the knowledge out of the configuration frames, the data frames
can't be dissected.
  So I created a struct, called config_frame to save the information of
the configuration frames and put these config_frames into a global
GArray. This global array is (re)initialized in my dissectors init
function and pinfo->fd->num (also saved in config_frame) is used to
ensure that every configuration frame just adds one config_frame struct
to the array. When the dissector comes to a data frame, he searches
through that global array for a matching config_frame and uses it to
dissect the data frame. Is that the right way to do that?

2) What are the ett_... values for? Section 1.6.2 of README.developer
explains how to initialize and use them, but I didn't get what they are
good for. Is it save to pass the same ett_ variable multiple times? (The
data in the data frames is nested and I do that, didn't have a problem
so far)

3) To understand the sequence in that the "proto_register_...", the
"proto_reg_handoff_...", the init and the dissect functions are called,
I put the following at the beginning of the dissect function:

   printf("dissect(): %s\n", (!tree ? "*tree null" : "*tree valid"));

   and always get "*tree valid". Do i need a special configure switch to
enable the "Operational dissection" (enable something like a release
build)?

Thanks,
Jens