[Scamper-dev] DW's code review

[Duane Wessels]

The code looks really good overall.  I didn't see any gaping security
problems or bugs.  Some of my comments may seem like nitpicks.

I have some comments about portability and readability (i.e., things
that would have helped me understand something faster).  I do not
really have any comments related to the performance of this code.

Keep in mind that I've never run or used skitter before so some of
the techniques are new to me.  Also, of course, these are just my
suggestions.   You can take them or leave them.

overall:

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

	- maybe use wrapper for malloc that exits/aborts upon failure
	so you don't need an if statement after every malloc().

	- many places have code like this:
		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.

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

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.

	- read_address()
		- 'byte' not initialized
		- fread() return value not checked.
		- under 'byte == 0x02' case, should pass 'array_count'
		to printerrror() instead of 'id'

	- main()
		- add sanity check on 'array_size' after you fread() it
		to make sure its not negative or huge.
		- the output loop is long enough be a separate function.
		- if(addr == 0x00) should be if(addr == NULL)
		- 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");

mjl_sockaddr.c:

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

	- 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

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

to_binary1.c:

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

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

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

	- write_addresses()
		- seems like 'addresses_seen++' belongs in add_address()
		- 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.

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?

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

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

scamper.h:

	- add comment describing what 'hop.tx' and 'hop.rx' are
	for?

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);
	 }

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

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

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

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

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

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


	- get_next_address():

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

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

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

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

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

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

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

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

	- handle_icmp():
		- this function is long enough to be split up.
		- there is no 'else' block for the case when
		resp->type != ICMP_UNREACH.  What is supposed to
		happen in those cases?

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

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