meta data for this page


This shows you the differences between two versions of the page.

Link to this comparison view

Next revision
Previous revision
courses:ct30a5000:assignment2notes [2011/10/19 12:23]
julaakko created
courses:ct30a5000:assignment2notes [2012/11/06 20:42] (current)
julaakko [Comments]
Line 1: Line 1:
 ====== Assignment2:​ Notes ====== ====== Assignment2:​ Notes ======
-These pages will contain notes about the first assignment including some corrections and ways how to do things correctly.+These pages will contain notes about the second ​assignment including some corrections and ways how to do things correctly.
-===== Test your application! ​=====+===== Comments ​=====
-Check before send that your application ​compiles ​and runs with required parametersFollow also the given names for the program executables.+^ Student ^ Comments ^ 
 +| 0312399 | Ok division of code. Partial send should handle the possible error in sending, same for reading. For possible longer user input it would be good idea to check if fgets() stopped at newline marker (is there anything else in the buffers). Nice wey of tokenizing user input. With IPv6 support you should use struct sockaddr_storage instead of sockaddr_in → IPv6 support is insufficient. Server message handling can be blocked if client sends only 4 bytes → server keeps awaiting for more, consider implementing this completely with select and read what you can then process packets after they are fully received. The length of payload in protocol packets is not properly checked using the length-header-nickname. Invalid binary name. Only IPv6 addresses work with client. If client gives plain “/​connect” without any parameters it gets no response but when given next time with a message server reports that nickname is in use. Server does not seem to handle all premature disconnections since in previously mentioned situation the server replies that nickname is in use. If nickname is too long the message is sent to others. | 
 +| 0316023 | Ok division of code. Partial sends are not handled, remember to check how many bytes have been sent and is there need to send the rest. Partial receiving is not handled – the packet length Is not checked properly, you should always verify that we've received a full packet ​before ​processing and if not, receive the rest. The length is utilized well in packet parsing – good. For possible longer user input it would be good idea to check if fgets() stopped at newline marker (is there anything else in the buffers). Client can connect to server when already taken nick is replaced with new – nice. | 
 +| 0342246 | Adding more comments to code wouldn'​t hurt in the future. Good division of code. Nice way of handling partial data. Might be a good idea to differentiate between select() error (-1) and timeout (0). Quitting the application when less than header size is read is not really a good idea and with MSG_WAITALL the application might be vulnerable for DoS attacks – read what you can and if it is not enough try to read more next time data is detected on socket. Otherwise partial data reading/​writing is done well but errors with recv() and send() (-1) should be handled too. The nickname buffer is not padded with NULLs since it is not initialized at any point. Invalid binary name. Duplicate nicknames are not checked. Client can be crashed with invalid input (letters and numbers as port) | 
 +| 0342327 | Ok division of code but reading some guides about code indentation wouldn'​t hurt – hard to read this stuff. More comments would ease the reading. For possible longer user input it would be good idea to check if fgets() stopped at newline marker (is there anything else in the buffers). Remember to always check that correct amount of data is sent. Partial sending or receiving is not handled. Do not expect that you have more data awaiting in the network buffer – your approach will open possibilities for easy DoS attacks as server would block when only header is sent.  Lots of valgrind errors. Nickname change from client leaves a newline marker into nickname and server does not initialize the nickname list that is sent, each request increases the list that is sent. Client leaks memory. | 
 +| 0358454 | Good division of code.  For possible longer user input it would be good idea to check if fgets() stopped at newline marker (is there anything else in the buffers). Partial sending or receiving not handled, the approach using loop to peek the length will introduce problems at the second round since it starts writing to buf[0] at every round. You should always check that is the packet fully received before processing – also processing could be done after the reading of incoming data: separating these functions might bring flexibility. The packet length is converted to network order with htons() instead of htonl() → the result is 16bit instead of 32bit, this works in your application ​but does not work with others. You should make sure that nicknames have terminating NULL at the end especially when the nickname list is sent (nicks must be separated with NULLs as stated in specification). Client leaks memory. Client does not report connection error if e.g. port is wrong. | 
 +| 0358674 | Empty return. | 
 +| 0377037 | Ok division of code. Comments only here and there and usually stating the obvious – comment the non-obvious. Most parts are a real mess – hard to read. Partial sending is not handled, you should react to errors and send the rest if all cannot be written to buffers at once. With partial receiving you could read only the amount that is missing – it seems here that you can read more than one packet but it is not clear if the following packet is dealt properly (moved to front of the buffer for example). The chat message from client should not contain nickname. For possible longer user input it would be good idea to check if fgets() stopped at newline marker (is there anything else in the buffers). Lots of errors ​with valgrind (client)Leave the debug messages out from return, it is hard to see what actually is received. Client does not send the last character that was given via stdin. Nickname duplicates are not checked. Client can crash when server quits. Huge problems with memory usage – lots of memory leaks. Cannot follow what is sent to what client – seems like the messages are not properly handled as one client seems to get the same line again and again. | 
 +| 0378227 | Around 70 lines of warnings, would you really expect this to work? Why do you use obsolete functions like bzero()? Somewhat ok division of code. Lacks comments. Invalid binary name. You should react to errors returned by select(). Startup parameters are not followed, nickname cannot be given on command line and changing of nickname and chat messages are done with some other command – not following specification. IPv6 not supported. Valgrind errors. Server leaks memory as it cannot be closed with /quit. Client gets stuck at some point when just text is given via stdin without any command – standard chat messages should not require command as stated in specification. Some replies from server are sent really late or client reacts to these in very slow manner – hard to test as it clearly does not follow given specification. Partial sending or receiving not handled. Error packet length defined with 16bit instead of 32bit int. You should use htonl()/​ntohl() ​for 32bit integers – the htons()/​ntohs() are for 16bit. Nickname limited to 6 characters without padding? | 
 +| 0404450 | Compiler optimizations are not really needed here. Nice and logical division of code. Nice handling of partial sending and receiving. Some VG errors when the client cannot connect. | 
 +| 0404489 | Nice and logical division of code. Nice handling of partial sending and receiving. For possible longer user input it would be good idea to check if fgets() stopped at newline marker (is there anything else in the buffers). Server disconnects wrong client when duplicate nickname is sent from some other client, sometimes server crashes in this situation. Some memory leaks in server. | 
 +| 0404531 | Late return, hence 0. Good division of code and well documented. Client crashes with first command. Using write()/​read() for network sockets will work but with send()/​recv() the flags can be passed. Always check that correct amount is written and with reading do not expect that there will be data. | 
 +| 0404544 | Ok division of code but using multiple files would make it easier to read.  For possible longer user input it would be good idea to check if fgets() stopped at newline marker (is there anything else in the buffers). The amount of received data is not checked – you should not expect that you always get enough data. Partial receiving is not handled. Nicknames should be separated with NULLs (\0) instead of semicolon. You should set the data buffers to NULLs before sending since it might contain some garbage – this is because you're not padding the nickname with NULLs before putting it into buffer, look up the memset() function. Lots of Valgrind errors. Duplicate nicknames are not checked. Server does not inform clients when quitting. Lots of leaking memory. | 
 +| 0404557 | Lots of implicit declaration errors, see manual pages of those functions and use the headers mentioned there. Ok division of code but usage of more functions will provide flexibility to code. Partial receiving or sending not handled – you should always check how much has been sent or received. Valgrind errors. Nothing really works with multiple clients, only messages from first client reaches the server. Do not expect that clients send data immediately after connecting, after the client is accepted wait for select to tell if the new socket (not the server listening socket) contains data available for reading. Read what you can in one recv() and check if you got a full packet ready to be processed, if not, read the rest when it arrives and process it later – this would require some buffering of data for each client. You should at least initialize all variables, especially buffers, before use |
-Check also that the application works! If some feature cannot be tested or it is not working at all it will not be counted as a feature! So; make sure that the features are //a) documented in README// ​and //b) working//.+===== Making sure that all data is sent and received with TCP =====
 +A example when all is sent in one call, e.g. in client ([[http://​​guide/​bgnet/​output/​html/​multipage/​advanced.html#​sendall|another one]]):
 +<code c>
 +int write_to_socket(int sockfd, char* data, unsigned int data_len, unsigned int *data_written)
 +  int wrote = 0;
 +  ​
 +  // Initialize the pointer value to 0
 +  *data_written = 0;
 +  // While we have something to write
 +  while(*data_written < data_len)
 +  {
 +    // Write remaining data
 +    if((wrote = send(sockfd,&​data[*data_written],​data_len - *data_written,​0)) < 0) return -1;
-===== Remember to drop membership from multicast =====+    // Update the written amount for pointer 
 +    *data_written +wrote; 
 +  } 
 +  return 1; 
-Always when you join to multicast group you must tell the kernel also to leave from the multicast so the switches handling the groups know that. The kernel informs the switches about this when you call <code c>​setsockopt (socket, IPPROTO_IP, IP_DROP_MEMBERSHIP,​ &mreq, sizeof(mreq));</​code>​ 
-===== FOLLOW THE PROTOCOL =====+For receiving data you have to use a different approach, especially with multiple connections. If the data is not received in one ''​recv''​ call keep reading until the full packet is completely read. One example: 
 +<code c>
-The [[assignment1notes|notes from assignment ​1]] presented ways how to implement ​protocol that is written in different waysFOLLOW ITIn next ones (especially 4thif the protocol ​is not followed then it is 0 points+int read_from_socket(int sockfd,​char* buffer, unsigned int total, unsigned int *remaining) 
 +  do 
 +  { 
 +    // Read 0 or less -> we're done here, return -or 0 
 +    if((data_read = recv(sockfd,&​buffer[total-*remaining],​*remaining)) < 1) return data_read;​  
 +    // otherwise update the remaining amount 
 +    else *remaining -= data_read;​ 
 +  } while (*remaining > 0) 
 +  return 1; //ok 
 +Just keep in mind that when designing the reading approach for a server it is good way to balance the load by reading only certain amount of data per client and then checking if it is fully received. If not, proceed to the next client and so on. 
 +A {{:​courses:​ct30a5000:​tcptest.tar.gz|demonstration server and client}} using ''​MSG_DONTWAIT''​ flag with ''​send''​. Client sends 1 Mb of data to the server and both quit after the data is sent full / all is receivedCompile with ''​make''​ and run the client with <​code>​./tcptestc <​serverIP>​ <server port></​code>​ and the server with <​code>​./​tcptests <​port></​code>​ 
 +The ''​MSG_DONTWAIT''​ demonstrates the effect when the send buffer is full and the application cannot write more -> the function returns and in next round attempts to write more into the buffer. Without the flag the ''​send()''​ function **blocks until all requested data is sent** - it will fill the buffer, wait until there is space, fill the remaining space, wait .. and it continues until all is sent.
-So a list for you to follow: 
-  - Use correct packet format. If the protocol states that the integers are sent as binary, do not even think about putting them as characters. 
-  - Always send **multi-byte integers** in network order! 8 bits = 1 byte, not a multi byte! 
-And remember that invalid packets should not crash or make it possible to shut down the application. Instead discard invalid data.