[Scamper-dev] DW's code review

[Matthew Luckie]

i've put a new version of scamper up at
http://voodoo.cs.waikato.ac.nz/~mjl12/scamper-0.9.tar.gz

the biggest flaw in the code that i see - one of the last points in this
code review - is the use of real time when the clock jumps forward or
back, to figure out

  when to send another probe

  the time elapsed between sending a probe and receiving an ICMP response

this could be fixed if i could figure out a way to get the uptime of a
machine.  the method used in /usr/src/usr.bin/w/w.c is flawed.  I could
write a kernel syscall module to allow me access to getnanouptime(9), but
that's not very portable, if that is a concern.

pointers?

while i'm on the portability subject, this code compiles and runs on linux
as well.

the other thing I added was a continuous probing option.  the address list
is read into memory at start.  scamper will re-read the address list if it
is sent a HUP, but not until it has finished with the current list.  this
is by design, but can be changed.

here's how i addressed each of the code review points:

> 	- storage of IPv4/6 addresses as void *'s is a little odd.
> 	Seems like might be better to use sockaddr_storage (but
> 	that could waste some memory).  You actually do use
> 	sockaddr's in some places, which leads to duplicate code.

using sockaddr_storage seems a waste of memory.  all i need to know about
is the type of addresses stored, which is available from
trace->addr->sa_family.

> 	- maybe use wrapper for malloc that exits/aborts upon failure
> 	so you don't need an if statement after every malloc().
>
> 		x = malloc(len);
> 		if (x == NULL) {
> 			print error;
> 			return;
> 		}
> 		bzero(x, len);
> 	but you can simplify that a lot with a wrapper to calloc:
> 		x = my_calloc(1, len, "error message");
> 	so if the underlying calloc() fails, you print "error message"
> 	and exit the program.

i've followed that suggestion.  malloc_wrap.

> 	- bzero/bcopy/bcmp may not be as portable/preferable as
> memset/memcpy/memcmp

i've replaced them as suggested.

> from_binary1.c:
>
> 	- I guess you don't want the binary files to be portable
> 	across CPU architectures.   Otherwise, write multi-byte
> 	data with htonl() etc.

done

> 	- read_address()
> 		- 'byte' not initialized

fixed

> 		- fread() return value not checked.

fixed

> 		- under 'byte == 0x02' case, should pass 'array_count'
> 		to printerrror() instead of 'id'

fixed

> 	- main()
> 		- add sanity check on 'array_size' after you fread() it
> 		to make sure its not negative or huge.

fixed

> 		- the output loop is long enough be a separate function.

the output loop is quite a bit smaller now.  a separate function would
reduce main to { return from_binary1(); }

> 		- if(addr == 0x00) should be if(addr == NULL)

fixed

> 		- consider rearranging printfs like this:
> 			printf("%2d", i+1);
> 			...
> 			printf(" *");
> 			...
> 			printf(" %s", str);
> 			printf(" %d.03d ms", rtt/1000,rtt%1000);
> 			printf("%s", stop_code);
> 			printf("\n");

good suggestion.  made the output loop smaller.  fixed.

> mjl_sockaddr.c:
>
> 	- sockaddr_tostr()
> 		- you seem to have an extra buffer copy (snmprintf()).
> 		why not just pass 'buf' to inet_ntop()?

fixed

> 	- sockaddr_in_cmp()
> 		- treating 'sin_addr.s_addr' as an int is probably
> 		not portable between big/little-endian systems.
> 	- sockaddr_in6_cmp()
> 		- same deal

the purpose of these functions was for use in sorting addresses for
finding purposes, not for numerical order.  i've put a comment in the code
to make this clear.

> 	- sockaddr_cmp()
> 		- comments state that function returns zero if
> 		sockaddr's aren't compatible.  I interpret that to
> 		mean belonging to different address families.
> 		However, the code actually compares sa_family.

fixed this function up with asserts, and fixed the comments to be more
precise.

> to_binary1.c:
>
> 	- in the comments that document the file format, mention
> 	that files are byte-order-dependent and not portable.

fixed byte order.  what about big endian vs little endian?

> 	- do you want to check the fwrite() return values?

yes, i've written a function fwrite_wrap that makes sure the write
succeeded, if not then we exit like in malloc.

> 	- hash_function()
> 		- this function seems to rely on (or assume) that
> 		the external 'hash_size' value is equal to or less
> 		than 65536.  Perhaps add an assertion to remind the
> 		coder about this:
> 			assert(hash >= 0);
> 			assert(hash <= hash_size);
> 			return hash;

hash is an unsigned 16 bit integer, so those asserts shouldn't really be
necessary, but i've put them in.  the hash function is still nasty

> 	- write_addresses()
> 		- seems like 'addresses_seen++' belongs in add_address()

fixed

> 		- constantly seeking to the beginning of the file
> 		to write the array strikes me as odd.  And really,
> 		you probably don't even need to know the array size.
> 		you could just automatically grow the array as
> 		needed in from_binary.

then the question becomes how much should i grow the array by each time.
if I'm going to need a large array, i'd like to know that at the outset.
if constantly writing addresses_seen is a big issue, then i could just do
it when closing the file...
the to_binary1 and from_binary1 files are merely supposed to be some kind
of concept proof.

> utils.c:
>
> 	- addr_cmp() seems largely redundant with
> 	mjl_sockaddr::sockaddr_cmp() (except that the arguments are
> 	of different types).  Better to combine them somehow?

combined, moved into mjl_sockaddr.c

> 	- gettimeofday_wrap():
> 		- I think gettimeofday() on some (maybe really old?)
> 		operating systems only takes one argument?

portability will be easier in these cases with gettimeofday_wrap.

> 	- is_zero_address()
> 		- assuming else case is AF_INET6 seems dangerous.

put an assert in there.

> scamper.h:
>
> 	- add comment describing what 'hop.tx' and 'hop.rx' are
> 	for?

done.  put a lot more comments in scamper.h as well.

> scamper.c:
>
> 	- I find it a little odd that "external" functions like
> 	to_binary1() are calling trace_destroy, but I understand
> 	the motivation.  You probably could have tricked simply by
> 	adding an appropriately named function in between, like:
>
> 	 void printer_is_done(trace *trace)
> 	 {
> 		 trace_destroy(trace);
> 	 }

the intention was to allow the output routine to deal with recording the
output in it's own time without blocking the main scamper loop.  this
might be useful if we record traceroute results to a postgresql database
across the other side of the world.

granted, this isn't what i do, but i'd like to keep this odditity.

> 	- trace_alloc():
> 		- I am surprised to see the 'gettimeofday(trace->begin)'
> 		call here because this function is really only about
> 		allocating memory.  seems like it belongs in scamper()
> 		right after the call to trace_alloc() instead.

it does other things as well other than just mallocs.  the goal was to
keep the scamper loop as clutter free as possible.

> 	- send_probe6():
> 		- here and other places I see things like this:
> 			socklen_t len;
> 			len = sizeof(struct sockaddr_in6);
> 			sendto(..., sin6, len);
> 		this seems overly complex.  why not just:
> 			sendto(..., sin6, sizeof(sin6));

i do that to keep function calls from spilling over onto two lines where
possible.  that's my personal coding style preference, but i'm not
attached too much to it.

> 	- send_probe4():
> 		- a lot of duplicate code with send_probe6().  there should
> 		be a way to combine them.

i agree, but the function would simply become an if(sa->sa_family ==
AF_INET) with maybe 5 or 6 shared lines.

> 	- send_probe():
> 		- why the ugly bcopy() of tv?  why not just:
> 			trace->hops[i].tx_time[attempt] = tv;

i never did understand unaligned accesses on the alpha, so i live in fear.

> 	- rtt():
> 		- seems like something that should be in utils.c.

done

> 	- recv_icmp6():
>
> 		- why iov[2] when you only use iov[0]?
> 		- why use msgget() anyway, since you only use one
> 		iov buffer?  looks like recvfrom() would be so much
> 		simpler.

once upon a time i was going to use msg_control for something, don't
recall what, but I'm using recvfrom now.  thanks.

> 	- get_next_address():
>
> 		- I think this:
> 			sscanf(line, ...) != -1)
> 		should be:
> 			sscanf(line, ...) == 1)
> 		(what if sscanf returns 0?)

i've replaced the sscanf with a strsep, the sscanf was a stupid idea.

> 		- what does getaddrinfo() give you since the address
> 		is already numeric?  why not just use inet_pton()?

there is no requirement for the address to be a particular format -
PF_UNSPEC for example.  AFAIK inet_ntop does not take PF_UNSPEC

> 	- open_sockets4():
> 		- I find the SO_SNDBUF, sizeof(probe) very odd since
> 		probe is such a tiny structure.

my understanding is that it causes a packet to be sent when the buffer
becomes full.  i took that hint from traceroute, but i'm not convinced it
is necessary either.

> 		- 'sin4.sin_len = sizeof(sin4)' is not portable.

fixed

> 	- find_trace():
> 		- would like to see an assertion that traces != NULL;

that would introduce a bug if i got an ICMP response but had no active
traces at that time.

> 		- you use bcmp() but have comparison functions
> 		elsewhere.

fixed

> 	- trace_is_done():
> 		- would like to see an assertion that trace != NULL;

done

> 	- is_loop_detected():
> 		- would like to see an assertion that trace != NULL;

done

> 	- handle_icmp():
> 		- this function is long enough to be split up.

the place i'd split it would be where i compose the trace->reason, but
it doesn't really seem that necessary.  i thought about trying a
#define CASE(c,s) case c: trace->reason = s; break;
but that makes the code really ugly

> 		- there is no 'else' block for the case when
> 		resp->type != ICMP_UNREACH.  What is supposed to
> 		happen in those cases?

that was a bug.  well spotted.

> 	- handle_done_trace():
> 		- put a comment here that the output function frees
> 		the trace memory (unless you are going to rewrite
> 		the code).

done

> 	- scamper():
> 		- the 'timeval_add()' approach seems like it won't
> 		handle the situation well when the clock suddenly
> 		jumps forward or backward by large amounts of time.
> 		i.e., it would cause a sudden burst of packets or
> 		a large period of inactivity.

this is unresolved.  comments appreciated.

re: friday's code review call:
i've put more friendly usage guidelines in where the parameters supplied
are out of bounds.

i've got notes here regarding the following items, but i don't recall the
problems

 - scamper returning -1
 - usage of fgets

Matthew