I received Konstantin's code review over the weekend, so I'm reposting
my last email with changes after both Eric's and Kon's comments. This
diff contains fixes for both.
I also gave powertop.h a well deserved face lift, so it follows what you
usually see in ON code. #defines, structures and typedefs, global vars
and then extern declarations - plus removing whatever wasn't being used
and fixing a few declarations.
Because we've reached a good amount of fixes since the last bump in the
version number, I suggest that we release version 1.2 after this batch
of changes.
Here are Kon's comments (hope you don't mind me posting them here, Kon).
Please see my previous email for Eric's).
> 1. usr/src/cmd/powertop/powertop.c#99:
>
> if (dump_count == NULL || dump_count < 0)
>
> why not: if (dump_count <= 0)
Done.
> 2. general: I think it is proper to use func(void) instead of func().
> 3. battery_stat_snapshot(), pt_cpufreq_snapshot():
> not invoking kstat_close() if function returns with error.
Yep.
> 4. general: Why not use __FILE__ macro instead of hardcoded filename, e.g:
> pt_error("cpufreq.c : cannot open ..."?
Dido.
> 5. pt_cpufreq_stat_prepare():
> - do while loop at lines 131-148:
> what for we need both strstr() and strtok().
> Something like seems sufficeint for me:
> for (token = strtok(s, ":"), s = NULL;
> NULL != token;
> token = strtok(NULL, ":")) {..}
Okay.
> - line 147: state++;
> Can you garantee that npstates will be always less then
> NSTATES? Probably we need to check that we are not going
> over array bounds?
It *should* be, but I'm adding a check anyway.
> 6. get_bit_depth() - not a big deal as invoked only once,
> but: buf never freed. Instead of malloc(), why not just
> allocate buf on the stack?
Sure.
Please review these changes as soon as possible so we have plenty of
time to integrate PowerTOP into snv100.
Thanks again to everyone who sent comments.
Rafael
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: reviews.diff
URL:
<http://mail.opensolaris.org/pipermail/tesla-dev/attachments/20080921/15c167d6/attachment.ksh>
