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

[Ismo.Laitinen at nokia.com]

>-----Original Message-----
>From: dsm-devel-bounces at garage.maemo.org 
>[mailto:dsm-devel-bounces at garage.maemo.org] On Behalf Of ext 
>Lars Wirzenius
>Sent: 17 August, 2006 22:32
>To: dsm-devel at garage.maemo.org
>Subject: RE: [DSM-devel] dsme code cleanup, comments before merging?
>

Hi,

thanks for your work. Here's some more comments. I need to look more 
into ones that I left without a comment, especially io.c.

Have you come up with other major changes that should or could be done?

One additional thing that needs to be looked into is the messages.h and
how to split it up between the core and plugins.

>> > 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.

I commented this a bit in the previous mail.

>
>> > * 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.

To me this sounds good. It would indeed make it easier to start with
the clients.

>> > * 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.

Agreed.

>> > * 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.

/dev/null would be more approriate?

>> > * 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.

Actually this used to use malloc() but someone bugged me to change it to

alloca() to avoid problems with failing malloc().. Thinking of it now,
it can use malloc() as it is used elsewhere, too.

>> > * 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.

Agreed.

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

Looks right. I don't know if there was some original idea in not
using lists.c/h.

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

I think that is has something to do with Doxygen comments, but I'm
not sure.

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

Ok, as long as it is consistent :)

>> > * 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.)

Agreed, sounds sane.

>> > * 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.

I commented this a bit in the previous mail. STI is a serial type of 
interface on OMAP that is used mostly for debugging. There's a kernel 
driver that has an netlink interface to userspace. It's not useful in 
other environments and it uses a hardcoded channel value so something 
should be done about it.

 
>
>> > * 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.

messages.h needs to be eventually split up between each plugins.
We haven't really tought about the best way, 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.

Is there a reason why not to do it?

>
>> > * 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.

Agreed. 

>> > * 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.

Probably a good idea.

>
>> > * 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.

Agreed.

>> > * modulebase.c: remove obsolete code, don't just comment it out
>
>Harmless, but should be cleaned up before release.

Agreed. It has not been used ever, afaik.

>
>> > * 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.

Braindamage? Should be removed.

>> > * 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.

No idea. It's just been there forever.

>> > * 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.

I think this is a fair assumption now. However, there has been some 
ideas that maybe someone wants to use dsme to control several machines. 
As you said, many things need to be checked and changed to do that.

>> > * 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.

Agreed.

>> > * 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.

Especially for the general use, I'd say that we should care about thread
safety. 

David, any toughts?

>
>> > * 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.

Good point. Seems that our uclibc now defines those already.

>> > * 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.

This was a way to avoid timer breakage if the system time is changed.
Do you know if there's a more common way for this?

>
>> > * 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.

Agreed.

BR, Ismo

>
>_______________________________________________
>DSM-devel mailing list
>DSM-devel at garage.maemo.org
>https://garage.maemo.org/mailman/listinfo/dsm-devel
>