Wireshark · Wireshark-dev: Re: [Wireshark-dev] Another one with dissector questions

[Jens Steinhauser <jens.steinhauser@xxxxxxxxxx>]

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
> >>>
> _______________________________________________
> Wireshark-dev mailing list
> Wireshark-dev@xxxxxxxxxxxxx
> http://www.wireshark.org/mailman/listinfo/wireshark-dev
-- 
*****************************************************************
Jens Steinhauser (Mr.), Software Dude
Tel.: +43 5523 507 422, Fax.: +43 5523 507 999
http://www.omicron.at/  jens.steinhauser AT omicron.at
*****************************************************************
OMICRON electronics GmbH, Oberes Ried 1
A-6833 Klaus / Austria
Company Registration No. FN 34227i, Commercial Court of Feldkirch
*****************************************************************

Index: epan/conversation.c
===================================================================
--- epan/conversation.c	(revision 25164)
+++ epan/conversation.c	(working copy)
@@ -615,7 +615,6 @@
 conversation_lookup_hashtable(GHashTable *hashtable, guint32 frame_num, address *addr1, address *addr2,
     port_type ptype, guint32 port1, guint32 port2)
 {
-	conversation_t* conversation;
 	conversation_t* match;
 	conversation_key key;
 
@@ -631,17 +630,13 @@
 
 	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;
-		}
-		if (match->setup_frame > frame_num)
-			match = NULL;
+	for (; match; match = match->next) {
+		if ((match->setup_frame <= frame_num) &&
+		    (frame_num < (match->next ? match->next->setup_frame : (guint32)-1)))
+			return match;
 	}
 
-	return match;
+	return NULL;
 }