kiwix-serve displays both protocol attached ips #704
Conversation
sgourdas
force-pushed
the
feature/serve-all-ips
branch
2 times, most recently
from
9afa991
to
0d43858
Compare
sgourdas
force-pushed
the
feature/serve-all-ips
branch
from
8126c6d
to
9f20116
Compare
kelson42
force-pushed
the
feature/serve-all-ips
branch
from
628b677
to
7372d88
Compare
Not yet. Comments from my previous review have not been addressed. |
src/server/kiwix-serve.cpp
Outdated
| kiwix::IpMode ipMode = (address == "all") ? kiwix::IpMode::all : | ||
| (address == "ipv6") ? kiwix::IpMode::ipv6 : | ||
| kiwix::IpMode::ipv4; |
There was a problem hiding this comment.
"all", "ipv6", & "ipv4" are special values for the --address option which can take a concrete IP address. If an IPv6 address (e.g. 2606:2800:21f:cb07:6820:80da:af6b:8b2c) is passed then this new logic will set ipMode to kiwix::IpMode::ipv4 which is wrong for downstream code.
There was a problem hiding this comment.
Essentially the logic remains the same as before. How it is setup is that the InternalServer::start is going to set the m_ipMode as ipv6 (previously all) if a valid v6 ip has been provided by the user.
There was a problem hiding this comment.
Maybe. But codewise it appears flawed. I think that the interaction of Server::setIpMode() and Server::setAddress() should be defined in such a way that the former takes precedence. In other words, if an IPv4 mode is requested then only IPv4 addresses should be accepted.
There was a problem hiding this comment.
So essentially we should:
- Remove all server mode handling and move it to kiwix-serve (removing also kiwix-serve.cpp:381).
- Set default mode (no mode through
-iprovided) toall(also, server.h:84 should be changed).
Correct?
There was a problem hiding this comment.
- Remove all server mode handling and move it to kiwix-serve
I don't know what is covered by this so cannot answer
- Set default mode (no mode through
-iprovided) toall
It will work, but IMHO it's better to add an extra enum member IpMode::auto
There was a problem hiding this comment.
Would IpMode::auto have any use case other than IpMode::all's?
There was a problem hiding this comment.
Or just an easier way for kiwix-serve to understand that no ip mode has been supplied?
There was a problem hiding this comment.
Would
IpMode::autohave any use case other thanIpMode::all's?
It was included in my original proposal for enhancing the kiwix-serve CLI with support for IPv6, but did not make way into implementation. Whatever the reason for that (you can find it in the ensuing discussion) it has a comeback at least in the C++ code.
Or just an easier way for kiwix-serve to understand that no ip mode has been supplied?
Mostly yes, but with some potential to find other uses later.
There was a problem hiding this comment.
Alright, I see how it can be useful in future scenarios. I will add it and make it the default mode for when an address is provided or no -i at all.
P.S. auto is not allowed as a name. Will use flex.
There was a problem hiding this comment.
To be honest it is a bit complicated with the current implementation to really put it in use, but I will wait for your next review after committing to discuss further.
Oops! It turns out that I failed to publish my review - it was in pending state since last Wednesday :( |
sgourdas
force-pushed
the
feature/serve-all-ips
branch
from
7372d88
to
b5e180b
Compare
| if (address.find(':') != std::string::npos) ipMode = kiwix::IpMode::ipv6; | ||
| else ipMode = kiwix::IpMode::ipv4; |
There was a problem hiding this comment.
Why don't you handle IpMode::flex in libkiwix?
| bool validMode4 = (ipMode == kiwix::IpMode::ipv4 || ipMode == kiwix::IpMode::all); // Mode is valid to support ipv4 | ||
| bool validMode6 = (ipMode == kiwix::IpMode::ipv6 || ipMode == kiwix::IpMode::all); // Mode is valid to support ipv6 |
There was a problem hiding this comment.
I don't understand why you need these. I think that Server::getAddress() must return only the addresses actually in use by the server. If it doesn't work that way - fix it in libkiwix.
This changes the logic to use the refactored
getBestPublicIpsso kiwix-serve can display all attached ips based on the protocol that was requested by the user.Fixes #703
Relies on kiwix/libkiwix#1132