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

wasi-sockets: Add services database and implement getservbyname/getservbyport functions #532

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Henkru
Copy link
Contributor

@Henkru Henkru commented Sep 5, 2024

This is a follow-up PR for #524 by implementing the following features:

  • Embeds a minimal network services database with 17 common protocols, suggested by @badeend. The database array is a weak symbol allowing applications to override the default database at link time.
  • Updates getaddrinfo to resolve named services in the address info. For example getaddrinfo("google.com", "https", NULL, &res);
  • Implements the getservbyname and getservbyport functions. These functions are implemented using a static variable (global_serv), which holds the returned service entry. This approach is acceptable because these functions are defined as not being thread-safe.

Additionally, this PR introduces an optional, more comprehensive services database (sockets_full_services_db.c), based on Debian Bookworm's /etc/services file (320 entries). To use this database, link with the -lc-full-services-db flag.

@Henkru
Copy link
Contributor Author

Henkru commented Sep 5, 2024

Initially, I reviewed the macOS's services file, which contained approximately 10,000 entries. Based on this, I estimated the required space for the embedded services database to be around 70 kB. However, after reviewing Debian's services file, which has only about 320 entries, I found that it occupies merely 4 kB. So we could include it by default. However, I still like the minimal db approach better, but I'd also like to hear your opinions.

Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Nice work!

I wonder if we want to bother with the complexity of including a "full" version here? How many folks will actually find that useful?

Also, I wonder if the name "full" is correct, since even with ~300 entries its still a subset of what it could be. If we really do want a "full" version should use the filesystem to actually load /etc/services? Maybe not.

@Henkru
Copy link
Contributor Author

Henkru commented Sep 5, 2024

That's valid point. One option could be to remove the bigger optional db, which keeps things simple, but leave the weak symbol definition. So, if application requires specific db, it can still override the default one.

@sbc100
Copy link
Member

sbc100 commented Sep 5, 2024

That's valid point. One option could be to remove the bigger optional db, which keeps things simple, but leave the weak symbol definition. So, if application requires specific db, it can still override the default one.

That simpler option SGTM

@Henkru Henkru force-pushed the feat/network-services branch from b9d1fad to 84f6f6f Compare September 6, 2024 18:59
@Henkru
Copy link
Contributor Author

Henkru commented Sep 6, 2024

That's valid point. One option could be to remove the bigger optional db, which keeps things simple, but leave the weak symbol definition. So, if application requires specific db, it can still override the default one.

That simpler option SGTM

I removed the bigger db from the PR.

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.

2 participants