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

The setsockopt and getsockopt API diffs from BSD socket and WSA one #662

Merged
merged 5 commits into from
Aug 9, 2019

Conversation

dragonation
Copy link

The optval of SO_RCVTIMEO and SO_SNDTIMEO is a pointer to DWORD, not a pointer to struct timeval in Windows SDK.

ref:

@mnunberg
Copy link
Contributor

mnunberg commented May 6, 2019

This looks fine to me, though we don't ever use getsockopt in hiredis code

@dragonation
Copy link
Author

This looks fine to me, though we don't ever use getsockopt in hiredis code

But we use setsockopt, which will cause the real timeout divided by 1000 in windows, if we connect redis server with a specified timeout, and usually it will make the connection very unstable when testing the new windows port of hiredis.

sockcompat.c Outdated
int ret = setsockopt(sockfd, level, optname, (const char*)optval, optlen);
int ret = 0;
if ((level == SOL_SOCKET) && ((optname == SO_RCVTIMEO) || (optname == SO_SNDTIMEO))) {
struct timeval *tv = (struct timeval *)optval;
Copy link
Contributor

Choose a reason for hiding this comment

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

const struct timeval *tv = optval; eliminate casting plz

Copy link
Author

Choose a reason for hiding this comment

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

Did it

@mbitsnbites
Copy link
Contributor

I suggest that #663 is merged before this PR since it adds Travis builders for MinGW and MSVC.

If this branch is rebased on master after the merge of #663, we get CI checking of the Windows code.

@mbitsnbites
Copy link
Contributor

This looks fine to me, though we don't ever use getsockopt in hiredis code

Actually, it's used here: https://github.com/redis/hiredis/blob/master/net.c#L294, although only for SOL_SOCKET, SO_ERROR. However I think it's wise to have a correct implementation in sockcompat.c in case someone decides to use it later.

@michael-grunder
Copy link
Collaborator

Is this safe to merge? A few people have been having timeout related issues in Windows.

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.

4 participants