Byte level length description

Question

I have a protocol that requires a length field up to 32-bits, and it must be generated at runtime to describe how many bytes are in a given packet.

The code below is kind of ugly but I am wondering if this can be refactored to be slightly more efficient or easily understandable. The problem is that the code will only generate enough bytes to describe the lenght of the packet, so less than 255 bytes = 1 byte of length, less than 65535 = 2 bytes of length etc...

{ extern char byte_stream[]; int bytes = offset_in_packet; int n = length_of_packet; /* Under 4 billion, so this can be represented in 32 bits. / int t; / 32-bit number used for temporary storage. */

    /* These are the bytes we will break up n into. */
    unsigned char first, second, third, fourth;

    t = n & 0xFF000000;
    /* We have used AND to "mask out" the first byte of the number. */
    /* The only bits which can be on in t are the first 8 bits. */
    first = t >> 24;
    if (t)  {
        printf("byte 1: 0x%02x\n",first );
        byte_stream[bytes] = first; bytes++;
        write_zeros = 1;
    }
    /* Now we shift t so that it is between 0 and 255. This is the first, highest byte of n. */
    t = n & 0x00FF0000;
    second = t >> 16;
    if (t || write_zeros) {
        printf("byte 2: 0x%02x\n", second );
        byte_stream[bytes] = second; bytes++;
        write_zeros = 1;
    }

    t = n & 0x0000FF00;
    third = t >> 8;
    if ( t || write_zeros) {
        printf("byte 3: 0x%02x\n", third );
        byte_stream[bytes] = third; bytes++;
        write_zeros = 1;
    }

    t = n & 0x000000FF;
    fourth = t;
    if (t || write_zeros) {
        printf("byte 4: 0x%02x\n", fourth);
        byte_stream[bytes] = fourth; bytes++;
    }
}

Answer 1

Try this loop:

{
 extern char byte_stream[];
 int bytes = offset_in_packet;
 int n = length_of_packet; /* Under 4 billion, so this can be represented in 32 bits. */
 int t; /* 32-bit number used for temporary storage. */
 int i;

 unsigned char curByte;

 for (i = 0; i < 4; i++) {
  t = n & (0xFF000000 >> (i * 16));

  curByte = t >> (24 - (i * 8));
  if (t || write_zeros)  {
   printf("byte %d: 0x%02x\n", i, curByte );
   byte_stream[bytes] = curByte;
                            bytes++;
   write_zeros = 1;
  }

 }

}

Answer 2

Why not just the full 4 bytes everytime? How does the entity which handles the packet know how many bytes you've used?

Answer 3

I'm not sure I understand your question. What exactly are you trying to count? If I understand correctly you're trying to find the Most Significant non-zero byte.
You're probably better off using a loop like this:

int i;  
int write_zeros = 0;  
for (i = 3; i >=0 ; --i) {  
    t = (n >> (8 * i)) & 0xff;  
    if (t || write_zeros) {  
        write_zeros = 1;  
        printf ("byte %d : 0x%02x\n", 4-i, t);  
        byte_stream[bytes++] = t;
    }  
}

Answer 4

You should really use a fixed-width field for your length.

• When the program on the receiving end has to read the length field of your packet, how does it know where the length stops?
• If the length of a packet can potentially reach 4 GB, does a 1-3 byte overhead really matter?
• Do you see how complex your code has already become?

Answer 5

Really you're only doing four calculations, so readability seems way more important here than efficiency. My approach to make something like this more readable is to

1. Extract common code to a function
2. Put similar calculations together to make the patterns more obvious
3. Get rid of the intermediate variable print_zeroes and be explicit about the cases in which you output bytes even if they're zero (i.e. the preceding byte was non-zero)

I've changed the random code block into a function and changed a few variables (underscores are giving me trouble in the markdown preview screen). I've also assumed that bytes is being passed in, and that whoever is passing it in will pass us a pointer so we can modify it.

Here's the code:

/* append byte b to stream, increment index */
/* really needs to check length of stream before appending */
void output( int i, unsigned char b, char stream[], int *index )
{
    printf("byte %d: 0x%02x\n", i, b);
    stream[(*index)++] = b;
}


void answer( char bytestream[], unsigned int *bytes, unsigned int n)
{
    /* mask out four bytes from word n */
    first  = (n & 0xFF000000) >> 24;
    second = (n & 0x00FF0000) >> 16;
    third  = (n & 0x0000FF00) >>  8;
    fourth = (n & 0x000000FF) >>  0;

    /* conditionally output each byte starting with the */
    /* first non-zero byte */
    if (first) 
       output( 1, first, bytestream, bytes);

    if (first || second) 
       output( 2, second, bytestream, bytes);

    if (first || second || third) 
       output( 3, third, bytestream, bytes);

    if (first || second || third || fourth) 
       output( 4, fourth, bytestream, bytes);
 }

Ever so slightly more efficient, and maybe easier to understand would be this modification to the last four if statements:

    if (n>0x00FFFFFF) 
       output( 1, first, bytestream, bytes);

    if (n>0x0000FFFF) 
       output( 2, second, bytestream, bytes);

    if (n>0x000000FF)  
       output( 3, third, bytestream, bytes);

    if (1) 
       output( 4, fourth, bytestream, bytes);

I agree, however, that compressing this field makes the receiving state machine overly complicated. But if you can't change the protocol, this code is much easier to read.

Answer 6

Why not just the full 4 bytes everytime? How does the entity which handles the packet know how many bytes you've used?

The protocol was handed to me and I get no say in how the protocol works. I only get to determine how my implementation will work.