Address review feedback

This commit is contained in:
Billy Laws 2021-03-13 17:34:34 +00:00 committed by ◱ Mark
parent 74cd0bc2c3
commit d8d4b4c9e0
19 changed files with 122 additions and 136 deletions

View File

@ -98,8 +98,8 @@ namespace skyline {
constexpr ResultValue(Result result) : result(result) {};
template<typename G>
constexpr ResultValue(ResultValue<G> result) : result(result) {};
template<typename U>
constexpr ResultValue(ResultValue<U> result) : result(result) {};
constexpr operator Result() const {
return result;

View File

@ -2,7 +2,6 @@
#include "uuid.h"
namespace skyline {
namespace {
union UuidLayout {
struct {
u64 high;
@ -35,7 +34,7 @@ namespace skyline {
u64 node : 48;
struct {
std::array<u8, 6> nodeRaw;
std::array<u8, 6> nodeArray;
};
};
};
@ -45,7 +44,7 @@ namespace skyline {
swappedLayout.timeLow = util::SwapEndianness(timeLow);
swappedLayout.timeMid = util::SwapEndianness(timeMid);
swappedLayout.timeHighAndVersion = util::SwapEndianness(timeHighAndVersion);
swappedLayout.nodeRaw = util::SwapEndianness(nodeRaw);
swappedLayout.nodeArray = util::SwapEndianness(nodeArray);
UUID out;
std::memcpy(&out, &swappedLayout, sizeof(UUID));
@ -53,7 +52,6 @@ namespace skyline {
}
};
static_assert(sizeof(UuidLayout) == 0x10);
}
UUID UUID::GenerateUuidV4() {
constexpr u8 reserved{0x1}; // RFC4122 variant
@ -64,10 +62,13 @@ namespace skyline {
std::uniform_int_distribution<u64> dist;
UuidLayout uuid;
uuid.low = dist(gen);
uuid.high = dist(gen);
// Create an initial random UUID
UuidLayout uuid{
.low = dist(gen),
.high = dist(gen),
};
// Set format bits
uuid.reserved = reserved;
uuid.version = version;

View File

@ -24,7 +24,7 @@ namespace skyline::kernel {
* @param settings An instance of the Settings class
* @param window The ANativeWindow object to draw the screen to
*/
OS(std::shared_ptr<JvmManager> &jvmManager, std::shared_ptr<Logger> &logger, std::shared_ptr<Settings> &settings, std::string appFilesPath, std::string deviceTimeZone, std::shared_ptr<vfs::FileSystem>);
OS(std::shared_ptr<JvmManager> &jvmManager, std::shared_ptr<Logger> &logger, std::shared_ptr<Settings> &settings, std::string appFilesPath, std::string deviceTimeZone, std::shared_ptr<vfs::FileSystem> assetFileSystem);
/**
* @brief Execute a particular ROM file

View File

@ -21,14 +21,12 @@ namespace skyline::service::glue {
return timesrv::result::PermissionDenied;
auto locationName{span(request.Pop<timesrv::LocationName>()).as_string(true)};
// TODO Checked
auto timeZoneBinaryFile{state.os->assetFileSystem->OpenFile(fmt::format("tzdata/zoneinfo/{}", locationName))};
std::vector<u8> timeZoneBinaryBuffer(timeZoneBinaryFile->size);
timeZoneBinaryFile->Read(timeZoneBinaryBuffer);
auto ret{core->SetDeviceLocationNameWithTimeZoneBinary(span(locationName).as_string(true), timeZoneBinaryBuffer)};
if (ret)
return ret;
auto result{core->SetDeviceLocationNameWithTimeZoneBinary(span(locationName).as_string(true), timeZoneBinaryBuffer)};
if (result)
return result;
locationNameUpdateEvent->Signal();
return {};
@ -50,8 +48,6 @@ namespace skyline::service::glue {
Result ITimeZoneService::LoadTimeZoneRule(type::KSession &session, ipc::IpcRequest &request, ipc::IpcResponse &response) {
auto locationName{span(request.Pop<timesrv::LocationName>()).as_string(true)};
// TODO Checked
auto timeZoneBinaryFile{state.os->assetFileSystem->OpenFile(fmt::format("tzdata/zoneinfo/{}", locationName))};
std::vector<u8> timeZoneBinaryBuffer(timeZoneBinaryFile->size);
timeZoneBinaryFile->Read(timeZoneBinaryBuffer);

View File

@ -8,14 +8,12 @@
namespace skyline::service::timesrv {
class ITimeZoneService;
namespace core {
struct TimeServiceObject;
}
}
namespace skyline::service::glue {
/**
* @brief ITimeZoneService is glue's extension of psc::ITimeZoneService, it adds support for reading TimeZone location data and simplifies rule handling. This is the variant normally used by applications.
* @url https://switchbrew.org/wiki/Glue_services#ITimeZoneService

View File

@ -183,7 +183,6 @@ namespace skyline::service::timesrv {
return snapshot;
request.outputBuf.at(0).as<ClockSnapshot>() = *snapshot;
return {};
}

View File

@ -43,11 +43,9 @@ namespace skyline::service::timesrv {
constexpr StaticServicePermissions StaticServiceSystemUpdatePermissions{
.ignoreUninitializedChecks = true,
};
}
class ITimeZoneService;
namespace core {
struct TimeServiceObject;
}

View File

@ -17,7 +17,7 @@ namespace skyline::service::timesrv {
class ISteadyClock : public BaseService {
private:
core::SteadyClockCore &core;
bool writeable;
bool writeable; //!< If this instance can set the test offset
bool ignoreUninitializedChecks;
public:

View File

@ -7,7 +7,7 @@
#include "ISystemClock.h"
namespace skyline::service::timesrv {
ISystemClock::ISystemClock(const DeviceState &state, ServiceManager &manager, core::SystemClockCore &core, bool writeClock, bool ignoreUninitializedChecks) : BaseService(state, manager), core(core), writeClock(writeClock), ignoreUninitializedChecks(ignoreUninitializedChecks) {}
ISystemClock::ISystemClock(const DeviceState &state, ServiceManager &manager, core::SystemClockCore &core, bool writeClock, bool ignoreUninitializedChecks) : BaseService(state, manager), core(core), writable(writeClock), ignoreUninitializedChecks(ignoreUninitializedChecks) {}
Result ISystemClock::GetCurrentTime(type::KSession &session, ipc::IpcRequest &request, ipc::IpcResponse &response) {
if (!ignoreUninitializedChecks && !core.IsClockInitialized())
@ -21,7 +21,7 @@ namespace skyline::service::timesrv {
}
Result ISystemClock::SetCurrentTime(type::KSession &session, ipc::IpcRequest &request, ipc::IpcResponse &response) {
if (!writeClock)
if (!writable)
return result::PermissionDenied;
if (!ignoreUninitializedChecks && !core.IsClockInitialized())
@ -42,7 +42,7 @@ namespace skyline::service::timesrv {
}
Result ISystemClock::SetSystemClockContext(type::KSession &session, ipc::IpcRequest &request, ipc::IpcResponse &response) {
if (!writeClock)
if (!writable)
return result::PermissionDenied;
if (!ignoreUninitializedChecks && !core.IsClockInitialized())
@ -62,5 +62,4 @@ namespace skyline::service::timesrv {
response.copyHandles.push_back(handle);
return {};
}
}

View File

@ -18,7 +18,7 @@ namespace skyline::service::timesrv {
class ISystemClock : public BaseService {
private:
core::SystemClockCore &core;
bool writeClock;
bool writable; //!< If this instance can set the clock time
bool ignoreUninitializedChecks;
std::shared_ptr<kernel::type::KEvent> operationEvent{};

View File

@ -54,7 +54,6 @@ namespace skyline::service::timesrv::core {
std::lock_guard lock(mutex);
auto timePoint{TimeSpanType::FromNanoseconds(util::GetTimeNs()) + rtcOffset};
if (timePoint > cachedValue)
cachedValue = timePoint;
@ -81,16 +80,12 @@ namespace skyline::service::timesrv::core {
}
Result SystemClockCore::UpdateClockContext(const SystemClockContext &newContext) {
auto ret{SetClockContext(newContext)};
if (ret)
return ret;
auto result{SetClockContext(newContext)};
if (result)
return result;
// Writes new state to shared memory etc
if (updateCallback) {
return updateCallback->UpdateContext(newContext);
} else {
return {};
}
return updateCallback ? updateCallback->UpdateContext(newContext) : Result{};
}
Result SystemClockCore::SetCurrentTime(PosixTime posixTimePoint) {
@ -127,15 +122,15 @@ namespace skyline::service::timesrv::core {
void StandardLocalSystemClockCore::Setup(const SystemClockContext &context, PosixTime posixTime) {
auto timePoint{steadyClock.GetCurrentTimePoint()};
Result ret{};
Result result{};
// If the new context comes from the same clock as what we currently have we don't need to set any offset as they share the same base
if (timePoint && timePoint->clockSourceId == context.timestamp.clockSourceId)
ret = UpdateClockContext(context);
result = UpdateClockContext(context);
else
ret = SetCurrentTime(posixTime);
result = SetCurrentTime(posixTime);
if (ret)
if (result)
throw exception("Failed to setup StandardLocalSystemClockCore");
MarkInitialized();
@ -168,9 +163,9 @@ namespace skyline::service::timesrv::core {
if (!ctx)
return ctx;
auto ret{localSystemClock.SetClockContext(*ctx)};
if (ret)
return ret;
auto result{localSystemClock.SetClockContext(*ctx)};
if (result)
return result;
}
automaticCorrectionEnabled = enable;
@ -183,8 +178,8 @@ namespace skyline::service::timesrv::core {
}
Result StandardUserSystemClockCore::UpdateAutomaticCorrectionState(bool enable) {
auto ret{SetAutomaticCorrectionEnabled(enable)};
if (!ret) {
auto result{SetAutomaticCorrectionEnabled(enable)};
if (!result) {
timeSharedMemory.SetStandardUserSystemClockAutomaticCorrectionEnabled(enable);
auto timePoint{steadyClock.GetCurrentTimePoint()};
@ -194,7 +189,7 @@ namespace skyline::service::timesrv::core {
return timePoint;
}
return ret;
return result;
}
void StandardUserSystemClockCore::Setup(bool enableAutomaticCorrection, const SteadyClockTimePoint &automaticCorrectionUpdateTime) {
@ -214,9 +209,9 @@ namespace skyline::service::timesrv::core {
if (!ctx)
return ctx;
auto ret{localSystemClock.SetClockContext(*ctx)};
if (ret)
return ret;
auto result{localSystemClock.SetClockContext(*ctx)};
if (result)
return result;
}
return localSystemClock.GetClockContext();
@ -266,7 +261,6 @@ namespace skyline::service::timesrv::core {
if (!timezoneUpdateTime)
throw exception("Failed to create a timezone updated timepoint!");
// TODO Checked
auto timeZoneBinaryListFile{state.os->assetFileSystem->OpenFile("tzdata/binaryList.txt")};
std::vector<u8> buffer(timeZoneBinaryListFile->size);
timeZoneBinaryListFile->Read(buffer);
@ -284,12 +278,10 @@ namespace skyline::service::timesrv::core {
}
}
// TODO Checked
auto timeZoneBinaryVersionFile{state.os->assetFileSystem->OpenFile("tzdata/version.txt")};
std::array<u8, 0x10> timeZoneBinaryVersion{};
timeZoneBinaryVersionFile->Read(timeZoneBinaryVersion);
timeZoneBinaryVersionFile->ReadUnchecked(timeZoneBinaryVersion);
// TODO Checked
auto timeZoneBinaryFile{state.os->assetFileSystem->OpenFile(fmt::format("tzdata/zoneinfo/{}", state.os->deviceTimeZone))};
buffer.resize(timeZoneBinaryFile->size);
timeZoneBinaryFile->Read(buffer);

View File

@ -228,7 +228,7 @@ namespace skyline::service::timesrv::core {
void Setup(const SystemClockContext &context, TimeSpanType newSufficientAccuracy);
/**
* @brief Checks if the clock accuracy is less than sufficientAccuracy
* @return If the clock accuracy is less than sufficientAccuracy
*/
bool IsAccuracySufficient();
};
@ -282,7 +282,7 @@ namespace skyline::service::timesrv::core {
};
/**
* @brief The ephemeral system clock provides a per-boot timepoint
* @brief The ephemeral network system clock provides a per-boot timepoint
*/
class EphemeralNetworkSystemClockCore : public SystemClockCore {
public:
@ -311,7 +311,7 @@ namespace skyline::service::timesrv::core {
EphemeralNetworkSystemClockCore empheralSystemClock;
TimeZoneManager timeZoneManager;
std::vector<LocationName> locationNameList; //!< N stores in glue but we are fine putting it here
std::vector<LocationName> locationNameList; //!< N stores in glue
TimeManagerServer managerServer;

View File

@ -5,6 +5,8 @@
#include "time_manager_server.h"
namespace skyline::service::timesrv::core {
constexpr size_t TimeSharedMemorySize{0x1000}; //!< The size of the time shared memory region
struct __attribute__((packed)) TimeSharedMemoryLayout {
template<typename T>
struct ClockContextEntry {
@ -24,15 +26,17 @@ namespace skyline::service::timesrv::core {
static_assert(offsetof(TimeSharedMemoryLayout, localSystemClockContextEntry) == 0x38);
static_assert(offsetof(TimeSharedMemoryLayout, networkSystemClockContextEntry) == 0x80);
static_assert(offsetof(TimeSharedMemoryLayout, standardUserSystemClockAutomaticCorrectionEnabledEntry) == 0xC8);
static_assert(sizeof(TimeSharedMemoryLayout) <= TimeSharedMemorySize);
/**
* @brief Time Shared Memory uses a double buffered format that alternates writes context data, this is a helper to simplify that
* @brief Time Shared Memory uses a double buffered format that alternates context data writes, this is a helper to simplify that
*/
template<typename T>
static void UpdateTimeSharedMemoryItem(u32 &updateCount, std::array<T, 2> &item, const T &newValue) {
u32 newCount{updateCount + 1};
item[newCount & 1] = newValue;
asm volatile("DMB ISHST"); // 0xA
// The item value must be updated prior to updateCount to prevent reading in an invalid item value
std::atomic_thread_fence(std::memory_order_release);
updateCount = newCount;
}
@ -47,14 +51,12 @@ namespace skyline::service::timesrv::core {
do {
checkUpdateCount = updateCount;
out = item[updateCount & 1];
asm volatile("DMB ISHLD"); // 0x9
std::atomic_thread_fence(std::memory_order_consume);
} while (checkUpdateCount != updateCount);
return out;
}
constexpr size_t TimeSharedMemorySize{0x1000}; //!< The size of the time shared memory region
TimeSharedMemory::TimeSharedMemory(const DeviceState &state) : kTimeSharedMemory(std::make_shared<kernel::type::KSharedMemory>(state, TimeSharedMemorySize)), timeSharedMemory(reinterpret_cast<TimeSharedMemoryLayout *>(kTimeSharedMemory->kernel.ptr)) {}
void TimeSharedMemory::SetupStandardSteadyClock(UUID rtcId, TimeSpanType baseTimePoint) {
@ -97,14 +99,14 @@ namespace skyline::service::timesrv::core {
void SystemClockContextUpdateCallback::SignalOperationEvent() {
std::lock_guard lock(mutex);
for (const auto &event : operationEventList)
for (const auto &event : operationEvents)
event->Signal();
}
void SystemClockContextUpdateCallback::AddOperationEvent(const std::shared_ptr<kernel::type::KEvent> &event) {
std::lock_guard lock(mutex);
operationEventList.push_back(event);
operationEvents.push_back(event);
}
LocalSystemClockUpdateCallback::LocalSystemClockUpdateCallback(TimeSharedMemory &timeSharedMemory) : timeSharedMemory(timeSharedMemory) {}

View File

@ -46,8 +46,8 @@ namespace skyline::service::timesrv::core {
*/
class SystemClockContextUpdateCallback {
private:
std::list<std::shared_ptr<kernel::type::KEvent>> operationEventList; //!< List of KEvents to be signalled when this callback is called
std::mutex mutex; //!< Protects access to operationEventList
std::list<std::shared_ptr<kernel::type::KEvent>> operationEvents; //!< List of KEvents to be signalled when this callback is called
std::mutex mutex; //!< Synchronises access to operationEvents
std::optional<SystemClockContext> context; //!< The context that used when this callback was last called
protected:
@ -69,7 +69,7 @@ namespace skyline::service::timesrv::core {
void AddOperationEvent(const std::shared_ptr<kernel::type::KEvent> &event);
/**
* @brief Repllaces the current context with the supplied one and signals events if the context differs from the last used one
* @brief Replaces the current context with the supplied one and signals events if the context differs from the last used one
*/
virtual Result UpdateContext(const SystemClockContext &newContext) = 0;
};

View File

@ -111,7 +111,8 @@ namespace skyline::service::timesrv::core {
},
};
std::string_view(posixCalendarTime->tm_zone).copy(out.additionalInfo.timeZoneName.data(), strlen(posixCalendarTime->tm_zone));
std::string_view timeZoneName(posixCalendarTime->tm_zone);
timeZoneName.copy(out.additionalInfo.timeZoneName.data(), timeZoneName.size());
return out;
}

View File

@ -6,25 +6,25 @@
#include "android_asset_backing.h"
namespace skyline::vfs {
AndroidAssetBacking::AndroidAssetBacking(AAsset *backing, Mode mode) : Backing(mode), backing(backing) {
AndroidAssetBacking::AndroidAssetBacking(AAsset *asset, Mode mode) : Backing(mode), asset(asset) {
if (mode.write || mode.append)
throw exception("AndroidAssetBacking doesn't support writing");
size = AAsset_getLength64(backing);
size = AAsset_getLength64(asset);
}
AndroidAssetBacking::~AndroidAssetBacking() {
AAsset_close(backing);
AAsset_close(asset);
}
size_t AndroidAssetBacking::ReadImpl(span<u8> output, size_t offset) {
if (AAsset_seek64(backing, offset, SEEK_SET) != offset)
if (AAsset_seek64(asset, offset, SEEK_SET) != offset)
throw exception("Failed to seek asset position");
auto ret{AAsset_read(backing, output.data(), output.size())};
if (ret < 0)
auto result{AAsset_read(asset, output.data(), output.size())};
if (result < 0)
throw exception("Failed to read from fd: {}", strerror(errno));
return static_cast<size_t>(ret);
return static_cast<size_t>(result);
}
}

View File

@ -5,7 +5,7 @@
#include "backing.h"
struct AAsset;
struct AAsset; //!< https://developer.android.com/ndk/reference/group/asset#aasset
namespace skyline::vfs {
/**
@ -15,13 +15,13 @@ namespace skyline::vfs {
*/
class AndroidAssetBacking : public Backing {
private:
AAsset *backing; //!< The backing AAsset object we abstract
AAsset *asset; //!< The NDK AAsset object we abstract
protected:
size_t ReadImpl(span<u8> output, size_t offset) override;
public:
AndroidAssetBacking(AAsset *backing, Mode mode = {true, false, false});
AndroidAssetBacking(AAsset *asset, Mode mode = {true, false, false});
virtual ~AndroidAssetBacking();
};

View File

@ -6,26 +6,26 @@
#include "android_asset_filesystem.h"
namespace skyline::vfs {
AndroidAssetFileSystem::AndroidAssetFileSystem(AAssetManager *backing) : FileSystem(), backing(backing) {}
AndroidAssetFileSystem::AndroidAssetFileSystem(AAssetManager *assetManager) : FileSystem(), assetManager(assetManager) {}
std::shared_ptr<Backing> AndroidAssetFileSystem::OpenFileImpl(const std::string &path, Backing::Mode mode) {
auto file{AAssetManager_open(backing, path.c_str(), AASSET_MODE_RANDOM)};
auto file{AAssetManager_open(assetManager, path.c_str(), AASSET_MODE_RANDOM)};
if (file == nullptr)
return nullptr;
return std::make_shared<AndroidAssetBacking>(file, mode);
return file ? std::make_shared<AndroidAssetBacking>(file, mode) : nullptr;
}
std::optional<Directory::EntryType> AndroidAssetFileSystem::GetEntryTypeImpl(const std::string &path) {
// Tries to open as a file then as a directory in order to check the type
// This is really hacky but it at least it works
if (AAssetManager_open(backing, path.c_str(), AASSET_MODE_RANDOM) != nullptr)
// This is really hacky but at least it works
if (AAssetManager_open(assetManager, path.c_str(), AASSET_MODE_RANDOM) != nullptr)
return Directory::EntryType::File;
if (AAssetManager_openDir(backing, path.c_str()) != nullptr)
if (AAssetManager_openDir(assetManager, path.c_str()) != nullptr)
return Directory::EntryType::Directory;
// Doesn't exit
// Path doesn't exit
return std::nullopt;
}

View File

@ -13,7 +13,7 @@ namespace skyline::vfs {
*/
class AndroidAssetFileSystem : public FileSystem {
private:
AAssetManager *backing; //!< The backing manager of the filesystem
AAssetManager *assetManager; //!< The NDK asset manager for the filesystem
protected:
std::shared_ptr<Backing> OpenFileImpl(const std::string &path, Backing::Mode mode) override;
@ -23,6 +23,6 @@ namespace skyline::vfs {
std::shared_ptr<Directory> OpenDirectoryImpl(const std::string &path, Directory::ListMode listMode) override;
public:
AndroidAssetFileSystem(AAssetManager *backing);
AndroidAssetFileSystem(AAssetManager *assetManager);
};
}