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

Replace gethostbyname_r() with getaddrinfo() #1328

Merged
merged 1 commit into from
Jan 23, 2017

Conversation

jablko
Copy link
Contributor

@jablko jablko commented Jan 13, 2017

Fixes #1312

@atsci
Copy link

atsci commented Jan 13, 2017

FreeBSD build failed! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/1334/ for details.

@atsci
Copy link

atsci commented Jan 13, 2017

Linux build failed! See https://ci.trafficserver.apache.org/job/Github-Linux/1228/ for details.

@jablko jablko force-pushed the gethostbyname_r branch 2 times, most recently from 2209aff to 3a34b28 Compare January 13, 2017 22:50
@atsci
Copy link

atsci commented Jan 13, 2017

FreeBSD build failed! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/1335/ for details.

@atsci
Copy link

atsci commented Jan 13, 2017

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/1229/ for details.

@atsci
Copy link

atsci commented Jan 16, 2017

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/1230/ for details.

@atsci
Copy link

atsci commented Jan 16, 2017

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/1336/ for details.

@bgaff
Copy link
Member

bgaff commented Jan 17, 2017

Sure I'll review this very soon. Was this the final dependency for openbsd? I know when I was looking into it way back when it looked like this was only going to be one piece of it.

@jablko
Copy link
Contributor Author

jablko commented Jan 17, 2017

@bgaff Thanks! I messaged @jirib to see if building Traffic Server works for him now. This is the last of the four issues he reported, but yeah, I suspect there'll be more once his build gets past this point.

Either way, I think this change is productive, since 1) it moves a step closer to building on this platform -- OpenBSD doesn't implement gethostbyname_r(), 2) "The gethostbyname*() and gethostbyaddr*() functions are obsolete." at least according to [1]. "Applications should use getaddrinfo(3) and getnameinfo(3) instead.", and 3) this is the only remaining occurrence of gethostbyname_r(), whereas we already depend on getaddrinfo() -- it's used elsewhere in the code.

[1] https://linux.die.net/man/3/gethostbyname_r

@bgaff
Copy link
Member

bgaff commented Jan 17, 2017

This looks good to me! 👍

Thanks Jack

@bryancall bryancall added the DNS label Jan 18, 2017
@bryancall bryancall added this to the 7.2.0 milestone Jan 18, 2017
@bgaff
Copy link
Member

bgaff commented Jan 20, 2017

Shall I merge it?

@jirib
Copy link

jirib commented Jan 21, 2017

OpenBSD amd64 build passes this stage.

@atsci
Copy link

atsci commented Jan 23, 2017

Linux build failed! See https://ci.trafficserver.apache.org/job/Github-Linux/1253/ for details.

@atsci
Copy link

atsci commented Jan 23, 2017

FreeBSD build failed! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/1360/ for details.

@atsci
Copy link

atsci commented Jan 23, 2017

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/1361/ for details.

@atsci
Copy link

atsci commented Jan 23, 2017

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/1254/ for details.

@jablko jablko merged commit 582e049 into apache:master Jan 23, 2017
@jablko
Copy link
Contributor Author

jablko commented Jan 23, 2017

Thanks @bgaff and @jirib! Merged.

@zwoop
Copy link
Contributor

zwoop commented Jan 24, 2017

Are you guys comfortable with cherry-picking this to 7.1.0?

@bgaff
Copy link
Member

bgaff commented Jan 24, 2017

I dont think it's a good idea personally. The patch is trivial to backport and the only known benefit is openbsd support. IMO risk isn't worth the reward, just my two cents.

@jablko
Copy link
Contributor Author

jablko commented Jan 24, 2017

I agree with @bgaff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants