Wireshark-bugs: [Wireshark-bugs] [Bug 2688] Mojito Protocol Dissestor Plugin
Date: Tue, 26 Aug 2008 14:48:39 -0700 (PDT)
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=2688





--- Comment #18 from Jeff Morriss <jeff.morriss.ws@xxxxxxxxx>  2008-08-26 14:48:38 PDT ---
(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'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.

I meant these cases where the define is not used:

switch (socketaddressversion)                                                  
                {
        case 4: /* IPv4 */

> > - 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 

> > (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.


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