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

[1.x] Replace gethostbyname with getaddrinfo #3156

Closed
bkmgit opened this issue Jan 31, 2023 · 10 comments
Closed

[1.x] Replace gethostbyname with getaddrinfo #3156

bkmgit opened this issue Jan 31, 2023 · 10 comments
Labels
multistream Related to Janus 1.x pr-exists

Comments

@bkmgit
Copy link
Contributor

bkmgit commented Jan 31, 2023

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.

@bkmgit bkmgit added the multistream Related to Janus 1.x label Jan 31, 2023
@lminiero
Copy link
Member

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
Copy link
Contributor Author

bkmgit commented Jan 31, 2023

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
Copy link
Member

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
Copy link
Contributor Author

bkmgit commented Jan 31, 2023

Only online.

@lminiero
Copy link
Member

lminiero commented Feb 1, 2023

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
Copy link

renich commented Feb 1, 2023

Will do. Thank you! :D

@lminiero
Copy link
Member

Any feedback on the current state of the PR?

@renich
Copy link

renich commented Feb 28, 2023

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
Copy link

renich commented Feb 28, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multistream Related to Janus 1.x pr-exists
Projects
None yet
Development

No branches or pull requests

3 participants