Prev: Using getnameinfo()
Next: How do you update a Makefile?
From: arnuld on 30 Jun 2008 06:21
I have this program, which uses poll() to handle multiple clients. It is
from UNP section 6.11 with my modifications

WANTED: If a client sends something then server should echo back that
data to *all* the connected clients.

PROBLEM: It echoes it back to only one client, the client which sent the
data.

The only major modification I made to the original program, is
introducing a for loop just after recv()ing the data:


--------------------- echo-server -----------------------------------
/* A server handeling multiple clients using poll() */

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <errno.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <arpa/inet.h>
#include <poll.h>

#define SERVER_IP "127.0.0.1"

enum { ARRSIZE = 101, BACKLOG = 100, OPEN_MAX = 10000, INFTIM = -1 };

int main( int argc, char** argv )
{
int sockfd, accept_fd, myport;
struct sockaddr_in servaddr, cliaddr;
socklen_t clilen;
char arrc[ARRSIZE];
int recv_err;

/* preparations for poll() */
struct pollfd clients[OPEN_MAX];
int i, nready;
int curr_index, max_index;

/* clear the array and the address structures */
memset( arrc, '\0', ARRSIZE );
memset( &servaddr, '\0', sizeof(servaddr) );
memset( &cliaddr, '\0', sizeof(cliaddr) );

if( argc != 2 )
{
fprintf(stderr, "USAGE: ./file PORT\n");
exit( EXIT_FAILURE );
}
else
{
myport = atoi( argv[1] );
}

if( (sockfd = socket(PF_INET, SOCK_STREAM, 0)) < 0 )
{
perror("SOCKET() error\n");
exit( EXIT_FAILURE );
}

servaddr.sin_family = AF_INET;
servaddr.sin_port = htons(myport);
if( inet_pton(AF_INET, SERVER_IP, &servaddr.sin_addr) <= 0 )
{
perror("inet_pton: error\n");
exit(EXIT_FAILURE);
}

if( bind(sockfd, (struct sockaddr*)&servaddr, sizeof(struct sockaddr)) < 0 )
{
perror("BIND() error\n");
exit( EXIT_FAILURE );
}

if( listen(sockfd, BACKLOG) < 0 )
{
perror("LISTEN() error\n");
exit( EXIT_FAILURE );
}

/* poll() initialisation */
clients[0].fd = sockfd;
clients[0].events = POLLIN;
max_index = 0;

for(i = 1; i < OPEN_MAX; ++i )
{
clients[i].fd = -1;
}

/* here we start to accept connections */
for( ; ; )
{
nready = poll( clients, max_index + 1, INFTIM );

if( clients[0].revents & POLLIN )
{
clilen = sizeof( cliaddr );
accept_fd = accept(sockfd, (struct sockaddr*)&servaddr, &clilen);

printf("ACCEPT()ed \n");
/* add the new FD to the poll() */
for( i=1; i < OPEN_MAX; ++i )
{
if( clients[i].fd < 0 )
{
clients[i].fd = accept_fd;
break;
}
}

curr_index = i;

/* check if the poll() is full of connections */
if( curr_index == OPEN_MAX )
{
perror("Too many connections\n");
exit( EXIT_FAILURE );
}

/* After the check, now we can add to events */
clients[curr_index].events = POLLIN;

if( curr_index > max_index )
{
max_index = curr_index;
}

if( --nready <= 0 )
{
continue;
}

} /* if condition for checking connection is done with */

/* now we check & read the data */
for( i = 1; i <= max_index; ++i )
{
if( clients[i].fd < 0 )
{
continue;
}

if( clients[i].revents & (POLLIN | POLLERR) )
{
if( (recv_err = recv(accept_fd, arrc, ARRSIZE, 0)) <= 0 )
{
if( errno == ECONNRESET )
{
perror("Remote-Clients did a RESET");
close( accept_fd );
clients[i].fd = -1;
}
else if( 0 == recv_err )
{
perror("Remote-Clients closed the connection");
close( accept_fd );
clients[i].fd = -1;
}
else
{
perror("RECV() error");
exit( EXIT_FAILURE );
}
}
else
{
printf("RECV()ed Data\n");
/* here we have recieved the data without any error
Now we will send this datt to every accepted file-decriptor
stores on poll array named clients. We do so using a for loop
till we hit the max_index.
*/
for( i=1; i <= max_index; ++i )
{
/* if( clients[i].fd <= 0 )
{
continue;
}*/
if( send( clients[i].fd, arrc, ARRSIZE, 0 ) < 0 )
{
perror("SEND() error");
exit( EXIT_FAILURE );
}
}
}

printf("::\n");
if( --nready <= 0 )
{
break;
}
}
}
}


return 0;

}


--------------------- echo-client ----------------------------------
/* A client to connect and send some data to server */

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <errno.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <arpa/inet.h>


#define SERVER_IP "127.0.0.1"

enum { ARRSIZE = 101, BACKLOG = 100, OPEN_MAX = 10000, INFTIM = -1 };

int main( int argc, char** argv )
{
int sockfd, myport;
struct sockaddr_in cliaddr;
char arrc[ARRSIZE] = "A Canticle for Leibowitz\n";
char arrc_store[ARRSIZE];

if( argc != 2 )
{
fprintf(stderr, "USAGE: ./file PORT\n");
exit( EXIT_FAILURE );
}
else
{
myport = atoi( argv[1] );
}


memset( arrc_store, '\0', ARRSIZE);
memset( &cliaddr, '\0', sizeof(cliaddr) );

if( (sockfd = socket(PF_INET, SOCK_STREAM, 0)) < 0 )
{
perror("SOCKET() error\n");
exit( EXIT_FAILURE );
}

cliaddr.sin_family = AF_INET;
cliaddr.sin_port = htons(myport);
if( inet_pton(AF_INET, SERVER_IP, &cliaddr.sin_addr) < 0 )
{
perror("inet_pton: error\n");
exit(EXIT_FAILURE);
}


if( connect(sockfd, (struct sockaddr*) &cliaddr, sizeof(struct sockaddr)) == -1 )
{
perror("CONNECT() error");
exit( EXIT_FAILURE );
}


if( send(sockfd, arrc, strlen(arrc), 0) == -1 )
{
perror("SEND() error");
exit( EXIT_FAILURE );
}


if( recv(sockfd, arrc_store, ARRSIZE, 0) == -1 )
{
perror("RECV() error");
exit( EXIT_FAILURE );
}

printf("--%s", arrc_store);

/* just to keep client connected */
while(1)
{
;
}


return 0;
}




--
www.lispmachine.wordpress.com
my email is @ the above blog
check the "About Myself" page

From: phil-news-nospam on 30 Jun 2008 12:03
On Mon, 30 Jun 2008 15:21:52 +0500 arnuld <sunrise(a)invalid.address> wrote:

| /* now we check & read the data */
| for( i = 1; i <= max_index; ++i )

Variable i is used to control the outer loop.

| {
| if( clients[i].fd < 0 )
| {
| continue;
| }
|
| if( clients[i].revents & (POLLIN | POLLERR) )
| {
| if( (recv_err = recv(accept_fd, arrc, ARRSIZE, 0)) <= 0 )
| {
| if( errno == ECONNRESET )
| {
| perror("Remote-Clients did a RESET");
| close( accept_fd );
| clients[i].fd = -1;
| }
| else if( 0 == recv_err )
| {
| perror("Remote-Clients closed the connection");
| close( accept_fd );
| clients[i].fd = -1;
| }
| else
| {
| perror("RECV() error");
| exit( EXIT_FAILURE );
| }
| }
| else
| {
| printf("RECV()ed Data\n");
| /* here we have recieved the data without any error
| Now we will send this datt to every accepted file-decriptor
| stores on poll array named clients. We do so using a for loop
| till we hit the max_index.
| */
| for( i=1; i <= max_index; ++i )

Variable i is used to control the inner loop, corrupting its value for the
outer loop.

| {
| /* if( clients[i].fd <= 0 )
| {
| continue;
| }*/
| if( send( clients[i].fd, arrc, ARRSIZE, 0 ) < 0 )
| {
| perror("SEND() error");
| exit( EXIT_FAILURE );
| }
| }
| }
|
| printf("::\n");
| if( --nready <= 0 )
| {
| break;
| }
| }
| }

I don't know if this conflict of usage will cause your error, but it may
well cause the list of clients to be corrupt, and so ultimately itt could
be the problem. I generally stop analysis when I see a problem this severe,
fix it, and retry, before any further analysis.

--
|WARNING: Due to extreme spam, googlegroups.com is blocked. Due to ignorance |
| by the abuse department, bellsouth.net is blocked. If you post to |
| Usenet from these places, find another Usenet provider ASAP. |
| Phil Howard KA9WGN (email for humans: first name in lower case at ipal.net) |
From: David Schwartz on 30 Jun 2008 22:43
On Jun 30, 3:21 am, arnuld <sunr...(a)invalid.address> wrote:

>   /* here we start to accept connections */
>   for( ; ; )
>     {
>       nready = poll( clients, max_index + 1, INFTIM );

Oops, you forgot to setup 'clients'. It will be right the first time,
but it will be wrong the second time.

>       if( clients[0].revents & POLLIN )
>         {
>           clilen = sizeof( cliaddr );
>           accept_fd = accept(sockfd, (struct sockaddr*)&servaddr, &clilen);
>
>           printf("ACCEPT()ed \n");
>           /* add the new FD to the poll() */
>           for( i=1; i < OPEN_MAX; ++i )
>             {
>               if( clients[i].fd < 0 )
>                 {
>                   clients[i].fd = accept_fd;
>                   break;
>                 }
>             }
>
>           curr_index = i;
>
>           /* check if the poll() is full of connections */
>           if( curr_index == OPEN_MAX )
>             {
>               perror("Too many connections\n");
>               exit( EXIT_FAILURE );
>             }
>
>           /* After the check, now we can add to events */
>           clients[curr_index].events = POLLIN;

Umm, you're manipulating the 'clients' you got *BACK* from 'poll'.
That makes no sense.

>           if( curr_index > max_index )
>             {
>               max_index = curr_index;
>             }
>
>           if( --nready <= 0 )
>             {
>               continue;
>             }
>
>         } /* if condition for checking connection is done with */
>
>           /* now we check & read the data */
>           for( i = 1; i <= max_index; ++i )
>             {
>               if( clients[i].fd < 0 )
>                 {
>                   continue;
>                 }
>
>               if( clients[i].revents & (POLLIN | POLLERR) )
>                 {
>                   if( (recv_err = recv(accept_fd, arrc, ARRSIZE, 0)) <= 0 )
>                     {
>                       if( errno == ECONNRESET )

Oops, you're checking 'errno' even if the return value is zero. And
why are you operating on 'accept_fd' rather than clients[i].fd? And if
you throw away the return value from 'recv', how will you know how
many bytes to send each client?

>                         {
>                           perror("Remote-Clients did a RESET");
>                           close( accept_fd );
>                           clients[i].fd = -1;
>                         }
>                       else if( 0 == recv_err )
>                         {
>                           perror("Remote-Clients closed the connection");
>                           close( accept_fd );
>                           clients[i].fd = -1;
>                         }
>                       else
>                         {
>                           perror("RECV() error");
>                           exit( EXIT_FAILURE );
>                         }
>                     }
>                   else
>                     {
>                       printf("RECV()ed Data\n");
>                       /* here we have recieved the data without any error
>                          Now we will send this datt to every accepted file-decriptor
>                          stores on poll array named clients. We do so using a for loop
>                          till we hit the max_index.
>                       */
>                       for( i=1; i <= max_index; ++i )
>                         {
>                           /*                      if( clients[i].fd <= 0 )
>                             {
>                               continue;
>                               }*/
>                           if( send( clients[i].fd, arrc, ARRSIZE, 0 ) < 0 )
>                             {
>                               perror("SEND() error");
>                               exit( EXIT_FAILURE );
>                             }
>                         }
>                     }

Umm, you didn't receive 'ARRSIZE' bytes. You received *at* *most*
ARRSIZE bytes. So you are sending the clients the wrong number of
bytes. You are also clobbering your outer loop index.

>                   printf("::\n");
>                   if( --nready <= 0 )
>                     {
>                       break;
>                     }
>                 }
>             }
>     }
>
>   return 0;
>
> }

>   if( send(sockfd, arrc, strlen(arrc), 0) == -1 )
>     {
>       perror("SEND() error");
>       exit( EXIT_FAILURE );
>     }

Note that this does not send the terminating zero byte. It does send
the newline. This may or may not make sense.

>   if( recv(sockfd, arrc_store, ARRSIZE, 0) == -1 )
>     {
>       perror("RECV() error");
>       exit( EXIT_FAILURE );
>     }

You threw away the return value from 'recv'. Now you have no idea how
many bytes you got back. Also, you have no idea if 'recv' returned
zero (indicating normal closing of the connection).

>   printf("--%s", arrc_store);

Umm, '%s' is for C-style strings, not for arbitrary bytes. You need to
either make a C-style string or print each character received.

>   /* just to keep client connected */
>   while(1)
>     {
>       ;
>     }

Umm, huh? You don't need to run the CPU at 100% just to keep the
client connected. Why not block in 'recv'?

You have a large number of bugs.

DS
From: arnuld on 1 Jul 2008 00:52
> On Mon, 30 Jun 2008 16:03:14 +0000, phil-news-nospam wrote:


> Variable i is used to control the outer loop.
> ........... SNIP ..........
>
> Variable i is used to control the inner loop, corrupting its value for the
> outer loop.

Oh.. no... my bad :(



> I don't know if this conflict of usage will cause your error, but it may
> well cause the list of clients to be corrupt, and so ultimately itt
> could be the problem. I generally stop analysis when I see a problem
> this severe, fix it, and retry, before any further analysis.

I fixed it but it does not make much difference. The program still behaves
the same. I also did some changes David Schwartz mentioned.



--
www.lispmachine.wordpress.com
my email is @ the above blog
check the "About Myself" page

From: arnuld on 1 Jul 2008 01:41
> On Mon, 30 Jun 2008 19:43:52 -0700, David Schwartz wrote:

>> On Jun 30, 3:21�am, arnuld <sunr...(a)invalid.address> wrote:

>> nready = poll( clients, max_index + 1, INFTIM );

> Oops, you forgot to setup 'clients'. It will be right the first time,
> but it will be wrong the second time.


I did initialize it up properly, see:

/* poll() initialisation */
clients[0].fd = sockfd;
clients[0].events = POLLIN;
max_index = 0;

for(i = 1; i < OPEN_MAX; ++i )
{
clients[i].fd = -1;
}


Is there something wrong with that kind of initialization ?


>> /* After the check, now we can add to events */
>> clients[curr_index].events = POLLIN;

> Umm, you're manipulating the 'clients' you got *BACK* from 'poll'.
> That makes no sense.


ummmm... that is just a new accept()ed connection added into the clients
array and hence I need to make "events" POLLIN.

There is only one array and that is an array of pollfd structures named
"clients" in my code and simply "client" in code from UNP.

it seems like I did not get your point.




> Oops, you're checking 'errno' even if the return value is zero.

Now I know why Stevens did that. Code changed.



> And why are you operating on 'accept_fd' rather than clients[i].fd?

My mistake. code changed.



> And if you throw away the return value from 'recv', how will you
> know how many bytes to send each client?

I threw it away because I am really confused between bytes and the
array of characters I send. I want send a sentence Like "David Schwartz is
an ancient ape who knows about sockets ;)" but that sentence can be sent
as an array of characters only but then I can't because send() sends bytes
not array. Well, how I am supposed to send the sentence ? and on the top
of that recv() gives me bytes, not the characters. Why do I have to mess
with bytes rather than the intent of what I want to send ?

Mixing my program design, my characters, sentence and bytes I see that
send() does send *exactly* the whole sentence as bytes even when I tell it
the size in an integer format, not in byte format. ... Eh..... I am
getting confused :-\





>> if( send( clients[i].fd, arrc,ARRSIZE, 0 ) < 0 ) � � � � � � �
>> � � � � � � � { � � � � � � � � � � � � � � �
>> perror("SEND() error"); � � � � � � � � � � � � � � �
>> exit( EXIT_FAILURE ); � � � � � � � � � � � � � � } � �� � �

> Umm, you didn't receive 'ARRSIZE' bytes. You received *at* *most*
> ARRSIZE bytes. So you are sending the clients the wrong number of bytes.
> You are also clobbering your outer loop index.


same as above


>> � if( send(sockfd, arrc, strlen(arrc), 0) == -1 ) � � { � � �
>> perror("SEND() error");
>> � � � exit( EXIT_FAILURE );
>> � � }

> Note that this does not send the terminating zero byte. It does send the
> newline. This may or may not make sense.

so it should be strlen(arrc) + 1


>> � if( recv(sockfd, arrc_store, ARRSIZE, 0) == -1 ) � � { � � �
>> perror("RECV() error");
>> � � � exit( EXIT_FAILURE );
>> � � }

> You threw away the return value from 'recv'. Now you have no idea how
> many bytes you got back. Also, you have no idea if 'recv' returned zero
> (indicating normal closing of the connection).


>> � printf("--%s", arrc_store);
>
> Umm, '%s' is for C-style strings, not for arbitrary bytes. You need to
> either make a C-style string or print each character received.

arrac_store is declared as an array of characters. May be C automatically
converted the bytes into an array of characters


>> � /* just to keep client connected */ � while(1) � � { � � � ;
>> � � }

> Umm, huh? You don't need to run the CPU at 100% just to keep the client
> connected. Why not block in 'recv'?


because recv() does not work. Client disconnects automatically when I
replace while(1) with recv(...)



> You have a large number of bugs.

I did not know that when I created the program. Well, thats why I posted
it here, to ask for improvements from socket-masters =:)



--
www.lispmachine.wordpress.com
my email is @ the above blog
check the "About Myself" page

 |  Next  |  Last
Pages: 1 2 3 4
Prev: Using getnameinfo()
Next: How do you update a Makefile?