Function for reading bits

I've made this simple function for reading N bits from an unsigned char* (with a possible offset) and I'm willing to share it here, get a review, suggestions for improving it, etc...

typedef struct bits_t {
    unsigned char *data;
    unsigned int len;
} bits_t;

bits_t *read_bits(unsigned char *src, int bits_offset, int nbits){

    unsigned int curr_bit, curr_byte, remaining_to_read, bit_position_in_byte;

    curr_bit = curr_byte = 0;
    remaining_to_read = nbits;

    bits_t *bits = malloc(sizeof(bits_t));

    bits->len = nbits;
    bits->data = calloc(1, bits->len);

    for(curr_bit = 1; curr_bit = bits_offset && remaining_to_read){
            curr_byte = (curr_bit - 1) / 8;
            bit_position_in_byte = (curr_bit - 1) - (curr_byte * 8);
            bits->data[remaining_to_read - 1] = read_bit(src[curr_byte], bit_position_in_byte);
            remaining_to_read--;
        }
    }

    return bits;
}

Main:

unsigned char *x = "oo";
bits_t *bits = read_bits(x, 0, 16);
for(i = 0; i len; i++){
    printf("%d", bits->data[i]);
}

Result: 0110111101101111

• Consider checking src[currentByte] for '\0' and abort when it's true (apparently the passed in string wasn't long enough). Depends on your usage though.

• Alternatively to the previous point you could add a source_length parameter to let the caller supply the information of how long the sequence really is.

-
I would consider changing the interface slightly. Have some methods which manage the object for you:

bits_t *new_bits_t(int nbits)
{
     bits_t *res = malloc(sizeof(bits_t));
     res->data = calloc(nbits);
     res->len = nbits;
     return res;
}

// frees the memory allocated for the structure and sets the reference to NULL
void delete_bits_t(bits_t **bits)
{
     if (bits == NULL || *bits == NULL) return;
     free((*bits)->data);
     free(*bits);
     *bits = NULL;
}

Then your read_bits function could fill in the structure passed in and also return error code in case the reading failed (i.e. source is too short):

int read_bits(unsigned char *src, bits_t *bits, int bits_offset)
{
    ...
}

-
bit_position_in_byte = (curr_bit - 1) - (curr_byte * 8); should be equivalent to bit_position_in_byte = (curr_bit - 1) % 8;

• Your loop is one based for some reason but inside the loop you have to subtract 1 from current_bit everywhere. You should just make your loop 0 based which would de-clutter the loop a bit.

• I would find bit_in_byte just as descriptive a name as bit_position_in_byte.

-
You could get rid of the remaining_to_read by calculating the position:

for (curr_bit = 0; curr_bit data[nbits - curr_bit - 1] = read_bit(src[input_byte], bit_in_byte);
}

• The loop apparently fills data from the end which seems little bit unexpected to me (kind of string reversal).

StackExchange Code Review Q#35555, answer score: 3