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

Correct hostent_addr h_addrtype typespec #5412

Closed
wants to merge 1 commit into from

Conversation

varnerac
Copy link
Contributor

Fixes a broken spec for the hostent h_addrtype spec. It covers the results from this function head.

@CLAassistant
Copy link

CLAassistant commented Nov 17, 2021

CLA assistant check
All committers have signed the CLA.

@varnerac varnerac force-pushed the hostent_spec_fix branch 2 times, most recently from 56464be to 4153cd2 Compare November 17, 2021 19:08
@rickard-green rickard-green added the team:PS Assigned to OTP team PS label Nov 19, 2021
@KennethL KennethL added the types The issue is related to types label Nov 22, 2021
@IngelaAndin IngelaAndin added the stalled waiting for input by the Erlang/OTP team label Nov 30, 2021
Comment on lines +27 to +29
h_addrtype :: inet | inet6 | caa | cname | gid | hinfo | ns | mb | md | mg
| mf | minfo | mx | naptr | null | ptr | soa | spf | srv | txt
| uid | uinfo | unspec | uri | wks, %% host address type
Copy link
Contributor

@RaimoNiskanen RaimoNiskanen Jan 7, 2022

Choose a reason for hiding this comment

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

I suggest this should be:

h_addrtype :: inet | inet6 | inet_res:rr_type(),

and that inet_res:rr_type() should be exported.
This would be a bit incorrect since a and aaaa today cannot be in #hostent.h_addrtype, but future safer since it uses the type that the rest of inet_res uses for DNS record types, and future additions will be only to inet_res:rr_type().

Copy link
Contributor

@IngelaAndin IngelaAndin Jan 25, 2022

Choose a reason for hiding this comment

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

Would it perhaps make sense to have a inet_res:dns_sockaddr_record that could be exported and make the type h_addrtype :: inet | inet6 | inet_res:dns_sockaddr_record() and make inet_res:rr_type be
rr_type :: a | aaaa | dns_sockaddr_record() or for even more clarity
dns_ip_record:: a | aaaa and rr_type :: dns_ip_record() | dns_sockaddr_record() It could also be more clear to call rr_type dns_record_type , longer but much easier to understand!

Copy link
Contributor

Choose a reason for hiding this comment

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

inet_res:rr_type() should probably rather be named inet_res:dns_rr_type() similar to inet_res:dns_name() and inet_res:dns_class()...

Other than that I think that subdividing rr_type() would be artificial since it simply specifies the flat set of RR types.

The name dns_sockaddr_record is misleading since:

  • It is not a record - it is a record type or rather a Resource Record type, which within DNS is abbreviated RR, hence "RR Type". So "record" should be "resource_record_type" or rather an artificial subset thereof.
  • It has very little to do with a socket address ("sockaddr"). Normally, when a #hostent record is used, the field h_addrtype contains inet or inet6, which both are socket address types, a.k.a address families, so there is a connection between this field and a socket address type. But when the field is abused by inet_res:getbyname/2,3, it contains a DNS RR Type, and the content of a RR need not be an address - it can be a CNAME string, a TXT string and whatnot.

The problem is inet_res:getbyname/2,3 that returns the result as a #hostent record. That return format is only suitable for RR types a and aaaa that have the translations into socket address types inet and inet6. For all other RR types the #hostent record return value is unsuitable and is abused.

So inet_res:getbyname/2,3 creates the type problem and I see no good solution. The least bad I can think of is to type specify #hostent.h_addrtype to be the union of the address families that can be returned from gethosbyname(), i.e inet | inet6, and the DNS RR types, i.e inet_res:rr_type(). That two RR types are translated into socket address types by the function so they can never occur in the result, I think is a minor problem.

But, maybe we should change inet_res:rr_type() into inet_res:dns_rr_type() before exporting it...

Copy link
Contributor

Choose a reason for hiding this comment

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

For the record, I see I misunderstood. We have discussed this internally and will make a different solution to try to make the best of it.

@IngelaAndin IngelaAndin removed the stalled waiting for input by the Erlang/OTP team label Jan 26, 2022
@RaimoNiskanen
Copy link
Contributor

I have created an alternative PR #5803 that instead updates the inet_res:getbyname/2,3 return type.

RaimoNiskanen added a commit that referenced this pull request Mar 31, 2022
…c_fix/PR-5412/OTP-17986

Correct typespec for inet_res:getbyname/2,3 (PR #5412, OTP-17986)
@RaimoNiskanen
Copy link
Contributor

Fixed by PR #5803

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:PS Assigned to OTP team PS types The issue is related to types
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants