Skip to content

Commit

Permalink
[FOLD] Review feedback from @mellery451:
Browse files Browse the repository at this point in the history 
* Optional return value class members.
* Refactor a few separate functions.
* Simplify some file error checks.
  • Loading branch information
@ximinez
ximinez committed Oct 23, 2019
1 parent a50bb59 commit c57bfce
Show file tree
Hide file tree
Showing 3 changed files with 90 additions and 71 deletions.
15 changes: 13 additions & 2 deletions src/ripple/app/misc/ValidatorList.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -197,9 +197,9 @@ class ValidatorList
}

ListDisposition disposition;
PublicKey publisherKey;
boost::optional<PublicKey> publisherKey;
bool available = false;
std::size_t sequence;
boost::optional<std::size_t> sequence;
};

/** Load configured trusted keys.
Expand Down Expand Up @@ -514,6 +514,17 @@ class ValidatorList


private:
/** Get the filename used for caching UNLs
*/
boost::filesystem::path
GetCacheFileName(PublicKey const& pubKey);

/** Write a JSON UNL to a cache file
*/
void
CacheValidatorFile(PublicKey const& pubKey,
PublisherList const& publisher);

/** Check response for trusted valid published list
@return `ListDisposition::accepted` if list can be applied
Expand Down
131 changes: 68 additions & 63 deletions src/ripple/app/misc/impl/ValidatorList.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,46 @@ ValidatorList::load (
return true;
}

boost::filesystem::path
ValidatorList::GetCacheFileName(PublicKey const& pubKey)
{
return dataPath_ / (filePrefix_ + strHex(pubKey));
}

void
ValidatorList::CacheValidatorFile(PublicKey const& pubKey,
PublisherList const& publisher)
{
if (!dataPath_.empty())
{
boost::filesystem::path const filename =
GetCacheFileName(pubKey);

boost::system::error_code ec;

Json::Value value(Json::objectValue);

value["manifest"] = publisher.rawManifest;
value["blob"] = publisher.rawBlob;
value["signature"] = publisher.rawSignature;
value["version"] = publisher.rawVersion;

writeFileContents(ec, filename, value.toStyledString());

if (ec)
{
// Log and ignore any file I/O exceptions
JLOG(j_.error()) <<
"Problem writing " <<
filename <<
" " <<
ec.value() <<
": " <<
ec.message();
}
}
}

ValidatorList::PublisherListStats
ValidatorList::applyListAndBroadcast(
std::string const& manifest,
Expand All @@ -211,15 +251,15 @@ ValidatorList::applyListAndBroadcast(
HashRouter& hashRouter)
{
auto const result = applyList(manifest, blob, signature,
version, siteUri, hash);
version, std::move(siteUri), hash);
auto const disposition = result.disposition;

bool broadcast = disposition == ListDisposition::accepted ||
disposition == ListDisposition::same_sequence;

if (broadcast)
{
assert(result.available);
assert(result.available && result.publisherKey && result.sequence);
protocol::TMValidatorList msg;
msg.set_manifest(manifest);
msg.set_blob(blob);
Expand All @@ -230,8 +270,8 @@ ValidatorList::applyListAndBroadcast(

if (toSkip)
{
auto const& publisherKey = result.publisherKey;
auto const sequence = result.sequence;
auto const& publisherKey = *result.publisherKey;
auto const sequence = *result.sequence;

auto peercondition =
[&publisherKey, &sequence]
Expand Down Expand Up @@ -410,41 +450,7 @@ ValidatorList::applyList (
}

// Cache the validator list in a file
if (!dataPath_.empty())
{
boost::filesystem::path const filename{
dataPath_ / (filePrefix_ + strHex(pubKey)) };
try
{
boost::system::error_code ec;

Json::Value value(Json::objectValue);

value["manifest"] = publisher.rawManifest;
value["blob"] = publisher.rawBlob;
value["signature"] = publisher.rawSignature;
value["version"] = publisher.rawVersion;

writeFileContents(ec, filename, value.toStyledString());
if (ec)
{
// Log and ignore any file I/O exceptions
JLOG(j_.error()) <<
"Problem writing " <<
filename <<
" " <<
ec.value() <<
": " <<
ec.message();
}
}
catch (std::exception const& e)
{
// Log and ignore any file I/O exceptions
JLOG(j_.error()) << "Exception writing validator list to file " <<
filename << ": " << e.what();
}
}
CacheValidatorFile(pubKey, publisher);

return applyResult;
}
Expand All @@ -467,38 +473,37 @@ ValidatorList::loadLists()
if (publisher.available)
continue;

boost::filesystem::path const filename{
dataPath_ / (filePrefix_ + strHex(pubKey)) };
boost::filesystem::path const filename =
GetCacheFileName(pubKey);

auto const fullPath{ canonical(filename, ec) };
if (!ec)
if (ec)
continue;

auto size = file_size(fullPath, ec);
if (!ec && !size)
{
auto size = file_size(fullPath, ec);
if (!ec && !size)
{
// Treat an empty file as a missing file, because
// nobody else is going to write it.
ec = make_error_code(no_such_file_or_directory);
}
// Treat an empty file as a missing file, because
// nobody else is going to write it.
ec = make_error_code(no_such_file_or_directory);
}
if (ec)
continue;

if (!ec)
{
std::string const prefix = [&fullPath]() {
std::string const prefix = [&fullPath]() {
#if _MSC_VER // MSVC: Windows paths need a leading / added
{
return fullPath.root_path() == "/"s ?
"file://" : "file:///";
}
{
return fullPath.root_path() == "/"s ?
"file://" : "file:///";
}
#else
{
(void)fullPath;
return "file://";
}
{
(void)fullPath;
return "file://";
}
#endif
}();
sites.emplace_back(prefix + fullPath.string());
}
}();
sites.emplace_back(prefix + fullPath.string());
}

// Then let the ValidatorSites do the rest of the work.
Expand Down
15 changes: 9 additions & 6 deletions src/ripple/overlay/impl/PeerImp.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2048,7 +2048,8 @@ PeerImp::onMessage (std::shared_ptr <protocol::TMValidatorList> const& m)
auto const disp = applyResult.disposition;

JLOG(p_journal_.debug()) << "Processed validator list from " <<
strHex(applyResult.publisherKey) << " from " <<
(applyResult.publisherKey ? strHex(*applyResult.publisherKey) :
"unknown or invalid publisher") << " from " <<
remote_address_.to_string() << " (" << id_ << ") with result " <<
to_string(disp);

Expand All @@ -2065,10 +2066,11 @@ PeerImp::onMessage (std::shared_ptr <protocol::TMValidatorList> const& m)

std::lock_guard<std::mutex> sl(recentLock_);

auto const& pubKey = applyResult.publisherKey;
assert(applyResult.sequence && applyResult.publisherKey);
auto const& pubKey = *applyResult.publisherKey;
assert(publisherListSequences_.count(pubKey) == 0 ||
publisherListSequences_[pubKey] < applyResult.sequence);
publisherListSequences_[pubKey] = applyResult.sequence;
publisherListSequences_[pubKey] < *applyResult.sequence);
publisherListSequences_[pubKey] = *applyResult.sequence;

}
break;
Expand All @@ -2083,8 +2085,9 @@ PeerImp::onMessage (std::shared_ptr <protocol::TMValidatorList> const& m)
#ifndef NDEBUG
{
std::lock_guard<std::mutex> sl(recentLock_);
assert(publisherListSequences_[applyResult.publisherKey]
== applyResult.sequence);
assert(applyResult.sequence && applyResult.publisherKey);
assert(publisherListSequences_[*applyResult.publisherKey]
== *applyResult.sequence);
}
#endif // !NDEBUG

Expand Down

0 comments on commit c57bfce

Please sign in to comment.