copying an array of structs to new variable

utdrmac · 06-27-2011, 11:12 AM

Hi all,
I had a question about threads and mutexes recently answered and a members' answer sparked a different problem.

'Nominal Animal' in the post mentioned said I should hold a mutex only long enough to copy my data to a temporary variable and then output by reading that temp. Here's some code on this. I must be copying the array of structs wrong because when the gui thread reads the temp copy, I end up with lots of garbage. Even running through gdb verifies that different data is in the temp array-copy than in the original.

How do I go about copying this array of structs to another variable correctly in the gui thread?

Thanks!!
-Matthew

Code:

typedef struct
{
        char server[40];
        int sbm;
        int position;
        int isSeparator;
        int errno;
        char error[40];
} replInfo_st;

replInfo_st *replInfo;

int windowManager()
{
        // replInfo won't change size after program init so
        // safe use its size to make this temp one
        replInfo_st *tempReplInfo = malloc(sizeof(replInfo));
        
        for(;;)
        {
                // copy to temp
                pthread_rwlock_wrlock(&rwlock);
                memcpy(tempRepInfo, replInfo, sizeof(replInfo));
                pthread_rwlock_unlock(&rwlock);

                // print out using temp variable
                ....
        }
}

int main()
{
        ....
        replInfo = (replInfo_st *) malloc(sizeof(replInfo_st) * numServers);

        /* code to populate all array members of replInfo goes here */

        /* spawn and wait for threads here */
}

dwhitney67 · 06-27-2011, 11:52 AM

You are only allocating the size of a pointer (4 bytes on 32-bit system, 8 bytes on 64-bit system) with this statement:

Code:

replInfo_st *tempReplInfo = malloc(sizeof(replInfo));

As for what you are attempting, you intent is correct, but you do need to allocate the correct space, and of course, copy the appropriate amount of data from the source.

The statement above should be:

Code:

replInfo_st *tempReplInfo = malloc(sizeof(replInfo_st) * numServers);

and then followed by:

Code:

for (;;)
{
   pthread_rwlock_wrlock(&rwlock);
   memcpy(tempReplInfo, replInfo, sizeof(replInfo_st) * numServers);
   pthread_rwlock_unlock(&rwlock);

   ...
}

I personally do not see the need for you to rely on global variables, when these could just as easily be passed into the function. Nevertheless, if you use the code above, you will need to declare 'numServers' as a global variable.

utdrmac · 06-27-2011, 12:01 PM

I'll give that a try thanks.

A bit more background... the app reads in hostnames from a flat-file and sets numServers to however many it read. I might have to make that one global now. replInfo is used by all N threads to it has to be global (right?) and the tempRepInfo is local to that thread.

Thanks so much!

dwhitney67 · 06-27-2011, 12:27 PM

Quote:

Originally Posted by utdrmac

I'll give that a try thanks.

A bit more background... the app reads in hostnames from a flat-file and sets numServers to however many it read. I might have to make that one global now. replInfo is used by all N threads to it has to be global (right?) and the tempRepInfo is local to that thread.

Thanks so much!

Global variables are not always necessary. Your thread(s) can accept a locally declared object, as long as that object is not destroyed (or falls out of scope) during the lifetime of the thread(s). Consider the following, which is based on the project from your other thread:

Code:

...

#define NUM_WORK_THRDS 60

typedef struct
{
   int               id;
   int*              array;
   pthread_rwlock_t* rwlock;
} ThreadData;


void* workThread(void* arg)
{
   ThreadData* tdata = (ThreadData*) arg;

   for (;;)
   {
   }

   return NULL;
}

void* readThread(void* arg)
{
   ThreadData* tdata = (ThreadData*) arg;

   for (;;)
   {
   }

   return NULL;
}


int main()
{
   int              array[NUM_WORK_THRDS];
   ThreadData       tdata[NUM_WORK_THRDS];
   pthread_rwlock_t rwlock;
   pthread_t        tid[NUM_WORK_THRDS + 1];   /* +1 for reader thread */

   pthread_rwlock_init(&rwlock, NULL);

   for (int i = 0; i < NUM_WORK_THRDS; ++i)
   {
      tdata[i].id     = i;
      tdata[i].array  = array;
      tdata[i].rwlock = &rwlock;

      pthread_create(&tid[i], NULL, workThread, &tdata[i]);
   }

   pthread_create(&tid[i], NULL, readThread, &tdata[0]);

   /* ... make sure this thread (main thread) does not exit; or better yet
          dispense with the readThread above, and do that task here.
    */

   return 0;
}

utdrmac · 06-27-2011, 12:47 PM

Oh I see; because you put a pointer var in the struct, you just assign the address of the initialized mutex. Everybody is still operating on the same mutex. Neato.

I also had the #define for the number of threads in the beginning versions of my app, but I wanted that number to be variable so you didn't have to recompile the program if you brought a new sever online.

And I'll take a look at moving the "gui" thread to inside the main to reduce the number of threads by 1.

Thanks!

utdrmac · 06-27-2011, 12:51 PM

By putting the mutex pointer into the struct, however, you will increase the amount of memory the application uses by (sizeof(pthread_rwlock_t) * numServers) right? Because instead of having 1 global variable, you now have a copy for each thread, right?

dwhitney67 · 06-27-2011, 12:55 PM

Quote:

Originally Posted by utdrmac

By putting the mutex pointer into the struct, however, you will increase the amount of memory the application uses by (sizeof(pthread_rwlock_t) * numServers) right? Because instead of having 1 global variable, you now have a copy for each thread, right?

Yes that is correct. Running on a 64-bit system, with say 100 servers to support, the total amount of memory for all of those pointers will be 800 bytes.

My suggestion was merely to show that global variables are not always warranted.

utdrmac · 06-27-2011, 02:16 PM

Haha. Love the emoticon for 800 bytes. Coolio. Everything looks great on my app now! Thanks so much!