[LCP]sockets problem

[Paul Gearon]

On Sun, Jan 27, 2002 at 05:56:56PM +0530, Srinath ? wrote:
> Hi all,
> 
> I am just learning sockets. I wrote two programs - serv.c and clie.c .
> my aim is to run them separately in two virtual terminals and if i type
> something in one of them and press enter, it should go to the other one.
> I've copied both of them into this email. now , my problem is that if i
> type in client, it is reaching the server, but when i type in the server, it
> is not reaching the client. In fact, that particular block never gets control.
> Please tell me where i am making mistake !!

There's a few things here, both the problems that you describe plus a few others
you haven't seen yet.  I'll see if I can get them all for you (sorry if I miss
something).  This is a bit long winded, so anyone else may not want to read it
all.  :-)

I'll approach it one part at a time:

Firstly, it's a little unusual to create a new connection for every message.  If
that kind of thing is needed then it's more usual to use UDP packets rather than
the connection oriented protocol of TCP.  However, since you're doing this to
learn, I'll assume that you know this and just move on...

> *******************clie.c*******************
<snip>
> #define CLIENTPORT 4999

It's rather unusual to define the local port number.  You normally allow the
local port to be allocated automatically (though it won't hurt to do this).

>  ht=gethostbyname("SRI");

I recommend using "localhost" here (that way it will run on other machines).

<snip>
>  memset(buffer,'\0',34);

You don't need this (especially since you re-use the buffer regularly in a loop
without re-initializing it).

>  FD_ZERO(&clientset);
and also...
>  FD_SET(0,&clientset);
>  FD_SET(scklisten,&clientset);
> 
>  while(1)
>  {
>   select(scklisten+1,&clientset,NULL,NULL,0);

OK, here's your first big problem.  You're setting up "clientset" outside of your
loop.  That only works the first time through.  After select() returns, clientset
has one of the file descriptors set (which you correctly check with FD_ISSET) but
it *stays* set the next time through the loop.  That means that clientset is not
set correctly the next time you call select().

You need to move the FD_ZERO and FD_SET macros inside the loop so you can reset
clientset before calling select(), something like this:

  while(1) {
    FD_ZERO(&clientset);
    FD_SET(0,&clientset);
    FD_SET(scklisten,&clientset);

    select(scklisten+1,&clientset,NULL,NULL,0);
    /* rest of loop */
  }


>   if(FD_ISSET(scklisten,&clientset))
>   {
>    sockno=accept(scklisten, (struct sockaddr *)&serv, &size);
>    read(sockno,buffer,34);
>    printf("\n%s\n",buffer);
>    close(sockno);
>   }

You shouldn't make the mistake of not checking the return value of your function
calls.  This gets you into trouble later in serv.c.  (OK, so you're only trying
to learn this stuff, and you're just trying to get the code running.  I've done
this sort of thing myself  :-)  I only really mention it because of the problem
in serv.c).

Next you make a big error by not checking the return value of the read() call.
After the first time through the loop the data you've just read may not end with
a zero, and you'll end up with left over data at the end of the buffer (at least
the last byte of the buffer will always be 0, so you won't segfault).  You have
a spare variable called "len" which I assume is for this purpose.

A better way to do this might be:

    if(FD_ISSET(scklisten,&clientset)) {
      if (-1 != (sockno=accept(scklisten, (struct sockaddr *)&serv, &size))) {
        len = read(sockno,buffer,34);
        buffer[len] = 0;  /* make sure the string is null terminated! */
        printf("\n%s\n",buffer);
        close(sockno);
      } else printf("Error accepting connection\n");
    }


>   if(FD_ISSET(0,&clientset))
>   {
>    read(0,buffer,34);
>    sockno=socket(AF_INET,SOCK_STREAM,0);
>    connect(sockno,(struct sockaddr *)&serv, sizeof(struct sockaddr));
>    write(sockno,buffer,strlen(buffer));
>    close (sockno);
>   }

Again, you are ignoring the return values, but that's not showing up as a problem
here.  However, if you look at the return value of the read() call you can tell
both how long the data is the buffer is, and also when stdin is finished (that's
normally the signal to exit the program, particularly when stdin is not the
console).

You're also making the same mistake of not terminating your string.  This is a
problem since you are using strlen() on the buffer.  The correct way of solving
both of these problems is:

    if(FD_ISSET(0,&clientset)) {
      if (len = read(0,buffer,34)) {
        sockno=socket(AF_INET,SOCK_STREAM,0);
        connect(sockno,(struct sockaddr *)&serv, sizeof(struct sockaddr));
        write(sockno,buffer,len);  /* don't use strlen() */
        close (sockno);
      } else exit(0);
    }

> *******************serv.c*******************
<snip>

>  FD_ZERO(&masterset);
>  ht=gethostbyname("SRI");
<snip>
>  FD_SET(0,&masterset);
>  FD_SET(sockfd,&masterset);
> 
>  while(1)
>  {
>    select(sockfd+1,&masterset,NULL,NULL,0);

Again, you need FD_ZERO and FD_SET inside your loop.  I also recommend using
"localhost".

>    if(FD_ISSET(0,&masterset))
>    {
>     /* CONTROL IS NEVER COMING HERE AS NOTHING IS PRINTED */
>      printf("keyboard getting ctrl");

Actually, control *is* being passed here, but nothing gets printed until the
buffer for stdout gets flushed.  :-)  When I got this working the program ran
fine, and when I exited I had a line full of "keyboard getting ctrl" flushed to
stdout.

>      wsock=socket(AF_INET,SOCK_STREAM,0);
> 
>      cli.sin_port=htons(CLIENTPORT);
>      cli.sin_family=AF_INET;
>      cli.sin_addr.s_addr=INADDR_ANY;
>      bcopy((char *)ht->h_addr,(char *)&cli.sin_addr,ht->h_length);
>      memset(&(cli.sin_zero),'\0',8);
> 
>      connect(wsock, (struct sockaddr *)&cli, sizeof(struct sockaddr));
>      read(0,buffer,34);
>      write(wsock,buffer,strlen(buffer));
>      close(wsock);
>    }

Again, you're relying on strlen() when the buffer isn't null terminated.  Your
last few lines here should be more like:

      if ((bufsize = read(0,buffer,34)) > 0)
        write(wsock,buffer,bufsize);
      else
        exit(0);
      close(wsock);

>    if(FD_ISSET(sockfd,&masterset))
>    {
>      newsockfd=accept(sockfd, (struct sockaddr *)&cli, &size);
>      read(newsockfd, buffer, 34);
>      printf("\nClient: %s\n",buffer);
>      memset(buffer,'\0',34);
>      close(newsockfd);
>    }

Finally we get to the show stopper!  This accept() call is failing, but since you
don't check the return value you don't know it.  read() then leaves the buffer
empty, so you won't see any data when you print, but you will see "Client:".

So what's wrong with the accept() call?  The value of "size" has not been set.
Here's the relevant part of the man page:

"The addrlen argument is a value-result parameter: it should
initially contain the size of the structure pointed to by
addr; on return it will contain the actual length (in bytes)
of the address returned."

So just set the value of size before calling accept.

Also, you're not null terminating the string again, which will lead to problems
trying to print the buffer.  In fact, by null terminating the string you don't
need the memset() either (which sort of null terminates for you, but it's not a
very good way to do it).  I'd recommend something more like:

    if(FD_ISSET(sockfd,&masterset)) {
      size = sizeof(struct sockaddr);
      if (-1 != (newsockfd=accept(sockfd, (struct sockaddr *)&cli, &size))) {
        if (-1 != (bufsize = read(newsockfd, buffer, 34))) {
          buffer[bufsize] = 0;
          printf("\nClient: %s\n", buffer);
        } else printf("Error receiving data\n");
        close(newsockfd);
      } else printf("Error accepting connection\n");
    }

As a parting remark, I'd should recommend the book "Unix Network Programming" by
W. Richard Stevens (as seen in the movie "Wayne's World 2"!  :-)  He describes
networking code very clearly, with useful examples.  He subsequently wrote a
series of books which elaborate on the theme that he sets in this first book, but
this is certainly the best to start with.

I hope this has helped (and it has not been too verbose to be useful).  Let me
know if I was unclear on anything.

-- 
Regards,
Paul Gearon

Software Engineer                Telephone:   +61 7 3876 2188
Plugged In Software              Fax:         +61 7 3876 4899
http://www.PIsoftware.com        PGP Key available via finger

Catapultam habeo. Nisi pecuniam omnem mihi dabis, ad caput tuum saxum
immane mittam.
(Translation from latin: "I have a catapult. Give me all the money,
or I will fling an enormous rock at your head.")