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

docs: ares_set_local_ip4() byte order #368

Merged
merged 1 commit into from Nov 5, 2020

Conversation

dlundquist
Copy link
Contributor

Signed-off-by: Dustin Lundquist d.lundquist@tempered.io

Signed-off-by: Dustin Lundquist <d.lundquist@tempered.io>
@bradh352
Copy link
Member

bradh352 commented Nov 4, 2020

i haven't checked, but is one really host byte order and the other network byte order?

@dlundquist
Copy link
Contributor Author

dlundquist commented Nov 4, 2020

@bradh352 yes, the IPv4 version ends up calling htonl() in configure_socket(), hence updating the documentation.

https://github.com/c-ares/c-ares/blob/master/src/lib/ares_process.c#L1023

@bradh352
Copy link
Member

bradh352 commented Nov 4, 2020

yikes, hmm ... the docs for these aren't even up on https://c-ares.haxx.se/

@coveralls
Copy link

Coverage Status

Coverage remained the same at 88.625% when pulling e9814b2 on dlundquist:ares_set_local_ip4_docs into c15f403 on c-ares:master.

@bradh352
Copy link
Member

bradh352 commented Nov 4, 2020

looks like that was introduced about 10 years ago in e3b04e5 ...

I've never seen an ipv4 address stored in little endian.

@bagder ... i'd really love to break API/ABI compatibility to fix this one since its really braindead. That said, I do see curl calls this function (and of course calls ntohl() on the arg passed in).

@bagder
Copy link
Member

bagder commented Nov 5, 2020

(I fixed the omission and minor docs breakage on the website)

I'd really love to break API/ABI compatibility to fix this one since its really braindead

I agree that it is rather braindead behavior (and quite likely I am partially to blame as I didn't spot and react about it back in the day), but if we're going to break ABI for this (and I'm not really against it) I will just insist that we bump the SONAME and do it "correctly". The question is then only if it actually is worth the friction such a thing causes as it ripples out in the world.

bagder
bagder approved these changes Nov 5, 2020
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

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

As this is how the function currently works, I think this PR should go in as-is and the discussion on what to do about it going forward is slightly independent of this fix.

@bradh352
Copy link
Member

bradh352 commented Nov 5, 2020

heh, i'd actually vote to silently break ABI in this case :) I think API/ABI breakage is a no-go, if we were going to do that it would need to be something a lot bigger than something like this (which I doubt is used much). But yeah, you're right ... lets just document it the way it is.

@bradh352 bradh352 merged commit 3d4e80b into c-ares:master Nov 5, 2020
3 checks passed
@dlundquist dlundquist deleted the ares_set_local_ip4_docs branch March 24, 2021 16:50
sergepetrenko pushed a commit to tarantool/c-ares that referenced this pull request Jul 29, 2022
Properly document brain-dead behavior of ares_set_local_ip4() using host byte order instead of expected network byte order.

Fix By: Dustin Lundquist <d.lundquist@tempered.io>
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.

None yet

4 participants