Network programming @ Noppa | @ wiki - Updated: — Jussi Laakkonen 2012/09/27 20:11

Comments about assignments

Student Comments
0312399 Very thorough readme-file. Nice division of code and reusability has been thought. Lots of comments, in future commenting parts of the code on one line would enhance readability. The message content should always be as characters (not integers of any length) as the assignment1 protocol states → this kind of usage breaks protocol compatibility. Good quit handling in server. Some variables storing user input or other data are not initialized in any way. XML files as news headers are a real overkill here. Remember that int = 32bit integer, short = 16bit integer, in few places these seem to mix up. Terminating NULLs for strings do not seem to be properly handled or set. Using length returned by strlen() for copying is dangerous (overflow). AI_PASSIVE is meant for server only (bind), no need to use it in client. Partial sends are not handled.
0316023 Good division of code. Packing functions do work – if the endianess of the operating system is checked first – it is recommended to use htons()/ntohs()/etc for changing the order of the bytes for integers. Partial data is not received or sent, nor properly checked. No date string is sent. Protocol not fully followed, there is no quit message, no calculation of average delay, additional messages? Empty lines sent sometimes as news items. No client register checks – even for correct packet types. No user input in client. Lots of errors with valgrind.
0342246 Implicit declaration warnings, check the header usage. Ok division of code. Protocol not implemented. Partial data is not received or sent, nor properly checked. No user input. Lots of stuff missing.
0342327 Invalid binary name. Ok division of code but use more indentation as it makes code more readable. Consider using fgets() for reading user input as it is more safe. The length variable in protocol messages is not properly utilized – check the payload length! Partial data is not received or sent, nor properly checked. Lengths in the sent packets seem to be calculated wrong, use strlen() to check charstring length instead of sizeof() and use these for the data that is actually sent. Remember to initialize variables before use and also remember to use/check terminating NULLs in charstrings. A rather interesting structure in server; a change with select() usage for checking both listening socket and the client socket would make it more extensible (and check returnvalue). News header is static. Cannot serve clients in row if client is disonnected prematurely; disconnection detection is not implemented. Can produce a “Floating point exception (core dumped)” if client is quit before receiving a single message. Client leaks memory.
0358454 Invalid binary name. Good division of code. Partial data is not received or sent, nor properly checked. A rather interesting structure in server; a change with select() usage for checking both listening socket and the client socket would make it more extensible (and check returnvalue). Remember to use/check terminating NULLs in charstrings. The length variable in protocol messages is not properly utilized – check the payload length! Server can serve clients in a row – good. Client leaks memory.
0358674 does not compile
0377037 Good division of code. A good way to buffer incoming data per client basis. On first attempt client reports invalid message and shuts down, second try makes the server printout invalid argument, after server is shut down client crashes. Leaks memory. Server cannot deliver anything to client and vice versa? Why does the server print to stdout every millisecond? Partial data receiving and sending would work when looking the code but does not seem to be working.
0378227 LOTS of compile warnings which might implicate poor functionality. Invalid binary name. Does not follow given input parameter format and gives wrong instructions for parameter usage? Protocol packet format is not followed, check the assignment specification. Why htons() is used with each character of a charstring in protocol packets? Partial data send / receive not handled. Packet length not utilized. Code is a real mess, hard to understand what is happening. Time is hardcoded? No news header implementation. Time sent only once? Valgrind errors. No avg. delay calculations. Client does not handle server disconnection.
0404450 Good division of code. Remember to add name & student number to files, incl. readme. Compile warnings. Partial send and receive handled nicely and terminating NULLs dealt with – good. Quitting handled well in server. Consider using fgets() for user input. Valgrind errors on server. Client crashes when quitting (segfault).
0404463 Good division of code, a const length warning in compilation. Add more comments to code. Protocol packets formed in odd way, 16bit stored in 32bit from which 2 bytes are copied, remember the ordering of bytes in big and little endian; safer way to use exact sized int variables. Client does not do a thing and neither does the server. Protocol not implemented, nothing sent between client and server.
0404476 Good division of code. Nothing really implemented, client sends something not described in the protocol. Invalid binary name. Client leaks memory.
0404489 Good division of code. Partial sends handled but partial receiving is not dealt with. Multiuser support lacks some checks, third client does not get anything (not a negative in this first as it was not a requirement). Good implementation.
0404531 Good division of code. write() for sockets does work but consider using send() as you can pass TCP flags with it. Partial send or receive of data is not dealt with. Client does not start – segfaults, cannot test. Server leaks memory.
0404544 Division of code into multiple files would increase readability a bit. Partial sends handled well, put partial receiving is not. Remember that htons()/ntohs()/etc. return integers as unsigned. Valgrind errors in both. Server “freaks out” when at some point client quits. Clients drop from server after a while. Server leaks lots of memory.
0404557 Lots of compile warnings which might implicate poor functionality. Ok code division but consider dividing the large functions into smaller pieces as it makes them easier to debug. Also using parameter names in English would help others who look the code. Sending (and receiving) the packet in small pieces is ok with TCP if you always check that everything is sent. This might result in scenario where client actually receives data in smaller pieces. See the packing approach from the wiki pages. Valgrind errors. Server actually froze the computer completely when run with valgrind. Server reacts to client only after the client has quit and after that “broken pipe” and it shuts down.

Assignment1: Notes

These pages will contain notes about the first assignment including some corrections and ways how to do things correctly.

Most of these are from last year but they still apply as quidelines.

Utilizing code examples and tutorials

It is good if you try to find other examples than mentioned on course pages and state them as your sources. However just stating your source does not mean that you can copy the code directly from the example as it is required that the work is your own. But there aren't really many ways to do simple stuff, like getaddrinfo() usage, so this kind of simple things are usually similar than the example you have been looking. But if the whole code is too similar to examples with minor changes (changing variable names) you'll balancing on the edge of the blade.

Makefiles

COMPILER=gcc
CFLAGS=-Wall -std=gnu99
#List sources in separate makefile variables (separate sourcefiles with spaces)
#For client 
SOURCES1=yahtzeeclient.c yahtzeecommon.c
#For server
SOURCES2=yahtzeeserver.c yahtzeecommon.c
 
#Client
OUTPUT1=yahtzeeclient
#Server
OUTPUT2=yahtzeeserver
 
#Multiple lines can be added into one makefile rule
#with 'make build' both server and client would be compile.
#Use one compilation per line in these exercises.
build:
	$(COMPILER) $(CFLAGS) $(SOURCES1) -o $(OUTPUT1)
	$(COMPILER) $(CFLAGS) $(SOURCES2) -o $(OUTPUT2)

Calling make build would result in:

gcc -Wall -std=gnu99 yahtzeeclient.c yahtzeecommon.c -o yahtzeeclient
gcc -Wall -std=gnu99 yahtzeeserver.c yahtzeecommon.c -o yahtzeeserver

Endianess

Remember that the network order concerns ONLY the multi byte variables, i.e. variables which are longer than 1 byte. One 8bit integer is the same length as one character, which is not expected to be in any other form. With 16/32/64bit integers for instance the byte ordering (big endian - network order vs. little endian - e.g. in linux) is significant as the order of bytes defines the value of the variable and if the other end is not using the same byte order as the sender - the received value is interpreted as different value as the sender intended. This is the reason network order is defined to follow a certain byte order: big endian. With htons/ntohs/htonl/ntohl() you can change the byte order of your variables, see the manual pages of these functions.

E.g. if you have a value 255 in a 16bit integer, the binary form in little endian would be:

1111 1111 0000 0000

And in big endian (network order after htons()):

0000 0000 1111 1111

Which are the same number but with different endianess.

If you use only bit shifting to get the bytes of, e.g. 16bit integer into two 8bit integers, it restricts the usage of the application as all applications should then do the reverse operations based on system architecture (endianess), especially if you are not converting the integers into network order before the bit shifting. If some other applications following the same protocol handle the protocol packets differently and expect the, e.g. 16bit integer to be in network order and use internal functions to convert it to host endianess there will be problems as the order of bytes might change. Especially when endianess is different on sender and receiver.

Following the protocol

When the protocol is defined:

8 bit integer x char data
6 data
char buffer[255];
memset(&buffer,0,255);
uint8_t ppid = 6;
int position = 0;
 
//No endianess conversion as dealing with one byte
*(uint8_t*)&buffer[0] = ppid; 
//Copy some data from 'inputdata' after the integer
position += sizeof(ppid);
memcpy(&buffer[position],inputdata,200);
16 bit integer x char data
6
char buffer[255];
memset(&buffer,0,255);
uint16_t ppid = 6;
int position = 0;
 
//Convert 16 bit integer into network order
*(uint16_t*)&buffer[0] = htons(ppid); 
//Copy some data from 'inputdata' after the integer
position += sizeof(ppid);
memcpy(&buffer[position],inputdata,200);
char (id) x char data
6
char buffer[255];
memset(&buffer,0,255);
char ppid = '6';
int position = 0;
 
//Just put the character into buffer
buffer[0] = ppid;
//Copy some data from 'inputdata' after the character
position += sizeof(ppid);
memcpy(&buffer[position],inputdata,200);
 
//or with snprintf
snprintf(buffer,255,"%u%s",ppid,inputdata);

One major difference here is that when e.g. value 156 has to be sent over network, it takes:

  • 1 byte as unsigned 8bit integer
  • 2 bytes as unsigned 16bit integer
  • 3 bytes when sent as character digits

In UDP one write to socket, e.g. with sendto() equals one UDP packet! If you try to write a packet with, e.g. 3 parameters with separate calls to sendto() it will result in 3 different packets sent to the receiver. If the protocol states that one message consists of these three parameters then you are not following the protocol.

Remember the connection between characters and 8bit integers, test the following:

char buf[15];
memset(&buf,0,15);
uint8_t value_of_ASCII_A = 65; 
for(uint8_t i = 0; i < 15; i++) buf[i] = value_of_ASCII_A + i;
printf("buf: %s\n",buf);

System calls

It would be a good way to always check and react to the return value of system calls (e.g. select() socket() bind() … etc.) as they might not always succeed. Especially with proper usage of select() as the return value tells whether there are errors, timeout or data on the reported amount of sockets.

Another thing is that most of the system calls are blocking (see manual pages to check this on the calls you're using). This means that the execution of code continues after the called function gets some event (e.g. recvfrom() gets incoming data).

Using #define preprocessor macros

If some constant number is used widely among your code it might be easier to define it with #define preprocessor macro. E.g.

#define FULL_HOUSE 8
#define SMALL_STRAIGHT 9
...

Then it would be easy to access e.g. the fields of Yahtzee table by:

score_sheet->values[FULL_HOUSE] = 25;

without remembering the actual integer value.

The preprocessor macros are also useful when including headers, e.g:

/* If __HEADER_FILE_H_ (e.g. name of this header in capital letters) is not defined
   use the next lines until #else or #elif or #endif
*/
#ifndef __HEADER_FILE_H_
/* Define the header name , include stdio.h and declare function */
#define __HEADER_FILE_H_
#include <stdio.h>
void funcion(int input);
#endif

as the block inside #ifndef and #endif is included while compiling only if the first #ifndef is true, i.e. the file is included for the first time in the compilation process.

Documenting work aka commenting

A nice example about how to document ones work:

// Loop through all the values setting field ids
	// and values
	for (i = 0; i<13; i++)
	{
		// Set the field id to be the current number
		(*score_sheet).upper_values[i].field_id = i;
		// Set all the points to be ASCII a (as in available)
		(*score_sheet).upper_values[i].points = available;
	}
 
	// Loop through the lower values and set them straight
	for (i = 0; i < 3; i++)
	{
		// The id_value is current number
		(*score_sheet).lower_values[i].field_id = i;
		// And set the points to be ASCII a (available)
		(*score_sheet).lower_values[i].points = available;
	}

AND COMMENT IN ENGLISH, PLEASE! On this course the only languages that are acceptable are: c and English. It is also recommended to use variable names that the assistant and the lecturer can understand.

User input validation

Remember to always sanitize user input, either removing unwanted characters or discarding the invalid input. Both parameter and input from UI should be checked. Invalid input should not crash your application.

Checking that the sender is the same as earlier

Use the address structures that are filled with receiver address information by system calls (e.g. recvfrom()) to check if the sender of data is the same as earlier (or to match the server to one of the, e.g. clients in your database). To compare the data you can do the following (example with IPv4 address structure):

int CheckClient(struct sockaddr_in * original, struct sockaddr_in * newcomer)
{
  // To check that the port is the same
  if(original->sin_port != newcomer->sin_port) return -1;
  // To check that the address is the same
  if(original->sin_addr.s_addr != newcomer->sin_addr.s_addr) return -1;
 
  return 0;
}