-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Add BPF support & C-based BPF tic-tac-toe #1422
Conversation
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.
Looks like shellcheck is a little angry still.
The duplication in the various build.sh and the common C runtime prelude that’s included in the various C examples (like magic number 6) feels like a WIP. I guess all that is going to get ripped out as we continue to iterate on this, so I’m ok with it as is for now.
programs/bpf/move_funds_c/build.sh
Outdated
@@ -0,0 +1,12 @@ | |||
#!/bin/bash | |||
|
|||
set -e |
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: add -ex to the shebang instead: #!/bin/bash -ex
programs/bpf/noop_rust/build.sh
Outdated
@@ -0,0 +1,13 @@ | |||
#!/bin/sh |
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 be /bin/bash as there are bash-specific features in here
//#include <stddef.h> | ||
|
||
#if 1 | ||
// one way to define a helper function is with idex as a fixed value |
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.
“idex” => “index”?
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.
Where does the 6 come from?
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.
Linux kernel definition. This is another possible modification I'd like to make to rbpf to make it more general
|
||
fn verify_prog_len(prog: &[u8]) { | ||
if prog.len() % ebpf::INSN_SIZE != 0 { | ||
panic!( |
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.
Can a Result<> be returned instead of panicking?
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 verifier was pretty much pulled directly from rbpf and contrary to it returning a bool rpbf ignores the return value and relies on panic. This is another thing that's on my list of things to change about rbpf, fail gracefully
Yeah, the boilerplate code in the programs will be consolidated to a common library. Magic number 6 is defined by rbps currently, I'll create a definition for it. |
…na-labs#1422) * stake-pool-cli: Fix withdraw calc for CLI / program consistency * Run cargo fmt * Integrate review feedback
* Add detailed metrics reporting for packet filtering * fix conflicts
…abs#1422) * Add detailed metrics reporting for packet filtering * fix conflicts
This PR addresses the next two boxes of #1255
Making this a PR because it makes changes to the build system, otherwise, most of the changes are featurized with bpf_c
What this PR does include:
--features="bpf_c"
)What it does not include and next steps:
Account.Userdata