1
\$\begingroup\$

Overview

I got rubber-stamped in code review today even though I called out specific concerns about my own code and asked for advice, so I'm turning to this community for advice. Thank you for taking a look!

I'm implementing a simple protocol in C. The protocol is composed mostly of fixed-width messages, each of which can be directly expressed as a single struct.

I've abstracted the protocol in this post for simplicity and to focus on one aspect that is complicating the design of my client library.

Quick note: this is in an embedded environment, I am trying to avoid heap allocations, and all the structs are packed so byte alignment isn't done on their underlying layout.

Description

Each message has a message id, which are defined in an enum:

typedef enum {
    NJ_MESSAGE_ID_LOGIN_REQUEST             = 0x00000000,
    NJ_MESSAGE_ID_LOGIN_RESPONSE            = 0x00008000,
    NJ_MESSAGE_ID_REGISTER_REQUEST          = 0x00000100,
    NJ_MESSAGE_ID_REGISTER_RESPONSE         = 0x00008100,
} nj_message_id;

All messages have a header, which is defined in a struct:

typedef struct {
    uint16_t    protocol_version;
    uint16_t    message_id;
    uint16_t    sequence_id;
} nj_header;

Each message has its own struct that starts with the header:

typedef struct {
    nj_header   header;
    uint8_t     username[32];
    uint8_t     password[32];
} nj_login_request;

Finally, there is the nj_send function:

void nj_send(nj_header *message, nj_message_id message_id);

The nj_send function accepts a pointer to nj_header but it is expected that this is actually a pointer to a full message that has been casted.

Here is an example of how to construct and send a message:

#include <string.h>
#include "nj.h"

int main(void)
{
    nj_login_request login_request = { 0 };
    strncpy(login_request.username, "joe", sizeof(login_request.username));
    strncpy(login_request.password, "joe", sizeof(login_request.password));
    nj_send((nj_header *) login_request, NJ_MESSAGE_ID_LOGIN_REQUEST);
}

I like this because:

  • It's not verbose and easy to do on the stack.
  • Working directly with the flat struct enables sizeof.
  • The object is in exactly the same format it will be on the wire.

Challenge

Most of the protocol is fixed-width, and for those messages the example above is perfect.

There is one message, however, that is variable-width: the register request. In this message, the client may specify zero or more "services", each of which have an "id" and "flags".

So a definition for that message type might look like this:

typedef struct {
    uint8_t     id;
    uint8_t     flags;
}

typedef struct {
    nj_header   header;
    uint8_t     services_count;
    nj_services *services;
} nj_register_request;

However, this is no longer a flat struct. That makes things difficult to deal with on the stack, and complicates my nj_send logic which so far has been able to assume that the pointer it is given refers to a byte-for-byte flat representation of the full image.

To address that, I've created a struct nj_register_request_partial (rather than nj_register_request, which doesn't exist in my code):

typedef struct {
    nj_header   header;
    uint8_t     services_count;
    // will be followed by some number of `nj_service` structs
} nj_register_request_partial;

And two functions that get and set services within a buffer which is assumed to start with the nj_register_request_partial struct:

nj_service *nj_get_register_request_service(nj_register_request_partial *request,
                                            uint8_t index)
{
    off_t offset = 0;

    if (index > request->services_count)
    {
        return NULL;
    }

    offset = sizeof(nj_register_request_partial) + sizeof(nj_service) * index;
    return (nj_service *) request + offset;
}

void nj_set_register_request_service(nj_register_request_partial *request,
                                     nj_service *service, uint8_t index)
{
    off_t offset = 0;

    if (index > request->services_count)
    {
        return;
    }

    offset = sizeof(nj_register_request_partial) + sizeof(nj_service) * index;
    memcpy(request + offset, service, sizeof(nj_service));
}

To construct and send a register request message would look like this:

#include <stdint.h>
#include <string.h>
#include "nj.h"

int main(void)
{
    // create a buffer of the maximum size, then a pointer to the front of
    // that buffer. the pointer represents the portion that is described by
    // the `nj_register_request_partial` struct. the remaining bytes can store
    // instances of the `nj_service` struct.

    uint8_t buffer[NJ_MAX_MESSAGE_LEN] = { 0 };
    nj_register_request_partial *request = (nj_register_request_partial *) buffer;
    request->services_count = 2;

    for (int i = 0; i < 2; i++)
    {
        nj_service service = { 0 };
        service.id = i;
        service.flags = 0x00;
        nj_set_register_request_service(request, &service, i);
    }

    nj_send((nj_header *) request, NJ_MESSAGE_ID_REGISTER_REQUEST);
}

I'm not satisfied with this because:

  • It requires allocating a buffer for the max message length.
  • It requires the user to do type punning in a non-obvious way (if they want to avoid heap allocations).
  • But most of all, the message definition is not obvious from looking at the struct definition.

However, I do like that I'm still dealing with the direct byte representation of what will go on the wire.

Is there a better way to represent variable-width messages as a composition of structs?


Full Source

nj.h

#pragma once

#include <stdint.h>

#define NJ_PROTOCOL_VERSION                 0x00000001
#define NJ_MAX_MESSAGE_LEN                  256

typedef enum {
    NJ_MESSAGE_ID_LOGIN_REQUEST             = 0x00000000,
    NJ_MESSAGE_ID_LOGIN_RESPONSE            = 0x00008000,
    NJ_MESSAGE_ID_REGISTER_REQUEST          = 0x00000100,
    NJ_MESSAGE_ID_REGISTER_RESPONSE         = 0x00008100,
} nj_message_id;

#pragma pack(push, 1)

typedef struct {
    uint8_t     id;
    uint8_t     flags;
} nj_service;

typedef struct {
    uint16_t    protocol_version;
    uint16_t    message_id;
    uint16_t    sequence_id;
} nj_header;

typedef struct {
    nj_header   header;
    uint8_t     username[32];
    uint8_t     password[32];
} nj_login_request;

typedef struct
{
    nj_header   header;
    uint8_t     error;
} nj_login_response;

typedef struct
{
    nj_header   header;
    uint8_t     sevices_count;
    // will be followed by some number of `nj_service` structs
} nj_register_request_partial;

typedef nj_login_response nj_register_response;

#pragma pack(pop)

void nj_send(nj_header *message, nj_message_id message_id);

nj_service *nj_get_register_request_service(nj_register_request_partial *request,
                                            uint8_t index);

void nj_set_register_request_service(nj_register_request_partial *request,
                                     nj_service *service, uint8_t index);

nj.c

#include <stdio.h>
#include <string.h>
#include "nj.h"

static uint16_t nj_get_unique_sequence_id(void)
{
    static uint16_t rolling_counter = 0;
    return rolling_counter++;
}

static void nj_populate_header(nj_header *message, nj_message_id message_id)
{
    message->protocol_version = NJ_PROTOCOL_VERSION;
    message->message_id = message_id;
    message->sequence_id = nj_get_unique_sequence_id();
}

static size_t nj_sizeof(nj_header *msg)
{
    if (msg->message_id == NJ_MESSAGE_ID_REGISTER_REQUEST)
    {
        nj_register_request_partial *req = (nj_register_request_partial *) message;
        size_t size = sizeof(nj_register_request_partial);
        size += req->services_count * sizeof(nj_service);
        return size;
    }

    switch (msg->message_id)
    {
    case NJ_MESSAGE_ID_LOGIN_REQUEST:
        return sizeof(nj_login_request);
    case NJ_MESSAGE_ID_LOGIN_RESPONSE:
        return sizeof(nj_login_response);
    case NJ_MESSAGE_ID_REGISTER_RESPONSE:
        return sizeof(nj_register_response);
    default:
        break;
    }

    return 0;
}

void nj_send(nj_header *message, nj_message_id message_id)
{
    uint8_t buffer[NJ_MAX_MESSAGE_LEN] = { 0 };
    size_t size = nj_sizeof(message);
    size_t iterator = 0;

    if (message == NULL)
    {
        return;
    }

    nj_populate_header(message, message_id);

    // implementation not important
    // `message` is treated as a void pointer and its bytes are put directly
    // onto the wire. single datagram packet.

    // instead i'm just printing the buffer.
    memcpy(buffer, message, size);
    while (iterator < size)
    {
        printf("0x%02x ", buffer[iterator++]);

        if (iterator % 0x10 == 0)
        {
            printf("\n");
        }
    }
    printf("\n");
}

nj_service *nj_get_register_request_service(nj_register_request_partial *request,
                                            uint8_t index)
{
    off_t offset = 0;

    if (index > request->services_count)
    {
        return NULL;
    }

    offset = sizeof(nj_register_request_partial) + sizeof(nj_service) * index;
    return (nj_service *) request + offset;
}

void nj_set_register_request_service(nj_register_request_partial *request,
                                     nj_service *service, uint8_t index)
{
    off_t offset = 0;

    if (index > request->services_count)
    {
        return;
    }

    offset = sizeof(nj_register_request_partial) + sizeof(nj_service) * index;
    memcpy(request + offset, service, sizeof(nj_service));
}

```
\$\endgroup\$
  • \$\begingroup\$ Where is the definition of the type nj_register_request? \$\endgroup\$ – Cacahuete Frito Jul 11 at 15:59
  • \$\begingroup\$ @CacahueteFrito hello! that was a mistake, it should be nj_register_request_partial. i've updated the post. \$\endgroup\$ – Woodrow Barlow Jul 11 at 16:25
  • \$\begingroup\$ I don't see what is going on in these lines: uint8_t buffer[NJ_MAX_MESSAGE_LEN] = { 0 }; nj_register_request_partial *request = (nj_register_request_partial *) buffer; . The size of the struct is fixed, so I don't see why you need to use an array and not just directly declare a struct. Could you add some extra comments about that? \$\endgroup\$ – Cacahuete Frito Jul 11 at 18:02
  • \$\begingroup\$ @CacahueteFrito your confusion pretty good evidence that my solution leaves a lot to be desired. :) i've tried to explain that a little bit more in my most recent edit. \$\endgroup\$ – Woodrow Barlow Jul 11 at 18:33
  • \$\begingroup\$ @CacahueteFrito basically, because nj_register_request_partial is a partial definition of the full register request message. it will be followed by some number of nj_service structs, and i'm trying to find an elegant way of expressing that while keeping the byte representation flat. \$\endgroup\$ – Woodrow Barlow Jul 11 at 18:35
1
\$\begingroup\$

for

size_t iterator = 0;
while (iterator < size) {
        printf("0x%02x ", buffer[iterator++]);
        if (iterator % 0x10 == 0)
        {
                printf("\n");
        }
}

should use a for loop:

for (size_t i = 0; i < size; i++) {
        printf("0x%02X", buffer[i]);
        if ((i + 1) % 16)
                putchar(' ');
        else
                putchar('\n');
}

Note that I also removed the trailing space, which was unneeded.

Also note that I used uppercase for the hex number to differentiate it from the 0x prefix.


const

Functions that accept a pointer but are not going to modify it's contents should note this, to allow some optimizations, and as documentation:

static size_t nj_sizeof(const nj_header *msg)

typedef

I would advise to differentiate typedef names from any other name, so that when I read the code it is visually clear that it's a type, and not the name of a variable or function.

Being typedefs to struct I would add the suffix _s:

struct My_Struct {
        int a;
};
typedef struct My_Struct my_struct_s;

However, I wouldn't even typedef. It just hides the information that the type is a struct, which I prefer to know even if I write 5 extra characters


ARRAY_SIZE()

I recently found an interesting thing:

Consider your line strncpy(login_request.username, "joe", sizeof(login_request.username));

It's obvious that you know that login_request.username is a char [], but let's imagine that some day you (or worse, someone else that doesn't have the same knowledge of the program as you) change it to be a char *. That line of the strncpy will probably get unnoticed, but will produce a wrong result.

Solution: Use the following:

#define ARRAY_SIZE(a)   (sizeof(a) / sizeof((a)[0]))

strncpy(login_request.username, "joe", ARRAY_SIZE(login_request.username));

If the array changes to a pointer, it will be noticed by the compiler (at least on a decent compiler). On GCC this is a warning: -Wsizeof-pointer-div (GCC 8 has it enabled in -Wall. It's a very recent warning, and don't expect to have it in old GCC versions).

\$\endgroup\$
  • \$\begingroup\$ hi! these are good style comments, thank you. regarding for: our style guide requires declaring and initializing all variables at the top of scope. so, generally, while ends up getting used instead of for in most cases (since the iterator is already initialized). we actually have found it to be liberating, because while is easier to read (more closely matches the way humans speak naturally) and it lets us do things after the increment (avoiding the + 1 in your if). \$\endgroup\$ – Woodrow Barlow Jul 11 at 14:44
  • \$\begingroup\$ const is absolutely a good idea and an oversight on my part. as for the typedefs, our style guide discourages hungarian notation except in the case of real-world units like "ms" (milliseconds), but i see your point about hiding the underlying data so ultimately i'm going to do away with the typedefs entirely and just use (e.g.) struct nj_login_request and enum nj_message_id directly. \$\endgroup\$ – Woodrow Barlow Jul 11 at 14:47
  • \$\begingroup\$ any advise for a clean way to manage variable-width messages as a composition of multiple structs? \$\endgroup\$ – Woodrow Barlow Jul 11 at 14:48

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.