Reputation: 6689
I am getting a segmentation fault with the following code after adding structs to my queue.
The segmentation fault occurs when the MAX_QUEUE is set high but when I set it low (100 or 200), the error doesn't occur. It has been a while since I last programmed in C, so any help is appreciated.
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#define MAX_QUEUE 1000
struct myInfo {
char data[20];
};
struct myInfo* queue;
void push(struct myInfo);
int queue_head = 0;
int queue_size = 0;
int main(int argc, char *argv[])
{
queue = (struct myInfo*) malloc(sizeof(struct myInfo) * MAX_QUEUE);
struct myInfo info;
char buf[10];
strcpy(buf, "hello");
while (1)
{
strcpy(info.data, buf);
push(info);
}
}
void push(struct myInfo info) {
int next_index = sizeof(struct myInfo) * ((queue_size + queue_head) % MAX_QUEUE);
printf("Pushing %s to %d\n", info.data, next_index);
*(queue + (next_index)) = info;
queue_size++;
}
Output:
Pushing hello to 0
Pushing hello to 20
...
Pushing hello to 7540
Pushing hello to 7560
Pushing hello to 7580
Segmentation fault
Upvotes: 1
Views: 1376
Reputation: 620
You can treat queue as an array and then it should be simple to push items:
void push(struct myInfo info) { if (queue_size < MAX_QUEUE) { printf("Pushing %s to %d\n", info.data, queue_size); queue[queue_size] = info; queue_size++; } else { printf("ERROR: Queue is full.\n"); /* alternatively you could have a queue_insertion_point variable to keep track of where you are in the queue and use that as your index into your array. You'd then reset it to 0 (to wrap around) when it hit MAX_QUEUE. You need to ensure you don't overwrite data currently in the queue by comparing it against queue_head */ } }
Upvotes: 0
Reputation: 882776
I think your problem lies here:
int next_index = sizeof(struct myInfo) * ...
*(queue + (next_index)) = info;
You're scaling next_index
by the size of your structure but this is something done automatically by that second statement - *(queue + (next_index))
is equivalent to queue[next_index]
and the latter is more readable to all but those of us who have been using C since K&R was first published :-)
In other words, next_index
should be a value from 0
to MAX_QUEUE-1
, so try changing the first statement to remove the multiplication by sizeof(struct myInfo)
:
void push(struct myInfo info) {
int next_index = (queue_size + queue_head) % MAX_QUEUE;
printf("Pushing %s to %d\n", info.data, next_index);
queue[next_index] = info;
queue_size++;
}
And keep in mind that you'll eventually overflow queue_size
in that infinite loop of yours. You will presumably be checking to ensure that queue_size
is not incremented beyond MAX_QUEUE in the final production-ready code, yes?
Upvotes: 4
Reputation: 11694
*(queue + (next_index)) = info;
queue
is a pointer to a struct myInfo
. You only need to add 1 to it to get the next address -- you're treating it like a char *
.
You can just do:
*(queue + queue_size++) = info;
Upvotes: 0
Reputation: 182093
void push(struct myInfo info) {
int next_index = (queue_size + queue_head) % MAX_QUEUE;
printf("Pushing %s to %d\n", info.data, next_index);
queue[next_index] = info;
queue_size++;
}
Also, you don't need that temporary buf
:
int main(int argc, char *argv[])
{
queue = (struct myInfo*) malloc(sizeof(struct myInfo) * MAX_QUEUE);
while (1)
{
struct myInfo info; /* Seems you're using C99 so we can declare here */
strcpy(info.data, "hello");
push(info);
}
}
Upvotes: 0
Reputation: 41398
You're multiplying next_index
by sizeof(struct myInfo)
, which isn't necessary. When you add to a pointer type, the offset is automatically calculated in terms of the size of the pointed-to object. Changing the first line of push()
should be sufficient:
int next_index = (queue_size + queue_head) % MAX_QUEUE;
Upvotes: 1