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

Cleanly report invalid [server] settings (RIPD-1562) #2305

Closed
wants to merge 3 commits into from
Closed

Cleanly report invalid [server] settings (RIPD-1562) #2305

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8e09374
Cleanly report invalid [server] settings (RIPD-1562)
bachase Dec 18, 2017
d1ef7b0
[FOLD] Print exception message
bachase Dec 20, 2017
890a56f
[FOLD] Print exception message
bachase Dec 20, 2017
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
17 changes: 12 additions & 5 deletions src/ripple/app/main/Application.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1227,11 +1227,18 @@ bool ApplicationImp::setup()
}

{
auto setup = setup_ServerHandler(
*config_,
beast::logstream { m_journal.error() });
setup.makeContexts();
serverHandler_->setup (setup, m_journal);
try
{
auto setup = setup_ServerHandler(
*config_, beast::logstream{m_journal.error()});
setup.makeContexts();
serverHandler_->setup(setup, m_journal);
}
catch (std::exception const&)
{
JLOG(m_journal.fatal()) << "Unable to setup server handler";
Copy link
Contributor

@miguelportilla miguelportilla Dec 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It might be useful to log the exception message if one exists. Even if the current code is unlikely to produce one now, it might catch a new message in the future.

Just an idea- If the exception message was logged here, throws in to_Port or populate etc.. could include the message instead of logging and throwing which seems kinda kludgy.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good suggestions. I went with printing the message if one exists to keep a limited scope of this PR.

return false;
}
}

// Begin connecting to network.
Expand Down
16 changes: 8 additions & 8 deletions src/ripple/rpc/impl/ServerHandlerImp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -893,19 +893,19 @@ to_Port(ParsedPort const& parsed, std::ostream& log)

if (! parsed.ip)
{
log << "Missing 'ip' in [" << p.name << "]\n";
log << "Missing 'ip' in [" << p.name << "]";
Throw<std::exception> ();
Copy link
Contributor

Choose a reason for hiding this comment

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

Food for thought: why not do something like:

Throwstd::runtime_exception("Missing 'ip' in [" + p.name + "]");

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@miguelportilla suggested a similar change.

I had gone down that route (along with eliminating the exceptions altogether), but it was starting to touch more code than I was comfortable with for this targeted change. I wasn't sure if some callers wanted to distinguish logging from exception handling.

}
p.ip = *parsed.ip;

if (! parsed.port)
{
log << "Missing 'port' in [" << p.name << "]\n";
log << "Missing 'port' in [" << p.name << "]";
Throw<std::exception> ();
}
else if (*parsed.port == 0)
{
log << "Port " << *parsed.port << "in [" << p.name << "] is invalid\n";
log << "Port " << *parsed.port << "in [" << p.name << "] is invalid";
Throw<std::exception> ();
}
p.port = *parsed.port;
Expand All @@ -916,7 +916,7 @@ to_Port(ParsedPort const& parsed, std::ostream& log)

if (parsed.protocol.empty())
{
log << "Missing 'protocol' in [" << p.name << "]\n";
log << "Missing 'protocol' in [" << p.name << "]";
Throw<std::exception> ();
}
p.protocol = parsed.protocol;
Expand Down Expand Up @@ -947,7 +947,7 @@ parse_Ports (
if (! config.exists("server"))
{
log <<
"Required section [server] is missing\n";
"Required section [server] is missing";
Throw<std::exception> ();
}

Expand All @@ -961,7 +961,7 @@ parse_Ports (
if (! config.exists(name))
{
log <<
"Missing section: [" << name << "]\n";
"Missing section: [" << name << "]";
Throw<std::exception> ();
}
ParsedPort parsed = common;
Expand Down Expand Up @@ -997,12 +997,12 @@ parse_Ports (

if (count > 1)
{
log << "Error: More than one peer protocol configured in [server]\n";
log << "Error: More than one peer protocol configured in [server]";
Throw<std::exception> ();
}

if (count == 0)
log << "Warning: No peer protocol configured\n";
log << "Warning: No peer protocol configured";
}

return result;
Expand Down
16 changes: 8 additions & 8 deletions src/ripple/server/impl/Port.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ populate (Section const& section, std::string const& field, std::ostream& log,
if (! addr.second)
{
log << "Invalid value '" << ip << "' for key '" << field <<
"' in [" << section.name () << "]\n";
"' in [" << section.name () << "]";
Throw<std::exception> ();
}

Expand All @@ -97,7 +97,7 @@ populate (Section const& section, std::string const& field, std::ostream& log,
{
log << "0.0.0.0 not allowed'" <<
"' for key '" << field << "' in [" <<
section.name () << "]\n";
section.name () << "]";
Throw<std::exception> ();
}
else
Expand All @@ -110,7 +110,7 @@ populate (Section const& section, std::string const& field, std::ostream& log,
{
log << "IP specified along with 0.0.0.0 '" << ip <<
"' for key '" << field << "' in [" <<
section.name () << "]\n";
section.name () << "]";
Throw<std::exception> ();
}

Expand All @@ -123,7 +123,7 @@ populate (Section const& section, std::string const& field, std::ostream& log,
) != admin_ip.end())
{
log << "IP specified for " << field << " is also for " <<
"admin: " << ip << " in [" << section.name() << "]\n";
"admin: " << ip << " in [" << section.name() << "]";
Throw<std::exception> ();
}

Expand All @@ -146,7 +146,7 @@ parse_Port (ParsedPort& port, Section const& section, std::ostream& log)
catch (std::exception const&)
{
log << "Invalid value '" << result.first <<
"' for key 'ip' in [" << section.name() << "]\n";
"' for key 'ip' in [" << section.name() << "]";
Rethrow();
}
}
Expand All @@ -169,7 +169,7 @@ parse_Port (ParsedPort& port, Section const& section, std::ostream& log)
{
log <<
"Invalid value '" << result.first << "' for key " <<
"'port' in [" << section.name() << "]\n";
"'port' in [" << section.name() << "]";
Rethrow();
}
}
Expand Down Expand Up @@ -199,7 +199,7 @@ parse_Port (ParsedPort& port, Section const& section, std::ostream& log)
{
log <<
"Invalid value '" << lim << "' for key " <<
"'limit' in [" << section.name() << "]\n";
"'limit' in [" << section.name() << "]";
Rethrow();
}
}
Expand All @@ -222,7 +222,7 @@ parse_Port (ParsedPort& port, Section const& section, std::ostream& log)
{
log <<
"Invalid value '" << result.first << "' for key " <<
"'send_queue_limit' in [" << section.name() << "]\n";
"'send_queue_limit' in [" << section.name() << "]";
Rethrow();
}
}
Expand Down