Skip to content

pcm-sensor-server: add bind ip argument #938

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

pcm-sensor-server: add bind ip argument #938

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
be10f70
pcm-sensor-server: add bind ip argument
marenz2569 Apr 21, 2025
df18c34
pcm-sensor-server: use -l for bind ip
marenz2569 Apr 21, 2025
6d2d67d
pcm-sensor-server: update docs for ip6 address bind
marenz2569 Apr 21, 2025
02e79be
Merge branch 'master' into marenz.pcm_sensor_server_bind_ip
marenz2569 Apr 24, 2025
949cbbe
fix ipv6 formatting and remame variables to highlight that this is no…
marenz2569 Apr 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 19 additions & 8 deletions src/pcm-sensor-server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3686,8 +3686,8 @@ void my_get_callback( HTTPServer* hs, HTTPRequest const & req, HTTPResponse & re
}
}

int startHTTPServer( unsigned short port ) {
HTTPServer server( "", port );
int startHTTPServer( std::string const & ip, unsigned short port ) {
HTTPServer server( ip, port );
try {
// HEAD is GET without body, we will remove the body in execute()
server.registerCallback( HTTPRequestMethod::GET, my_get_callback );
Expand All @@ -3701,8 +3701,8 @@ int startHTTPServer( unsigned short port ) {
}

#if defined (USE_SSL)
int startHTTPSServer( unsigned short port, std::string const & cFile, std::string const & pkFile) {
HTTPSServer server( "", port );
int startHTTPSServer( std::string const & ip, unsigned short port, std::string const & cFile, std::string const & pkFile) {
HTTPSServer server( ip, port );
try {
server.setPrivateKeyFile ( pkFile );
server.setCertificateFile( cFile );
Expand All @@ -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";
Copy link
Contributor

Choose a reason for hiding this comment

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

I still prefer using -i and or use a long option --ipv6 or so. What about ipv4? Why only ipv6?

Copy link
Author

@marenz2569 marenz2569 Apr 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@ogbrugge ogbrugge Apr 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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...

std::cout << " -p portnumber : Run on port <portnumber> (default port is " << DEFAULT_HTTP_PORT << ")\n";
std::cout << " -r|--reset : Reset programming of the performance counters.\n";
std::cout << " -D|--debug level : level = 0: no debug info, > 0 increase verbosity.\n";
Expand Down Expand Up @@ -3764,6 +3765,7 @@ int mainThrows(int argc, char * argv[]) {
#endif
bool forceRTMAbortMode = false;
bool printTopology = false;
std::string ip6Address = "";
unsigned short port = 0;
unsigned short debug_level = 0;
std::string certificateFile;
Expand All @@ -3788,6 +3790,14 @@ int mainThrows(int argc, char * argv[]) {
for ( int i=1; i < argc; ++i ) {
if ( check_argument_equals( argv[i], {"-d"} ) )
daemonMode = true;
else if ( check_argument_equals( argv[i], {"-l"} ) )
{
if ( (++i) < argc ) {
ip6Address = argv[i];
Copy link
Contributor

Choose a reason for hiding this comment

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

Still no validation of the user input, see previous comment for how you could do this

Copy link
Author

Choose a reason for hiding this comment

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

the comment about user input validation seems to be missing?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

} else {
throw std::runtime_error( "main: Error no bind ip argument given" );
}
}
else if ( check_argument_equals( argv[i], {"-p"} ) )
{
if ( (++i) < argc ) {
Expand Down Expand Up @@ -4072,19 +4082,20 @@ int mainThrows(int argc, char * argv[]) {
}

// Now that everything is set we can start the http(s) server
const auto formattedIp6Address = ip6Address.length() > 0 ? "[" + ip6Address + "]" : "localhost";
#if defined (USE_SSL)
if ( useSSL ) {
if ( port == 0 )
port = DEFAULT_HTTPS_PORT;
std::cerr << "Starting SSL enabled server on https://localhost:" << port << "/\n";
startHTTPSServer( port, certificateFile, privateKeyFile );
std::cerr << "Starting SSL enabled server on https://" << formattedIp6Address << ":" << port << "/\n";
startHTTPSServer( ip6Address, port, certificateFile, privateKeyFile );
} else
#endif
{
if ( port == 0 )
port = DEFAULT_HTTP_PORT;
std::cerr << "Starting plain HTTP server on http://localhost:" << port << "/\n";
startHTTPServer( port );
std::cerr << "Starting plain HTTP server on http://" << formattedIp6Address << ":" << port << "/\n";
startHTTPServer( ip6Address, port );
}
delete pcmInstance;
} else if ( pid > 0 ) {
Expand Down
Loading