hald-addon-acpi cannot survive acpid restart
Richard Hughes
hughsient at gmail.com
Tue Sep 13 01:45:36 PDT 2005
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.
More information about the hal
mailing list