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

Fix #1172, Check Return Value for setsockopt #1174

Closed
wants to merge 1 commit into from

Conversation

himanshu007-creator
Copy link

Describe the contribution
A clear and concise description of what the contribution is.

  • Include explicitly what issue it addresses [e.g. Fixes #X]

Resolves #1172
Testing performed
Steps taken to test the contribution:

  1. Build steps '...'
  2. Execution steps '...'

Expected behavior changes
A clear and concise description of how this contribution will change behavior and level of impact.

  • API Change: xxx (if applicable)
  • Behavior Change: xxx (if applicable)
  • Or no impact to behavior

System(s) tested on

  • Hardware: [e.g. PC, SP0, MCP750]
  • OS: [e.g. Ubuntu 18.04, RTEMS 4.11, VxWorks 6.9]
  • Versions: [e.g. cFE 6.6, OSAL 4.2, PSP 1.3 for mcp750, any related apps or tools]

Additional context
Add any other context about the contribution here.

Third party code
If included, identify any third party code and provide text file of license

Contributor Info - All information REQUIRED for consideration of pull request
Full name and company/organization/center of all contributors ("Personal" if individual work)

  • If NASA Civil Servant Employee or GSFC Contractor on SES II
    • Address/email/phone and contract/task information (if applicable) must be on file
  • Else if Company
  • Else if Individual

@ArielSAdamsNASA ArielSAdamsNASA changed the title Check return value Fix #1172, Check Return Value for setsockopt Oct 6, 2021
@ArielSAdamsNASA
Copy link
Contributor

ArielSAdamsNASA commented Oct 6, 2021

Thank you for the contribution @himanshu007-creator. I would change the commit message to follow our format of Fix #XYZ, SHORT_DESCRIPTION as seen in our contributing guide. So, the updated commit message can be Fix #1174, Check Return Value of setsockopt.

@astrogeco astrogeco added CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) and removed CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) labels Nov 3, 2021
@astrogeco
Copy link
Contributor

Also try to fix the format differences found in https://github.com/nasa/osal/runs/3883153364?check_suite_focus=true
you can always run clang-format on the flagged files

@astrogeco
Copy link
Contributor

CCB:2021-11-03 Need to re-review

@astrogeco astrogeco added the CCB:Ignore Incomplete Pull Request with open actions. label Jun 10, 2022
@himanshu007-creator
Copy link
Author

Hi @astrogeco, any updated on this PR status?

@skliper
Copy link
Contributor

skliper commented Oct 14, 2022

See comments in #1172 (comment). @dmknutsen @dzbaker now manage the repo.

@skliper skliper removed the request for review from zanzaben October 14, 2022 12:32
@dzbaker dzbaker added CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) and removed CCB:Ignore Incomplete Pull Request with open actions. labels Oct 14, 2022
@dzbaker
Copy link
Collaborator

dzbaker commented Oct 14, 2022

Will review at next CCB to consider closing PR.

Copy link
Contributor

@jphickey jphickey left a comment

Choose a reason for hiding this comment

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

I don't see what this achieves here - it's calling setsockopt, and if successful, calls setsockopt again with the same option?

As noted in the comments, its OK if the call to setsockopt fails, maybe the implementation doesn't support that option or something - but the connection will still work without it, so there is no need to do full checking on this call, as there is no recovery or alternate action required if it fails.

Recommend closing this PR without merge.

@dzbaker
Copy link
Collaborator

dzbaker commented Oct 20, 2022

CCB 20 October 2022: Agreed to close without merging.

@dzbaker dzbaker closed this Oct 20, 2022
@dzbaker dzbaker added CCB:Ignore Incomplete Pull Request with open actions. and removed CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) labels Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CCB:Ignore Incomplete Pull Request with open actions. community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check Return Value for setsockopt
6 participants