11
\$\begingroup\$

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;
}
\$\endgroup\$
2
  • 3
    \$\begingroup\$ "Created by macbook air"? (See here if you want to fix it.) \$\endgroup\$ Jul 5 '14 at 20:21
  • \$\begingroup\$ Hey, I am a total noob in the area. can you guide me on how to run this on ubuntu machine. ? \$\endgroup\$ Nov 27 '14 at 6:33
10
\$\begingroup\$

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)
\$\endgroup\$
5
  • \$\begingroup\$ thanks. So it seems you did not notice any major problems with the code? Your suggestions seem more stylystical although still legitimate (I will consider them). ps. although I get some warnings like "/Users/macbookair/Documents/tlv/tlv/tlv_chain.c:17:39: Incompatible pointer types passing 'int32_t *' (aka 'int *') to parameter of type 'unsigned char *'" in addint32 method etc. \$\endgroup\$
    – user38434
    Jul 5 '14 at 19:39
  • \$\begingroup\$ Good review (+1), but I have one quibble with this answer: names that end with _t are reserved for additional type names in the POSIX standard. \$\endgroup\$
    – syb0rg
    Jul 5 '14 at 21:00
  • \$\begingroup\$ Interesting, @syb0rg, I wasn't aware of that. The _t postfix is so commonly used for typedefs... \$\endgroup\$
    – glampert
    Jul 6 '14 at 0:05
  • \$\begingroup\$ @dmcr_code, one point I didn't notice, for the add_raw function, you actually use an unsigned char*. This is not a major issue, but will produce the warning you mention. Replace the unsigned char* with a const void*. That should fix the warning and make your interface more "standard". \$\endgroup\$
    – glampert
    Jul 6 '14 at 0:09
  • \$\begingroup\$ Many top C programmers recommend against the use of typedef struct. E.g. in the Linux kernel: kernel.org/doc/html/v4.10/process/coding-style.html#typedefs \$\endgroup\$
    – Lassi
    Nov 21 '18 at 16:15