Ronnie-
Thanks for the feedback. Obviously some of the suggestions will be
easier to implement than others -- I've already modified it so that
offsets are passed by value, not address, and the style-related changes
are simple as well.
Are these all must-haves for this protocol to be considered for
inclusion? In particular, I realize that the overuse of
proto_tree_add_text() isn't ideal and had noted it as such in my
comments in packet-dis.c. However, I believe that in its current state,
the dissector is already very useful to the simulation community, and
adding it to the source tree would provide a mechanism to begin making
your suggested changes as well as extending the dissector to cover
additional PDU types as needed by other users.
Thanks,
Jeremy Ouellette
Scientific Research Corporation
-----Original Message-----
From: ethereal-dev-bounces@xxxxxxxxxxxx
[mailto:ethereal-dev-bounces@xxxxxxxxxxxx] On Behalf Of ronnie sahlberg
Sent: Thursday, April 07, 2005 4:58 AM
To: Ethereal development
Subject: [Ethereal-dev] Re: DIS dissector
Hi,
Comments:
1,
You should not use that DIS_BitMask structure.
You should instead use
explicit true_false_string and FT_BOOLEAN if it is a boolean or
value_string if it is not a boolean.
This makes the filter gui much nicer.
See packet-ip.c for examples on how to do this.
2,
All these DIS_ParserNodes should also have their own
value_string associated with it and the corresponding hf_field should
have a VALS(that_value_string) associated with it.
See multiple examples on how to use VALS and value_strings in the code.
See also examples for how to use val_to_str() to retreive the string
from a value_string if you want to put it in the info column or
something.
Again, having them as proper value_string's will make the filter giu
much nicer.
3, dont use a pointer to offset.
All other dissectors use offset explicitely.
Helper functions in dissector take offset as a parameter and return
the new value for offset.
offset = call_some_helper( ... , offset, ...);
Change it to be consistent with the rest of ethereal so that it will
be easier to maintain.
4, you use too many proto_tree_add_text() change all these into
proto_tree_add_item() and define proper hf fields for them so that
filtering will work.
this will make the dissectyor much better.
You wont need to do silly things such as parseField_Int either. these
will go away when using proto_tree_add_item properly.
5,
change all the places where you do
type* variable;
into
type *variable;
to be more consistent with ethereal coding style.
if you can do these changes which are likely to be very intrusive on
the patch it will be much easier to review it.
On Tue, 05 Apr 2005 11:15:32 -0400, "Ouellette, Jeremy J
(N-Scientific Research Corp)" <jeremy.j.ouellette@xxxxxxxx> wrote:
> I've attached an Ethereal submission to dissect Distributed
Interactive
> Simulation (DIS) packets, as defined in IEEE-1278. I've done my best
to
> follow the Ethereal coding guidelines as I understand them. The patch
> file was generated against revision 14018 in the svn repository (the
> latest as of a few minutes ago). The packet-dis.c header text
contains
> a few comments on what's currently included and some ideas for
> enhancement / next steps.
> Also included in the tarball is a sample capture. Note that the UDP
> port for the DIS packets in this capture is 3001 instead of the
default
> 3000, so the correct port must be specified in the DIS preferences to
> properly view this capture.
>
> Jeremy Ouellette
> Scientific Research Corporation
>
> <<DIS_dissector.tgz>>
>
>
_______________________________________________
Ethereal-dev mailing list
Ethereal-dev@xxxxxxxxxxxx
http://www.ethereal.com/mailman/listinfo/ethereal-dev
