-
Notifications
You must be signed in to change notification settings - Fork 10
added more tests for socketpair syscall and fixed an issue with prior socketpair test #256
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! Tests are comprehensive. LGTM with a minor question
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall! A few minor comments / request for changes...
Good work on the tests! , Needs more documentation on what is intended and how we are testing. Missing comments on the original Socketpair syscall in net_calls.rs, and potential error checking improvements there. |
@davidge20 Could you review my socketpair PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I learned a lot from reading through your tests! Great work overall!
Just a note, I would appreciate more comments especially at the top of blocks of code that perform something. This would help me out a lot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/// On success, zero is returned. Otherwise, errors or panics are returned for | ||
/// different scenarios. | ||
/// | ||
/// ### Errors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a panic section after this, mentioning when can this function panic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not find any place that might cause panic currently. But I just wonder is the panic something that should occur inside the syscalls? I just feel like whenever something goes wrong, we should always return a syscall error instead of letting it panic directly. A panic inside the syscall would look like a bug to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm that's something to think about. ideally I agree with your line of thought, but the purpose of panic is kind of to say something went wrong when it shouldn't. And it might not be related to the logic of the Syscall. @rennergade your thoughts on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor changes requested from Yash and I. Once those are addressed I think this is good to go!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Approved.
Description
Fixes # (issue)
This PR added more tests for the socketpair syscall, and fixed an issue for prior socketpair test that could stuck the entire test in some cases, especially, when a test inside a non-main thread failed, the thread will panic and the main thread will never be able to return from recv_syscall as it is waiting message from an already dead thread. Fix involves creating dedicated thread for each peer and using non-blocking thread join method. Besides the fix of the old test, below is the new tests cased being added:
Type of change
How Has This Been Tested?
This PR is a test itself, and currently all the tests were passed.
Checklist: