sending struct - what am I doing wrong?

1) You use htonl() to put UID, SID and TID into network order, but UID, SID and TID are only 16 bits wide.
2) You don''t try to switch the data from network order to host order in the receive code.
3) When sending the packet you take the address of pck and use that as the buffer for send. But pck already is a pointer to the struct, so you''re probably sending random stack data over the wire.

*Updated code, thanks to SiCrane.
Still works as wrong as the previous one.


Client side:

bool CNetwork::SendInPacket(INPACKET *pck)
{
	CClientSock sck;

	
	pck->AID = htons(pck->AID);
	pck->UID = htons(pck->UID);
	pck->SID = htons(pck->SID);
	pck->TID = htons(pck->TID);
	

	sck.Init("127.0.0.1", 1985);
	sck.Send((char *)pck, sizeof(INPACKET));
	sck.Shutdown();

	return true;
}
  

Server side:

if (sockret = Sock.Receive(buffer, 512))
{
	INPACKET *pack;

	pack = (INPACKET *)&buffer

	cout << "pack->AID: " << ntohs(pack->AID) << "\n";
	cout << "pack->UID: " << ntohs(pack->UID) << "\n";
	cout << "pack->SID: " << ntohs(pack->SID) << "\n";
	cout << "pack->TID: " << ntohs(pack->TID) << "\n";
	cout << "pack->Param: " << pack->Param << "\n";
}
  

Sent values by client:
pack.AID: 1
pack.UID: 2
pack.SID: 3
pack.TID: 4
pack.Param: 00000000

Resulting output of server:
pack->AID: 30712
pack->UID: 8021
pack->SID: 1244860
pack->TID: 10816
pack->Param: 7FFDF000

[edited by - Shadowed on May 22, 2004 3:13:13 AM]

possible alignment and packing issues, never just send a struct over the wire

quote:

---

Original post by yugami
possible alignment and packing issues, never just send a struct over the wire

---

What''s the alternative you suggest?

//Sending

	char *ch = (char*) &var

	for( short lo = 0 ; lo <= sizeof(var) ; lo++ , ch++ )
	{	Net.put(*ch);					}
//Recieving

	MyStruct var;
	char *ch = (char*) &var

	for( short lo = 0 ; lo <= sizeof(var) ; lo++ , ch++ )
	{	*ch = File.get();				}

EDIT: Forgot to say if you use chars, same logc applies to whatever byte size

[edited by - genjix on May 22, 2004 8:52:10 AM]

But what''s wrong with my code? I want to use my style of sending/receiving, not Genjix''s, if possible.

*Update:
There''s a little bug in the sources posted:

SID in INPACKET is __int32, not __int16, so all ntohs()s and htons()s regarding INPACKET.SID should be interpreted as ntohl() and htonl().
Just didn''t want to edit the post.

You stil have the same pointer problem on the receiving side
pack = (INPACKET *)&buffer
should be
pack = (INPACKET*)buffer;

quote:

---

Original post by Shadowed

quote:

---

Original post by yugami
possible alignment and packing issues, never just send a struct over the wire

---

What''s the alternative you suggest?

---

interestingly enough I''m working on an article for generic serialization of structs.

any idea when that article of yours will be available yugami and where it will be available?

In addition to the error mentioned by fredizzimo, the Param member of the structure is useless. Sending a pointer to the server will not transfer any data, since the data will have a different address on the server, so the server will end up with a pointer to a random place in memory. If you want to send the data it points to, you''ll need some kind of mini protocol for it. I would suggest also storing the size of the data pointed to by Param in the struct and sending the IDs individually {sck.Send((char *)&(pck->SID), sizeof(pck->SID));}, then the size member in a simmilar way, and then sck.Send((char *)&(pck->Param), pck->ParamSize);
Well, there might be an error in there because I''ve been up far too long, but hopefully you get the idea

were I you, I would use a packet_header:

struct PACKET_HEADER {
	__int32		SID;	// Session ID

	__int16		UID;	// User ID

	__int16		AID;	// Action ID

	__int16		TID;	// Type ID

	__int16		PACKSIZE;// packet size

};

then create the packet:

// we use a void pointer so that it can be any type of data

// and pass the size of the data to avoid size issues.

char *create_packet( PACKET_HEADER *pHeader, void *data, __int16 size )
{
    static char packet[MAX_PACKET_SIZE];
    ZeroMemory( packet, MAX_PACKET_SIZE );

    if ( size+sizeof(*pHeader) >= MAX_PACKET_SIZE )
    {
        // error stuff

    }

    // place the header at the beginning of the packet

    memcpy( packet, pHeader, sizeof(*pHeader) );
    // add in the packet

    memcpy( packet+sizeof(*pHeader), data, size );

    return packet;
}

when the server/client receives the packet, it should cut out the header to know what it is looking at and then copy the rest of the data into the appropriate struct.

I have always used tcp/ip so I just send a string command instead of messing with packets. For the most part the server side has it easy just relaying messages from client to client and globally unique id''s make the clients side easy by telling it which object the command is for. I doubt my way is best for say a FPS where udp would be better suited but for simple turn based strategy or small 2d rpg''s it seems easier to me and much simpler to debug.

Genjix:

I don't understand what the problem is with sending the structs over the pipe if you assume both hosts are using the same executable (and thus, have the same byte ordering and the same sizes for each type). The only thing is that you might be sending unnecessary alignment bytes.

From my knowledge, your code is identical to:

To write:

Something var(...);
write(fd, &var, sizeof(var));

To read:

Something var;
read(fd, &var, sizeof(var));


Of course, in my code, I have to check for errors such as if I couldn't read the whole var in one shot.

If we cared about making our message cross-platform, then we'd have to mess with the byte ordering and alignment on different machines... but that's usually not the case.

In my case, if I ended up re-compiling my program for the Windows 64-bit system, then people on 64-bit systems wouldn't be able to communicate with people on the 32-bit systems. Well, one of them would probably crash. But most people (myself included) wouldn't recompile their programs on other systems.

Am I missing something?

[edited by - canuckle on May 31, 2004 2:36:43 AM]

Shadowed:

It seems that you're sending garbage over the pipe.

The line:
sck.Send((char *)&pck, sizeof(INPACKET));

should be:
sck.Send((char *)pck, sizeof(INPACKET));


I'm assuming that
sck.Send(ptr, s);

Sends a chunk of data of size s that ptr points to.

In the case of &pck, you're passing in a pointer to a pointer to the INPACKET structure. You should pass in a pointer to the INPACKET structure instead, which will send the INPACKET itself.

Note that you still will have a problem, since Param is a pointer. As mentioned before, you cannot send pointers over pipes because different computers do not share the same memory.

I'm guessing Param is of variable length. In that case, there are two solutions to sending Param:

1. Use packet header as shown above. This is recommended for when you have a lot of possible Params and care about network traffic. This is what you usually see in networks.

2. Use unions (I can hear people complaining already). Unions are good for when Params could be only a few different things. This could make your life a lot simpler, since unions always have a fixed size. But it is not generic and you might end up having a huge union if you keep increasing possibilities of Params.

[edited by - canuckle on May 31, 2004 2:55:16 AM]

[edited by - canuckle on May 31, 2004 2:55:48 AM]

Some suggestions:
1. what kind of socket you are using, blocking or non-blocking?
if you are using blocking one, recv will block forever, as the code you use, I guess the socket is NonB, so you will probably not recv the enough data for the structure. So the easiest way to solve the problem is using block IO and always provide the actual size of the struct.

2. if you are using non-fixed size of structure, always provide the structure size first. then the recv will know the size.

3. use UDP instead of TCP. TCP is byte-stream protocol, this means that the data is send 1 by 1. Ex. if the send size of data is larger than the max send buffer size which can be get by socketgetopt function, the data you are sending will be divided in to certain amounts of packets. On the oppisite the Non-block will return only the first packets, so the size is not the actual size. UDP is acctully a small packet, but the problem is that UDP''s not garentee that the packet can be received by peer or not. so the packet loss happens in UDP. But you could add reliablity manually. ex. when you are sending the packet to clients, always wait for the client''s ACK and then send the next; After certain amounts of time, if no ACK returns, send the packet again. (detailed in UNIX NETWORK PROGRAMMING Volume 1. 2nd, R.Stevens)