Ethereal-dev: Re: [ethereal-dev] Patches containing assert statements (& not much else)
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.
.tgz is a gzipped tar file, .swp is a vim state file. The etherXXXX files
are in the current directory because I have copied them there
to use as test files. They are actually very small. (/tmp has considerably
more room than my humble home directory).
My point is that they do not change (meaningfully) and that nobody
but me would be interested in them.
> In principle cvs ought to pick those files and those files only which
> need to be 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.)
That is more or less what I mean. If you find warnings of the kind that
between us we have characterised useful then you must reject this
patch, as it will specifically switch them off.
> 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".
You can scroll those lines off the top of the screen and still see
where you are.
If you don't want it, you needn't take it!
> Note: this file may need major surgery to improve
> the dissector selection (see similar code in packet-tcp.c).
The current UDP sub-dissector-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?
My point was that it was silly my suggesting changes to that part of the
file whilst it seems to be being changed by experts. I'm grateful for the extra
words. Incidentally WAP uses ports in the range 9201 to 9208 or thereabouts.
I was intending to register a distinctive descriptive name for each required
port. Is this wrong & would you prefer ports to be registered in a block.
> 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++.
In preference to initialising them to an indeterminate bit pattern! I use
0 to indicate that pointer has been used and freed. I thought that I was
following the model in the developer document
+ 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.
Possibly. My practice when I find an assertion statement to have been
triggered is to locate the locate point in the code at which the errant
path might have been detected and place an assertion statement there.
My code often contains many assertion statements. Few are there by rote,
most mark the grave of some bug or other. My policy is to make my code
as bug unfriendly as possible.
I'm grateful for your attention.
Ben.
--
Leedsnet - The information resource for Leeds and the West Riding
< URL:http://www.leedsnet.com/mobile/ >
