Archived

This topic is now archived and is closed to further replies.

csolar

Winsock recv is screwing with my variables!!

Recommended Posts

Can anyone tell me whats going on with my winsock program? Heres the code: for( int i = 1; i <= num_players; ++i ) { char rcvBuffer[10]; recv(s[i],rcvBuffer,10,0); if( rcvBuffer == "Moving" ) { recv(s[i],rcvBuffer,10,0); player[i].pos = rcvBuffer; cout << player[i].strUsername << " moved to " << rcvBuffer << endl; } } Heres exactly whats happening: I run the program, it gets to the first recv function. i = 1, it gets the message from s[1] (s is a socket, with a user at one end), then, it resets i to 0! I have no idea why, or how, it just turns my variable into nothing! The only way I can make it not change my i variable is if I put i as the last parameter. So the function looks like this: recv(s[i],rcvBuffer,200,i); It doesn't change i, but it doesn't recieve anything! I have other recv functions above this one that don't do this. So I am very confused and frustrated. If anyone knows what I am doing wrong, then please tell me. Programming is like sex; make one mistake and you have to support it the rest of your life. [edited by - csolar on March 3, 2003 11:30:05 PM] [edited by - csolar on March 3, 2003 11:31:14 PM]

Share this post


Link to post
Share on other sites
first, that''s not how you compare c-style strings.

second, problem is most likely elsewhere in your code.

third, without any error checking, you won''t get far.

fourth, recv doesn''t have to receive more than one byte per call, and you don''t account for this.

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
Not knowing what the rest of your code looks like (and the problem could lie therein) I don''t see anything noticably wrong, except that the only things you would (rarely) pass to the last parameter are MSG_PEEK and MSG_OOB (which might equate to the literals 1 and 2). You have an array of sockets I take it? Are your sending/receiving on all of these in the same thread? Have you considered spawning a new thread for each new client, because each call to recv blocks until it gets data or times out according to what options you have set on the socket. Each server and/or client should really run in a separate thread, the design of which gets a little tricky sometimes. ALSO, you should check the return value of recv and check for extened error information also in the event of a return value of SOCKET_ERROR.

ie,

int nReturn = recv(s,buffer, bufsize, 0);
if (nReturn == 0)
{
//socket disconnected
}
else if (nReturn == SOCKET_ERROR)
{
//check error condition
}
else
{
whatever
}

As gratifying as successfully hacking WinSock might be (I know, I''ve been there once upon a time), have you considered DirectPlay?

Share this post


Link to post
Share on other sites
yes, I know that this isn''t very good code. But, I have been mostly concerned about that recv function. I havn''t changed anything else in my program because I have been tring to get this to work. I am positive that the problem is in that section of code, if not just the recv function. I ran the debugger, and when it got to this line:

recv(s,rcvBuffer,10,0);

i = 1, once it ran that line, i = 0. And yes, I do have a array of sockets. The array is 10 sockets big. It is declared like this: SOCKET s[10]; what I am trying to do with this code is to go through all of the sockets, and recieve any new info that the client on one end of the socket sent. Then I compare the string that I got with some predifened strings: Moving, Update, GetFile, etc. And if the recved string matches one of those, it does that code and sends the requested info back to the client. I guess that I am probably going about this all wrong. Maybe one of you could help out and post some code for me. I looked in the Networking and Multiplayer articles, and all I found was tutorials on how to set up winsock, not to send, receive, and compare msgs. Could someone point me to a good tutorial if I missed one?


Programming is like sex; make one mistake and you have to support it the rest of your life.

Share this post


Link to post
Share on other sites
1. Since rcvBuffer will NEVER == "Moving" (you're comparing pointers), it is unlikely that your code ever even gets to the recv() call. (As niyaw noted, you can't compare character arrays with '=='. You use strcmp() to compare character arrays in C).

2. An array in C uses zero-based indexing. Your socket array goes from 0 to 9, NOT 1 to 10. If that's how you're doing array indexing throughout your code, it is very likely that you are hosing your stack and clobbering your local 'i' variable.

I would recommend that you go back through your code and make sure that you are using array indexes and string assignment/comparison properly.


[edited by - Dave Hunt on March 4, 2003 4:49:41 PM]

Share this post


Link to post
Share on other sites
quote:
Original post by csolar then, it resets i to 0!
Check your compiler setting. If you have turned on optimizations, your debugger may be confusing stack-based variables with registers.

-cb

Share this post


Link to post
Share on other sites
I tried using strcmp before, and it was weird. It would return 1 at the beginning, and now its returning 0. With no change.

Also, the problem is with the first recv function. Not the secound. I included that extra code just to show what I am triing to do.

Finaly, the 0 index for s is the server''s own socket. Heres how its setup:

s[0] = Server socket
s[1] = Client
...
s[10] = Client

so I have to start the index at 1.

Programming is like sex; make one mistake and you have to support it the rest of your life.

Share this post


Link to post
Share on other sites
Ok, I got it working. Heres what I did to fix it. First, I changed my socket array to two things. One, a server socket. Named ServerS. The other, an array of sockets for the clients. Called ClientS[MAX_PLAYERS]. And I changed my for loop to start at 0. I was still getting the same problem, so I studied the post that Dave Hunt made about hosing my stack. I assume that was the problem, because I made that i a static int and it solved the problem. Heres all the code.


for( static int i = 0; i < num_players; ++i )
{

char rcvBuffer[10];

recv(ClientS,rcvBuffer,200,0);

int n = strcmp( rcvBuffer, "Moving" );

if( n == 0 )
{



recv(ClientS[i],rcvBuffer,200,0);

strcpy(player[i].pos, rcvBuffer);

cout << player[i].strUsername << " moved to " << rcvBuffer << endl;

}
}

[/CODE]

and that works. So thanks to everyone who helped me figure out what was wrong. If you still have any suggestions on how I can improve this, then I am still open. My email is csolar1124@attbi.com if you want to contact me that way.

Thanks
-Charles


Programming is like sex; make one mistake and you have to support it the rest of your life.

Share this post


Link to post
Share on other sites
Why, oh why, are you passing 200 as the buffer length when the array is only 10 bytes long?

Share this post


Link to post
Share on other sites
Finally someone pointed that out.

You are telling recv() that your buffer can receive 200 bytes, yet your buffer is only 10 bytes in size!!! No wonder your local i variable was getting mangled (among other things.)

Further more, this line:


cout << player.strUsername << " moved to " << rcvBuffer << endl;


Is guaranteed to print garbage sooner or later if rcvBuffer doesn''t receive a proper null-terminated string.

So what does your call to the send() function look like (the one that sends the "Moving" string)?



Share this post


Link to post
Share on other sites
quote:
Original post by Digitalfiend
Finally someone pointed that out.



His original posting didn''t have the 200, nor did his subsequent "when I got to this line..." have the 200. It would have saved us all a lot of time if the code posted was actually the code being tested.

Share this post


Link to post
Share on other sites
Can someone tell me how to set up a messaging system? I read the article about it and I tryed his way, but what I am trying to do is make the client totally dependent on the server. the server sends the client the welcome message, the login msg, etc. So that if I want to change the game, the users won't have to constantly download new clients. So the buffer size for almost every msg has to be different. Right now I am trying a sizeof command, but thats not working to well. It only sends the first several characters then stops. So can somebody post a really simple msg system that will suit my needs?

I am going to post all of my code for the server. Its not that long. But maybe someone can tell me what I am doing wrong or something. I am trying to get a hold of this, but I need a little help.


  
#include <iostream.h>
#include <winsock2.h>
#include <stdio.h>
#include <fstream.h>
#include <string.h>

#define MAX_PLAYERS 10

#define GAME_FILE "World.txt"

#define YES 1
#define NO 0

#define MSG_WELCOME "Welcome to Trinity! Please log in and enjoy your adventure!"
#define MSG_ACCEPTED "You are logged in! Enjoy your stay!"
#define MSG_MOVING "1"
#define MSG_REQUESTFILE "2"
#define MSG_ROOMDESC "3"
#define MSG_ROOMMONSTERS "4"
#define MSG_CLIENTQUIT "5"



struct CLIENT_TYP // This is the Structure that describes a Client.

{
int InUse; // Is The Client Connected to Server

SOCKET ClientSocket; // A Socket for the client to sit on

struct sockaddr_in clnt_addr; // Holds the address of the client

char pos[10];
char strUsername[10];
char strPassword[10];
};

char strPlayerStart[50];
char strTemp[20];

char* g_buffer[MAX_PLAYERS];

CLIENT_TYP Client[MAX_PLAYERS];
SOCKET ServerS;
sockaddr_in me;
sockaddr you[10];
int addr_size = sizeof (sockaddr);
int num_players = 0;

timeval timeout; // This is used for the select function its seeing

// how long it should wait before timing out.



void SendMsgtoClient( char*, int );
void RecvMsgfromClient( char*, int );
int Disconnect( int );
int Check_Connections();
void Shutdown_Server();
void GetRoomInfo( ifstream, int, char [10] );

void main(){

// Create the file stream that will open and read the file

ifstream fin;

// Open the file defined in GAME_FILE

fin.open(GAME_FILE);

// Check if the file was found, if not, quit

if( fin.fail() )
{
// Display an error message and return -1 (quit)

cout << "Unable to find " << GAME_FILE << endl;
exit(0);
}

fin >> strTemp >> strPlayerStart;

// Setup WinSock


cout << "Trinity Server v1.0" << endl;
cout << "By Charles Solar" << endl << endl;
WSADATA w;
int error = WSAStartup (0x0202,&w);
if (error)
{
cout << "Error: You need WinSock 2.2!" << endl;
exit(0);
}
if (w.wVersion!=0x0202)
{
cout << "Error: Wrong WinSock version!" << endl;
WSACleanup ();
exit(0);
}

ServerS = socket (AF_INET,SOCK_STREAM,0);
me.sin_family = AF_INET;
me.sin_port = htons (5555);
me.sin_addr.s_addr = htonl (INADDR_ANY);

if (bind(ServerS,(LPSOCKADDR)&me,sizeof(me))==SOCKET_ERROR)
{
cout << "Error: Unable to bind socket!" << endl;
WSACleanup ();
exit(0);
}
if (listen(ServerS,1)==SOCKET_ERROR)
{
cout << "Error: Unable to listen!" << endl;;
WSACleanup ();
exit(0);
}

// Make all Clients Not In Use

for (int i = 0; i < MAX_PLAYERS; i++)
{
Client[i][i].InUse = NO;
}

cout << "Listening for connections..." << endl << endl;
while( 1 )
{
if( num_players < MAX_PLAYERS )
Check_Connections();

static int i;
i = 0;

for( i; i < num_players; ++i )
{

char rcvBuffer[10];

//recv(Client[i].ClientSocket,rcvBuffer,200,0);

RecvMsgfromClient( rcvBuffer, i );

if( strcmp( rcvBuffer, "Moving" ) == 0 )
{

//recv(Client[i].ClientSocket,rcvBuffer,200,0);

RecvMsgfromClient( rcvBuffer, i );

strcpy(Client[i].pos, rcvBuffer);

cout << Client[i].strUsername << " moved to " << rcvBuffer << endl;

}
else if( strcmp( rcvBuffer, "Idle" ) == 0 )
{
//Do nothing because the client is idling

}
else if( strcmp( rcvBuffer, "RequestFromFile" ) == 0 )
{

RecvMsgfromClient( rcvBuffer, i );

if( strcmp( rcvBuffer, "RoomDesc" ) == 0 )
{
// Get room desc and send it back

GetRoomInfo( fin, i, Client[i].pos );

}
}
}


}

Shutdown_Server();
}

void SendMsgtoClient( char* buffer, int iClient ){send(Client[iClient].ClientSocket,buffer,sizeof(buffer),0);}
void RecvMsgfromClient( char* buffer, int iClient ){recv(Client[iClient].ClientSocket,buffer,30,0);}


int Check_Connections()
{
int s; // s is where the data is stored from the select function

int nfds; // This is used for Compatibility

fd_set conn; // Setup the read variable for the Select function

int Client_Length = sizeof(Client->clnt_addr); // Store the length of the client address


timeout.tv_sec=0; // Set the timeout to 0 for Non Blocking

timeout.tv_usec=0;

if (num_players < MAX_PLAYERS)
{
FD_ZERO(&conn); // Set the data in conn to nothing

FD_SET(ServerS, &conn); // Tell it to get the data from the Listening Socket

// Up the nfds value by one, shouldnt be the same for each

// client that connects for compatability reasons

nfds=ServerS+1;
s = select(nfds, &conn, NULL, NULL, &timeout); // Is there any data coming in?


if (s > 0) // Someone is trying to Connect

{
for (static int index = 0; index < MAX_PLAYERS; index++)
{
if (Client[index].InUse == NO)
{
Client[index].ClientSocket = accept(ServerS,
(struct sockaddr *)&Client[index].clnt_addr,
&Client_Length);

// Client Connected

// Store the Ip of the Client that Just Connected.

char *ClientIp = inet_ntoa(Client[num_players].clnt_addr.sin_addr);
cout << "Accepted Connection From: " << ClientIp << endl;

SendMsgtoClient( MSG_WELCOME, num_players );

char strUsername[20];
char strPassword[20];

RecvMsgfromClient( strUsername, num_players );
RecvMsgfromClient( strPassword, num_players );

cout << "Player logged in as " << strUsername << endl;

SendMsgtoClient( MSG_ACCEPTED, num_players );

strcpy(Client[num_players].strUsername, strUsername);
strcpy(Client[num_players].strPassword, strPassword);

SendMsgtoClient( strPlayerStart, num_players );

num_players++;

// This Client is now in use.

Client[index].InUse = YES;

// Exit the For loop that is looking for empty clients.

break;
}
}
}
}

return (1);
}

// Function That Disconnects a Specified Client

int Disconnect(int s)
{
closesocket(Client[s].ClientSocket);
Client[s].InUse = NO;
num_players--;

return(1);
}

// Shutdown the Entire Server

void Shutdown_Server()
{
// Close The Socket that Listens for Connections

closesocket(ServerS);

// Loop that closes down all the Client Connections

for (int index = 0; index < MAX_PLAYERS; index++)
{
Disconnect(index);
}

// Remove Winsock

WSACleanup();
}

void GetRoomInfo( ifstream fin, int iClient, char strRoomName[50] )
{
char strLine[100];
static char strRoom[50];

bool found = false;

char strRoomDesc[200];
char strRoomNorth[30];
char strRoomSouth[30];
char strRoomEast[30];
char strRoomWest[30];

strcat( strRoom, "<" );
strcat( strRoom, strRoomName );
strcat( strRoom, ">" );

fin.seekg( NULL, ios::beg );
fin.clear();



while( !found || !fin.eof() )
{
fin.getline( strLine, '\n' );

if( strcmp(strLine, strRoom) == 0 )
{

while( strcmp( strRoomDesc, "*" ) != 0 )
{
fin.getline( strRoomDesc, 100, '\n' );
SendMsgtoClient( strRoomDesc, iClient );
}

SendMsgtoClient( "*", iClient );

fin >> strTemp >> strRoomNorth;
fin >> strTemp >> strRoomEast;
fin >> strTemp >> strRoomSouth;
fin >> strTemp >> strRoomWest;


SendMsgtoClient( strRoomNorth, iClient );
SendMsgtoClient( strRoomEast, iClient );
SendMsgtoClient( strRoomSouth, iClient );
SendMsgtoClient( strRoomWest, iClient );

found = true;

}
}
}


All the room information is in a file called "World.txt" in the server directory. The server is supposed to do the following things when the client needs the room desc.

1. Search teh file for the room name
2. Read in the room desc, send it line by line to the client
3. Read in the posible directions, and send them to the client
4. And generally just do everything.

The game is going to be an online text game. Like a MUD but without Telnet. Hopefuly someone can point me in the irght direction.

PS. This current setup does not work. The client doesn't recieve the msgs right, and everything gets screwed up. I've given this problem a lot of thought over the past 2 or 3 days. So I did try to think of a solution, but like I said, this is my first Winsock program and I'm a little confused.

Programming is like sex; make one mistake and you have to support it the rest of your life.

[edited by - csolar on March 7, 2003 11:11:28 PM]

[edited by - csolar on March 8, 2003 11:52:01 PM]

Share this post


Link to post
Share on other sites
You might want to check the amount of bytes sent and received when using send, recv. I am pretty sure that although tcp/ip guarantees that your data will get sent, it doesn''t guarantee it will all be sent at once. It has been a while since I used winsock, but off the top of my head, here is what I remember of doing a recv function...

  
// message bust be at least the same size as buffer

int recvmessage( SOCKET sock, char *message )
{
int ibytesread, iread;
char buffer[128]; // make this the largest size your server will send

// the MSG_PEEK doesn''t read the buffer, just tells you how much data is waiting

int ibyteswaiting = recv( sock, buffer, 128, MSG_PEEK );

if ( ibyteswaiting >= 128 )
{
//data waiting too large for buffer

return -1;
}

ibytesread = 0;
for (;;)
{
iread = recv( sock, buffer+ibytesread, 128, 0 );
if ( iread == SOCKET_ERROR )
{
int ierror = WSAGetLastError();
if ( ierror == WSAEWOULDBLOCK )
break;
else
{
//send appropriate error message

return -1;
}
}

ibytesread += iread;
if ( ibytesread == ibyteswaiting )
break;
}

// add in a null terminator for kicks

buffer[ibytesread] = ''\0'';

// copy the buffer to the message

strcpy( message, buffer );

// return bytes read

return ibytesread;
}

The MSG_PEEK line is optional but is a good idea to make sure your not going to overflow your buffer.
like I said this is off the top of my head and it might not be your problem, but this is what I noticed.

Have you played any MUDs lately? All of the ones I have played don''t require you to download anything when they change the game and they don''t require you use telnet either... just some client that can send and receive text over a tcp/ip connection. Most clients now support full ansii color and some clients even support a MUD Sound Protocol which you do have to download stuff for when they change it but it is optional to use.

As for the messaging system... Merc based MUDs parse the data received from the client into 3 parts... the client that sent it, the command, and the arguments supplied to the command. Basicly they use the first word (understands quotes for multiple word commands) as the command, look up the command in a table and call the function associated with that command. The arguments supplied to that function are always the client/player that sent the command and the rest of the string they sent to the server. So if your client sends "cast fireball monster" to the server, the server looks up the command "cast", finds that the function void do_cast( CHAR_DATA *ch, char *argument ) is associated with it. It calls do_cast with ch being the character associated with the socket it received the command from and argument being "fireball dragon". It is then up to do_cast to figure out what to do with the information it has received. Since the server is what processes commands, the client only has to send what the user types and receive what the server sends in response to the client.

wow that was long. it sounds like it should be easy but it can become pretty in depth once you get started. Especially since creating a text based world requires A LOT of typing.. did I mention it requires a lot of typing?

hope that helped...

Share this post


Link to post
Share on other sites
MSG_PEEK most certainly does read the buffer. It actually copies the data from the network buffer to your application buffer, though it doesn''t remove that data from the network buffer.

Using MSG_PEEK is, in general, a bad idea.

Share this post


Link to post
Share on other sites
quote:

MSG_PEEK most certainly does read the buffer.


your right and I am sorry for the confusion...

quote:

Using MSG_PEEK is, in general, a bad idea.


Why is it a bad idea? I would think in general you would want to know how much data is waiting... I am not relying on the buffer to give me the data so why is this bad? I don''t have any documentation in front of me atm but I have always used MSG_PEEK when dealing with winsock in this manner without any problems so if I have been doing something wrong, please tell me why it is bad.

Share this post


Link to post
Share on other sites
Learn to post code on this forum. Use:

[source]
code should be written here.
[/source]

That will show:


  
code should be written here.




Update GameDev.net system time campaign - success at last

Share this post


Link to post
Share on other sites
Use <iostream> and not <iostream.h>. iostream.h is pre standard and deprecated.



Update GameDev.net system time campaign - success at last

Share this post


Link to post
Share on other sites
quote:
Original post by evillive2
edit: hit post too soon.. DOH!

[edited by - evillive2 on March 8, 2003 1:08:05 AM]


To delete a post: Press edit on your message, check the "Did you double post?" checkbox, then press "Make Modifications".

You where successful to press edit on your message, so you are almost there.



Update GameDev.net system time campaign - success at last

Share this post


Link to post
Share on other sites
Evillive2, thats is an awesome bit of code you just introduced me to. That looks like it will do exactly what I need it to do! I''m just going to go insert that into my code now, and report back once I got it. Thanks a whole lot!

-Charles

PS. I tried to figure out what the code was to make that code box thing in a message. I tried
 and it didn''t work.  So I''ll now its source from now on.    

Share this post


Link to post
Share on other sites
I just read an article on why it is bad to use MSG_PEEK. To sum it up, it is because it is an unnecessary performance hit and it could also return incorrect values (I have never run into the later of the problems). However, Here is a link to that article from Microsoft. After reading the article I would suggest removing it from the previous code. Instead, since your writing the server side as well, you know what the maximum buffer size your server will send is beforehand so make sure you have at least that much on your client side buffer.

Thanks to Digitalfiend for bringing this to my attention.

Edit:
Upon re-reading the article, I think the warning was more geared towards the idea that someone may use recv with MSG_PEEK instead of the select function(or equivilent). I agree it is an unnecessary performance hit, but if used properly it doesn't sound like it would be that bad for debugging purposes.

[edited by - evillive2 on March 10, 2003 3:17:15 PM]

Share this post


Link to post
Share on other sites