Gentoo Forums
Gentoo Forums
Gentoo Forums
Quick Search: in
Basic pthreads questions
View unanswered posts
View posts from last 24 hours

 
Reply to topic    Gentoo Forums Forum Index Portage & Programming
View previous topic :: View next topic  
Author Message
methodtwo
Apprentice
Apprentice


Joined: 01 Feb 2008
Posts: 226

PostPosted: Wed Feb 20, 2013 7:34 pm    Post subject: Basic pthreads questions Reply with quote

Hi there
In my C program i have the main function call a function that creates some pthreads. The program is a TCP/IP daemon
that serves some basic text-based games like tic-tac-toe(yes this is just a programming exercise). Thus it is pre-threaded and each thread serves a game. So the main loop of the program happens in the code of each thread. However because the main loop of these basic games is now called in the thread code, instead of in main where it originally was, functions external to the thread have to get called from within the thread. The games state is a [3][3] char array which was global originally. So now because it's supposed to be a network daemon, i have to use "thread-specific-data" for this [3][3] char array. Here is the main loop from each thread:
Code:

void *thread_main()
{
   char done  = ' ';
   init_matrix();
 do {
     disp_matrix();
     get_player_move();
     done = check();
     if(done != ' ') break;
     get_computer_move();
     done = check();
} while(done == ' ');

  if(done == 'X') printf("You Won\n");
  else printf("I won\n");

  disp_matrix();
  freeaddrinfo(res);
  return(0);
}


And here is the "thread-specific-data" for the games state([3][3] char array)created in "get_player_move"
Code:
pthread_once(&ts_once, pt_once);
   if((tsd = pthread_getspecific(tskey)) == NULL) {
   tsd = calloc(1, sizeof(Thread));
        pthread_setspecific(tskey, tsd);
   }

So now because the [3][3] char array for the games "state" is in the Thread struct pointed to by tsd then "struct Thread *tsd" has to be global as in the code example above! or has to be passed as an argument to each function. Having global doesn't make any sense because it's thread specific data right? But if i pass "struct Thread *tsd" to the functions the compiler kicks out like 12 errors. The main one is "expected expression" with reference to functions like init_matrix. the call conforms to the prototype namely "init_matrix(struct Thread *tsd)".
I don't know what the recommended/correct solution is. I'm very sorry that i haven't written about this in a concise and general way. I don't know if i understand enough about C to do that yet. Any help would be great. Thank you very much for reading
Back to top
View user's profile Send private message
Hu
Watchman
Watchman


Joined: 06 Mar 2007
Posts: 8845

PostPosted: Wed Feb 20, 2013 11:14 pm    Post subject: Reply with quote

Please give us a minimal program that you think should work, but that instead fails with the compilation errors described. You showed part of a working single-threaded program and described in English what you changed, but it is far better for us to see what the compiler is given.
Back to top
View user's profile Send private message
methodtwo
Apprentice
Apprentice


Joined: 01 Feb 2008
Posts: 226

PostPosted: Thu Feb 21, 2013 12:14 am    Post subject: Reply with quote

Thank you very much for the reply. Here is what i've got so far for a pre-threaded daemon with each thread handling a game of tic-tac-toe. This is not a joke i really am this bad at C programming and i'm sorry that i don't know how to highlight text that's inside the forum "code" tags:

Code:
#include <stdio.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <stdlib.h>
#include <arpa/inet.h>
#include <netdb.h>
#include <netinet/in.h>
#include <sys/socket.h>
#include <fcntl.h>
#include <sys/types.h>
#include <unistd.h>
#include <string.h>
#include <errno.h>
#include <syslog.h>
#include <signal.h>
#include <pthread.h>

#define MAXLINE 1024
struct Thread{
   pthread_t thread_tid;
   long thread_count;
   char matrix[3][3];
}Thread;
struct Thread *tptr;

char check(struct Thread *ptr);
void init_matrix(struct Thread *ptr);
void get_player_move(struct Thread *ptr);
int daemonize();
static void pt_destructor(void *ptr);
static void pt_once(void);
void get_computer_move(struct Thread *ptr);
void disp_matrix(struct Thread *ptr);
void thread_make(int);

int sockfd;
struct addrinfo hints, *res=0;
struct sockaddr cli_addr;
static pthread_key_t tskey;
static pthread_once_t ts_once = PTHREAD_ONCE_INIT;
int nthreads;

pthread_mutex_t mlock = PTHREAD_MUTEX_INITIALIZER;
socklen_t cli_len=sizeof(cli_addr);

int main(int argc, char *argv[])
{
 int err, i;
 int result;
 const char* hostname=0;
 const char *port_num="12349";
 nthreads = atoi(argv[argc - 1]);
 
 if((result = daemonize()) != 0) {
    perror("Can't become a daemon");
    exit(1);
 }

 memset(&hints, 0, sizeof(hints));
 hints.ai_socktype = SOCK_STREAM;
 hints.ai_family = AF_INET;
 hints.ai_flags = AI_PASSIVE|AI_ADDRCONFIG;

 
 if((err=getaddrinfo(hostname, port_num, &hints, &res)) != 0){
    perror("getaddrinfo failed");
    exit(1);
 }
 sockfd = socket(res->ai_family, res->ai_socktype, res->ai_protocol);
 if(sockfd < 0){
   perror("socket failed");
   exit(1);
 }
 if(bind(sockfd, res->ai_addr, res->ai_addrlen)==-1) {
   perror("bind failed");
   exit(1);
 }
 if(listen(sockfd, 5) < 0) {
   perror("listen failed");
   exit(1);
 }
 for(i = 0; i < nthreads; i++)
    thread_make(i);

 return(0);
}
static void pt_once(void)
{
   pthread_key_create(&tskey, pt_destructor);
}
static void pt_destructor(void *tsd)
{
  free(tsd);
}
   
void thread_make(int i)
{
  void *thread_main();
 
  pthread_create(&tptr[i].thread_tid, NULL, &thread_main, NULL);
  return; /*main thread returns */
}
void *thread_main()
{
   char done  = ' ';
   init_matrix(struct Thread *tsd);
 do {
     disp_matrix(struct Thread *tsd);
     get_player_move(struct Thread *tsd);
     done = check(struct Thread *tsd);
     if(done != ' ') break;
     get_computer_move(struct Thread *tsd);
     done = check(struct Thread *tsd);
} while(done == ' ');

  if(done == 'X') printf("You Won\n");
  else printf("I won\n");

  disp_matrix(struct Thread *tsd);
  freeaddrinfo(res);
  return(0);
}
void init_matrix(struct Thread *tsd)
{
  int i,j;
  for(i=0;i<3;i++)
     for(j=0;j<3;j++) tsd->matrix[i][j] = ' ';
}
void get_player_move(struct Thread *tsd)
{
   char recvline[16], sendline[30], textline[30];
   int x, y, connfd;
   char space=' ';

   
   pthread_once(&ts_once, pt_once);
   if((tsd = pthread_getspecific(tskey)) == NULL) {
   tsd = calloc(1, sizeof(Thread));
        pthread_setspecific(tskey, tsd);
   }
   for(;;){
   /*if(recvfrom(sockfd, recvline, sizeof(recvline), 0,(struct sockaddr*)&cli_addr, &cli_len) ==-1){
     perror("recvfrom error");
     exit(1);
   }*/
   /*changed to n==0 instead of != 2 */
   pthread_mutex_lock(&mlock);
   if((connfd = accept(sockfd, &cli_addr, &cli_len)) == -1){
      perror("Can't accept move");
      exit(1);
   }
   /* pthread_mutex_unlock(&mlock); */
   if(sscanf(recvline, "%d%*c%d", &x, &y) == 0){
      perror("sscanf error on server");
      exit(1);
   }
   x--; y--;
   printf("x is %d, y is %d\n", x, y);
   if(tsd->matrix[x][y] != ' '){
    snprintf(textline, sizeof(textline), "Invalid move, try again.\n");
    if(write(sockfd, textline, sizeof(textline))  ==-1){
       perror("write failed");
       exit(1);
   }
   
   }
   else {
        tsd->matrix[x][y] = 'X';
        snprintf(sendline, sizeof(sendline), "%d%c%d\n", x, space, y);
        if(write(sockfd, sendline, sizeof(sendline)) ==-1){
            perror("send move back failed");
            exit(1);
        }
   else
       break;
       /*exit(0);*/
  }
 }
 pthread_mutex_unlock(&mlock);
}

void get_computer_move(struct Thread *tsd)
{
  int i, j;
  char sendlinez[10];
  char sendtext[10];
  char spacez=' ';
  /*Add intelligence here*/ 
  pthread_mutex_lock(&mlock); 
  for(i=0;i<3;i++){
     for(j=0;j<3;j++)
       if(tsd->matrix[i][j]==' ') break;
     if(tsd->matrix[i][j]==' ') break;
  }
  printf("i is %d, j is %d\n", i, j);
  if(i*j==9) {
   snprintf(sendtext, sizeof(sendtext), "Draw!\n");
   if(write(sockfd, sendtext, sizeof(sendtext)) ==-1){
      perror("sending draw message error");
      exit(1);
   }
   /*exit(0);*/
  }
  else {
   
   snprintf(sendlinez, sizeof(sendlinez), "%d%c%d\n", i, spacez, j);
   if(write(sockfd, sendlinez, sizeof(sendlinez)) ==-1){
     perror("Error sending computer move");
     exit(1);
   }
  tsd->matrix[i][j] = 'O';
  pthread_mutex_unlock(&mlock);
 }
}
void disp_matrix(struct Thread *tsd)
{
  int t;
  for(t=0;t<3;t++)
  {
    printf(" %c | %c | %c ",tsd->matrix[t][0],tsd->matrix[t][1],tsd->matrix[t][2]);
    if(t != 2) printf("\n---|---|---\n");
  }
  printf("\n");
}

char check(struct Thread *tsd)
{
  int i;

  for(i=0; i<3; i++)  /* check rows */
    if(tsd->matrix[i][0]==tsd->matrix[i][1] &&
       tsd->matrix[i][0]==tsd->matrix[i][2]) return tsd->matrix[i][0];

  for(i=0; i<3; i++)  /* check columns */
    if(tsd->matrix[0][i]==tsd->matrix[1][i] &&
       tsd->matrix[0][i]==tsd->matrix[2][i]) return tsd->matrix[0][i];

  /* test diagonals */
  if(tsd->matrix[0][0]==tsd->matrix[1][1] &&
     tsd->matrix[1][1]==tsd->matrix[2][2])
       return tsd->matrix[0][0];

  if(tsd->matrix[0][2]==tsd->matrix[1][1] &&
     tsd->matrix[1][1]==tsd->matrix[2][0])
       return tsd->matrix[0][2];

  return ' ';
}

There's lots of code in there not relevant to the discussion. So the daemonize function has been removed although the call is still there.
Back to top
View user's profile Send private message
Hu
Watchman
Watchman


Joined: 06 Mar 2007
Posts: 8845

PostPosted: Thu Feb 21, 2013 3:16 am    Post subject: Reply with quote

As a general comment, threaded programming is easier if you minimize the number of globals.

I see why your code fails to compile, but rather than just fix that, I will provide some specific criticisms.
methodtwo wrote:
This is not a joke i really am this bad at C programming and i'm sorry that i don't know how to highlight text that's inside the forum "code" tags:
Forum code tags are designed to suppress all other formatting, including any highlighting.
methodtwo wrote:
Code:

#define MAXLINE 1024
struct Thread{
   pthread_t thread_tid;
   long thread_count;
   char matrix[3][3];
}Thread;
struct Thread *tptr;
This defines a structure of type Thread and instantiates one at global scope. You probably do not want a Thread at global scope, nor do you want a Thread * at global scope.
methodtwo wrote:
Code:

int daemonize();
GNU libc provides a function daemon with the same signature, and likely the same purpose.
methodtwo wrote:
Code:
static void pt_destructor(void *ptr);
static void pt_once(void);
void get_computer_move(struct Thread *ptr);
void disp_matrix(struct Thread *ptr);
void thread_make(int);
Why did you make some of these static and not others? I suggest using static wherever you can do so without complicating the design.
methodtwo wrote:
Code:
int sockfd;
You should give this variable a better name. Subsequent code seems to be confused as to whether this variable is the listener socket or the socket associated with a particular client.
methodtwo wrote:
Code:
struct addrinfo hints, *res=0;
struct sockaddr cli_addr;
These can be local to where they are used.
methodtwo wrote:
Code:
static pthread_key_t tskey;
static pthread_once_t ts_once = PTHREAD_ONCE_INIT;
Instead of using thread-specific storage, why not allocate the data on the heap and pass the pointer to the individual thread that uses it?
methodtwo wrote:
Code:
int nthreads;
This should be local to main.

methodtwo wrote:
Code:
pthread_mutex_t mlock = PTHREAD_MUTEX_INITIALIZER;
This is unnecessary if you remove the globals which it guards.
methodtwo wrote:
Code:
socklen_t cli_len=sizeof(cli_addr);
This should be local to main.
methodtwo wrote:
Code:

int main(int argc, char *argv[])
{
 if((err=getaddrinfo(hostname, port_num, &hints, &res)) != 0){
    perror("getaddrinfo failed");
    exit(1);
I prefer return 1;, so that the code can be moved into a subroutine without causing unexpected side effects on failure.
methodtwo wrote:
Code:

void thread_make(int i)
{
  void *thread_main();
Although block-scope declarations are legal, they are often not a good choice. I suggest making both thread_make and thread_main static, then defining thread_main before thread_create, so that the definition of thread_main doubles as the declaration for it.

The prototype for pthread_create indicates that the thread entry point can take a void*, but you use a thread entry point which takes no arguments. This is legal, but it makes your life harder because it deprives you of the ability to pass thread-specific data into the thread.
methodtwo wrote:
Code:
  pthread_create(&tptr[i].thread_tid, NULL, &thread_main, NULL);
You never set tptr to a value, so this line will crash at runtime.
methodtwo wrote:
Code:
void *thread_main()
{
   char done  = ' ';
   init_matrix(struct Thread *tsd);
If you want to call the function, pass the parameter to it without the type: init_matrix(tsd);, assuming that a variable of the right type named tsd is currently in scope.
methodtwo wrote:
Code:
  freeaddrinfo(res);
This is wrong. You allocated address information once in main, but every thread frees the same address information on exit. You should move this line to main, immediately after res is last used.
methodtwo wrote:
Code:
void get_player_move(struct Thread *tsd)
{
   pthread_once(&ts_once, pt_once);
   if((tsd = pthread_getspecific(tskey)) == NULL) {
   tsd = calloc(1, sizeof(Thread));
        pthread_setspecific(tskey, tsd);
   }
You pass tsd as a parameter, but then you always overwrite it with the thread-specific data. This is wrong.
methodtwo wrote:
Code:
 pthread_mutex_lock(&mlock);
What does this lock protect?
methodtwo wrote:
Code:

   if((connfd = accept(sockfd, &cli_addr, &cli_len)) == -1){
      perror("Can't accept move");
      exit(1);
Do you want to kill the whole program if one call to accept fails?
methodtwo wrote:
Code:
 if(sscanf(recvline, "%d%*c%d", &x, &y) == 0){
Array recvline[] is used uninitialized here. Also, you check for one specific form of failure. If sscanf reads anything, you consider it a success, but I would only consider it a success if it decoded all the required inputs.
methodtwo wrote:
Code:
   if(tsd->matrix[x][y] != ' '){
You use x and y without checking that they are valid.
methodtwo wrote:
Code:
    snprintf(textline, sizeof(textline), "Invalid move, try again.\n");
No variables are used, so snprintf is overkill.
methodtwo wrote:
Code:
    if(write(sockfd, textline, sizeof(textline))  ==-1){
sockfd is your listener, not your client.
methodtwo wrote:
Code:
  if(i*j==9) {
Why not just test i==3 && j==3?
Back to top
View user's profile Send private message
methodtwo
Apprentice
Apprentice


Joined: 01 Feb 2008
Posts: 226

PostPosted: Thu Feb 21, 2013 3:45 am    Post subject: Reply with quote

Thank you very very much. That is the the most helpful reply i think i've ever had. I was just at the end of a long day and you saved me from, like, reading 4 chapters of K&R. Also in K&R sometimes a combination of advanced topics are treated separately. So it might be hard to find something on "pointers to structures as function arguments" for example. You've already said more than enough. If you don't want to answer the question i've still got that is more than understandable
1) You said that i always over-write tsd, which is a passed parameter, with thread-specific data. I thought i just set it to point to the thread-specific-data then passed a pointer to this thread-specific-data to the needed functions? That is what i needed to do right? So what i erroneously was doing was passing a pointer to a struct of type Thread which over-wrote the pointer(?) or struct(?) that belonged to the thread-specific-data? So to fix it i should just not pass the "struct Thread *tsd to "get_player_move"? So *tsd has to be global? If i'm wrong about this then (in english) what should i have done? Going back to what the original question should have been;Can you have a pointer, that is returned by "pthread_get_specific", declared as a global variable? Thinking about it i guess you can because the array of key structs that hold the destructor function and pointer returned by pthread_get_specific are only pointers to the array of pointers to the malloc'd thread-specific-data, so they should be process wide?. If not how do you pass it to functions that the threads call? I'm sorry this was a long winded and badly thought out as a question!

Thank you for the learned and instructive reply. How do i "officially" "thank" you?
regards
Back to top
View user's profile Send private message
Genone
Retired Dev
Retired Dev


Joined: 14 Mar 2003
Posts: 9005
Location: beyond the rim

PostPosted: Thu Feb 21, 2013 2:57 pm    Post subject: Reply with quote

I'd suggest before bothering with threads you get your pointer and memory handling basics right. As I see it you don't fully understand how pointer arguments work:
Code:
void *thread_main()
{
   char done  = ' ';
   init_matrix(struct Thread *tsd);
 do {
     disp_matrix(struct Thread *tsd);
     get_player_move(struct Thread *tsd);

The core problem with your *tsd variable is that it doesn't actually exist. You have to allocate memory for it somewhere before you can pass it to a function. I assume the declaration/calling signature mixup is a copy+paste error.
Code:
   if((tsd = pthread_getspecific(tskey)) == NULL) {
   tsd = calloc(1, sizeof(Thread));
        pthread_setspecific(tskey, tsd);
   }

Maybe explain what you're trying to do here. Remember that the pointer variable tsd itself (other than it's content *tsd) is still passed into the function by value, so changing it will not affect the passed in value. If you want a function to return a pointer you have to pass a pointer-pointer in, or use the return value.
Back to top
View user's profile Send private message
methodtwo
Apprentice
Apprentice


Joined: 01 Feb 2008
Posts: 226

PostPosted: Fri Feb 22, 2013 4:56 am    Post subject: Reply with quote

Thanks for the reply. To answer the question: i wanted to have each thread call functions that operated on the games "state". This state is maintained as thread-specific-data, as each player could be writing into the [3][3] char array at the same time that other players wrote/read this array(if it was global). So "here" i was trying to set up a pointer to the thread-specific-data that each thread could pass to the functions it calls, so that each thread could handle a "game". I realise that when each thread called functions then the pointer to the Thread structure should be coded as just tsd and not struct Thread *tsd. Going back to the original question; can you define structs or variables as global and then use the pointer returned from pthread_get_specific as a pointer to these globals so that say struct Thread can be used to hold thread specific data?. If struct Thread is not defined global then the compiler complains that all these functions that the threads call don't know what [b]struct Thread *tsd is! Even though it says in K&R "pointer arguments enable a function to access and change objects in the function that called it. And yes setting up the thread pointer *tsd is now in the main thread function.
[/b]
Thanks
Back to top
View user's profile Send private message
Genone
Retired Dev
Retired Dev


Joined: 14 Mar 2003
Posts: 9005
Location: beyond the rim

PostPosted: Fri Feb 22, 2013 7:21 am    Post subject: Reply with quote

I'm still not sure you understand what you're doing. One example to hopefully clarify:
Code:
void foo(int *x) {
  printf("foo: %p (%d)\n", x, *x);
  *x = 5;
  printf("foo: %p (%d)\n", x, *x);
  x = malloc(sizeof(int));
  *x = 10;
  printf("foo: %p (%d)\n", x, *x);
}

int main(void) {
  int *a = malloc(sizeof(int));
  *a = 1;
  printf("main: %p (%d)\n", a, *a);
  foo(a);
  printf("main: %p (%d)\n", a, *a);
  return 0;
}

%p outputs the address that the pointer contains (e.g. the actual value of x) while %d outputs the value of the dereferenced pointer (e.g. the assigned value of *x)
Assume the first 4 prints output the following values:
Code:
main: 0x0004 (1)
foo: 0x0004 (1)
foo: 0x0004 (5)
foo: 0x0012 (10)

What do you think will be the last output after calling foo()?
Back to top
View user's profile Send private message
methodtwo
Apprentice
Apprentice


Joined: 01 Feb 2008
Posts: 226

PostPosted: Fri Feb 22, 2013 10:42 am    Post subject: Reply with quote

Thanks for being very patient with me. I thought from looking at the code that the final value would be 10 but it's not it's 5. So *x = 5 actually altered a, but, in this case, after the malloc in foo we were no longer dealing with a but a new dynamically generated int so this didn't have any effect on a.
Thank you very much. I think that's enough to be able to let me get it right.
Thanks very much for all the wonderful help
Back to top
View user's profile Send private message
Display posts from previous:   
Reply to topic    Gentoo Forums Forum Index Portage & Programming All times are GMT
Page 1 of 1

 
Jump to:  
You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum