Details
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Can you upload the diff using either of the following commands to get the context (or use arc).
git diff -U999999 other-branch svn diff --diff-cmd=diff -x -U999999
There needs to be a warning on CDDL code being compiled into the kernel.
You should move the list of files to build from sys/conf/files.arm to sys/conf/files as its doesn't appear to be arm specific.
This seems to be half dealing with dtrace on arm, and half allowing dtrace to be in kernel config files.
Or, I'm misreading it and it's going to break the latter.
I'd suggest splitting things.
| sys/cddl/dev/dtrace/arm/dtrace_asm.S | ||
|---|---|---|
| 32 | Why would this be defined already? | |
| sys/conf/files | ||
| 107–113 | And how are these picked up now with zfs? | |
| sys/conf/kern.post.mk | ||
| 187–192 | How was this not working before? | |
| sys/conf/kern.pre.mk | ||
| 153 | Lots of repetition with ZFS_CLFAGS. Perhaps you could combine the lists? | |
The reason why we have DTrace and ZFS only as modules is because of the CDDL. Can you please make sure core is okay with this change? They will have access to all the email archives discussing the problems and the outcomes. They can tell you exactly the reasons why we never had DTrace/ZFS in the kernel.
core is ok. this only allows to proceed the build for someone who want to try it locally
| sys/cddl/dev/dtrace/arm/dtrace_asm.S | ||
|---|---|---|
| 32 | It is defined here: <built-in>:316:9: note: previous definition is here what it that <built-in> ? clang ? | |
| sys/conf/files | ||
| 107–113 | Now it is assumed people should include 'device opensolaris' also with 'device zfs' in their kernel configs | |
| 107–113 | alternatively we can do something like: is it better ? | |
| sys/conf/kern.post.mk | ||
| 187–192 | these was working before fine for ZFS, but now it also work for dtrace. | |
| sys/conf/kern.post.mk | ||
|---|---|---|
| 187–192 | defines renamed to 'CDDL' assuming it is both for zfs and dtrace now | |
| sys/cddl/dev/dtrace/arm/dtrace_asm.S | ||
|---|---|---|
| 32 | Doesn't it come from the command line? | |
| sys/cddl/dev/dtrace/arm/dtrace_asm.S | ||
|---|---|---|
| 32 | yes it comes from sys/conf/kern.pre.mk: DTRACE_ASM_CFLAGS= -x assembler-with-cpp -DLOCORE ${DTRACE_CFLAGS} (I copied it from ZFS_ASM_CFLAGS) should it be removed both from ZFS_ASM_CFLAGS and DTRACE_ASM_CFLAGS ? | |
| sys/cddl/dev/dtrace/arm/dtrace_asm.S | ||
|---|---|---|
| 32 | The other way around I think: since it's now defined in the command line, can't you remove it from this file? | |
| sys/cddl/dev/dtrace/arm/dtrace_asm.S | ||
|---|---|---|
| 32 | yes it is also works both with modules & with no modules | |
| sys/conf/files | ||
|---|---|---|
| 248 | I think you should make the 'device' or 'options' lines match with the layout of the modules, so I would maybe make this 'optional dtmalloc | dtraceall' and similarly for the other things like lockstat, sdt, etc. | |
| 255 | This one should be something like 'optional dtnfscl nfscl | dtraceall nfscl'. | |
In addition, there should be some updates to some NOTES files, either sys/conf/NOTES for things that are truly MI or MD NOTES files for things that are only supported on a subset of platforms.
Thanks. I would please ask that you consider adding the dtrace lines to the x86 and powerpc NOTES file as well (the other architectures that build the dtrace modules).
| sys/amd64/conf/NOTES | ||
|---|---|---|
| 20 ↗ | (On Diff #5688) | nitpick: DTrace - two initial capitals I also wonder if we should mention the license issue in here, something like NOTE: introduces CDDL-licensed components into the kernel |