Skip to content
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

Add support for multiple listen addresses #2604

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
10 participants
@ShadowNinja
Copy link
Member

ShadowNinja commented Apr 5, 2015

  • Ask the OS to assign an unused port instead of:
    • Using a static one (in tests, specifically 30001 and 30003)
    • Trying a random one that might be used (client outgoing connections)
  • ^ Fixes a few reported issues, but I only found #2344 with a cursory look.
  • Make banning normalize IPv6-mapped IPv4 addresses (so you can't try 1.2.3.4 and then ::ffff:1.2.3.4).
  • Fix some horrible main menu code (multiple lookups, no popping, using luaL_check* for table fields).
  • Move socket.cpp into network folder.
  • Some style fixes.
  • Lots of stuff that I don't remember right now.

To do:

  • Maybe figure out a nicer format for the --listen argument (eg --listen [::1]:0 --listen [::2]:0 instead of --listen '[::1]:0 [::2]:0')

To do later:

  • Fix how gamedata.port is stored as a string until just before it's used.
&state, &uptime, &ser_vers, &prot_vers,
&major, &minor, &patch, &vers_string))
&state, &uptime, &ser_vers, &prot_vers,
&major, &minor, &patch, &vers_string))

This comment has been minimized.

Copy link
@est31

est31 Apr 5, 2015

Contributor

why two tabs?

This comment has been minimized.

Copy link
@ShadowNinja

ShadowNinja Apr 5, 2015

Author Member

Because it looks nice to me that way.

@nerzhul

This comment has been minimized.

Copy link
Member

nerzhul commented Apr 5, 2015

@ShadowNinja for "Fix SQLite3 database statement initialization (because apparently that wasn't already done)." please push the fix with an atomic commit directly into master please and remove it from this PR

(m_connection->m_udpSocket.WaitData(50))) {
// First read packets from socket
// Check for incoming data available
while ((loop_count < 10) && wait(100 * 1000, readset)) {

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 5, 2015

Member

maybe we could use a g_setting instead of a static 100*1000 (and cache it). And also 100 sec timeout is too large. Please set 30sec instead, like a classic TCP connection

This comment has been minimized.

Copy link
@ShadowNinja

ShadowNinja Apr 6, 2015

Author Member

It's 100 * 1000 microseconds (us), that's 100 miliseconds (ms) or 0.1 seconds (s).
And it doesn't really make sense for this to be configurable. You can't really optimize anything by changing it.

@ShadowNinja ShadowNinja force-pushed the ShadowNinja:multi-socket branch 5 times, most recently from 562a572 to 62ed00a Apr 6, 2015

@ShadowNinja

This comment has been minimized.

Copy link
Member Author

ShadowNinja commented Apr 8, 2015

@ShadowNinja ShadowNinja force-pushed the ShadowNinja:multi-socket branch from 62ed00a to c4ef86d Apr 8, 2015

@ShadowNinja

This comment has been minimized.

Copy link
Member Author

ShadowNinja commented Apr 9, 2015

@nerzhul http://jenkins.unix-experience.fr/job/minetest%20-%20FreeBSD%20-%20Official/474/ is apparently a Jenkins or GitHub failure. Please re-build t.

src/ban.cpp Outdated
infostream<<"BanManager: failed loading from "<<m_banfilepath<<std::endl;
throw SerializationError("BanManager::load(): Couldn't open file");
}

while(!is.eof() && is.good())
{
while (is) {

This comment has been minimized.

Copy link
@kwolekr

kwolekr Apr 9, 2015

Contributor

Just because some standard classes have operator overloads doesn't mean you should use them! Isn't this less expressive than the former condition?

This comment has been minimized.

Copy link
@ShadowNinja

ShadowNinja Apr 9, 2015

Author Member

I find them about the same, but std::ios::good checks eofbit, so the eof check should be removed.

const SubgameSpec &gamespec, u16 port, std::string *address)
bool Game::createServer(const std::string map_dir,
const SubgameSpec &gamespec,
const std::string &listen)

This comment has been minimized.

Copy link
@kwolekr

kwolekr Apr 9, 2015

Contributor

Even if it's obvious that listen is an argument of the function in this context, we should still try to avoid shadowing symbols.

This comment has been minimized.

Copy link
@ShadowNinja

ShadowNinja Apr 9, 2015

Author Member

Huh? I'm shadowing something?

This comment has been minimized.

Copy link
@kwolekr

kwolekr Apr 9, 2015

Contributor

listen()

This comment has been minimized.

Copy link
@est31

est31 Oct 7, 2015

Contributor

And listen_addr would make the meaning more clear.

m_peer = 0;
}
}
if (!peer) return;

This comment has been minimized.

Copy link
@kwolekr

kwolekr Apr 9, 2015

Contributor

Please don't add conditional bodies on the same line as the statement unless you're trying to hide something... this violates the Linux code style guidelines anyway (which ours inherit)

@@ -199,63 +203,59 @@ void set_default_settings(Settings *settings)
settings->setDefault("fallback_font_shadow", "1");
settings->setDefault("fallback_font_shadow_alpha", "128");

std::stringstream fontsize;
fontsize << TTF_DEFAULT_FONT_SIZE;
std::string font_size_str = to_string(TTF_DEFAULT_FONT_SIZE);

This comment has been minimized.

Copy link
@kwolekr

kwolekr Apr 9, 2015

Contributor

How does this even compile? to_string() is C++11
Shouldn't you just use ## to stringify a numerical constant?

This comment has been minimized.

Copy link
@ShadowNinja

ShadowNinja Apr 9, 2015

Author Member
  1. std::to_string is C++11, ::to_string is defined in util/string.h.
  2. It's TOSTRING(), but yes, I probably should.

This comment has been minimized.

Copy link
@ShadowNinja

ShadowNinja Apr 10, 2015

Author Member

Actually TOSTRING() won't work, because *DEFAULT_FONT_SIZE are defined with parenthesis around their values and that's included in the preprocessor-generated string.

The definitions could probably be changed though, I can't think of any unexpected behavior that would result from that.

This comment has been minimized.

Copy link
@Zeno-

Zeno- Apr 10, 2015

Contributor

What's wrong with the original method (using stringstream)?

This comment has been minimized.

Copy link
@ShadowNinja

ShadowNinja Apr 26, 2015

Author Member

@Zeno-: Just that it's longer. A stringstream is still used internally by to_string.


// Copy data
switch (resolved->ai_family) {
case AF_INET: {

This comment has been minimized.

Copy link
@kwolekr

kwolekr Apr 9, 2015

Contributor

Shouldn't bodies inside of cases have their closing brace on a line separate line from the next case statement?
And cases shouldn't have bodies unless they actually need them (i.e. declares a variable inside)

This comment has been minimized.

Copy link
@ShadowNinja

ShadowNinja Apr 9, 2015

Author Member

I'm not sure if that's defined somewhere, but I'll remove the blocks (they're an artifact of a previous iteration of the code where they were necessary).

@@ -2089,12 +1981,57 @@ void * ConnectionReceiveThread::Thread()
return NULL;
}

bool ConnectionReceiveThread::wait(int timeout_us, fd_set &readset) {

This comment has been minimized.

Copy link
@kwolekr

kwolekr Apr 9, 2015

Contributor

Non-const reference parameter here

This comment has been minimized.

Copy link
@ShadowNinja

ShadowNinja Apr 9, 2015

Author Member

It's modified. Do you mean that you'd prefer a pointer parameter?

This comment has been minimized.

Copy link
@kwolekr

kwolekr Apr 9, 2015

Contributor

Yes. If it's being modified it should be passed as a pointer.
If it's more advantageous syntax-wise to treat it as a reference, there's nothing stopping you from doing T &fooref = *fooarg inside of the function body.

// Initialize the set
FD_ZERO(&readset);
int max_fd = 0;
for (std::vector<UDPSocket *>::const_iterator

This comment has been minimized.

Copy link
@kwolekr

kwolekr Apr 9, 2015

Contributor

Shouldn't this use array-iteration syntax? for (int i = 0; i != m_sockets.size(); i++) { m_sockets[i]->whatever(); ...

This comment has been minimized.

Copy link
@ShadowNinja

ShadowNinja Apr 9, 2015

Author Member

I don't suppose it matters?

This comment has been minimized.

Copy link
@kwolekr

kwolekr Apr 9, 2015

Contributor

Functionality-wise, no. But everywhere vectors are used (aside from the nerzhul list -> vector changes) they are treated as arrays.

This comment has been minimized.

Copy link
@ShadowNinja

ShadowNinja Apr 10, 2015

Author Member

Well, I guess it's less likely that you'll accidentally decrease performance more than necessary if you change the socket container type to a list this way. It's probably slightly less performant with a vector though. Either way's fine with me.

@@ -2986,7 +2942,7 @@ float Connection::getLocalStat(rate_stat_type type)
return retval;
}

u16 Connection::createPeer(Address& sender, MTProtocols protocol, int fd)
u16 Connection::createPeer(const Address &sender, size_t sock_id, MTProtocols protocol, int fd)

This comment has been minimized.

Copy link
@kwolekr

kwolekr Apr 9, 2015

Contributor

Is sock_id really supposed to be a size_t?

This comment has been minimized.

Copy link
@ShadowNinja

ShadowNinja Apr 9, 2015

Author Member

Well, it should be a std::vector<UDPSocket*>::size_type, but I figured size_t was close enough.

This comment has been minimized.

Copy link
@kwolekr

kwolekr Apr 9, 2015

Contributor

Okay then. That's good enough for me.

@kwolekr

This comment has been minimized.

Copy link
Contributor

kwolekr commented Apr 9, 2015

All of these changes are obviously welcome, but kind of wish it didn't have to be so large.

@ShadowNinja

This comment has been minimized.

Copy link
Member Author

ShadowNinja commented Apr 10, 2015

@nerzhul, your build broke again.

@nerzhul

This comment has been minimized.

Copy link
Member

nerzhul commented Apr 10, 2015

Looking at this today, i will update Jenkins plugins

@Zeno- Zeno- removed the Medium priority label Apr 10, 2015

ShadowNinja added some commits Oct 15, 2015

Add support for multiple listen addesses
  * Ask the OS to assign an unused port instead of using a static one
    (in tests, specifically `30001` and `30003`), or trying a random one that
    might be used (client outgoing connections).  This fixes issues with
    running more than one testing Minetest instance at the same time,
    test port collisions with running servers, and a very rare bind failure if
    the randomly chosen port was already used.
  * Fix some horrible main menu code (multiple lookups, no popping, using
    `luaL_check*` for table fields).
  * Move `socket.cpp` into `network` folder.
  * Some style fixes.

@ShadowNinja ShadowNinja force-pushed the ShadowNinja:multi-socket branch from 9f3b45d to 585fff2 Jan 13, 2017

@ShadowNinja

This comment has been minimized.

Copy link
Member Author

ShadowNinja commented Jan 13, 2017

Rebased.

@ShadowNinja ShadowNinja added the Feature label Jan 13, 2017

@ShadowNinja ShadowNinja force-pushed the ShadowNinja:multi-socket branch from 585fff2 to d17c6ba Jan 13, 2017

@ShadowNinja ShadowNinja referenced this pull request Jan 13, 2017

Closed

Server list cleanup #5031

@ShadowNinja ShadowNinja force-pushed the ShadowNinja:multi-socket branch from d17c6ba to c49dcb2 Jan 13, 2017

@ShadowNinja ShadowNinja referenced this pull request Jan 13, 2017

Closed

Main menu tweaks #5032

bool Address::isLoopback() const
{
switch (family) {
case AF_INET: return ntohl(host.v4.s_addr) == INADDR_LOOPBACK;

This comment has been minimized.

Copy link
@numberZero

numberZero Jan 18, 2017

Contributor

AFAIK the whole 127.0.0.0/8 network is defined as loopback. INADDR_LOOPBACK is just a single address in it (127.0.0.1).
Note: 127.0.0.2 works perfectly on Linux. Not sure about Windows, it may have some problems with that.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Apr 29, 2017

May be best to leave the refactor / code style stuff to another PR to make this easier? should at least be a separate commit.

@ShadowNinja ShadowNinja changed the title Add support for multiple listen addesses Add support for multiple listen addresses May 6, 2017

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Aug 13, 2017

@ShadowNinja please can you attend to this or close it? Old PR.
Please split off anything that can be into separate PRs so we can get those merged.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Sep 5, 2017

@ShadowNinja assuming adoption needed.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Sep 10, 2017

Anyone interested in adopting this? If so is any of this useful seeing how old it is and the conflicts?
Closing in 1 month unless there is a reason not to.

lua_pushnumber(L, min_rtt);
lua_settable(L, table);
lua_setfield(L, table, "min_rtt");

This comment has been minimized.

Copy link
@HybridDog

HybridDog Sep 10, 2017

Contributor

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Oct 7, 2017

IRC http://irc.minetest.net/minetest-dev/2017-10-07#i_5101918

18:24 paramat what about #2604 ? should we close due to amount of conflicts?
18:25 paramat i've marked all your open PRs for adoption
18:26 ShadowNinja paramat: I'd rather not. I think it's still usefull, and it should be merged at some point.
18:27 paramat it would have to be reworked, it's probably easier to start again from nothing due to conflicts
18:27 paramat it's only open to be seen for adoption

18:34 nerzhul let the multiple adress alone paramat as i'm working on rewriting network stack atm

@rubenwardy

This comment has been minimized.

Copy link
Member

rubenwardy commented Dec 4, 2017

This is very old with massive rebase problems, I also see this as a low-priority feature on the scale of things. If you'd like this then please reopen once you've updated and fixed any issues

@rubenwardy rubenwardy closed this Dec 4, 2017

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Dec 4, 2017

I support the close, no adoption interest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.