0

I need to be able to be able to send a numeric value to a remote socket server and so I need to encode possible numbers as bytes.

The numbers are up to 64 bit, ie requiring up to 8 bytes. The very first byte is the type, and it is always a number under 255 so fits in 1 byte.

For example, if the number was 8 and the type was a 32 bit unsigned integer then the type would be 7 which would be copied to the first (leftmost) byte and then the next 4 bytes would be encoded with the actual number (8 in this case).

So in terms of bytes:

byte1: 7
byte2: 0
byte3: 0
byte4: 0
byte5: 8

I hope this is making sense.

Does this code to perform this encoding look like a reasonable approach?

int type = 7;
uint32_t number = 8;

unsigned char* msg7 = (unsigned char*)malloc(5);
unsigned char* p = msg7;

*p++ = type;

 for (int i = sizeof(uint32_t) - 1; i >= 0; --i) 
    *p++ = number & 0xFF << (i * 8);
Tom Zych
  • 13,329
  • 9
  • 36
  • 53
Angus Comber
  • 9,316
  • 14
  • 59
  • 107

4 Answers4

3

You'll want to explicitly cast type to avoid a warning:

*p++ = (unsigned char) type;

You want to encode the number with most significant byte first, but you're shifting in the wrong direction. The loop should be:

for (int i = sizeof(uint32_t) - 1; i >= 0; --i)
    *p++ = (unsigned char) ((number >> (i * 8)) & 0xFF);

It looks good otherwise.

Tom Zych
  • 13,329
  • 9
  • 36
  • 53
  • You've also fixed another issue you haven't mentioned: his code was shifting the 0xff mask, not `number`, so 3 out of 4 of the bytes were always set to zero. It's fixed in yours, though. – Dmitri Sep 10 '11 at 19:52
  • As in this case, whenever I'm not sure of precedence, I assume no one else will be sure either and I use parentheses to make it clear. – Tom Zych Sep 10 '11 at 19:54
0

Do you mean?

for (int i = sizeof(uint32_t) - 1; i >= 0; --i)
  *p++ = (number >> (i * 8)) & 0xFF;

Another option to might be to do

// this would work on Big endian systems, e.g. sparc
struct unsignedMsg {
    unsigned char type;
    uint32_t value;
}

unsignedMsg msg;
msg.type = 7;
msg.value = number;
unsigned char *p = (unsigned char *) &msg;

or

unsigned char* p = 
p[0] = 7;
*((uint32_t *) &(p[1])) = number;
Peter Lawrey
  • 525,659
  • 79
  • 751
  • 1,130
0

Your code is reasonable (although I'd use uint8_t, since you are not using the bytes as “characters”, and Peter is of course right wrt the typo), and unlike the commonly found alternatives like

uint32_t number = 8;
uint8_t* p = (uint8_t *) &number;

or

union {
  uint32_t number;
  uint8_t bytes[4];
} val;
val.number = 8;
// access val.bytes[0] .. val.bytes[3]

is even guaranteed to work. The first alternative will probably work in a debug build, but more and more compilers might break it when optimizing, while the second one tends to work in practice just about everywhere, but is explicitly marked as a bad thing™ by the language standard.

Christopher Creutzig
  • 8,656
  • 35
  • 45
0

I would drop the loop and use a "caller allocates" interface, like

int convert_32 (unsigned char *target, size_t size, uint32_t val)
{
if (size < 5) return -1;

target[0] = 7;
target[1] = (val >> 24) & 0xff;
target[2] = (val >> 16) & 0xff;
target[3] = (val >> 8) & 0xff;
target[4] = (val) & 0xff;

return 5;
}

This makes it easier for the caller to concatenate multiple fragments into one big binary packet and keep track of the used/needed buffer size.

wildplasser
  • 43,142
  • 8
  • 66
  • 109