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.

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

Let the OS choose the server port when the config doesn't have one. #504

Merged
merged 2 commits into from
Nov 3, 2015
Merged

Let the OS choose the server port when the config doesn't have one. #504

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
41db4c1
Let the OS choose the server port whenever it's not given.
Oct 22, 2015
42a4c98
Merge branch 'master' into no_default_port
Nov 3, 2015
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
34 changes: 15 additions & 19 deletions eq/fabric/client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,31 +54,27 @@ bool Client::connectServer( co::NodePtr server )
if( server->isConnected( ))
return true;

co::ConnectionDescriptionPtr connDesc;
if( server->getConnectionDescriptions().empty( ))
if( !server->getConnectionDescriptions().empty( ))
return connect( server );

co::ConnectionDescriptionPtr connDesc( new co::ConnectionDescription );
connDesc->port = EQ_DEFAULT_PORT;
Copy link
Member

Choose a reason for hiding this comment

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

No, this should stay. This is for the standalone server, which still runs on the hardcoded port.

Copy link
Author

Choose a reason for hiding this comment

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

I removed it without remembering there was a tool to start a server process.
I've never used the resident server process and I don't think I'll ever use it. For debugging purposes I've always found misleading to have a connection error trying to access the default server (where default means a different port in each machine because the uid may change, so it's not really hardcoded). Since having a resident server is not the primary use case, don't you think it's better to require the server URI explicitly in this case with EQ_SERVER (explicit better than implicit).

const std::string& globalServer = Global::getServer();
const char* envServer = getenv( "EQ_SERVER" );
std::string address = !globalServer.empty() ? globalServer :
envServer ? envServer : "localhost";
if( !connDesc->fromString( address ))
{
connDesc = new co::ConnectionDescription;
connDesc->port = EQ_DEFAULT_PORT;

const std::string& globalServer = Global::getServer();
const char* envServer = getenv( "EQ_SERVER" );
std::string address = !globalServer.empty() ? globalServer :
envServer ? envServer : "localhost";

if( !connDesc->fromString( address ))
LBWARN << "Can't parse server address " << address << std::endl;
LBASSERT( address.empty( ));
LBDEBUG << "Connecting server " << connDesc->toString() << std::endl;

server->addConnectionDescription( connDesc );
LBWARN << "Can't parse server address " << address << std::endl;
return false;
}
LBDEBUG << "Connecting server " << connDesc->toString() << std::endl;
server->addConnectionDescription( connDesc );

if( connect( server ))
return true;

if( connDesc ) // clean up
server->removeConnectionDescription( connDesc );

server->removeConnectionDescription( connDesc );
return false;
}

Expand Down
2 changes: 1 addition & 1 deletion eq/fabric/types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
# define EQ_DEFAULT_PORT (4242)
#else
// #241: Avoid using privilege ports below 1024
# define EQ_DEFAULT_PORT ( (getuid() % 64511) + 1024 )
# define EQ_DEFAULT_PORT (( getuid() % 64511 ) + 1024 )
#endif

namespace eq
Expand Down
2 changes: 1 addition & 1 deletion eq/server/convert11Visitor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class ConvertTo11Visitor : public ServerVisitor
LBINFO << "Adding default server connection for multi-node config"
<< std::endl;
co::ConnectionDescriptionPtr desc = new co::ConnectionDescription;
desc->port = EQ_DEFAULT_PORT;
desc->port = 0; // Let the OS choose the port.
server->addConnectionDescription( desc );
}
return TRAVERSE_CONTINUE;
Expand Down
2 changes: 1 addition & 1 deletion eq/server/loader.y
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -578,7 +578,7 @@ serverConnection: EQTOKEN_CONNECTION
'{' {
connectionDescription = new eq::server::ConnectionDescription;
connectionDescription->setHostname( "" );
connectionDescription->port = EQ_DEFAULT_PORT;
connectionDescription->port = 0; // OS chosen port
Copy link
Member

Choose a reason for hiding this comment

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

Ui, so innocently looking, but still a dragon. This at least should break the admin API for window creation. Investigating.

}
connectionFields '}'
{
Expand Down
4 changes: 2 additions & 2 deletions tools/server/eqServer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,12 @@
* This library is free software; you can redistribute it and/or modify it under
* the terms of the GNU Lesser General Public License version 2.1 as published
* by the Free Software Foundation.
*
*
* This library is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
* FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License for more
* details.
*
*
* You should have received a copy of the GNU Lesser General Public License
* along with this library; if not, write to the Free Software Foundation, Inc.,
* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
Expand Down