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

rpmsg_sockif: add some modifaction for rpsocket #14118

Merged
merged 5 commits into from
Oct 13, 2024

Conversation

yintao707
Copy link

@yintao707 yintao707 commented Oct 11, 2024

Note: Please adhere to Contributing Guidelines.

Summary

  1. add rpmsg_socket_ept_release for free connection
    to Fix possible use-after-free issues when closing

  2. sendsize should be protected by sendlock

  3. remove backlog=-1 because patch1 has removed

  4. Minor cleanups
    Simplify the message process in rpmsg_socket_send_single and fix the style issue

  5. use hash to handle rp_name
    expand the user's rpname size
    hash when exceed RPMSG_SOCKET_NAME_LEN due to the limitation of eptname

Impact

none

Testing

sim

@github-actions github-actions bot added Area: Networking Effects networking subsystem Size: S The size of the change in this PR is small labels Oct 11, 2024
@nuttxpr
Copy link

nuttxpr commented Oct 11, 2024

[Experimental Bot, please feedback here]

NuttX PR Requirements Review

This PR does not meet the NuttX requirements. Here's why:

  • Summary:
    • The summary is too brief. It only mentions the problem ("use-after-free issues when closing") without explaining the cause or the solution.
    • It lacks details about the functional part of the code being changed.
    • It doesn't mention related issues or PRs.
  • Impact:
    • While stating "none", it's unlikely a bug fix has zero impact. Be specific about what aspects remain unaffected.
  • Testing:
    • Insufficient Detail: Simply stating "sim/qemu" isn't descriptive enough. Specify:
      • Host OS and versions used
      • QEMU version
      • Specific NuttX configuration for the simulation
    • Missing Logs: The "Testing logs before change" and "Testing logs after change" sections are empty. Provide concrete evidence of the issue before the fix and the successful resolution after.

To improve this PR:

  1. Expand the Summary:
    • Clearly state the root cause of the use-after-free issue.
    • Describe the specific code changes made to address the problem.
    • Link any related NuttX issues this PR addresses.
  2. Elaborate on Impact:
    • Even if no user-facing changes occur, explain why.
    • If internal code structure changed, mention potential future impacts.
  3. Provide Detailed Testing Information:
    • List all host and target environments used for testing.
    • Include relevant snippets from logs demonstrating the issue and its resolution.

By providing a comprehensive summary, outlining the impact, and including detailed testing information, you'll make it easier for reviewers to understand and approve your PR.

@github-actions github-actions bot added Size: M The size of the change in this PR is medium and removed Size: S The size of the change in this PR is small labels Oct 11, 2024
@yintao707 yintao707 changed the title rpmsg_sockif: add rpmsg_socket_ept_release for free conn rpmsg_sockif: add some modifaction for rpsocket Oct 11, 2024
@yintao707 yintao707 force-pushed the release_conn branch 3 times, most recently from 6e6f0c5 to c867e6f Compare October 12, 2024 11:51
yintao and others added 5 commits October 12, 2024 21:01
1.Simplify the message process in rpmsg_socket_send_single
2.Add more lock protection in rpmsg socket
3.Fix the style issue

Signed-off-by: Xiang Xiao <[email protected]>
expand the user's rpname size
hash when exceed RPMSG_SOCKET_NAME_LEN due to the limitation of eptname

Signed-off-by: yintao <[email protected]>
@xiaoxiang781216 xiaoxiang781216 merged commit d2fc3f2 into apache:master Oct 13, 2024
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Networking Effects networking subsystem Size: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants