select function help

Can someone please clean this up for me a little bit. Make it more basic! I can code the basic server with no problem and think it is time to learn the select function! I took the code for this select server from a web site and tweaked it a bit. Towards the end it kind of gets sloppy! The program uses so many nested if's and a few for loops. I just need it much more basic.
There aren't any good tutorials out there for the select server. There is beej's guide but I have a hard time with their approach! So I figure, make it as basic as possible, study the make up and placement of the various select functions. Such as FD_SET, FD_ISSET, setsockopt, and all that good stuff. I have a basic understanding of the select functions, but not to great.
Also, I am aware that inet_ntoa is a depricated function and that I should use inet_ntop, because it works with both ipv4 and ipv6. I'll work on that later! Thanks in advance for any help and/or criticism!

Code:

---

    /* *******select.c*********/
    /* *******Using select() for I/O multiplexing */
    #include <stdio.h>
    #include <string.h>
    #include <unistd.h>
    #include <sys/types.h>
    #include <sys/socket.h>
    #include <netinet/in.h>
    #include <netdb.h>
    #include <sys/select.h>

    int main(int argc, char *argv[]) {

    /* Not a problem! */
    fd_set master;

    /* Not a problem! */
    fd_set read_fds;

    /* Not a problem! */
    struct sockaddr_in serv_addr, cli_addr;
    int sockfd, newsockfd, fdmax, i, j, portno, nbytes; 
    socklen_t clilen;
    int yes = 1;
    char buf[1024];

    /* Not a problem! */
    FD_ZERO(&master);
    FD_ZERO(&read_fds);


    if((sockfd = socket(AF_INET, SOCK_STREAM, 0)) < 0)
    fprintf(stderr, "Error creating Socket.");

    /* Not a problem! */
    if(setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR, &yes, sizeof(int)) < 0)
    fprintf(stderr, "Server-setsockopt() error.");


    bzero((char *) &serv_addr, sizeof(serv_addr));
    portno = atoi(argv[1]);
    serv_addr.sin_family = AF_INET;
    serv_addr.sin_addr.s_addr = INADDR_ANY;
    serv_addr.sin_port = htons(portno);


    if(bind(sockfd, (struct sockaddr *)&serv_addr, sizeof(serv_addr)) < 0)
    fprintf(stderr, "Error binding to socket.");


    /* listen */
    listen(sockfd, 10);
    clilen = sizeof(cli_addr);

    /* Not a problem! */
    FD_SET(sockfd, &master);

    /* Not a problem! */
    fdmax = sockfd; /* so far, it's this one*/


    /* It gets a little sloppy here. There are so many nested if statements and loops. */
    for( ; ; ) {

    /* copy it */
    read_fds = master;



    if(select(fdmax+1, &read_fds, NULL, NULL, NULL) < 0)
    fprintf(stderr, "Select server error.");


    /*run through the existing connections looking for data to be read*/
    for(i = 0; i <= fdmax; i++) {

        if(FD_ISSET(i, &read_fds)) {

        if(i == sockfd) {

    if((newsockfd = accept(sockfd, (struct sockaddr *) &cli_addr, &clilen)) < 0)
    fprintf(stderr, "Error accepting connection.");


    FD_SET(newsockfd, &master); /* add to master set */

    /* keep track of the maximum */
    if(newsockfd > fdmax) {
        fdmax = newsockfd;

    }

    /*Use of depricated function inet_ntoa (network to ascii). I should use inet_ntop*/
    printf("%s: New connection from %s on socket %d\n", argv[0], inet_ntoa(cli_addr.sin_addr), newsockfd);

    }

    }

    else {

    /* handle data from a client */
    if((nbytes = recv(i, buf, sizeof(buf), 0)) <= 0) {

    /* got error or connection closed by client */

    if(nbytes == 0)

      /* connection closed */
      fprintf(stderr, "%s: socket %d hung up\n", argv[0], i);

    /* close it... */
    close(i);

    /* remove from master set */
    FD_CLR(i, &master);
    }

    else {
    /* we got some data from a client*/

    for(j = 0; j <= fdmax; j++) {

    /* send to everyone! */

    if(FD_ISSET(j, &master)) {

            /* except the listener and ourselves */

            if(j != sockfd && j != i) {

                  if(send(j, buf, nbytes, 0) < 0)

                          fprintf(stderr, "send() error!");
                                                  }

                                                }

                                                }

          }

    }
 }   
 }


    return 0;

    }

---

Well it would help if your indentation didn't suck as badly as it does.

One mistake is a mis-matched if/else, meaning you're doing the wrong thing.

Code:

---

    /* *******select.c******** */
    /* *******Using select() for I/O multiplexing */
#include <stdio.h>
#include <string.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <netdb.h>
#include <sys/select.h>

int main(int argc, char *argv[])
{
  /* Not a problem! */
  fd_set master;

  /* Not a problem! */
  fd_set read_fds;

  /* Not a problem! */
  struct sockaddr_in serv_addr, cli_addr;
  int sockfd, newsockfd, fdmax, i, j, portno, nbytes;
  socklen_t clilen;
  int yes = 1;
  char buf[1024];

  /* Not a problem! */
  FD_ZERO(&master);
  FD_ZERO(&read_fds);

  if ((sockfd = socket(AF_INET, SOCK_STREAM, 0)) < 0)
    fprintf(stderr, "Error creating Socket.");

  /* Not a problem! */
  if (setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR, &yes, sizeof(int)) < 0)
    fprintf(stderr, "Server-setsockopt() error.");

  bzero((char *) &serv_addr, sizeof(serv_addr));
  portno = atoi(argv[1]);
  serv_addr.sin_family = AF_INET;
  serv_addr.sin_addr.s_addr = INADDR_ANY;
  serv_addr.sin_port = htons(portno);

  if (bind(sockfd, (struct sockaddr *) &serv_addr, sizeof(serv_addr)) < 0)
    fprintf(stderr, "Error binding to socket.");

  /* listen */
  listen(sockfd, 10);
  clilen = sizeof(cli_addr);

  /* Not a problem! */
  FD_SET(sockfd, &master);

  /* Not a problem! */
  fdmax = sockfd;              /* so far, it's this one */

  /* It gets a little sloppy here. There are so many nested if statements and loops. */
  for (;;) {
    /* copy it */
    read_fds = master;

    if (select(fdmax + 1, &read_fds, NULL, NULL, NULL) < 0)
      fprintf(stderr, "Select server error.");

    /*run through the existing connections looking for data to be read */
    for (i = 0; i <= fdmax; i++) {
      if (FD_ISSET(i, &read_fds)) {
        if (i == sockfd) {
          if ((newsockfd =
              accept(sockfd, (struct sockaddr *) &cli_addr, &clilen)) < 0)
            fprintf(stderr, "Error accepting connection.");

          FD_SET(newsockfd, &master); /* add to master set */
          /* keep track of the maximum */
          if (newsockfd > fdmax) {
            fdmax = newsockfd;
          }

          /*Use of depricated function inet_ntoa (network to ascii). I should use inet_ntop */
          printf("%s: New connection from %s on socket %d\n", argv[0],
                inet_ntoa(cli_addr.sin_addr), newsockfd);
        }
      }
      //!! OOPS
      //!! This is the else for "if (FD_ISSET(i, &read_fds))"
      //!! not the else for any other connection except sockfd
      else {
        /* handle data from a client */
        if ((nbytes = recv(i, buf, sizeof(buf), 0)) <= 0) {
          /* got error or connection closed by client */
          if (nbytes == 0)
            /* connection closed */
            fprintf(stderr, "%s: socket %d hung up\n", argv[0], i);

          /* close it... */
          close(i);
          /* remove from master set */
          FD_CLR(i, &master);
        }
        else {
          /* we got some data from a client */
          for (j = 0; j <= fdmax; j++) {
            /* send to everyone! */
            if (FD_ISSET(j, &master)) {
              /* except the listener and ourselves */
              if (j != sockfd && j != i) {
                if (send(j, buf, nbytes, 0) < 0)
                  fprintf(stderr, "send() error!");
              }
            }
          }
        }
      }
    }
  }

  return 0;
}

---

The other problem you have is you don't update fdmax when connections are closed.

You might consider simplifying the code as well, by adding some functions.
Eg.

Code:

---

    for (i = 0; i <= fdmax; i++) {
      if (FD_ISSET(i, &read_fds)) {
        if (i == sockfd) {
          doNewClient();//!! choose some parameters
        } else {
          doExistingClients();//!! choose some parameters
        }
      }
    }

---

Server side: I open the server and connect the client. The automatic response from server is an error; Error reading from socket. Then prints out a blank message from client and skips to message for the server to send back.
Client side: Client automatically prompts, please enter message, as soon as it connects. When you enter a message, it prints blank space like a new line char in the buffer and the repeats the please enter a message prompt. What am I doing wrong? BTW I cleaned it up a great deal Salem and improved the indentation.

SERVER:

Code:

---

#include <string.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <netdb.h>
#include <netinet/in.h>
#include <sys/select.h>

int main(int argc, char *argv[]) {
int sockfd, newsockfd, portno, i, b, fdmax;
struct sockaddr_in serv_addr, cli_addr;
socklen_t clilen;
char buffer[1024];
fd_set master;
fd_set read_fds;
int yes = 1;

if(argc < 2) fprintf(stderr, "No port provided.\n");

FD_ZERO(&master);
FD_ZERO(&read_fds);

sockfd = socket(AF_INET, SOCK_STREAM, 0);
if(sockfd < 0) fprintf(stderr, "Error creating socket.\n");

if(setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR, &yes, sizeof(int)) < 0) fprintf(stderr, "Socket already in use.\n");

bzero((char *) &serv_addr, sizeof(serv_addr));
portno = atoi(argv[1]);
serv_addr.sin_family = AF_INET;
serv_addr.sin_addr.s_addr = INADDR_ANY;
serv_addr.sin_port = htons(portno);

if(bind(sockfd, (struct sockaddr *) &serv_addr, sizeof(serv_addr)) < 0) fprintf(stderr, "Error binding to socket.\n");

listen(sockfd, 10);
clilen = sizeof(cli_addr);

FD_SET(sockfd, &master);

fdmax = sockfd;

        for( ; ; ) {

        read_fds = master;

        if(select(fdmax+1, &read_fds, NULL, NULL, NULL) < 0)    fprintf(stderr, "Select error.\n");

          for(i = 0; i <= fdmax; i++) {
                if(FD_ISSET(i, &read_fds)) {
                        if(i == sockfd) {
                        newsockfd = accept(sockfd, (struct sockaddr *) &cli_addr, &clilen);
                        if(newsockfd < 0) fprintf(stderr, "Error accepting new connection.\n");
                        FD_SET(newsockfd, &master);
                        if(newsockfd > fdmax) fdmax = newsockfd;
                        }
                fprintf(stdout, "New connection from %c on %d.\n", inet_ntoa(cli_addr.sin_addr), newsockfd);
                }
                                /*handle data, close connection, remove from temp set(read_fds)*/
                                for( ; ; ) {
                                int b = sizeof(buffer); if(b > 0) bzero(buffer, 1024);
                                if(recv(i, buffer, strlen(buffer)-1, 0) < 0) fprintf(stderr, "Error reading from socket.\n");
                                fprintf(stdout, "Client: %s\n", buffer);

                                b = sizeof(buffer); if(b > 0) bzero(buffer, 1024);
                                fprintf(stdout, "message: ");
                                fgets(buffer, sizeof(buffer), stdin);
                                if(send(i, buffer, strlen(buffer)-1, 0) < 0) fprintf(stderr, "Error writing to socket.\n");
                                }
          close(i);
          FD_CLR(i, &read_fds);
          }
        }
}

---

CLIENT:

Code:

---

#include <string.h>
#include <stdio.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <netdb.h>
#include <netinet/in.h>

int main(int argc, char *argv[]) {
int sockfd, portno;
char buffer[1024];
struct hostent *server;
struct sockaddr_in serv_addr;

if(argc < 3) fprintf(stderr, "Ussage: %s IP Address port #\n", argv[0]);

sockfd = socket(AF_INET, SOCK_STREAM, 0);
if(sockfd < 0) fprintf(stderr, "Error creating socket.\n");

bzero((char *) &serv_addr, sizeof(serv_addr));
server = gethostbyname(argv[1]);
if(server == NULL) fprintf(stderr, "No such host.\n");
portno = atoi(argv[2]);
serv_addr.sin_family = AF_INET;
memcpy(&serv_addr.sin_addr.s_addr, server->h_addr, server->h_length);
serv_addr.sin_port = htons(portno);

if(connect(sockfd, (struct sockaddr *) &serv_addr, sizeof(serv_addr)) < 0) fprintf(stderr, "Error connecting to server.\n");

                                for( ; ; ) {
                                    fprintf(stdout, "Please enter the message: ");
                                  int i = sizeof(buffer); if(i > 0) bzero(buffer, 1024);
                                    fgets(buffer,sizeof(buffer), stdin);
                                    if(send(sockfd, buffer, strlen(buffer)-1, 0)< 0)
                                fprintf(stderr, "ERROR writing to socket.\n");
                                    i = sizeof(buffer); if(i > 0) bzero(buffer, 1024);
                                    if(recv(sockfd, buffer, strlen(buffer)-1, 0)< 0)
                                fprintf(stderr, "ERROR reading from socket.\n");
                                    printf("%s\n", buffer);                                 
                                }

close(sockfd);
}

---

Any help is appreciated!

So I don't think you've got the hang of this indentation yet: Indent style - Wikipedia, the free encyclopedia

You're saying that this line prints?

Code:

---

if(recv(i, buffer, strlen(buffer)-1, 0) < 0) fprintf(stderr, "Error reading from socket.\n");

---

You should check errno, because there are lots of things it could be.

ETA: Okay, some other points.

1) You've got some warnings about functions you didn't include the headers for (inet_ntoa in the server, atoi in the client).
2) Your error checking is incomplete:

Code:

---

tabstop@ubuntu:~/helping$ ./srvr
No port provided.
Segmentation fault

---

You should either stop once you recognize the error, or somehow acquire a valid port. And for the all the <0 error checks, you really should be checking errno.

tabstop, when you run the server on the command line your getting that error because your not providing the port. the server takes in 2 arguments on the command line. One is obviously the program name and 2 is the port you want to listen on. Try this; ./srvr 1234

and sorry I forgot the arpa/inet header. I included that now.

but im still not seeing what im doing wrong.

The point is that you have an error on nearly every line. If a port is not provided, you must stop, you cannot continue. If you have an error creating the socket, you must stop, you cannot continue. And so on.

Also, you need to check errno on errors. Have you typed "man recv" at your computer yet? If not, you should do so; at the bottom are a list of all the possible reasons recv can return -1, and there are about a dozen. It's a lot easier to identify the source of your problem if you actually take the time to figure out what your problem is (instead of just ignoring it).

ETA: Changing that particular line to

Code:

---

if(recv(i, buffer, strlen(buffer)-1, 0) < 0) perror("Error reading from socket.");

---

gives (me, anyway) the error "Socket operation on non-socket", which suggests to me that i is not what you think it is at this point in your code.