Return to Answer

• 35.2k
• 13
• 134
• 238

• 303
• 1
• 3
• 7

Thanks for your detailed answer.

So first of, 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.

Thanks for Your help, I have also other questions, but I will start new thread.

Marek