Socket arbitrarily connects - or doesnt

Question

I'm working on a university project, in which I have to connect a raspberry pi to an Android smartphone to control 2 motors. We are new to socket programming, so we started out with an example we found on wikibooks and tried to modify in to our needs. We're now facing the problem, that the connection between server and client is very arbitrary and unstable, sometimes connecting, and after a brief disconnect doesnt connect again. The weird thing (for me) is, that after we edit the code above the part responsible for connection:

 /* bind serv information to mysocket */
bind(mysocket, (struct sockaddr *)&serv, sizeof(struct sockaddr));

/* start listening, allowing a queue of up to 2 pending connection */
listen(mysocket, 2);
consocket = accept(mysocket, (struct sockaddr *)&dest, &socksize);

like inserting in a printf, the next time we launch the programm, everthing does work, sometimes two or three times, and then it just stops connecting.

I've searched all over google and so for a similar problem, but I haven't found an equivalent, so I turn to you directly now.

This is code for our server running on the raspberry pi, which also serves as a network hotspot:

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

#define PORTNUM 5298
#define MAXRCVLEN 1000

#define PIN9 RPI_GPIO_P1_21
#define PIN10 RPI_GPIO_P1_19
#define PIN11 RPI_GPIO_P1_23
#define PIN22 RPI_GPIO_P1_15


int setpins();
int forward();
int backward();

int main(int argc, char *argv[])
{
char msg[] = "Connected!\n";
char testchar[] = "stillthere?";
char quitstring[] = "quit";   
char *recbuf; 

int qflag = 0;
int lflag = 0;
int mysocket, consocket, len;       /* socket used to listen for incoming connections */

struct sockaddr_in dest;        /* socket info about the machine connecting to us */
struct sockaddr_in serv;        /* socket info about our server */

socklen_t socksize = sizeof(struct sockaddr_in);


memset(&serv, 0, sizeof(serv));           /* zero the struct before filling the fields */
serv.sin_family = AF_INET;                /* set the type of connection to TCP/IP */
serv.sin_addr.s_addr = htonl(INADDR_ANY); /* set our address to any interface */
serv.sin_port = htons(PORTNUM);           /* set the server port number */

mysocket = socket(AF_INET, SOCK_STREAM, 0);

/* bind serv information to mysocket */
bind(mysocket, (struct sockaddr *)&serv, sizeof(struct sockaddr));
/* start listening, allowing a queue of up to 2 pending connection */
listen(mysocket, 2);
consocket = accept(mysocket, (struct sockaddr *)&dest, &socksize);


if (!bcm2835_init())   return 1;

setpins();
while(consocket)
{
    printf("Incoming connection from %s - sending welcome\n", inet_ntoa(dest.sin_addr));
    send(consocket, msg, strlen(msg), 0);

    while (!qflag && !lflag) {

        // Do something when connection is lost: SO_KEEPALIVE?
    // if (!send(consocket,testchar, strlen(testchar), 0)) lflag = 1;
            recbuf = malloc (MAXRCVLEN+1);
            len = recv(consocket, recbuf, MAXRCVLEN, 0);
            recbuf[len] = '\0';

    if (len > 0) printf("Client sent %s (%d bytes). \n", recbuf, len);
            if (recbuf[0] == 'v') forward(); // this function lets our car drive forward
            if (recbuf[0] == 'r') backward();// this one backwards ;)
    // Leave this loop if the client sends you the quitstring
            if (!strcmp (recbuf, quitstring))     qflag = 1;

    free(recbuf);
    }   

if (qflag) break;
    listen(mysocket, 1);
    consocket = accept(mysocket, (struct sockaddr *)&dest, &socksize);
}

close(consocket);
close(mysocket);
printf("sockets closed\n");
return EXIT_SUCCESS;
}

One line in there

 // if (!send(consocket,testchar, strlen(testchar), 0)) lflag = 1;

is our idea to test wether the connection is still up, is this viable?

And this is the client code, thats not in Java yet but in C:

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

#define MAXRCVLEN 500
#define PORTNUM 5298

int main(int argc, char *argv[])
{
   char buffer[MAXRCVLEN + 1]; /* +1 so we can add null terminator */
   int len, mysocket;
   struct sockaddr_in dest; 

   mysocket = socket(AF_INET, SOCK_STREAM, 0);

   memset(&dest, 0, sizeof(dest));                /* zero the struct */
   dest.sin_family = AF_INET;
   dest.sin_addr.s_addr = inet_addr("192.168.42.1"); /* set destination IP number */ 
   dest.sin_port = htons(PORTNUM);                /* set destination port number */

   do {
    connect(mysocket, (struct sockaddr *)&dest, sizeof(struct sockaddr));
    len = recv(mysocket, buffer, MAXRCVLEN, 0);
   }while(len < 0);
   /* We have to null terminate the received data ourselves */
   buffer[len] = '\0';

   // Received
   printf("Received %s (%d bytes).\n", buffer, len);

   // send:
   char msg[] = " ";
    do{
    scanf("%s",msg);    
    printf("Sending Msg to %s \n", inet_ntoa(dest.sin_addr));
    send( mysocket, msg, strlen(msg),0);
    }while (strcmp(msg,"quit"));


   close(mysocket);
   return EXIT_SUCCESS;
}

Any ideas what we did wrong?

Thanks in advance!

Answer 1

Unless what you actually, really want to learn is low-level berkeley socket manipulation, I'd suggest you look at libevent or a similar library.

The structure of your main loop is a little unusual. You can clearly only handle one connection at a time, and you don't cope well with any connection attempts that happened while you were servicing a previous connection.

bind(mysocket, (struct sockaddr *)&serv, sizeof(struct sockaddr));

bind can fail, e.g. if another process has recently had the socket open and the OS hasn't finished cleaning up use of the port. You can change this behavior, but you should still check, from die.net's bind manpage

Return Value
  On success, zero is returned. On error, -1 is returned, and errno is set appropriately.

so

if(bind(mysocket, (struct sockaddr *)&serv, sizeof(struct sockaddr))) {
    perror("bind failed");
    exit(1);
}

listen() only needs to be called once, but also needs to be checked

if(listen(mysocket, 2)) {
    perror("listen failed");
    exit(1);
}

after this, if you are content to do the single-service approach, then you can do the following:

mysocket = socket(AF_INET, SOCK_STREAM, 0);
if(mysocket < 0) {
    perror("socket failed");
    exit(1);
}

if(bind(mysocket, (struct sockaddr *)&serv, sizeof(struct sockaddr))) {
    perror("bind failed");
    exit(1);
}

if(listen(mysocket, 2)) {
    perror("listen failed");
    exit(1);
}

for (;;) {
    consocket = accept(mysocket, (struct sockaddr *)&dest, &socksize);
    if(consocket < 0) // might return if the connection has already gone away.
        continue;
    if (!sendGreeting(consocket)) {
        // sendGreeting should return -1 if it was unable to send, 0 if successful
        while (!readLoop(consocket, recvBuf, MAXRCVLEN))
            ;
    }
    close(consocket);
}

readLoop would then be something like:

int readLoop(int socket, char* buffer, size_t bufSize) {
    int len = recv(socket, buffer, bufSize);
    if (len > 0)
        return processBuffer(socket, buffer, len);
    if (len < 0 && (errno == EINTR || errno == EAGAIN))
        return 0; // do-over
    return -1;
}

make sure that processBuffer also returns 0 or -1 accordingly.

As I mentioned above, there are still problems with this approach, but it's not my intent here to teach you everything you need to know about sockets in one pass :) If you want to further develop your socket knowledge, your next stop should be learning about select or poll with non-blocking sockets so that you can host multiple sockets and service them as they become active.

Answer 2

This line code

char msg[] = " ";
do{
  scanf("%s",msg);   

will fail miserably if the number of bytes scanned in is larger then 1 character, as msg provides exactly two bytes (from which one is always used as 0-terminator). Feeding more would write out of the bounds of msg and doing so will provoke undefined behaviuor.

To fix this providing at least a minimum of 255 characters to so:

char msg[256] = "";
do{
  scanf("%255s",msg);   

Answer 3

Generally, you should use tcpdump/wireshark to see what packets are seen by you Rpi, and strace to see what your program does. My first guess about your connections sometimes not working would be loss of packets. By using wired LAN (Ethernet), you could rule this possibility out.

But the example server code that you're using is a rather bad example. Even if you only want to accept a single client connection at a time, your server should not use blocking waits for any remote message. You should read about using non-blocking I/O, select or poll, and look at examples using these. Also, please read about SO_REUSEADDR, you probably need that one in your server as well.