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

fix: __sigval_t causes compile error. #21

Merged
merged 1 commit into from
Oct 30, 2022

Conversation

Super-long
Copy link
Contributor

Signed-off-by: Super-long [email protected]

I encountered the following problem when compiling:
截屏2022-10-25 20 29 35

__sigval_t has limitations, to take two different examples:
Linux VM-9-54-centos 5.4.119-1-tlinux4-0008 #1 SMP Fri Nov 26 11:17:45 CST 2021 x86_64 x86_64 x86_64 GNU/Linux
截屏2022-10-25 20 15 01

Linux VM-16-16-ubuntu 5.4.0-126-generic #142-Ubuntu SMP Fri Aug 26 12:12:57 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux
截屏2022-10-25 20 27 28

This change was made for better compatibility.

@romange
Copy link
Owner

romange commented Oct 25, 2022

Looks good, can you please add a comment saying that __sigval_t is not compatible with some linux distributions? Otherwise I will break it again at some point. @Super-long

@romange romange self-requested a review October 25, 2022 15:03
@Super-long
Copy link
Contributor Author

@romange Of course, this may take a bit of time, but I will add this information soon.

@Super-long
Copy link
Contributor Author

This continues to look like a Glibc version issue, mentioned in this document: sigval_t, although not a standard name, seems to be widely used outside of glibc.
https://patchwork.ozlabs.org/project/glibc/patch/[email protected]/

There is reason to believe that the new Glibc version will be compatible with this, and I also found the following code in glibc2.31:
截屏2022-10-30 18 53 59

So we should change __sigval_t to sigval_t.

@romange
Copy link
Owner

romange commented Oct 30, 2022

I agree. Can you just add a comment before DnsResolveNotify definition that says we must use sigval_t in order to support differrent glibc distributions besides ubuntu.

@Super-long
Copy link
Contributor Author

Of course, I added some comments to explain this change.

@romange romange merged commit 8987035 into romange:master Oct 30, 2022
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.

2 participants