From 9778aa83e78f7273bd2ffe05e0dce72f270e67b9 Mon Sep 17 00:00:00 2001 From: gumartinm Date: Mon, 30 Jan 2012 23:47:56 +0100 Subject: [PATCH] Clening up code, and errors control. TODO: stop using XML, I am going to use my own protocol for sending stdout, stderr and return status code in this way I will avoid the shared memory and the code complexity. Besides it should reduce the memory usage. Next weekend more. --- Daemon/Makefile | 2 +- Daemon/javafork.c | 264 +++++++++++++-------- Daemon/javafork.h | 1 + .../src/main/java/de/fork/java/TCPForkDaemon.java | 5 +- .../src/main/java/de/fork/java/XmlForkParser.java | 28 +-- 5 files changed, 177 insertions(+), 123 deletions(-) diff --git a/Daemon/Makefile b/Daemon/Makefile index d785678..7c28c45 100644 --- a/Daemon/Makefile +++ b/Daemon/Makefile @@ -1,7 +1,7 @@ all: javafork javafork: javafork.c javafork.h - gcc -Wall -g -o javafork javafork.c -lpthread -D_GNU_SOURCE + gcc -Wall -g -o javafork javafork.c -lpthread clean: rm -f javafork diff --git a/Daemon/javafork.c b/Daemon/javafork.c index 429ecfe..81063c4 100644 --- a/Daemon/javafork.c +++ b/Daemon/javafork.c @@ -1,3 +1,10 @@ +#define _GNU_SOURCE + +/* Beaware: this program uses GNU extensions (the TEMP_FAILURE_RETRY macro) + * I am writing (non-)portable code because I am running out of time + * TODO: my portable macro with the same funcionality + */ + #include #include #include @@ -24,15 +31,10 @@ #include "javafork.h" -pid_t daemonPID; /*Stores the daemon server PID*/ -int sockfd = -1; /*Stores the daemon server TCP socket.*/ - +pid_t daemonPID; /*Stores the daemon server PID*/ +int sockfd = -1; /*Stores the daemon server TCP socket.*/ -static int restartableClose(int fd) -{ - return TEMP_FAILURE_RETRY(close(fd)); -} @@ -40,7 +42,7 @@ static int closeSafely(int fd) { /*If we always initialize file descriptor variables with value -1*/ /*this method should work like a charm*/ - return (fd == -1) ? 0 : restartableClose(fd); + return (fd == -1) ? 0 : TEMP_FAILURE_RETRY(close(fd)); } @@ -52,6 +54,7 @@ int main (int argc, char *argv[]) char *avalue = IPADDRESS; /*Address: numeric value or hostname*/ int pvalue = PORT; /*TCP port*/ int qvalue = QUEUE; /*TCP listen queue*/ + struct sigaction sa; /*sig actions values*/ /*This process is intended to be used as a daemon, it sould be launched by the INIT process, because of that*/ @@ -61,11 +64,7 @@ int main (int argc, char *argv[]) /*Changing session.*/ setsid(); - - if (signal(SIGPIPE,SIG_IGN) == SIG_ERR) { - syslog (LOG_ERR, "signal SIGPIPE desactivation failed: %m"); - return 1; - } + opterr = 0; while ((c = getopt (argc, argv, "a:p:q:")) != -1) { @@ -102,12 +101,34 @@ int main (int argc, char *argv[]) return 1; } - /*INIT process sending SIGINT? Should I catch that signal?*/ - daemonPID = getpid(); - if (signal(SIGINT, sigint_handler) == SIG_ERR) { - syslog (LOG_ERR, "SIGTERM signal handler failed: %m"); - return 1; - } + + /* From man sigaction(2): + * A child created via fork(2) inherits a copy of its parent's signal dispositions. + * During an execve(2), the dispositions of handled signals are reset to the default; the + * dispositions of ignored signals are left unchanged. + * I want to ignore SIGCHLD without causing any issue to child processes. + */ + memset (&sa, 0, sizeof(sa)); + /*SIG_DFL: by default SIGCHLD is ignored.*/ + sa.sa_handler = SIG_DFL; + sigemptyset(&sa.sa_mask); + sa.sa_flags = SA_NOCLDSTOP | SA_RESTART; + if (sigaction(SIGCHLD, &sa, NULL) < 0) { + syslog (LOG_ERR, "SIGCHLD signal handler failed: %m"); + return 1; + } + + + /*INIT process sending SIGINT? Should I catch that signal?*/ + daemonPID = getpid(); + memset (&sa, 0, sizeof(sa)); + sa.sa_handler = &sigint_handler; + sigemptyset(&sa.sa_mask); + if (sigaction(SIGINT, &sa, NULL) < 0) { + syslog (LOG_ERR, "SIGINT signal handler failed: %m"); + return 1; + } + if (main_daemon (avalue, pvalue, qvalue) < 0) return 1; @@ -168,7 +189,7 @@ int main_daemon (char *address, int port, int queue) goto err; } - while(1) { + for(;;) { clilen = sizeof(addr_client); if ((sockclient = TEMP_FAILURE_RETRY(accept (sockfd, (struct sockaddr *) &addr_client, &clilen))) < 0) { syslog (LOG_ERR, "socket accept failed: %m"); @@ -236,12 +257,13 @@ int daemonize(const char *pname, int facility, int option) void *serverThread (void * arg) { - int socket = -1; /*Open socket by the Java client*/ - long timeout, utimeout; /*Timeout for reading data from client: secs and usecs respectively*/ - int len; /*Control parameter used while receiving data from the client*/ - char buffer[1025]; /*This buffer is intended to store the data received from the client*/ - char *command = NULL; /*The command sent by the client, to be executed by this process*/ - uint32_t *commandLength = NULL; /*Store the command length*/ + int socket = -1; /*Open socket by the Java client*/ + long timeout, utimeout; /*Timeout for reading data from client: secs and usecs*/ + /*respectively*/ + int len; /*Control parameter used while receiving data from the client*/ + char buffer[1025]; /*This buffer is intended to store the data received from the client*/ + char *command = NULL; /*The command sent by the client, to be executed by this process*/ + uint32_t *commandLength = NULL; /*Store the command length*/ socket = (int) arg; @@ -350,10 +372,10 @@ int required_sock_options (int socket) return -1; } - /*TODO: keepalive is not enough to find out if the connection is broken */ - /* apparently it just works while the handshake phase. See: */ - /* http://stackoverflow.com/questions/4345415/socket-detect-connection-is-lost */ - /* I have to implement an echo/ping messages protocol (application layer) */ + /*TODO: keepalive is not enough to find out if the connection is broken */ + /* apparently it just works while the handshake phase. See: */ + /* http://stackoverflow.com/questions/4345415/socket-detect-connection-is-lost */ + /* I have to implement an echo/ping messages protocol (application layer) */ return 0; } @@ -402,7 +424,8 @@ int readable (int socket, char *data, int len, int flags) syslog (LOG_INFO, "read TCP socket spurious readiness"); } } else if (received == 0) { - /*if nData is 0, client closed connection but we wanted to receive more data, this is an error */ + /*if nData is 0, client closed connection but we wanted to receive more data, */ + /*this is an error */ syslog (LOG_ERR, "expected more data, closed connection from client"); return -1; } @@ -432,6 +455,7 @@ int receive_from_socket (int socket, char *data, int len, long timeout, long uti } + int pre_fork_system(int socket, char *command) { /*Required variables in order to share memory between processes*/ @@ -444,17 +468,20 @@ int pre_fork_system(int socket, char *command) /*Required variables in order to share the semaphore between processes*/ key_t keysemaphore; int idsemaphore = -1; - sem_t *semaphore = NULL; /*Used as a barrier: the child process just can start after sending the XML init code*/ - - int returnValue = -1; /*Return value from this function can be caught by upper layers, NOK by default*/ + sem_t *semaphore = NULL; /*Used as a barrier: the child process just can start after */ + /*sending the XML init code*/ + int returnValue = -1; /*Return value from this function can be caught by upper*/ + /*layers, NOK by default*/ - /*Allocate shared memory because we can not use named semaphores*/ - /*We are using this semaphore as a barrier, because we just want to start the child process when the parent process has sent*/ - /*the XML header (see: fork_system function)*/ - - keysemaphore=ftok("/bin/ls", SHAREMEMSEM); /*the /bin/ls must exist otherwise this does not work... */ + /* Allocate shared memory because we can not use named semaphores + * We are using this semaphore as a barrier, because we just want to start the child process + * when the parent process has sent the XML header (see: fork_system function) + */ + + /*the /bin/ls must exist otherwise this does not work... */ + keysemaphore=ftok("/bin/ls", SHAREMEMSEM); if (keysemaphore == -1) { syslog (LOG_ERR, "ftok failed: %m"); goto end; @@ -482,15 +509,20 @@ int pre_fork_system(int socket, char *command) - /*Allocate shared memory for the return status code from the process which is going to be launched by the system function.*/ - /*We want to share the returnstatus variable between this process and the child that is going to be created in the fork_system method.*/ - /*The goal is to store in this variable the return status code received from the process launched with the system method by the child process,*/ - /*then the parent process can retrieve that return status code and send it by TCP to the Java client.*/ - /*There are not concurrency issues because the parent process will just try to read this variable when the child process is dead, taking*/ - /*in that moment its last value and sending it to the Java client.*/ - - - keyvalue=ftok("/bin/ls", SHAREMEMKEY); /*the /bin/ls must exist otherwise this does not work... */ + /* Allocate shared memory for the return status code from the process which is + * going to be launched by the system function. We want to share the returnstatus + * variable between this process and the child that is going to be created in the + * fork_system method. + * The goal is to store in this variable the return status code received from the + * process launched with the system method by the child process, then the parent + * process can retrieve that return status code and send it by TCP to the Java client. + * There are not concurrency issues because the parent process will just try to read + * this variable when the child process is dead, taking in that moment its last value + * and sending it to the Java client. + */ + + /*the /bin/ls must exist otherwise this does not work... */ + keyvalue=ftok("/bin/ls", SHAREMEMKEY); if (keyvalue == -1) { syslog (LOG_ERR, "ftok failed: %m"); goto end_destroy_sem; @@ -545,16 +577,16 @@ end: int fork_system(int socket, char *command, sem_t *semaphore, int *returnstatus) { - int pid; - int out[2], err[2]; /*Store pipes file descriptors. Write ends attached to the stdout and stderr streams.*/ + int pid; /*Child or parent PID.*/ + int out[2], err[2]; /*Store pipes file descriptors. Write ends attached to the stdout*/ + /*and stderr streams.*/ char buf[2000]; /*Read data buffer.*/ - char string[100]; - /*We are going to use a poll in order to find out if there are data coming from the*/ - /*pipes attached to the stdout and stderr streams.*/ - struct pollfd polls[2]; - int n; + char string[3000]; + struct pollfd polls[2]; /*pipes attached to the stdout and stderr streams.*/ + int n; /*characters number from stdout and stderr*/ int childreturnstatus; - int returnValue = 0; /*return value from this function can be caught by upper layers, OK by default*/ + int returnValue = 0; /*return value from this function can be caught by upper layers,*/ + /*OK by default*/ /*Value by default*/ @@ -564,7 +596,7 @@ int fork_system(int socket, char *command, sem_t *semaphore, int *returnstatus) out[0] = out[1] = err[0] = err[1] = -1; - /*Creating the pipes, they will be attached to the stderr and stdout streams*/ + /*Creating pipes, they will be attached to the stderr and stdout streams*/ if (pipe(out) < 0 || pipe(err) < 0) { syslog (LOG_ERR, "pipe failed: %m"); goto err; @@ -590,7 +622,7 @@ int fork_system(int socket, char *command, sem_t *semaphore, int *returnstatus) exit(-1); } - /*TODO: I should use execve with setlocale instead of system.*/ + /*TODO: I should use execve with setlocale and the environment instead of system.*/ /*During execution of the command, SIGCHLD will be blocked, and SIGINT and SIGQUIT*/ /* will be ignored. From man system(3)*/ *returnstatus=system(command); @@ -607,68 +639,99 @@ int fork_system(int socket, char *command, sem_t *semaphore, int *returnstatus) polls[0].fd=out[0]; polls[1].fd=err[0]; polls[0].events = polls[1].events = POLLIN; - sprintf(string,""); - send(socket,string,strlen(string),0); - sprintf(string,""); - send(socket,string,strlen(string),0); + + bzero(string, sizeof(string)); + /*TODO: stop using XML. Next improvements: my own client/server protocol*/ + sprintf(string,""); + + if (TEMP_FAILURE_RETRY(send(socket,string,strlen(string),0)) < 0) { + syslog (LOG_INFO, "error while sending xml header: %m"); + + if (kill(pid, SIGKILL /*should I use SIGTERM and my own handler?*/) < 0) { + /*We are not sure if the child process will die. In this case, probably the child */ + /*process is going to be an orphan and its system process (if there is one) as well*/ + syslog (LOG_ERR, "error while killing child process: %m"); + goto err; + } + + if (TEMP_FAILURE_RETRY(waitpid(pid, NULL, 0)) < 0) { + /*We are not sure if the child process is dead. In this case, probably the child */ + /*process is going to be an orphan and its system process (if there is one) as well*/ + syslog (LOG_ERR, "error while waiting for killed child process: %m"); + } + + /*In Java the client will get a XMLParser Exception.*/ + goto err; + } /*Releasing barrier, the child process can keep running*/ if (sem_post(semaphore) < 0 ) { - syslog (LOG_ERR, "parent error releasing barrier: %m"); - /*TODO: SIGTERM and SIGCHLD by default are ignored (interactive bash) */ /*if the child process launched the system command the child process will die */ /*and the system process is going to be an orphan process... :( */ + syslog (LOG_ERR, "parent error releasing barrier: %m"); + if (kill(pid, SIGKILL /*should I use SIGTERM and my own handler?*/) < 0) { + /*We are not sure if the child process will die. In this case, probably the child */ + /*process is going to be an orphan and its system process (if there is one) as well*/ syslog (LOG_ERR, "error while killing child process: %m"); - /*This is the worst that could happen, we are not sure if the child process is going to die */ - /*in this case, probably the child process is going to be an orphan and its system process ass welll*/ - /*goto err?*/ + goto err; + } + + if (TEMP_FAILURE_RETRY(waitpid(pid, NULL, 0)) < 0) { + /*We are not sure if the child process is dead. In this case, probably the child */ + /*process is going to be an orphan and its system process (if there is one) as well*/ + syslog (LOG_ERR, "error while waiting for killed child process: %m"); } - /*TODO: sigprocmask(SIG_BLOCK, SIGCHLD, NULL) at the beginning of this whole program */ - /*from bash it makes a clone and this parent process inherits the bash signal handlers */ - /*but not its signal masks. we want to block until child process is dead */ - /*because of that we must block SIGCHLD.*/ - waitpid(pid, NULL, 0); - /*goto err?*/; + + /*In Java the client will get a XMLParser Exception.*/ + goto err; } while(1) { if(poll(polls,2,100)) { if(polls[0].revents && POLLIN) { bzero(buf,2000); - n=read(out[0],buf,1990); - sprintf(string,""); - send(socket,string,strlen(string),0); + bzero(string, sizeof(string)); + n=TEMP_FAILURE_RETRY(read(out[0],buf,1990)); + sprintf(string,"", buf); + if (TEMP_FAILURE_RETRY(send(socket,string,strlen(string),0)) < 0) + syslog (LOG_INFO, "error while sending stdout: %m"); } if(polls[1].revents && POLLIN) { bzero(buf,2000); - n=read(err[0],buf,1990); - sprintf(string,""); - send(socket,string,strlen(string),0); + bzero(string, sizeof(string)); + n=TEMP_FAILURE_RETRY(read(err[0],buf,1990)); + sprintf(string,"", buf); + if (TEMP_FAILURE_RETRY(send(socket,string,strlen(string),0)) < 0) + syslog (LOG_INFO, "error while sending stderr: %m"); } if(!(polls[0].revents && POLLIN) && !(polls[1].revents && POLLIN)) { syslog (LOG_ERR, "parent error polling pipes: %m"); - /*TODO: Should I implement a SIGCHILD handler? */ - /*TODO: Should I have a SIGTERM handler in the child process? */ - kill(pid, SIGTERM); - /*I want to send an error status to the remote calling process */ - /*TODO: Before giving this value I should make sure the child process is dead */ - /* otherwise I could finish having in *returnstatus the value from the child process */ - (*returnstatus) = -1; - break; + + if (kill(pid, SIGKILL /*should I use SIGTERM and my own handler?*/) < 0) { + /*We are not sure if the child process will die. In this case, */ + /*probably the child process is going to be an orphan and its */ + /*system process (if there is one) as well*/ + syslog (LOG_ERR, "error while killing child process: %m"); + goto err; + } + + if (TEMP_FAILURE_RETRY(waitpid(pid, NULL, 0)) < 0) { + /*We are not sure if the child process is dead. In this case, */ + /*probably the child process is going to be an orphan and its */ + /*system process (if there is one) as well*/ + syslog (LOG_ERR, "error while waiting for killed child process: %m"); + } + + /*In Java the client will get a XMLParser Exception.*/ + goto err; } } else { /*When timeout*/ - if(waitpid(pid, &childreturnstatus, WNOHANG)) { + if(TEMP_FAILURE_RETRY(waitpid(pid, &childreturnstatus, WNOHANG))) { /*Child is dead, we can finish the connection*/ /*First of all, we check the exit status of our child process*/ /*In case of error send an error status to the remote calling process*/ @@ -681,11 +744,10 @@ int fork_system(int socket, char *command, sem_t *semaphore, int *returnstatus) } } /*Reaching this code when child finished or if error while polling pipes*/ - sprintf(string,"", (*returnstatus)); - send(socket,string,strlen(string),0); - sprintf(string,""); - send(socket,string,strlen(string),0); - + bzero(string, sizeof(string)); + sprintf(string,"", (*returnstatus)); + if (TEMP_FAILURE_RETRY(send(socket,string,strlen(string),0)) < 0) + syslog (LOG_INFO, "error while sending return status: %m"); /*Stuff just done by the Parent process. The child process ends with exit*/ end: @@ -707,10 +769,8 @@ void sigint_handler(int sig) return; } - if (signal (SIGINT, SIG_IGN) == SIG_ERR) - syslog (LOG_ERR, "signal SIGINT desactivation failed: %m"); closeSafely (sockfd); - /*TODO: kill child processes and release allocate memory*/ + /*TODO: kill child processes, finish threads and release allocate memory*/ exit (0); } diff --git a/Daemon/javafork.h b/Daemon/javafork.h index 20af188..1ae1eac 100644 --- a/Daemon/javafork.h +++ b/Daemon/javafork.h @@ -9,6 +9,7 @@ + /****************************************************************************************/ /* This method is used by pthread_create */ /* */ diff --git a/JavaExample/javafork-example/src/main/java/de/fork/java/TCPForkDaemon.java b/JavaExample/javafork-example/src/main/java/de/fork/java/TCPForkDaemon.java index c3f08cf..343e734 100644 --- a/JavaExample/javafork-example/src/main/java/de/fork/java/TCPForkDaemon.java +++ b/JavaExample/javafork-example/src/main/java/de/fork/java/TCPForkDaemon.java @@ -104,7 +104,8 @@ public class TCPForkDaemon { /* */ /******************************************************************************************/ - + + //SocketChannel socketNIO = new SocketChannelImpl(); socket = new Socket(InetAddress.getByName(host), port); try { @@ -139,7 +140,7 @@ public class TCPForkDaemon { // the only way to fix this issue in Java is sending ping messages (Could we fix it using custom settings in the OS // of the client and server machines? for example in Linux see /proc/sys/net/ipv4/) InputSource inputSource = new InputSource(socket.getInputStream()); - //Must be used the remote locale character set encoding? + //It must be used the remote locale character set encoding? inputSource.setEncoding("UTF-8"); parser.setStream(socket.getInputStream()); diff --git a/JavaExample/javafork-example/src/main/java/de/fork/java/XmlForkParser.java b/JavaExample/javafork-example/src/main/java/de/fork/java/XmlForkParser.java index 255cb0f..6e12a2a 100644 --- a/JavaExample/javafork-example/src/main/java/de/fork/java/XmlForkParser.java +++ b/JavaExample/javafork-example/src/main/java/de/fork/java/XmlForkParser.java @@ -42,8 +42,10 @@ import org.xml.sax.ext.DefaultHandler2; public class XmlForkParser extends DefaultHandler2 { private static final Logger logger = Logger.getLogger(XmlForkParser.class); private StringBuffer accumulator = new StringBuffer(); - private String stderr = new String(); - private String stdout = new String(); + private StringBuilder stderr = new StringBuilder(); + private String lastStderr; + private StringBuilder stdout = new StringBuilder(); + private String lastStdout; private String returnCode = new String(); final SAXParserFactory spf = SAXParserFactory.newInstance(); private final SAXParser saxParser; @@ -70,10 +72,10 @@ public class XmlForkParser extends DefaultHandler2 { public void endElement (final String uri, final String localName, final String qName) { if (qName.equals("error")) { // After , we've got the stderror - stderr = stderr + accumulator.toString(); + stderr = stderr.append(accumulator.toString()); } else if (qName.equals("out")) { // After , we've got the stdout - stdout = stdout + accumulator.toString(); + stdout = stdout.append(accumulator.toString()); } else if (qName.equals("ret")) { returnCode = returnCode + accumulator.toString(); } @@ -91,20 +93,10 @@ public class XmlForkParser extends DefaultHandler2 { public void endDocument () throws SAXException { if (stderr.length() != 0) { - String lastStderr = stderr.replaceFirst("\\\n$", ""); - stderr = lastStderr; - } - else { - //if there is nothing from the stderr stream - stderr = null; + lastStderr = stderr.toString().replaceFirst("\\\n$", ""); } if (stdout.length() != 0) { - String lastStdout = stdout.replaceFirst("\\\n$", ""); - stdout = lastStdout; - } - else { - //if there is nothing from the stdout stream - stdout = null; + lastStdout = stdout.toString().replaceFirst("\\\n$", ""); } } @@ -135,7 +127,7 @@ public class XmlForkParser extends DefaultHandler2 { * @return stderr */ public String getStderr() { - return stderr; + return lastStderr; } @@ -148,7 +140,7 @@ public class XmlForkParser extends DefaultHandler2 { * @return stdout */ public String getStdout() { - return stdout; + return lastStdout; } -- 2.1.4