On Wed, Oct 14, 2009 at 05:22:24PM +0200, Jan Marten wrote: > -INCLUDES += -I@PISA_HIPL_SRCDIR@/libdht > +INCLUDES += -I@PISA_HIPL_SRCDIR@/opendht Diego, is this correct? I remember some bugfixes related to a change in the directory name not so long ago... > +if PISA_WITH_HIPL > +# TODO correct linking > +# pisa_split_cert function is needed in co_client.c > + co_client_LDADD += @PISA_HIPL_SRCDIR@/firewall/pisa_cert.o > +endif Not correct indeed, but I didn't have a nice solution for that problem when I talked to Jan yesterday. Still this needs to be resolved, before we can continue to create a better world (i.e. have a single makefile). Perhaps the C file could be added to the sources of the project directly? Not by copying, but by referencing the out of tree source file in @PISA_HIPL_SRCDIR@/firewall. > +/** > + * Main loop > + */ > +static void co_client_main(void) > +{ > + while(running) > + { } > +} This leads to a 100% CPU use! sleep() helps to reduce that, but still wakes up periodically. As far as I can see, you rely on signals to trigger your events. In theory a select on a bunch of NULL parameters should sleep indefinitely without causing that load on the CPU and still allow signals to be processed. This infinite loop is unacceptable from a performance point of view. > +/** > + * Terminate community-operator client by receiving signal > + * @param signal signal quit code > + */ > +static void co_client_quit(int signal) > +{ > + running = 0; > + PISA_INFO("Shutting down Community-Operator client...\n"); > + pisa_cfg_cleanup(); > +} Why is there no call to exit()? The function name suggests that we quit immediately, especially as you use this as a signal handler, see below. > + signal(SIGTERM, co_client_quit); > + signal(SIGINT, co_client_quit); > + signal(SIGQUIT, co_client_quit); > + signal(SIGILL, co_client_quit); > + signal(SIGHUP, co_client_quit); > + signal(SIGBUS, co_client_quit); > + signal(SIGSEGV, co_client_quit); ... > + co_client_quit(0); > + exit(EXIT_SUCCESS); > + return 0; I was tempted to praise triple redundancy for safety at this point before I noticed that co_client_quit doesn't really do what the name seems to promise. It somehow doesn't seem correct to have a signal handler for SIGSEGV return to normal execution and act as if nothing happened. Then again: why have those signal handlers at all? Even so the last two lines are redundant, exit(EXIT_SUCCESS) quits the program with a result of 0 just as return 0 does in the main() function. Some comments on style: I appreciate that you changed my quick-n-dirty CamelCaseNotation to the appropriate underscore_notation, but why did you replace tab indentation with spaces? It is inconsistent with virtually every other line of C code in the repository and uses four times as many characters to achieve the same as a tab would do. I know the rule in doc/HACKING states "Use 4 spaces as TAB in userspace and 8 spaces ad TAB in kernelspace!" and for me that translates to "set your editor to a tab width of 4 or 8". I admit though, that it is easy to misunderstand the way that the rule is written. K&R style has been mentioned by Diego already. Please don't get me wrong, I'm not trying to enforce my personal beliefs of how code should be formatted, but I'm aiming for consistency throughout the PISA project. What Diego also mentioned: This commit is far too big. You should have splitted that up, perferrably along the way of development, not at the end. It's not only better for the commit log but also for your own safety: That way you would have been able to pinpoint a bug to a specific change rather than to one big summary. It would also have been easier to go back to a certain point in your personal development history if you needed to. -- Thomas Jansen, "Mithi" --- mithi@xxxxxxxxx GPG 9D5C682B, feel free to sign or encrypt your mail