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.

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

Remove poll/select and eventfd stubs #23

Conversation

marcrittinghaus
Copy link
Member

@marcrittinghaus marcrittinghaus commented Aug 11, 2022

This commit removes the stubs for functions provided by the new posix-socket and posix-event libraries.

This PR is part of a larger group of dependent PRs which must be merged in the following order:

  1. Remove poll/select and eventfd stubs #23 (this PR)
  2. VFS Eventpoll API unikraft#484
  3. Introduce POSIX socket driver microlibrary unikraft#65
  4. Switch to lib/posix-socket lib-lwip#17
  5. Introduce lib/posix-event unikraft#485

⚠️ Without having merged all of these PRs, network is no longer functional. All PRs should thus be checked and reviewed first and then merged in quick succession⚠️

This commit removes the stubs for functions provided by the
new posix-socket and posix-event libraries.

Signed-off-by: Marc Rittinghaus <marc.rittinghaus@kit.edu>
@craciunoiuc
Copy link
Member

craciunoiuc commented Aug 11, 2022

All good here 👍

Waiting for more approval before adding the tag

@razvand razvand self-assigned this Aug 14, 2022
@razvand razvand added the enhancement New feature or request label Aug 14, 2022
@razvand
Copy link
Contributor

razvand commented Aug 14, 2022

Hi, @marcrittinghaus, this is my first pass. It seems OK, I have some questions:

Why were the STD..._FILENO macro defined in this file.c file in the first place?

What was the initial purpose of including the uk/plat/console.h and uk/print.h header files?

Does it make sense to still have the stdlib.h header file included in file.c?

@razvand razvand added this to the v0.10.0 (Phoebe) milestone Aug 14, 2022
@marcrittinghaus
Copy link
Member Author

Hi, @marcrittinghaus, this is my first pass. It seems OK, I have some questions:
Why were the STD..._FILENO macro defined in this file.c file in the first place?
What was the initial purpose of including the uk/plat/console.h and uk/print.h header files?

@razvand If you look at the history of the file you see that I once required all these definitions and headers. Over time more and more stubs were removed but these definitions remained.

Does it make sense to still have the stdlib.h header file included in file.c?

Looking at the man page realpath seems to be defined in stdlib.h although this is not the case in newlib. So technically we could remove it. I just kept it due to conformance with the man page.

@razvand
Copy link
Contributor

razvand commented Aug 16, 2022

@marcrittinghaus , all is OK. @craciunoiuc , please approve this PR and then I'll approve it as well. I'll be careful to do the approval in the order signaled by @marcrittinghaus .

Copy link
Member

@craciunoiuc craciunoiuc left a comment

Choose a reason for hiding this comment

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

All good on my side!

Reviewed-by: Cezar Craciunoiu cezar.craciunoiu@gmail.com

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.

Thanks, @marcrittinghaus . This works.

Reviewed-by: Razvan Deaconescu razvan.deaconescu@cs.pub.ro
Approved-by: Razvan Deaconescu razvan.deaconescu@cs.pub.ro

unikraft-bot pushed a commit that referenced this pull request Aug 16, 2022
This commit removes the stubs for functions provided by the
new posix-socket and posix-event libraries.

Signed-off-by: Marc Rittinghaus <marc.rittinghaus@kit.edu>
Reviewed-by: Cezar Craciunoiu <cezar.craciunoiu@gmail.com>
Reviewed-by: Razvan Deaconescu <razvan.deaconescu@cs.pub.ro>
Approved-by: Razvan Deaconescu <razvan.deaconescu@cs.pub.ro>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Pull-Request: #23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/merged enhancement New feature or request
Projects
Status: Done!
Development

Successfully merging this pull request may close these issues.

4 participants