Simple protocol client library

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));
}

```