GNOME Bugzilla – Bug 129167
fix for slightly broken acpid events
Last modified: 2004-12-22 21:47:04 UTC
The acpid event handling that recently got into battstat seems to be broken for me. It looks like sometimes the read ins acpi_process event is getting half and event, or something. At any rate, acpi status changes aren't updated by battstat. I'm using gnome-2.4.1 from fedora-core 1 with kernel 2.6-test11 with a few custom patches, mainly for acpi. I've written a patch that cleans this up a bit, and makes things more readable, and best of all, fixes the problem. Take a look at bug 88553 for more info. I diffed my changes with the latest cvs and went through by hand and mined out all of the cruft, so apologies if it's somehow mangled. diff -ur battstat/acpi-linux.c /tmp/rpm/BUILD/gnome-applets-2.4.1/battstat/acpi-linux.c --- battstat/acpi-linux.c 2003-10-22 18:01:18.000000000 -0400 +++ /tmp/rpm/BUILD/gnome-applets-2.4.1/battstat/acpi-linux.c 2003-12-11 21:33:52.847038352 -0500 @@ -294,31 +294,12 @@ /* Given a string event from the ACPI system, returns the type * of event if we're interested in it. str is updated to point * to the next event. */ -static int parse_acpi_event(char * event, char ** str) +static int parse_acpi_event(GString *buffer) { - char * end; - char * tok[4]; - int i; - - end = strchr(event, '\n'); - if (!end) - return ACPI_EVENT_DONE; - - *end = '\0'; - *str = end + 1; - - tok[0] = event; - for (i=0; i < 3; i++) { - tok[i+1] = strchr(tok[i], ' '); - if (!tok[i+1]) - return ACPI_EVENT_IGNORE; - *(tok[i+1]) = '\0'; - tok[i+1]++; - } - if (!strcmp(tok[0], "ac_adapter")) + if (strstr(buffer->str, "ac_adapter")) return ACPI_EVENT_AC; - if (!strcmp(tok[0], "battery") && atoi(tok[2]) == 81) + if (strstr(buffer->str, "battery") ) return ACPI_EVENT_BATTERY_INFO; return ACPI_EVENT_IGNORE; @@ -330,16 +311,18 @@ * by select(). */ gboolean acpi_process_event(struct acpi_info * acpiinfo) { - int i; + gsize i; int evt; char * s; - char str[256]; + /* char str[1024];*/ gboolean result = FALSE; + GString *buffer; + GError *gerror=NULL; + buffer=g_string_new(NULL); + g_io_channel_read_line_string ( acpiinfo->channel,buffer,&i,&gerror); - i = read(acpiinfo->event_fd, str, sizeof(str)-1); - str[i] = '\0'; - s = str; - while ((evt = parse_acpi_event(s, &s)) != ACPI_EVENT_DONE) { + + evt = parse_acpi_event(buffer); switch (evt) { case ACPI_EVENT_AC: update_ac_info(acpiinfo); @@ -350,7 +333,7 @@ result = TRUE; break; } - } + return result; }
Hi, I'm the original contributor of the patch for using ACPI events in bug 88553. I've tried this new patch and it works for me. I've also heard a success report from at least one other user. This patch should definitely be applied ASAP since it fixes a critical bug in the original patch that seems to affect all Fedora users.
*** Bug 126921 has been marked as a duplicate of this bug. ***
Adding PATCH keyword and some other priority bits. You guys should feel free to raise priority yourself in future if you need to.
Applied to both branches. Thanks for the patch Brian
Created attachment 24076 [details] [review] Alternative patch
The patch doesn't seem to addressing the real problem, which is failing to read complete events. The old code just did a read() and expected that to read a complete event, but that won't work reliably because it's reading from a SOCK_STREAM which makes no guarantees about preserving message boundaries and in any case acpid uses a separate write() for the newline. Since acpid terminates each event with a newline, why not simply fix the reading code to read up to a newline? The trivial attached patch (against the old code) does this. It works for me (on Gentoo). I also wonder whether things can be simplified further. I get an event: ac_adapter ACAD 00000080 00000000 when the ac_adapter is removed, and an event ac_adapter ACAD 00000080 00000001 when it is inserted, so it seems that it might be possible to simply use the 4th token to determine whether the AC is online rather than reading /proc/acpi/ac_adapter.
The patch provided by Brian did fix the core problem of which you speak by using g_io_channel_read_line_string() instead of read(), thus preserving the boundaries. You're right that it could be done with far less code. He wanted to simplify parse_acpi_event() also, which wasn't strictly necessary. About the reading of /proc/acpi/ac_adapter: It may not be sufficient to rely on the argument to the ac_adapter event, because I haven't confirmed that it works on all hardware. I believe the arguments themselves are BIOS-dependent, so it's likely that some BIOSes will not provide valid arguments or that broken BIOSes will only confuse us. I figure reading /proc/acpi/ac_adapter is a fool-proof way of avoiding such problems.