Add ability to specify a script's license #3157
Conversation
ajor
force-pushed
the
licences
branch
2 times, most recently
from
cbda372
to
3d1396d
Compare
Not all code will be GPL-licensed, so provide users with a way to override the assumed GPL license.
IDs representing a GPL v2 license are translated into "GPL" to be recognised by the kernel. Licenses defined in a BpfScript config block take precedence over SPDX IDs.
| // SPDX-License-Identifier: GPL-2.0-or-later | ||
| ---- | ||
|
|
||
| If a license is not explicitly declared, bpftrace will assume that the script is GPL v2 compatible. |
There was a problem hiding this comment.
Nit: This language feels a little weird to me. How about "If a license is not explicitly declared, bpftrace will license the script as GPL v2."
| @@ -607,14 +607,12 @@ CallInst *IRBuilderBPF::CreateProbeReadStr(Value *ctx, | |||
| // int bpf_probe_read_str(void *dst, int size, const void *unsafe_ptr) | |||
| FunctionType *probereadstr_func_type = FunctionType::get( | |||
| getInt64Ty(), { dst->getType(), getInt32Ty(), src->getType() }, false); | |||
| PointerType *probereadstr_func_ptr_type = PointerType::get( | |||
| probereadstr_func_type, 0); | |||
| Constant *probereadstr_callee = ConstantExpr::getCast( | |||
There was a problem hiding this comment.
Sorry, I might have missed it but what is this change?
There was a problem hiding this comment.
Oops, you're right this could have done with an explanation. It's to produce more useful error messages for the bpf_probe_read_str helper. A lot of other helpers are already using this CreateHelperCall utility function, and then rest probably should be switched to it in the future as well.
bpf_probe_read_str is a GPL-only helper function so I ended up getting a lot of error messages with this one in particular when playing around with different licenses.
Without this change:
# bpftrace -e 'config = {license = "poo"; } BEGIN { str(0); }'
WARNING: Addrspace is not set
Attaching 1 probe...
ERROR: helper bpf_probe_read_str can only be used in GPL-compatible programs
With this change:
# bpftrace -e 'config = {license = "poo"; } BEGIN { str(0); }'
WARNING: Addrspace is not set
Attaching 1 probe...
stdin:1:38-44: ERROR: helper bpf_probe_read_str can only be used in GPL-compatible programs
config = {license = "poo"; } BEGIN { str(0); }
~~~~~~
There was a problem hiding this comment.
Ah nice. Thanks for the explanation.
|
|
||
| // XXX: Set licenses for manually loaded programs. Remove once libbpf is used | ||
| // instead: | ||
| bpftrace_.resources.license = license; |
There was a problem hiding this comment.
I know it's just temporary but this doesn't feel like a good place to set this. Why not do it in ResourceAnalyser?
| @@ -13,6 +13,7 @@ Config::Config(bool has_cmd, bool bt_verbose) : bt_verbose_(bt_verbose) | |||
| config_map_ = { | |||
| { ConfigKeyBool::cpp_demangle, { .value = true } }, | |||
| { ConfigKeyBool::lazy_symbolication, { .value = false } }, | |||
| { ConfigKeyString::license, { .value = std::nullopt } }, | |||
There was a problem hiding this comment.
This is a bit strange to me. The default license is GPL but we set the default config value to none here and only return "GPL" from BPFtrace::get_license. Why not set it to "GPL" here and only override the value from SPDX/config, if specified?
If the only reason is that we don't have a config priority defined for config value coming from a script comment header (SPDX here), then I believe that we should introduce that. It will also save the introduction of std::optional<T> Config::try_get.
There was a problem hiding this comment.
Yes. Thank you @viktormalik for articulating this. I also didn't understand why we didn't want to set the default in the config.
| constexpr std::string_view spdx_id_marker = "SPDX-License-Identifier: "; | ||
| auto spdx_id_pos = program.find(spdx_id_marker); |
There was a problem hiding this comment.
Should we ensure this is inside a comment? Seems like this would match this string anywhere in the file
Not all code will be GPL-licensed, so provide users with a way to override the assumed GPL license.
Licenses defined in a BpfScript config block take precedence over SPDX IDs.
Checklist
man/adoc/bpftrace.adocCHANGELOG.md