Handling serial port with select and local socket

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

• Serial port is opened and monitored by select

• Local socket is opened and put in Listen state

• inside loop I call my function accept_connection periodically

• When new connection arrives I allocate new array of struct to handle connections data

• I'm adding socket to &input with FD_SET(socket,&input), and this is what select is monitoring

• I'm checking with FD_ISSET (after select returns) if there is data from some client

• If there is data I'm reading it with process_client

It appears that it is working so far.

My questions are:

• I'm doing this the first time. Are there some obvious mistakes?

• How do I know that client has disconnected, and socket should be cleared, and removed from clients array?

• 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

On your questions:

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

-
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.

-
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 ('=', '==', '

-
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.

StackExchange Code Review Q#40039, answer score: 4