Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
}
share|improve this question
1  
"Created by macbook air"? (See here if you want to fix it.) –  Gareth Rees Jul 5 at 20:21
    
@GarethRees: thanks –  user38434 Jul 5 at 20:46

1 Answer 1

Typedef your structs:

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

struct tlv xyz;

I would suggest that you use a typedef for your struct 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.

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:

#define MAX_TLV_OBJECTS 50

Maybe consider returning error codes from your functions:

Instead of returning 0 or -1, a better approach would be to use some error 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 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)
share|improve this answer
    
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. –  user38434 Jul 5 at 19:39
    
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. –  syb0rg Jul 5 at 21:00
    
Interesting, @syb0rg, I wasn't aware of that. The _t postfix is so commonly used for typedefs... –  glampert Jul 6 at 0:05
    
@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". –  glampert Jul 6 at 0:09

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.