3
\$\begingroup\$

Below code just for assemble data into network packet. but the code logic has many repeat parts to convert integer for byte array. what's the best method for such purpose to simplify below logic?

typedef struct _INFO {
    int pid;
    int score;
} INFO;

void send_packet(int connfd,INFO* info,int len) {
    int buflen = len*8 + 8;
    unsigned char* buf = (int*)malloc(buflen);
    int p = 0;
    buf[p++] = 0x55;
    buf[p++] = 0x55;
    buf[p++] = 0x55;
    buf[p++] = 0x55;

    buf[p++] = buflen & 0xFF;
    buf[p++] = (buflen >> 8) & 0xFF;
    buf[p++] = (buflen >> 16) & 0xFF;
    buf[p++] = (buflen >> 24) & 0xFF;
    for (int i=0;i<len;i++) {
        buf[p++] = info[i].pid & 0xFF;
        buf[p++] = (info[i].pid >> 8) & 0xFF;
        buf[p++] = (info[i].pid >> 16) & 0xFF;
        buf[p++] = (info[i].pid >> 24) & 0xFF;

        buf[p++] = info[i].score & 0xFF;
        buf[p++] = (info[i].score >> 8) & 0xFF;
        buf[p++] = (info[i].score >> 16) & 0xFF;
        buf[p++] = (info[i].score >> 24) & 0xFF;
    }
    write(connfd, buf, buflen);
    free(buf);
}
\$\endgroup\$
  • \$\begingroup\$ What is the potential range of len? \$\endgroup\$ – chux - Reinstate Monica May 22 at 20:45
  • \$\begingroup\$ BTW, a leading 0x55555555 looks like it has some weakness for the read() side to synchronize with. I look forward to seeing the read code. \$\endgroup\$ – chux - Reinstate Monica May 22 at 21:26
  • \$\begingroup\$ @chux - Reinstate Monica, for synchronize purpose, any keyword can be used I guess. some hardware guy tell me use 0x55555555. what's your suggestion? \$\endgroup\$ – lucky1928 May 22 at 21:35
  • \$\begingroup\$ Ideally the start-of-frame bytes never/rarely occur in the payload and don't look like a start-of-frame shifted with the beginning/trailing byte. Yet there is more than just good start-of-frame selection such as What type of framing to use in serial communication \$\endgroup\$ – chux - Reinstate Monica May 23 at 1:38
3
\$\begingroup\$

what's the best method for such purpose to simplify below logic?

With a helper function as there is repetitive code.

#include <stdint.h>

// Write 32-bit integer in little endian order to buf
unsigned char *u32tobuf(unsigned char *buf, uint32_t value) {
  buf[0] = value;
  buf[1] = value >> 8;
  buf[2] = value >> 16;
  buf[3] = value >> 24;
  return buf + 4;
}

I'd also 1) add some error checking, 2) drop the unneeded cast 3) prevent int overflow 4) use const.

// Return error status
int send_packet(int connfd, const INFO* info, int len) {
    uint32_t buflen = len*8lu + 8;
    unsigned char* buf = malloc(buflen);
    if (buf == NULL) {
      return 1; // or perhaps some enum
    }
    unsigned char* p = u32tobuf(buf, 0x55555555);
    p = u32tobuf(p, buflen);
    for (int i=0; i<len; i++) {
      p = u32tobuf(p, info[i].pid);
      p = u32tobuf(p, info[i].score);
    }
    ssize_t write_count = write(connfd, buf, buflen);
    free(buf);
    if (write_count != buflen) [
      return 1; // or perhaps some enum
    }
    return 0;
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Great, the cast is means & 0xFF? I saw java use such cast always. for C, I am not sure if we need it. \$\endgroup\$ – lucky1928 May 22 at 21:09
  • \$\begingroup\$ @lucky1928 Casting does has its place, yet it is often a sign of brittle learner's.code. With converting a void * to/from some other object *, it is not needed in C. \$\endgroup\$ – chux - Reinstate Monica May 22 at 21:17
  • \$\begingroup\$ Oh, the buf pointer has been moved to the end, so the write will be overflow. need to add a variable to remember the pointer header. \$\endgroup\$ – lucky1928 May 22 at 21:33
  • \$\begingroup\$ @lucky1928 Yes code was amended \$\endgroup\$ – chux - Reinstate Monica May 23 at 12:35
  • \$\begingroup\$ could a union be used to simplify the u32tobuf function? \$\endgroup\$ – Shipof123 May 25 at 21:51
1
\$\begingroup\$

A 0xFF character is all ones. So AnyChar & 0xFF will always return the same char. Also you can simplify the first 4 lines with memset(). You can also simplify a few of the lines with for loops:

#include <string.h>
void send_packet(int connfd,INFO* info,int len) {
    int buflen = len*8 + 8;
    unsigned char* buf = malloc(buflen);
    int p = 4;
    memset(buf, 0x55, 4);

    for (int i=0; i<4; i++) {
        buf[p++] = buflen >> i*8;
    }
    for (int i=0;i<len;i++) {
        for (int i=0; i<4; i++) {
            buf[p++] = info[i].pid >> i*8;
            buf[p++] = info[i].score >> i*8;
        }
    }
    write(connfd, buf, buflen);
    free(buf);
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ memset is awesome! \$\endgroup\$ – lucky1928 May 22 at 21:10
  • \$\begingroup\$ @lucky1928 Indeed \$\endgroup\$ – user12211554 May 22 at 21:15
  • 1
    \$\begingroup\$ for (int i=0; i<4; i++) { buf[p++] = info[i].pid >> i*8; buf[p++] = info[i].score >> i*8; } needs to be 2 loops. \$\endgroup\$ – chux - Reinstate Monica May 22 at 21:18
  • \$\begingroup\$ The code contains way too many for loops that have exactly the same structure. \$\endgroup\$ – Roland Illig May 23 at 13:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.