hald-addon-acpi cannot survive acpid restart

[Ryan Lortie]

On Mon, 2005-12-09 at 18:13 +0100, Richard Hughes wrote:
> On Mon, 2005-09-12 at 06:16 -0400, Ryan Lortie wrote:
> > This is a patch to make it so that if acpid is restarted then the hald
> > acpi 'addon' will reconnect to it once it comes back rather then exiting
> > completely.
> 
> Sounds like a plan.
> 
> > Basically, if a dropped connection was detected before, the addon would
> > exit().  Now I've added another while(1) around the body of the program
> > and changed some other things around very slightly.
> 
> Hmm. The code seems a bit wild to me...
yah.  one too many loops :)

> 
> > This patch has been in Ubuntu for the past few days and nobody is
> > screaming about it so I guess it probably works fairly OK.
> > 
> > Ubuntu bug is here -- http://bugzilla.ubuntu.com/show_bug.cgi?id=14852
> > 
> > The patch attached to the bug report is difference since I made it for
> > Breezy's HAL (which has sort of taken on a life of its own).  The patch
> > against CVS HEAD is attached to this email.
> 
> What about a similar idea (but with less tabbing :-) attached.... (no
> comments or sensible function names yet)
> 
> This is completely untested btw, as I've removed acpid.
> 
> Does this do the same stuff another (slightly prettier) way?

It's more or less the same.

You made a bit of a mistake in your while(1) loop for ACPI.  It will
never exit even in the case that we're just starting the addon for the
first time and acpid is not running.  In this way it is different than
the code I had written (but maybe this was your intention?).

But... if we're going to clean this up we may as well clean it up all
the way.  I've rewritten your rewritten patch even further to fix some
compiler warnings and leaks. (Specifically, sometimes your static
functions would go unused (see below) and the dbus 'error' parameter was
continuously being reinitialised without being free'd -- this one not
your fault).

I fixed the unused function warnings by doing this sort of thing:

static int func() {
#ifdef foo
..stuff..
#endif
}

main() {
func();
}

instead of this:

static int func() {
..stuff..
}

main() {
#ifdef foo
func();
#endif
}

Otherwise my patch is semantically equivalent to yours (ie: in theory it
should always do the same things except for the exception mentioned
above).  The biggest change is that I've done away with this whole
read_line business since it was just reinventing the fgets() wheel.  As
a result, we now pass around FILE* instead of int fd.

To be completely fixed up, the inner part of main_loop that does all of
the event parsing should probably be rewritten.  As written, it has some
very obvious buffer overflow possibilities.  I don't want to go
tinkering around here though because I have no idea what the format of
ACPI event messages is.

I've only given this a very cursory testing (ie: started it up,
restarted acpid, etc to make sure it was working).

See any room for further improvement?

Cheers.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: addon-acpi.c
Type: text/x-csrc
Size: 4426 bytes
Desc: not available
Url : http://lists.freedesktop.org/archives/hal/attachments/20050912/0c89327a/addon-acpi-0001.c