GNOME Bugzilla – Bug 168355
[PATCH] Add FreeBSD ACPI support to battstat
Last modified: 2005-05-10 05:47:53 UTC
Please describe the problem: The attached patch adds ACPI support to battstat for FreeBSD, and only enables the building of battstat on i386 FreeBSD (for now). Steps to reproduce: Actual results: Expected results: Does this happen every time? Other information:
Created attachment 37876 [details] [review] Add FreeBSD ACPI support to battstat
*** Bug 162641 has been marked as a duplicate of this bug. ***
Created attachment 38134 [details] [review] Same patch as before minus a superfluous include
A few questions. 1. I don't understand why apm_info is now a global variable (but I haven't looked very closely, either). 2. #include <dev/acpica/acpiio.h> copacetic:~$ locate acpica/acpiio.h /usr/obj/usr/src/i386/usr/include/dev/acpica/acpiio.h /usr/src/sys/dev/acpica/acpiio.h copacetic:~$ uname -a FreeBSD copacetic.desrt.ca 5.3-STABLE FreeBSD 5.3-STABLE #0: Wed Feb 9 05:11:04 EST 2005 desrt@copacetic.desrt.ca:/usr/src/sys/i386/compile/COPACETIC i386 This header appears not to be available on my 5.3 box except in /usr/src. I can't imagine the build looks there (and even if it did, this would fail if i didn't have the src packages installed). I guess this header is installed in /usr/include on more recent systems? Can you add some autoconf magic to conditionalise the enabling of this code so that gnome-applets will continue to compile on non-bleeding-edge FreeBSD systems?
This code will compile on non-bleeding-edge FreeBSD systems provided system src is available. I handle this in the gnomeapplets2 port, and this code has been compiling on 5.3 for sometime. See my patches to configure.in.
We can't seriously accept this code into the official gnome-applets distribution unless it can be compiled and installed, without src, on a 4.x system. (despite the name, 5-STABLE isn't stable enough for some people's tastes and many 4.x boxes still exist).
Quite honestly, that's not a good reason to keep the code out of GNOME CVS. Most FreeBSD users use the ports tree or packages to install GNOME, and this code builds just fine on the FreeBSD package cluster as well as on users' own machines under 4.X. Having this committed into GNOME CVS just means I have fewer local patches to maintain. On top of that, the FreeBSD GNOME team is considering dropping 4.X support since it lacks many of the features needed for a desktop platform. Keeping this code out of GNOME CVS just my my work harder which in turn hurts FreeBSD GNOME users.
If you have patches in the portage/packages builds to conditionalise this code so that 4.x boxes are still supported (or even 5.x boxes that don't have src/ installed) then please share them. We really can't go half-way about this. Even if most users use the ports or packages, people who want to download the package from the gnome servers and build it for themselves should still be able to.
Created attachment 46200 [details] [review] Same patch but won't build battstat if requisite headers are not found
I've uploaded a new patch with additional configure.in support to not build battstat if the acpiio.h header cannot be found. I've also genericized the FREEBSD_SYS variable to OS_SYS for NetBSD and OpenBSD.
The desired behaviour is to have battstat just be built using the old FreeBSD APM interface if ACPI is not available. Anything else is a regression in behaviour.
Created attachment 46203 [details] [review] Fallback on the old APM interface is ACPI support is not available
Just from a quick look at the latest patch it looks like the definition of a couple of functions (acpi_freebsd_read, acpi_process_event, acpi_freebsd_cleanup) depend on #ifdef HAVE_ACPIIO but are used in code that is not guarded with the same ifdef. I havn't actually tried it, but I think this would result in battstat failing to link on FreeBSD systems that don't have the acpiio headers installed.
Ok. I've made a few minor changes to your patch: Added stub functions in acpi-freebsd.c to allow linking to succeed even if ACPI support is missing. Moved the #include "acpi-freebsd.h" outside of the #ifdef HAVE_ACPIIO so that the stubs are properly prototyped. Changed the global variables in power-management.c to be static. Removed: if (using_acpi) { acpi_process_event(&acpiinfo); acpi_freebsd_read(&apminfo, &acpiinfo); } since it causes an ACPI read every time (and twice every 30 times) instead of the probably desired effect of a read every 30 times. Also removed fd = -1 and added acpi_process_event(&acpiinfo); to the code that runs once every 30 seconds. (I'm not *exactly* sure what acpi_process_event is for, but if you're supposed to call it before every call to acpi_freebsd_read then this is correct). Removed the extra prototype for freebsd_acpi_read() in power-management.c since this is in the .h file. Added a check on the return value of acpi_freebsd_read and return an error if there is a problem. (is there a good suggestion for the user that can be added to this error message?) --- I'm attaching the new acpi-freebsd.c and power-management.c files here (from HEAD). I don't have a FreeBSD GNOME box so I haven't been able to test them at all (even compile them) so your feedback is greatly appreciated here (particularly on the specifics of if my understanding of what acpi_process_event is correct). Thanks.
Created attachment 46206 [details] [review] freebsd-acpi.c
Created attachment 46207 [details] freebsd-acpi.c
Created attachment 46208 [details] power-management.c sorry. not sure why freebsd-acpi.c got attached twice :(
Yes, these look good. I had a similar stubs patch for freebsd-acpi.c, but your changes to power-management.c are good as well. As for the error, I'd add, "Check to make sure the ACPI subsystem is properly loaded."
Ok. I just commited the code to CVS HEAD. If you could check it out and make sure everything's OK it'd be appreciated. Thanks for your efforts and putting up with my nit-picking :)
Thanks. Looks good to me.