Hi Nico,
I've taken some time to review the patches in BTS #410478, and I think I'm
going to pass most of them up. Here's a rationale, per change:
--- acpi-support-0.90-bckp/webbtn.sh 2007-02-10 23:44:57.000000000 +0100
+++ acpi-support-0.90-work/webbtn.sh 2007-02-11 00:05:03.000000000 +0100
@@ -1,3 +1,2 @@
#!/bin/bash
-. /usr/share/acpi-support/key-constants
-acpi_fakekey $KEY_WWW
+sensible-browser &
Interesting, but this script is run by acpid, which is running as a
daemon, as root. Hence, the browser will run as root and have no display.
Did you actually test this? Furthermore, the acpi-support key handling
design generally dictates that it simply translates specific hardware
buttons into generic keypresses, and it tries to avoid doing anything
regarding policy. There are exceptions, but this is definitely not the
place to make one.
--- acpi-support-0.90-bckp/asus-wireless.sh 2007-02-10 23:44:57.000000000
+0100
+++ acpi-support-0.90-work/asus-wireless.sh 2007-02-10 23:59:38.000000000
+0100
@@ -1,7 +1,7 @@
#!/bin/bash
# Find and enable/disable wireless devices
-state=`. /etc/acpi/wireless.sh`
+state=$((`cat /proc/acpi/asus/wled`)
if [ "$state" = "0" ]; then
echo -n 0 > /proc/acpi/asus/wled
Interesting, the lack of matching brackets. Did you actually test this?
And judging from earlier discussions in the bug report, it doesn't
actually fix any bug.
--- acpi-support-0.90-bckp/asus-touchpad.sh 2007-02-10 23:44:57.000000000
+0100
+++ acpi-support-0.90-work/asus-touchpad.sh 2007-02-10 23:45:37.000000000
+0100
@@ -1,7 +1,7 @@
#!/bin/sh
# get the current state of the touchpad
-TPSTATUS=`synclient -l | grep TouchpadOff | awk '{print $3}'`
+TPSTATUS=`synclient -l | awk '{/TouchpadOff/ print $3}'`
# if getting the status failed, exit
test -z $TPSTATUS && exit 1
This one actually seems reasonable, but not compelling enough to increase
the diff with upstream for. And it fixes no bug.
--- acpi-support-0.90-bckp/acpi_fakekey.c 2007-02-10 23:44:57.000000000
+0100
+++ acpi-support-0.90-work/acpi_fakekey.c 2007-02-10 23:52:03.000000000
+0100
@@ -16,7 +16,10 @@
for (i=0; i<32; i++) {
snprintf(filename,sizeof(filename), "/dev/input/event%d",
i);
- fd = open(filename, O_RDWR);
+ if((fd = open(filename, O_RDWR))<0){
+ perror("open");
+ exit(EXIT_FAILURE);
+ }
ioctl(fd, EVIOCGBIT(EV_KEY, sizeof(key_bitmask)),
key_bitmask);
for (j = 0; j < BTN_MISC; j++) {
This one's valid, applied (with coding style fixups).
@@ -38,17 +41,13 @@
struct input_event event;
if (argc == 2) {
- key = atoi(argv[1]);
+ key = strtol(argv[1], NULL, 10);
} else {
- return 1;
+ return -1;
}
fd = find_keyboard();
- if (!fd) {
- return 2;
- }
-
event.type = EV_KEY;
event.code = key;
event.value = 1;
I don't see any reason for the first change if you're not going to pass an
endptr (the second parameter to strtol). Regarding the second change, IIRC
negative error codes are for kernel API functions and such, and
executables normally return positive error codes. And I wonder what the
reason for removing that last check is? Since find_keyboard still has a
very big "return 0" at the end of the function, I don't see why this check
has become obsolete.
In summary: I'll accept the one change, and reject the rest. The change
will be included in the next upload, and then I'll close this bug report,
since the rest of the changes are actually about bugs nor features.
Cheers,
Bart
