Add support for stack argument #773

liyan-ah · 2023-08-30T07:13:57Z

Add support for stack argument

related to #659

netlify · 2023-08-30T07:14:02Z

✅ Deploy Preview for aya-rs-docs ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`5fc269e`
🔍 Latest deploy log	https://app.netlify.com/sites/aya-rs-docs/deploys/6525205a2a35b80008af43d2
😎 Deploy Preview	https://deploy-preview-773--aya-rs-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

liyan-ah · 2023-08-30T08:59:42Z

@tamird
A test case is added to ensure stack_argument works okay.
Any other suggestions?

vadorovsky

Just one nit.

The rest looks good. I like the tests!

bpf/aya-bpf/src/args.rs

vadorovsky

Thanks!

test/integration-test/Cargo.toml

Co-authored-by: vadorovsky <[email protected]>

test/integration-test/src/tests/stack_argument.rs

bpf/aya-bpf/src/args.rs

test/integration-test/src/tests/stack_argument.rs

test/integration-ebpf/src/stack_argument.rs

test/integration-test/src/tests/stack_argument.rs

mergify · 2023-09-25T01:24:43Z

@liyan-ah, this pull request is now in conflict and requires a rebase.

liyan-ah · 2023-09-27T11:41:57Z

@tamird @vadorovsky
Any further suggestions?

tamird · 2023-09-27T14:53:39Z

test/integration-ebpf/src/stack_argument.rs

+
+//read argument, and send event
+fn try_stack_argument(ctx: ProbeContext) -> Result<i32, i64> {
+    let _ = ARGS.insert(&0, &ctx.arg(0).ok_or(255)?, 0);


can we avoid these _?

There would be warning if no let _.

warning: unused `Result` that must be used --> test/integration-ebpf/src/stack_argument.rs:23:5 | 23 | ARGS.insert(&0, &ctx.arg(0).ok_or(255)?, 0); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this `Result` may be an `Err` variant, which should be handled = note: `#[warn(unused_must_use)]` on by default help: use `let _ = ...` to ignore the resulting value | 23 | let _ = ARGS.insert(&0, &ctx.arg(0).ok_or(255)?, 0); | +++++++

Yes, the proper thing to do is handle these errors rather than silently ignore them. You probably want to return from the program on any error.

tamird · 2023-09-27T14:53:53Z

test/integration-ebpf/src/stack_argument.rs

+    }
+}
+
+//read argument, and send event


what event? I don't understand this comment.

comment modified.

tamird · 2023-09-27T14:56:04Z

rustfmt.toml

@@ -1,3 +1,4 @@
+edition = "2021"


tamird · 2023-09-27T14:56:30Z

test/integration-test/src/tests/stack_argument.rs

+        HashMap::try_from(bpf.take_map("ARGS").unwrap()).unwrap();
+    trigger_stack_argument(0, 1, 2, 3, 4, 5, 6, 7);
+
+    tokio::time::sleep(std::time::Duration::from_millis(100)).await;


why are we sleeping here?

The sleeping could be removed, and it works okay in my local environment. The sleeping is added just as referred in integration-test/src/tests/log.rs.

tamird · 2023-09-27T14:57:03Z

test/integration-test/src/tests/stack_argument.rs

+    a_3: u64,
+    a_4: u64,
+    a_5: u64,
+    //in x86_64, from arg6, stack_argument would be used


please write a proper sentence and add a space after the //.

can you add a citation?

citation added.

tamird · 2023-09-27T15:03:28Z

test/integration-ebpf/src/stack_argument.rs

+//read argument, and send event
+fn try_stack_argument(ctx: ProbeContext) -> Result<i32, i64> {
+    let _ = ARGS.insert(&0, &ctx.arg(0).ok_or(255)?, 0);
+    let _ = ARGS.insert(&1, &ctx.arg(1).ok_or(255)?, 0);


what's the story with these .ok_or(255)? I don't understand why this function returns a Result where the only possible "error" is 255.

There is no means for 255, just to make sure the test in stack_argument would failed.

I understand, but I do not think this is an appropriate thing to do. If these methods fail, we should just return from this program.

tamird · 2023-09-27T15:04:01Z

test/integration-test/src/tests/stack_argument.rs

+    assert_eq!(args_map.keys().count(), 8);
+    for iter in args_map.iter() {
+        let iter_v = iter.unwrap();
+        assert_eq!(iter_v.0 as u64, iter_v.1);


please destructure instead of these point accesses.

I'm not sure about this. Any problems with map.iter()?

let (key, value) = iter.unwrap()

mergify · 2023-09-27T20:58:47Z

@liyan-ah, this pull request is now in conflict and requires a rebase.

liyan-ah · 2023-09-28T03:45:24Z

@tamird

tamird · 2023-09-28T13:49:54Z

Please test this locally before pinging again.

liyan-ah · 2023-10-10T03:59:08Z

@tamird
Check about this?

liyan-ah · 2023-10-10T07:07:44Z

@tamird
The stack_argument should be used differently in different arch. How can I specify different integeration-ebpf test-case for arm, aarch64 and amd?
I have found none examples in other test cases. Does #[cfg(bpf_target_arch="amd")] works?
Or I would have to try with argument return values, which is rigorous.

tamird

I'm running out of patience with this PR. Every revision is ridden with bugs. I'm inclined to stop reviewing this TBH.

tamird · 2023-10-10T13:49:38Z

test/integration-ebpf/src/stack_argument.rs

+
+//read argument, and send event
+fn try_stack_argument(ctx: ProbeContext) -> Result<i32, i64> {
+    let _ = ARGS.insert(&0, &ctx.arg(0).ok_or(255)?, 0);


Yes, the proper thing to do is handle these errors rather than silently ignore them. You probably want to return from the program on any error.

tamird · 2023-10-10T13:50:01Z

test/integration-ebpf/src/stack_argument.rs

+//read argument, and send event
+fn try_stack_argument(ctx: ProbeContext) -> Result<i32, i64> {
+    let _ = ARGS.insert(&0, &ctx.arg(0).ok_or(255)?, 0);
+    let _ = ARGS.insert(&1, &ctx.arg(1).ok_or(255)?, 0);


I understand, but I do not think this is an appropriate thing to do. If these methods fail, we should just return from this program.

tamird · 2023-10-10T13:50:21Z

test/integration-test/Cargo.toml

@@ -10,6 +10,7 @@ assert_matches = { workspace = true }
 aya = { workspace = true }
 aya-log = { workspace = true }
 aya-obj = { workspace = true }
+bytes = { workspace = true }


what is this?

tamird · 2023-10-10T13:50:52Z

test/integration-test/src/tests/stack_argument.rs

+    // Notice that other languages, like Golang, or in other archs, like aarch64, may
+    // have different convention rules.
+    _a_6: u64,
+    _a_7: i64,


why is this a different type?

tamird · 2023-10-10T13:51:09Z

test/integration-test/src/tests/stack_argument.rs

+    assert_eq!(args_map.keys().count(), 8);
+    for iter in args_map.iter() {
+        let iter_v = iter.unwrap();
+        assert_eq!(iter_v.0 as u64, iter_v.1);


let (key, value) = iter.unwrap()

tamird · 2023-10-10T13:53:47Z

test/integration-ebpf/src/stack_argument.rs

+            break;
+        }
+        if stack {
+            let _ = ARGS.insert(&arg, &ctx.arg(arg as usize).ok_or(255)?, 0);


shouldn't this be stack_arg?

liyan-ah · 2023-10-11T02:17:03Z

I'm running out of patience with this PR. Every revision is ridden with bugs. I'm inclined to stop reviewing this TBH.

Sorry about that.

From my view, the mainly errors are specify different integeration-ebpf test-case for arm, aarch64 and amd. I would fix it latter.

mergify · 2023-10-12T00:58:06Z

@liyan-ah, this pull request is now in conflict and requires a rebase.

liyan-ah · 2023-11-13T03:37:11Z

Would reopen when I get some spare time.

merge sarg patch.

9c167c7

liyan-ah mentioned this pull request Aug 30, 2023

Add support for stack argument #659

Closed

mergify bot added aya-bpf This is about aya-bpf (kernel) test A PR that improves test cases or CI labels Aug 30, 2023

fix bug and format.

454bf0d

tamird requested a review from vadorovsky August 31, 2023 15:51

vadorovsky requested changes Sep 1, 2023

View reviewed changes

bpf/aya-bpf/src/args.rs Outdated Show resolved Hide resolved

use c_ulonglong instead of __u64.

f00ddb6

vadorovsky approved these changes Sep 4, 2023

View reviewed changes

fix bug.

08d038c

vadorovsky requested changes Sep 4, 2023

View reviewed changes

test/integration-test/Cargo.toml Outdated Show resolved Hide resolved

liyan-ah and others added 3 commits September 5, 2023 10:39

Update test/integration-test/Cargo.toml

d8f7675

Co-authored-by: vadorovsky <[email protected]>

format Cargo.toml

24cc238

Merge branch 'main' into new_sarg

e807cb7

tamird reviewed Sep 5, 2023

View reviewed changes

test/integration-test/src/tests/stack_argument.rs Outdated Show resolved Hide resolved

pdliyan added 2 commits September 7, 2023 10:29

change the way loading stack_argument bpf bytes.

a2201e6

fix aya_log.

7060235

tamird reviewed Sep 12, 2023

View reviewed changes

mergify bot added the needs-rebase label Sep 25, 2023

fix stack_argument test.

b3b8d44

liyan-ah closed this Sep 26, 2023

liyan-ah force-pushed the new_sarg branch from b3b8d44 to 8ae5b0d Compare September 26, 2023 09:33

mergify bot removed the needs-rebase label Sep 26, 2023

Merge branch 'main' into new_sarg

5ed278a

liyan-ah reopened this Sep 26, 2023

remove unused struct.

b249789

liyan-ah requested a review from vadorovsky September 26, 2023 09:42

liyan-ah requested a review from tamird September 26, 2023 09:42

fix.

31fb700

tamird reviewed Sep 27, 2023

View reviewed changes

mergify bot added the needs-rebase label Sep 27, 2023

pdliyan added 2 commits September 28, 2023 11:34

add further annotation.

5812252

Merge branch 'main' into new_sarg

59e8098

mergify bot removed the needs-rebase label Sep 28, 2023

liyan-ah requested a review from tamird September 28, 2023 03:45

pdliyan added 3 commits September 28, 2023 17:02

add riscv support.

f9861aa

fix.

da29228

try fix Cargo.toml

c627cde

pdliyan added 2 commits October 9, 2023 19:50

fix build error.

0347dae

Merge branch 'main' into new_sarg

21f63e0

pdliyan added 3 commits October 10, 2023 17:31

Use argument results to decide whether or not to read stack_argument.

b8c43a0

format

dd4ee87

fix unwrap error.

5fc269e

tamird reviewed Oct 10, 2023

View reviewed changes

mergify bot added the needs-rebase label Oct 12, 2023

liyan-ah closed this Nov 13, 2023

mergify bot removed the needs-rebase label Nov 13, 2023

liyan-ah deleted the new_sarg branch November 24, 2023 08:52

Add support for stack argument #773

Add support for stack argument #773

Conversation

liyan-ah commented Aug 30, 2023

netlify bot commented Aug 30, 2023 • edited Loading

✅ Deploy Preview for aya-rs-docs ready!

liyan-ah commented Aug 30, 2023

vadorovsky left a comment

Choose a reason for hiding this comment

vadorovsky left a comment

Choose a reason for hiding this comment

mergify bot commented Sep 25, 2023

liyan-ah commented Sep 27, 2023

Choose a reason for hiding this comment

liyan-ah Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Sep 27, 2023

liyan-ah commented Sep 28, 2023

tamird commented Sep 28, 2023

liyan-ah commented Oct 10, 2023

liyan-ah commented Oct 10, 2023

tamird left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liyan-ah commented Oct 11, 2023

mergify bot commented Oct 12, 2023

liyan-ah commented Nov 13, 2023

netlify bot commented Aug 30, 2023 •

edited

Loading

liyan-ah Sep 28, 2023 •

edited

Loading