Ethereal-dev: Re: [ethereal-dev] Patches containing assert statements (& not much else)
On Wed, Apr 12, 2000 at 09:17:52AM +0100, Ben Fowler wrote:
> I try to avoid making any changes to the generality of files in the
> project, because it only makes work for CVS and may confuse
> me; but sometimes it is desirable either to make life a little easier
> for me, or as an aid for debugging.
>
> I submit these patches for these reasons.
>
> .cvsignore - adding three file types which my system seems not
> to ignore by default.
The patch adds "*.swp", "etherXXXX*, and "*.tgz" files to the list; how
is CVS to have any clue what the first two are, at least, so that it
would know that they *should* be ignored by default? (What *is* a .swp
file?)
I assume the "etherXXXX" files show up in the current directory because
you're setting the temporary file directory for Ethereal to the current
directory, e.g. because neither "/tmp" nor "/var/tmp" have enough disk
space for large captures you take.
> In principle cvs ought to pick those files and those files only which
> seed to under cvs control.
How is CVS to know which files would go under CVS control, so that it
*could* pick them? (Or do you mean that it should ignore all files that
are not currently under CVS control? Perhaps it does not do so in order
to provide some warning, on, say, a "cvs diff", of files that you've
added to the tree but have not done a "cvs add" on; I have certainly
found warnings such as that quite useful in the past.)
> packet-udp.c - added comment to make some lengthy code
> easier to scan.
I'm not sure I'd consider the 16-line block of code that adds the few
fields in the UDP header "lengthy".
> Note: this file may need major surgery to improve
> the dissector selection (see similar code in packet-tcp.c).
The current UDP subdissector-selection code differs in form from that of
UDP only in that there are a few places where it still does explicit
testing of ports; three of them (RIP, NCP, and Vines) are done that way
because of the comments that imply that more should be done than just
checking one port, one of them (the RX/AFS stuff) is done that way
because it checks for a *range* of port numbers rather than for single
port numbers (that one, I guess, should perhaps be handled by a loop
that registers each of the ports in that range), and one of them (TFTP)
is done that way because only the first protocol in a TFTP conversation
necessarily has the TFTP port number in it (I think there are ways to do
this, but we haven't done that yet). What sort of "major surgery" are
you suggesting?
> proto.c/h - Added symbols and initialisations to enable assertions on
> field_info pointers. If you never make mistakes with pointers you don't
> need these.
- field_info *new_fi;
+ field_info *new_fi = FI_EMPTY;
...
+#define FI_EMPTY (field_info *)( -1 )
Why initialize them to a value with all bits set? NULL is the right
value to use here - it's the official "distinguished value" for pointers
in C and C++.
+ g_assert(hfindex >= 0 && hfindex < gpa_hfinfo->len);
hfinfo = proto_registrar_get_nth(hfindex);
"proto_registrar_get_nth()" makes the same check itself; I assume the
problem here that no core dump is produced, or that your debugger
doesn't produce a stack trace that shows the line at which
"proto_registrar_get_nth()" is called.
