[erlang-patches] DTrace patch, review draft #2

[Björn Gustavsson]

On Tue, Nov 15, 2011 at 3:19 AM, Scott Lystig Fritchie
<fritchie@REDACTED> wrote:
> Andreas, thanks for your work and advice.  I've included your
> efile_drv.d converted script into
> lib/dtrace/examples/efile_drv.systemtap.
>
> OTP team, I've a new branch for review.  I know that the top-level
> README.md changes aren't appropriate for the real OTP repo, but having
> the diffs there is nice for people who visit my GitHub repo's page at
> https://github.com/slfritchie/otp/tree/dtrace-review2.
>
> Are there other things that might be preventing this branch from going
> into the "pu" branch?
>

To put my comments in context, we (the Erlang/OTP team)
think that dtrace support could potentially be very useful feature
in Erlang/OTP and we therefore consider including it in R15 as an
experimental feature turned off by default.

For an experimental feature, we have the following requirements
in general:

1. If the feature is not enabled, it should not cause any problems.
Basically, that what we will focus on when testing your branch.

2. The approach should reasonable and there should not be any
obvious problems or blatant coding style violations.

That was the general stuff. Now to my specific comments what
needs to be fixed.

I found one major problem and some minor things that should
be fixed.

Doing a default build should work without problems. I did a
"./otp_build setup -a" on my Mac (Mac OS 10.7.2) and got:

=== Entering application dtrace
erlc -W  +debug_info -I../include -I ../../et/include -I
../../../libraries/et/include -o../ebin dtrace.erl
sed -e 's;%VSN%;0.8;' dtrace.app.src > ../ebin/dtrace.app
sed -e 's;%VSN%;0.8;' dtrace.appup.src > ../ebin/dtrace.appup
dtrace.erl:71: Warning: variable 'Message' is unused
make -f i386-apple-darwin11.2.0/Makefile TYPE=opt
/usr/bin/install -c -d ../priv/obj/i386-apple-darwin11.2.0
gcc -c -o ../priv/obj/i386-apple-darwin11.2.0/dtrace.o -Wall
-Wstrict-prototypes -Wmissing-prototypes -Wdeclaration-after-statement
 -DUSE_THREADS -D_THREAD_SAFE -D_REENTRANT -DPOSIX_THREADS -m32 -g -O2
-I/Users/bjorng/Downloads/otp/erts/i386-apple-darwin11.2.0
-no-cpp-precomp  -D_XOPEN_SOURCE -fPIC -fno-common
-I/Users/bjorng/Downloads/otp/erts/emulator/beam
-I/Users/bjorng/Downloads/otp/erts/include
-I/Users/bjorng/Downloads/otp/erts/include/i386-apple-darwin11.2.0
-I/Users/bjorng/Downloads/otp/erts/include/internal
-I/Users/bjorng/Downloads/otp/erts/include/internal/i386-apple-darwin11.2.0
-I/Users/bjorng/Downloads/otp/erts/emulator/sys/unix
-I../priv/obj/i386-apple-darwin11.2.0
-I/Users/bjorng/Downloads/otp/erts/emulator/i386-apple-darwin11.2.0
dtrace.c
dtrace.c:30:25: error: dtrace_user.h: No such file or directory
make[4]: *** [../priv/obj/i386-apple-darwin11.2.0/dtrace.o] Error 1
make[3]: *** [opt] Error 2
make[2]: *** [opt] Error 2
make[1]: *** [opt] Error 2
make: *** [libs] Error 2

That must be fixed. Also, there should be no compilation warnings.

Now on to more minor things:

The erts/.gitignore file is included in the commit, but it only adds
a blank line.

Regarding coding style, our main rule is to imitate the style
of the surrounding code in the same file. In this case, in most
C source files, we put the beginning curly bracket on a new line
like this:

int foo(void)
{
...
}

(You've put the curly bracket at the end of function declaration line.)

Also, the most common comment style for "block" comments
is:

/*
 * Here is a comment.
 */

(Your added comments seem to have different styles.)

Other than that, I didn't notice any coding style problems after a quick
look.

My final comment is that your changes are included in a single
commit, which make it harder to review and will it make it harder
to understand in the future. I would suggest breaking it up into several
commits (at least three):

First one commit that introduces the dtrace mechanism, i.e. the
dtrace application, the dtrace support in header files in erts/emulator/beam,
erlang_dtrace.d, changes in configure and Makefiles.

A second commit that introduces all the example D programs in
the dtrace application.

One or more commits that add the probes in the emulator. For example,
there could be one commit that adds all probes for file operations
(in both the emulator and the kernel application), followed by other
commits for other categories of probes. Or all probes could be in a
single commit.

/Björn

-- 
Björn Gustavsson, Erlang/OTP, Ericsson AB