Boston Linux & Unix (BLU) Home | Calendar | Mail Lists | List Archives | Desktop SIG | Hardware Hacking SIG
Wiki | Flickr | PicasaWeb | Video | Maps & Directions | Installfests | Keysignings
Linux Cafe | Meeting Notes | Linux Links | Bling | About BLU

BLU Discuss list archive


[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

f*ing pointer! Why doesn't this work?



I think your problem is here. I'll simplify;

Afunc(void *idata)
{
	struct foo *data;
	data = (struct foo *)idata; 
		:
	free(data); /* This is a segfault*/
}
void *data;
data = malloc(...);
Afunc(&data); 

Now, here is the way it should be:

Afunc(void *idata)
{
	struct foo *data;
	data = (struct foo *)*idata; 
		:
	free(data); /* This woks nicely but ...*/
}

Since you are passing in a pointer to a pointer (double indirect), your 
assignment is wrong:
data = (struct spawnData_s *)obj;

What you are assigning is a (struct spawnData_s **) not a (struct 
spawnData_s *).

Actually, your code has several major problems (I assume that threadSpawn 
is called for each thread you want to start).
1. don't use &data as your argument, just use data. The reason is that 
in threadSpawn,  data is an automatic variable and it's memor is 
deallocated when the function returns. If the child thread continues to use 
that pointer, several things could happen.
	Another function in the parent is invoked and ovewrites that memory or 
second call to threadSpawn overwrites it before your child thread finished 
using it. This is a major bug.
2. The use of ThreadPool should be protected by a mutex. (I assume that 
ThreadPool is initialized properly.(This is race condition). 
ThreadPool[idx].Status is tested in threadSpawn. What if the child thread 
should be switched before the child has a chance to set the Status. You 
need to make sure you set the status BEFORE main or any other thread tests 
it.

I suggest you make 2 corrections to your program:
1. add an element to ThreadPool for spawnData_s, then in threadSpawn,
	ThreadPool[idx].data = (spawnData_s *)malloc(....).
2. pthread_create(&ThreadPool[idx].aThread, NULL, (void *)funcWrapper, 
(void *)idx);
3. change void funcWrapper(void *obj) {
struct spawnData_s *data;
int idx;
 data = (struct spawnData_s *)obj;
to:
void funcWrapper(void *obj) {
>   struct spawnData_s *data;
>   int idx;
/*    You should pthread_mutex_lock here */
 idx = (int)obj;
 ThreadPool[idx].Status = tp_InUse;
 data = ThreadPool[idx].data;
/*    You may pthread_mutex_unlock here (or before setting data)*/

And in threadSpawn:
	/* pthread_mutex_lock */
	if (ThreadPool[idx].Status == tp_Available) {
	  ThreadPool[idx].data = malloc(...).
	  /* pthread_mutex_lock */
       pthread_create(&ThreadPool[idx].aThread, NULL, (void *)funcWrapper, 
(void *)idx);

Actually, this leaves a very slight open window, so I would add a 
ThreadPool[idx].Status = InProcess; before unlocking the mutex in the 
parent. 


Much simpler. You eliminate the loop inside the thread, you eliminate the 
confusion about the double indirect. 



--
Jerry Feldman <gaf at blu.org>
Associate Director
Boston Linux and Unix user group
http://www.blu.org PGP key id:C5061EA9
PGP Key fingerprint:053C 73EC 3AC1 5C44 3E14 9245 FB00 3ED5 C506 1EA9





BLU is a member of BostonUserGroups
BLU is a member of BostonUserGroups
We also thank MIT for the use of their facilities.

Valid HTML 4.01! Valid CSS!



Boston Linux & Unix / webmaster@blu.org