Skip to content

curl_stream: use CURLINFO_ACTIVESOCKET if curl is recent enough#3996

Merged
carlosmn merged 1 commit into
libgit2:masterfrom
pks-t:pks/curl-lastsocket-deprecation
Nov 15, 2016
Merged

curl_stream: use CURLINFO_ACTIVESOCKET if curl is recent enough#3996
carlosmn merged 1 commit into
libgit2:masterfrom
pks-t:pks/curl-lastsocket-deprecation

Conversation

@pks-t

@pks-t pks-t commented Nov 11, 2016

Copy link
Copy Markdown
Member

The CURLINFO_LASTSOCKET information has been deprecated since
curl version 7.45.0 as it may result in an overflow in the
returned socket on certain systems, most importantly on 64 bit
Windows. Instead, a new call CURLINFO_ACTIVESOCKET has been
added which instead returns a curl_socket_t, which is always
sufficiently long to store a socket.

As the new call has only been introduced with version 7.45.0
which was released in October 2015, we cannot simply remove the
old code but instead are required to conditionally compile based
on libcurl's version.

@pks-t

pks-t commented Nov 11, 2016

Copy link
Copy Markdown
Member Author

While I don't like the conditional compile, there's no real alternative I could think of. We definitly cannot drop support for libcurl<7.45.0 but we also have to fix the issue if possible.

I also wanted to write a test for #3993, but we actually have no abstractions around socket, listen and accept and I didn't want to introduce them just for the sake of testing. We might as well just compile the test on POSIX-systems, but yeah...

@carlosmn

Copy link
Copy Markdown
Member

I'd rather check for the flag existing in the headers rather than the version of the library. I would also much prefer for us to create a #define for the curl type if the library version doesn't define it so we can write the code against as though we had the newer version available, and the defines take care of using the right value.

The `CURLINFO_LASTSOCKET` information has been deprecated since
curl version 7.45.0 as it may result in an overflow in the
returned socket on certain systems, most importantly on 64 bit
Windows. Instead, a new call `CURLINFO_ACTIVESOCKET` has been
added which instead returns a `curl_socket_t`, which is always
sufficiently long to store a socket.

As we need to provide backwards compatibility with curl versions
smaller than 7.45.0, alias CURLINFO_ACTIVESOCKET to
CURLINFO_LASTSOCKET on platforms without CURLINFO_ACTIVESOCKET.
@pks-t pks-t force-pushed the pks/curl-lastsocket-deprecation branch from 894bb22 to 5cbd526 Compare November 15, 2016 08:12
@pks-t

pks-t commented Nov 15, 2016

Copy link
Copy Markdown
Member Author

Well, this is how it looks like when #define'ing things instead. Probably looks a bit nicer. Would be nice if we could simply define things like "CURL_SOCKET_BAD" instead of introducing "GIT_CURL_BADSOCKET", but unfortunately these macros existed earlier than 7.45.0 and have wrong semantics on Windows platforms in that they work with Window's SOCKET type.

@carlosmn

Copy link
Copy Markdown
Member

Yeah, I do like this better. The version-dependent changes are all together in a single block rather than mixed together with real code.

@carlosmn carlosmn merged commit 1db3035 into libgit2:master Nov 15, 2016
@pks-t pks-t deleted the pks/curl-lastsocket-deprecation branch February 8, 2017 15:14
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