Software Style Guide
Global Variables
There is absolutely no good reason to have global variables for any of the projects. Global variable makes sense in some very limited scenarios, but not for any of the programs you have to develop for this class.
For example, consider the following ftp client that keep all data attributed to a session as global variables:
/* global state */ char hostname[MAX_HOSTNAME_LEN]; char username[MAX_USER_NAME_PASS]; char password[MAX_USER_NAME_PASS]; int port_number; int total_bytes_copied;
You might be able to build a functioning program using the above global variables. However, imagine you later wanted to implement multiple parallel sessions. You would have to go through all of your code and replace the global variables with session specific variables. A better way to implement it might be to have the session specific variables in a structure that is passed along. For example:
/* session state
*/
struct session_state_t {
char *hostname; /* source server name */
char *username; /* username for logging into the machine */
char *password; /* password for authentication */
short port_number; /* port number of ftp */
int bytes_copied; /* number of bytes copied */
};
[...]
void parse_cmd_line(struct session_state_t *info, int argc, char **argv)
{
[...]
This structure can now be passed to all functions that make use of this information. If you ever wanted to have several sessions in parallel each would get its own structure and no changes to the functions would be required.This example is also well commented and allows strings of any length (as opposed to fixed length strings).
Similarly, you may be able to construct a functional router using global variables. But, once you want to simulate a collection of routers, this strategy will not work. For these reasons and many others,all of your router state should be contained in a structure that you register as a subsystem with the provided sr instance.
Decomposition
When you are using very similar code in several functions, you are probably doing something wrong. Try to find an abstraction for the common functionality and put it into a seperate function. For example in the code below, both functions have almost exactly the same code.
/*--------------------------------------------
* get_host_dir
*--------------------------------------------
* pre : takes the socket, server_hostname, extension, localdir
* post: if fails, frees the server_hostname, extension, localdir
* otherwise, gets host directory and returns the dir on heap
*/
char *get_host_dir(int s, char *server_hostname, char *extension, char *localdir)
{
char *server_reply;
if(strlen(PWDSTR) != write(s, PWDSTR, strlen(PWDSTR)) ){
fprintf(stderr, "ERROR: failed while writing to server\n");
close(s);
free(server_hostname);
free(extension);
free(localdir);
exit(FAILURE);
}
server_reply = get_reply_from_server(s);
return server_reply;
}
/*---------------------------------------------
* log_off_server
*---------------------------------------------
* pre : takes the socket, server_hostname, extension, localdir
* post: if fails, frees the server_hostname, extension, localdir
* otherwise, sends a quit message to the host/server
*/
void log_off_server(int s, char *server_hostname, char *extension, char *localdir)
{
char *server_reply;
if(strlen(QUITSTR) != write(s, QUITSTR, strlen(QUITSTR)) ){
fprintf(stderr, "ERROR: failed while writing to server\n");
close(s);
free(server_hostname);
free(extension);
free(localdir);
exit(FAILURE);
}
server_reply = get_reply_from_server(s);
free(server_reply);
}
[...]
A better way would have been to encapsulate the writing of the command and the error handling in one function that takes the command as a parameter. The above code has a few other issues. It will abort if only a part of the string were written to the socket. The write() call will sometimes not write all characters in one pass but instead return with a partial write.
Readability
Below is an example of how decomposition can make a program much more readable. The function below sends an ARP reply packet. This function in many implementations is pretty hard to read as it contains a lot of code that just assigns the values to the different fields in the packet, reorders the bytes into network byte order, etc. This implementation here however is very readable.
/* -----------------------------
* send_arp_reply
* -----------------------------
* Takes an ARP request and the interface on which it was received.
* Creates the appropriate ARP reply and sends it out on the same
* interface.
*/
void send_arp_reply(struct sr_instance *sr, /* router state */
struct sr_vns_if *vns_if, /* interface */
struct sr_arphdr *req) /* ARP request */
{
uint8_t *buf;
int len;
/* allocate memory for ARP reply */
len = sizeof(struct sr_ethhdr) + sizeof(struct sr_arphdr);
buf = (uint8_t *) Malloc(len); /* malloc wrapper that checks return value and exits on failure */
/* build Ethernet and ARP headers */
build_eth_hdr((struct sr_ethhdr *) buf, ETHERTYPE_ARP, vns_if->addr, req->sha);
build_arp_hdr((struct sr_arphdr *) &buf[sizeof(struct sr_ethhdr)], ARP_REPLY, vns_if->addr, vns_if->ip, req->sha, req->sip);
/* display packet (for debugging) */
print_pkt(stdout, buf, len);
/* send packet (responsible for deallocating memory for buf) */
send_pkt(sr, buf, len, vns_if->name);
}
The reason it is so clean is that all of the packet assembly has been moved to the build_eth_hdr and build_arp_hdr functions that are in a separate packet assembly "library". This library is then used by several functions to manipulate packets. This decomposition makes the code readable and debugging much easier. Obviously, there are many other good ways of implementing this function. For example, it may be appropriate to allocate packet memory within the "build" functions, as they know exactly how much memory the packet will need. The above structure requires knowledge of packet length before building any headers. In the case of an ARP reply, this is okay, but for other types of packets, it may be a problem.