pcm-sensor-server: add bind ip argument #938
Conversation
marenz2569
force-pushed
the
marenz.pcm_sensor_server_bind_ip
branch
from
98c48f0 to
6d2d67d
Compare
|
@marenz2569 thank you for submitting this PR. Unfortunately a merge conflict has appeared. Could you please resolve it and resubmit your PR? |
…t a hostname, but an ipv6 address
|
@rdementi Hi, I resolved the merge conflict and updated the variable names to highlight that this is ipv6 only |
| else if ( check_argument_equals( argv[i], {"-l"} ) ) | ||
| { | ||
| if ( (++i) < argc ) { | ||
| ip6Address = argv[i]; |
There was a problem hiding this comment.
Still no validation of the user input, see previous comment for how you could do this
There was a problem hiding this comment.
the comment about user input validation seems to be missing?
There was a problem hiding this comment.
I wrote a comment 2 days ago but I guess it never published. I wold like you to do some validation on the string you get from the user to make sure it is valid before trying to pass it around and then error out.
The URL class has some code to do IPv4 and IPv6 validation, you could take the code from that, put it in some static functions or refactor an IP class out of that and use that.
| @@ -3726,6 +3726,7 @@ void printHelpText( std::string const & programName ) { | |||
| #if defined (USE_SSL) | |||
| std::cout << " -s : Use https protocol (default port " << DEFAULT_HTTPS_PORT << ")\n"; | |||
| #endif | |||
| std::cout << " -l bind ip6 : Bind to ip6 address. (default ip6 is ::)\n"; | |||
There was a problem hiding this comment.
I still prefer using -i and or use a long option --ipv6 or so. What about ipv4? Why only ipv6?
There was a problem hiding this comment.
I would need to refactor the HTTPServer and Server class to both accept ipv4 and/or ipv6. Currently the Server class uses ipv6 only internally.
There was a problem hiding this comment.
Server class uses dual IP stack properties on Linux, no need to be specific.
Anyway you use a string to pass it around, what refactoring would that require?
But before we continue this, why exactly do you need to pass an IPv6 address in here? By default it should take all addresses, ipv4 and ipv6, is this not working for you?
There was a problem hiding this comment.
As you said, currently the server listens on ::. This is working.
I'd like to specify the address the server is listening on. To the best of my knowledge I would need to specify AF_INET to listen on a specific v4 address or AF_INET6 to listen on a specific v6 address. Currently the Server class is using AF_INET6 internally.
There was a problem hiding this comment.
IPv4 addresses can be represented as ipv6 addresses, see RFC 4291 for details but it looks like ::ffff:d.d.d.d, d.d.d.d is the ipv4 address just like normal using decimal format, not hexadecimal like ipv6. This would be one way to make it listen on an ipv4 address while still using AF_INET6 but i am not sure this is needed. Going the route of ipv4 mapped ipv6 addresses means the URL IPv6 validator needs to be fixed, it does not account for this format. That is an oversight on my part when writing that IPv6 validation in URL.
There was a problem hiding this comment.
Caveat: I have not tested my IPv6 code on a system that does not support IPv6 (are there still some around?) so i do not know what :: using AF_INET6 does on such a system that can only handle IPv4...
This PR adds support to
pcm-sensor-serverto bind to a specifed ip6 instead of::.