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.

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

AM Service and NCCH Archive Rework #2993

Merged
merged 4 commits into from Oct 21, 2017

Conversation

Projects
None yet
6 participants
@shinyquagsire23
Copy link
Contributor

shinyquagsire23 commented Oct 5, 2017

This PR can be reviewed commit-by-commit (somewhat, am.cpp is pushing things but it's mostly rewritten)

  • TitleMetadata has been tweaked to expose needed data for AM
  • Title scanning is performed on AM Init() to gather a std::vector of title IDs per-MediaType, and previously stubbed AM functions will now use this data to return better responses. All of am.cpp now uses ipc_helper functionality for incoming and outgoing data.
  • The NCCH loader will use AM to get update title paths
  • ncch_container now supports ExeFS accesses and specific content accesses, AM is used to get the content path for a title

This PR relies on #2975 and allows mset, Homebrew Launcher to properly get a list of installed titles on NAND and SDMC. Gamecard and NCSD content ID accesses are not currently implemented yet.

Title scanning works by checking sdmc:/Nintendo 3DS/.../.../title/ for pairs of directories which can be decoded to u64s, ie 00040000/12345678 decodes to 0x0004000012345678 and that will get stored into the SDMC MediaType title list. NAND works similarly.

DLC content seems to have a 00000000/ folder which holds all of the .apps, there is some functionality in AM to allow DLC content paths to be properly retrieved, but I'm not sure on the significance of 00000000/ other than the possibility that DLC has of only having a subset of content data installed.


This change is Reviewable

content_id = tmd.GetContentIDByIndex(index);

// TODO(shinyquagsire23): how does DLC actually get this folder on hardware?
if (tmd.GetContentCount() > 1 &&

This comment has been minimized.

@Subv

Subv Oct 5, 2017

Member

What is this code doing?

This comment has been minimized.

@shinyquagsire23

shinyquagsire23 Oct 6, 2017

Contributor

Checks if the second (index 1) content has the optional flag set, that index for most apps is usually the manual and not set optional but for DLC it has the optional flag set and all .apps (including index 0) will be in the 00000000/ folder. Might be worth checking other DLC apps though just in case.

This comment has been minimized.

@Subv

Subv Oct 6, 2017

Member

Can you add this as a comment to the code please?

u32 low = static_cast<u32>(tid & 0xFFFFFFFF);

if (media_type == Service::FS::MediaType::NAND) {
return Common::StringFromFormat("%s%08x/%08x/", GetMediaTitlePath(media_type).c_str(), high,

This comment has been minimized.

@Subv

Subv Oct 5, 2017

Member

If the code is the same for NAND and SDMC, why are they two different branches?

for (const FileUtil::FSTEntry& tidHigh : entries.children) {
for (const FileUtil::FSTEntry& tidLow : tidHigh.children) {
std::string tidString = tidHigh.virtualName + tidLow.virtualName;
u64 tid = std::stoull(tidString.c_str(), NULL, 16);

This comment has been minimized.

@Subv

Subv Oct 5, 2017

Member

nullptr

VAddr content_requested_in = rp.PopMappedBuffer();
VAddr content_info_out = rp.PopMappedBuffer();

std::vector<u16_le> content_requested;

This comment has been minimized.

@Subv

Subv Oct 5, 2017

Member

content_requested(content_count); and then you can remove the resize call

for (size_t i = 0; i < content_count; i++) {
// TODO(shinyquagsire23): Open the NCCH for RomFS size
ContentInfo content_info = {
.index = static_cast<u16>(i),

This comment has been minimized.

@Subv

Subv Oct 5, 2017

Member

Is this valid C++ syntax?

This comment has been minimized.

@lioncash

lioncash Oct 6, 2017

Member

C++ doesn't have designated initializers (they may be introduced in C++20)

This comment has been minimized.

@shinyquagsire23

shinyquagsire23 Oct 6, 2017

Contributor

Huh, I would have thought C++ would have had this like C but I guess not.

}
if (!Memory::IsValidVirtualAddress(title_ids_output_pointer) || media_type > 2) {
IPC::RequestBuilder rb = rp.MakeBuilder(2, 0);
rb.Push((u32)-1); // TODO(shinyquagsire23): Find the right error code

This comment has been minimized.

@Subv

Subv Oct 5, 2017

Member

Push<u32>

if (tmd.Load() == Loader::ResultStatus::Success)
rb.Push(tmd.GetContentCount());
else {
rb.Push((u32)1); // Number of content infos plus one

This comment has been minimized.

@Subv

Subv Oct 5, 2017

Member

Push<u32>

std::string file_path = GetNCCHPath(mount_point, high, low);
u32 content_index = data[1];
u32 type = data[2];
char *exefs_file = reinterpret_cast<char*>(vec.data() + (3 * sizeof(u32)));

This comment has been minimized.

@Subv

Subv Oct 5, 2017

Member

can you use memcpy instead of reinterpret cast here, please?

std::unique_ptr<FileBackend> file;

// NCCH RomFS
if (type == 0x0) {

This comment has been minimized.

@Subv

Subv Oct 5, 2017

Member

Can these be enum values?


ResultVal<std::unique_ptr<ArchiveBackend>> ArchiveFactory_NCCH::Open(const Path& path) {
auto vec = path.AsBinary();
const u32* data = reinterpret_cast<u32*>(vec.data());

This comment has been minimized.

@Subv

Subv Oct 5, 2017

Member

Please use memcpy instead of reinterpret_cast

LOG_DEBUG(Service_FS, "Full Path: %s. Category: 0x%X. Path: 0x%X.", path.DebugStr().c_str(),
high, low);

std::string archive_name = "";

This comment has been minimized.

@lioncash

lioncash Oct 6, 2017

Member

std::strings initialize to empty by default;

auto file = std::make_shared<FileUtil::IOFile>(file_path, "rb");
u32 content_index = data[1];
u32 type = data[2];
char *exefs_file = reinterpret_cast<char*>(vec.data() + (3 * sizeof(u32)));

This comment has been minimized.

@lioncash

lioncash Oct 6, 2017

Member

This can be a const char* as far as I can tell. Also asterisks go against the type.

archive_name = "NG bad word list";
}

if (archive_name != "") {

This comment has been minimized.

@lioncash

lioncash Oct 6, 2017

Member

!archivename.empty()


ResultVal<size_t> NCCHFile::Read(const u64 offset, const size_t length, u8* buffer) const {
LOG_TRACE(Service_FS, "called offset=%llu, length=%zu", offset, length);
size_t read_length = (size_t)std::min((u64)length, data_size - offset);

This comment has been minimized.

@lioncash

lioncash Oct 6, 2017

Member

static_cast

////////////////////////////////////////////////////////////////////////////////////////////////////

ResultVal<size_t> NCCHFile::Read(const u64 offset, const size_t length, u8* buffer) const {
LOG_TRACE(Service_FS, "called offset=%llu, length=%zu", offset, length);

This comment has been minimized.

@lioncash

lioncash Oct 6, 2017

Member

%llu should likely be PRIu64, otherwise this can cause warnings on different platforms.

FileUtil::FSTEntry entries;
FileUtil::ScanDirectoryTree(title_path, entries, true);
for (const FileUtil::FSTEntry& tidHigh : entries.children) {
for (const FileUtil::FSTEntry& tidLow : tidHigh.children) {

This comment has been minimized.

@lioncash

lioncash Oct 6, 2017

Member

ditto

FileUtil::ScanDirectoryTree(title_path, entries, true);
for (const FileUtil::FSTEntry& tidHigh : entries.children) {
for (const FileUtil::FSTEntry& tidLow : tidHigh.children) {
std::string tidString = tidHigh.virtualName + tidLow.virtualName;

This comment has been minimized.

@lioncash

lioncash Oct 6, 2017

Member

ditto

for (size_t i = 0; i < content_count; i++) {
// TODO(shinyquagsire23): Open the NCCH for RomFS size
ContentInfo content_info = {
.index = static_cast<u16>(i),

This comment has been minimized.

@lioncash

lioncash Oct 6, 2017

Member

C++ doesn't have designated initializers (they may be introduced in C++20)

if (!Memory::IsValidVirtualAddress(title_ids_output_pointer) || media_type > 2) {
IPC::RequestBuilder rb = rp.MakeBuilder(2, 0);
rb.Push((u32)-1); // TODO(shinyquagsire23): Find the right error code
rb.Push((u32)0);

This comment has been minimized.

@lioncash

lioncash Oct 6, 2017

Member

0U or static_cast

}
}

return Loader::ResultStatus::ErrorNotUsed;

This comment has been minimized.

@lioncash

lioncash Oct 6, 2017

Member

Should this also set offset and size to default values prior to returning in the error case to prevent potential uninitialized variable usage cases?

i.e.

u64 a, b
ReadOverrideRomFS(ptr, a, b); // Assume this fails
...
// Use of a or b in some form that is uninitialized
if (a == whatever) {
}

This comment has been minimized.

@shinyquagsire23

shinyquagsire23 Oct 6, 2017

Contributor

also #2975, but the offset and size aren't really useful without the file getting set (which only happens if it returns Success)

@einstein95

This comment has been minimized.

Copy link
Contributor

einstein95 commented Oct 6, 2017

Just a note that this PR merge conflicts with #2975 so only one can be merged at a time.

@shinyquagsire23

This comment has been minimized.

Copy link
Contributor

shinyquagsire23 commented Oct 6, 2017

@einstein95 yeah this PR relies on #2975, so I have a squashed commit of that PR's commits until things get merged and I can rebase against master. Things are good now, removed the squashed commit since #2975 got pushed to master so the total diffs are more accurate.

@shinyquagsire23 shinyquagsire23 force-pushed the shinyquagsire23:am-and-archive-ncch-rework branch 2 times, most recently from 1a6f895 to ec1d045 Oct 6, 2017

std::unique_ptr<FileBackend> file;

// NCCH RomFS
if (static_cast<NCCHFilePathType>(openfile_path.filepath_type) == NCCHFilePathType::RomFS) {

This comment has been minimized.

@Subv

Subv Oct 9, 2017

Member

You can perform this cast once and assign it to a variable

/**
* Scans all storage mediums for titles for listing.
*/
void ScanForAlltitles();

This comment has been minimized.

@Subv

Subv Oct 9, 2017

Member

ScanForAllTitles

std::string content_path = GetTitlePath(media_type, tid) + "content/";

if (media_type == Service::FS::MediaType::GameCard) {
// TODO(shinyquagsire23): get current app file if TID matches?

This comment has been minimized.

@Subv

Subv Oct 9, 2017

Member

Maybe log a warning/error here?

if (tmd.GetContentCount() > 1 &&
tmd.GetContentTypeByIndex(1) & FileSys::TMDContentTypeFlag::Optional) {
content_path += "00000000/";
LOG_WARNING(Service_AM, "%s", content_path.c_str());

This comment has been minimized.

@Subv

Subv Oct 9, 2017

Member

what's with this warning?

This comment has been minimized.

@shinyquagsire23

shinyquagsire23 Oct 9, 2017

Contributor

debug print that slipped in >.>

return Common::StringFromFormat("%s%08x/%08x/", GetMediaTitlePath(media_type).c_str(), high,
low);
} else if (media_type == Service::FS::MediaType::GameCard) {
// TODO(shinyquagsire23): get current app path if TID matches?

This comment has been minimized.

@Subv

Subv Oct 9, 2017

Member

log a warning?

void FindContentInfos(Service::Interface* self) {
IPC::RequestParser rp(Kernel::GetCommandBuffer(), 0x1002, 4, 2); // 0x10020104

Service::FS::MediaType media_type = static_cast<Service::FS::MediaType>(rp.Pop<u8>());

This comment has been minimized.

@Subv

Subv Oct 9, 2017

Member

you can do
auto media_type = static_cast<Service::FS::MediaType>(rp.Pop<u8>());

void ListContentInfos(Service::Interface* self) {
IPC::RequestParser rp(Kernel::GetCommandBuffer(), 0x1003, 5, 1); // 0x10030142
u32 content_count = rp.Pop<u32>();
Service::FS::MediaType media_type = static_cast<Service::FS::MediaType>(rp.Pop<u8>());

This comment has been minimized.

@Subv

Subv Oct 9, 2017

Member

same

if (!Memory::IsValidVirtualAddress(title_ids_output_pointer) || media_type > 2) {
IPC::RequestBuilder rb = rp.MakeBuilder(2, 0);
rb.Push(0xFFFFFFFF); // TODO(shinyquagsire23): Find the right error code
rb.Push(0U);

This comment has been minimized.

@Subv

Subv Oct 9, 2017

Member

I'd prefer it if you used rb.Push<u32>

}
if (!Memory::IsValidVirtualAddress(title_ids_output_pointer) || media_type > 2) {
IPC::RequestBuilder rb = rp.MakeBuilder(2, 0);
rb.Push(0xFFFFFFFF); // TODO(shinyquagsire23): Find the right error code

This comment has been minimized.

@Subv

Subv Oct 9, 2017

Member

rb.Push<u32>(-1);

if (tmd.Load() == Loader::ResultStatus::Success)
rb.Push(tmd.GetContentCount());
else {
rb.Push(1U); // Number of content infos plus one

This comment has been minimized.

@Subv

Subv Oct 9, 2017

Member

rb.Push<u32>

@shinyquagsire23 shinyquagsire23 force-pushed the shinyquagsire23:am-and-archive-ncch-rework branch 2 times, most recently from ec3afaf to 341f16d Oct 9, 2017

return "";
}

// The TMD ID is usually held the install databases, which we don't implement.

This comment has been minimized.

@Subv

Subv Oct 12, 2017

Member

The TMD ID is usually held the install databases what does that mean?

This comment has been minimized.

@shinyquagsire23

shinyquagsire23 Oct 12, 2017

Contributor

https://3dbrew.org/wiki/Title_Database, the TMD ID being like, 00000000, 00000001, etc. I think I forgot a word though, should be "in the install databases".

// For now, just scan for any .tmd files which exist and use the first .tmd
// found (there should only really be one unless the directories are meddled with)
FileUtil::FSTEntry entries;
FileUtil::ScanDirectoryTree(content_path, entries, true);

This comment has been minimized.

@Subv

Subv Oct 12, 2017

Member

ScanDirectoryTree doesn't take a boolean third parameter, i think you can remove that to scan only the content_path directory.

This comment has been minimized.

@shinyquagsire23

shinyquagsire23 Oct 12, 2017

Contributor

Ah, I think I was looking at some Dolphin usages and their args are different.

@shinyquagsire23 shinyquagsire23 force-pushed the shinyquagsire23:am-and-archive-ncch-rework branch 2 times, most recently from 04bb316 to d1835e4 Oct 14, 2017

@lioncash
Copy link
Member

lioncash left a comment

Mostly just really minor stylistic stuff, nothing really out of the ordinary to point out (that I could see anyways)

// TODO(shinyquagsire23): get current app parent folder if TID matches?
LOG_ERROR(Service_AM, "Request for gamecard parent path unimplemented!");
return "";
}

This comment has been minimized.

@lioncash

lioncash Oct 17, 2017

Member

There should probably be a return ""; after this closing brace otherwise this can cause warnings about a code path with no return.

FileSys::NCCHContainer ncch_container(GetTitleContentPath(media_type, title_id, i));
ncch_container.ReadRomFS(romfs_file, romfs_offset, romfs_size);

ContentInfo content_info = {0};

This comment has been minimized.

@lioncash

lioncash Oct 17, 2017

Member

the 0 doesn't need to be present in C++ (this is mostly a C-ism).

FileSys::NCCHContainer ncch_container(GetTitleContentPath(media_type, title_id, i));
ncch_container.ReadRomFS(romfs_file, romfs_offset, romfs_size);

ContentInfo content_info = {0};

This comment has been minimized.

@lioncash
for (u32 i = 0; i < title_count; i++) {
std::string tmd_path = GetTitleMetadataPath(media_type, title_id_list[i]);

TitleInfo title_info = {0};

This comment has been minimized.

@lioncash
std::string tmd_path = GetTitleMetadataPath(media_type, title_id);

FileSys::TitleMetadata tmd(tmd_path);
if (tmd.Load() == Loader::ResultStatus::Success)

This comment has been minimized.

@lioncash

lioncash Oct 17, 2017

Member

This top branch should probably be consistently braced like the bottom else case.

@shinyquagsire23 shinyquagsire23 force-pushed the shinyquagsire23:am-and-archive-ncch-rework branch from d0aee94 to 2b42d3e Oct 18, 2017

@shinyquagsire23

This comment has been minimized.

Copy link
Contributor

shinyquagsire23 commented Oct 18, 2017

@lioncash fixed those bits, can squash things if we're good to go

@shinyquagsire23 shinyquagsire23 force-pushed the shinyquagsire23:am-and-archive-ncch-rework branch from 7413d67 to 1ac5137 Oct 19, 2017

@Subv

Subv approved these changes Oct 21, 2017

Copy link
Member

Subv left a comment

LGTM

@lioncash
Copy link
Member

lioncash left a comment

LGTM

@jroweboy jroweboy merged commit dcb4884 into citra-emu:master Oct 21, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Dragios

This comment has been minimized.

Copy link
Contributor

Dragios commented Oct 22, 2017

Your PR introduce 3 new warnings. If you can fix it in upcoming PR, that will be great.

'FileSys::GetSignatureSize': not all control paths return a value
src\core\file_sys\title_metadata.cpp ------------------------------------------------- Line 32 [core]
'argument': conversion from 'size_t' to 'u16', possible loss of data
src\core\hle\service\am\am.cpp -------------------------------------------------------- Line 207 [core]
'Loader::AppLoader_NCCH::ReadUpdateRomFS': not all control paths return a value
src\core\loader\ncch.cpp --------------------------------------------------------------- Line 225 [core]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment