[Jaap Keuter <jaap.keuter@xxxxxxxxx>]

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