4 ms·

Thanks, I'm not much of a C programmer, so I appreciate the feedback. I'm not sure what you're referring to with the mutexes and pcond not being on the struct b

Thanks, I'm not much of a C programmer, so I appreciate the feedback. I'm not sure what you're referring to with the mutexes and pcond not being on the struct because they are. Can you clarify? You are right with the disposal though, I need to clean that stuff up :)

eps 12y ago
You don't need to allocate pthread_mutex_t instances on heap, you can include them into the struct directly - struct foo { pthread_mutex_t bar; ... };
- tylertreat 12y ago
  Got it, that makes more sense.
sdab 12y ago
I'm more of a c++ programmer, but I think i can chime in on the mutex/pcond being "part" of the struct. Essentially, OP means that why arent they on chan's stack? In the struct you have pointers to mutex and pcond which means you have to malloc them. OP is suggesting they not be pointers.
millstone 12y ago
What eps means is that, currently you have a pointer to a mutex in your struct: struct chan_t { pthread_mutex_t* m_mu;... } And initialize it like so: pthread_mutex_t* mu = (pthread_mutex_t*) malloc(sizeof(pthread_mutex_t)); pthread_mutex_init(mu, NULL); chan->m_mu = mu; eps's suggestion is to put the mutex directly in the struct: struct chan_t { pthread_mutex_t m_mu;... } and initialize it by passing its address to pthread_mutex_init: pthread_mutex_init(&chan->m_mu, NULL); chan->m_mu = mu; that saves some mallocs and dereferences. If you are new to C, the notion of taking the address of fields in a struct may be unnerving, but you get used to it :)
pmahoney 12y ago
The struct is currently storing a pointer to the mutex (which was allocated with malloc). The call to pthread_mutex_destroy does not free that memory. Valgrind is your friend :) http://valgrind.org/docs/manual/quick-start.html#quick-start.prepare http://valgrind.org/docs/manual/quick-start.html#quick-start...
clarry 12y ago
You shouldn't allocate heap space for a pointer in blocking_pipe_read. Also, it looks like you're leaking it in the else branch. Just allocate a pointer on the stack, read into it, and then assign the value into the provided space: void *msg_ptr; read(..., &msg_ptr, sizeof msg_ptr); *data = msg_ptr;

eps 12y ago

You don't need to allocate pthread_mutex_t instances on heap, you can include them into the struct directly - struct foo { pthread_mutex_t bar; ... };

tylertreat 12y ago
Got it, that makes more sense.

sdab 12y ago

I'm more of a c++ programmer, but I think i can chime in on the mutex/pcond being "part" of the struct. Essentially, OP means that why arent they on chan's stack? In the struct you have pointers to mutex and pcond which means you have to malloc them. OP is suggesting they not be pointers.

millstone 12y ago

What eps means is that, currently you have a pointer to a mutex in your struct: struct chan_t { pthread_mutex_t* m_mu;... } And initialize it like so: pthread_mutex_t* mu = (pthread_mutex_t*) malloc(sizeof(pthread_mutex_t)); pthread_mutex_init(mu, NULL); chan->m_mu = mu; eps's suggestion is to put the mutex directly in the struct: struct chan_t { pthread_mutex_t m_mu;... } and initialize it by passing its address to pthread_mutex_init: pthread_mutex_init(&chan->m_mu, NULL); chan->m_mu = mu; that saves some mallocs and dereferences. If you are new to C, the notion of taking the address of fields in a struct may be unnerving, but you get used to it :)

pmahoney 12y ago

The struct is currently storing a pointer to the mutex (which was allocated with malloc). The call to pthread_mutex_destroy does not free that memory. Valgrind is your friend :) http://valgrind.org/docs/manual/quick-start.html#quick-start.prepare http://valgrind.org/docs/manual/quick-start.html#quick-start...

clarry 12y ago

You shouldn't allocate heap space for a pointer in blocking_pipe_read. Also, it looks like you're leaking it in the else branch. Just allocate a pointer on the stack, read into it, and then assign the value into the provided space: void *msg_ptr; read(..., &msg_ptr, sizeof msg_ptr); *data = msg_ptr;