Skip to content

Listen sys call comments + tests#298

Merged
rennergade merged 9 commits intodevelopfrom
listen-comments
Jul 17, 2024
Merged

Listen sys call comments + tests#298
rennergade merged 9 commits intodevelopfrom
listen-comments

Conversation

@davidge20
Copy link
Copy Markdown
Contributor

@davidge20 davidge20 commented Jul 10, 2024

Description

Added comments to listen_syscall and added two tests

Fixes # (issue)

Two issues that I would like to share:

  1. While calling the listen sys call, the backlog is set to 5 in the function, so the test queues more than 5 connections. The 6th connection should return with an error, given the queue is maximized. However, the 6th queue returns with EINPROGRESS.
  2. We create a blocking socket to connect to a "server" and it returns immediately. The blocking socket should not return with success immediately as the server did not accept the connection.

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?

  • Test A - ut_lind_net_listen_more_than_backlog()
  • Test B - ut_lind_net_listen_nonblock_then_block()

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 davidge20 changed the title Listen comments + tests Listen sys call comments + tests Jul 10, 2024
qianxichen233
qianxichen233 reviewed Jul 10, 2024
View reviewed changes
Comment thread rustc-ice-2024-07-10T16_06_47-19579.txt 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 Outdated
Comment thread src/tests/networking_tests.rs Outdated
Comment thread src/tests/networking_tests.rs Outdated
namanlalitnyu
namanlalitnyu reviewed Jul 11, 2024
View reviewed changes
Comment thread src/safeposix/syscalls/net_calls.rs
@davidge20 davidge20 mentioned this pull request Jul 14, 2024
Open
@davidge20
Copy link
Copy Markdown
Contributor Author

davidge20 commented Jul 16, 2024

@rupeshkoushik07 requesting your review, thanks!

namanlalitnyu
namanlalitnyu approved these changes Jul 16, 2024
View reviewed changes
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.

LGTM!

rupeshkoushik07
rupeshkoushik07 approved these changes Jul 16, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@rupeshkoushik07 rupeshkoushik07 left a comment

Choose a reason for hiding this comment

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

LGTM !

yashaswi2000
yashaswi2000 approved these changes Jul 16, 2024
View reviewed changes
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

Comment thread src/tests/networking_tests.rs Outdated
Comment thread src/safeposix/syscalls/net_calls.rs Outdated
rennergade
rennergade reviewed Jul 16, 2024
View reviewed changes
Comment thread src/safeposix/syscalls/net_calls.rs
rennergade
rennergade reviewed Jul 16, 2024
View reviewed changes
Comment thread src/tests/networking_tests.rs Outdated
rennergade
rennergade requested changes Jul 16, 2024
View reviewed changes
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 very close and quite thorough. Good job!

A few minor changes, lets add the backlog and make sure that test is working.

@rennergade rennergade merged commit 66b24fa into develop Jul 17, 2024
Anway-Agte pushed a commit that referenced this pull request Jul 18, 2024
@davidge20
Listen sys call comments + tests (#298)
55a736a 
* Initial header

* Final comments

* Updated tests without necessary files

* Updates based on initial feedback

* Without ice files

* Removed bad test and updated code

* added backlog to INET and updated tests

* fixed backlog test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rennergade rennergade rennergade approved these changes

@yashaswi2000 yashaswi2000 yashaswi2000 approved these changes

@qianxichen233 qianxichen233 qianxichen233 approved these changes

+2 more reviewers

@rupeshkoushik07 rupeshkoushik07 rupeshkoushik07 approved these changes

@namanlalitnyu namanlalitnyu namanlalitnyu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@davidge20 @rennergade @yashaswi2000 @qianxichen233 @rupeshkoushik07 @namanlalitnyu
⚡