hald-addon-acpi cannot survive acpid restart

[Richard Hughes]

On Mon, 2005-09-12 at 20:32 -0400, Ryan Lortie wrote:
> 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?).

Ahh no, I missed this case.

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

Sure, I was just provoking discussion :-) - get this fixed upstream in
the best way.

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

Davidz: was there a reason we used a custom read_line?

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

Sure, but what could overflow?

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

Totally minor:

if (fp == NULL)
{
    dbg ("fdopen failed: %s", strerror (errno));
    close (fd);
}

->

if (fp == NULL) {
    dbg ("fdopen failed: %s", strerror (errno));
    close (fd);
}

Danny, is there a way we could incorporate your SUSE changes into this
and get a common code base?

Richard.