After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 129167 - fix for slightly broken acpid events
fix for slightly broken acpid events
Status: RESOLVED FIXED
Product: gnome-applets
Classification: Other
Component: battery
2.4.x
Other Linux
: High major
: ---
Assigned To: gnome-applets Maintainers
gnome-applets Maintainers
: 126921 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2003-12-12 03:11 UTC by Brian Perkins
Modified: 2004-12-22 21:47 UTC
See Also:
GNOME target: 2.6.next
GNOME version: ---


Attachments
Alternative patch (615 bytes, patch)
2004-02-05 05:43 UTC, James Clark
none Details | Review

Description Brian Perkins 2003-12-12 03:11:15 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;
 }
Comment 1 David Moore 2003-12-19 16:12:46 UTC
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.
Comment 2 Luis Villa 2004-01-03 00:11:37 UTC
*** Bug 126921 has been marked as a duplicate of this bug. ***
Comment 3 Luis Villa 2004-01-03 00:13:08 UTC
Adding PATCH keyword and some other priority bits. You guys should 
feel free to raise priority yourself in future if you need to.
Comment 4 Kevin Vandersloot 2004-01-04 16:56:36 UTC
Applied to both branches. Thanks for the patch Brian
Comment 5 James Clark 2004-02-05 05:43:37 UTC
Created attachment 24076 [details] [review]
Alternative patch
Comment 6 James Clark 2004-02-05 05:58:20 UTC
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.
Comment 7 David Moore 2004-02-05 15:01:55 UTC
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.