[bugzilla-daemon@xxxxxxxxxxxxx]

https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=2688





--- Comment #19 from Travis Dawson <travis.dawson@xxxxxxxxxx>  2008-08-28 20:02:47 PDT ---
(In reply to comment #18)
> (In reply to comment #16)
> > > - More (sub-)functions, please!
> > Not really a need to do this much, but I will put some of the contact tree
> > stuff in subfunctions.

I am working on adding some subfunctions to take care of repeated stuff.

> I'm not sure I get your comment.  My point was that you've got some mighty big
> functions in there.  Maybe it looks worse than it is due to the indenting
> problem you had before?
> 
> > > - I noticed a couple cases where the values "IPV4" or "IPV6" were used instead
> > > of the #defines
> > IPV4 and IPv6 are defines.

Fixed 

> > > - by the time you get to this code:
> > > 
> > >         if (!initialized)               {
> > >                 mojito_handle = create_dissector_handle(dissect_mojito,
> > > proto_mojito);
> > >                 initialized = TRUE;
> > >         }               else                    {
> > >                 dissector_delete("udp.port", global_mojito_port,
> > > mojito_handle);
> > >         }
> > > 
> > >         dissector_add("udp.port", global_mojito_port, mojito_handle);
> > > 
> > > 'global_mojito_port' will already have the new value--so the delete won't work.
> > >  That's why you need a 2nd variable (check some of the other dissectors for
> > > examples.
> > 
> > I think I get it, can you point out a quick example so I know what it is
> > exactly.
> 
> One example is packet-beep.c 

Fixed this and added Hueristics for good measure :)


> 
> > > (That's all I had time for now; one other thing I didn't have time to finish
> > > looking at: it looks like there is only 1 possible subtree (ett_mojito) which
> > > makes for an awfully flat tree.  But maybe that's the way the protocol is?)
> > I have a bunch of subtree's under the main tree. 
> > I think that's what your asking. 
> 
> OK, I see that now.  But each branch in the tree "should" have its own 'ett'
> variable.  For example here:
> 
>         /*  Setup the Header Tree */
>         mojito_headertree = proto_item_add_subtree(mojito_tree_temp,
> ett_mojito);
> 
> you're reusing ett_mojito for the Header tree.  That's not a big deal but it
> means that if you open the Mojito tree (but not the Header tree) and then move
> to another packet all the subtrees you have created will be opened.  If you had
> multiple ett_ variables the user would get the same view (same trees
> expanded/opened) when going from packet to packet.

I like that behavior. If someone complains (its not policy or something) I will
change it, but I prefer that behavior actually.



-- 
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.