Hi Rafael, Rafael Vanoni wrote: > 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 ? The suggestions framework I think is fine (actually, isn't there a suggestion to suggest running as root to get all the functionality?). Which interface are you looking to migrate to for enabling CPU power management (if it's disabled)?
>> - 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. Heh. >> 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. Thanks..although I think it's the case that nothing is suggested other than suggesting that the user run as root (to get suggestions). :) > 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, -Eric
