Handling serial port with select and local socket

Question

I have another issue: I'm trying to handle connections made by local socket along with data from serial port. Here is my concept:

1. Serial port is opened and monitored by select
2. Local socket is opened and put in Listen state
3. inside loop I call my function accept_connection periodically
4. When new connection arrives I allocate new array of struct to handle connections data
5. I'm adding socket to &input with FD_SET(socket,&input), and this is what select is monitoring
6. I'm checking with FD_ISSET (after select returns) if there is data from some client
7. If there is data I'm reading it with process_client

It appears that it is working so far.

My questions are:

1. I'm doing this the first time. Are there some obvious mistakes?
2. How do I know that client has disconnected, and socket should be cleared, and removed from clients array?
3. Should I move some variables like *clients pointer clients_num outside the main function to have easier access to them from other functions, otherwise next functions can have a really long parameter list?

main.c

struct client_struct {
     int socket;
     char cmd_type;
};
void main {
int  loop_num=250;
int            local_socket;
int            serial_fd;
int            max_fd;
int            tmp_socket;
fd_set         input;
fd_set         tmp_input;
char           *serial_output_buffer;
struct timeval timeout;
struct client_struct *clients;
int    clients_num=0;

serial_output_buffer=malloc(INPUT_BYTES_NUM * sizeof(char));

serial_fd=open_port();
local_socket=open_local_socket();
FD_ZERO(&input);
FD_SET(serial_fd, &input);
max_fd = serial_fd+1;
while(TRUE) {

    loop_num++;
    if(loop_num>250) {
        pool_from_serial(serial_fd);
        loop_num=0;
    }
    tmp_socket=accept_connection();
    if(tmp_socket) {
            clients=alloc_client(clients,clients_num);
            clients[clients_num].socket=tmp_socket;
            clients_num++;
            FD_SET(tmp_socket, &input);
            max_fd = getMaxFd(serial_fd,clients,clients_num);
    }
    tmp_input=input;
    n = select(max_fd,&tmp_input,NULL,NULL,&timeout);

    /* See if there was an error */
    if (n<0)
        perror("select failed");
    else if (n == 0)
        ;//puts("TIMEOUT");
    else {
        /* We have input */
        if (FD_ISSET(serial_fd, &input)) {
            if(process_serial(serial_fd,serial_output_buffer)) {
                process_serial_data(serial_output_buffer);
            }
        }
        for(i=0;i<clients_num;i++) {
            if(FD_ISSET(clients[i].socket, &input)) {
                process_client(clients,i);
            }
        }
    }
    fflush(stdout);
    usleep(10000);
}
return 0;
}

int  getMaxFd(int serial_fd,struct client_struct *clients,int client_num) {
    int i;
    int res=serial_fd;
    for(i=0;i<client_num;i++) {
            res=(res > clients[i].socket ? res : clients[i].socket);
    }
    return (res+1);
}
struct client_struct* alloc_client(struct client_struct *clients,int num) {
    int i;
    if(num==0) {
            clients=(struct client_struct*)malloc(sizeof(struct client_struct));
    } else {
            struct client_struct *new_array=(struct client_struct*)malloc((num+1)*sizeof(struct client_struct));
            if(!new_array) {
                    error_exit("alloc_client malloc failed");
            }

            for(i=0;i<num;i++) {
                    new_array[i]=clients[i];
            }
            free(clients);
            clients=new_array;
    }
    return clients;
}

local_socket.c

int         sck_unix;                 /* Listen Socket */
struct      sockaddr_un adr_unix;/* AF_UNIX */
int         len_unix;                  /* length */

void process_client(struct client_struct *clients,int num) {
    char buf[256];
    int nread=0;
    int socket=clients[num].socket;

    int bytes=0;
    ioctl(socket, FIONREAD, &bytes);
    if(!bytes)
            return;

    nread = recv(socket, buf, sizeof(buf), 0);
    /* If error or eof, terminate. */
    if(nread < 1 && errno!=EAGAIN){
            close(socket);
            error_exit("read client");
    }
    buf[nread]='\0';
    printf("client read:%s\n",buf);
}
int accept_connection() {
    int ns=0;
    ns = accept(sck_unix, (struct sockaddr *) &adr_unix, &len_unix);
    if(ns<0) {
            if(errno==EAGAIN)
                return 0;
            else
                error_exit("accept connection failed");
    } else {
            int flagss = fcntl(ns,F_GETFL,0);
            flagss |= O_NONBLOCK;
            fcntl(ns,F_SETFL,flagss);
            printf("ns:%d\n",ns);
            return ns;
    }
}
int open_local_socket() {
    int         z;            /* Status return code */
    const char  pth_unix[]="/tmp/my_sock";      /* pathname */

    sck_unix = socket(AF_UNIX,SOCK_STREAM,0);

    if ( sck_unix == -1 )
        error_exit("Creation of local socket failed");

    unlink(pth_unix);

    memset(&adr_unix,0,sizeof adr_unix);

    adr_unix.sun_family = AF_UNIX;

    strncpy(adr_unix.sun_path,pth_unix,
        sizeof adr_unix.sun_path-1)
        [sizeof adr_unix.sun_path-1] = 0;

    len_unix = SUN_LEN(&adr_unix);
    z = bind(sck_unix,
         (struct sockaddr *)&adr_unix,
         len_unix);

   if ( z == -1 )
       error_exit("Bind to local socket failed");

   if (listen(sck_unix, 5) == -1) {
       error_exit("listen error");
   }
       int flagss = fcntl(sck_unix,F_GETFL,0);
       flagss |= O_NONBLOCK;
       fcntl(sck_unix,F_SETFL,flagss);
   return sck_unix;
}

Answer 1

On your questions:

1. There are some oddities in your code, for example, the use of non-blocking sockets.

2. Detecting disconnected clients is difficult. I'm not that familiar with UNIX-domain sockets, but I imagine they are much the same as Internet-domain sockets. You probably need either to set up periodic keep-alive messages within whatever protocol you have adopted for the client-server communication (ie. empty messages whose purpose is just to tell the server that the client is still there), or add a timeout so that a client that hasn't talked to the server for some time is assumed to be dead, or both.

3. Global variables are best avoided. Use them only as a last resort, when the alternatives are really bad. Knowing when you have reached that point is difficult, so I suggest just not using globals unless there is no alternative.

Naming:

• struct client_struct tells me no more than struct client, so call it that.

• getMaxFd uses camel case, unlike all other functions. Simply max_fd would be enough.

• giving your variables the suffix 'unix', as in adr_unix adds nothing and makes it more difficult to change the code (eg if you wanted to switch to Internet-domain sockets) than it need be.

Formatting:

You need to add spaces around operators ('=', '==', '<', etc) to make the code more readable. I also think it is nicer to add spaces after keywords ('if', 'while' etc).

And you should be consistent with bracketing - sometimes you use braces on single statements and sometimes not. Many people think it is best always to use braces, whether needed or not.

---

In getMaxFd, your use of

res=(res > clients[i].socket ? res : clients[i].socket);

is not obviously better than the more normal:

if (res < clients[i].socket) {
    res = clients[i].socket;
}

Also define loop variables within the loop

for(int i = 0; i < client_num; i++) {
    if (res < clients[i].socket) {
        res = clients[i].socket;
    }

---

Your alloc_client is verbose. Perhaps you have not come across realloc, which is effectively what you are doing.

struct client* alloc_client(struct client *clients, size_t num)
{
    struct client *c = realloc(clients, (num+1) * sizeof *c);
    if (!c) {
        error_exit("alloc_client malloc failed");
    }
    return c;
}

---

Your use of globals is unnecessary. It gains nothing but costs the reader in being less understandable.

int         sck_unix;                 /* Listen Socket */
struct      sockaddr_un adr_unix;/* AF_UNIX */
int         len_unix;                  /* length */

---

Your process_client should just take a socket not the list of structs and the index. It is also unclear to me why you need to check the socket for available data using ioctl before reading it. The function is called in response to selecting-ready so you know it can be read. And in addition you have set your socket non-blocking which again means the ioctl was unnecessary. For completeness, EINTR might also be checked as well as EAGAIN.

---

Both your accept_connection and open_local_socket set their sockets non-blocking. It is not clear to me why you need those sockets non-blocking - I don't think it is normal to do this, although it is clearly possible. Note that you should extract the duplicated three lines of code that set O_NONBLOCK into a separate function.

---

Your open_local_socket can be simplified significantly.

struct      sockaddr_un adr_unix;/* AF_UNIX */
...
const char  pth_unix[]="/tmp/my_sock";      /* pathname */
...
memset(&adr_unix,0,sizeof adr_unix);

adr_unix.sun_family = AF_UNIX;

strncpy(adr_unix.sun_path,pth_unix,
    sizeof adr_unix.sun_path-1)
    [sizeof adr_unix.sun_path-1] = 0;

Can be replaced by a local address variable adr defined as follows:

#define USOCKET "/tmp/my_sock"
    struct sockaddr_un adr = {0, AF_UNIX, USOCKET};

The avoids the horrible strncpy and termination.

The function also needs a (void) parameter list and the accepted socket should be used by the caller directly instead of via a global sck_unix.

---

In main

• define variables at their first point of use not all at the top.

• main is too long - extract the contents of the loop into a function (not including the loop_num conditional). So main will have something simple, like:

  while (process_connections(sck, serial_fd, clients, nclients)) {
      if (++loop_num>250) { ...
      }
  }
  

• the usleep call should be unnecessary and should be removed.

• You use select to detect activity on accepted sockets or the serial line, but not for the listening socket which you instead set non-blocking and poll each time round the loop. Your select call has an uninitialized timeout, which might allow the loop to reach the accept_connection call occasionally. That arrangement make no sense.

  You should add the listening socket to the input file descriptor set and handle it when select returns. And there is probably no need for a timeout on select unless there is some periodic activity that needs to be serviced (eg detecting dead clients). If you use a timeout, initialize it appropriately.

• You call pool_from_serial every 250 cycles of the loop. That seems rather arbitrary (could be 250 serial chars received or 250 clients connecting or a mixture). I don't know the purpose of this, but it might need to be more robustly scheduled.

Answer 2

So first off, global variables. Since posting this code, I made a lot of changes, signals, daemonization, logging, processing data from device, and at some point I have moved variables like:

int sigflag=0;
char dev_status=0x00;
char dev_power_status=0x00;
int serial_fd;
int local_socket;
int clients_num=0;
struct client_struct *clients;
static char const * const lock_file = "/var/lock/device/device.lock";
static char const * const log_filename = "/var/log/device.log";
FILE *logfile;
int log_level=9;

to global scope, due to multi-parameter functions that otherwise I had to made.

If I declare for example

int clients_num;
struct client_struct *clients;

inside main(), then every function called from main must have this parameters passed in arguments, which is not so bad, problem arises when next function calls next function and so on, and third function actually allocates or frees structure "client_struct" so it needs to return new pointer to structure and also updated variable clients_num, what it can't obviously do. So I ended up passing pointers to clients_num (among other pointers) so my alloc_clients actually returned new pointer for clients and used and modified *clients_num.

So to sum up, is there any alternative that I don't know about except passing and returning variables throughout 3 functions, and working with pointers like *clients_num ?

Below are some functions from approach without global variables (this is from backup, with new additions to the program some functions would have to be even longer)

 struct client_struct* process_serial_data(char *buffer,struct client_struct *clients,int *clients_num,fd_set *input);
 struct client_struct* alloc_client(struct client_struct *clients,int num);
 struct client_struct* delete_clients(struct client_struct *clients,int to_delete_arr[5],int to_delete,int clients_num);

And this is with global variables

void process_serial_data(char *buffer,fd_set *input);
void alloc_client();
void delete_clients(int to_delete_arr[5],int to_delete); //array with client index to delete, number of clients to delete

I don't want to advocate for global variables, but... at some point my code was really complicated, although safer, due to lack of risk of global variable shadowing, right?

Except for putting some variables into global scope I made also helper_function.h where I put client_struct definition and some variables for logging.Reason for that is that I divided my program into files main.c local_socket.h/c serial_port.h/c helper_function.h/c (and it will grow), so when I pass variable struct client_struct *clients to function in other files they need to know what this is client_struct.

Anyway, I;m seriously thinking about rewriting this daemon in C++, so I would have nice private variables ;) and I wouldn't have to pass variables throughout many functions.

Next thing is this NONBLOCKING sockets, I used mostly examples from the internet, but I thought about this as good idea. My program primare objective is to monitor device, and to do that it needs to query this device periodically, so I wanted to avoid situation when socket is blocked for some reason and I program doesn't pool data from device. And this is also why I used usleep in a loop, to send query over serial port in regular intervals.

Although now I think I could setup a longer timeout in pselect and wait for this timeout to query the device, and when signal arrives, or data from client, and pselect returns prematurely I could check remaining time in timeout passed to pselect and restart pselect until this timeout reaches end, right? So this way I would get rid of usleep and query device at regular intervals. I'm using pselect due to signal handling and with sigmask for sigusr1 and sigterm so they can only arrive during pselect call and not during sending/receiving data or something. Signal_handler actually only changes global variable flag that is later on checked when pselect returns.

Other issues:

• "struct client_struct tells me no more than struct client, so call it that."

  I have variable called clients that is pointer to actual data, so it can't be only "s" to distinguish between type and variable, I will stick with that.

• getMaxFd uses camel case, unlike all other functions. Simply max_fd would be enough.

  Yes, will change to get_max_fd (max_fd is my variable). Also with addr_unix You are right.

• Also define loop variables within the loop

  I had an error saying:

  ‘for’ loop initial declarations are only allowed in C99 mode

  use option -std=c99 or -std=gnu99 to compile your code

  I'm using CMake on Linux, probably some flag needs to be added to CMakeList.txt or something...

• realloc is good I didn't know about that, but still when I need to delete lets say second and fourth client from my *clients list in one go I still need to do it manually, and simply not include deleted clients in new copy like this

  void delete_clients(int to_delete_arr[5],int to_delete) {
      int i,k;
      if(clients_num==1) {
          free(clients);
          clients=NULL;
      } else {
          struct client_struct *new_array=(struct client_struct*)malloc((clients_num-1)*sizeof(struct client_struct));
          if(!new_array) {
                  error_exit(ERROR,"delete_clients malloc failed");
          }
          for(i=0;i<clients_num;i++) {
                  for(k=0;k<to_delete;k++) {
                          if(to_delete_arr[k]!=i)
                                  new_array[i]=clients[i];
                  }
          }
          free(clients);
          clients=new_array;
     }
  }
  

• process_clients takes list of structs and the index for one reason, data that arrived from serial device can be send to many clients and inside that function I'm iterating throughout list to check if any clients is waiting for that kind of response from device. In other words, when process_clients starts I don't know which client (if any) wants this data, I would have to check data type before calling process_clients

• ioctl is over the top in local_socket functions, and EINTR and EAGAIN will be checked

• open_local_socket I will simplify and it will work on local variables returning socket at the end.

• pselect should also monitor local_socket to check for new connections - right. Some "code ago" I had a problem with select returning immediately, probably due to error in local_socket creation there was no "listen" call. I saw examples with select listening for new connections, so pselect should also handle this.

• I will shorten my "main" and put some code to functions, however I'm not sure about defining variables in place where they are used, somehow this is clearer for me.

I have also other questions, but I will start new thread.