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

Services/AM: Add CIA title installation support. #3029

Merged
merged 6 commits into from Nov 6, 2017

Conversation

Projects
None yet
7 participants
@shinyquagsire23
Copy link
Contributor

shinyquagsire23 commented Oct 22, 2017

This PR can be reviewed commit-by-commit.

This PR adds support for am:u and am:net CIA information retrieval and installation commands. It does not add any Qt UI methods for installation, that will probably get handled in its own PR. The CIAContainer class is designed to allow for CIAs to be loaded from a filepath, FileBackend, or buffered CIA data, or for instances where more control is needed, CIAContainer can load portions of data as it is retrieved.

This PR was tested with GodMode9-dumped CIAs and homebrew CIAs using https://github.com/Steveice10/FBI/releases/tag/2.4.2. This can be run by extracting the main CXI from the CIA using ctrtool -x --contents=fbi FBI.cia.

A video demonstration of this PR can be found here.


This change is Reviewable

@valentinvanelslande
Copy link
Contributor

valentinvanelslande left a comment

[ 60%] Building CXX object src/core/CMakeFiles/core.dir/file_sys/title_metadata.cpp.obj
C:/projects/citra/src/core/file_sys/title_metadata.cpp:6:44: fatal error: boost/iostreams/device/array.hpp: No such file or directory
#include <boost/iostreams/device/array.hpp>
^
compilation terminated.

@@ -3,6 +3,7 @@
// Refer to the license.txt file included.

#include <cinttypes>
#include <streambuf>

This comment has been minimized.

@mailwl

mailwl Oct 22, 2017

Contributor

not alphabetical order of include

This comment has been minimized.

@shinyquagsire23

shinyquagsire23 Oct 23, 2017

Contributor

Yeah clang-format really wants it there for some reason so I'm not going to question it.

@ghost

This comment has been minimized.

Copy link

ghost commented Oct 22, 2017

@acnleditor2

Boost 1.65.1 Full Edition, for testing files 2.
https://github.com/Jhon591/boost5

@ghost

This comment has been minimized.

Copy link

ghost commented Oct 22, 2017

edit: did a local compile, compiled fine anyway without the latest boost full.

@Subv
Copy link
Member

Subv left a comment

I didn't finish the review but i really don't like how FSFile interfacing was implemented.

The CIAFile installation code could use some cleanup to make it clearer, right now it's a really hard to understand blob of code


namespace FileSys {

Loader::ResultStatus CIAContainer::LoadFromFileBackend(FileBackend& backend) {

This comment has been minimized.

@Subv

Subv Oct 22, 2017

Member

I don't know how to feel about these LoadFromX functions, we could use function overloading here and have a single Load method with different parameters

return result;

// Load CIA Metadata
std::vector<u8> meta_data(sizeof(Metadata));

This comment has been minimized.

@Subv

Subv Oct 22, 2017

Member

Why does this block not check the cia_header.meta_size variable like in LoadFromFileBackend?

This comment has been minimized.

@shinyquagsire23

shinyquagsire23 Oct 22, 2017

Contributor

Good catch

return Loader::ResultStatus::Success;
}

Loader::ResultStatus CIAContainer::LoadHeader(std::vector<u8>& header_data, u64 offset) {

This comment has been minimized.

@Subv

Subv Oct 22, 2017

Member

the offset parameter of all of these functions should be size_t

return cia_metadata.dependencies;
}

u32 CIAContainer::GetCoreVersion() {

This comment has been minimized.

@Subv

Subv Oct 22, 2017

Member

const method

}

u64 CIAContainer::GetCertificateOffset() const {
return Common::AlignUp(cia_header.header_size, 0x40);

This comment has been minimized.

@Subv

Subv Oct 22, 2017

Member

please define this 0x40 as a named constant somewhere

return;
}

if (server->hle_handler != nullptr) {

This comment has been minimized.

@Subv

Subv Oct 22, 2017

Member

Always try to keep the normal function flow in the least possible indentation level.

}

if (server->hle_handler != nullptr) {
auto file = std::dynamic_pointer_cast<Service::FS::File>(server->hle_handler);

This comment has been minimized.

@Subv

Subv Oct 22, 2017

Member

same as before, this may break horribly if the handle isn't a File handle

std::vector<u8> temp(output_buffer_size);

// Read from the Meta offset + 0x400 for the 0x36C0-large SMDH
auto read_result = file->backend->Read(container.GetMetadataOffset() + 0x400,

This comment has been minimized.

@Subv

Subv Oct 22, 2017

Member

What is this 0x400?

VAddr output_buffer = rp.PopMappedBuffer(&output_buffer_size);
output_buffer_size = std::min(
output_buffer_size,
static_cast<size_t>(0x36C0)); // TODO(shinyquagsire23): is this size defined anywhere?

This comment has been minimized.

@Subv

Subv Oct 22, 2017

Member

This is sizeof(SMDH)

rb.Push(Kernel::ERR_NOT_IMPLEMENTED);
return;
}
}

This comment has been minimized.

@Subv

Subv Oct 22, 2017

Member

add a blank line here

@Subv

This comment has been minimized.

Copy link
Member

Subv commented Oct 22, 2017

Github hid some of my comments because you moved some code around, but they are still valid and should be addressed.

@shinyquagsire23

This comment has been minimized.

Copy link
Contributor

shinyquagsire23 commented Oct 23, 2017

@Subv addressed all of your comments I think, ended up finding a bug in how I got update TMD paths so that's fixed now. I think I need to do some research on the CIA content index because it just occurred to me that CIAs can possibly install less content than is specified in the TMD (ie a single theme CIA, where the TMD will list all themes but the only content it has is the one theme), so that can change things I think. Sorted, turns out that wasn't too hard.

@shinyquagsire23 shinyquagsire23 force-pushed the shinyquagsire23:am-title-install branch 2 times, most recently from 6053db0 to 9fe5e51 Oct 23, 2017

@Subv

This comment has been minimized.

Copy link
Member

Subv commented Oct 23, 2017

Reviewed 1 of 3 files at r1, 2 of 4 files at r4, 1 of 5 files at r11, 5 of 5 files at r12.
Review status: all files reviewed at latest revision, 40 unresolved discussions, some commit checks failed.


src/core/file_sys/cia_container.cpp, line 18 at r1 (raw file):

Previously, Subv (Sebastian Valle) wrote…

I don't know how to feel about these LoadFromX functions, we could use function overloading here and have a single Load method with different parameters

LGTM


src/core/file_sys/cia_container.cpp, line 110 at r1 (raw file):

Previously, Subv (Sebastian Valle) wrote…

the offset parameter of all of these functions should be size_t

LGTM


src/core/file_sys/cia_container.cpp, line 140 at r1 (raw file):

Previously, Subv (Sebastian Valle) wrote…

const method

OK


src/core/file_sys/cia_container.cpp, line 145 at r1 (raw file):

Previously, Subv (Sebastian Valle) wrote…

please define this 0x40 as a named constant somewhere

OK


src/core/file_sys/cia_container.cpp, line 199 at r1 (raw file):

Previously, Subv (Sebastian Valle) wrote…

similarly, all these logs use a 64-bit format specifier, but you're only printing the lower 32 bits (%08)?

ok


src/core/file_sys/cia_container.cpp, line 181 at r12 (raw file):

        // is included in the CIA, so check the bit for this index and add if set.
        // The bits in the content index are arranged w/ index 0 as the MSB, 7 as the LSB, etc.
        if (cia_header.content_present[i >> 3] & 0x80 >> (i & 7))

Can you use Common::BitSet for this?


src/core/file_sys/cia_container.cpp, line 205 at r12 (raw file):

u64 CIAContainer::GetContentSize(u16 index) const {
    // If the content doesn't exist in the CIA, it doesn't have a size.
    if (!(cia_header.content_present[index >> 3] & 0x80 >> (index & 7)))

Can you use Common::BitSet for this?


src/core/file_sys/cia_container.h, line 36 at r12 (raw file):

public:
    // Load whole CIAs outright
    Loader::ResultStatus Load(FileBackend& backend);

Can't these be const parameters?


src/core/file_sys/cia_container.h, line 41 at r12 (raw file):

    // Load parts of CIAs (for CIAs streamed in)
    Loader::ResultStatus LoadHeader(std::vector<u8>& header_data, size_t offset = 0);

The vectors can all be const


src/core/file_sys/cia_container.h, line 45 at r12 (raw file):

    Loader::ResultStatus LoadMetadata(std::vector<u8>& meta_data, size_t offset = 0);

    TitleMetadata& GetTitleMetadata();

This can be const and return a const reference


src/core/file_sys/title_metadata.cpp, line 6 at r8 (raw file):

Previously, shinyquagsire23 (Max Thomas) wrote…

Yeah clang-format really wants it there for some reason so I'm not going to question it.

You don't need this include anymore


src/core/file_sys/title_metadata.cpp, line 79 at r12 (raw file):

    if (file_data.size() - offset < expected_size) {
        LOG_ERROR(Service_FS, "Malformed TMD, expected size 0x%zx, got 0x%zx!", expected_size,
                  file_data.size() - offset);

Does this subtraction return size_t instead of int? You're using the size_t format specifier


src/core/file_sys/title_metadata.h, line 95 at r12 (raw file):

#pragma pack(pop)

    Loader::ResultStatus Load(std::string& file_path);

Make these ref parameters const


src/core/hle/service/am/am.cpp, line 39 at r4 (raw file):

Previously, shinyquagsire23 (Max Thomas) wrote…

I wanted to make them members of CIAFile, only thing stopping that is that FileBackend is NonCopyable, so all member variables are read-only. Kinda annoying having a big hunk of state stuff sitting around though so I'll try and describe things better.

I don't understand the problem here, why does NonCopyable prevent you from assigning member variables?


src/core/hle/service/am/am.cpp, line 86 at r4 (raw file):

Previously, Subv (Sebastian Valle) wrote…

what is this supposed to do? add an ASSERT(false) / UNIMPLEMENTED() here if unimplemented

ok


src/core/hle/service/am/am.cpp, line 89 at r4 (raw file):

Previously, shinyquagsire23 (Max Thomas) wrote…

I can try and do that I suppose, at the very least I suppose I can try and describe the process of things a bit better?

it's much better now, but i still think it should be split in some functions


src/core/hle/service/am/am.cpp, line 95 at r4 (raw file):

Previously, Subv (Sebastian Valle) wrote…

constexpr static

ok


src/core/hle/service/am/am.cpp, line 100 at r4 (raw file):

Previously, Subv (Sebastian Valle) wrote…

use cia_header_size here instead of the magic number

ok


src/core/hle/service/am/am.cpp, line 109 at r4 (raw file):

Previously, Subv (Sebastian Valle) wrote…

break up this line using variables

ok


src/core/hle/service/am/am.cpp, line 111 at r4 (raw file):

Previously, shinyquagsire23 (Max Thomas) wrote…

If offset is greater than the Content offset, then the data in buffer won't contain TMD data which is what's being buffered here.

Edit: Oh, wait I totally misread what you said, yeah let me fix the unneeded copying bit.

ok


src/core/hle/service/am/am.cpp, line 656 at r4 (raw file):

Previously, Subv (Sebastian Valle) wrote…

Please add a comment about what this loop is doing, and use more meaningful variable names than i and j

ok


src/core/hle/service/am/am.cpp, line 674 at r4 (raw file):

Previously, Subv (Sebastian Valle) wrote…

This is not necessary

ok


src/core/hle/service/am/am.cpp, line 699 at r4 (raw file):

Previously, Subv (Sebastian Valle) wrote…

IMO, invert the condition and handle the error in the innermost indentation level, instead of the opposite

ok


src/core/hle/service/am/am.cpp, line 700 at r4 (raw file):

Previously, Subv (Sebastian Valle) wrote…

Keep in mind that checking the result of a dynamic_cast will only work if RTTI is enabled.

ok


src/core/hle/service/am/am.cpp, line 727 at r4 (raw file):

Previously, Subv (Sebastian Valle) wrote…

Add an ASSERT(false) or UNIMPLEMENTED here, along with a log.

ok


src/core/hle/service/am/am.cpp, line 744 at r4 (raw file):

Previously, Subv (Sebastian Valle) wrote…

This is sizeof(SMDH)

ok


src/core/hle/service/am/am.cpp, line 762 at r4 (raw file):

Previously, Subv (Sebastian Valle) wrote…

same as before, this may break horribly if the handle isn't a File handle

ok


src/core/hle/service/am/am.cpp, line 769 at r4 (raw file):

Previously, Subv (Sebastian Valle) wrote…

What is this 0x400?

ok


src/core/hle/service/am/am.cpp, line 105 at r12 (raw file):

        // content sizes so it ends up becoming a problem of keeping track of how  much has been
        // written and what we have been able to pick up.
        if (cia_install_state < HeaderLoaded) {

Please don't use enums like this


src/core/hle/service/am/am.cpp, line 255 at r12 (raw file):

    // For now, just scan for any .tmd files which exist, the smallest will be the
    // base ID and the largest will be the (currently installing) update ID.
    u32 base_id = 0xFFFFFFFF;

use an inline static constexpr named constant for this guard value


src/core/hle/service/am/am.cpp, line 760 at r12 (raw file):

    if (server->hle_handler != nullptr) {
        auto file = std::dynamic_pointer_cast<Service::FS::File>(server->hle_handler);
        if (file != nullptr)

add a comment and a log about this, silently failing is not a good behavior, note that this check will only work with RTTI enabled. Is there really no other way to do this? it may hang up the real system but crashing citra is not an acceptable behavior


src/core/hle/service/am/am.cpp, line 767 at r12 (raw file):

        // Probably the best bet if someone is LLEing the fs service is to just have them LLE AM
        // while they're at it, so not implemented.
        return Kernel::ERR_NOT_IMPLEMENTED;

add a log here


src/core/hle/service/am/am.cpp, line 811 at r12 (raw file):

                       ErrorLevel::Permanent));
    return;
}

add a blank line between each service function


src/core/hle/service/am/am.cpp, line 873 at r12 (raw file):

    FileSys::CIAContainer container;
    if (container.Load(*file->backend) == Loader::ResultStatus::Success) {

Invert the condition so that the error case is higher up and the normal flow is unindented, same everywhere else, i'll stop pointing this out


src/core/hle/service/am/am.cpp, line 885 at r12 (raw file):

                       ErrorLevel::Permanent));
}
void GetTransferSizeFromCia(Service::Interface* self) {

add a blank line, i'll stop pointing this out


Comments from Reviewable


namespace FileSys {

static constexpr u32 CIA_SECTION_ALIGNMENT = 0x40;

This comment has been minimized.

@lioncash

lioncash Oct 23, 2017

Member

Note that const and constexpr at file-scope mean static/internal linkage by default in C++, so you don't need to write static explicitly in these cases.

e.g. In the compatibility section of the standard:

Change: A name of file scope that is explicitly declared const, and not explicitly declared extern, has internal linkage, while in C it would have external linkage
Rationale: Because const objects can be used as compile-time values in C++, this feature urges programmers to provide explicit initializer values for each const. This feature allows the user to put const objects in header files that are included in many compilation units.


namespace FileSys {

static constexpr size_t CIA_CONTENT_MAX_COUNT = 0x10000;

This comment has been minimized.

@lioncash

lioncash Oct 23, 2017

Member

Ditto about statics

#include <vector>
#include "common/common_types.h"
#include "common/swap.h"
#include "core/file_sys/file_backend.h"

This comment has been minimized.

@lioncash

lioncash Oct 23, 2017

Member

A few of these seem like they could be forward declarations.

} // namespace ErrCodes

enum CIAInstallState : u32 {
NotInstalling = 0,

This comment has been minimized.

@lioncash

lioncash Oct 23, 2017

Member

The = 0 is kind of superfluous here.

};
} // namespace ErrCodes

enum CIAInstallState : u32 {

This comment has been minimized.

@lioncash

lioncash Oct 23, 2017

Member

enum class?

// content sizes so it ends up becoming a problem of keeping track of how much has been
// written and what we have been able to pick up.
if (cia_install_state < HeaderLoaded) {
size_t buf_copy_size = std::min(length, static_cast<size_t>(FileSys::CIA_HEADER_SIZE));

This comment has been minimized.

@lioncash

lioncash Oct 23, 2017

Member

<algorithm> and <cstddef> should be included.

size_t buf_copy_size = std::min(length, static_cast<size_t>(FileSys::CIA_HEADER_SIZE));
size_t buf_max_size = std::min(offset + length, FileSys::CIA_HEADER_SIZE);
cia_installing_data.resize(buf_max_size);
memcpy(cia_installing_data.data() + offset, buffer, buf_copy_size);

This comment has been minimized.

@lioncash

lioncash Oct 23, 2017

Member

<cstring> should be included.

return MakeResult<std::shared_ptr<Service::FS::File>>(file);

return Kernel::ERR_INVALID_HANDLE;
} else {

This comment has been minimized.

@lioncash

lioncash Oct 23, 2017

Member

This else block can be removed and the comment can be placed on the return outside of the if-else here. Otherwise the outermost return is effectively dead code.

for (u16 new_index = 0; new_index < new_tmd.GetContentCount(); new_index++) {
if (old_tmd.GetContentIDByIndex(old_index) ==
new_tmd.GetContentIDByIndex(new_index))
abort = true;

This comment has been minimized.

@lioncash

lioncash Oct 23, 2017

Member

conditionals that have the condition portion travel more than one line should be braced.

IPC::RequestBuilder rb = rp.MakeBuilder(1, 0);
rb.Push(ResultCode(ErrCodes::InvalidCIAHeader, ErrorModule::AM, ErrorSummary::InvalidArgument,
ErrorLevel::Permanent));
return;

This comment has been minimized.

@lioncash

lioncash Oct 23, 2017

Member

This return isn't necessary, since it's a void function (and it being top-level and at the end is redundant).

@shinyquagsire23

This comment has been minimized.

Copy link
Contributor

shinyquagsire23 commented Oct 23, 2017

Review status: all files reviewed at latest revision, 49 unresolved discussions, some commit checks failed.


src/core/file_sys/cia_container.cpp, line 18 at r12 (raw file):

Previously, lioncash (Mat M.) wrote…

Note that const and constexpr at file-scope mean static/internal linkage by default in C++, so you don't need to write static explicitly in these cases.

e.g. In the compatibility section of the standard:

Change: A name of file scope that is explicitly declared const, and not explicitly declared extern, has internal linkage, while in C it would have external linkage
Rationale: Because const objects can be used as compile-time values in C++, this feature urges programmers to provide explicit initializer values for each const. This feature allows the user to put const objects in header files that are included in many compilation units.

Good to know, thanks.


src/core/file_sys/cia_container.cpp, line 181 at r12 (raw file):

Previously, Subv (Sebastian Valle) wrote…

Can you use Common::BitSet for this?

Not directly, Common::BitSet only seems to support the least significant bit as index 0 and this uses the most significant bit as index 0, so I'd have to do some weird (7 - i) thing to check indexes.


src/core/file_sys/cia_container.cpp, line 205 at r12 (raw file):

Previously, Subv (Sebastian Valle) wrote…

Can you use Common::BitSet for this?

ditto above


src/core/hle/service/am/am.cpp, line 659 at r4 (raw file):

Previously, Subv (Sebastian Valle) wrote…

What if the content changed but the id did not change?

In this case the old content ID will have been overwritten with new content anyhow, but it shouldn't be deleted if the same content ID is present in the new TMD.


src/core/hle/service/am/am.cpp, line 760 at r12 (raw file):

Previously, Subv (Sebastian Valle) wrote…

add a comment and a log about this, silently failing is not a good behavior, note that this check will only work with RTTI enabled. Is there really no other way to do this? it may hang up the real system but crashing citra is not an acceptable behavior

The only other way I'm seeing is to implement service calls instead, which I guess would sort out LLE issues but I'm not sure what the logistics of doing that would entail. But it's totally an option if we really want to avoid RTTI.


Comments from Reviewable

@Subv

This comment has been minimized.

Copy link
Member

Subv commented Oct 23, 2017

Review status: 3 of 10 files reviewed at latest revision, 47 unresolved discussions.


src/core/file_sys/cia_container.h, line 36 at r12 (raw file):

Previously, Subv (Sebastian Valle) wrote…

Can't these be const parameters?

ok


src/core/file_sys/cia_container.h, line 41 at r12 (raw file):

Previously, Subv (Sebastian Valle) wrote…

The vectors can all be const

ok


src/core/file_sys/cia_container.h, line 45 at r12 (raw file):

Previously, Subv (Sebastian Valle) wrote…

This can be const and return a const reference

ok


src/core/file_sys/title_metadata.cpp, line 61 at r2 (raw file):

Previously, Subv (Sebastian Valle) wrote…

why? just use std::memcpy as necessary

ok


src/core/file_sys/title_metadata.cpp, line 6 at r8 (raw file):

Previously, Subv (Sebastian Valle) wrote…

You don't need this include anymore

ok


src/core/file_sys/title_metadata.cpp, line 79 at r12 (raw file):

Previously, Subv (Sebastian Valle) wrote…

Does this subtraction return size_t instead of int? You're using the size_t format specifier

ok


src/core/file_sys/title_metadata.h, line 95 at r12 (raw file):

Previously, Subv (Sebastian Valle) wrote…

Make these ref parameters const

ok


src/core/hle/service/am/am.cpp, line 80 at r4 (raw file):

Previously, Subv (Sebastian Valle) wrote…

mark this class as final

ok


src/core/hle/service/am/am.cpp, line 613 at r4 (raw file):

Previously, Subv (Sebastian Valle) wrote…

std::make_shared(std::make_unique())

I don't understand what this code is doing anymore, you're constructing a shared_ptr from an unique_ptr?


src/core/hle/service/am/am.cpp, line 703 at r4 (raw file):

Previously, Subv (Sebastian Valle) wrote…

IMO, invert the condition and handle the error in the innermost indentation level, instead of the opposite

ok


src/core/hle/service/am/am.cpp, line 795 at r4 (raw file):

Previously, Subv (Sebastian Valle) wrote…

add a blank line here

ok


src/core/hle/service/am/am.cpp, line 255 at r12 (raw file):

Previously, Subv (Sebastian Valle) wrote…

use an inline static constexpr named constant for this guard value

ok


src/core/hle/service/am/am.cpp, line 767 at r12 (raw file):

Previously, Subv (Sebastian Valle) wrote…

add a log here

ok


src/core/hle/service/am/am.cpp, line 873 at r12 (raw file):

Previously, Subv (Sebastian Valle) wrote…

Invert the condition so that the error case is higher up and the normal flow is unindented, same everywhere else, i'll stop pointing this out

ok


src/core/hle/service/am/am.cpp, line 229 at r13 (raw file):

        if (cia_installing_written >= cia_installing.GetContentOffset() &&
            cia_install_state != TMDLoaded) {
            auto result = WriteTitleMetadata(offset, length, buffer);

what are you using this variable for?


Comments from Reviewable

@shinyquagsire23

This comment has been minimized.

Copy link
Contributor

shinyquagsire23 commented Oct 24, 2017

Review status: 3 of 10 files reviewed at latest revision, 47 unresolved discussions.


src/core/hle/service/am/am.cpp, line 613 at r4 (raw file):

Previously, Subv (Sebastian Valle) wrote…

I don't understand what this code is doing anymore, you're constructing a shared_ptr from an unique_ptr?

OK I think for now I'm going to revert back because I'm not 100% here, I think you might have to spell this one out a bit. My understanding is that file should be a shared_ptr and the CIAFile a unique_ptr here.


src/core/hle/service/am/am.cpp, line 229 at r13 (raw file):

Previously, Subv (Sebastian Valle) wrote…

what are you using this variable for?

Should be returned if failed like the one below.


Comments from Reviewable

@Subv

This comment has been minimized.

Copy link
Member

Subv commented Oct 25, 2017

Reviewed 4 of 7 files at r13, 3 of 3 files at r14.
Review status: all files reviewed at latest revision, 13 unresolved discussions, some commit checks failed.


src/core/hle/service/am/am.cpp, line 39 at r4 (raw file):

Previously, Subv (Sebastian Valle) wrote…

I don't understand the problem here, why does NonCopyable prevent you from assigning member variables?

As discussed on IRC, feel free to make the FileBackend::Write method nonconst and move all these variables into the CIAFile class.


src/core/hle/service/am/am.cpp, line 613 at r4 (raw file):

Previously, shinyquagsire23 (Max Thomas) wrote…

OK I think for now I'm going to revert back because I'm not 100% here, I think you might have to spell this one out a bit. My understanding is that file should be a shared_ptr and the CIAFile a unique_ptr here.

this has to be a shared_ptr, since it has std::enable_shared_from_this in its inheritance graph.
auto file = std::make_shared<Service::FS::File>(std::make_unique<CIAFile>(media_type), cia_path);


Comments from Reviewable

@shinyquagsire23 shinyquagsire23 force-pushed the shinyquagsire23:am-title-install branch 2 times, most recently from 5c4599c to f5671d8 Oct 26, 2017

@shinyquagsire23 shinyquagsire23 force-pushed the shinyquagsire23:am-title-install branch 3 times, most recently from abf6a8f to 13a1e5f Oct 26, 2017

@Subv

This comment has been minimized.

Copy link
Member

Subv commented Oct 28, 2017

Reviewed 12 of 12 files at r15.
Review status: all files reviewed at latest revision, 11 unresolved discussions.


Comments from Reviewable

@Subv

This comment has been minimized.

Copy link
Member

Subv commented Oct 28, 2017

:lgtm:


Review status: all files reviewed at latest revision, 11 unresolved discussions.


Comments from Reviewable

@Subv

Subv approved these changes Oct 28, 2017

@wwylele
Copy link
Member

wwylele left a comment

Haven't finished reviewing am.cpp yet. Will do soon

Common::AlignUp(GetTitleMetadataOffset() + cia_header.tmd_size, CIA_SECTION_ALIGNMENT);

// Meta exists after all content in the CIA
for (u16 i = 0; i < cia_tmd.GetContentCount(); i++) {

This comment has been minimized.

@wwylele

wwylele Nov 1, 2017

Member

Can header.content_size be used to skip the content section?

// The content_present is a bit array which defines which content in the TMD
// is included in the CIA, so check the bit for this index and add if set.
// The bits in the content index are arranged w/ index 0 as the MSB, 7 as the LSB, etc.
if (cia_header.content_present[i >> 3] & 0x80 >> (i & 7))

This comment has been minimized.

@wwylele

wwylele Nov 1, 2017

Member

This bit checking pattern, along with the comment, deserves its own member function in cia_header.

btw, this isn't documented on 3dbrew. Could you also add it to 3dbrew?

u32 tik_size;
u32 tmd_size;
u32 meta_size;
u64 content_size;

This comment has been minimized.

@wwylele

wwylele Nov 1, 2017

Member

All these members above should be marked as _le

struct Metadata {
std::array<u64, 0x30> dependencies;
std::array<u8, 0x180> reserved;
u32 core_version;

This comment has been minimized.

@wwylele

wwylele Nov 1, 2017

Member

u32_le


bool loaded = false;
std::string filepath;
std::unique_ptr<FileBackend> backend;

This comment has been minimized.

@wwylele

wwylele Nov 1, 2017

Member

I can't find any where filepath or backend being used.

if (header_data.size() - offset < sizeof(Header))
return Loader::ResultStatus::Error;

memcpy(&cia_header, header_data.data(), sizeof(Header));

This comment has been minimized.

@wwylele

wwylele Nov 1, 2017

Member

std::memcpy. Applies for all usage below.


// The data in CIAs is always stored CIA Header > Cert > Ticket > TMD > Content > Meta.
// The CIA Header describes Cert, Ticket, TMD, total content sizes, and TMD is needed for
// content sizes so it ends up becoming a problem of keeping track of how much has been

This comment has been minimized.

@wwylele

wwylele Nov 1, 2017

Member

how much
double space

void Flush() const override {}

private:
// Are we installing an update, and what step of installation are we at?

This comment has been minimized.

@wwylele

wwylele Nov 1, 2017

Member

A question in the comment looks more like a "TODO" thing. Better to word it as declarative sentences like // These members indicates whether... and what...


// How much has been written total, CIAContainer for the installing CIA, buffer of all data
// prior to content data, how much of each content index has been written, and where is the
// CIA being installed to?

This comment has been minimized.

@wwylele

wwylele Nov 1, 2017

Member

ditto. Also, better to split the comment for each member, and make it doc strings


Memory::WriteBlock(output_buffer, temp.data(), output_buffer_size);

IPC::RequestBuilder rb = rp.MakeBuilder(1, 0);

This comment has been minimized.

@wwylele

wwylele Nov 1, 2017

Member

The header should be likely MakeBuilder(1, 2), since the request buffer has a mapped buffer, in which case the response buffer usually have an identical PushMappedBuffer to inform the kernel to unmap the buffer. This is not implemented in citra, but better to leave a comment here.

This comment has been minimized.

@shinyquagsire23

shinyquagsire23 Nov 1, 2017

Contributor

Huh, I had to go back and check that but yeah that's consistent. Never knew.

}

size_t output_buffer_size;
VAddr output_buffer = rp.PopStaticBuffer(&output_buffer_size);

This comment has been minimized.

@wwylele

wwylele Nov 1, 2017

Member

Should use PeekStaticBuffer because this is the output buffer descriptor (the "mailbox" descriptor) in TLS+0x180 area (so-called command buffer [64-65]), not the input buffer descriptor (the "mail" descriptor) in TLS+0x80 area (the command buffer)


Memory::WriteBlock(output_buffer, container.GetDependencies().data(), output_buffer_size);

IPC::RequestBuilder rb = rp.MakeBuilder(1, 0);

This comment has been minimized.

@wwylele

wwylele Nov 1, 2017

Member

The header should be likely MakeBuilder(1, 2);, because the response need to push a input buffer descriptor ("a mail") and let the kernel to post it to the output buffer ("the mailbox") set by client. This is unimplemented in citra, and the mail posting is done by the service itself (Memory::WriteBlock), but better to leave a comment here.

return;
}

VAddr output_buffer = rp.PopStaticBuffer();

This comment has been minimized.

@wwylele

wwylele Nov 1, 2017

Member

ditto about PeekStaticBuffer

}

Memory::WriteBlock(output_buffer, temp.data(), output_buffer_size);
IPC::RequestBuilder rb = rp.MakeBuilder(1, 0);

This comment has been minimized.

@wwylele

wwylele Nov 1, 2017

Member

ditto about header with static buffer posting

@wwylele

wwylele approved these changes Nov 2, 2017

@shinyquagsire23 shinyquagsire23 force-pushed the shinyquagsire23:am-title-install branch from 8dc50f2 to e00a45c Nov 5, 2017

@Subv Subv merged commit 8ba2de1 into citra-emu:master Nov 6, 2017

2 of 3 checks passed

code-review/reviewable 3 files, 26 discussions left (lioncash, mailwl, shinyquagsire23, Subv, wwylele)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment