Forwarding Eric's code review with comments and attaching a patch with fixes. Please see inline.
> battery.c > - battery_stat_snapshot() returns error which print_battery ignores. If > print_battery() isn't > going to do anything, should battery_stat_snapshot() just be of type > static void? > - Perhaps it could instead call pt_error(), and emit some sort of error > message if something > goes wrong reading the battery kstats? Fixed in the proposed patch. > cpufreq.c: > - I'm still dubious about having PowerTOP check, and enable cpupm in the > way that it does. Yeah, I'm working on that. But if we remove it (since cpupm is on by default now), we should remove all the suggestions code. Which is also okay, since I'm working on something to allow suggestions through a proper interface. Does that sound reasonable ? > - pt_cpufreq_snapshot() isn't very robust if something goes wrong looking > up the kstats. It > should probably check the return codes from the kstat interfaces, and > use pt_error, and > perhaps return either a fatal (or non-fatal) error as appropriate if > something goes wrong. Also fixed in this patch. > display.c: > - event_bubblesort(): there's a qsort() library interface...can we use > that, or some other more > efficient sorting routine instead since bubble sort is O(n2) Done.. that was easier than I thought - and should've been fixed a while ago :/ > - show_acpi_power_line(): 351: typo: "enery" should be "energy"? Should > this say something > like: "Critically low battery power"? Yeah, "critical energy state" could be anything from a nuclear meltdown to low batt :) Changed in this patch too. > powertop.c > - 315: Do you have to run as root to get suggestions, or do you need > certain "privileges"? > Will PT allow you to run as non-root anyway? The only existing suggestion requires root privileges, so we don't suggest anything if a regular user is running the tool. Non-root users can run PowerTOP as long as they have dtrace privileges. Another change I'm suggesting with this patch is that we only display the Intel copyright banner as part of the usage message. The rationale for this one is that showing the copyright during execution might imply that the data being generated on a user's system is "owned" by Intel. I've also updated the copyright year to 2008. But I'll obviously defer to our friends at Intel on both of these changes ;) Please send your comments asap as we're putting PowerTOP back into snv100 for OpenSolaris 2008.11 - which closes on the 29th. thanks Rafael -------------- next part -------------- An embedded and charset-unspecified text was scrubbed... Name: erics_review.diff URL: <http://mail.opensolaris.org/pipermail/tesla-dev/attachments/20080920/ab7d5d17/attachment.ksh>
