Type-length-value (TLV) encode/decode

I wrote these methods to encode data as array of TLV objects, and also to serialize and deserialize them. Any feedback on improvements, etc. would be appreciated.

Please note that I ignored endianness issues so far, as they are not relevant for me.
Also I am using fixed size array for TLV objects which should be fine for me at this stage.

ps. similarly I also plan to create functions like add_int16, add_uint32, etc. probably I will have to use respective parameters in printf later on, to avoid UB?

tlv_chain.c

```
//
// tlv_chain.c
// tlv
//
// Created by macbook air on 1/17/12.
// Copyright (c) 2012 macbook air. All rights reserved.
//

#include "tlv_chain.h"
#include
#include
#include

int32_t tlv_chain_add_int32(struct tlv_chain *a, int32_t x)
{
return tlv_chain_add_raw(a, 1, 4, &x);
}

// add tlv object which contains null terminated string
int32_t tlv_chain_add_str(struct tlv_chain a, char str)
{
return tlv_chain_add_raw(a, 2, strlen(str) + 1, str);
}

int32_t tlv_chain_add_raw(struct tlv_chain a, unsigned char type, int16_t size, unsigned char bytes)
{
if(a == NULL || bytes == NULL)
return -1;

// all elements used in chain?
if(a->used == 50)
return -1;

int index = a->used;
a->object[index].type = type;
a->object[index].size = size;
a->object[index].data = malloc(size);
memcpy(a->object[index].data, bytes, size);

// increase number of tlv objects used in this chain
a->used++;

// success
return 0;
}

int32_t tlv_chain_free(struct tlv_chain *a)
{
if(a == NULL)
return -1;

for(int i =0; i used; i++)
{
free(a->object[i].data);

a->object[i].data = NULL;
}

return 0;
}

// serialize the tlv chain into byte array
int32_t tlv_chain_serialize(struct tlv_chain a, unsigned char dest, / out / int32_t* count)
{
if(a == NULL || dest == NULL)
return -1;

// Number of bytes serialized
int32_t counter = 0;

for(int i

Typedef your structs:

Personally, I don't like having to write struct every time when using a user defined type:

struct tlv xyz;

I would suggest that you use a typedef for your structured types:

typedef struct tlv
{
    int8_t type;    // type
    uint8_t * data; // pointer to data
    int16_t size;   // size of data

} tlv_t;

typedef struct tlv_chain
{
    struct tlv object[50];
    uint8_t used; // keep track of tlv elements used

} tlv_chain_t;

This will also give users of your code the option of choosing their favorite way.
Note however that the _t suffix, which I have used in this example, is reserved for future use by the POSIX standard (thanks @syb0rg for pointing that out). If you aim for full compatibility with that standard, then it is better to avoid the common _t suffix.

Make consistent use of const:

In the function:

int32_t tlv_chain_add_str(struct tlv_chain *a, char *str);

The str parameter is read-only (you are passing char* literals to it on your test), so it should be const, to enforce this constraint and avoid accidental change of the parameter inside the function:

int32_t tlv_chain_add_str(struct tlv_chain *a, const char *str);
                                               ^^^^^

The same goes for these other two:

int32_t tlv_chain_add_raw(struct tlv_chain *a, unsigned char type, int16_t size, unsigned char *bytes);
int32_t tlv_chain_deserialize(unsigned char *src, struct tlv_chain *dest, int32_t length);

Rewrite them as:

int32_t tlv_chain_add_raw(struct tlv_chain *a, unsigned char type, int16_t size, const unsigned char *bytes);
int32_t tlv_chain_deserialize(const unsigned char *src, struct tlv_chain *dest, int32_t length);

Adding const to bytes and src.

Avoid "magic" numbers:

You have a few lines like this one in tlv_chain.c:

if(dest->used == 50)
    return -1;

50 is the size of the object[] array of tlv_chain. Replace these raw constants with a #define or enum named constant:

#define MAX_TLV_OBJECTS 50

Consider returning error codes from your functions:

Instead of returning 0 or -1, a better approach would be to use some error-code constants:

typedef enum tlv_result {

    TLV_SUCESS,
    TLV_ERROR_1,
    TLV_ERROR_2,
    // etc...

} tlv_result_t;

Replacing the number in the TLV_ERROR_* constants with a description of the error, of course. E.g.: TLV_ERROR_OUT_OF_MEMORY.

Then replace the return type of the functions with tlv_result_t:

tlv_result_t tlv_chain_print(struct tlv_chain *a);

Other general suggestions:

In the function:

int32_t tlv_chain_print(struct tlv_chain *a);

You use printf to dump the TLV object. It might be interesting to allow the user to provide his/her own printer function or even a pointer to a FILE (stream) object, which can be STDOUT or a user file. E.g:

int32_t tlv_chain_print(struct tlv_chain *a, void(*printer)(const char *))

Or

int32_t tlv_chain_print(struct tlv_chain *a, FILE *dest_stream)

StackExchange Code Review Q#56203, answer score: 10