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

Add getservbyname and getservbyport #204

Merged
merged 5 commits into from
Jul 4, 2021
Merged

Conversation

ahgamut
Copy link
Collaborator

@ahgamut ahgamut commented Jul 3, 2021

getservbyname and getservbyport have implementation similar to that of gethostbyname and gethostbyaddr.
output matches glibc, except that the aliases for a service (ie struct servent.s_aliases) are not filled.

There is no struct ServicesTxt like struct HostsTxt. For every call of getservbyname/getservbyport the lookup of /etc/services is done by opening the file and parsing each line one-by-one to compare. This is slow, but the implementation is simple; the file loading is similar to GetHostsTxt and the file parsing is similar to ParseHostsTxt.

The remaining functions are stubs in the musl source code, so I left them unimplemented here as well.

ahgamut added 3 commits July 4, 2021 05:06
getservbyname and getservbyport have implementation similar to that of
gethostbyname and gethostbyaddr. However, there is no struct ServicesTxt
like struct HostsTxt.

For every call of getservbyname/getservbyport the lookup of
/etc/services is done by opening the file and parsing each line
one-by-one. This is slow, but the implementation is simple, and the file
parsing is similar to ParseHostsTxt.
@ahgamut
Copy link
Collaborator Author

ahgamut commented Jul 4, 2021

@jart would you prefer a struct HostsTxt-style implementation? I skipped that because I saw TODOs for some of the internal stuff used for setting up struct HostsTxt (for eg. concat/append vs CONCAT/APPEND).

ahgamut added 2 commits July 4, 2021 19:43
when gethostbyname is provided an IP address in the form of a string, it
is supposed to write the address to h_name; the function instead was
writing an empty string. getaddrinfo does the correct thing by calling
inet_pton and returning on success, and the mistake is in gethostbyname.

The fix is to strlen(result->ai_canonname): If the inet_pton call in
getaddrinfo is successful, ai_canonname will point to valid memory
(non-NULL) but the string will be empty. in this case, the name (which
is an IP address string) will now correctly be copied to h_name of the
hostent structure.
using statically allocated values instead. also, the canonname check in
gethostbyname doesn't need strlen, just checking the first character to
be nonzero should be enough.
@ahgamut
Copy link
Collaborator Author

ahgamut commented Jul 4, 2021

What is the maximum length of a service name in /etc/services?
RFC 6335 says it is 15 characters, but this says it is 32.

If the bound is known, we can avoid malloc calls in the getservby* functions as well.

Copy link
Owner

@jart jart left a comment

Choose a reason for hiding this comment

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

Thanks for sending this! Looks good to merge.

I noticed there's no unit tests for this change. Here's how that could have worked:

#include "libc/testlib/testlib.h"

char testlib_enable_tmp_setup_teardown;

void SetUp(void) {
  int fd;
  const char *s = "\
# hello\n\
finger          79/tcp\n\
http            80/tcp          www             # WorldWideWeb HTTP\n";
  ASSERT_NE(-1, (fd = creat("hosts", 0755)));
  ASSERT_NE(-1, write(fd, s, strlen(s)));
  ASSERT_NE(-1, close(fd));
}

TEST(ftruncate, test) {
  ASSERT_EQ(80, LookupServicesByName("http", ptr, "hosts"));
}

libc/dns/servicestxt.c Show resolved Hide resolved
libc/dns/servicestxt.h Show resolved Hide resolved
libc/dns/getservbyport.c Show resolved Hide resolved
libc/dns/servicestxt.c Show resolved Hide resolved
libc/dns/servicestxt.c Show resolved Hide resolved
@jart
Copy link
Owner

jart commented Jul 4, 2021

@jart would you prefer a struct HostsTxt-style implementation? I skipped that because I saw TODOs for some of the internal stuff used for setting up struct HostsTxt (for eg. concat/append vs CONCAT/APPEND).

This is fine for now. We can revise it to have a global singleton as soon as it presents performance issues.

What is the maximum length of a service name in /etc/services?

Our malloc() is very fast but this does look like a case where it's not needed. If RFC6335 says it's 15 characters then it must be 15 characters. I'm fine enforcing that limit and having a char[16] buffer. But, due to how people are:

image

I would support your decision if you chose to make the buffer larger. It doesn't change the fact that the number is officially still 15. It's more of a Postel's Maxim kind of thing in being generous towards nonconformity. But in that case I'd argue that unlimited malloc is much better than defining another arbitrary but somewhat higher limit.

@jart jart merged commit 0ea2907 into jart:master Jul 4, 2021
@ahgamut
Copy link
Collaborator Author

ahgamut commented Jul 5, 2021

@jart I have pushed commit(s) to the servent-impl branch opened a new PR #207 after reading your comments.

I noticed there's no unit tests for this change. Here's how that could have worked:

Thanks for providing an outline for the testing.

I would support your decision if you chose to make the buffer larger. It doesn't change the fact that the number is officially still 15. It's more of a Postel's Maxim kind of thing in being generous towards nonconformity. But in that case I'd argue that unlimited malloc is much better than defining another arbitrary but somewhat higher limit.

My preference for a static buffer for struct servent.s_name over unlimited malloc is because

  • the similarity with gethostby* code, and
  • avoid the bookkeeping (like the free/strdup/free for localproto) to ensure names were being written correctly.
  • I tried using malloc everywhere (including aliases) at first and got stuck.

Currently I'm just using DNS_NAME_MAX as the size limit for struct servent.s_name .
I'm okay using malloc as well (in fact it is likely it will be needed for filling out the aliases).

ahgamut added a commit to ahgamut/cosmopolitan that referenced this pull request Jul 5, 2021
* use calloc instead malloc for s_aliases
* use strcasecmp instead of strcmp
* look through aliases when comparing for LookupServicesByName
* provide buffer for name in LookupServicesByName also
* provide a filepath param in LookupServicesBy* for testing
* use static buffers and DNS_NAME_MAX for s_name
ahgamut added a commit to ahgamut/cosmopolitan that referenced this pull request Jul 9, 2021
as mentioned by @jart in jart#204, the LookupServicesBy functions don't need
to handle network byte order, that can be done in getservbyname or
getservbyport. Changed the tests to also reflect this arrangement.
@ahgamut ahgamut mentioned this pull request Jul 12, 2021
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