meetecho / janus-gateway

Janus WebRTC Server
https://janus.conf.meetecho.com
GNU General Public License v3.0
8.25k stars 2.48k forks source link

[1.x] Replace gethostbyname with getaddrinfo #3156

Closed bkmgit closed 1 year ago

bkmgit commented 1 year ago

What version of Janus is this happening on? Commit 963c4b6, current head of repository

Have you tested a more recent version of Janus too? No - issue affects future releases

Was this working before? Not applicable

Is there a gdb or libasan trace of the issue? Not applicable

Additional context When building for inclusion in Fedora, warning to replace gethostbyname with getaddrinfo is raised. This will allow use of IPv6 addresses. gethostbyname appears at:

getaddrinfo is used at:

Can make a pull request if of interest.

lminiero commented 1 year ago

When building for inclusion in Fedora, warning to replace gethostbyname with getaddrinfo is raised

Is this a new warning on Fedora 38? I don't seem to be getting those on my build system on Fedora 37. That said, I agree that a more consistent usage would be useful, and that switching to getaddrinfo everywhere helps. If you have a PR ready, that would be welcome, otherwise I can look into making the changes myself. Thanks for the feedback!

bkmgit commented 1 year ago

This warning arises from Fedora tooling for inclusion as a package, it is not a compiler warning. Can make a pull request within the next few days as would like to understand this a little better, but if you want to make the changes before then, that is fine.

lminiero commented 1 year ago

Ack, I'll have a look later today, to make sure such a change won't cause issues in existing features. As a side note, not sure if you'll attend FOSDEM in Brussels in a few days, but I'll be there, so in case I'd love to be able to say hello!

bkmgit commented 1 year ago

Only online.

lminiero commented 1 year ago

I just created a PR, linked above. It required many more changes than I originally anticipated, due to some assumptions the plugins were making on the address family of RTP streams. I plan to make some further changes before this is merged, but you may want to test this already, to check if the warnings you mentioned are solved.

renich commented 1 year ago

Will do. Thank you! :D

lminiero commented 1 year ago

Any feedback on the current state of the PR?

renich commented 1 year ago

Hey @lminiero, sorry for the super dalayed response. I'm gonna try and test this tomorrow morning and give you some feedback. Sorry for the huge delay once again.

renich commented 1 year ago

Hey @lminiero, in my test, it seems to fix the issue.

https://bugzilla.redhat.com/show_bug.cgi?id=2121585#c61

We will still go through Fedora's copr build system in order to see if it passes that test. copr will install all packages and carry out some additional tests and the report will be public. I'll share it once we're done.

renich commented 1 year ago

And it seems to work. No more warnings.

https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2121585-janus/fedora-rawhide-x86_64/05580064-janus/fedora-review/review.txt