Skip to content

Parse IPv6 scope IDs. #923

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 6, 2019
Merged

Parse IPv6 scope IDs. #923

merged 1 commit into from
Nov 6, 2019

Conversation

PhilipHomburg
Copy link
Contributor

I added support for parsing scope IDs in IPv6 literals. This is required to support link local addresses in /etc/resolv.conf

azat
azat requested changes Oct 29, 2019
View reviewed changes
Copy link
Member

@azat azat left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!
There are some minor comments, and also the unit test is missing, can address this?

ygj6
ygj6 reviewed Oct 30, 2019
View reviewed changes
ygj6
ygj6 reviewed Oct 30, 2019
View reviewed changes
@PhilipHomburg
Copy link
Contributor Author

I'll make the requested changes. Though I have no clue about Windows. Maybe some can check if if_nametoindex exists on windows.

@PhilipHomburg
Copy link
Contributor Author

It is not clear to me why the AppVeyor build fails

@azat
Copy link
Member

azat commented Oct 30, 2019 via email

azat
azat reviewed Oct 30, 2019
View reviewed changes
test/regress_util.c
((ev_uint32_t)in6.s6_addr[j*4+1] << 16) |
((ev_uint32_t)in6.s6_addr[j*4+2] << 8) |
((ev_uint32_t)in6.s6_addr[j*4+3]);
if (u != ent->res[j]) {
Copy link
Member

Choose a reason for hiding this comment

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

ok for now, but should be adjusted later

@azat
Copy link
Member

azat commented Oct 30, 2019

It is not clear to me why the AppVeyor build fails

It is not your fault, will take a look

Looks like the appveyor issue, after restarting the build everything is fine.

@PhilipHomburg
Copy link
Contributor Author

It seems that including <iphlpapi.h> doesn't result in a prototype for if_nametoindex. Maybe somebody with windows can check.

@PhilipHomburg
Copy link
Contributor Author

Ah, not only no prototype, but the function doesn't exist at all.

@PhilipHomburg
Copy link
Contributor Author

I'm done with this. I don't want to make random changes for windows. Somebody else can sort this out. I'll drop the pull request soon.

@ygj6
Copy link
Member

ygj6 commented Oct 31, 2019

@PhilipHomburg , I have Windows machine, I'll check it, please keep going.

@ygj6
Copy link
Member

ygj6 commented Nov 1, 2019

You should include netioapi.h or iphlpapi.h, and link iphlpapi.lib when using if_nametoindex(), like this:
in CMakeLists:

if (${EVENT_LIBRARY_STATIC})
    target_link_libraries(event_core_static iphlpapi)
    target_link_libraries(event_static iphlpapi)
endif()
if (${EVENT_LIBRARY_SHARED})
    target_link_libraries(event_core_shared iphlpapi)
    target_link_libraries(event_shared iphlpapi)
endif()

in Makefile.am line 185

SYS_LIBS = -lws2_32 -lshell32 -ladvapi32 -liphlpapi

@PhilipHomburg
Copy link
Contributor Author

Just send me a diff, or create a new pull request with that change

@azat
Copy link
Member

azat commented Nov 5, 2019

@ygj6 any news on this one? (you can simply pull the changes from this PR and fix win32 support)

@ygj6
Copy link
Member

ygj6 commented Nov 6, 2019

I pushed this patch to the branch

And you can view the CI result here

@azat azat merged commit 9fecb59 into libevent:master Nov 6, 2019
@azat
Copy link
Member

azat commented Nov 6, 2019

Applied, thanks! (@PhilipHomburg I force pushed rebased changes to your repo to force github merge detection)

azat added a commit to azat/libevent that referenced this pull request Nov 6, 2019
@azat
Link with iphlpapi only on windows
1495f8b 
Fixes: 9fecb59 ("Parse IPv6 scope IDs.")
Refs: libevent#923
azat added a commit to azat/libevent that referenced this pull request Jun 28, 2020
@azat
Link with iphlpapi only on windows
976f7d3 
Fixes: 9fecb59 ("Parse IPv6 scope IDs.")
Refs: libevent#923
(cherry picked from commit 1495f8b)
@azat azat mentioned this pull request Sep 14, 2020
Closed
@edo9300 edo9300 mentioned this pull request Oct 19, 2020
Open
hebasto added a commit to hebasto/libevent that referenced this pull request Apr 22, 2024
@hebasto
build: Add missed Iphlpapi library to *.pc files on Windows
8edd579 
It is required since libevent#923.
hebasto added a commit to hebasto/libevent that referenced this pull request Apr 22, 2024
@hebasto
build: Add missed Iphlpapi library to *.pc files on Windows
31c593c 
It is required since libevent#923.
hebasto added a commit to hebasto/libevent that referenced this pull request Apr 22, 2024
@hebasto
build: Add Iphlpapi to Libs.private in *.pc files on Windows
6b1db88 
It has been required since libevent#923
at least for the `if_nametoindex` call.
@hebasto hebasto mentioned this pull request Apr 22, 2024
Merged
azat pushed a commit that referenced this pull request Apr 25, 2024
@hebasto @azat
build: Add Iphlpapi to Libs.private in *.pc files on Windows
71d41cd 
It has been required since #923
at least for the `if_nametoindex` call.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@azat azat azat requested changes

+1 more reviewer

@ygj6 ygj6 ygj6 left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@PhilipHomburg @azat @ygj6