CODEWITHSUNDEEP
cfilefile-iobuffer
Deniz Cetinalp
Deniz Cetinalp

Reputation: 911

Circular (ring) buffer write method in C

Im trying to create an exact copy of a text file, and I am using a circular buffer.

The write method I am using is:

void addItem(CircularBuffer *cBuff, BufferItem *cbItem) {
    cBuff->cBuffItems[cBuff->lastInd] = *cbItem;
    cBuff->lastInd = ( ((cBuff->lastInd) + 1) % cBuff->size);
    if (cBuff->lastInd == cBuff->startInd)
    {
        cBuff->startInd = (cBuff->startInd + 1) % cBuff->size; // Overwriting full buffer.
    }
}

And here is how I am copying each char into the buffer:

BufferItem result;
char ch;
while( ( ch = fgetc(fp) ) != EOF ){
    result.offset = ftell(fp);
    result.data = ch;
    addItem(&cBuff, &result);
}

It only writes the first three characters then gives me a segmentation fault. I made sure the buffer size is big enough, ive tried different datasets, all give the same result (only the first 3 chars copied into buffer).

If I dont add it to the buffer, and just print result.offset, and result.data I get what I expect. So addItem must be the problem, If I delete the second line in addItem, it works but obviously it just overwrites the first spot.

What am I doing wrong?

Edit:

Here is the circular buffer and circular buffer implementation:

// Circular buffer items.
typedef  struct {
     char  data ;
     off_t offset ;
} BufferItem ;

// Circular buffer
typedef struct {
    int startInd; // Index of first element added to buffer.
    int lastInd; // Index of most recent element added to buffer.
    int size; // Number of elements in circular buffer.
    BufferItem *cBuffItems; // Circular buffer items.
} CircularBuffer;

void initializeBuffer(CircularBuffer *cBuff, int size) {
    cBuff->cBuffItems = calloc(cBuff->size, sizeof(BufferItem));
    cBuff->size  = size + 1;
    cBuff->startInd = 0;
    cBuff->lastInd   = 0;
}

And like I mentioned, I did initialize the buffer, here is a simplified version of main:

int main( int argc, char *argv[] )
{
    if (argc != 4)
    {
        printf("Expected 3 arguments, received %d\n", argc - 1);
        return 1;
    }

    int bufSize; // Capacity of BufferItems in circular buffer.
    char *file; // Pathname of file to be copied.
    char *copy; // Name to be given to the copy.

    sscanf(argv[1], "%d", &bufSize);
    file = argv[2];
    copy = argv[3];

    initializeBuffer(&cBuff, bufSize);

    // Open file to be copied.
    FILE *fp = fopen(file, "r" );

    // Create copy file.
    FILE *cp = fopen(copy, "w+" ); // Overwrite if file exists.

    BufferItem result;
    char ch;
    while( ( ch = fgetc(fp) ) != EOF ){
        result.offset = ftell(fp);
        result.data = ch;
        addItem(&cBuff, &result);
    }

    fclose(fp);
    fclose(cp);

    return 0;
}// Main.

Upvotes: 0

Views: 1223

Answers (1)

Jonathan Leffler
Jonathan Leffler

Reputation: 754570

Once the initialize code is shown, the problem is clear: you are using cBuff->size before you set it. 

void initializeBuffer(CircularBuffer *cBuff, int size) {
    cBuff->cBuffItems = calloc(cBuff->size, sizeof(BufferItem));
    cBuff->size  = size + 1;
    cBuff->startInd = 0;
    cBuff->lastInd   = 0;
}

Consequently, you're using a quasi-random size, and getting correspondingly quasi-random results. As you can now see, it is the code you didn't show originally that is causing the trouble — that's why we have to demand to see a workable example!

You can fix it pretty trivially:

void initializeBuffer(CircularBuffer *cBuff, int size) {
    cBuff->cBuffItems = calloc(size + 1, sizeof(BufferItem));
    cBuff->size  = size + 1;
    cBuff->startInd = 0;
    cBuff->lastInd   = 0;
}

This uses the parameter size instead of the uninitialized element of cBuff. I suggest that allocating one number of items and then saying that there is a different number of them available for use is going to cause trouble. I've opted to add one to both numbers; on the whole, though, it might be better to use the value of size without the increment.

You could also reorder the assignments (so you set cBuff->size before you allocate cBuff->cBuffItems); that would also work (as long as you make the allocation size consistent with the recorded size.

You should, arguably, check the result of calloc().

Upvotes: 1

Related Questions