On Sun, Nov 16, 2008 at 10:46 AM, Dan McDonald <[EMAIL PROTECTED]> wrote:
> On Sun, Nov 16, 2008 at 07:43:36AM -0600, Mike Gerdts wrote:
>> I've posted an updated code review that adds a test case. I've
>> verified a full clobber build, that bfu delivers the new test case,
>> and that the test case fails without the fix and succeeds with the
>> fix.
>
> I'm certainly fine with the dtri.c fix (check env ONCE, then set state), and
> you can claim me as a reviewer for that file. Let's just hope there aren't
> any grinning weirdos out there who DEPEND on being able to change
> DTRACE_DOF_INIT_DEBUG in the middle of a run. :) (You know there's probably
> at least one person out there who'll complain...)
>
> You should get a 2nd okay on the test script, however. I see how it works,
> but I'm not familiar enough with .../dtrace/test/... to know if your script
> fits in with the big picture of how tests are done.
I'm not sure that I can count as an official code reviewer, but here
are my comments, anyway:
- The drti.c change looks fine.
- I don't know that I'd create a new test suite directory just for
drti. I'd say that this belongs in the usdt directory.
- Minor aesthetic complaint: There's a tab in front of "all" in the
Makefile embedded in the test script. It works either way, but it
doesn't match other test scripts. (Of course, I go to verify that
this is the case and find the other instance of this in
pid/tst.provregex3.ksh:
> hg history ./pid/tst.provregex3.ksh
changeset: 5985:a183e54573d2
user: jhaslam
date: Thu Feb 07 06:05:33 2008 -0800
description:
6325485 A stddev() aggregator would be a nice adjunct to avg()
6618705 p*d123 doesn't cause pid probes to be created
6624541 dtrace aggregations should assume signed arguments
Contributed by Chad Mynhier <[EMAIL PROTECTED]>.
>
Doh!)
Chad
_______________________________________________
dtrace-discuss mailing list
[email protected]
