Skip to content

Accept comments#292

Merged
rennergade merged 7 commits intodevelopfrom
accept-comments
Jul 18, 2024
Merged

Accept comments#292
rennergade merged 7 commits intodevelopfrom
accept-comments

Conversation

@davidge20
Copy link
Copy Markdown
Contributor

Description

Fixes # (issue)

All functions related to and including accept_syscall were commented out

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Will add test cases if necessary

Checklist:

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been added to a pull request and/or merged in other modules (native-client, lind-glibc, lind-project)

@davidge20
Copy link
Copy Markdown
Contributor Author

@ve1nard requesting your review, thanks

Comment thread src/safeposix/syscalls/net_calls.rs
@rennergade rennergade requested a review from pranav-bhatt July 8, 2024 16:17
@rennergade
Copy link
Copy Markdown
Contributor

@pranav-bhatt can you take a look at this one as well

Comment thread src/safeposix/syscalls/net_calls.rs Outdated
Comment thread src/safeposix/syscalls/net_calls.rs Outdated
Comment thread src/safeposix/syscalls/net_calls.rs
Comment thread src/safeposix/syscalls/net_calls.rs Outdated
Comment thread src/safeposix/syscalls/net_calls.rs Outdated
Comment thread src/safeposix/syscalls/net_calls.rs
Comment thread src/safeposix/syscalls/net_calls.rs
Comment thread src/safeposix/syscalls/net_calls.rs Outdated
Comment thread src/safeposix/syscalls/net_calls.rs Outdated
Comment thread src/safeposix/syscalls/net_calls.rs
Comment thread src/safeposix/syscalls/net_calls.rs
@davidge20
Copy link
Copy Markdown
Contributor Author

@rupeshkoushik07 requesting your review, thanks!

Comment thread src/safeposix/syscalls/net_calls.rs Outdated
Comment thread src/safeposix/syscalls/net_calls.rs Outdated
@davidge20 davidge20 requested a review from yashaswi2000 July 16, 2024 22:05
@davidge20
Copy link
Copy Markdown
Contributor Author

@yashaswi2000 requesting your review, thanks!

Copy link
Copy Markdown
Contributor

@yashaswi2000 yashaswi2000 left a comment

Choose a reason for hiding this comment

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

looks mostly good, we dont have any tests for this?

@rennergade
Copy link
Copy Markdown
Contributor

looks mostly good, we dont have any tests for this?

Yeah we should test for common errors. I think there are enough tests for cases where this works but worth spending a few minutes brainstorming edge cases.

Copy link
Copy Markdown
Contributor

@yashaswi2000 yashaswi2000 left a comment

Choose a reason for hiding this comment

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

LGTM

@rennergade rennergade merged commit f045767 into develop Jul 18, 2024
Anway-Agte pushed a commit that referenced this pull request Jul 18, 2024
* fn accept_unix commented

* Final comments

* updates based on pranav's and vlad's feedback

* removed unnessecary comments

* Added basic test to check for errors in accept sys call
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.

5 participants