-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ability to specify a script's license #3157
base: master
Are you sure you want to change the base?
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I might have missed it but what is this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah nice. Thanks for the explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, just some implementation details to clear up.
|
||
// XXX: Set licenses for manually loaded programs. Remove once libbpf is used | ||
// instead: | ||
bpftrace_.resources.license = license; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.adoc
CHANGELOG.md