Jump to content
  • Advertisement
Sign in to follow this  
Tispe

WinSock, identifying a packet

This topic is 2776 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Well, you want to loop while recvfrom() is greater than 0.

Beware of operator precedence. Use:

while((size = recvfrom(...)) >= 0) {
// ...
}

Your current code is logically the same as:

while(size = (recvfrom(...) >= 0)) {
// ...
}


Generally, one allocates a buffer with an extra byte of space:

// Don't use magic numbers!
// They'll get out of sync
const int BufSize = 1400;

char Buffer[BufSize + 1];
// ...
while((size = recvfrom(Socket, Buffer, BufSize, 0, (sockaddr*)&RemoteAddress, &SizeInt)) > 0) {
Buffer[BufSize] = '\0';
// ...
}


You still aren't checking if you've got enough bytes before casting to a chat structure type. I'd handle this in the calling code:

bool handled = false;

switch(ID)
{
case 0:
if(size == sizeof(InfoPayload)) {
const InfoPayload &Payload = *reinterpret_cast<const InfoPayload *>(Buffer);
handled = info(Payload, size, RemoteAddress);
}
break;
case 1:
if(size == sizeof(ChatPayload)) {
const ChatPayload &Payload = *reinterpret_cast<const ChatPayload *>(Buffer);
chat(Payload, size, RemoteAddress);
}
break;
};

if(!handled) {
InvalidPacket(Buffer, size, RemoteAddress);
}

Also note how I allow the other functions to return "false" to indicate an invalid packet, which will then be handled. You should also handle/log any time recvfrom() returns a value less than 0, at the end of your loop.

Really, this is basic stuff. You won't be ready to debug complex networking issues if you cannot get the above stuff correct. (Warning: The above code is not compiled or tested).

[edit: code tags] Edited by rip-off

Share this post


Link to post
Share on other sites
Advertisement
I know I need more checking, I just need the basic principle down first. Also, I am passing a pointer to the Buffer, not the whole buffer. Casting in to a chat struct in the calling code will then truncate the chat message as the text will be outside this range. Which is why I want to cast in the function.

Share this post


Link to post
Share on other sites

struct ChatPayload1{UINT ID, tag, TxtLen;};


vs


struct ChatPayload2{UINT ID, tag, TxtLen; Char Buffer[]};



sizeof(ChatPayload2) = ???


If client is sending a ChatPayload1 object with a 1200 Char buffer appended you can cast without truncation. However if client is sending a ChatPayload2 object its size is variable?

Share this post


Link to post
Share on other sites
There is no truncation in the code I posted. I am not psychic, I did not divine the existence of these new packet types you are referring to.

Here is another solution:

#include <Windows.h>
#include <WinSock.h>
#include <string>

enum PacketType
{
INFO, CHAT
};

struct BasicPayload
{
UINT ID;
};

struct InfoPayload
{
UINT ID;
float x, y, z;
};

struct ChatHeader
{
UINT ID, tag, TxtLen;
};

struct ChatPayload
{
ChatHeader header;
char text[0]; // Variable length, not nul terminated.
};

union Packet
{
BasicPayload basic;
InfoPayload info;
ChatPayload chat;
};

void ProcessPackets(SOCKET socket, int addressSize);
bool chat(const ChatPayload &, int, sockaddr_in);
bool info(const InfoPayload &, sockaddr_in);
void InvalidPacket(const char*, int, sockaddr_in);

void ProcessPackets(SOCKET Socket, int SizeInt)
{
int size;
Packet packet;
sockaddr_in RemoteAddress;
char *data = reinterpret_cast<char *>(&packet);
while((size = recvfrom(Socket, data, sizeof(packet), 0, (sockaddr*)&RemoteAddress, &SizeInt)) > 0)
{
bool processed = false;

if(size >= sizeof(BasicPayload))
{
switch(packet.basic.ID)
{
case INFO:
if(size == sizeof(InfoPayload))
{
processed = info(packet.info, RemoteAddress);
}
break;
case CHAT:
if(size >= sizeof(ChatHeader))
{
int bytes = size - sizeof(ChatHeader);
processed = chat(packet.chat, bytes, RemoteAddress);
}
break;
}
}

if(!processed)
{
InvalidPacket(data, size, RemoteAddress);
}
}

if(size < 0)
{
// Handle error
}
}

bool chat(const ChatPayload &chat, int bytes, sockaddr_in Address)
{
if(chat.header.TxtLen != bytes)
{
return false;
}

std::string Text(chat.text, bytes);
// ...
return true;
}

bool info(const InfoPayload &, sockaddr_in Address)
{
// update entity
return true;
}

void InvalidPacket(char* Buffer, int size, sockaddr_in Address)
{
// errors, log, ban IP ect...
}

Note how it contains no magic numbers and minimises the number of casts. It is far from perfect, the previously mentioned issues of data size and endianness aren't handled.

Also note that while each message is a packet sending the text length is unnecessary, it can be inferred from the packet size.

Share this post


Link to post
Share on other sites

recvfrom(Socket, data, sizeof(packet).........)


sizeof(packet) is how long now? will you read a chat payload with this?

Share this post


Link to post
Share on other sites

I get slapped in the face with this cryptic bible.


Step one of becoming an expert at something is to learn the language used to discuss that topic.


If you think that the clear, lucid explanations of the UNIX man pages or the MSDN WinSock reference are "cryptic bibles," then you lack the necessary language skills to even START becoming an expert network programmer. At this point, I suggest that what you need to do is to learn how to read documentation, rather than to learn any particular thing.

If you want to "empty" a socket using recvfrom(), then you can make the socket non-blocking, and call recvfrom() until it returns a negative value.

However, if the only reason to use a socket is to talk between threads in the same process, then you probably want to use a producer/consumer queue, regulated with semaphores, rather than sockets.

Share this post


Link to post
Share on other sites
What do you think of this client code? (I altered the ChatHeader)


struct ChatHeader
{
UINT ID, recipient, tag;
};

char Buffer[1400];

ChatHeader Header;
Header.ID = 1;
Header.recipient = 5;
Header.tag = 0;

char *data = reinterpret_cast<char *>(&Header);


std::string chatter = "This is chat from a client";


strcpy_s(Buffer,data);
strcat_s(Buffer,chatter.c_str());

sendto(Socket, Buffer, sizeof(ChatHeader)+chatter.length(), 0, (sockaddr*)&RemoteAddress, sizeof(sockaddr));

Share this post


Link to post
Share on other sites
It doesn't make sense to strcpy() non NUL terminated character data.

I was using the variable sized struct idiom from C. The client code would look someting like this:

void SendChatPacket(/* SOCKET, Address ... */, UINT recipient, UINT tag, const std::string &text)
{
ChatPayload *chat = reinterpret_cast<ChatPayload *>(std::malloc(sizeof(ChatHeader) + text.length()));
if(!chat)
{
// Error handling...
}
chat->header.ID = CHAT;
chat->header.recipient = recipient;
chat->header.tag = tag;
// chat->header.TextLen = text.length() if you wish
std::copy(text.begin(), text.end(), chat->text);

const char *data = reinterpret_cast<const char *>(chat);
// Send code here

std::free(chat);
}


Alternatively you can could use alloca() (if you know what you are doing), or as you did just define a max size:

void SendChatPacket(SOCKET socket, UINT recipient, UINT tag, const std::string &text)
{
int MaxTextLength = sizeof(Packet) - sizeof(ChatHeader);

if(text.length() > MaxTextLength)
{
// Error, or break the chat into chunks
}

char buffer[sizeof(Packet)];

ChatPayload *chat = reinterpret_cast<ChatPayload*>(buffer);
chat->header.ID = CHAT;
chat->header.recipient = recipient;
chat->header.tag = tag;
std::copy(text.begin(), text.end(), chat->text);

int actualSize = sizeof(ChatHeader) + text.length();
// Send code, using "buffer" and "actualSize".
}

Share this post


Link to post
Share on other sites
Could this be better?


struct ChatHeader
{
unsigned int ID, recipient, tag;
char text;
};

void SendChatPacket(unsigned int recipient, unsigned int tag, const std::string &chatter)
{
char Buffer[1400];

ChatHeader Header;
Header.ID = 1;
Header.recipient = 0;
Header.tag = 0;
Header.text = '\0';

char *data = reinterpret_cast<char *>(&Header);

strcpy_s(Buffer,data);
strcat_s(Buffer,chatter.c_str());

sendto(Socket, Buffer, sizeof(ChatHeader)+chatter.length(), 0, (sockaddr*)&RemoteAddress, sizeof(sockaddr));

}

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!