Skip to content
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

netpacket/sockaddr_ll: Add valid packet types for sll_pkttype #15389

Merged
merged 1 commit into from
Dec 31, 2024

Conversation

tmedicci
Copy link
Contributor

Summary

  • netpacket/sockaddr_ll: Add valid packet types for sll_pkttype

Add valid packet types for the sll_pkttype member of the struct sockaddr_ll. These definitions are commonly used by third-party libraries.

Impact

Enable using the socket module of Python (WIP). No impact on existing configs.

Testing

Internal CI testing

Add valid packet types for the sll_pkttype member of the struct
sockaddr_ll. These definitions are commonly used by third-party
libraries.
@github-actions github-actions bot added the Size: S The size of the change in this PR is small label Dec 30, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 30, 2024

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the basic NuttX requirements, although it could be improved with more detail.

Here's a breakdown of why and where it could be better:

Strengths:

  • Clear Summary: The summary explains the "what" and "why" of the change. It mentions the specific code affected (netpacket/sockaddr_ll) and the intended outcome (adding valid packet types).
  • Impact is Addressed: The impact section highlights the primary benefit (enabling Python's socket module) and notes the lack of impact on existing configurations.
  • Testing is Mentioned: The PR mentions internal CI testing, indicating some level of verification.

Weaknesses (and suggestions for improvement):

  • Missing Issue References: If this PR addresses a specific issue in either the NuttX or NuttX-apps repositories, those issues should be linked.
  • Vague Impact Description: While the impact on users is briefly mentioned, more detail is needed. How exactly will this enable Python's socket module? Are there any potential edge cases or limitations? The "NO" answers for other impact areas are fine if truly no impact exists.
  • Insufficient Testing Detail: "Internal CI testing" is too vague. Specify the build host(s) and target(s) used in testing, as per the requirements. Ideally, include snippets of relevant log output before and after the change to demonstrate the fix or improvement. Even if the CI does the testing, summarizing the platforms tested is important for reviewers.
  • Missing "How it Works": The summary mentions what is being added (valid packet types), but not how the change is implemented. A brief explanation of the code changes would be helpful. For example, "Added PACKET_HOST, PACKET_BROADCAST, etc., as defined in [relevant standard/header] to sockaddr_ll."

In short: The PR provides a good starting point but needs more detail, especially in the Impact and Testing sections, to meet the NuttX requirements fully.

@xiaoxiang781216 xiaoxiang781216 merged commit a4fd1b3 into apache:master Dec 31, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants