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\$
4
  • \$\begingroup\$ What is the potential range of len? \$\endgroup\$ Commented May 22, 2020 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\$ Commented May 22, 2020 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
    Commented May 22, 2020 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\$ Commented May 23, 2020 at 1:38

2 Answers 2

4
\$\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;
}
\$\endgroup\$
6
  • \$\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
    Commented May 22, 2020 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\$ Commented May 22, 2020 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
    Commented May 22, 2020 at 21:33
  • \$\begingroup\$ @lucky1928 Yes code was amended \$\endgroup\$ Commented May 23, 2020 at 12:35
  • \$\begingroup\$ could a union be used to simplify the u32tobuf function? \$\endgroup\$
    – Shipof123
    Commented May 25, 2020 at 21:51
2
\$\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);
}
\$\endgroup\$
4
  • \$\begingroup\$ memset is awesome! \$\endgroup\$
    – lucky1928
    Commented May 22, 2020 at 21:10
  • \$\begingroup\$ @lucky1928 Indeed \$\endgroup\$
    – user220256
    Commented May 22, 2020 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\$ Commented May 22, 2020 at 21:18
  • \$\begingroup\$ The code contains way too many for loops that have exactly the same structure. \$\endgroup\$ Commented May 23, 2020 at 13:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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