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

I believe that I have incorporated all of the feedback given
(thanks!).  Changes since the 2008-11-16 version include:

- ksh style & coding standards compliance in test script (Roland)
- "dof_init_debug == B_FALSE" vs. "!dof_init_debug" (Adam)

The updated webrev is at:

http://cr.opensolaris.org/~mgerdts/6750659-2008-12-03/

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

Mike Gerdts wrote:
> 
> I believe that I have incorporated all of the feedback given
> (thanks!).  Changes since the 2008-11-16 version include:
> 
> - ksh style & coding standards compliance in test script (Roland)
> - "dof_init_debug == B_FALSE" vs. "!dof_init_debug"
(Adam)
> 
> The updated webrev is at:
> 
> http://cr.opensolaris.org/~mgerdts/6750659-2008-12-03/
Quick review (patch code is quoted with "> "):
-- snip --
[snip]
> +
> +PATH=/usr/bin:/usr/sbin:$PATH
Is "export" not needed in this case, e.g. shouldn''t
subprocesses inherit
PATH, too ?
> +if (( $# != 1 )); then
> +       print -u2 ''expected one argument:
<dtrace-path>''
> +       exit 2
> +fi
> +
> +#
> +# jdtrace does not implement the -h option that is required to generate
> +# C header files.
> +#
> +if [[ "$1" == */jdtrace ]]; then
> +       exit 0
Is the zero return code (= "suceess") Ok in this case ?
> +fi
> +
> +dtrace="$1"
> +startdir="$PWD"
> +dir=$(mktemp -td drtiXXXXXX)
> +if (( $? != 0 )); then
> +       print -u2 ''Could not create safe temporary
directory''
> +       exit 2
> +fi
> +
> +cd "$dir"
> +
> +cat > Makefile <<EOF
> +all: main
> +
> +main: main.o prov.o
> +       \$(CC) -o main main.o prov.o
> +
> +main.o: main.c prov.h
> +       \$(CC) -c main.c
> +
> +prov.h: prov.d
> +       $dtrace -h -s prov.d
> +
> +prov.o: prov.d main.o
> +       $dtrace -G -32 -s prov.d main.o
> +EOF
> +
> +cat > prov.d <<EOF
> +provider tester {
> +       probe entry();
> +};
> +EOF
> +
> +cat > main.c <<EOF
> +#include <stdlib.h>
> +#include <sys/sdt.h>
> +#include "prov.h"
> +
> +int
> +main(int argc, char **argv, char **envp)
> +{
> +       envp[0] = (char*)0xff;
> +       TESTER_ENTRY();
> +       return 0;
ISO C defines |EXIT_SUCCESS| (it''s defined as |#define EXIT_SUCCESS
(0)|) ... I don''t know whether it''s better or worse in this
case...
> +}
> +EOF
> +
> +make > /dev/null
> +status=$?
> +if (( $status != 0 )) ; then
Umpf... please remove the extra ''$'' - see
http://www.opensolaris.org/os/project/shell/shellstyle/#avoid_unneccesary_string_number_conversions
-- snip --

The remaining stuff looks good... :-)

----

Bye,
Roland

-- 
  __ .  . __
 (o.\ \/ /.o) roland.mainz at nrubsig.org
  \__\/\/__/  MPEG specialist, C&&JAVA&&Sun&&Unix
programmer
  /O /==\ O\  TEL +49 641 3992797
 (;O/ \/ \O;)

I''d like to put an end to this discussion. Mike has done enough work,  
and these tests are more than up to the task. If there are further  
comments on the code, that''s fine, but we can suffer through tests  
that are functionally correct, but stylistically less than perfect.

Adam

On Dec 10, 2008, at 6:13 AM, Roland Mainz wrote:
> Mike Gerdts wrote:
>>
>> I believe that I have incorporated all of the feedback given
>> (thanks!).  Changes since the 2008-11-16 version include:
>>
>> - ksh style & coding standards compliance in test script (Roland)
>> - "dof_init_debug == B_FALSE" vs. "!dof_init_debug"
(Adam)
>>
>> The updated webrev is at:
>>
>> http://cr.opensolaris.org/~mgerdts/6750659-2008-12-03/
>
> Quick review (patch code is quoted with "> "):
> -- snip --
> [snip]
>> +
>> +PATH=/usr/bin:/usr/sbin:$PATH
>
> Is "export" not needed in this case, e.g. shouldn''t
subprocesses
> inherit
> PATH, too ?
>
>> +if (( $# != 1 )); then
>> +       print -u2 ''expected one argument:
<dtrace-path>''
>> +       exit 2
>> +fi
>> +
>> +#
>> +# jdtrace does not implement the -h option that is required to  
>> generate
>> +# C header files.
>> +#
>> +if [[ "$1" == */jdtrace ]]; then
>> +       exit 0
>
> Is the zero return code (= "suceess") Ok in this case ?
>
>> +fi
>> +
>> +dtrace="$1"
>> +startdir="$PWD"
>> +dir=$(mktemp -td drtiXXXXXX)
>> +if (( $? != 0 )); then
>> +       print -u2 ''Could not create safe temporary
directory''
>> +       exit 2
>> +fi
>> +
>> +cd "$dir"
>> +
>> +cat > Makefile <<EOF
>> +all: main
>> +
>> +main: main.o prov.o
>> +       \$(CC) -o main main.o prov.o
>> +
>> +main.o: main.c prov.h
>> +       \$(CC) -c main.c
>> +
>> +prov.h: prov.d
>> +       $dtrace -h -s prov.d
>> +
>> +prov.o: prov.d main.o
>> +       $dtrace -G -32 -s prov.d main.o
>> +EOF
>> +
>> +cat > prov.d <<EOF
>> +provider tester {
>> +       probe entry();
>> +};
>> +EOF
>> +
>> +cat > main.c <<EOF
>> +#include <stdlib.h>
>> +#include <sys/sdt.h>
>> +#include "prov.h"
>> +
>> +int
>> +main(int argc, char **argv, char **envp)
>> +{
>> +       envp[0] = (char*)0xff;
>> +       TESTER_ENTRY();
>> +       return 0;
>
> ISO C defines |EXIT_SUCCESS| (it''s defined as |#define
EXIT_SUCCESS
> (0)|) ... I don''t know whether it''s better or worse in
this case...
>
>> +}
>> +EOF
>> +
>> +make > /dev/null
>> +status=$?
>> +if (( $status != 0 )) ; then
>
> Umpf... please remove the extra ''$'' - see
>
http://www.opensolaris.org/os/project/shell/shellstyle/#avoid_unneccesary_string_number_conversions
> -- snip --
>
> The remaining stuff looks good... :-)
>
> ----
>
> Bye,
> Roland
>
> -- 
>  __ .  . __
> (o.\ \/ /.o) roland.mainz at nrubsig.org
>  \__\/\/__/  MPEG specialist, C&&JAVA&&Sun&&Unix
programmer
>  /O /==\ O\  TEL +49 641 3992797
> (;O/ \/ \O;)
> _______________________________________________
> dtrace-discuss mailing list
> dtrace-discuss at opensolaris.org

--
Adam Leventhal, Fishworks                        http://blogs.sun.com/ahl