Building a Multicast Packet - memcpy casting issues

[trolley01]

Hi,

I am just getting back in to C++ after 10 years not doing any, contributing to an open source project. I'm adding in some functionality and am hitting a road block.

I need to send a multicast packet out on the network that is structured in a certain way. I have the definition, and know what data is going in each byte. I can successfully send a message using multicast, I now just need to send the right message.

I have used a char array to hold the message, as each char represents 1 byte, and I can transmit the array.

I am having trouble putting all of the data in the right place though. If my source data is a string, then I seem to be able to convert it, but if it is a short or int, then I keep getting errors when compiling. Similarly, two of the lines, (version and type) i initially tried using char arrays with a length of one.

What am I doing wrong? Should I be using memcpy or a different function, or even be doing this in a totally different way altogether?

This is the code that I am using, along with the packet structure:

   //Construct a Zone Query packet
   // 4 bytes - Signature "Ohz " = 0x6f, 0x68, 0x7a, 0x20
   // 1 bytes - Version = 1
   // 1 bytes - Type (0 = Zone Query, 1 = Zone Uri)
   // 2 bytes - Entire message length = 12 + zone length
   // 4 bytes - Length in bytes of the zone ID
   // n bytes - Zone ID to query

    //This value will be passed in to the function
    string zoneID = "714f9b83b2beb170b19db6a281d2f9512";
     
    //Calculate all of the values that we need
    short packetLength = 4 + 1 + 1 + 2 + 4 + strlen(zoneID.c_str());
    int zoneIDLength = strlen(zoneID.c_str());
    char buffer[packetLength];
    string headerText = "Ohz ";
    string version = "1";
    string messageType = "0";

    //Build the packet
    memcpy(buffer + 0, headerText.c_str(), 4);
    memcpy(buffer + 4, version.c_str(), 1);
    memcpy(buffer + 5, messageType.c_str(), 1);
    memcpy(buffer + 6, packetLength, sizeof(packetLength));
    memcpy(buffer + 8, zoneIDLength, sizeof(zoneIDLength));
    memcpy(buffer + 12, zoneID.c_str(), zoneIDLength);

    LOGDEB("Buffer: " << buffer << " : Ended" << endl);

The errors that I get are:
error: invalid conversion from ‘short int’ to ‘const void*’ [-fpermissive]
memcpy(buffer + 6, packetLength, sizeof(packetLength));
^
error: initializing argument 2 of ‘void* memcpy(void*, const void*, size_t)’ [-fpermissive]
__NTH (memcpy (void *__restrict __dest, const void *__restrict __src,
^
error: invalid conversion from ‘int’ to ‘const void*’ [-fpermissive]
memcpy(buffer + 8, zoneIDLength, sizeof(zoneIDLength));
^
error: initializing argument 2 of ‘void* memcpy(void*, const void*, size_t)’ [-fpermissive]
__NTH (memcpy (void *__restrict __dest, const void *__restrict __src,

Any guidance will be much appreciated!

[Peter87]

memcpy expects a pointer to the data.

memcpy(buffer + 6, &packetLength, sizeof(packetLength));
memcpy(buffer + 8, &zoneIDLength, sizeof(zoneIDLength));

[abhishekm71]

In your memcpy function call, instead of packetLength and zoneIDLength, try &packetLength and &zoneIDLength.

This is because the compiler expects the second parameter of memcpy function to be a pointer.

Also, as a side note, AFAIK Variable Length Arrays are not supported at present in C++, so the following line is incorrect.

char buffer[packetLength];

[trolley01]

Thank you both. That makes sense, and now it compiles after including the ampersand at the beginning.

Regarding the variable length array, it is not intended to be variable, more that it is defined at the correct size. I could define packetLength as a const. Is there are better way to define a char array as a specific length?


After having the code compiling though, it is not generating the output that I was expecting. The memcpy(buffer + 12, zoneID.c_str(), zoneIDLength); line now does not seem to insert the information any more, and printing the buffer does not show the packetLength and zoneIDlength values.

So, having now learnt about the pointers, am I actually going about this whole construction of the packet to send in the best way? If I am, why is it not returning the values that I am expecting?

[abhishekm71]

Try using a debugger to see what is actually happening to your buffer array at each step.

There maybe a possible NULL termination issue.

Also, is there any specific reason that you are using memcpy, instead of the simpler, secure and robust std::string library functions?

To declare a char array of length packetLength, IMO packetLength should be a compile time constant. Hence zoneID should also be a compile time constant.

    //This value will be passed in to the function
    const string zoneID = "714f9b83b2beb170b19db6a281d2f9512";
     
    //Calculate all of the values that we need
    const short packetLength = 4 + 1 + 1 + 2 + 4 + zoneID.size();
    size_t zoneIDLength = zoneID.size();
    char buffer[packetLength];

Other way would be to use dynamic memory allocation with new and delete.

[trolley01]

Also, is there any specific reason that you are using memcpy, instead of the simpler, secure and robust std::string library functions?

No, not that I can think of. It just seemed at first as I was trying to deal with bytes of data and requiring data to be in specific places in the packet that a char array would be a simpler way to work with the information. I may need to go and do some more investigation into the std::string library.

Eventually this is going to be extracted into a function which is designed to take the zoneID and zoneType as input parameters, and return the constructed 'packet', so I can't make them both compile time constants.

[abhishekm71]

@trolley01
Using std::string would be much simpler and less error-prone. It will also eliminate the need to use a char array. the variable buffer can also be of type std::string, thus eliminating complications arising out of null terminations etc.

Read about std::string and std::stringstream libraries.

[trolley01]

I will do.

Thank you very much.

[dhayden]

Line 17: I think version is 1, not '1'. You can simplify my making line 22 buffer[4] = 1;

Lines 13 & 14: use string::size() to get the size of a string. E.g., zoneIDLength = zoneID.size(). It's faster because STL strings store the size as a member.

Same comment for lines 18 and 23.

For packetLength and zoneIDLength, you need to ensure that the byte order in the packet is correct. Usually numbers in network packets are big-endian. You can set them in a portable way with htons() ("host to network short") and htonl() ("host to network long"), so lines 24 & 25 should be:

    short tmps = htons(packetLength);
    memcpy(buffer + 6, &tmps, sizeof(tmps));
    long tmpl = htonl(zoneIdLength)
    memcpy(buffer + 8, &tmpl, sizeof(tmpl));

When reading the packet, you can recover the values using ntohs() and ntohl().

Come to think of it, are you sure you have the format right? You say the packet length is 2 bytes and the zone length is 4 bytes. What if the zone length is too big to fit in the packet length?

Definitely try using an std::string to hold the packet, but you may find that it's harder to do this sort of byte twiddling than what you have.

Another possibility is to encode the packet directly:

#pragma pack(push,1)
struct ZoneQueryPacket {
    char signature[4];
    char version[1];
    char type[1];
    unsigned short msgLen;
    unsigned long zoneLen;
    char zoneID[1];  // more space as needed
};
#pragma pack(pop)

...
char buffer[packetLength]
ZoneQueryPacket *zqp = (ZoneQueryPacket*)buffer;
memcpy(zqp->signature, "0hz ", 4);
zqp->version = 1;
zqp->type = 0;
zqp->msgLen = htons(packetLen);
zqp->zoneLen = htonl(zoneLen);
memcpy(zqp->zoneID, zoneID.c_str(), zoneID.size());

[trolley01]

Just a quick thanks to you all for your assistance. I have got it all working now. I did investigate strings, but in the end found it easier to use memcpy to ensure that I got everything in the right place.

Thanks again.