0

I'm using a simple queue implementation and I'm trying to make a simple program that has two threads: first one queue events on the queue and the second one processes them. I'm using mutexes to avoid sync problems. However, I'm still getting quite often (not always) segmentation faults.

Mind you, I built my codes using the -D_REENTRANT option as well. Also, the code works perfectly fine if queue/dequeue is called from one thread only.

Here's the simple code that shows how I am synchronizing the stuff:

int main(void) {
    init_queue(&my_queue);

    pthread_t thread_queue, thread_process;
    int iret1, iret2;

    iret1 = pthread_create( &thread_queue, NULL, svetlin_queue_events, (void*) NULL);
    iret2 = pthread_create( &thread_process, NULL, svetling_process_events, (void*) fb);

    pthread_join(thread_queue, NULL);
    pthread_join(thread_process, NULL);
}

the queuing function:

void svetlin_queue_events(void * v) {
    int fd;
    if ((fd = open("/dev/input/mouse1", O_RDONLY)) == -1) {
        printf("error with touchscreen device\n");
        exit(1);
    }

    struct input_event ev;
    struct input_event *being_sent;
    int rd;

    while (1) {
        read(fd, &ev, sizeof(struct input_event));

        being_sent = malloc(sizeof(struct input_event));
        memcpy(being_sent, &ev, sizeof(struct input_event));

        pthread_mutex_lock(&my_mutex);
        enqueue(&my_queue, being_sent);
        pthread_mutex_unlock(&my_mutex);

        printf("enqueueing...\n");
    }
}

and the processing function:

void svetling_process_events(void *v) {

    printf("FB pointer is: %p\n", fb);

    int x, y = 0;
    int has_item = 0;
    struct input_event **being_fetched;
    struct input_event *ev;

    while(1) {

        pthread_mutex_lock(&my_mutex);
        has_item = dequeue(&my_queue, being_fetched);
        pthread_mutex_unlock(&my_mutex);

        if (has_item) {
            ev = *being_fetched;
            printf("dequeueing...\n");
            if (ev->type == 3) {
                if (ev->code == 0) {
                    x = ev->value;
                }

                if (ev->code == 1) {
                    y = ev->value;
                }
            }
        }
    }
}

I hope I'm not overwhelming you with so much code, but it's something simple really.

So my question is what regarding synchronization am I doing wrong?

Thanks!

UPDATE

Here's the queue implementation:

struct queue_node
{
    struct queue_node *next;
    void *data;
};

struct queue
{
    struct queue_node *first;
    struct queue_node *last;
};

int enqueue(struct queue *q, void * const value)
{
    struct queue_node *node = malloc(sizeof(struct queue_node));

    if (node == NULL) {
        errno = ENOMEM;
        return 1;
    }

    node->data = value;

    if (q->first == NULL) {
        q->first = q->last = node;
    } else {
        q->last->next = node;
        q->last = node;
    }

    node->next = NULL;
    return 0;
}

int dequeue(struct queue *q, void **value)
{
    if (!q->first) {
        value = NULL;
        return 0;
    }
    *value = q->first->data;
    q->first = q->first->next;
    return 1;
}

void init_queue(struct queue *q)
{
    q->first = q->last = NULL;
}

int queue_empty_p(const struct queue *q)
{
    return q->first == NULL;
}

It segfaults on the line *value = q->first->data; in dequeue.

Thanks!

4
  • Where in the code does it segfault?
    – NPE
    Apr 1, 2011 at 15:12
  • I know there's a memory leak because I'm not deleting the created stuff, but I don't think that's the issue. Apr 1, 2011 at 15:13
  • in my queuing function. I'll post it, too if you think it could help Apr 1, 2011 at 15:13
  • Sure. I'd be most gratified if one could help! Apr 1, 2011 at 15:17

2 Answers 2

2

One minor problem is:

if (!q->first) {
    *value = NULL;
    return 0;
}

(note the added asterisk).

The main problem seems to be that you don't allocate memory for *being_fetched in the process_events function.

To be honest, I would change the type of that variable to struct input_event *being_fetched and simply pass its address to dequeue: has_item = dequeue(&my_queue, &being_fetched). This would also enable you to get rid of ev as a separate variable.

5
  • Yeah, right. There's also the fact that I should free some mem in my dequeue function. That's not important though, because it doesn't explain the segfault either. Apr 1, 2011 at 15:26
  • I'm baffled. I did absolutely nothing, just recompiled and run it doesn't give any kind of a problem... Apr 1, 2011 at 15:28
  • 1
    It (the second point) almost certainly does. You're writing to memory pointed to by an uninitialized pointer.
    – NPE
    Apr 1, 2011 at 15:29
  • Not fixing being_fetched will cause the segfault.
    – BMitch
    Apr 1, 2011 at 15:29
  • Yeah, C is damn tough for a on originally Java dev like me :-D That fixed it! And I added all necessary frees, too without issue. Thanks! Apr 1, 2011 at 15:41
1

As aix said, you didn't init *being_fetched. But I think you're better off defining it differently:

struct input_event *being_fetched;
...
has_item = dequeue(&my_queue, &being_fetched);
...
ev = being_fetched;

And then make absolute sure you fix the other bug that aix mentions, otherwise you won't get the null pointer back out of the function.

2
  • 1
    Dang, even aix's exit was faster than mine. +1 for that. :-)
    – BMitch
    Apr 1, 2011 at 15:29
  • Yup. Still, thanks a lot for the bother :-) Really kind of you. Apr 1, 2011 at 15:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.