dtrace discuss - Nov 2008 - Round two: Re: code review req: 6750659 drti.o crashes app due to corrupt environment

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.

http://cr.opensolaris.org/~mgerdts/6750659-2008-11-15/

Mike


On Wed, Nov 12, 2008 at 9:50 PM, Mike Gerdts <mgerdts at gmail.com>
wrote:
> I believe that I have a fix for 6750659 ready to go.  Can I get a few eyes
on:
>
> http://cr.opensolaris.org/~mgerdts/6750659-2008-11-12/
>
> There are only 5 lines of meaningful change.  Some background info is
> available at
>
>
http://mail.opensolaris.org/pipermail/dtrace-discuss/2008-September/006637.html
>
> Jon Haslam has offered to sponsor this change.
>
> --
> Mike Gerdts
> http://mgerdts.blogspot.com/
>


-- 
Mike Gerdts
http://mgerdts.blogspot.com/

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.

Dan

On Sun, Nov 16, 2008 at 9:46 AM, Dan McDonald <danmcd at sun.com>
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...)
Thanks for taking a look.  I had thought about that case as well, and
figured nearly everyone else would agree that the person that
complained was a weirdo.  As best as I can tell, this environment
variable is not documented as an interface and it is new enough that
google (after eliminating duplicates) shows only 8 references.  Five
of those references are related to the initial problem that led to the
bug report or are related to the fix.
> 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.
The test script started out as a different one from the test suite
that also needed to create a USDT provider.  The changes were:

- Specify $PATH instead of providing full path to some commands
- Use mktemp(1) instead of introducing symlink vulnerabilities
- Clean up temporary directory even if the test fails
- Changed the provider name from something that seemed to be
  specific to the test I copied it from
- Got rid of the ident string
- Respect CC environment variable during compile

I noticed that I missed the bits to get it into the SUNWdtrt package
and am in the process of another full build in preparation for
generating an updated webrev.

Thanks for the review.

-- 
Mike Gerdts
http://mgerdts.blogspot.com/

On Sun, Nov 16, 2008 at 10:46 AM, Dan McDonald <danmcd at sun.com>
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 <cmynhier at gmail.com>.
>
Doh!)

Chad

On Sun, Nov 16, 2008 at 11:38 AM, Chad Mynhier <cmynhier at gmail.com>
wrote:
> On Sun, Nov 16, 2008 at 10:46 AM, Dan McDonald <danmcd at sun.com>
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.
ok.  Thanks!
> - 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.
ok,  moved.
> - 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
Fixed.
> 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 <cmynhier at gmail.com>.
>
>>
>
> Doh!)
Well, at least you caught it this time.  :)

I had initially missed the packaging of the test case and have fixed
that as well.  I''m doing a fresh build to be sure I didn''t
break
anything and will post the updated webrev later today.

-- 
Mike Gerdts
http://mgerdts.blogspot.com/