[DSM-devel] dsme code cleanup, comments before merging?

[Lars Wirzenius]

su, 2006-08-13 kello 22:52 +0300, sampsa.fabritius at nokia.com kirjoitti:
> Could you Ismo and David take a moment and comment these if you
> haven't done it yet. 
> Anyhow, let's try to keep discussion in this mailing list :)

Sorry for the delay in my reply. After I'd sent my mail, and didn't get
a reply for a while, I went ahead and merged my changes to the trunk.
After all, it's easy enough to revert changes in Subversion. Then I
continued and did further changes.

> > Additional remarks for discussion:
> > 
> > * Error handling needs to become robust, no core dumping on broken
> >   connections (dsmetool), and at the very least, all memory
> >   allocations should be checked (strdup often isn't, now). Related
> >   to this, a decision on what to do upon these errors should happen:
> >   do we try to continue even though there is no memory? Or do we
> >   simply die, after trying to report the error? How critical is it
> >   for dsme to never die?

This is still open, and should be discussed. If dsme runs out of memory
(malloc fails), there is not a whole lot it can do anymore. On the other
hand, if it is considered critical enough, it should still try to run,
in the hope that it can perform some useful service. On the third hand,
if memory has run out because dsme itself has grown very big, it might
be possible to recover by having dsme terminate and restart itself.

> > * Currently, a loop around select and dsme_receive and related
> >   things is implemented in many place (dsme.c, dsmetool.c,
> >   dsmetool.c again, probably others). Would a simple "main loop"
> >   function, or other suitable abstraction, in libdsme be
> >   appropriate? Should be simple enough to make it generic enough, I
> >   think, and would make it simpler to write client end stuff
> >   (dsmetool, and other similar tools).

I've since come to the conlusion that this should be done, but haven't
yet implemented it.

> > * I suspect that there's a few extra #includes for system headers,
> >   such as <malloc.h> (<stdlib.h> is the correct one). These should
> >   be cleaned up at some point.

This is harmless, but should be done before a public release, just to
avoid unnecessary embarrassment.

> > * dsme.c: function daemonize: is setting stdout/err to /dev/console
> >   entirely appropriate in the general case?

This needs a comment from someone. I think the behavior is wrong, but
I'm not sure I'm right.

> > * lifeguard.c uses alloca; this is non-standard, and should be
> >   simple enough to be rewritten to use malloc instead.

This is still unfixed, but I'll do that soon.

> > * dsme should decide on a prefix (I suggest dsme_ and DSME_) and use
> >   that on all publically visible identifiers in a totally systematic
> >   way. At the moment, this is not quite the case, and sometimes the
> >   prefix is just a plain "dsme" without the underscore. Also, dsme
> >   should never define identifiers that start with two underscores,
> >   or one underscore and a capital letter (happens now in some
> >   headers to protect against multiple inclusion).

This is also part of the cleanup that needs to happen before a public
release.

> > * dsmesock.h should just use the lists.c/h lists, instead of
> >   duplicating code

More cleanup.

> > * dsmesock.h: is the empty comment at end useful in some way that is
> >   not obvious?

More cleanup.

> > * I'd like to remove spaces following pointer dereferencing
> >   asterisks ("void *foo", instead of "void * foo", for example).

Nitpicking, I know. Not particularly important.

> > * lists.h: document dsme_list_compare_fn_t

Done. I've added all missing documentation comments that doxygen
complained about.

> > * lists: how should the head element be allocated? who frees it? is
> >   it necessary at all? what happens if the caller removes the head?
> >   all this needs to be documentd. I do find the extra head element a
> >   bit confusing, and I think it would be possible to do without out,
> >   but haven't yet looked at it, since I didn't want to change
> >   existing interfaces on my own.

I haven't investigated this yet.

> > * lists: rename dsme_list_empty to dsme_list_is_empty?

Not done yet. (The rationale is that dsme_list_empty sounds like it
returns a new, empty list.)

> > * logging: is STI generic enough? maybe just say "SERIAL"?

I'd like someone to comment on this. I don't even really know what STI
is.

> > * logging.h: uses non-standard macro varargs, maybe use the standard
> >   variant instead?

Needs fixing.

> > * logging.h: document dsme_log_open arguments

Done.

> > * messages.h: much should be moved to plugins

Not done yet.

> > * modules.h: does this need to be supported in C++?

The C++ include protection things should be used consistently for all
headers, or for no headers. Someone needs to make the strategic decision
of whether the dsme library code needs to be callable from C++.

> > * modules.h: don't use u_int32_t, which is nonstandard; use uint32_t
> >   instead

This is partially nitpicking, and it's going to result in a pretty big
diff, so I haven't done it yet. If anyone objects, I'll not do it at
all, though I think it'd be cleaner to use standard types.

> > * protocol.h: move all but core message types to plugins

Not done yet.

> > * select.h: move the private stuff out from the public header

We no longer have a select.h; I created something that is, in my
opinion, a better abstraction (io.h, io.c).

> > * timers.h: if the dsme_timer_t fields aren't meant to be used
> >   outside of timers.c, move the struct definition to inside timers.c

Not done yet.

> > * dsme.c: use sig_atomic_t instead of int for gotsignal

We don't have gotsignal anymore. This was also part of the io.c change.

> > * dsme.c: signal handling seems somewhat suspect; maybe move from
> >   signal(2) to more modern Unix signal handling, to make things
> >   safer (sigaction)

Not done yet.

> > * modulebase.c, others: no point in having a prototype for a
> >   function directly followed by its definition in the same file (if
> >   it's a static function, -Wmissing-prototypes shouldn't complain
> >   anyway)

Harmless, but should be cleaned up before release.

> > * modulebase.c: remove obsolete code, don't just comment it out

Harmless, but should be cleaned up before release.

> > * what's with ending string literals with an explicit \0 ?

I haven't heard of, or figured out, a good explanation for this yet. If
there is none, I'd remove the explicit zero bytes.

> > * protocol.c: why is an explicit 9 used as the size for a
> >   dsmemsg_close_t? is transmitting the extra padding bytes bad in
> >   some way that matters?

This I haven't changed yet, because I keep assuming that there must be
an important reason for it. However, I can't figure it out.

> > * is the dsme message protocol ever going to be useful for
> >   communicating between hosts? if so, it needs to be re-thought
> >   (struct sizes, etc); but I assume it won't be

After thinking about this for a few weeks, I think it's fairly clear
that the dsme protocol is only useful within one machine. Everything
must be re-examined if it's going to be useful across machines. For
example, when notifying that a process has ended, it's no longer enough
to have just the process ID, you need to give an ID for the machine as
well.

> > * protocol.c: in function dsmesock_receive: if we get end-of-file (a
> >   zero byte read), we immediately close the connection and free the
> >   buffer; is there not a possiblity that the buffer still contains
> >   something useful that just hasn't been processed yet?

I think the answer is "yes, there is a possiblity", but haven't verified
it yet.

> > * it's not necessary to check for a NULL pointer before calling free

Harmless, but should be fixed before release.

> > * protocol.c: dsmesock_send: the write may block (if non-blocking
> >   I/O is used), or not write everything (if it is)

This has been fixed. The io.c changes alluded above is the fix. It is a
subsystem that buffers both reads and writes, and only writes from the
buffer to the socket as much as possible without blocking.

> > * protocol.c: dsmesock_getucred: not thread safe; do we care about
> >   thread safety?

This is a strategic design decision that someone needs to make. I think
the answer should be "yes, we care about thread safety", but I didn't
feel comfortable making the decision unilaterally.

> > * spawn.c: either the comment or the sigchldhandler function is
> >   wrong, since they contradict; also, again, sigaction signal
> >   handling may be required for safety; also, it writes two bytes to
> >   the signal_pipe, when it would seem one is sufficient

There is no longer a sigchldhandler in spawn.c (there is one now in
dsme.c, which is arguably the right place). Both the code and the
comment have been rewritten and fixed.

> > * spawn.c: is_inside can be written as: return str[strcspn(str,
> >   chars)];

Not implemented yet.

> > * timers.c: is the __NR_SYSCALL hack for uclibc still needed?

I haven't used uclibc, and I don't know which version is relevant to
Nokia.

> > * timers.c: why are posix timers (clock_gettime(CLOCK_MONOTONIC))
> >   better than gettimeofday for this application? (if they are,
> >   document the reason; either way, try to use only one, for that
> >   little bit of extra simplicity)

Again, this is outside my sphere of knowledge. Please educate me.

> > * timers.c: a dsme_list_sorted_insert function would be helpful

The function would clarify the timer code a little bit, I think, but not
enough that I have bothered to do it yet. Should be easy and quick to
do, though.

> > * timers.c: function dsme_shortest_timeout: struct assignment works
> >   as well as memcpy, and is cleaner

Again a minor cleanup that could be done before release.