-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Add source address binding to HTTP/HTTPS ClientSession (#1475) #2078
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
Conversation
I'm ok with IPv4-only for now. If anyone has IPv6 itch, they can scratch it :) |
@@ -0,0 +1,274 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since secure sockets are supported here, it properly belongs in NetSSL_OpenSSL/samples
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure I can move it there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aleks, did you retrigger running the checks? I remember them being successful last time I ran them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I see that you made some small changes (adding copyright notice and fixing tabs), must have retriggered the checked with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, can you also add CMakeLists.txt and Makefile for the sample - it should be simple, just take the ones from an existing example and modify the names accordingly.
OK, I moved the SetSourceIP example to sample directory in NetSSL_OpenSSL. First time that I update a branch like this and pleasantly surprised that the checks are run automatically. I thought I had to make a new force commit at same level. Lets hope checks pass. :-) |
One of the check jobs failed due to some scripting issue. Should I squash all changes into one commit and try again? |
Hi Aleks I received an email with comments from you about adding files CMakeLists.txt and Makefile for SetSourceIp example program. I can't seem to find your comments on the github site, so I can't answer them directly. Anyway, I tried to copy CMakeLists.txt from HTTPSTimeServer and make changes to it to refer to SetSourceIp where expected. Ran cmake on the file, seems to work, and it created a 'Makefile' file. Tried to 'make' the Makefile, but I get a lot of errors when compiling related to enabling C++ 11. Add the the following line in CMakeLists.txt Everything works with that addition. Not sure if forcing c++11 is OK? When I compile other samples I get same C++ 11 issues. Do I have too old or too new cmake or g++?!?! Any suggestions to move forward? |
I've made a new commit with CMakeLists.txt and Makefile changes. Not 100% sure how to properly test them. Would appreciate feedback. |
I think travis-ci might be stuck!! Is there a way to abort and restart the check jobs? |
@roccocorsi You can either restart the whole build or restart a single job. Click on the small icon on the right of each job or on the top left icon. |
I restarted the build. As for CMake, I will look at the issue, hopefully by the end of week. Generally speaking, yes - C++11 is ok in develop branch |
Sorry, been away for some time. Is there anything else that is needed from me on this commit to get it merge on develop branch? |
had to add VS projects, then had upstream conflict. reset, then commit again and @roccocorsi name in commits got lost. sorry, we like people to get proper credit for their contributions, but it's too much to go through the whole cycle again |
tried to amend to at least point to your contributions, ended up wth two commits and a merge, lol. anyway, thank you for the contribution :) |
Glad to hear that it is all merged. No problem if my name is not part of the commit. I might be back in a few weeks with IPv6 part, maybe... |
Commit will be part of Poco 2.0.0? No back porting, right? |
yes, if you need it in 1.x cherry-pick 26e952f and send a pull |
Any ideas when 2.0.0 might go stable? |
Have tried the changes from this pull request but still it doesn't work. Is there any plans to expose setsock option for secure connection directly to user for a secure connection? |
There is issue when we close connection and try to reconnect. Such session wont use correct source address.
because in first line we do:
Which will recreate new socket without binding. I'm working on fix.
|
New pull request with changes suggested by Günter.
I was thinking about this over the holidays and it is obvious that this is an IPv4 only solution. I'm not an IPv6 expert at all so not really sure what would be the proper way to make it IPv4 and IPv6 compliant, and how smart to make it.