Cfrtjryk

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 <stdio.h>

#include <string.h>

#include <stdlib.h>



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 < a->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 = 0; i < a->used; i++)

    {

        dest[counter] = a->object[i].type;

        counter++;



        memcpy(&dest[counter], &a->object[i].size, 2);

        counter += 2;



        memcpy(&dest[counter], a->object[i].data, a->object[i].size);

        counter += a->object[i].size;

    }



    // Return number of bytes serialized

    *count = counter;



    // success

    return 0;

}



int32_t tlv_chain_deserialize(unsigned char *src, struct tlv_chain *dest, int32_t length)

{

    if(dest == NULL || src == NULL)

        return -1;



    // we want an empty chain

    if(dest->used != 0)

        return -1;



    int32_t counter = 0;

    while(counter < length)

    {

        if(dest->used == 50)

            return -1;



        // deserialize type

        dest->object[dest->used].type = src[counter];

        counter++;



        // deserialize size

        memcpy(&dest->object[dest->used].size, &src[counter], 2);

        counter+=2;



        // deserialize data itself, only if data is not NULL

        if(dest->object[dest->used].size > 0)

        {

            dest->object[dest->used].data = malloc(dest->object[dest->used].size);

            memcpy(dest->object[dest->used].data, &src[counter], dest->object[dest->used].size);

            counter += dest->object[dest->used].size;

        }else

        {

            dest->object[dest->used].data = NULL;

        }



        // increase number of tlv objects reconstructed

        dest->used++;

    }



    // success

    return 0;



}







int32_t tlv_chain_print(struct tlv_chain *a)

{

    if(a == NULL)

        return -1;



    // go through each used tlv object in the chain

    for(int i =0; i < a->used; i++)

    {



        if(a->object[i].type == 1)

        {

            // int32

            int32_t x;

            memcpy(&x, a->object[i].data, sizeof(int32_t));

            printf("%d n",x);



        }else if(a->object[i].type == 2)

        {

            // string

            printf("%s n",a->object[i].data);

        }

    }





    return 0;

}

tlv_chain.h

//

//  tlv_chain.h

//  tlv

//

//  Created by macbook air on 1/17/12.

//  Copyright (c) 2012 macbook air. All rights reserved.

//



#ifndef tlv_tlv_chain_h

#define tlv_tlv_chain_h

#include <stdint.h>



// TLV data structure

struct tlv

{

    int8_t type;    // type

    uint8_t * data; // pointer to data

    int16_t size;   // size of data

};



// TLV chain data structure. Contains array of (50) tlv

// objects. 

struct tlv_chain

{

    struct tlv object[50];

    uint8_t used; // keep track of tlv elements used

};



int32_t tlv_chain_add_int32(struct tlv_chain *a, int32_t x);

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

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

int32_t tlv_chain_serialize(struct tlv_chain *a, unsigned char *dest, int32_t *count);

int32_t tlv_chain_deserialize(unsigned char *src, struct tlv_chain *dest, int32_t length);

int32_t tlv_chain_print(struct tlv_chain *a);

int32_t tlv_chain_free(struct tlv_chain *a);



#endif

Test program which works ok:

//

//  main.c

//  tlv

//

//  Created by macbook air on 1/17/12.

//  Copyright (c) 2012 macbook air. All rights reserved.

//



#include <stdio.h>

#include "tlv_chain.h"

#include <stdio.h>

#include <string.h>

#include <stdlib.h>



int main(int argc, const char * argv)

{

    struct tlv_chain chain1, chain2;

    memset(&chain1, 0, sizeof(chain1));

    memset(&chain2, 0, sizeof(chain2));

    unsigned char chainbuff[2048] = {0};

    int32_t l = 0;



    tlv_chain_add_int32(&chain1, 31144);

    tlv_chain_add_str(&chain1, "george");

    tlv_chain_add_int32(&chain1, 7);

    tlv_chain_add_str(&chain1, "998967-44-33-44-12");

    tlv_chain_add_str(&chain1, "Grand Chamption Atlanta; details: Ave12");

    tlv_chain_add_int32(&chain1, 7900);



    // serialization/deserialization test

    tlv_chain_serialize(&chain1, chainbuff, &l);

    tlv_chain_deserialize(chainbuff, &chain2, l);



    // print the tlv chain contents

    tlv_chain_print(&chain2);



    // free each chain

    tlv_chain_free(&chain1);

    tlv_chain_free(&chain2);



    return 0;

}

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)