Skip to content

added tests and comments for epoll syscalls#297

Merged
rennergade merged 7 commits intodevelopfrom
qianxi_epoll
Jul 17, 2024
Merged

added tests and comments for epoll syscalls#297
rennergade merged 7 commits intodevelopfrom
qianxi_epoll

Conversation

@qianxichen233
Copy link
Copy Markdown
Contributor

Description

This PR added some tests and comments for epoll related syscalls (epoll_create_syscall, epoll_ctl_syscall and epoll_wait_syscall).

Fixes # (issue)

Several minor issues in epoll are fixed in this PR:

  1. fixed some wrongly returned errno, and added a few errnos that can be easily supported
  2. added check for fd range to prevent panic from get_filedescriptor
  3. fixed when calling epoll_ctl on EPOLL_CTL_DEL flag with non-existing fd would causing panic
  4. fixed the error case that EINVAL should be returned when maxevents is smaller or equal 0 (previously is smaller 0).
  5. fixed misuse of EPOLLERR, should be EPOLLPRI instead
  6. fixed maxevents issue in epoll_wait. Previous behavior of maxevents is that it only checks the first maxevents fds for readability/writability. E.g. if there are 10 file descriptors, and 5 of them are ready. Passing maxevents of 5 would cause epoll only monitors the first 5 file descriptor, and report for its readability/writability. Therefore, the number of ready fd could be less than 5. However, the correct behavior should be that epoll should always report first 5 ready file descriptor, in this case, its return value should always be 5.

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?

All the fixed behavior passed the test. (before the fix, it won't pass the test)

  • Test A - ut_lind_net_epoll_create_bad_input
  • Test B - ut_lind_net_epoll_ctl_bad_input
  • Test C - ut_lind_net_epoll_wait_bad_input
  • Test D - ut_lind_net_epoll_maxevents_arg
  • Test E - ut_lind_net_epoll_timeout
  • Test F - ut_lind_net_epoll (this test is marked as ignore since an unfixed bug will panic the test)

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)

Copy link
Copy Markdown
Contributor

@ve1nard ve1nard left a comment

Choose a reason for hiding this comment

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

some changes requested

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 Outdated
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 Outdated
Comment thread src/safeposix/syscalls/net_calls.rs Outdated
Comment thread src/safeposix/syscalls/net_calls.rs Outdated
Comment thread src/tests/networking_tests.rs
Copy link
Copy Markdown
Contributor

@namanlalitnyu namanlalitnyu left a comment

Choose a reason for hiding this comment

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

Some minor doubts related to the variables.

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
Comment thread src/safeposix/syscalls/net_calls.rs
Comment thread src/tests/networking_tests.rs
Copy link
Copy Markdown
Contributor

@namanlalitnyu namanlalitnyu left a comment

Choose a reason for hiding this comment

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

Just one minor comment, but overall looks good!

Comment thread src/safeposix/syscalls/net_calls.rs
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.

few minor comments, good job with the extensive tests

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
@qianxichen233
Copy link
Copy Markdown
Contributor Author

@yashaswi2000 just resolved these comments, could you check again?

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

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

@rennergade rennergade left a comment

Choose a reason for hiding this comment

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

This is really thorough. Great job! One minor request.

@qianxichen233 qianxichen233 requested a review from rennergade July 17, 2024 03:10
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