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.

Sign up for GitHub

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

Jump to bottom

fix/eventfd: O_NONBLOCK #903

Closed
wants to merge 5 commits into from
Closed

fix/eventfd: O_NONBLOCK #903

wants to merge 5 commits into from

Conversation

skuenzer
Copy link
Member

Base target

  • Architecture(s): [all]
  • Platform(s): [all]
  • Application(s): [N/A]

Additional configuration

  • CONFIG_LIBPOSIX_EVENT=y

Description of changes

Commit fc96850 introduced an complete implementation of handling FNONBLOCK and FASYNC with fcntl, including forwarding of error codes to the caller. Because a standard EINVAL handler for ioctl was used for eventfd file descriptors, setting O_NONBLOCK caused a failing assertion.

@skuenzer skuenzer requested a review from a team as a code owner May 23, 2023 10:04
@skuenzer skuenzer force-pushed the skuenzer/fix/eventfd_nonblock branch from 3181c78 to 13a6204 Compare May 23, 2023 10:05
@skuenzer skuenzer added this to the v0.14.0 (Prometheus) milestone May 23, 2023
@razvand razvand requested review from John-Ted and removed request for a team May 23, 2023 10:14
@razvand razvand added area/lib Internal Unikraft Microlibrary lang/c Issues or PRs to do with C/C++ bug/fix This PR fixes a bug lib/posix-event POSIX event interface labels May 23, 2023
John-Ted
John-Ted approved these changes May 27, 2023
View reviewed changes
Copy link
Contributor

@John-Ted John-Ted left a comment

Choose a reason for hiding this comment

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

Both eventfd with EFD_NONBLOCK and fcntl with O_NONBLOCK now work, without any assertion fails.

Reviewed-by: Ioan-Teodor Teugea ioan_teodor.teugea@stud.acs.upb.ro

razvand
razvand requested changes Jun 1, 2023
View reviewed changes
lib/posix-event/eventfd.c Outdated Show resolved Hide resolved
@flpostolache
Copy link
Contributor

@skuenzer Thank you for this PR. There is a typo in the first commit message an complete implementation. Also, would it be better if we change the assertion in eventfd.c from UK_ASSERT(ret >= 0) to UK_ASSERT(ret == 0); to be more restrictive? 0 is the only value that can be returned in case of success when fcntl is used with F_SETFL. Otherwise, the PR looks good and works on my side.

@skuenzer skuenzer force-pushed the skuenzer/fix/eventfd_nonblock branch 2 times, most recently from 61e6126 to 3acb51f Compare June 7, 2023 12:15
@skuenzer
Copy link
Member Author

skuenzer commented Jun 7, 2023 •
edited

@skuenzer Thank you for this PR. There is a typo in the first commit message an complete implementation. Also, would it be better if we change the assertion in eventfd.c from UK_ASSERT(ret >= 0) to UK_ASSERT(ret == 0); to be more restrictive? 0 is the only value that can be returned in case of success when fcntl is used with F_SETFL. Otherwise, the PR looks good and works on my side.

Sure, I updated it. I was seeing it more relaxed that fcntl should just not fail. However, you are right, specification requirs it to return 0 in this case. 👍
I also fixed the typo that you found. Thanks for finding it.

flpostolache
flpostolache approved these changes Jun 12, 2023
View reviewed changes
Copy link
Contributor

@flpostolache flpostolache left a comment

Choose a reason for hiding this comment

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

Looks good! @razvand please have another look when you can.
Reviewed-by: Florin Postolache florin.postolache.of@gmail.com

@skuenzer
lib/syscall_shim: uk_prsyscall(): Signed decimal parameter format
4ddf1cb 
Introduces the parameter type `PT_DEC` as format for signed decimal
arguments.

Signed-off-by: Simon Kuenzer <simon@unikraft.io>
@skuenzer
lib/syscall_shim: uk_prsyscall(): Decode eventfd, eventfd2
ce1320b 
Adds the format definitions for the system calls`eventfd` and `eventfd2`
for decoding with `uk_prsyscall()`.

Signed-off-by: Simon Kuenzer <simon@unikraft.io>
@skuenzer
lib/posix-event: Debug config option
845e147 
Introduces a configuration option to enable debug messages for
`lib/posix-event`.

Signed-off-by: Simon Kuenzer <simon@unikraft.io>
@skuenzer
lib/posix-event: SYS_eventfd2: Use low-level fcntl syscall
a2bc0cf 
The eventfd constructor system call `eventfd2` uses the the `fcntl` system
call to setup the file descriptor flag `O_NONBLOCK` during creation. This
commit makes sure that the low-level implementation is directly called so
that there is no dependency to `errno`.

Checkpatch-Ignore: AVOID_EXTERNS
Checkpatch-Ignore: FUNCTION_ARGUMENTS
Signed-off-by: Simon Kuenzer <simon@unikraft.io>
@skuenzer
lib/posix-event: SYS_eventfd2: Fix assertion with O_NONBLOCK
737a7eb 
Commit
 fc96850 - lib/vfscore: Enable FIONBIO in fcntl(F_SETFL)
introduced a complete implementation of handling `FNONBLOCK` and `FASYNC`
with `fcntl`, including forwarding of error codes to the caller. Because
a standard `EINVAL` handler for `ioctl` was used for `eventfd` file
descriptors, setting `O_NONBLOCK` caused a failing assertion.

Checkpatch-Ignore: USE_NEGATIVE_ERRNO
Signed-off-by: Simon Kuenzer <simon@unikraft.io>
@skuenzer skuenzer force-pushed the skuenzer/fix/eventfd_nonblock branch from 3acb51f to 737a7eb Compare July 4, 2023 23:30
@unikraft-bot
Copy link
Member

✅ Checkpatch passed

Beep boop! I ran Unikraft's checkpatch.pl support script on your pull request and it all looks good!

SHA commit checkpatch
4ddf1cb lib/syscall_shim: uk_prsyscall(): Signed decimal parameter format ✅
ce1320b lib/syscall_shim: uk_prsyscall(): Decode `eventfd`, `eventfd2` ✅
845e147 lib/posix-event: Debug config option ✅
a2bc0cf lib/posix-event: `SYS_eventfd2`: Use low-level `fcntl` syscall ✅
Checkpatch-Ignore: AVOID_EXTERNS
Checkpatch-Ignore: FUNCTION_ARGUMENTS
737a7eb lib/posix-event: `SYS_eventfd2`: Fix assertion with O_NONBLOCK ✅
Checkpatch-Ignore: USE_NEGATIVE_ERRNO

@razvand razvand self-requested a review July 12, 2023 10:07
razvand
razvand approved these changes Jul 12, 2023
View reviewed changes
Copy link
Contributor

@razvand razvand left a comment

Choose a reason for hiding this comment

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

Approved-by: Razvan Deaconescu razvand@unikraft.io

unikraft-bot pushed a commit that referenced this pull request Jul 24, 2023
@skuenzer @unikraft-bot
lib/syscall_shim: uk_prsyscall(): Decode eventfd, eventfd2
18b9a95 
Adds the format definitions for the system calls`eventfd` and `eventfd2`
for decoding with `uk_prsyscall()`.

Signed-off-by: Simon Kuenzer <simon@unikraft.io>
Reviewed-by: Florin Postolache <florin.postolache.of@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #903
unikraft-bot pushed a commit that referenced this pull request Jul 24, 2023
@skuenzer @unikraft-bot
lib/posix-event: Debug config option
eaafc31 
Introduces a configuration option to enable debug messages for
`lib/posix-event`.

Signed-off-by: Simon Kuenzer <simon@unikraft.io>
Reviewed-by: Florin Postolache <florin.postolache.of@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #903
unikraft-bot pushed a commit that referenced this pull request Jul 24, 2023
@skuenzer @unikraft-bot
lib/posix-event: SYS_eventfd2: Use low-level fcntl syscall
e90a896 
The eventfd constructor system call `eventfd2` uses the the `fcntl` system
call to setup the file descriptor flag `O_NONBLOCK` during creation. This
commit makes sure that the low-level implementation is directly called so
that there is no dependency to `errno`.

Checkpatch-Ignore: AVOID_EXTERNS
Checkpatch-Ignore: FUNCTION_ARGUMENTS
Signed-off-by: Simon Kuenzer <simon@unikraft.io>
Reviewed-by: Florin Postolache <florin.postolache.of@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #903
unikraft-bot pushed a commit that referenced this pull request Jul 24, 2023
@skuenzer @unikraft-bot
lib/posix-event: SYS_eventfd2: Fix assertion with O_NONBLOCK
b9104d2 
Commit
 fc96850 - lib/vfscore: Enable FIONBIO in fcntl(F_SETFL)
introduced a complete implementation of handling `FNONBLOCK` and `FASYNC`
with `fcntl`, including forwarding of error codes to the caller. Because
a standard `EINVAL` handler for `ioctl` was used for `eventfd` file
descriptors, setting `O_NONBLOCK` caused a failing assertion.

Checkpatch-Ignore: USE_NEGATIVE_ERRNO
Signed-off-by: Simon Kuenzer <simon@unikraft.io>
Reviewed-by: Florin Postolache <florin.postolache.of@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #903
@unikraft-bot unikraft-bot added the ci/merged Merged by CI label Jul 24, 2023
i-Pear pushed a commit to i-Pear/unikraft that referenced this pull request Jul 27, 2023
@skuenzer @i-Pear
lib/syscall_shim: uk_prsyscall(): Signed decimal parameter format
0197021 
Introduces the parameter type `PT_DEC` as format for signed decimal
arguments.

Signed-off-by: Simon Kuenzer <simon@unikraft.io>
Reviewed-by: Florin Postolache <florin.postolache.of@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: unikraft#903
i-Pear pushed a commit to i-Pear/unikraft that referenced this pull request Jul 27, 2023
@skuenzer @i-Pear
lib/syscall_shim: uk_prsyscall(): Decode eventfd, eventfd2
84ff89a 
Adds the format definitions for the system calls`eventfd` and `eventfd2`
for decoding with `uk_prsyscall()`.

Signed-off-by: Simon Kuenzer <simon@unikraft.io>
Reviewed-by: Florin Postolache <florin.postolache.of@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: unikraft#903
i-Pear pushed a commit to i-Pear/unikraft that referenced this pull request Jul 27, 2023
@skuenzer @i-Pear
lib/posix-event: Debug config option
57aea92 
Introduces a configuration option to enable debug messages for
`lib/posix-event`.

Signed-off-by: Simon Kuenzer <simon@unikraft.io>
Reviewed-by: Florin Postolache <florin.postolache.of@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: unikraft#903
i-Pear pushed a commit to i-Pear/unikraft that referenced this pull request Jul 27, 2023
@skuenzer @i-Pear
lib/posix-event: SYS_eventfd2: Use low-level fcntl syscall
bdbb90e 
The eventfd constructor system call `eventfd2` uses the the `fcntl` system
call to setup the file descriptor flag `O_NONBLOCK` during creation. This
commit makes sure that the low-level implementation is directly called so
that there is no dependency to `errno`.

Checkpatch-Ignore: AVOID_EXTERNS
Checkpatch-Ignore: FUNCTION_ARGUMENTS
Signed-off-by: Simon Kuenzer <simon@unikraft.io>
Reviewed-by: Florin Postolache <florin.postolache.of@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: unikraft#903
i-Pear pushed a commit to i-Pear/unikraft that referenced this pull request Jul 27, 2023
@skuenzer @i-Pear
lib/posix-event: SYS_eventfd2: Fix assertion with O_NONBLOCK
bc1845b 
Commit
 fc96850 - lib/vfscore: Enable FIONBIO in fcntl(F_SETFL)
introduced a complete implementation of handling `FNONBLOCK` and `FASYNC`
with `fcntl`, including forwarding of error codes to the caller. Because
a standard `EINVAL` handler for `ioctl` was used for `eventfd` file
descriptors, setting `O_NONBLOCK` caused a failing assertion.

Checkpatch-Ignore: USE_NEGATIVE_ERRNO
Signed-off-by: Simon Kuenzer <simon@unikraft.io>
Reviewed-by: Florin Postolache <florin.postolache.of@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: unikraft#903
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@razvand razvand razvand approved these changes

@John-Ted John-Ted John-Ted approved these changes

@flpostolache flpostolache flpostolache approved these changes

Assignees

@razvand razvand

Labels
area/lib Internal Unikraft Microlibrary bug/fix This PR fixes a bug ci/merged Merged by CI lang/c Issues or PRs to do with C/C++ lib/posix-event POSIX event interface lib/syscall_shim
Projects
(Binary) Application Compatibility
Status: Done
Milestone
v0.14.0 (Prometheus)
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@skuenzer @flpostolache @unikraft-bot @razvand @John-Ted