Eric Saxe wrote: > 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)?
Yeah, you're right. The suggestions code will still be used, the back end would be different. thanks Rafael >>> - 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 > _______________________________________________ > tesla-dev mailing list > tesla-dev at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/tesla-dev
