Hello.

Been learning winsock for the past weeks and written a basic server and client code. I would like help with improving the code. At the moment it is just one client that can connect to the server, but I will read about multiple client code later.

The first thing is that I've seem to forgotten to clear some variable, coz the first message I sent gets some extra characters after. Like if I write Hello the server gets helloòö#■.

The second thing is the quit in the server. I need a better way to shutdown the server. As you can see in the server-code it has to if-functions for !quit... If I remove one of them it wont work.

Server:

#include <stdio.h>
#include <stdlib.h>
#include <winsock.h>
#include <time.h>
#include <string.h>

#define BUFSIZE 1024
#define MAX_MESS_LEN 512


struct log
{
	char messages[1000];
	char date[100];
};

int main(int argc, char* argv[])
{
	/* Local variables */
	WSADATA wsaData;
	WORD wVer;
	SOCKET hClient  = {0};
	SOCKET hSock  = {0};
	struct sockaddr_in saListen = {0};
	struct sockaddr_in saClient = {0};
	int nSALen;
	int nRet;
	char wzRec[BUFSIZE];
	int nLeft = MAX_MESS_LEN;
	int iPos = 0;
	int nData = 0;
	int portnumber;
	char *clientIP;
	char cltIP[20];
	struct log toLog[1000];
	char timeStamp[100];
	int i=0, max, j;
	FILE *file;

	time_t rawtime;
	struct tm *ptm;
	char str[30];


	if(argc == 2)
	{
		portnumber = atoi(argv[1]);
		if(portnumber>1024)
	{
	/* Initialize WinSock2.2 DLL (for Windows only) */
	wVer = MAKEWORD(2,2);
	WSAStartup(wVer, &wsaData);

	if (nRet == SOCKET_ERROR) {
		printf("Failed to init Winsock library\n");
		return -1;
	}

	printf("Starting server\n");

	hSock = socket(AF_INET, SOCK_STREAM, IPPROTO_IP);

	if (hSock == INVALID_SOCKET) {
		printf("Invalid socket, failed to create socket\n");
		return -1;
	}

	saListen.sin_family	  = PF_INET;
	saListen.sin_port		= htons(portnumber);
	saListen.sin_addr.s_addr = htonl(INADDR_ANY);

	nRet = bind(hSock, (struct sockaddr*)&saListen, sizeof(struct sockaddr));

	if (nRet == SOCKET_ERROR) {
		printf("Failed to bind socket\n");
		return -1;
	}

	while (1)
	{
		printf("Listening for connections\n");

		nRet = listen(hSock, 5);

		if (nRet == SOCKET_ERROR)
		{
			int nErr = WSAGetLastError();

			if (nErr == WSAECONNREFUSED) {
				printf("Failed to listen, connection refused\n");
			}
			else {
				printf("Call to listen failed\n");
			}
			closesocket(hSock);
			return -1;
		}

		nSALen = sizeof(struct sockaddr);
		hClient = accept(hSock, (struct sockaddr *)&saClient, &nSALen );

		if (hClient == INVALID_SOCKET) {
			printf("Invalid client socket, connection failed\n");
			closesocket(hSock);
			return -1;
		}

		printf("Connection established!\n");

		clientIP = inet_ntoa(saClient.sin_addr); /*gets IP of client. inet_ntoa returns
												   pointer with address in dotted format*/

		strcpy(cltIP, clientIP);


		do

		{

			nLeft  = MAX_MESS_LEN;
			iPos   = 0;
			nData  = 0;


			nData = recv(hClient, &wzRec[iPos], nLeft, 0);

			strcpy(toLog[i].messages, wzRec);

			time(&rawtime);
			ptm = localtime(&rawtime);
			strftime(str, 30, "%Y-%m-%d %H:%M:%S", ptm);
			strcpy(toLog[i].date, str);



			if (nData == SOCKET_ERROR) {
				printf("Error receiving data\n");
				memset(wzRec, 0, sizeof(wzRec));
				break;
			}

			nLeft -= nData;
			iPos  += nData;

			printf("%s: %s\n", &cltIP, &toLog[i].messages);
			i++;
			if(strcmp(wzRec, "!quit")==0)
			{
				break;
			}


				/* Clear data buffer */
			memset(wzRec, 0, sizeof(wzRec));

		} while( nLeft > 0 );



		max=i;


		file = fopen("c:\\srvmess.log", "a");
		if (file == NULL) {
			printf("Couldn't open file.\n");
			exit(0);
		}
		else
		{
			for(j=0;j<max;j++)
			{
				fprintf(file,"From %s - %s: %s\n", &cltIP, &toLog[j].date, &toLog[j].messages);
			}

		}
		fclose(file);

		/*iPos  = 0;
		nLeft = MAX_MESS_LEN;*/

		/*do
		{
			nData = send(hClient, &wzRec[iPos], nLeft, 0);

			if (nData == SOCKET_ERROR) {
				printf("Error sending data\n");
				break;
			}
			nLeft -= nData;
			iPos  += nData;

		} while( nLeft > 0 );*/

		closesocket(hClient);
		hClient = 0;

		if(stricmp(wzRec, "!quit") == 0) { 
			break;
		}

		/* Clear data buffer */
		memset(wzRec, 0, sizeof(wzRec));
	}

	printf("Shutting down the server\n");

	nRet = closesocket(hSock);
	hSock = 0;
	if( nRet == SOCKET_ERROR ) {
		printf("Error failed to close socketn");
	}

	/* STEP 9: Release WinSock DLL  (for Windows only) */
	nRet = WSACleanup();
	if( nRet == SOCKET_ERROR ) {
		printf("Error cleaning up Winsock Library\n");
		return -1;
	}
	printf("Server is offline\n");


	}

	else
		printf("ERROR Portnumber must be > 1024!");
	}

	else
	printf("ERROR: Wrong number of arguments\nUsage: chatsrv.exe <portnumber>");

  return 0;
}

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <winsock.h>
#include <time.h>

#define BUFSIZE 1024
#define MAX_MESS_LEN 512


struct log
{
	char messages[1000];
	char date[100];
};

int main(int argc, char* argv[])
{
	/* Local variables */
	WSADATA wsaData;
	WORD wVer;
	SOCKET hServer  = {0};
	struct sockaddr_in saServer = {0};
	int nRet;
	char wzRec[BUFSIZE];
	int nLeft = MAX_MESS_LEN;
	int iPos = 0;
	int nData = 0;
	char ipaddress[20], hostname[50], timeStamp[100];
	int portnumber;
	struct log toLog[1000];
	int i=0, max;
	FILE *file;

	time_t rawtime;
	struct tm *ptm;
	char str[30];



	if (argc == 3)
	{
		portnumber =  atoi(argv[2]);
		strcpy(ipaddress, argv[1]); //strcpy eftersom det läses in som char

		if(portnumber>1024)
		{

			/* Initialize WinSock2.2 DLL (for Windows only) */
			wVer = MAKEWORD(2,2);
			WSAStartup(wVer, &wsaData);

			gethostname(hostname, sizeof(hostname)); //Get the hostname

			if( nRet == SOCKET_ERROR) {
				printf("Failed to init Winsock library\n");
				return -1;
			}

			printf("Opening connection to server\n");

			hServer = socket(AF_INET, SOCK_STREAM, IPPROTO_IP);

			if( hServer == INVALID_SOCKET ) {
				printf("Invalid socket, failed to create socket\n");
				return -1;
			}

			saServer.sin_family	  = AF_INET;
			saServer.sin_port		= htons(portnumber);
			saServer.sin_addr.s_addr = inet_addr(ipaddress);

			nRet = connect( hServer, (struct sockaddr *)&saServer, sizeof(struct sockaddr));

			if (nRet == SOCKET_ERROR) {
				printf("Connection to server failed\n");
				closesocket(hServer);
				return -1;
			}

			printf("Connected to server - write '!quit' - to close\n");



			while(strcmp( wzRec, "!quit") != 0)
			{

				printf("%s> ", &hostname);
				gets(wzRec);
				strcpy(toLog[i].messages, wzRec);

				time(&rawtime);
				ptm = localtime(&rawtime);
				strftime(str, 30, "%Y-%m-%d %H:%M:%S", ptm);
				strcpy(toLog[i].date, str);

				nData = send(hServer, &toLog[i].messages, strlen(toLog[i].messages), 0);


				if( nData == SOCKET_ERROR ) {
					printf("Error sending data\n");
					break;
				}

				i++;


			}

			max=i;

			file = fopen("c:\\cltmess.log", "a");
			if (file == NULL) {
				printf("Couldn't open file.\n");
				exit(0);
			}
			else
			{
				for(i=0;i<max;i++)
				{
					fprintf(file,"%s: %s\n", &toLog[i].date, &toLog[i].messages);
				}

			}


			/* Clear data buffer */
			memset(wzRec, 0, sizeof(wzRec));

			/*nLeft = MAX_MESS_LEN;
			iPos  = 0;
			nData = 0;*/

		   /* do
			{
				nData = recv(hServer, &wzRec[iPos], nLeft, 0);

				if( nData == SOCKET_ERROR ) {
					printf("Error receiving data\n");
					break;
				}
				nLeft -= nData;
				iPos  += nData;

			} while( nLeft > 0 );*/

			fclose(file);

			printf("Closing connection\n");

			nRet = closesocket(hServer);
			hServer = 0;

			if (nRet == SOCKET_ERROR) {
				printf("Error failed to close socket\n");
			}

			/* STEP 7: Release WinSock DLL (for Windows only) */
			nRet = WSACleanup();

			if (nRet == SOCKET_ERROR) {
				printf("Error cleaning up Winsock Library\n");
				return -1;
			}
			printf("Data sent successfully\n");

		}
		else
		printf("ERROR: Portnumber must be >1024");

	}
	else
	printf("ERROR: Wrong number of arguments\n Usage: chatclt.exe <ipaddr> <portnumber>");

	return 0;
}

/Ph

Yes, the problem here is related to how the strlen/strcpy functions actually work. These functions work based on null terminated strings. So you have to ensure that there is a null character at the end of the sequence in order for them to work properly. The client doesn't send a terminating null, which IMHO isn't the problem, but the server blindly appends the socket recv buffer with strcat. You probably should use a combination of memcpy and explicitly keeping track of the length instead. Then at the end, you can put a null character at the end so that the strlen/strcpy functions will work correctly.



That inner do loop may never terminate. You're resetting nLeft with each loop, so if you don't completely fill the buffer in a recv, then the loop goes on and on. Fixing this might include thinking about a protocol that allows you to explicitly determine the start and end of a packet, so that you know when to terminate this loop. This loop should only run while receiving a packet, to be certain that an entire packet is received. For your purposes, receiving a fragmented packet should almost never happen, because the packets would be very small.