[pisa-src] Re: r1152 - in trunk/community-operator: Makefile.am co_client.c co_client.cfg co_common_packets.h co_server.c hipl.c hipl.h

  • From: Thomas Jansen <mithi@xxxxxxxxx>
  • To: pisa-src@xxxxxxxxxxxxx
  • Date: Thu, 15 Oct 2009 11:19:04 +0200

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

Other related posts: