From 55ec0f5850d76fbedaa628c9024e729852bd7341 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Tue, 28 May 2019 21:07:34 -0400 Subject: [PATCH 1/3] core/telemetry_session: Explicitly delete copy and move constructors NonCopyable is misleading here. It also makes the class non-moveable as well, so we can be explicit about this. --- src/core/telemetry_session.h | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/core/telemetry_session.h b/src/core/telemetry_session.h index b3637e723..61658021b 100644 --- a/src/core/telemetry_session.h +++ b/src/core/telemetry_session.h @@ -15,11 +15,17 @@ namespace Core { * session, logging any one-time fields. Interfaces with the telemetry backend used for submitting * data to the web service. Submits session data on close. */ -class TelemetrySession : NonCopyable { +class TelemetrySession { public: TelemetrySession(); ~TelemetrySession(); + TelemetrySession(const TelemetrySession&) = delete; + TelemetrySession& operator=(const TelemetrySession&) = delete; + + TelemetrySession(TelemetrySession&&) = delete; + TelemetrySession& operator=(TelemetrySession&&) = delete; + /** * Wrapper around the Telemetry::FieldCollection::AddField method. * @param type Type of the field to add. From 0a82b00e356417f840340856b5aade8076e6b2d5 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Tue, 28 May 2019 21:12:23 -0400 Subject: [PATCH 2/3] core/telemetry_session: Remove usages of the global system accessor Makes the dependency explicit in the TelemetrySession's interface instead of making it a hidden dependency. This also revealed a hidden issue with the way the telemetry session was being initialized. It was attempting to retrieve the app loader and log out title-specific information. However, this isn't always guaranteed to be possible. During the initialization phase, everything is being constructed. It doesn't mean an actual title has been selected. This is what the Load() function is for. This potentially results in dead code paths involving the app loader. Instead, we explicitly add this information when we know the app loader instance is available. --- src/core/core.cpp | 2 +- src/core/telemetry_session.cpp | 51 ++++++++++++++++++---------------- src/core/telemetry_session.h | 22 ++++++++++++++- 3 files changed, 49 insertions(+), 26 deletions(-) diff --git a/src/core/core.cpp b/src/core/core.cpp index ef022dd8e..109f937f9 100644 --- a/src/core/core.cpp +++ b/src/core/core.cpp @@ -94,7 +94,6 @@ System::ResultStatus System::SingleStep() { System::ResultStatus System::Load(Frontend::EmuWindow& emu_window, const std::string& filepath) { app_loader = Loader::GetLoader(filepath); - if (!app_loader) { LOG_CRITICAL(Core, "Failed to obtain loader for {}!", filepath); return ResultStatus::ErrorGetLoader; @@ -125,6 +124,7 @@ System::ResultStatus System::Load(Frontend::EmuWindow& emu_window, const std::st return init_result; } + telemetry_session->AddInitialInfo(*app_loader); std::shared_ptr process; const Loader::ResultStatus load_result{app_loader->Load(process)}; kernel->SetCurrentProcess(process); diff --git a/src/core/telemetry_session.cpp b/src/core/telemetry_session.cpp index 3e287f76d..f052d234e 100644 --- a/src/core/telemetry_session.cpp +++ b/src/core/telemetry_session.cpp @@ -91,7 +91,32 @@ bool VerifyLogin(const std::string& username, const std::string& token) { #endif } -TelemetrySession::TelemetrySession() { +TelemetrySession::TelemetrySession() = default; + +TelemetrySession::~TelemetrySession() { + // Log one-time session end information + const s64 shutdown_time{std::chrono::duration_cast( + std::chrono::system_clock::now().time_since_epoch()) + .count()}; + AddField(Telemetry::FieldType::Session, "Shutdown_Time", shutdown_time); + +#ifdef ENABLE_WEB_SERVICE + auto backend = std::make_unique(Settings::values.web_api_url, + Settings::values.citra_username, + Settings::values.citra_token); +#else + auto backend = std::make_unique(); +#endif + + // Complete the session, submitting to web service if necessary + field_collection.Accept(*backend); + if (Settings::values.enable_telemetry) { + backend->Complete(); + } + backend = nullptr; +} + +void TelemetrySession::AddInitialInfo(Loader::AppLoader& app_loader) { // Log one-time top-level information AddField(Telemetry::FieldType::None, "TelemetryId", GetTelemetryId()); @@ -101,7 +126,7 @@ TelemetrySession::TelemetrySession() { .count()}; AddField(Telemetry::FieldType::Session, "Init_Time", init_time); std::string program_name; - const Loader::ResultStatus res{System::GetInstance().GetAppLoader().ReadTitle(program_name)}; + const Loader::ResultStatus res{app_loader.ReadTitle(program_name)}; if (res == Loader::ResultStatus::Success) { AddField(Telemetry::FieldType::Session, "ProgramName", program_name); } @@ -178,28 +203,6 @@ TelemetrySession::TelemetrySession() { AddField(Telemetry::FieldType::UserConfig, "System_RegionValue", Settings::values.region_value); } -TelemetrySession::~TelemetrySession() { - // Log one-time session end information - const s64 shutdown_time{std::chrono::duration_cast( - std::chrono::system_clock::now().time_since_epoch()) - .count()}; - AddField(Telemetry::FieldType::Session, "Shutdown_Time", shutdown_time); - -#ifdef ENABLE_WEB_SERVICE - auto backend = std::make_unique(Settings::values.web_api_url, - Settings::values.citra_username, - Settings::values.citra_token); -#else - auto backend = std::make_unique(); -#endif - - // Complete the session, submitting to web service if necessary - field_collection.Accept(*backend); - if (Settings::values.enable_telemetry) - backend->Complete(); - backend = nullptr; -} - bool TelemetrySession::SubmitTestcase() { #ifdef ENABLE_WEB_SERVICE auto backend = std::make_unique(Settings::values.web_api_url, diff --git a/src/core/telemetry_session.h b/src/core/telemetry_session.h index 61658021b..a5dd57ab1 100644 --- a/src/core/telemetry_session.h +++ b/src/core/telemetry_session.h @@ -8,6 +8,10 @@ #include #include "common/telemetry.h" +namespace Loader { +class AppLoader; +} + namespace Core { /** @@ -17,7 +21,7 @@ namespace Core { */ class TelemetrySession { public: - TelemetrySession(); + explicit TelemetrySession(); ~TelemetrySession(); TelemetrySession(const TelemetrySession&) = delete; @@ -26,6 +30,22 @@ public: TelemetrySession(TelemetrySession&&) = delete; TelemetrySession& operator=(TelemetrySession&&) = delete; + /** + * Adds the initial telemetry info necessary when starting up a title. + * + * This includes information such as: + * - Telemetry ID + * - Initialization time + * - Title ID + * - Title name + * - Title file format + * - Miscellaneous settings values. + * + * @param app_loader The application loader to use to retrieve + * title-specific information. + */ + void AddInitialInfo(Loader::AppLoader& app_loader); + /** * Wrapper around the Telemetry::FieldCollection::AddField method. * @param type Type of the field to add. From aa1b9a18c6042c316a0339c66c8f47492466f137 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Tue, 28 May 2019 21:32:48 -0400 Subject: [PATCH 3/3] core/telemetry_session: Remove unnecessary web service nulling out in destructor This will automatically occur when the backend instance goes out of scope at the end of the destructor's execution. --- src/core/telemetry_session.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/core/telemetry_session.cpp b/src/core/telemetry_session.cpp index f052d234e..6067303a5 100644 --- a/src/core/telemetry_session.cpp +++ b/src/core/telemetry_session.cpp @@ -108,12 +108,11 @@ TelemetrySession::~TelemetrySession() { auto backend = std::make_unique(); #endif - // Complete the session, submitting to web service if necessary + // Complete the session, submitting to the web service backend if necessary field_collection.Accept(*backend); if (Settings::values.enable_telemetry) { backend->Complete(); } - backend = nullptr; } void TelemetrySession::AddInitialInfo(Loader::AppLoader& app_loader) {