Gentoo Forums
Gentoo Forums
Gentoo Forums
Quick Search: in
pre-threaded TCP/IP Daemon 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: 231

PostPosted: Sun Mar 24, 2013 7:48 pm    Post subject: pre-threaded TCP/IP Daemon questions Reply with quote

Hi
Sorry if this is double posting! It might not be as the code has been worked on and improved as much as i could since the last time i asked about it!
For my own education and as an introduction to semi-non trivial C code i'm writing a TCP/IP daemon. It serves games like tic-tac-toe( or naughts and crosses, if you're from the U.K). The version of this daemon that i'm stuck with is one that creates a number of threads, each one to accept() a TCP connection and handle a game. It uses thread-specific-data to hold the games state, which is a [3][3] char array. I asked about this daemon before with regards to whether the structs, for the thread-specific-data, could be global. The answer i got indicated that yes they could because once calloc is used new structs are created dynamically on the heap. I tried a version with non-global structs but the compiler complained too much. Below is the code so far. I would not describe myself as a good C programmer so please don't laugh!
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 <unistd.h>
#include <string.h>
#include <errno.h>
#include <syslog.h>
#include <signal.h>
#include <pthread.h>

struct Thread{
   pthread_t thread_tid;
   long thread_count;
   char matrix[3][3];
};
struct Threadz{
    pthread_t thread_tid;
    long thread_count;
    char matrix[3][3];
    int connfd;
};

char check(struct Threadz *tsd);
void init_matrix(struct Threadz *tsd);
void get_player_move(struct Threadz *tsd);
int daemonize();
static void pt_destructor(void *tsd);
static void pt_once(void);
void get_computer_move(struct Threadz *tsd);
void disp_matrix(struct Threadz *tsd);
void thread_make(int, struct Thread *tptr);
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;

socklen_t cli_len=sizeof(cli_addr);

int main(int argc, char *argv[])
{
 int err, i;
 int result, nthreads;
 const char* hostname=0;
 const char *port_num="12349";
 struct Thread *tptr;
 nthreads = atoi(argv[argc - 1]);

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

 tptr = calloc(nthreads, sizeof(struct Thread));
 
 if((result = daemon(0,0)) != 0){
    perror("Can't become a daemon!");
    exit(1);
 }
 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);
 }
 freeaddrinfo(res);

 if(listen(sockfd, 5) < 0) {
   perror("listen failed");
   exit(1);
 }

 for(i = 0; i < nthreads; i++)
    thread_make(i, tptr);
 /* suspend main until threads have done there work */
 for(i = 0; i < nthreads; i++)
    pthread_join(tptr[i].thread_tid, NULL);

 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, struct Thread *tptr)
{
  void *thread_main();
 
  pthread_create(&tptr[i].thread_tid, NULL, &thread_main, NULL);
  return; /*main thread returns */
}
void *thread_main()
{
  int *p;
  struct Threadz *tsd;
  pthread_once(&ts_once, pt_once);
   if((tsd = pthread_getspecific(tskey)) == NULL) {
   tsd = calloc(1, sizeof(struct Threadz));
        pthread_setspecific(tskey, tsd);
   }
 
   char done  = ' ';
   init_matrix(tsd);
 do {
     disp_matrix(tsd);
     get_player_move(tsd);
     done = check(tsd);
     if(done != ' ') break;
     get_computer_move(tsd);
     done = check(tsd);
} while(done == ' ');

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

  disp_matrix(tsd);
 
  return(0);
}
void init_matrix(struct Threadz *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 Threadz *tsd)
{
   char recvline[16], sendline[30], textline[30];
   int x, y, n;
   char space=' ';
   
   for(;;) {
      if((tsd->connfd = accept(sockfd, &cli_addr, &cli_len)) == -1){
         perror("Failed to accept connection");
         continue;
   }
   if((n = read(tsd->connfd, recvline, sizeof(recvline))) <= 0){
      perror("Read error");
      exit(1);
   }
   if(sscanf(recvline, "%d%*c%d", &x, &y) != 2){
      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(tsd->connfd, 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(tsd->connfd, sendline, sizeof(sendline)) ==-1){
            perror("send move back failed");
            exit(1);
        }
   else
       break;
    }

 }

}

void get_computer_move(struct Threadz *tsd)
{
  int i, j;
  char sendlinez[10];
  char sendtext[10];
  char spacez=' ';
  /*Add intelligence here*/ 
  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(tsd->connfd, sendtext, sizeof(sendtext)) ==-1){
      perror("sending draw message error");
      exit(1);
   }
  }
  else {
   
   snprintf(sendlinez, sizeof(sendlinez), "%d%c%d\n", i, spacez, j);
   if(write(tsd->connfd, sendlinez, sizeof(sendlinez)) ==-1){
     perror("Error sending computer move");
     exit(1);
   }
  tsd->matrix[i][j] = 'O';
   }
}
void disp_matrix(struct Threadz *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 Threadz *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 ' ';
}

I wandered if anyone could explain why it isn't working and how to fix it?. I know this is a lot of code for a forum question, but i just thought i'd ask incase anyone was up for telling me what's wrong with it(and how to fix it). I have one that does pre-fork()ing instead of threads and that's alright. I just wanted to get the pre-threaded one fixed.More than one client can start a game and the first move gets entered and displayed. The game hangs, at the clients, when the first of the second moves gets entered by one of the clients. Sorry for the mass of useless code and thank you very much for any replies.
regards


Last edited by methodtwo on Sat Apr 13, 2013 2:12 pm; edited 1 time in total
Back to top
View user's profile Send private message
Hu
Moderator
Moderator


Joined: 06 Mar 2007
Posts: 21489

PostPosted: Sun Mar 24, 2013 8:30 pm    Post subject: Reply with quote

methodtwo wrote:
Code:

struct Thread{
   pthread_t thread_tid;
   long thread_count;
   char matrix[3][3];
};
struct Threadz{
    pthread_t thread_tid;
    long thread_count;
    char matrix[3][3];
    int connfd;
};
Why do you have two very similar definitions?
methodtwo wrote:
Code:
int daemonize();
I believe I told you before that glibc provides a function to do this. You should include the header that provides its prototype, rather than declaring it yourself.
methodtwo wrote:
Code:
static void pt_destructor(void *tsd);
static void pt_once(void);
Why are these static when the others are not?
methodtwo wrote:
Code:
struct addrinfo hints, *res=0;
struct sockaddr cli_addr;
socklen_t cli_len=sizeof(cli_addr);
Why are these global?
methodtwo wrote:
Code:
 nthreads = atoi(argv[argc - 1]);
You assume that an argument will be passed.
methodtwo wrote:
Code:
 if((result = daemon(0,0)) != 0){
    perror("Can't become a daemon!");
    exit(1);
 }
You should perform as much setup as possible before you become a daemon.
methodtwo wrote:
Code:
 if(bind(sockfd, res->ai_addr, res->ai_addrlen)==-1) {
Test for < 0 here, to be consistent with your test for a failed socket.
methodtwo wrote:
Code:
void *thread_main()
{
  pthread_once(&ts_once, pt_once);
   if((tsd = pthread_getspecific(tskey)) == NULL) {
   tsd = calloc(1, sizeof(struct Threadz));
        pthread_setspecific(tskey, tsd);
   }
I hope this is only written this way because you are determined to use thread-specific data in a situation where it is not a good solution. This is right at the entry to the thread, so we know that you will always need to allocate the data. You then allocate a data structure that is mostly a duplicate of the struct Thread allocated by main. Since you never call pthread_getspecific anywhere else, no other code can possibly retrieve the value you saved here. The only purpose of placing this pointer in the thread-specific storage is to exploit the thread exit cleanup to free it.
methodtwo wrote:
Code:

void get_player_move(struct Threadz *tsd)
{
   char recvline[16], sendline[30], textline[30];
   int x, y, n;
   char space=' ';
   
   for(;;) {
      if((tsd->connfd = accept(sockfd, &cli_addr, &cli_len)) == -1){
         perror("Failed to accept connection");
         continue;
   }
This requires the player to connect before each move, which is probably not what you want.
methodtwo wrote:
Code:

   if((n = read(tsd->connfd, recvline, sizeof(recvline))) <= 0){
      perror("Read error");
      exit(1);
   }
   if(sscanf(recvline, "%d%*c%d", &x, &y) != 2){
      perror("sscanf error on server");
      exit(1);
   }
Any player can force the entire server to exit.
methodtwo wrote:
Code:
   x--; y--;
   printf("x is %d, y is %d\n", x, y);
   if(tsd->matrix[x][y] != ' '){
These indexes may not be in range.
methodtwo wrote:
Code:
    snprintf(textline, sizeof(textline), "Invalid move, try again.\n");
You have a literal string. Why use snprintf?
methodtwo wrote:
Code:
        snprintf(sendline, sizeof(sendline), "%d%c%d\n", x, space, y);
Why use %c here? Set the format string to "%d %d\n".
methodtwo wrote:
Code:

void get_computer_move(struct Threadz *tsd)
{
  int i, j;
  char sendlinez[10];
  char sendtext[10];
  char spacez=' ';
  /*Add intelligence here*/
  for(i=0;i<3;i++){
     for(j=0;j<3;j++)
       if(tsd->matrix[i][j]==' ') break;
     if(tsd->matrix[i][j]==' ') break;
This index may be invalid.
Back to top
View user's profile Send private message
methodtwo
Apprentice
Apprentice


Joined: 01 Feb 2008
Posts: 231

PostPosted: Sun Mar 24, 2013 9:20 pm    Post subject: Reply with quote

Thank you very much for the reply. I think the problem is that i'm accepting a connection with every player move instead of using the same connection for the whole game I'm using "thread specific data" because i'm not an experienced C coder and i don't know any other way of each thread having it's own private data struct for the games "state". Come to think of it i'm only using structs to make the code as simple as possible too! I don't know of any other way of coding this application. Sorry
Thank you very much for the previous reply and any future replies
regards
Back to top
View user's profile Send private message
Hu
Moderator
Moderator


Joined: 06 Mar 2007
Posts: 21489

PostPosted: Sun Mar 24, 2013 10:30 pm    Post subject: Reply with quote

Each thread has a private stack, which you already rely on for local variables in thread_main. You can store small thread-specific state there, or place large thread-specific state on the heap and store a pointer to it as a local variable in thread_main. If you put it on the heap, you must free it before the thread exits, since no other threads can free it for you.

Try this version instead. To reduce code size slightly, I am omitting the three functions that I did not modify from your code: init_matrix, disp_matrix, and check. I verified that the compiler accepts it, but have not run it to check correctness.

My version addresses all of the critiques I made above, except for the criticism regarding a client being able to cause the entire server to exit. I leave that up to you.
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 <unistd.h>
#include <string.h>
#include <errno.h>
#include <syslog.h>
#include <signal.h>
#include <pthread.h>

struct Thread{
   pthread_t thread_tid;
};

struct Game{
    pthread_t thread_tid;
    long thread_count;
    char matrix[3][3];
    int connfd;
};

static char check(struct Game *tsd);
static void init_matrix(struct Game *tsd);
static void get_player_move(struct Game *tsd);
static void get_computer_move(struct Game *tsd);
static void disp_matrix(struct Game *tsd);
static void thread_make(int, struct Thread *tptr);
static int sockfd;

int main(int argc, char *argv[])
{
 int err, i;
 int result, nthreads;
 const char* hostname=0;
 const char *port_num="12349";
 struct Thread *tptr;
 struct addrinfo hints, *res=0;
 if(argc < 2)
    return EXIT_FAILURE;
 nthreads = atoi(argv[argc - 1]);

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

 tptr = calloc(nthreads, sizeof(struct Thread));
 
 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)<0) {
   perror("bind failed");
   exit(1);
 }
 freeaddrinfo(res);

 if(listen(sockfd, 5) < 0) {
   perror("listen failed");
   exit(1);
 }
 if((result = daemon(0,0)) != 0){
    perror("Can't become a daemon!");
    exit(1);
 }

 for(i = 0; i < nthreads; i++)
    thread_make(i, tptr);
 /* suspend main until threads have done there work */
 for(i = 0; i < nthreads; i++)
    pthread_join(tptr[i].thread_tid, NULL);

 return(0);
}
 
static void *thread_main(void *);
static void thread_make(int i, struct Thread *tptr)
{
 
  pthread_create(&tptr[i].thread_tid, NULL, &thread_main, NULL);
  return; /*main thread returns */
}
static void *thread_main(void *threadarg)
{
   (void)threadarg;
  struct Game tsd;
  for (;;) {
   char done  = ' ';
   struct sockaddr cli_addr;
   socklen_t cli_len=sizeof(cli_addr);
      if((tsd.connfd = accept(sockfd, &cli_addr, &cli_len)) == -1){
         perror("Failed to accept connection");
         continue;
   }
   init_matrix(&tsd);
 do {
     disp_matrix(&tsd);
     get_player_move(&tsd);
     done = check(&tsd);
     if(done != ' ') break;
     get_computer_move(&tsd);
     done = check(&tsd);
} while(done == ' ');

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

  disp_matrix(&tsd);
  }
 
  return(0);
}
static void get_player_move(struct Game *tsd)
{
   char recvline[16], sendline[30];
   unsigned x, y;
   int n;
   
   for(;;) {
   if((n = read(tsd->connfd, recvline, sizeof(recvline))) <= 0){
      perror("Read error");
      exit(1);
   }
   if(sscanf(recvline, "%u%*c%u", &x, &y) != 2){
      perror("sscanf error on server");
      exit(1);
   }
   if (x >= sizeof(tsd->matrix)/sizeof(tsd->matrix[0]) || y >= sizeof(tsd->matrix[0])/sizeof(tsd->matrix[0][0])) {
      fprintf(stderr, "bad input from client socket %i", tsd->connfd);
      exit(1);
   }
   x--; y--;
   printf("x is %u, y is %u\n", x, y);
   if(tsd->matrix[x][y] != ' '){
      static const char textline[] = "Invalid move, try again.\n";
    if(write(tsd->connfd, textline, sizeof(textline))  ==-1){
       perror("write failed");
       exit(1);
   }
   
   }
   else {
        tsd->matrix[x][y] = 'X';
        snprintf(sendline, sizeof(sendline), "%d %d\n", x, y);
        if(write(tsd->connfd, sendline, sizeof(sendline)) ==-1){
            perror("send move back failed");
            exit(1);
        }
   else
       break;
    }

 }

}

static void get_computer_move(struct Game *tsd)
{
  int i, j;
  char sendlinez[10];
  /*Add intelligence here*/
  for(i=0;i<3;i++){
     for(j=0;j<3;j++)
       if(tsd->matrix[i][j]==' ') break;
     if(j<3 && tsd->matrix[i][j]==' ') break;
  }
  printf("i is %d, j is %d\n", i, j);
  if(i*j==9) {
     static const char sendtext[] = "Draw!\n";
   if(write(tsd->connfd, sendtext, sizeof(sendtext)) ==-1){
      perror("sending draw message error");
      exit(1);
   }
  }
  else {
   
   snprintf(sendlinez, sizeof(sendlinez), "%d %d\n", i, j);
   if(write(tsd->connfd, sendlinez, sizeof(sendlinez)) ==-1){
     perror("Error sending computer move");
     exit(1);
   }
  tsd->matrix[i][j] = 'O';
   }
}
Back to top
View user's profile Send private message
methodtwo
Apprentice
Apprentice


Joined: 01 Feb 2008
Posts: 231

PostPosted: Mon Mar 25, 2013 2:44 pm    Post subject: Reply with quote

Thank you ever so much. I did get a variation of the original design sotra working. Needs more work though(Doesn't always work). I'm sure your version is much better though. Thank you very much for sharing it with me! Have just moved house so will test your version over the next day or two from now(28.3.13) If that is you're interested to know how it did?
regards
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