Skip to content

Conversation

@skorobogatydmitry
Copy link

@skorobogatydmitry skorobogatydmitry commented Nov 24, 2025

This change is Reviewable

@netlify
Copy link

netlify bot commented Nov 24, 2025

Deploy Preview for aya-rs ready!

Name Link
🔨 Latest commit 5e1a549
🔍 Latest deploy log https://app.netlify.com/projects/aya-rs/deploys/69499ad77403920008d2a2f1
😎 Deploy Preview https://deploy-preview-256--aya-rs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@skorobogatydmitry skorobogatydmitry marked this pull request as draft November 25, 2025 09:36
@skorobogatydmitry skorobogatydmitry marked this pull request as ready for review November 25, 2025 11:32
Copy link
Member

@tamird tamird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM, a few comments. @vadorovsky can you TAL as well?

@tamird reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @skorobogatydmitry)


-- commits line 7 at r2:
Please squash to a single commit.

Code quote:

New commits in r1 on 11/24/2025 at 2:09 PM:
- 301d145: Add a simple SockOps program example

New commits in r2 on 11/25/2025 at 4:44 AM:
- 8188702: Fix lint errors:
  - fit 80-symbols rows width
  - add spaces for the code blocks

docs/book/programs/sockets.md line 29 at r2 (raw file):

    Ok = 1,
    #[allow(dead_code)]
    Err = 2, // Some other number to indicate error

If 1 is success and negative is not supported, then what is 2?


docs/book/programs/sockets.md line 68 at r2 (raw file):

}

// Stub panic handler to please the compiler

This is actually not a stub - we use an infinite loop to ensure that any panicking code paths are rejected by the verifier (because it forbids infinite loops).


docs/book/programs/sockets.md line 19 at r1 (raw file):

enum SockOpsResult {
    // From the official docs: Regardless of the type of operation, the program should always return 1 on success.

what are the official docs? can you please link to the source?

@skorobogatydmitry
Copy link
Author

skorobogatydmitry commented Nov 26, 2025

If 1 is success and negative is not supported, then what is 2?

Nothing in particular. A enum with single option looks odd and current implementation of sock_ops requires -> u32, so I can't add a negative option.

what are the official docs? can you please link to the source?

It's linked at the top of the page, I don't see the point to repeat it in the comment here.

Copy link
Member

@tamird tamird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a rebase after #258. Thanks!

@tamird reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @skorobogatydmitry)


docs/book/programs/sockets.md line 19 at r1 (raw file):

It's linked at the top of the page, I don't see the point to repeat it in the comment here.

Those aren't official docs, those are docs hosted by a company called isovalent. The source code is https://github.com/isovalent/ebpf-docs.


docs/book/programs/sockets.md line 29 at r2 (raw file):

Nothing in particular. A enum with single option looks odd and current implementation of sock_ops requires -> u32, so I can't add a negative option.

Then just toss the enum and return a literal 0?

@skorobogatydmitry
Copy link
Author

This needs a rebase after #258. Thanks!

@tamird reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @skorobogatydmitry)

docs/book/programs/sockets.md line 19 at r1 (raw file):

It's linked at the top of the page, I don't see the point to repeat it in the comment here.

Those aren't official docs, those are docs hosted by a company called isovalent. The source code is https://github.com/isovalent/ebpf-docs.

docs/book/programs/sockets.md line 29 at r2 (raw file):

Nothing in particular. A enum with single option looks odd and current implementation of sock_ops requires -> u32, so I can't add a negative option.

Then just toss the enum and return a literal 0?

I rebased the change and removed all mentions of docs.ebpf.io as an official source. Thank you for pointing this out!

I left the enum around, as I don't have my playground to test the change handy. On the top of that, I think that having enum makes it clearer what the numbers are about, what's good for the code as an example.

Copy link
Member

@tamird tamird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codex review

@tamird reviewed 2 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @skorobogatydmitry).


docs/book/programs/sockets.md line 29 at r2 (raw file):

Previously, skorobogatydmitry (Dmitry Skorobogaty) wrote…

I rebased the change and removed all mentions of docs.ebpf.io as an official source. Thank you for pointing this out!

I left the enum around, as I don't have my playground to test the change handy. On the top of that, I think that having enum makes it clearer what the numbers are about, what's good for the code as an example.

I think it's fine to keep the enum, but the value of "2" is nonsense, right?

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +25 to +29
// the program should always return 1 on success.
// A negative integer indicate a operation is not supported.
Ok = 1,
#[allow(dead_code)]
Err = 2, // Some other number to indicate error

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Use negative value for SockOps error return

The example explains that SockOps programs should return 1 on success and a negative integer on unsupported operations, but the enum sets Err = 2. If a reader uses this enum for error handling, the program will return a non‑negative value that the kernel treats as success, so the intended error path won’t trigger. This is especially misleading because the comment explicitly documents negative values as the error contract.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants